1685570 - Addon network request origin header omitted in v85.

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[tmashuang]

Attachment: ff84-network-request.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

The production steps are to install the MetaMask addon in Firefox v84.0.1 and Firefox Developer Edition v85.0b5. Debug the addon and inspect both network requests.

Actual results:

Seems that the requests are potentially being malformed/omitted from the request. I have a personal api key that is different than the one the MetaMask team is using and I believe that they have solved it by removing the allowlist origin. I have attached an image pf the the same network request in both versions.

Expected results:

Set the network request origin header as the addon url.

Comment 1

[tmashuang]

Ahh I will

Comment 2

[Alex Cornestean]

Hello,

The issue also appears to occur on the latest Nightly (86.0a1/20210110213430). See the screenshot for further details. The only difference is that the status of the request on Nightly is 200 OK as opposed to what it’s like on Beta 85 (i.e 403 Forbidden). I’ll set the flag for Nightly 86 as well, but in case of error, please revert it.

The issue appears to be regressed by https://bugzilla.mozilla.org/show_bug.cgi?id=1605305 (
2021-01-11T09:42:58.559000: DEBUG : Found commit message: Bug 1605305 - Origin header field is not set to HTTP request in cases where it is required, r=ckerschb,necko-reviewers,JuniorHsu,valentin), with this differential Revision: https://phabricator.services.mozilla.com/D80905.

This is the corresponding pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3fd504b386e8751834f730b2ff5e6f12ab3d7e96&tochange=92d71744323a2cdd2eb06e9a8884d69d63b0a456 .

Comment 3

[Alex Cornestean]

Attachment: 2021-01-11_09h13_23.png

Comment 4

[Shane Caraveo (:mixedpuppy)]

Sebastian, this looks like a regression on a patch you did, could you look at this?

Comment 5

[Sebastian Streich [:sstreich]]

