1194847 - Synthesized responses shouldn't be subject to CORS checks

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Josh Matthews [:jdm]]

<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.

Comment 1

[(no longer active)]

Attachment: Part 1: Make it possible to tell whether the response of a channel has been synthesized; r=michal

Comment 2

[(no longer active)]

Attachment: Part 2: Bypass CORS checks if the response of a channel has been synthesized

Comment 3

[(no longer active)]

Please note that this will be tested with bug 1194848.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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".

Comment 6

[(no longer active)]

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.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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!

Comment 8

[(no longer active)]

(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.

Comment 9

[(no longer active)]

ping?

Comment 10

[(no longer active)]

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!

Comment 11

[(no longer active)]

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!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83424672d537
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5988cba3190

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83424672d537
https://hg.mozilla.org/mozilla-central/rev/b5988cba3190