Fix header overrides for intercepted redirects

[hbenl]

Continuing the discussion from here:

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.

Agreed but it's not Playwright that is stalling but the browser and in general we can't predict what overrides would make it stall.

use the original request values for the forbidden headers and ignore overrides for those

I would throw in this case.

Do you want to throw whenever a forbidden header is in the overrides? That would break tests that use Request.headers() (which includes the forbidden headers in Firefox) for the overrides. How about this: we could filter the forbidden headers from Request.headers() and then throw when a test tries to add one of them to the overrides. That would also make Request.headers() more consistent across browsers. We'd also need to keep the unfiltered headers so we can add the forbidden headers to the BiDi network.continueRequest command. @yury-s What do you think?

[yury-s]

We discussed the state of headers override in Playwright in the team meeting. As a guiding principle, the headers that we want to allow override are the same as the user could see in the service worker if the request was handled by one. This means:

• No forbidden headers are overridable. So if a header is in this list, it is going to be passed as is to the server. If the user provides an override for it (e.g. as a result of getting all headers from the request and then passing it to the route as below), the code should just work without throwing any errors.

  const headers = {
    ...request.headers(),
    foo: 'foo-value',
  };
  await route.continue({ headers });

• Ideally, we'd also not return any of the forbidden headers from request.headers(). The user can still see all the headers sent over the network by calling request.allHeaders(). This is again following the service worker analogy,

  • request.headers() - what service worker sees
  • request.allHeaders() - what the remote server sees

  We are not changing behavior of request.headers() right now though (and maybe never), as it is a breaking change for our clients.

We'll update Playwright code and tests to align with this and if there are no unforeseen issues, we can update Bidi implementation to align with it.