Skip to content
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.

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

fix: do not allow to override cookie header #35168

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Mar 12, 2025

Behavior before this PR regarding 'Cookie' header already varied between browsers:

  • Chromium would not respect the 'Cookie' header if there was one with the same name in its cookie jar. If there was no corresponding cooke in the cookie jar, Chromium would apply one from the overrides.
  • WebKit would always take one from the cookie jar.

To override cookies addCookies should be used instead.

See https://docs.google.com/document/d/1LXMSP4GVxFLYJxA6z4upKqwkgD-TnVCGeX-daS4VQjk/edit?usp=sharing for mode details.

Reference #35154

@zuzusik
Copy link

zuzusik commented Mar 12, 2025

I can't agree this closes #35154

This adds tests to existing functionality, but I believe it has a bug, which is clearly reproduced in the repo linked to #35154

This comment has been minimized.

@yury-s yury-s changed the title test: overridden cookie header on redirect fix: do not allow to override cookie header Mar 13, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@pavelfeldman pavelfeldman left a comment

Choose a reason for hiding this comment

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

I am not sure we can break people for now good reason. They already have very poor understanding of the interception and now their scenarios will stop working.

@yury-s yury-s force-pushed the test-cookie-redirect branch from e027c0b to 46ab37a Compare March 20, 2025 17:34

This comment has been minimized.

@yury-s yury-s requested a review from pavelfeldman March 20, 2025 18:23
Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › tests/playwright.spec.ts:877:5 › page.pause() should disable test timeout @ubuntu-latest-node20-1

3 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38811 passed, 811 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s added the CQ1 label Mar 20, 2025
Copy link
Contributor

Test results for "tests others"