Hi! So yes the patch was the cause - actually this is expected :(
Before the patch, fetch driver would just add any triggering origin to the request, that's why you've seen "moz-extension://" url's in the request.
We now follow https://fetch.spec.whatwg.org/#origin-header - and tie what to send here to the referrer-policy. We can see in ReferrerInfo::IsReferrerSchemeAllowed that "moz-extension://" is not allowed for the referrer and thus you get "null".

Comment 6

[Tom S [:evilpie]]

Didn't we try doing this in bug 1405971 and run into a lot of regressions?

Comment 7

[Rob Wu [:robwu]]

(In reply to Tom Schuster [:evilpie] from comment #6)

Didn't we try doing this in bug 1405971 and run into a lot of regressions?

Similar but not identical. In bug 1405971 (https://bugzilla.mozilla.org/show_bug.cgi?id=1405971#c51), the change was to omit the Origin request header. That broke extensions, so the change got backed out (including the unit test that checked that Origin is not set even when an extension doesn't have host permissions - https://hg.mozilla.org/mozilla-central/rev/dd473ab6821e). But CORS request should always have an Origin request header, per specification (https://fetch.spec.whatwg.org/#http-requests (also at https://fetch.spec.whatwg.org/#origin-header):

A CORS request is an HTTP request that includes an Origin header.

In this bug, the complaint is that Origin: moz-extension://[uuid] has been replaced with Origin: null when the extension has host permissions.
In contrast, when the extension does not have host permissions, then the Origin will still be moz-extension://[UUID].

:sstreich - comment 5 does not explain this behavior - why is Origin: moz-extension://[UUID] used for CORS requests from extensions that don't have special permissions (while Origin: null is used for extensions with host permissions)? It should be changed back to moz-extension://[UUID].

PS. There has been lots of discussions at Chromium's side on whether to send the Origin request header (yes) and what form it should take. In Chrome, chrome-extension://[UUID] is always sent. Details are at https://docs.google.com/document/d/1gJnF1jp7AFHAJBhwgC8cdSDBN5iOEzf4tdNsOTfJC2Y/edit and https://bugs.chromium.org/p/chromium/issues/detail?id=966223
The content script case is a bit more special, but Firefox does currently not support CORS from extension content scripts, so regressions in that area are not to be expected (a broken feature cannot be more broken than it currently is - bug 1605197).

Comment 9

[Bobby]

Any solutions on how to get it work? :)

Comment 10

[Sebastian Streich [:sstreich]]

comment 5 does not explain this behavior - why is Origin: moz-extension://[UUID] used for CORS requests from extensions that don't have special permissions (while Origin: null is used for extensions with host permissions)? It should be changed back to moz-extension://[UUID].

Agreed that we definitely should not have inconsistency like that. Thats weird. That usually means there is some other code that injects that origin header, we only use the referrer-based one only if there is none so far. (because there are a few places that add it, for several reasons)
Anyway we can easily add the principals-origin back for moz-extension:// in nsHttpChannel::SetOriginHeader, that should do the trick.

Comment 11

[John]

Any idea when this can be fixed? Its causing some serious issues for my product and it would really help if this was resolved...

Comment 12

[Wojtsen123]

Are you guys planning to make it work soon? It's frustrating a bit and we had to pause our work :)

Btw, have a nice day everyone! :)

Comment 13

[TLRS81]

Why is it not working? I need a fix asap, because my product is down!

Comment 14

[Tomislav Jovanovic :zombie]

Please don't spam with needless comments, it's not helping.

Sebastian, this got to the release, we should try to fix in 86 at least, do you plan to take this?

Comment 15

[Wojtsen123]

(In reply to Tomislav Jovanovic :zombie from comment #14)

Please don't spam with needless comments, it's not helping.

Sebastian, this got to the release, we should try to fix in 86 at least, do you plan to take this?

How long it might takes? 86 should be released on 23.02.21?

Thanks.

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

FYI, after talking with Sebastian, we are going to back out the regressor (bug 1605305) to unblock our users for our 86 release.

Comment 17

[Sebastian Streich [:sstreich]]

Attachment: Bug 1685570 - Revert Origin-Header Changes in FetchDriver.cpp r=pascalc,ckerschb

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Sebastian, we build out last beta tomorrow, please request the uplift for your updated patch once it is ready, thanks!

Comment 19

[Sebastian Streich [:sstreich]]

Comment on attachment 9202292 [details]
Bug 1685570 - Revert Origin-Header Changes in FetchDriver.cpp r=pascalc,ckerschb

Beta/Release Uplift Approval Request

• User impact if declined: Extensions might end up broken for users. Or the Extension Developers are forced to loosen their cors-rule-set
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: - Install MetaMask Addon

• Open Browser Toolbox
• Inspect requests to mainnet.infura.io
• Request Origin Header should not be null

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): We're only reverting the Origin Header setting Code - Which was in tree for a long time, so the risk of more origin header breakage should not be that critical.
• String changes made/needed:

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9202292 [details]
Bug 1685570 - Revert Origin-Header Changes in FetchDriver.cpp r=pascalc,ckerschb

Fix for a visible regression in 85, uplift accepted for our last beta tomorrow, thanks.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93213c026c61
Revert Origin-Header Changes in FetchDriver.cpp r=ckerschb

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68e386307da5

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/93213c026c61

Comment 24

[Alex Cornestean]

Hello,

Verified the fix using the latest Nightly (87.0a1/20210214213026) and Beta (86.0b9/20210211195159) with MetaMask add-on, under Windows 10 x64 and Ubuntu 16.04 LTS.

The Request Origin Header is no longer null but set to the extension’s URL, confirming the fix. For further details, see the attached screenshot.

Comment 25

[Alex Cornestean]

Attachment: Nightly and Beta Origin Header.png

Comment 26

[bugzilla.mozilla]

I still cannot get this to work on either 86.0 or latest nightly, origin is still set to null. Even tested with mozregression:

• 84 works
• 85 and 86 are broken
• 93213c026c61, 2021-02-12, 88.0a1.20210226 and various later nightly builds that should be fixed still don't work

The requests are from this extension in a new profile:
https://addons.mozilla.org/en-US/firefox/addon/gotify-for-firefox/
https://github.com/caltlgin/gotify-firefox

Attached is a screenshot of the request.

Comment 27

[bugzilla.mozilla]

Attachment: gotify_request.png

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

sstreich, can you take a look? With the mentioned extension, there is no Origin: header here with 84 while 86 has Origin: null.

Comment 29

[Tom S [:evilpie]]

Seems to me like the patch only fixed fetch but nothing else like XMLHttpRequest.

Comment 31

[Julien Cristau [:jcristau]]

It seems we should backout the rest of bug 1605305 here, add tests, and start fresh?

Comment 32

[Sebastian Streich [:sstreich]]

Agreed, we should back that out and have proper testing prior for that.

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please provide a patch which reverts the changes, there are conflicts in several files.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Just chatted with Basti - he is on it!

Comment 36

[voltron]

Take note that as mentioned in bug 1696787 there's a problem with both the "Referer" and "Origin" headers - not just Origin header as mentioned in this issue.

Comment 37

[Worcester12345]

(In reply to voltron from comment #36)

Take note that as mentioned in bug 1696787 there's a problem with both the "Referer" and "Origin" headers - not just Origin header as mentioned in this issue.

So, should that be added here, or should that other bug be reopened?

Comment 38

[Sebastian Streich [:sstreich]]

Attachment: Bug 1685570 Backed out changeset 92d71744323a r=jcristau

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4ae4ed532f1
Backed out changeset 92d71744323a r=necko-reviewers,kershaw

Comment 40

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c4ae4ed532f1

Comment 41

[Julien Cristau [:jcristau]]

Comment on attachment 9207810 [details]
Bug 1685570 Backed out changeset 92d71744323a r=jcristau

taking this in 87.0b9 as well.

Comment 42

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91a390801b4e