Bug 162520 – Do not follow redirects when sending violation report

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

As per section Reporting of the Content Security Policy Level 2 standard, <https://w3c.github.io/webappsec-csp/2/#violation-reports>, "the user agent MUST NOT follow redirects when [sending the violation report]".

[Daniel Bates]

<rdar://problem/27957639>

[Daniel Bates]

Created attachment 289897 [details]
Work-in-progress Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/PingHandle.h:61:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 57 files


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

[Daniel Bates]

Created attachment 290018 [details]
Patch and Layout Tests

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/PingHandle.h:61:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 66 files


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

[Daniel Bates]

I am not happy with the layout test added in attachment #290018 [details]. In particular, the tests wait 1 second before checking for a CSP/XSS Auditor report as a means of determining if a ping request followed redirects assuming redirects complete within 1 second. One idea to avoid this hardcoded wait is to teach the test runner to wait until all ping requests complete before dumping results (this could be an opt-in setting). Another idea is to implement a callback function that is invoked when we receive a response for the ping. I am open to suggestions.

[Build Bot]

Comment on attachment 290018 [details]
Patch and Layout Tests

Attachment 290018 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2157769

New failing tests:
http/tests/security/xssAuditor/report-script-tag-full-block-and-do-not-follow-redirect-when-sending-report.html
http/tests/security/xssAuditor/report-script-tag-and-do-not-follow-redirect-when-sending-report.html
http/tests/security/contentSecurityPolicy/report-blocked-uri-and-do-not-follow-redirect-when-sending-report.php

[Build Bot]

Created attachment 290025 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Daniel Bates]

Created attachment 290030 [details]
Patch and Layout Tests

Update patch to override ResourceHandleClient::willSendRequestAsync() when sending a ping using asynchronous callbacks.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/network/PingHandle.h:61:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/PingHandle.h:65:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 66 files


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

[Alex Christensen]

Comment on attachment 290030 [details]
Patch and Layout Tests

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

> LayoutTests/http/tests/security/xssAuditor/report-script-tag-and-do-not-follow-redirect-when-sending-report.html:19
> +    // FIXME: Is there are better way to test that a redirect did not occur?
> +    window.setTimeout(navigateToReport, 1000);

You could poll the server asking if it has a report yet.  You wouldn't want to overwhelm it, but you could do it more often than every second, and the test wouldn't be flaky if the server was busy for a whole second.

[youenn fablet]

Comment on attachment 290030 [details]
Patch and Layout Tests

