1376932 - webRequest.onHeadersReceived modified headers are cached and cannot be further modified

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P3)

Description

[April King [:April]]

This may be a duplicate of bug 1376060, or that may simply be a subset of this bug.

If a webextension subscribes to webRequest.onHeadersReceived, and modifies the HTTP headers in any way, those headers get cached. If a page then returns 304 on further retrieval, the extension is unable to further modify the HTTP headers. Here is the use case that I am running into:

1) In webRequest.onHeadersReceived, push this onto the HTTP headers:

> request.responseHeaders.push({
>   name: 'Content-Security-Policy',
>   value: `object-src 'none'; script-src 'none'`,
> });

2) Toggle something in an extension to whitelist a site so that it can load scripts from any https source, like so:

> request.responseHeaders.push({
>   name: 'Content-Security-Policy',
>   value: `object-src 'none'; script-src https:`,
> });

3) Refresh the page

What should happen is that scripts are allowed to execute. What instead happens is that if the site returns 304, Firefox will use the cached header and CSP will left as script-src 'none'.


There are a handful of ways around this; here is what I have done as a workaround to this cached header issue:

> if (request.documentUrl === undefined) {
>   removeHttpHeaders(request.responseHeaders, ['cache-control', 'expires']);
> 
>   request.responseHeaders.push({
>     name: 'Cache-Control',
>     value: 'no-cache, no-store, must-revalidate',
>   });
> 
>   request.responseHeaders.push({
>     name: 'Expires',
>     value: new Date().toUTCString(),
>   });
> }

There are probably other ways to do this as well. What could possibly happen is one of:

a) webextension modified headers should force cache invalidation
b) webextensions header modification should happen even with invisible redirections to cache, allowing them to strip the old header and replace with updated ones
c) webextensions should be allowed to invalidate cache that they have been injected

This affects extensions such as the Mozilla Laboratory as well as community extensions such as NoScript.

Comment 1

[Shane Caraveo (:mixedpuppy)]

With the workaround, while not ideal, this doesn't need to be fixed in the short term.

Comment 2

[brunoais]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> With the workaround, while not ideal, this doesn't need to be fixed in the
> short term.

What is the workaround, by the way? I can't find it here.

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to brunoais from comment #2)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> > With the workaround, while not ideal, this doesn't need to be fixed in the
> > short term.
> 
> What is the workaround, by the way? I can't find it here.

it's in comment 0

Comment 4

[brunoais]

Ups. Misread it. My bad.

Comment 5

[April King [:April]]

Please note that this comes with a significant performance penalty to the user, especially if you are injecting these headers into a lot of requests.

Comment 6

[[e-mail]]

I did use the suggested 'no-cache, no-store, must-revalidate' workaround in my extension, however this caused an undesirable side-effect, as reported by one user[1]: the pages for which the 'cache-control' header was modified as per above directives could no longer be loaded from the cache when going offline, as is normally the case.

I did "solve" the issue by removing the 'no-store' directive, but this brought back another undesirable side-effect which is that if a user toggles javascript on/off, this no longer works as it used to -- the cached headers are then used.

The only way to now ensure that a CSP directive changes in response headers are taken into account is to reload a page with a cache bypass, which is less than ideal since *all* resources on a page will be pulled with cache bypass, while this used to apply only to a document itself before, not secondary resources.

So now I am facing the dilemma of having to chose the least worse side-effect.

[1] https://github.com/uBlockOrigin/uBlock-issues/issues/229

Comment 7

[Shane Caraveo (:mixedpuppy)]

Revisiting CSP issues, I just wrote tests on this. The header is indeed changed and/or updated on cached requests, so webrequest itself seems to be doing the right thing. However, the cached headers themselves are not updated.

I'll post the test patch, the second test fails on the 3rd request.

request 1: initial request, initial header value set, passes
request 2: cached request, header value is modified, passes
request 3: cached request, header value is not modified again,
fails because the header value is that set in request 1 rather than the modified header from request 2.

In the platform layer I am unsure about:

