Closed Bug 1201740 Opened 4 years ago Closed 4 years ago

system XMLHttpRequest should not be intercepted.

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: nsm, Assigned: sajitk, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 1 obsolete file)

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.
    }
Whiteboard: [good first bug][lang=c++]
It sounds like b2g apps are the main concern here, so blocking v2.
I would like to work on this bug can you please assign this bug to me ?
Flags: needinfo?(nsm.nikhil)
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.
Flags: needinfo?(nsm.nikhil)
Attached patch 1201740.patch (obsolete) — Splinter Review
Attachment #8670224 - Flags: review?(nsm.nikhil)
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!
Mentor: nsm.nikhil → bkelly
Depends on: 1210941
Comment on attachment 8670224 [details] [diff] [review]
1201740.patch

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

See comment 5.
Attachment #8670224 - Flags: review?(nsm.nikhil)
Assignee: nobody → sajitk
Status: NEW → ASSIGNED
Attached patch 1201740.patchSplinter Review
Please find attached the change to use the new flag.
Attachment #8670224 - Attachment is obsolete: true
Attachment #8670963 - Flags: review?(bkelly)
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.
Attachment #8670963 - Flags: review?(bkelly) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7c5db9a3a4b1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.