Replay GET XHR with CORS are replaying only the OPTIONS part of the request

RESOLVED FIXED in Firefox 51

Status

defect
P1
major
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: jeremie.gourovitch, Assigned: jsnajdr, NeedInfo)

Tracking

43 Branch
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47- wontfix, firefox48+ wontfix, firefox49+ wontfix, firefox50 wontfix, firefox51 fixed)

Details

(Whiteboard: [necko-backlog] btpp-active)

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

- Execute some crossdomain and GET XHR (it will execute an OPTION request followed by a GET request) 
- Go to the debugger and try to replay the same XHR 
 


Actual results:

The OPTION XHR actually returns 200, so that's pretty good, i'm allowed to do some great stuffs.. but, the GET request NEVER launch... 



Expected results:

The options returns a 200, then the GET will be launched and executed as the first GET xhr was....
(Reporter)

Updated

3 years ago
Severity: normal → major

Updated

3 years ago
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Whiteboard: [necko-backlog]
(Reporter)

Comment 1

3 years ago
So, i've inspected further more, and this wild bug appears from version 42.

Comment 2

3 years ago
Could you provide a live testcase showing easily the issue?
Flags: needinfo?(jeremie.gourovitch)
(Reporter)

Comment 3

3 years ago
Go to https://app.expernova.com, then open the network debbuger and try to replay the GET XHR https://api.expernova.com/presets  then only the OPTIONS is re-triggered..

Comment 4

3 years ago
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3de175bd4e83775e35077d71167967fa457d0e0a&tochange=10343dcbe03d370fd57a261c6833c9928956bb33

Probably:
Michael Layzell — Bug 1200337 - Part 1: Don't expose standard HTTP headers during interception in non-e10s mode, r=mcmanus

or 
Michael Layzell — Bug 1188932 - Allow the User-Agent header to be explicitly set by requests, r=bkelly, r=jgraham
Blocks: 1200337
Status: UNCONFIRMED → NEW
Component: Networking: HTTP → DOM
Ever confirmed: true
Flags: needinfo?(jeremie.gourovitch) → needinfo?(michael)
Keywords: regression
Version: 46 Branch → 43 Branch
I believe that this patch should fix the problem, and from my testing it appears to. Unfortunately, I don't know how to test interactions with the devtools UI, so I'm going to need some help to write the tests for this patch. vporof, AFAICT you reviewed the original patch for adding replay support, so I figure you might know how to write these tests :).
Attachment #8754506 - Flags: feedback?(vporof)
Flags: needinfo?(michael)
Assignee: nobody → michael
Whiteboard: [necko-backlog] → [necko-backlog] btpp-active
(Assignee)

Comment 6

3 years ago
Michael, why exactly is the GET request not send and why does setting only the custom headers help?

When I try to test this without the patch, the only difference between the first and second OPTIONS request is that the Access-Control-Request-Headers header has an additional "user-agent" field in the second one.

I'll help you writing the test. It will be based on the existing browser_net_resend.js one.

