Bug 185789 – REGRESSION (r231107): CSP report-only policies are ignored for beacon, worker scripts, importScripts, fetch(), EventSource, and XHR

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Following the move to have NetworkProcess perform CSP checks for DocumentThreadableLoader initiated requests and pings, CSP report-only policies are ignored for all loads performed by DocumentThreadableLoader and PingLoad. This includes loads for beacon, importScripts(), fetch(), EventSource, and XHR.

[Daniel Bates]

I should clarify, that this bug does not represent a regression for beacon because we have never supported sending beacons without NetworkProcess.

[Radar WebKit Bug Importer]

<rdar://problem/40380175>

[Daniel Bates]

Created attachment 340749 [details]
Patch and tests

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:166:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 47 files


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

[youenn fablet]

Comment on attachment 340749 [details]
Patch and tests

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

> Source/WebCore/dom/EventInit.h:36
> +    template<class Decoder> static bool decode(Decoder&, EventInit&);

I believe the recommended way is std::optional<X> decode(Decoder&) nowadays.

> Source/WebCore/dom/SecurityPolicyViolationEvent.h:138
> +    encoder << columnNumber;

I guess NetworkProcess will never be able to give all that information, which only WebProcess probably knows.
Would there be a way for the WebProcess to reconstruct this information?
This makes me wonder whether NetworkProcess should only take the responsibility to trigger the CSP violation or error reporting by WebProcess. WebProcess would be responsible to get as much data as possible and send it to whoever makes sense.

> Source/WebCore/platform/network/ResourceRequestBase.h:166
> +    enum class Requester { Unspecified, Main, XHR, Fetch, Media, ImportScripts };

I am not clear why we need this new requester.
Isn't Requester somehow redundant with FetchOptions::Destination?
Should we try to remove Requester in the future?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:184
> +bool NetworkLoadChecker::isAllowedByContentSecurityPolicy(const ResourceRequest& request)

Nice!

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:194
> +        if ((request.requester() == ResourceRequest::Requester::XHR || request.requester() == ResourceRequest::Requester::Fetch || request.requester() == ResourceRequest::Requester::ImportScripts) && !contentSecurityPolicy()->allowScriptFromSource(request.url(), redirectResponseReceived))

This check is not very clear to me.
Why do we need to check requester?
Ideally, I would think checking FetchOptions::Destination would be good enough.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:382
> +void NetworkLoadChecker::addConsoleMessage(MessageSource messageSource, MessageLevel messageLevel, const String& message, unsigned long)

These are the first 3 methods for which NetworkLoadChecker will do IPC directly.
The model is currently to have NetworkResourceLoader handle the IPC, NetworkLoadChecker doing the meat stuff.
Maybe we could stick with a way to keep IPC handling at NetworkResourceLoader level?
For instance, NetworkResourceLoader could pass a lambda to NetworkLoadChecker.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:105
> +    Ref<NetworkConnectionToWebProcess> m_connection;

This is a bit problematic.
In case of a PingLoad, we will keep NetworkLoadChecker alive for some time and it will keep NetworkConnectionToWebProcess alive even if the WebProcess is gone.

In the future, the plan would be to make keep alive requests (beacon, ping) a regular NetworkResourceLoader with a null NetworkConnectionToWebProcess.
As long as WebProcess and document doing the load are there, the NetworkResourceLoader would have a NetworkConnectionToWebProcess and could send back information to WebProcess.
When the document goes away, NetworkResourceLoader would be detached and be owned elsewhere, by NetworkProcess maybe.

[Daniel Bates]

Created attachment 340757 [details]
Patch and layout tests

Rebase patch following the landing of the patch for bug #185661 and add Windows-specific expected results.

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:166:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 51 files


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

[Daniel Bates]

(In reply to youenn fablet from comment #5)
> Comment on attachment 340749 [details]
> Patch and tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340749&action=review
> 
> > Source/WebCore/dom/EventInit.h:36
> > +    template<class Decoder> static bool decode(Decoder&, EventInit&);
> 
> I believe the recommended way is std::optional<X> decode(Decoder&) nowadays.
> 
> > Source/WebCore/dom/SecurityPolicyViolationEvent.h:138
> > +    encoder << columnNumber;
> 
> I guess NetworkProcess will never be able to give all that information,
> which only WebProcess probably knows.
> Would there be a way for the WebProcess to reconstruct this information?
> This makes me wonder whether NetworkProcess should only take the
> responsibility to trigger the CSP violation or error reporting by
> WebProcess. WebProcess would be responsible to get as much data as possible
> and send it to whoever makes sense.
> 

I don't know.

> > Source/WebCore/platform/network/ResourceRequestBase.h:166
> > +    enum class Requester { Unspecified, Main, XHR, Fetch, Media, ImportScripts };
> 
> I am not clear why we need this new requester.
> Isn't Requester somehow redundant with FetchOptions::Destination?
> Should we try to remove Requester in the future?
> 

Please read ChangeLog and see the request’s initiator, destination, CSP directives, and features table in <https://fetch.spec.whatwg.org/#requests>.

> [...]
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:194
> > +        if ((request.requester() == ResourceRequest::Requester::XHR || request.requester() == ResourceRequest::Requester::Fetch || request.requester() == ResourceRequest::Requester::ImportScripts) && !contentSecurityPolicy()->allowScriptFromSource(request.url(), redirectResponseReceived))
> 
> This check is not very clear to me.
> Why do we need to check requester?
> Ideally, I would think checking FetchOptions::Destination would be good
> enough.
> 

See above remark.

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:382
> > +void NetworkLoadChecker::addConsoleMessage(MessageSource messageSource, MessageLevel messageLevel, const String& message, unsigned long)
> 
> These are the first 3 methods for which NetworkLoadChecker will do IPC
> directly.
> The model is currently to have NetworkResourceLoader handle the IPC,
> NetworkLoadChecker doing the meat stuff.
> Maybe we could stick with a way to keep IPC handling at
> NetworkResourceLoader level?
> For instance, NetworkResourceLoader could pass a lambda to
> NetworkLoadChecker.
> 

I am not happy with design of NetworkLoadChecker and we should think about how best to design such a class and/or whether such a class is necessary. I prefer to do such refactoring, which may include your suggested refactoring, in a separate bug/patch as this patch is large as-is.

[Daniel Bates]

Created attachment 340760 [details]
Patch and layout tests

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:166:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 51 files


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

[EWS Watchlist]

Comment on attachment 340760 [details]
Patch and layout tests

Attachment 340760 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7728017

New failing tests:
http/tests/quicklook/same-origin-xmlhttprequest-allowed.html

[EWS Watchlist]

Created attachment 340765 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Daniel Bates]

(In reply to Build Bot from comment #11)
> Comment on attachment 340760 [details]
> Patch and layout tests
> 
> Attachment 340760 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/7728017
> 
> New failing tests:
> http/tests/quicklook/same-origin-xmlhttprequest-allowed.html

Will update the expected result for this test.

Additional remarks:

This test failure represents a repression caused by the patch for bug #184741. For now, I will just update the results to reflect the logged CSP error and inclusion (now that such logging works from NetworkProcess) of a period at the end of the "Blocked by Content Security Policy" ResourceError description. Filed bug #185807 to address the test regression caused by the patch for bug #184741.

[Daniel Bates]

Created attachment 340805 [details]
Patch and layout tests

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:166:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 52 files


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

[Daniel Bates]

Comment on attachment 340805 [details]
Patch and layout tests

Clearing flags on attachment: 340805

Committed r232032: <https://trac.webkit.org/changeset/232032>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Comment on attachment 340805 [details]
Patch and layout tests

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

> Source/WebCore/platform/network/ResourceRequestBase.h:166
> +    enum class Requester { Unspecified, Main, XHR, Fetch, Media, ImportScripts };

I think we should try to deprecate Requester in favor of options defined by the fetch spec, probably fetch destination in that case.
Hopefully, this importScripts value can be removed in a near future.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:55
> +NetworkLoadChecker::NetworkLoadChecker(NetworkConnectionToWebProcess& connection, uint64_t webPageID, uint64_t webFrameID, ResourceLoadIdentifier loadIdentifier, FetchOptions&& options, PAL::SessionID sessionID, HTTPHeaderMap&& originalRequestHeaders, URL&& url, RefPtr<SecurityOrigin>&& sourceOrigin, PreflightPolicy preflightPolicy, String&& referrer)

We usually take a Ref<T>&& instead of a T&.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:111
> +    ResourceLoadIdentifier m_loadIdentifier;

As I said earlier, taking a Ref<NetworkConnectionToWebProcess> does not seem like a great idea.
NetworkLoadChecker is used for PingLoad that can last longer than a NetworkConnectionToWebProcess.
Also, if the corresponding WebPageID/WebFrameID document is gone, we might not care about sending the CSP violation either.

Maybe a weak pointer would be better suited here if we want to keep having NetworkLoadChecker do some IPC.
But ideally, NetworkLoadChecker should not have to do IPC.

Maybe we should allow passing violation reports as part of the parameters given to ValidationHandler.
That way, it would be PingLoad/NetworkResourceLoader responsibility to send a report if it makes sense to do so.