• should httpchannel update the cache with the modified header from request 2?
• Should httpchannel just invalidate the cache entry if a modification like this happens?
• when is the csp applied to the document, assuming that the header is request 2 is not actually used (I haven't tested for that yet)

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1376932 webrequest onHeadersReceived modification test

Comment 9

[Shane Caraveo (:mixedpuppy)]

:mayhemer, any thoughts on the bullet points in comment 7?

Comment 10

[Honza Bambas (:mayhemer)]

The test is missing in the patch.

Comment 11

[Honza Bambas (:mayhemer)]

Few questions in return:

• webRequest.onHeadersReceived is (also) called from "http-on-examine-merged-response" notification? (we call it for 304 and 206 responses)
• how is header modification performed by webRequest.onHeadersReceived behaving for non-cached responses?
• is this web-ext notification also called for non-cached responses from "http-on-examine-response"?

If the first and the last question answer is 'yes', then I know why we don't accept header modification on cached responses. For 200 network responses, we first call "http-on-examine-response" and only after that we call AddCacheEntryHeaders (from InitCacheEntry()) which stores the whole response head (with provided modification via SetResponseHeader()) as "response-head" metadata on the cache entry. For cached responses we first store "response-head" meta and then we call the notification leading onHeaderReceived. After that, we don't store "response-head" on the cache entry again, so modifications are lost. Note that the UpdateHeaders call just selectively merges headers from the new response (nothing web-ext should worry about at all, tho.)

I think moving AddCacheEntryHeaders call below OnExamineMergedResponse call and adding one after the notification in ProcessPartialContent would fix this bug.

I also think we can just remove SetMetaDataElement("response-head", head.get()); few lines above as those are duplicated (fix bug 1372961).

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #10)

The test is missing in the patch.

ugh, sorry, I've attached it now.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #11)

Few questions in return:

• webRequest.onHeadersReceived is (also) called from "http-on-examine-merged-response" notification? (we call it for 304 and 206 responses)

It is called for all on-examine notifications.

https://searchfox.org/mozilla-central/rev/b3fd653bc6078b3be4a8d06db39eddc5714755da/toolkit/components/extensions/webrequest/WebRequest.jsm#673-690

https://searchfox.org/mozilla-central/rev/b3fd653bc6078b3be4a8d06db39eddc5714755da/toolkit/components/extensions/webrequest/WebRequest.jsm#1032-1049

• how is header modification performed by webRequest.onHeadersReceived behaving for non-cached responses?

As one would expect. Actually the failure in the test file also illustrates that the header set in the non-cached response is what is cached.

• is this web-ext notification also called for non-cached responses from "http-on-examine-response"?

Yes, per above.

If the first and the last question answer is 'yes', then I know why we don't accept header modification on cached responses.

I'll write up a patch and see if your suggestions work.

Thansk!

Comment 14

[Shane Caraveo (:mixedpuppy)]

Honza, the actual notification that my test was triggering is on-examine-cached-response rather than merged-response. The patch includes changes that make the test pass, but there are likely still issues with the patch. Could you take a look? See my comment in the phab comments.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Honza, something that is also ticking in the back of my mind, do we actually want to modify headers in cache based on extension handlers?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)

Honza, something that is also ticking in the back of my mind, do we actually want to modify headers in cache based on extension handlers?

we probably don't. but my understand was that it was expected to see the modified headers in the next response, which happens to be fully read from the cache. so I'm a bit puzzled what is expected.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #16)

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)

Honza, something that is also ticking in the back of my mind, do we actually want to modify headers in cache based on extension handlers?

we probably don't. but my understand was that it was expected to see the modified headers in the next response, which happens to be fully read from the cache. so I'm a bit puzzled what is expected.

Well, there are two issues in this bug. One stating that changing headers in onHeadersReceived on a cached response doesn't modify the headers. It appears based on my test that those headers are indeed modified. However, we were not waiting on that notification to finish asynchronously (fixed in patch) so the headers may not have been modified prior to them being used.

The second issue is about CSP, and whether the CSP header modified on a cached request is actually used. I can probably write a test for that here as well.

If we don't have to modify the cache, then I think we can leave that part out and document the behavior. It would mean an extension would have to always watch for and modify the headers (again) on cached requests.

Comment 18

[April King [:April]]

If we don't have to modify the cache, then I think we can leave that part out and document the behavior. It would mean an extension would have to always watch for and modify the headers (again) on cached requests.

I think this is okay, right? It's an event firing and a small piece of JavaScript code, and I wouldn't have any problems modifying every request that way.

Comment 19

[Shane Caraveo (:mixedpuppy)]

April, the latest patch has a test for the CSP per commen 0, and passes without any changes. Can you look at the test and see if it is covering your original issue? Can you otherwise validate the problem?

Comment 20

[April King [:April]]

Is it nightly, or do I need to build the patch?

Comment 21

[Shane Caraveo (:mixedpuppy)]