There's another CORS-related bug in Network Monitor, bug 1214752. It should get fixed and landed before this one, otherwise we'll have to distinguish between e10s and non-e10s cases in the test.
Depends on: 1214752
(In reply to Jarda Snajdr [:jsnajdr] from comment #6)
> Michael, why exactly is the GET request not send and why does setting only
> the custom headers help?
> 
> When I try to test this without the patch, the only difference between the
> first and second OPTIONS request is that the Access-Control-Request-Headers
> header has an additional "user-agent" field in the second one.

Yup, that's the difference. The user-agent field is not a "standard" one, so if it is set by content explicitly, then a cors preflight is required to confirm that the request is approved. This preflight will have the extra "user-agent" header included, meaning that the server may reject it due to not listing user-agent in the list of approved headers. 

This patch avoids setting the user-agent field explicitly when replaying the request if it was not set explicitly in the first request, meaning that the preflight won't request the user-agent field.

> I'll help you writing the test. It will be based on the existing
> browser_net_resend.js one.
> 
> There's another CORS-related bug in Network Monitor, bug 1214752. It should
> get fixed and landed before this one, otherwise we'll have to distinguish
> between e10s and non-e10s cases in the test.

Sounds good.
Attachment #8754506 - Flags: feedback?(vporof)
(Assignee)

Comment 8

3 years ago
This is a test for the "Replay CORS" feature. It adds two things:

1. A "browser_net_resend_cors.js" testcase that sends a CORS POST request and then resends it without any change. Checks that an OPTIONS+POST combo is displayed in netmonitor two times.

2. In existing "browser_net_resend.js" case, I edit the "User-Agent" header of the replayed request and check if the changed header was really sent. Note that you need to run this test with --disable-e10s, because it doesn't work in e10s because of bug 1091612.

This patch is intended to be applied on top of the test from bug 1214752.

Running the tests reveals several problems with your patch:

1. When I resend the POST request in "browser_net_resend_cors.js", the headers "Cache-Control" and "Pragma", although added by the browser, are visited by visitNonDefaultRequestHeaders, detected as custom, and added to the "Access-Control-Request-Headers". That makes the replayed request different from the original one.

2. After I edit the headers in the netmonitor UI, they are parsed and set at [1]. At this moment, all the "custom" attributes are lost.

3. When I edit the "User-Agent" header in the netmonitor UI, I expect the new value to be sent in the replayed request. However, this header would be detected as non-custom and ignored when setting the replayed request headers.

Maybe the right solution is to detect which headers were edited by the users and which were left intact, and call setRequestHeader only on those that were explicitly edited. Setting them all blindly, even if the value is not changed, seems to add them to "Access-Control-Request-Headers" as an unwanted side-effect.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-view.js#2644
This is not a recent regression and given that there is no fix in the works, I think it is too late for Fx47.
Tracking for 48/49, seems likely we can fix this regression at least in 49.
This seems like it will require some changes to the way that devools works (namely, we need to expose some sort of option in the text view stating that a particular row should be ignored when the request is replayed).

I don't know very much about how devtools works, and don't know what this should look like. Is there any chance you could find someone more qualified than me to take over the rest of this bug?
Flags: needinfo?(jsnajdr)
(Assignee)

Comment 12

3 years ago
Yes, I can take it. It will most likely require tweaking the Netmonitor UI - to detect which headers were really changed, which should be modified on the new request and which should be left as defaults.
Assignee: michael → jsnajdr
Flags: needinfo?(jsnajdr)
Since this is not a recent regression, I wonder if it's necessary for FF48 or if we want to accept a patch for FF48, in consideration of the release schedule. May I know your opinion, Michael? Thanks!
Flags: needinfo?(michael)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> Since this is not a recent regression, I wonder if it's necessary for FF48
> or if we want to accept a patch for FF48, in consideration of the release
> schedule. May I know your opinion, Michael? Thanks!

I don't think it would make sense to block FF48 on this patch, as the regression isn't recent or seriously affecting most of our users.
Flags: needinfo?(michael)
(In reply to Michael Layzell [:mystor] from comment #14)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> > Since this is not a recent regression, I wonder if it's necessary for FF48
> > or if we want to accept a patch for FF48, in consideration of the release
> > schedule. May I know your opinion, Michael? Thanks!
> 
> I don't think it would make sense to block FF48 on this patch, as the
> regression isn't recent or seriously affecting most of our users.

Thanks for the comment, based on which I change "status-firefox48" to wontfix. Please feel free to change the status if you have different opinions.
(Assignee)

Updated

3 years ago
Depends on: 1292503
(Assignee)

Comment 16

3 years ago
I spent some time analyzing this bug and the conclusion is: if we want to fix it, we must define first what exactly it means to "resend a request".

If it's a CORS request that triggered a preflight, there are two choices:

1. Replay both the preflight and flight requests. In case I didn't do any edit (headers or body) in the Netmonitor UI, it should behave exactly the same as the original requests. The preflight should have the same Access-Control-Request-* headers etc.

2. Replay only the flight request, don't send the preflight.

It turns out that option 1 is impossible to implement correctly. It's because the list of headers in Access-Control-Request-Headers is determined not by their presence and content, but by HOW they were set. If they were set by the browser, they are safe and don't trigger a preflight. However, if they were set by the user (using the XHR or Fetch API), they are considered unsafe and are included in the A-C-R-H header of the preflight.

That means that even knowing the exact headers of the original request, I'm unable to figure out which were set by the user and which were set by the browser. Therefore, I cannot replicate exactly the preflight request.

So, only option 2 is possible to implement.

Alex, what do you think? Is there any other person who could contribute to this problem?
Flags: needinfo?(poirot.alex)
I'm not dogfooding this panel much, and I'm all but an expert on CORS requests, so I'm not sure my advice is really valuable...

But at first sight, given that we list both requests in the panel, it seems sensible to replay them individually.

May be someone from necko would have more useful input about this?

Also, looking at chrome, I don't see any OPTIONS requests. But while looking at https://app.expernova.com/, I do see duplicated requests made to /presets??

Thanks again for polishing the network monitor!!
Flags: needinfo?(poirot.alex)
Jason, wondering if you know what the next move is here?
Flags: needinfo?(jduell.mcbugs)

Comment 19

3 years ago
Sadly I don't know CORS or the XHR code very well :(
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Updated

3 years ago
See Also: → 983451
(Assignee)

Updated

3 years ago
See Also: → 1009748
(Assignee)

Updated

3 years ago
Component: DOM → Developer Tools: Netmonitor
Product: Core → Firefox
(Assignee)

Updated

3 years ago
Blocks: 906728
(Assignee)

Updated

3 years ago
Blocks: 922279
(Assignee)

Updated

3 years ago
Blocks: 1009748
See Also: 1009748
(Assignee)

Updated

3 years ago
Blocks: 1107529
(Assignee)

Updated

3 years ago
Blocks: 1124685
(Assignee)

Updated

3 years ago
Blocks: 1124690
(Assignee)

Updated

3 years ago
Blocks: 1124704
(Assignee)

Comment 21

3 years ago
The code that replays the selected HTTP request needs to satisfy the following requirements:
- don't trigger a preflight request, i.e., disable the CORS security checks. The CORS preflight mechanism distinguishes between headers that were added or changed by the page author and these that were added by the browser. This makes sense for XHR requests made by the web pages, but not so much for the devtools replay feature.

- allow to set headers that are otherwise considered unsafe for web pages. Like Host, Referer. Complete list is at [1]

- the request should have loadingNode and loadGroup set to the window/document being inspected. Otherwise, it won't appear in the Netmonitor

- the request shouldn't interact with cache - don't use cached responses, don't store received response to cache

- the request should be anonymous - don't add cookies or Authorization header unless explicitly specified in the replay UI

It turns out that XMLHttpRequest is not up to the task (not even with the mozSystem flag), so I rewrote the sending code to use a nsIHttpChannel directly, setting all the needed flags and properties on the channel.

Added tests that check that no CORS preflight is sent and that the headers are OK.

Should fix almost all the dependent bug, except bug 1124685 - when privacy.donottrackheader.enabled is true, the DNT header is added no matter what, without any way to opt-out.
(Assignee)

Updated

3 years ago
Priority: -- → P1
Comment hidden (mozreview-request)
Comment on attachment 8787638 [details]
Bug 1270096 - Netmonitor request replay: disable CORS checks, set request headers accurately

https://reviewboard.mozilla.org/r/76340/#review75890

Nice work!

I made some minor comments, but all look good to me, R+ assuming Try is green.

- tests pass for me localy
- also works for me on simple web app.

One additional thing I noticed in the UI is the 'cause' column value set to 'other'.
It would be nice to display 'resent' or something similar.


Honza

::: devtools/client/netmonitor/test/browser_net_resend_cors.js:43
(Diff revision 2)
> +
> +  info("Waiting for tab update");
> +  yield monitor.panelWin.once(EVENTS.TAB_UPDATED);
> +
> +  info("Cloning the selected request into a custom clone");
> +  RequestsMenu.cloneSelectedRequest();

Why cloneSelectedRequest() call is needed? As as user, I do just one action (edit & resend) from the  context menu. Shouldn't the test mimic that?

::: devtools/client/netmonitor/test/browser_net_resend_cors.js:62
(Diff revision 2)
> +  is(item.status, 200, "The response has the right status");
> +  // eslint-disable-next-line mozilla/no-cpows-in-tests
> +  is(item.responseContent.content.text, "Access-Control-Allow-Origin: *",
> +    "The response has the right content");
> +
> +  info("Finishing the test");

It's also possible to resend the(OPTIONS) Preflight request. Shouldn't the test also cover that?

::: devtools/client/netmonitor/test/browser_net_resend_headers.js:24
(Diff revision 2)
> +  let requestUrl = SIMPLE_SJS;
> +  let requestHeaders = [
> +    { name: "Host", value: "fakehost.example.com" },
> +    { name: "User-Agent", value: "Testzilla" },
> +    { name: "Referer", value: "http://example.com/referrer" },
> +    { name: "Accept", value: "application/jarda"},

Nice content type :)
Attachment #8787638 - Flags: review?(odvarko) → review+
(Assignee)

Comment 24

3 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> One additional thing I noticed in the UI is the 'cause' column value set to
> 'other'.
> It would be nice to display 'resent' or something similar.

Yeah, I'm thinking about adding a TYPE_DEVTOOLS value to nsIContentPolicyBase.idl. Bug 886233 (display source map requests in Netmonitor) also involves sending network requests on behalf of devtools, so it could be added while fixing that bug.

> > +  RequestsMenu.cloneSelectedRequest();
> 
> Why cloneSelectedRequest() call is needed? As as user, I do just one action
> (edit & resend) from the  context menu. Shouldn't the test mimic that?

Even as user, you do two actions:
1) Click on "Edit and Resend" - that shows the request editor UI
2) Click on "Send" - send the request after you're finished editing

