Summary: | Content extensions should block popups | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
Component: | WebCore Misc. | Assignee: | Alex Christensen <achristensen> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | beidson, commit-queue, japhet, sam | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 143507 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Alex Christensen
2015-04-07 14:19:58 PDT
Created attachment 250301 [details]
Patch
Comment on attachment 250301 [details]
Patch
As discussed in person, this prevents the creation of windows in a lot more cases than just "popups".
The check(s) should be moved to everywhere we already consult the existing popup blocker. i.e. calls to allowPopUp()
Created attachment 250310 [details]
Patch
Comment on attachment 250310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250310&action=review > Source/WebCore/page/DOMWindow.cpp:2147 > +#if ENABLE(CONTENT_EXTENSIONS) > + if (firstFrame > + && firstFrame->mainFrame().page() > + && firstFrame->mainFrame().page()->userContentController() > + && firstFrame->mainFrame().document()) { > + ResourceLoadInfo resourceLoadInfo = {URL(ParsedURLString, urlString), firstFrame->mainFrame().document()->url(), ResourceType::Popup}; > + Vector<ContentExtensions::Action> actions = firstFrame->mainFrame().page()->userContentController()->actionsForResourceLoad(resourceLoadInfo); > + for (const ContentExtensions::Action& action : actions) { > + if (action.type() == ContentExtensions::ActionType::BlockLoad) > + return nullptr; > + } > + } > +#endif Do we also need to consult the content extension before popping out to another app (say, from an itms:// link or some other custom scheme link)? I think popping out to another app is very similar to popping out to another window. (In reply to comment #4) > Do we also need to consult the content extension before popping out to > another app (say, from an itms:// link or some other custom scheme link)? Probably. Where should this code be, and where are tests that test something similar? Comment on attachment 250310 [details] Patch Clearing flags on attachment: 250310 Committed r182511: <http://trac.webkit.org/changeset/182511> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 143507 This caused https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r182511%20(3359)/results.html Used Document::completeURL with proper null checks instead of URL(ParsedURLStringTag, const String&) like is done elsewhere in DOMWindow::open Recommitted to http://trac.webkit.org/changeset/182564 |