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.
Actually, I don't think WebSockets support redirects, but the other two do.
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.
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.
This isn't likely to be fixed for CSP 1.0.
*** Bug 116067 has been marked as a duplicate of this bug. ***
As mentioned by Ryosuke Niwa in bug 116067, comment 0, we should merge <https://chromium.googlesource.com/chromium/blink/+/2853f594838e8bf24813482ad02f87853cae4366>.
<rdar://problem/24383025>
Created attachment 270764 [details] Patch and Layout Tests
Created attachment 270831 [details] Patch and Layout Tests Rebased patch following <https://bugs.webkit.org/show_bug.cgi?id=153622>.
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.
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.
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)
(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.
Committed r196283: <http://trac.webkit.org/changeset/196283>