Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DNR] in contrast to webRequestBlocking, declarativeNetRequest rules are not applied if the request is redirected #694

Open
derjanb opened this issue Sep 24, 2024 · 11 comments
Labels
inconsistency Inconsistent behavior across browsers needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time topic: dnr Related to declarativeNetRequest

Comments

@derjanb
Copy link

derjanb commented Sep 24, 2024

If a header is added to a request via webRequest then the redirected request also sends this header.
This does not apply to declarativeNetRequest modifyHeaders rules.

Please specify the correct behavior. You can test both behaviors by loading the unpacked extensions attached and investigating the background contexts.
Using webRequest the request to https://httpbin.org/headers sends a aaa header while using declarativeNetRequest the request does not send a bbb header.

From my point of view a redirect means to send the very same request to another URL, which includes extension based modifications which even might have caused the redirect.

t.zip

@github-actions github-actions bot added needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time labels Sep 24, 2024
@derjanb
Copy link
Author

derjanb commented Sep 24, 2024

Firefox never sends the added header at the second request.

mv2.zip
mv3-ff.zip

@dotproto dotproto changed the title [DNR] in opposition to webRequestBlocking declarativeNetRequest rules are not applied if the request is redirected [DNR] in opposition to webRequestBlocking, declarativeNetRequest rules are not applied if the request is redirected Sep 24, 2024
@dotproto dotproto changed the title [DNR] in opposition to webRequestBlocking, declarativeNetRequest rules are not applied if the request is redirected [DNR] in contrast to webRequestBlocking, declarativeNetRequest rules are not applied if the request is redirected Sep 24, 2024
@carlosjeurissen carlosjeurissen added the inconsistency Inconsistent behavior across browsers label Sep 24, 2024
@b-weinstein
Copy link

It appears Safari sends the added header to the second request with DNR.

@b-weinstein
Copy link

But the included test case doesn't reproduce that issue - because we limit the headers that can be modified, and that bbb isn't one of them.

@derjanb
Copy link
Author

derjanb commented Sep 26, 2024

Seem like I need to add some more information after reading the meeting notes.

Wondering about XHR/fetch. Do redirects preserve headers?

@Rob--W Yes, fetch requests preserve headers and the example logs the 'ccc' header.

fetch('https://httpbin.org/redirect-to?url=https://httpbin.org/headers', {
    headers: {
        'ccc': 'foo'
    }
})
.then(response => response.json())
.then(data => {
    console.log(data);
});

Preserving the header after a cross-origin redirect could result in inadvertent leakage of headers. Would especially be a problem for things like Authorization headers

@Rob--W "This header is stripped from cross-origin redirects" and should of course be stripped from DNR modified requests as well. I'm just asking for standard fetch behavior.

Yeah, we think this might be the intended behavior and webRequest was “wrong”.

@oliverdunk I think the above leads to the assumption that the webRequest API was right and it obviously worked fine for many years. ;-)

Let me explain a little bit more Tampermonkey's use case.

Userscripts want to add a header to GM_xmlhttpRequest and they want it to behave like a regular fetch request. Since there is no blocking webRequest API in MV3 I need to setup everything before the extension sends the request.
If redirected requests don't send the header like a regular added fetch header then I have to setup a DNR rule to match all extension requests of resource type xmlhttprequest, because I don't know where the request is redirected to.
This forces me to use an extension-wide lock (service worker, extension pages, offscreen document) effectively serializing all requests. This is a huge performance hit.

So if your really want not-preserving headers as standard behavior, then please make is configurable.

Thanks.

@carlosjeurissen carlosjeurissen added the topic: dnr Related to declarativeNetRequest label Sep 27, 2024
@Rob--W
Copy link
Member

Rob--W commented Sep 27, 2024

@derjanb

I don't recall whether it did the right thing, but I know that I once experimented with using reference fragments for the purpose of tracking requests across redirects. Reference fragments are not sent to servers, but still part of the URL at the client. This enables you to match requests across redirects. Would that address your use case?

I have a reverted proof of concept in one of my local repositories. It is not on my device that I carry at TPAC, but if you would like me to dig up my code, I can do that.

@derjanb
Copy link
Author

derjanb commented Sep 27, 2024

Puhh. Interesting idea, but handling subsequent redirects requires more logic to add the fragment to all URLs, right? I need to check how that can be achieved.

I also have to check what URL is seen by webRequest onHeadersReceived because scripts are interested in the final URL of the request including fragment.

All in all this looks very hacky to me, while according to @b-weinstein the existing code just works in Safari. ;-)

@Rob--W
Copy link
Member

Rob--W commented Sep 28, 2024

Puhh. Interesting idea, but handling subsequent redirects requires more logic to add the fragment to all URLs, right? I need to check how that can be achieved.

No, the point of the reference fragment being completely client-side is that the server cannot see it. If I append a reference fragment to your URL, the URL changes, but the reference fragment stays the same: example.

The reference fragment may however be changed if the server includes a reference fragment in the redirect target, example.

@derjanb
Copy link
Author

derjanb commented Oct 6, 2024

The reference fragment may however be changed if the server includes a reference fragment in the redirect target

@Rob--W And that is a problem I thought I can somehow solve with another DNR rule, but the API doesn't seem to be advanced enough.

So the problem remains: extensions in Chrome need an extension-wide lock to run only one request at a time, that wants to set headers that cannot be set via fetch, in order to ensure that those headers are set for all redirects as well.
In Firefox they can use blocking webRequest and in Safari DNR-set headers are forwarded like regular headers added to fetchare.

@Rob--W
Copy link
Member

Rob--W commented Oct 6, 2024

Another idea, untested: if the headers set by fetch()/XMLHttpRequest appear across redirects, can you then set a unique custom header and match this with DNR?

  • condition.requestHeaders to match (supported in Chrome 128+)
  • optional: modifyHeaders action to remove the custom request header so that the server cannot see it
  • If you do not use one of the CORS-safelisted request headers as the custom header, maybe also modifyHeaders action to append the name of your custom header to Access-Control-Allow-Headers, to avoid CORS issues.

@derjanb
Copy link
Author

derjanb commented Oct 8, 2024

Tracking a request this way might indeed work, but are you sure Chrome added condition.requestHeaders support? Didn't find any information about it and versions:

  • Version 129.0.6668.12
  • Version 131.0.6724.0
  • Version 131.0.6753.0 (Official Build) dev (64-bit)

throw an error:

Error at property 'addRules': Error at index 0: Error at property 'condition': Unexpected property: 'requestHeaders'.

when executing the following code:

chrome.declarativeNetRequest.updateDynamicRules({
  removeRuleIds: [ 1 ],
  addRules: [
    {
      id: 1,
      priority: 1,
      action: {
        type: 'modifyHeaders',
        requestHeaders: [
          {
            header: 'Test',
            operation: 'set',
            value: '1'
          },
        ],
      },
      condition: {
        requestHeaders: [
          { header: 'foo', values: [ '*' ] }
        ]
      }
    }
  ]
}, () => {
      console.log('Headers updated');
})

@Rob--W
Copy link
Member

Rob--W commented Oct 8, 2024

My bad - I was thinking of responseHeaders. requestHeaders matching often comes up in the same discussions, but ultimately Chrome has only implemented responseHeaders support.

Regardless of the current implementation state - I think that such a feature (generic request header support) would be preferable than implementing the original request here (automatically applying headers meant for one request to other requests that do not match the condition).

Ultimately the real goal here is to apply DNR actions to a specific request / chain of requests. This is not uncommon, so I am supportive of a way to do so, but without introducing a footgun. Concrete proposals of feasibly implementable APIs are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inconsistency Inconsistent behavior across browsers needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time needs-triage: safari Safari needs to assess this issue for the first time topic: dnr Related to declarativeNetRequest
Projects
None yet
Development

No branches or pull requests

4 participants