Closed Bug 1376932 Opened 7 years ago Closed 5 years ago

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

Categories

(WebExtensions :: Request Handling, defect, P3)

Product:
WebExtensions
Component:
Request Handling
Type:
defect

Tracking

(firefox70 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: April, Assigned: mixedpuppy)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [webRequest])

Attachments

(3 files)

Bug 1376932 webrequest onHeadersReceived modification test
47 bytes, text/x-phabricator-request
Details | Review
Firefox exhibits cached behavior.png
735.67 KB, image/png
Details
csp.xpi
887 bytes, application/x-xpinstall
Details 
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.
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
status-firefox57: --- → wontfix
Flags: needinfo?(mixedpuppy)
Priority: -- → P3
Whiteboard: [webRequest]
With the workaround, while not ideal, this doesn't need to be fixed in the short term.
Flags: needinfo?(mixedpuppy)
(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.
Flags: needinfo?(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
Flags: needinfo?(mixedpuppy)
Ups. Misread it. My bad.
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.
Product: Toolkit → WebExtensions
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

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)

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

Flags: needinfo?(honzab.moz)

The test is missing in the patch.

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).

Flags: needinfo?(honzab.moz)

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

The test is missing in the patch.

ugh, sorry, I've attached it now.

(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!

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.

Flags: needinfo?(honzab.moz)

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?

(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.

Flags: needinfo?(honzab.moz)

(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.

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.

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?

Flags: needinfo?(april)

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

Flags: needinfo?(april)

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

Flags: needinfo?(april)
Attachment #9081076 - Attachment description: Bug 1376932 webrequest csp header modification test → Bug 1376932 webrequest onHeadersReceived modification test

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

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:

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.

Flags: needinfo?(april)
Attached file 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

  1. uninstall extension
  2. reload mozilla.org

expect: images are loaded

Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff5f5a6975d6
webrequest onHeadersReceived modification test r=robwu

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → mixedpuppy
See Also: → 1573511

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

Build ID 20190813095543

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 !

Flags: needinfo?(mixedpuppy)

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

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

expect: images are not loaded

expect: images are loaded

actual: images are not loaded

Version 71.0a1
Build ID 20190903094847

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?

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

test only patch

Flags: needinfo?(mixedpuppy) → qe-verify-
See Also: → 1648635
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: