Skip to content

chore(bidi): fix header overrides for intercepted redirects #37754

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −6
Open

chore(bidi): fix header overrides for intercepted redirects #37754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/playwright-core/src/server/bidi/bidiNetworkManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,16 @@ export class BidiNetworkManager {
// We do not support intercepting redirects.
if (redirectedFrom) {
let params = {};
if (redirectedFrom._originalRequestRoute?._alreadyContinuedHeaders)
params = toBidiRequestHeaders(redirectedFrom._originalRequestRoute._alreadyContinuedHeaders ?? []);
if (redirectedFrom._originalRequestRoute?._alreadyContinuedHeaders) {
const { headers } = toBidiRequestHeaders(redirectedFrom._originalRequestRoute._alreadyContinuedHeaders ?? []);
const hostHeader = headers.find(header => header.name.toLowerCase() === 'host');
if (hostHeader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const requestHostHeader = param.request.headers.find(header => header.name.toLowerCase() === 'host');
if (requestHostHeader)
hostHeader.value = requestHostHeader.value;
}
params = { headers };
}

this._session.sendMayFail('network.continueRequest', {
request: param.request.request,
Expand Down
2 changes: 2 additions & 0 deletions packages/playwright-core/src/server/firefox/ffNetworkManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ class FFRouteImpl implements network.RouteDelegate {
private _request: InterceptableRequest;
private _session: FFSession;

readonly removeHostHeaderFromOverrides = true;

constructor(session: FFSession, request: InterceptableRequest) {
this._session = session;
this._request = request;
Expand Down
9 changes: 5 additions & 4 deletions packages/playwright-core/src/server/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,10 @@ export class Route extends SdkObject {
throw new Error('New URL must have same protocol as overridden URL');
}
if (overrides.headers) {
overrides.headers = overrides.headers?.filter(header => {
const headerName = header.name.toLowerCase();
return headerName !== 'cookie' && headerName !== 'host';
});
const headersToRemove = ['cookie'];
if (this._delegate.removeHostHeaderFromOverrides)
headersToRemove.push('host');
overrides.headers = overrides.headers.filter(header => !headersToRemove.includes(header.name));
}
overrides = this._request._applyOverrides(overrides);

Expand Down Expand Up @@ -668,6 +668,7 @@ export class WebSocket extends SdkObject {
}

export interface RouteDelegate {
readonly removeHostHeaderFromOverrides?: boolean;
abort(errorCode: string): Promise<void>;
fulfill(response: types.NormalizedFulfillResponse): Promise<void>;
continue(overrides: types.NormalizedContinueOverrides): Promise<void>;
Expand Down
2 changes: 2 additions & 0 deletions packages/playwright-core/src/server/webkit/wkInterceptableRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ export class WKRouteImpl implements network.RouteDelegate {
private readonly _session: WKSession;
private readonly _requestId: string;

readonly removeHostHeaderFromOverrides = true;

constructor(session: WKSession, requestId: string) {
this._session = session;
this._requestId = requestId;
Expand Down
Loading