3 flaky ⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-macos-latest
⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-ubuntu-latest
⚠️ [electron-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @electron-windows-latest

21770 passed, 512 skipped
✔️✔️✔️

Merge workflow run.

Copy link
Contributor

Test results for "tests 2"

10 failed
❌ [chromium-library] › tests/library/browsertype-connect.spec.ts:910:7 › run-server › socks proxy › should proxy local.playwright requests @chrome-macos-latest
❌ [chromium-library] › tests/library/browsertype-connect.spec.ts:925:7 › run-server › socks proxy › should lead to the error page for forwarded requests when the connection is refused @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:171:5 › should work with --save-har @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:182:5 › should work with --save-har and --save-har-glob @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:216:7 › should work with --save-har in nunit @chrome-macos-latest
❌ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:227:7 › should work with --save-har and --save-har-glob in nunit @chrome-macos-latest
❌ [chromium-library] › tests/library/downloads-path.spec.ts:91:5 › downloads path › should accept downloads in persistent context @msedge-beta-macos-latest
❌ [chromium-library] › tests/library/downloads-path.spec.ts:105:5 › downloads path › should delete downloads when persistent context closes @msedge-beta-macos-latest
❌ [webkit-library] › tests/library/browsercontext-viewport-mobile.spec.ts:206:5 › mobile viewport › view scale should reset after navigation @tracing-webkit
❌ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames crop @webkit-macos-15-xlarge

115 flaky ⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/fetch-proxy.spec.ts:21:3 › context request should pick up proxy credentials @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:441:5 › screencast › should work for popups @channel-chromium-macos-latest
⚠️ [chromium-library] › tests/library/client-certificates.spec.ts:400:3 › browser › should not hang on tls errors during TLS 1.2 handshake @channel-chromium-ubuntu-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-fetch.spec.ts:1231:3 › should work with connectOverCDP @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:670:5 › run-server › should fulfill with global fetch result @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:738:5 › run-server › setInputFiles should preserve lastModified timestamp @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch.spec.ts:115:3 › should fire close event for all contexts @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/chromium/chromium.spec.ts:151:15 › should close service worker together with the context @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/chromium/connect-over-cdp.spec.ts:26:5 › should connect to an existing cdp session @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/client-certificates.spec.ts:705:3 › browser › support http2 if the browser only supports http1.1 @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/fetch-proxy.spec.ts:21:3 › context request should pick up proxy credentials @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/popup.spec.ts:264:3 › should not throw when click closes popup @chrome-beta-macos-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @chrome-beta-windows-latest
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-fetch.spec.ts:1231:3 › should work with connectOverCDP @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:939:7 › run-server › socks proxy › should proxy based on the pattern @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch-server.spec.ts:29:5 › launch server › should work with host @chrome-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch.spec.ts:107:3 › should accept objects as options @chrome-macos-latest
⚠️ [chromium-library] › tests/library/fetch-proxy.spec.ts:21:3 › context request should pick up proxy credentials @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:216:7 › should work with --save-har in mstest @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @chrome-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @chrome-windows-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @chrome-windows-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:580:5 › screencast › should capture static page in persistent context @smoke @chrome-windows-latest
⚠️ [chromium-library] › tests/library/chromium/oopif.spec.ts:284:3 › should click @chromium-headed-macos-14-xlarge
⚠️ [chromium-library] › tests/library/inspector/pause.spec.ts:529:5 › pause › should record from debugger @chromium-headed-ubuntu-24.04
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @chromium-headed-ubuntu-24.04
⚠️ [chromium-page] › tests/page/page-screenshot.spec.ts:650:5 › page screenshot animations › should stop animations that happen right before screenshot @chromium-macos-14-large
⚠️ [chromium-library] › tests/library/global-fetch-cookie.spec.ts:190:1 › should remove cookie with expires far in the past @chromium-tip-of-tree-macos-13
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-3.spec.ts:25:7 › cli codegen › should click locator.first @chromium-tip-of-tree-ubuntu-22.04--headed
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-pick-locator.spec.ts:35:7 › should update locator highlight @chromium-tip-of-tree-windows-latest
⚠️ [chromium-page] › tests/page/page-request-continue.spec.ts:72:3 › should delete header with undefined value @chromium-windows-latest
⚠️ [chromium-page] › tests/page/page-set-input-files.spec.ts:146:5 › should upload large file @chromium-windows-latest
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-beta-macos-latest
⚠️ [firefox-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @firefox-beta-ubuntu-22.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-beta-ubuntu-22.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-beta-windows-latest
⚠️ [firefox-library] › tests/library/capabilities.spec.ts:252:3 › requestFullscreen @firefox-headed-macos-14-xlarge
⚠️ [firefox-library] › tests/library/inspector/cli-codegen-3.spec.ts:667:7 › cli codegen › should consume contextmenu events, despite a custom context menu @firefox-headed-macos-14-xlarge
⚠️ [firefox-page] › tests/page/page-click-scroll.spec.ts💯3 › should scroll into view element in iframe @firefox-headed-ubuntu-24.04
⚠️ [firefox-page] › tests/page/page-click-timeout-3.spec.ts:39:3 › should timeout waiting for hit target @firefox-headed-ubuntu-24.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-headed-ubuntu-24.04
⚠️ [firefox-library] › tests/library/browsercontext-basic.spec.ts:34:3 › should be able to click across browser contexts @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/browsercontext-pages.spec.ts:142:3 › should keep selection in multiple pages @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/client-certificates.spec.ts:400:3 › browser › should not hang on tls errors during TLS 1.2 handshake @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/client-certificates.spec.ts:652:3 › browser › should pass with matching certificates and trailing slash @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/download.spec.ts:54:5 › download event › should report download when navigation turns into download @smoke @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/download.spec.ts:244:5 › download event › should error when saving after deletion @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/hit-target.spec.ts:398:3 › should click in iframe with padding @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/inspector/cli-codegen-3.spec.ts:398:7 › cli codegen › should generate frame locators with id attribute @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/popup.spec.ts:20:3 › should inherit user agent from browser context @smoke @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/tracing.spec.ts:263:5 › should not include trace resources from the previous chunks @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/tracing.spec.ts:312:5 › should overwrite existing file @firefox-headed-windows-latest
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-headed-windows-latest
⚠️ [firefox-page] › tests/page/page-goto.spec.ts:81:3 › should work with Cross-Origin-Opener-Policy @firefox-headed-windows-latest
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-13-large
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-macos-13-large
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-13-xlarge
⚠️ [firefox-library] › tests/library/video.spec.ts:163:5 › screencast › should work with old options @firefox-macos-14-large
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-macos-14-large
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-24.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-windows-latest
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-base-url.spec.ts:37:3 › should construct a new URL when a baseURL in browserType.launchPersistentContext is passed to page.goto @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:142:5 › launchServer › should be able to reconnect to a browser @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-launch-server.spec.ts:23:5 › launch server › should work @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/chromium/connect-over-cdp.spec.ts:43:5 › should use logger in default context @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/downloads-path.spec.ts:77:5 › downloads path › should report downloads in downloadsPath folder with a relative path @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @msedge-beta-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-beta-ubuntu-22.04
⚠️ [chromium-library] › tests/library/browsercontext-proxy.spec.ts:27:3 › should work when passing the proxy only on the context level @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:277:5 › launchServer › disconnected event should be emitted when browser is closed or server is closed @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/chromium/connect-over-cdp.spec.ts:89:5 › should connectOverCDP and manage downloads in default context @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/client-certificates.spec.ts:705:3 › browser › support http2 if the browser only supports http1.1 @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/client-certificates.spec.ts:787:5 › browser › persistentContext › should pass with matching certificates @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @msedge-dev-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-dev-ubuntu-22.04
⚠️ [chromium-library] › tests/library/beforeunload.spec.ts:20:3 › should close browser with beforeunload page @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browser.spec.ts:54:5 › should dispatch page.on(close) upon browser.close and reject evaluate @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-fetch.spec.ts:1231:3 › should work with connectOverCDP @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-2.spec.ts:454:7 › cli codegen › should save assets via SIGINT @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-aria.spec.ts:76:7 › should update aria snapshot highlight @msedge-macos-latest
⚠️ [chromium-library] › tests/library/inspector/cli-codegen-csharp.spec.ts:171:5 › should work with --save-har @msedge-macos-latest
⚠️ [chromium-library] › tests/library/logger.spec.ts:19:3 › should log @smoke @msedge-macos-latest
⚠️ [chromium-library] › tests/library/browsercontext-reuse.spec.ts:30:1 › should re-add binding after reset @msedge-ubuntu-22.04
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @tracing-firefox
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:610:7 › cli codegen › should select @webkit-headed-macos-14-xlarge
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:678:1 › should restore control values @webkit-headed-ubuntu-22.04
⚠️ [webkit-library] › tests/library/beforeunload.spec.ts:79:3 › should access page after beforeunload @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:483:7 › cli codegen › should emit single keyup on ArrowDown @webkit-headed-ubuntu-24.04
⚠️ [webkit-page] › tests/page/page-click.spec.ts:261:3 › should click on checkbox input and toggle @webkit-headed-ubuntu-24.04
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:364:7 › cli codegen › should fill [contentEditable] @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-1.spec.ts:930:7 › cli codegen › should click button with nested div @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-2.spec.ts:248:7 › cli codegen › should handle dialogs @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:73:7 › cli codegen › should click locator.nth @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:607:7 › cli codegen › should generate getByLabel without regex @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/inspector/cli-codegen-3.spec.ts:737:7 › cli codegen › should assert value @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @webkit-headed-windows-latest
⚠️ [webkit-library] › tests/library/capabilities.spec.ts:217:3 › make sure that XMLHttpRequest upload events are emitted correctly @webkit-macos-13-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames fit @webkit-macos-13-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-13-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames crop @webkit-macos-14-xlarge
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-14-xlarge
⚠️ [webkit-page] › tests/page/page-request-continue.spec.ts:216:5 › post data › should amend post data @webkit-macos-14-xlarge
⚠️ [webkit-page] › tests/page/page-request-continue.spec.ts:228:5 › post data › should compute content-length from post data @webkit-macos-15-large
⚠️ [webkit-page] › tests/page/page-request-continue.spec.ts:259:5 › post data › should amend utf8 post data @webkit-macos-15-large
⚠️ [webkit-library] › tests/library/tracing.spec.ts:432:14 › should produce screencast frames scale @webkit-macos-15-xlarge

228661 passed, 9112 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants