Skip to content

fix(firefox): preserve response header field boundaries - #42832

Open
Devin Rousso (dcrousso) wants to merge 1 commit into
microsoft:mainfrom
dcrousso:fix-42687
Open

Devin Rousso (dcrousso) wants to merge 1 commit into
microsoft:mainfrom
dcrousso:fix-42687

Conversation

@dcrousso

Copy link
Copy Markdown
Contributor

splitting response headers on commas corrupts values and invents field boundaries

use the original fields reported by Juggler without parsing them again

reuse Chromium's Set-Cookie splitter for fulfillment so multiple cookies remain separate fields

work towards #42687

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 All 8 failures look caused by this PR

Hi, I'm the Playwright bot and I triaged the failing CI run.

Every failure is Firefox-only and lands squarely on response header splitting — including the four tests this PR adds. Dropping parseMultivalueHeaders() means Juggler's combined field values are now passed through verbatim on the observed response path; splitSetCookieHeader() is only applied on the fulfillment path (FFRouteImpl.fulfill), so Set-Cookie arrives as one \n-joined entry.

Details

Caused by this PR

  • [firefox-page] › page/firefox-network-response.spec.ts:50 (200 and 302), :80, :105 — the tests this PR adds. They assert separate header fields via headersArray(), but ffNetworkManager.ts now hands event.headers straight to network.Response with no splitting at all, so the combined Juggler values come through as single entries.

  • [firefox-library] › library/har.spec.ts:233 › should include set-cookies and :277 › should include set-cookies with comma — both send multiple Set-Cookie fields and expect multiple parsed cookies in the HAR. With the split gone from the response path, HAR sees one Set-Cookie header whose value is name1=…\nname2=…\nname3=…, so only the first cookie parses.

  • [firefox-page] › page/page-network-response.spec.ts:239 › should report multiple set-cookie headers — same cause: expects ['a=b', 'c=d'] from headersArray(), gets a single a=b\nc=d entry.

  • [firefox-page] › page/page-network-response.spec.ts:203 › should report all headers — expects header-a as three separate entries. Juggler combines those with , , and the comma split that used to recover them is gone. This one is the intentional behaviour change the PR documents ("separate entries are not always available"), but the test was left untouched, so it now fails on Firefox.

The Set-Cookie cases look like an oversight rather than intent: calling splitSetCookieHeader() on event.headers in FFNetworkManager._onResponseReceived should restore them without bringing back the comma splitting. should report all headers needs a deliberate call — either a Firefox fixme/adjusted expectation, or the header is genuinely unrecoverable and the test should say so.

Flaky (not blocking)

The 6 flaky entries (beforeunload, video+trace, trace-viewer-scrub, third-party partitioned cookies, prefetch script body) all passed on retry and are unrelated to header parsing. I didn't dig into their history since they aren't failures.

Triaged by the Playwright bot - agent run

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

🟡 Almost certainly flakes — one I can't prove

Hi, I'm the Playwright bot and I triaged the MCP run.

None of the 4 failures touch response headers: this PR only changes Firefox/Chromium header field handling, and the failing tests assert on browser listing, file upload, and WebMCP frame disambiguation. Three of them have a documented history of failing on other SHAs; the fourth has no history either way.

Details

Pre-existing flake / infra

Uncertain

  • [chromium] › mcp/cli-webmcp.spec.ts:162 › webmcp-call disambiguates duplicate tool names by frame @mcp-macos-latest-chromium — this is its only failure in the results DB (1 of 24 runs), so by the numbers alone I can't call it pre-existing. It does fail the same way as its sibling above — the echo tool resolved in a single frame instead of being reported as ambiguous — and this file is only ~25 runs old, so there is little history to lean on. To be sure it's a flake I'd want a re-run of the MCP job, or one more sighting on an unrelated SHA.

Triaged by the Playwright bot - agent run

splitting response headers on commas corrupts values and invents field boundaries

use the original fields reported by Juggler without parsing them again

reuse Chromium's `Set-Cookie` splitter for fulfillment so multiple cookies remain separate fields
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

4 flaky ⚠️ [chromium-library] › library/browsercontext-page-event.spec.ts:173 › should work with Ctrl-clicking `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-page] › page/workers.spec.ts:191 › should attribute network activity for worker inside iframe to the iframe `@chromium-ubuntu-22.04-node20`
⚠️ [firefox-library] › library/browsercontext-cookies-third-party.spec.ts:257 › third party 'Partitioned;' cookies `@firefox-ubuntu-22.04-node20`
⚠️ [firefox-library] › library/browsercontext-cookies-third-party.spec.ts:470 › top level 'Partitioned;' cookie and same origin iframe `@firefox-ubuntu-22.04-node20`

52544 passed, 1270 skipped


Merge workflow run.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Test results for "MCP"

8937 passed, 1489 skipped


Merge workflow run.

status: response.status,
statusText: network.statusText(response.status),
headers: response.headers,
headers: splitSetCookieHeader(response.headers),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

aren't they already split here? My understanding is that now we pass split cookies between playwright and browser, but at which point can we glue them together so that this split is necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

route.fulfill({ response }) uses APIResponse.prototype.headers which joins Set-Cookie values with \n so we still need to split them before fulfillment

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants