redirects blocked by CSP can return 30x response to content

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [domsecurity-active] [e10s-multi:+] )

Attachments

(2 attachments)

We currently apply CSP in redirects in the nsCSPService::AsyncOnChannelRedirect() global handler.  This does successfully stop loading a URL that should be blocked, but it does not actually cancel the original channel.  The result is that the client code that created the channel does not get an error code.  Instead that original channel completes with a 30x status code.

So, a fetch() that redirects to a CSP blocked URL does not reject.  It resolves to a Response.  An XHR fires a load event and not an error event.  etc...

The fetch spec suggests that if the response after redirects should be blocked by CSP we should return a NetworkError.  See step 16 in Main Fetch:

https://fetch.spec.whatwg.org/#main-fetch

This is a bit late after redirects, though.  So its a bit ambiguous.  I filed a spec issue to clarify:

https://github.com/whatwg/fetch/issues/485

We have at least once WPT test imported from blink that suggests that they fail these requests:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/service-worker-csp-worker.py#132

I'm trying to get this test working over in bug 1337543 which is how I stumbled on this issue.

Note, that our mochitest here:

https://dxr.mozilla.org/mozilla-central/source/dom/security/test/csp/test_redirects.html

Only tests that we fire the "csp-on-violate-policy" observer topic.  They don't actually test the end result from the APIs.  I locally hacked the xhr redirect case here and verified that we are getting a load event with a 302 response.

I would like to make nsCSPService cancel the channel so that we start rejecting these network requests.
Pass dom/security tests locally.  Lets see what try says:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f191548956cd142381a21ae446f64bb7e1abfb

I'm going to add a fetch() specific test to verify it rejects.
Mike West chimed in on the spec issue and pointed out that NetworkError should be returned.  And it does apply on each redirect.
This test fails on current mozilla-central and passes with the P1 patch.  I wrote this as a mochitest.  Bug 1337543 has a WPT test that also covers this case.  We currently have that WPT disabled, but I am going to enable it shortly after landing this.
Attachment #8835733 - Flags: review?(ckerschb)
Comment on attachment 8835693 [details] [diff] [review]
P1 Make nsCSPService cancel the channel if a redirect is blocked by CSP. r=ckerschb

This patch makes us actually fail network requests that hit a redirect that is blocked by CSP.  As discussed in the spec issue linked above we should be returning failure for these.  Without this patch we end up hitting nsIChannel OnStopRequest() with a success code and the 30x status code.
Attachment #8835693 - Flags: review?(ckerschb)
No longer blocks: 1337543
The test expectations in the WPT service-worker-csp-*.https.html tests will have to be updated when this lands as well.  They check that a blocked redirect fetch() rejects.
Comment on attachment 8835693 [details] [diff] [review]
P1 Make nsCSPService cancel the channel if a redirect is blocked by CSP. r=ckerschb

Review of attachment 8835693 [details] [diff] [review]:
-----------------------------------------------------------------

That all makes sense to me. Thanks for fixing. r=me
Attachment #8835693 - Flags: review?(ckerschb) → review+
Comment on attachment 8835733 [details] [diff] [review]
P2 Add a test that verifies fetch() rejects if a redirect is CSP blocked. r=ckerschb

Review of attachment 8835733 [details] [diff] [review]:
-----------------------------------------------------------------

Test looks great, but I suppose you have to update those WPT tests as well before landing, right?
r=me
Attachment #8835733 - Flags: review?(ckerschb) → review+
Priority: -- → P1
Whiteboard: [domsecurity-active]
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Test looks great, but I suppose you have to update those WPT tests as well
> before landing, right?
> r=me

They are marked expected failure.  I have an r+'d patch from bug 1337543 that makes them expected pass.
Blocks: 1337543

Comment 9

2 years ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d334627447da
P1 Make nsCSPService cancel the channel if a redirect is blocked by CSP. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b99d7a7582
P2 Add a test that verifies fetch() rejects if a redirect is CSP blocked. r=ckerschb

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d334627447da
https://hg.mozilla.org/mozilla-central/rev/45b99d7a7582
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Iteration: --- → 54.2 - Feb 20
Whiteboard: [domsecurity-active] → [domsecurity-active] [e10s-multi:+]
You need to log in before you can comment on or make changes to this bug.