RequestsMenu.cloneSelectedRequest() does the first step.

> It's also possible to resend the(OPTIONS) Preflight request. Shouldn't the
> test also cover that?

Good point, test amended.

> > +    { name: "Accept", value: "application/jarda"},
> 
> Nice content type :)

Thanks :) I'm testing that the header was really set, and not overriden by some default. So I need header values that are unique enough :)
Comment hidden (mozreview-request)

Comment 26

3 years ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f9c16c7e72ef
Netmonitor request replay: disable CORS checks, set request headers accurately r=Honza

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9c16c7e72ef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Hello Jarda, do you think it's worthwhile uplifting this patch to Beta50? I noticed it since it was marked as a P1 regression.
Flags: needinfo?(jsnajdr)
Hi Jeremie, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(jeremie.gourovitch)
(Assignee)

Comment 30

3 years ago
(In reply to Ritu Kothari (:ritu) from comment #28)
> Hello Jarda, do you think it's worthwhile uplifting this patch to Beta50? I
> noticed it since it was marked as a P1 regression.

I think it's not worth it for two reasons:

- it's not really a regression. Yes, the STR started happening only after bug 1188932 (support setting custom User-Agent in XHR) landed, but that just transformed one buggy behavior (User-Agent wasn't set at all) into a different buggy behavior (setting User-Agent triggers unwanted preflight request). The actual bug is a few years old - see the "Blocks" list to see all the duplicate reports dating back to 2013.

- backporting the tests would be nontrivial, as this bug got landed after a heavy refactoring of Netmonitor mochitests in bug 1297362.
Flags: needinfo?(jsnajdr)
Keywords: regression
(Assignee)

Updated

3 years ago
Duplicate of this bug: 906728
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1009748
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1107529
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1124685
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1124690
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1124704
(Assignee)

Updated

3 years ago
Duplicate of this bug: 922279
Given that this is not a new regression and comment 30, we will let this one ride the 51 train.

Updated

2 years ago
Depends on: 1340100
(Reporter)

Comment 39

2 years ago
Thank's mates! 

It appears solved - FF 51.0.1, i don't see anymore "Options" and XHR resending is fully functional!

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.