The patch it test-only, so you don't need it.

Comment 23

[April King [:April]]

Hmm, I'll try to take a look at it today! Thanks!

Comment 24

[April King [:April]]

I disabled the "cache busting" code inside Laboratory:
https://addons.mozilla.org/en-US/firefox/addon/laboratory-by-mozilla/

I then started by forcing this CSP with the extension on mozilla.org:
default-src *; img-src 'none'

And force refreshed the page. Sure enough, no images. Perfect. Then I turned off the custom CSP as seen by Laboratory, refresh the page (not forced refreshed) and the results I got back were truly bizarre:

• HTTP code: 200
• Transferred in dev tools: cached
• CSP in response header, which shows that it allows images: img-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com data: mozilla.org www.googletagmanager.com www.google-analytics.com adservice.google.com adservice.google.de adservice.google.dk creativecommons.org ad.doubleclick.net; script-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com 'unsafe-inline' 'unsafe-eval' www.googletagmanager.com www.google-analytics.com tagmanager.google.com www.youtube.com s.ytimg.com; connect-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com www.googletagmanager.com www.google-analytics.com https://accounts.firefox.com/ https://accounts.firefox.com.cn/; style-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com 'unsafe-inline'; frame-src www.googletagmanager.com www.google-analytics.com www.youtube-nocookie.com trackertest.org www.surveygizmo.com accounts.firefox.com accounts.firefox.com.cn www.youtube.com; default-src 'self' *.mozilla.net *.mozilla.org *.mozilla.com; child-src www.googletagmanager.com www.google-analytics.com www.youtube-nocookie.com trackertest.org www.surveygizmo.com accounts.firefox.com accounts.firefox.com.cn www.youtube.com
• Page display: no images
• The Laboratory event handler that would inject the CSP is -not- firing, so it's definitely not changing the CSP

All in all, Firefox's behavior is pretty bizarre unless it was secretly returning the cached headers. I'll attach a screenshot of everything happening.

Comment 25

[April King [:April]]

Attachment: Firefox exhibits cached behavior.png

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: csp.xpi

Using this extension the following str works just fine for me:

1. new profile
2. load mozilla.org
3. install extension
4. reload mozilla.org

expect: images are not loaded

5. uninstall extension
6. reload mozilla.org

expect: images are loaded

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff5f5a6975d6
webrequest onHeadersReceived modification test r=robwu

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 29

[gwarser]

When using example extension from bug 1376060 https://ftp.mozilla.org/favicon.ico still appears as image on soft refresh.

Build ID 20190813095543

Comment 30

[Alex Cornestean]

Hello,

Using the STR and extension provided in Comment 26, on Nightly (70.0a1/20190901214807) under Windows 10 Pro 64-bit and macOS High Sierra 10.13.6, the following results were obtained:

• with the extension installed, images are not loaded upon page reload
• after the extension is uninstalled, the images are loaded once the page is reloaded

Can you please confirm these are the proper STR to test the issue, as well as the expected results of the fix? Thanks !

Comment 31

[gwarser]

Modify extension from comment 26 to work on thezdi.com:

urls: ["*://www.thezdi.com/*"],

• new profile
• load https://www.thezdi.com/blog/2017/12/20/invariantly-exploitable-input-an-apple-safari-bug-worth-revisiting
• install extension
• reload https://www.thezdi.com/blog/2017/12/20/invariantly-exploitable-input-an-apple-safari-bug-worth-revisiting

expect: images are not loaded

• uninstall extension
• reload https://www.thezdi.com/blog/2017/12/20/invariantly-exploitable-input-an-apple-safari-bug-worth-revisiting

expect: images are loaded

actual: images are not loaded

Version 71.0a1
Build ID 20190903094847

Comment 32

[gwarser]

Other uBlock Origin feature, HTML filtering, which internally uses webRequest.filterResponseData is also suffering because of cache issues. From my experience It's also easier to reproduce. For example:

• fresh profile
• add github.com##^#user\[login\] to uBlockO "My filters"
• open github.com

field under "Username" will be missing - good

• middle click on refresh button to clone tab

"Username" field is now present - bad

I'm not sure it's strictly related to this Issue (although proposed workaround from comment 0 is working) should I maybe create new one?

Comment 33

[gwarser]

I forgot to mention in comment 32 - workaround in uBO advanced setting cachecontrolforfirefox1376932 must be set to unset to reproduce.

Comment 34

[Shane Caraveo (:mixedpuppy)]

test only patch