Bug 153154 – CSP: Ignore paths in CSP matching after redirects

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/24383215>

[Daniel Bates]

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

[Daniel Bates]

Created attachment 276455 [details]
Patch and Layout Tests

[Daniel Bates]

Will update patch tomorrow to remove test http/tests/security/contentSecurityPolicy/redirect-does-not-match-paths.html from file LayoutTests/TestExpectations.

[Daniel Bates]

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.

[Brent Fulgham]

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.

[Daniel Bates]

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.

[Daniel Bates]

Committed r199612: <http://trac.webkit.org/changeset/199612>