microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
66.78k stars 3.66k forks source link

[Bug]: allow multiple headers when using setExtraHTTPHeaders #31972

Open thomhurst opened 3 months ago

thomhurst commented 3 months ago

Version

1.45.1

Steps to reproduce

using System.Text.Json;
using Microsoft.Playwright;

Microsoft.Playwright.Program.Main(["install"]);

var playwright = await Playwright.CreateAsync();
var browser = await playwright.Chromium.LaunchAsync();
var context = await browser.NewContextAsync();
var page = await context.NewPageAsync();

await page.SetExtraHTTPHeadersAsync([
    new KeyValuePair<string, string>("Foo", "Bar1"),
    new KeyValuePair<string, string>("Foo", "Bar2"),
    new KeyValuePair<string, string>("Foo", "Bar3"),
]);

page.Request += (sender, request) =>
{
    Console.WriteLine(JsonSerializer.Serialize(request.Headers, new JsonSerializerOptions { WriteIndented = true}));
};

await page.GotoAsync("https://www.google.com");

Expected behavior

I expected to see the headers containing:

  "foo": "Bar1",
  "foo": "Bar2",
  "foo": "Bar3",

Actual behavior

I see just one header sent:

  "foo": "Bar3",

Additional context

Multiple headers with the same name are valid in the HTTP spec, and so Playwright should match that behaviour. The simplest solution would be for the headers dictionary to hold a collection as the value rather than a single string, though this would be a breaking change.

Environment

- Operating System: Windows 11
- Browser: Chromium
- .NET Version: net8.0
mxschmitt commented 3 months ago

We can collect upvotes for it and see if there is demand.

Is this request only about SetExtraHTTPHeadersAsync?

For Request and Response, we provide a way to get them:

thomhurst commented 3 months ago

Yes this would be a breaking change, but we can collect upvotes for it and consider it for the next major version / provide a different method for it if there is demand.

Is this request only about SetExtraHTTPHeadersAsync?

For Request and Response, we provide a way to get them:

I don't think SetExtraHTTPHeadersAsync is actually the problem. I think it's that IRequest.Headers is a Dictionary<string, string> - So if you try and store a new value with the same key it'll just overwrite the old one. But I may be wrong there.

It also affects other methods like IRequest.HeaderValueAsync(string key) - Which only return one value.

IResponse.Headers is also a Dictionary<string, string> so probably has the same problem (but I haven't tested that.)

Interestingly those docs say "Headers with multiple entries, such as Set-Cookie, appear in the array multiple times.". Is that because Set-Cookie uses a semi-colon delimiter?

Because the HTTP spec AFAIK also supports multiple same name headers too. So two ways to do the same thing, and so both should be supported IMO.

I did try using HeadersArrayAsync but still only Bar3 was returned.

mxschmitt commented 3 months ago

Yes, SetExtraHTTPHeadersAsync does not support that. So I'll mark it as a feature request for now.

Request/Response with HeadersArrayAsync support generally speaking multi-header scenarios. So we expose an API for it there. It returns still one, because SetExtraHTTPHeadersAsync only supports a single header.