Bug 182160 - CSP post checks should be done for service worker responses
Summary: CSP post checks should be done for service worker responses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-25 17:00 PST by youenn fablet
Modified: 2018-01-26 09:36 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.72 KB, patch)
2018-01-25 17:02 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.60 MB, application/zip)
2018-01-25 17:51 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.34 MB, application/zip)
2018-01-25 18:20 PST, EWS Watchlist
no flags Details
Patch (5.82 KB, patch)
2018-01-25 19:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (5.58 KB, patch)
2018-01-26 08:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-01-25 17:00:24 PST
CSP post checks should be done for service worker responses
Comment 1 youenn fablet 2018-01-25 17:02:25 PST
Created attachment 332336 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-01-25 17:03:55 PST
<rdar://problem/36889274>
Comment 3 EWS Watchlist 2018-01-25 17:51:55 PST
Comment on attachment 332336 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-referrer-policy.https.html
Comment 4 EWS Watchlist 2018-01-25 17:51:56 PST
Created attachment 332340 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 5 EWS Watchlist 2018-01-25 18:20:52 PST
Comment on attachment 332336 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-referrer-policy.https.html
Comment 6 EWS Watchlist 2018-01-25 18:20:54 PST
Created attachment 332341 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 7 youenn fablet 2018-01-25 19:33:41 PST
Created attachment 332344 [details]
Patch
Comment 8 Daniel Bates 2018-01-25 19:56:28 PST
Comment on attachment 332344 [details]
Patch

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

This patch does more than adds a CSP check. It also adds a mixed content check. We also need to add a nosniff check and the Fetch spec also has a MIME type check. Do you plan to follow up to add the other checks?

> Source/WebCore/loader/SubresourceLoader.cpp:305
> +            || !loader.checkInsecureContent(m_resource->type(), response.url())) {

Please add test(s) for mixed content.
Comment 9 youenn fablet 2018-01-26 08:16:44 PST
(In reply to Daniel Bates from comment #8)
> Comment on attachment 332344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332344&action=review
> 
> This patch does more than adds a CSP check. It also adds a mixed content
> check. We also need to add a nosniff check and the Fetch spec also has a
> MIME type check. Do you plan to follow up to add the other checks?

no sniff and mime type checks are done at the response processing level right now.
We could do some refactoring in the future to better match the spec but this is not needed right now.

> > Source/WebCore/loader/SubresourceLoader.cpp:305
> > +            || !loader.checkInsecureContent(m_resource->type(), response.url())) {
> 
> Please add test(s) for mixed content.

I'll remove the check for now and will investigate potential mixed content issues as a follow-up.
Comment 10 youenn fablet 2018-01-26 08:22:42 PST
Created attachment 332374 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2018-01-26 09:36:52 PST
Comment on attachment 332374 [details]
Patch for landing

Clearing flags on attachment: 332374

Committed r227680: <https://trac.webkit.org/changeset/227680>
Comment 12 WebKit Commit Bot 2018-01-26 09:36:53 PST
All reviewed patches have been landed.  Closing bug.