Synthesized responses shouldn't be subject to CORS checks

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

4 years ago
<img src="http://cross.origin" crossOrigin="anonymous"> causes nsCORSListenerProxy::CheckRequestApproved to fail if the response doesn't include the Access-Control-Allow-Origin header, for example. If this response is synthesized by a service worker, rather than coming from the network, we should not perform such checks. This is tested by fetch-canvas-tainting.https.html and fails in a trunk build.
Reporter

Updated

4 years ago
Blocks: 1194848
Reporter

Updated

4 years ago
Assignee

Updated

4 years ago
Assignee: nobody → ehsan
Assignee

Updated

4 years ago
Attachment #8649485 - Flags: review?(michal.novotny)
Assignee

Comment 3

4 years ago
Please note that this will be tested with bug 1194848.
Comment on attachment 8649486 [details] [diff] [review]
Part 2: Bypass CORS checks if the response of a channel has been synthesized

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

I don't actually know this well enough to usefully review this.
Attachment #8649486 - Flags: review?(jonas)
Shouldn't a same-origin response get the same treatment? In general it seems to me that the distinction is "readable vs. opaque", not "synthesized vs. loaded from the network".
Assignee

Comment 6

4 years ago
Comment on attachment 8649486 [details] [diff] [review]
Part 2: Bypass CORS checks if the response of a channel has been synthesized

Nikhil, see comment 5 please.
Attachment #8649486 - Flags: review?(nsm.nikhil)
Comment on attachment 8649486 [details] [diff] [review]
Part 2: Bypass CORS checks if the response of a channel has been synthesized

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

Also add a note to

::: dom/security/nsCORSListenerProxy.cpp
@@ +547,5 @@
>      LogBlockedRequest(aRequest, "CORSRequestNotHttp", nullptr);
>      return NS_ERROR_DOM_BAD_URI;
>    }
>  
> +  nsCOMPtr<nsIHttpChannelInternal> internal = do_QueryInterface(aRequest);

Since preflight requests aren't intercepted by SWs, it is ok to return early here and not validate the synthesized response for a correct preflight response (which is done later in this method). Could you add a very important comment to NS_StartCORSPreflight() near the call ForceNoIntercept() cataloging that if we ever change the situation to allow SWs to intercept preflights then the check this patch introduces is not enough. Thanks!
Attachment #8649486 - Flags: review?(nsm.nikhil) → review+
Assignee

Comment 8

4 years ago
(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #7)
> Since preflight requests aren't intercepted by SWs, it is ok to return early
> here and not validate the synthesized response for a correct preflight
> response (which is done later in this method). Could you add a very
> important comment to NS_StartCORSPreflight() near the call
> ForceNoIntercept() cataloging that if we ever change the situation to allow
> SWs to intercept preflights then the check this patch introduces is not
> enough. Thanks!

Good point, will do.  I will also add a MOZ_ASSERT(!mIsPreflight) in the body of this condition for additional safety.
Assignee

Comment 9

4 years ago
ping?
Assignee

Comment 10

4 years ago
Comment on attachment 8649485 [details] [diff] [review]
Part 1: Make it possible to tell whether the response of a channel has been synthesized; r=michal

Patrick, can you please help review this?  Thanks!
Attachment #8649485 - Flags: review?(mcmanus)
Attachment #8649485 - Flags: review?(michal.novotny) → review+
Assignee

Comment 11

4 years ago
Comment on attachment 8649485 [details] [diff] [review]
Part 1: Make it possible to tell whether the response of a channel has been synthesized; r=michal

Thanks for the review!
Attachment #8649485 - Flags: review?(mcmanus)
https://hg.mozilla.org/mozilla-central/rev/83424672d537
https://hg.mozilla.org/mozilla-central/rev/b5988cba3190
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.