> Source/WebCore/ChangeLog:14
> +        (Editor's Draft, 25 April 2016).

Looking at the spec, it tells to use fetch.
The spec should probably set fetch redirect mode to manual.

I do not have any historic of this, so maybe my questions are not relevant but:
- What is the reason to not use the usual fetch/CachedResourceLoader/ResourceLoader code path?
- Can we unify the two code paths?

If we were to use the regular code path, we would just need to use FetchOptions::Redirect::Manual in the passed ResourceLoaderOptions.

> Source/WebCore/ChangeLog:24
> +        keep our current behavior.

FetchOptions::Redirect::Follow or FetchOptions::Redirect::Manual would be equivalent I guess.

[Daniel Bates]

(In reply to comment #12)
> Comment on attachment 290030 [details]
> Patch and Layout Tests
> 
> > Source/WebCore/ChangeLog:14
> > +        (Editor's Draft, 25 April 2016).
> 
> Looking at the spec, it tells to use fetch.
> The spec should probably set fetch redirect mode to manual.
> 

Can you elaborate? 

> I do not have any historic of this, so maybe my questions are not relevant
> but:
> - What is the reason to not use the usual
> fetch/CachedResourceLoader/ResourceLoader code path?
>

Notice that CachedResourceLoader will use the PingLoader to perform a network load for an image when the request to load the image occurred from an onbeforeunload, onunload, or onpagehide JavaScript handler. When the PingLoader is used to load an image there is no intention to display the fetched image. The ping request for image (which may not even exist) serves as a notification to the server that the page dispatched a DOM beforeunload, unload, or pagehide event.

Following from this and disregarding the underlying response processing to detect a redirect, we do not care about the response data/validation/caching for-, or errors that occur (say, the server cannot be reached) as a result of sending- a ping request. We do not want to cache ping responses (because we do not care about their response) and ping requests have a different lifetime than the lifetime of the page (because they can be started during the DOM unload process of a page).

> - Can we unify the two code paths?
> 

There may be a way to unify the code paths though we would need to work out differences in lifetime among other issues.

> If we were to use the regular code path, we would just need to use
> FetchOptions::Redirect::Manual in the passed ResourceLoaderOptions.
> 
> > Source/WebCore/ChangeLog:24
> > +        keep our current behavior.
> 
> FetchOptions::Redirect::Follow or FetchOptions::Redirect::Manual would be
> equivalent I guess.

We should look to implement ping functionality in terms of the Fetch API as the HTML standard and CSP (and future specs) are using the Fetch API language to describe ping requests. I choose to defer such work as it seemed like a large effort. Although this effort may be accomplished iteratively it seemed straightforward to fix this bug and add additional tests as a step towards this iterative effort. We should ensure that we have reasonable test coverage for pings so as to have confidence that a ping implementation written in terms of the Fetch API is correct or, at least, does not regress expected behavior.

[Daniel Bates]

(In reply to comment #12)
> > Source/WebCore/ChangeLog:24
> > +        keep our current behavior.
> 
> FetchOptions::Redirect::Follow or FetchOptions::Redirect::Manual would be
> equivalent I guess.

I am unclear how these are equivalent from briefly reading the Fetch standard, https://fetch.spec.whatwg.org (15 September 2016), including section "HTTP fetch". Can you please elaborate?

[youenn fablet]

Comment on attachment 290030 [details]
Patch and Layout Tests

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

>>> Source/WebCore/ChangeLog:14
>>> +        (Editor's Draft, 25 April 2016).
>> 
>> Looking at the spec, it tells to use fetch.
>> The spec should probably set fetch redirect mode to manual.
>> 
>> I do not have any historic of this, so maybe my questions are not relevant but:
>> - What is the reason to not use the usual fetch/CachedResourceLoader/ResourceLoader code path?
>> - Can we unify the two code paths?
>> 
>> If we were to use the regular code path, we would just need to use FetchOptions::Redirect::Manual in the passed ResourceLoaderOptions.
> 
> Can you elaborate?

When fetch redirect mode is set to manual, the fetch algorithm will return the 30X response as the fetched response to the client.
It will not try to follow the redirection. The returned 30X response is opacified for security reasons when being exposed to JS.

This check is supported in the regular code path at SubresourceLoader level.

Given the

>>> Source/WebCore/ChangeLog:24
>>> +        keep our current behavior.
>> 
>> FetchOptions::Redirect::Follow or FetchOptions::Redirect::Manual would be equivalent I guess.
> 
> We should look to implement ping functionality in terms of the Fetch API as the HTML standard and CSP (and future specs) are using the Fetch API language to describe ping requests. I choose to defer such work as it seemed like a large effort. Although this effort may be accomplished iteratively it seemed straightforward to fix this bug and add additional tests as a step towards this iterative effort. We should ensure that we have reasonable test coverage for pings so as to have confidence that a ping implementation written in terms of the Fetch API is correct or, at least, does not regress expected behavior.

I think it is fine to have a different code path in that particular case, at least not going through ResourceLoader et al.
The main issue I see with the current ping code is that we are duplicating code to handle things such as Origin, Referrer, upgrades.
It would be better to do that logic in a single place.

[youenn fablet]

I hit the button too fast...

Memory cache should not be an issue, fetch API needs to be able to bypass it (patch not yet landed though).

Ideally, CachedResourceLoader should be the place where we put the code used to finalize the request according fetch options. Ping clients could go through it even if their request would not end-up in the ResourceLoader pipeline.

[Daniel Bates]

(In reply to comment #11)
> Comment on attachment 290030 [details]
> Patch and Layout Tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290030&action=review
> 
> > LayoutTests/http/tests/security/xssAuditor/report-script-tag-and-do-not-follow-redirect-when-sending-report.html:19
> > +    // FIXME: Is there are better way to test that a redirect did not occur?
> > +    window.setTimeout(navigateToReport, 1000);
> 
> You could poll the server asking if it has a report yet.  You wouldn't want
> to overwhelm it, but you could do it more often than every second, and the
> test wouldn't be flaky if the server was busy for a whole second.

Filed bug #162968 to implement a polling approach.

[Daniel Bates]

Comment on attachment 290030 [details]
Patch and Layout Tests

Clearing flags on attachment: 290030

Committed r206809: <http://trac.webkit.org/changeset/206809>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.