Bug 69359 – CSP connect-src directive should block redirects

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Sam Weinig]

As pointed out by Adam in bug 69353, we should be blocking redirect of XHR, EventSource and WebSockets if there is a connect-src directive.

[Sam Weinig]

Actually, I don't think WebSockets support redirects, but the other two do.

[Sam Weinig]

This is made slightly complicated by the fact that we don't get a chance to stop redirects in the same class that started the load.  I am told this is because we don't want to block an XHR on a worker from making progress, so all policy information has to be on the ThreadableLoader itself. 

One way we could do this is to make ThreadableLoader aware of either the whole ContentSecurityPolicy object (making access to it safe from multiple threads). Another is to just have a way to copy the relevant part of the policy into the loader, and have a mechanism for it provide reports.  There may be other ways, but I am currently leaning toward making the ContentSecurityPolicy thread safe.

Adam, your comments are appreciated.

[Adam Barth]

I would just teach DocumentThreadableLoader about ContentSecurityPolicy.  It might be worth keeping the check you have in XMLHttpRequest.cpp so we can throw an exception in the non-redirect case.  In the redirect case, we can just treat it as a network error.

[Adam Barth]

This isn't likely to be fixed for CSP 1.0.

[Daniel Bates]

*** Bug 116067 has been marked as a duplicate of this bug. ***

[Daniel Bates]

As mentioned by Ryosuke Niwa in bug 116067, comment 0, we should merge <https://chromium.googlesource.com/chromium/blink/+/2853f594838e8bf24813482ad02f87853cae4366>.

[Radar WebKit Bug Importer]

<rdar://problem/24383025>

[Daniel Bates]

Created attachment 270764 [details]
Patch and Layout Tests

[Daniel Bates]

Created attachment 270831 [details]
Patch and Layout Tests

Rebased patch following <https://bugs.webkit.org/show_bug.cgi?id=153622>.

[WebKit Commit Bot]

Attachment 270831 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/WorkerThreadableLoader.h:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Looks like an EFL build error:

Last 500 characters of output:
/loader/DocumentThreadableLoader.cpp.o -c ../../Source/WebCore/loader/DocumentThreadableLoader.cpp
../../Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'bool WebCore::DocumentThreadableLoader::isAllowedByContentSecurityPolicy(const WebCore::URL&)':
../../Source/WebCore/loader/DocumentThreadableLoader.cpp:451:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

[Brent Fulgham]

Comment on attachment 270831 [details]
Patch and Layout Tests

View in context: https://bugs.webkit.org/attachment.cgi?id=270831&action=review

r=me, thought you might ping an EFL person to see if they can help figure out why the build is failing.

Overall this looks good, but I had a couple of ideas/suggestions as I looked thing over, which you can choose to ignore if they don't seem useful.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:450
> +    }

For some reason, EFL thinks these new enums do not exist!

> Source/WebCore/loader/DocumentThreadableLoader.h:-42
> -#include <wtf/text/WTFString.h>

Thank you for getting rid of these! :-)

> Source/WebCore/loader/WorkerThreadableLoader.cpp:99
> +    ASSERT(contentSecurityPolicy);

Should we change the API to make these references, if they are never supposed to be nullptr?

> Source/WebCore/workers/AbstractWorker.cpp:46
> +URL AbstractWorker::resolveURL(const String& url, bool shouldBypassMainWorldContentSecurityPolicy, ExceptionCode& ec)

Would it make sense to pass the ContentSecurityPolicyEnforcement enum instead of a boolean for these uses? Maybe we only care about block/dont-block, so we don't need to worry about script versus other blocking scenarios.

> Source/WebCore/workers/Worker.cpp:97
> +    // FIXME: Enforce Content Security Policy child-src directive when shouldBypassMainWorldContentSecurityPolicy is false. See <https://bugs.webkit.org/show_bug.cgi?id=153562>.

Don't we use FIXME(153562) for this kind of thing?

> Source/WebCore/workers/WorkerGlobalScope.cpp:186
> +        // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved.

FIXME(104520)

[Daniel Bates]

(In reply to comment #12)
> Comment on attachment 270831 [details]
> Patch and Layout Tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270831&action=review
> 
> r=me, thought you might ping an EFL person to see if they can help figure
> out why the build is failing.
> 
> Overall this looks good, but I had a couple of ideas/suggestions as I looked
> thing over, which you can choose to ignore if they don't seem useful.
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:450
> > +    }
> 
> For some reason, EFL thinks these new enums do not exist!
> 

[[
Last 500 characters of output:
/loader/DocumentThreadableLoader.cpp.o -c ../../Source/WebCore/loader/DocumentThreadableLoader.cpp
../../Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'bool WebCore::DocumentThreadableLoader::isAllowedByContentSecurityPolicy(const WebCore::URL&)':
../../Source/WebCore/loader/DocumentThreadableLoader.cpp:451:1: error: control reaches end of non-void function [-Werror=return-type]
 }
]]
<https://webkit-queues.webkit.org/results/797253>

EFL is using cc1plus and it does not seem to be able to reason that the switch statement handles all cases.

Will add the following code after the switch statement DocumentThreadableLoader::isAllowedByContentSecurityPolicy() a==.

ASSERT_NOT_REACHED();
return false;


> [...]
> > Source/WebCore/loader/WorkerThreadableLoader.cpp:99
> > +    ASSERT(contentSecurityPolicy);
> 
> Should we change the API to make these references, if they are never
> supposed to be nullptr?
> 

We should look to pass this by references. Ideally, we should do this once we fix up SecurityContent to ensure it returns a reference. I suggest that we defer this task to separate bug/patch since we will need to change many places throughout WebCore.

> > Source/WebCore/workers/AbstractWorker.cpp:46
> > +URL AbstractWorker::resolveURL(const String& url, bool shouldBypassMainWorldContentSecurityPolicy, ExceptionCode& ec)
> 
> Would it make sense to pass the ContentSecurityPolicyEnforcement enum
> instead of a boolean for these uses? Maybe we only care about
> block/dont-block, so we don't need to worry about script versus other
> blocking scenarios.
> 

We only care about block/don't block. So, it is sufficient to use a boolean. The purpose of the enum class ContentSecurityPolicyEnforcement is to specify a particular CSP directive to enforce. It seems weird to make use of ContentSecurityPolicyEnforcement for this block/don't block decision.

> > Source/WebCore/workers/Worker.cpp:97
> > +    // FIXME: Enforce Content Security Policy child-src directive when shouldBypassMainWorldContentSecurityPolicy is false. See <https://bugs.webkit.org/show_bug.cgi?id=153562>.
> 
> Don't we use FIXME(153562) for this kind of thing?
> 

As far as I know we prefer to only prefix with "FIXME:" from <https://webkit.org/code-style-guidelines/#comments-fixme>. On another note, Xcode only recognizes FIXME comments of this form when showing them in the list of functions in a file.

> > Source/WebCore/workers/WorkerGlobalScope.cpp:186
> > +        // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved.
> 
> FIXME(104520)

Ditto.

[Daniel Bates]

Committed r196283: <http://trac.webkit.org/changeset/196283>