fix(routeFromHAR, take 2): hack harRouter to merge set-cookie headers #38487
+91
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Route.fulfill currently does not support multiple headers with the same name (microsoft#37342). There are workarounds when using this API directly (merging headers, tweaking the header name casing, etc.), but this is problematic for routeFromHAR, which depends on this API internally. This patch adds special handling for set-cookie headers within harRouter to merge them into one header. There's some precedent for treating set-cookie specially at various places in the codebase (ex: https://github.com/microsoft/playwright/blob/f54478a23e0daa450fe524905eabc8aabf6efb07/packages/playwright-core/src/utils/isomorphic/headers.ts#L29, https://github.com/microsoft/playwright/blob/baeb065e9ea84502f347129a0b896a85d2a8dada/packages/playwright-core/src/server/chromium/crNetworkManager.ts#L675), so I think this is okay.
yepitschunked
commented
Dec 9, 2025
| expect(await page2.evaluate(fetchFunction, { path: '/echo', body: '12' })).toBe('12'); | ||
| }); | ||
|
|
||
| it('should replay requests with multiple set-cookie headers properly', async ({ context, asset }) => { |
Author
There was a problem hiding this comment.
Not sure this test is still necessary with the multi-cookie test below (seems like that one both records and replays a HAR)
yepitschunked
commented
Dec 9, 2025
| expect(cookie2.split('; ').sort().join('; ')).toBe('first=foo'); | ||
| }); | ||
|
|
||
| it('should record multiple set-cookie headers', { |
Contributor
Test results for "MCP"2688 passed, 116 skipped Merge workflow run. |
Contributor
Test results for "tests 1"7 flaky39445 passed, 792 skipped Merge workflow run. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was previously closed in #38255 due to inactivity. This PR has been updated with the feedback from @yury-s on the previous PR to avoid a bug with single set-cookie headers.
Following PR description is copied from the original:
Route.fulfill currently does not support multiple headers with the same name (#37342). There are workarounds when using this API directly (merging headers, tweaking the header name casing, etc.), but this is problematic for routeFromHAR, which depends on
this API internally. This patch adds special handling for set-cookie headers within harRouter to merge them into one header. There's some precedent for treating set-cookie specially at various places in the codebase (ex:
playwright/packages/playwright-core/src/utils/isomorphic/headers.ts
Line 29 in f54478a
playwright/packages/playwright-core/src/server/chromium/crNetworkManager.ts
Line 675 in baeb065