Bug 143497

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 Flags
Patch
none
Patch none

Description Alex Christensen 2015-04-07 14:19:58 PDT
We should.
Comment 1 Alex Christensen 2015-04-07 14:27:25 PDT
Created attachment 250301 [details]
Patch
Comment 2 Brady Eidson 2015-04-07 14:47:00 PDT
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()
Comment 3 Alex Christensen 2015-04-07 15:45:42 PDT
Created attachment 250310 [details]
Patch
Comment 4 Geoffrey Garen 2015-04-07 16:56:58 PDT
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.
Comment 5 Alex Christensen 2015-04-07 17:03:36 PDT
(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 6 WebKit Commit Bot 2015-04-07 17:39:06 PDT
Comment on attachment 250310 [details]
Patch

Clearing flags on attachment: 250310

Committed r182511: <http://trac.webkit.org/changeset/182511>
Comment 7 WebKit Commit Bot 2015-04-07 17:39:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Commit Bot 2015-04-07 18:47:50 PDT
Re-opened since this is blocked by bug 143507
Comment 10 Alex Christensen 2015-04-08 13:28:18 PDT
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