chore(bidi): fix header overrides for intercepted redirects #37754
Conversation
Test results for "tests 1"5 flaky46926 passed, 816 skipped Merge workflow run. |
| if (redirectedFrom._originalRequestRoute?._alreadyContinuedHeaders) { | ||
| const { headers } = toBidiRequestHeaders(redirectedFrom._originalRequestRoute._alreadyContinuedHeaders ?? []); | ||
| const hostHeader = headers.find(header => header.name.toLowerCase() === 'host'); | ||
| if (hostHeader) { |
There was a problem hiding this comment.
Why is this different from juggler and simply not providing an override is not sufficient? Is it because we always preserve the original in this code ?
What is the semantics of overridden headers passed to network.continueRequest in Firefox - will overrides always replace all original headers or they are merged and if a header is present on the original request but not in the overrides, it will be preserved on the request? I believe for most of the network service headers if there is no explicit override, the default value will be sent (at least in Chromium and WebKit). If in Firefox Bidi the headers must contain all final headers then I believe we should change the logic and merge original headers with the overrides. In this particular case it would mean that we always add the host header regardless of wether it is in _alreadyContinuedHeaders or not (it should probably not be there as we don't allow to override it).
There was a problem hiding this comment.
To provide a bit more context, the header overrides is a mess for the headers that are added by the network service. I believe what we settled on is that we don't want to support overrides of any of the forbidden headers and want them to be propagated as is. I don't think we currently enforce that in all browsers though, so the behavior may vary. For the host and cookie headers there were explicit reports, so we have special handling of those.
There was a problem hiding this comment.
In Firefox BiDi the overrides always replace all original headers (and network requests stall if the overrides don't contain a Host header) whereas Chrome BiDi currently doesn't expect the overrides to contain a Host header (the network request stalls if they do). But the consensus on the BiDi spec seems to be that Firefox' behavior is the correct one ("We should consider data from the BiDi client safe, and provide whatever they want to.").
To get the desired Playwright behavior that you described with BiDi, I think we should
- use the original request values for the forbidden headers and ignore overrides for those
- use the override values for all other headers and ignore the original request values for those
Do you agree?
There was a problem hiding this comment.
I mostly agree with the exception that I don't think anyone would appreciate stalling when an unexpected override is passed or expected header is not passed, immediately throwing a validation error in that case sounds more user friendly.
use the original request values for the forbidden headers and ignore overrides for those
I would throw in this case. There is also an option of not throwing an applying overrides in best effort manner, basically not reporting an error when the override is lost.
use the override values for all other headers and ignore the original request values for those
Yes, agree with this.
But the consensus on the BiDi spec seems to be that Firefox' behavior is the correct one (w3c/webdriver-bidi#983 (comment)).
As far as I know in Chromium at the interception time we don't have headers added by the network stack. There is a special dance for fetching "would be sent" cookies, but there are other headers too added by the network service. Due to this limitation the user cannot easily pass complete set of the headers where they override say just one.
2 participants
#36932 filters the host header from overrides to fix intercepted redirects in Firefox/Juggler (#36719).
This broke network intercepts in Firefox/BiDi because Firefox expects the host header to be present in overrides.
With this PR:
Fixes the following tests in Firefox:
tests/page/page-network-request.spec.ts:tests/page/page-request-continue.spec.ts:tests/page/page-request-fallback.spec.ts:tests/page/page-request-fulfill.spec.ts:tests/page/page-route.spec.ts: