We should merge <https://src.chromium.org/viewvc/blink?view=rev&revision=191574>. Ignore paths in CSP matching after redirects When a ResourceRequest has a redirect chain before it, set an ignoreSourcePaths flag that gets passed all the way down to the CSP source matching, which will ignore paths in sources when the resource URL is the result of a redirect.
<rdar://problem/24383215>
*** Bug 97030 has been marked as a duplicate of this bug. ***
Created attachment 276455 [details] Patch and Layout Tests
Will update patch tomorrow to remove test http/tests/security/contentSecurityPolicy/redirect-does-not-match-paths.html from file LayoutTests/TestExpectations.
Created attachment 276481 [details] Patch and Layout Tests Update expected result for LayoutTests/http/tests/security/contentSecurityPolicy/redirect-does-not-match-paths.html and unskip it now that it passes.
Comment on attachment 276481 [details] Patch and Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=276481&action=review I think this patch is good, but it would be better if you were passing enums, rather than booleans (which require you to add comments in a number of places). Please consider switching to enums before landing. r=me. > Source/WebCore/loader/DocumentThreadableLoader.cpp:444 > + return contentSecurityPolicy().allowChildContextFromSource(url, overrideContentSecurityPolicy, redirectResponseReceived); It would be better (clearer) if we passed an enum { honorContentSecurityPolicy, overrideContentSecurityPolicy }, but you do not need to do so as part of this patch. It's just something I noticed while reviewing. > Source/WebCore/loader/FrameLoader.cpp:939 > +bool FrameLoader::checkIfFormActionAllowedByCSP(const URL& url, bool didReceiveRedirectResponse) const Consider passing via the ContentSecurityPolicy::RedirectResponseReceived enum directly. > Source/WebCore/loader/FrameLoader.cpp:944 > + bool overrideContentSecurityPolicy = false; It seems funny to declare this local temporary, but I assume you are doing so to make the function call self-documenting? > Source/WebCore/loader/FrameLoader.cpp:1245 > + policyChecker().checkNavigationPolicy(request, false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState.release(), [this](const ResourceRequest& request, PassRefPtr<FormState>, bool shouldContinue) { Again, passing ContentSecurityPolicy::RedirectResponseReceived::No here would be clearer, but might involve a much larger change that this patch.
Comment on attachment 276481 [details] Patch and Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=276481&action=review >> Source/WebCore/loader/FrameLoader.cpp:939 >> +bool FrameLoader::checkIfFormActionAllowedByCSP(const URL& url, bool didReceiveRedirectResponse) const > > Consider passing via the ContentSecurityPolicy::RedirectResponseReceived enum directly. I chose to use a boolean here because the purpose of the second argument seems clear at the call site, DocumentLoader::willSendRequest(). Notice that at the call site we pass a local variable for the second argument with the same name as the name of the function argument. >> Source/WebCore/loader/FrameLoader.cpp:944 >> + bool overrideContentSecurityPolicy = false; > > It seems funny to declare this local temporary, but I assume you are doing so to make the function call self-documenting? Yes, I was debating between using a local variable or inlining its value. This is one of the few call sites that hardcodes a value for overrideContentSecurityPolicy. It seems sufficient to inline its value and use an inline comment. >> Source/WebCore/loader/FrameLoader.cpp:1245 >> + policyChecker().checkNavigationPolicy(request, false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState.release(), [this](const ResourceRequest& request, PassRefPtr<FormState>, bool shouldContinue) { > > Again, passing ContentSecurityPolicy::RedirectResponseReceived::No here would be clearer, but might involve a much larger change that this patch. I hope you do not mind that I keep it like this for now. I thought about using ContentSecurityPolicy::RedirectResponseReceived::No, but it would require either #include'ing ContentSecurityPolicy.h from PolicyChecker.h or make ContentSecurityPolicy::RedirectResponseReceived a non-class enum so that we can forward declare it in PolicyChecker.h. I was not happy with either of these options though I do not have a strong opinion against the latter.
Committed r199612: <http://trac.webkit.org/changeset/199612>