1201740 - system XMLHttpRequest should not be intercepted.

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

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

On b2g, apps with the right permissions can create new XMLHttpRequest({mozSystem: true}) which lifts a lot of restrictions like CORS checks and certain header checks. fetch() has no such mode and there is no code in our fetch implementation of network stack right now to propagate this information through. For example, if a mozSystem XHR sets certain invalid headers, when the SW intercepts it, the Request object sent to the SW won't have these headers since they aren't allowed by Request. I think we should disable interception for system XHR. CORS requests will also likely not be handled properly.

We don't have to worry about the case where chrome (system principal) creates XHR since we don't allow registering SWs for chrome at all at present.

Jonas and Ehsan have agreed to this in principal.

This is a very simple and great first bug. In dom/base/nsXMLHttpRequest.cpp, right after we create a new channel with NS_NewChannel, you want to add a check for

    if (IsSystemXHR()) {
      downcast mChannel to an nsIHttpChannelInternal and call ForceNoIntercept() on it.
    }

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

It sounds like b2g apps are the main concern here, so blocking v2.

Comment 2

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

I would like to work on this bug can you please assign this bug to me ?

Comment 3

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

https://wiki.mozilla.org/Contribute/Coding/Mentoring#Good_First_Bugs
"Somebody who'd like to be assigned a good-first-bug should have their development environment spun up and a first attempt at a patch submitted for review before they can be properly assigned the bug."

Please have a preliminary patch ready and then I'll change the assignee.

Comment 4

[sajitk]

Attachment: 1201740.patch

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry, but I just pushed a change that removes ForceNoIntercept() with a new load flag.  You can now specify nsIChannel::LOAD_BYPASS_SERVICE_WORKER on a channel to get the same effect as ForceNoIntercept().

So here you will need to make the check for system XHR before the NS_NewChannel() call and include that load flag if necessary.

Also, I'll take care of the review here since Nikhil is no longer working on the project.

Thanks for your help!

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8670224 [details] [diff] [review]
1201740.patch

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

See comment 5.

Comment 7

[sajitk]

Attachment: 1201740.patch

Please find attached the change to use the new flag.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8670963 [details] [diff] [review]
1201740.patch

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

Looks good.  Can you just correct the one nit and do a try build?  Thanks!

::: dom/base/nsXMLHttpRequest.cpp
@@ +1726,5 @@
>      // principal.  Hence we set the sandbox flag in loadinfo, so that 
>      // GetChannelResultPrincipal will give us the nullprincipal.
>      secFlags |= nsILoadInfo::SEC_SANDBOXED;
> +
> +    //For a XHR, disable interception

nit: I think you can lose this comment.  It should be "for a system XHR", but I think thats pertty clear from the code here.

Comment 9

[sajitk]

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

Comment 10

[sajitk]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5db9a3a4b1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c5db9a3a4b1