Repository navigation
[py] send extra headers with network.setExtraHeaders - #18117
Open
yuriy-mikityuk wants to merge 7 commits into
Open
yuriy-mikityuk wants to merge 7 commits into
yuriy-mikityuk wants to merge 7 commits into
Conversation
add_extra_header() paused every request with a match-everything intercept and continued it with the headers from the beforeRequestSent event. In Chrome that hangs driver.get(), because ChromeDriver does not process network.continueRequest while a classic command is running, and it drops the headers the event does not list, such as Accept. network.setExtraHeaders is in the spec and set_extra_headers() already wraps it, so the store is pushed to the browser instead and the browser adds the headers itself. Nothing is paused and the public API does not change. The browser tests navigated with browsing_context.navigate, which hid the hang, so the new one uses driver.get() and also checks that Accept survives. Fixes SeleniumHQ#18116
The existing composition test only checked that the request continued and that the handler ran. Assert that both the extra header and the header the handler sets arrive, since the browser now adds the extra headers after the handler's continue.
selenium-ci
added
C-py
|
|
yuriy-mikityuk
marked this pull request as ready for review
October 2, 2026 10:04
Contributor
PR Summary by Qodo[py] Send extra headers through BiDi instead of intercepting requests
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Open
3 tasks done
Author
|
Three weeks on this one, and the reason it looks stalled is probably not review: CI has never run. The three workflows are still sitting at action_required from October 2, so the only status on the PR is the CLA check. Could someone approve the run? If you would rather fold this into the get()-to-BiDi work instead, say so and I will close it, though #17715 has not moved since June. |
Member
|
@AutomatedTester @cgoldberg can you please review this one? |
Member
|
/agentic_review |
Contributor
Code Review by Qodo
1. Failed removal keeps headers active
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+806
to
+808
| def _push_extra_headers(self) -> None: | ||
| """Replace the browser's extra headers with the current store.""" | ||
| self._network.set_extra_headers(headers=dict_to_headers(self.extra_headers)) |
Contributor
There was a problem hiding this comment.
1. Headers fail on unsupported browsers 📘 Rule violation ≡ Correctness
_push_extra_headers() now unconditionally calls network.setExtraHeaders instead of retaining the previous interception path. When a browser does not support that command, add_extra_header(), remove_extra_header(), and clear_extra_headers() raise rather than preserving their prior behavior for those users.
Agent Prompt
Issue description
The public extra-header APIs now depend unconditionally on `network.setExtraHeaders`, which breaks callers using browsers or browser versions that do not support the command.
Fix Focus Areas
- py/private/_network_handlers.py[806-808]
- py/private/bidi_enhancements_manifest.py[1347-1350]
Recommended Fix
Detect whether `network.setExtraHeaders` is supported and retain the prior interception-based implementation as a fallback, or provide a clear compatibility path that preserves `add_extra_header()`, `remove_extra_header()`, and `clear_extra_headers()` behavior on unsupported browsers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| request.set_headers(merged) | ||
| def _push_extra_headers(self) -> None: | ||
| """Replace the browser's extra headers with the current store.""" | ||
| self._network.set_extra_headers(headers=dict_to_headers(self.extra_headers)) |
Contributor
There was a problem hiding this comment.
2. Directly set headers disappear on add 🐞 Bug ≡ Correctness
_push_extra_headers() sends only the registry’s private extra_headers store as the complete network.setExtraHeaders list. If a caller first configures headers through the existing low-level Network(driver).set_extra_headers() API and then calls driver.network.add_extra_header(), the second command replaces the first API’s headers rather than preserving them.
Agent Prompt
## Issue description
The high-level registry replaces browser extra headers without preserving headers set through the low-level Python API.
## Fix Focus Areas
- py/private/_network_handlers.py[789-808]
- py/selenium/webdriver/remote/webdriver.py[1293-1302]
## Recommended Fix
Coordinate browser-side extra-header state across the low-level command and high-level helper so adding, removing, or clearing a registry header does not discard headers configured through the other API. Add a test that sets headers through both APIs and verifies both are sent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| raise ValueError(f"Extra header '{name}' not found") | ||
| if not self.extra_headers: | ||
| self._drop_extra_headers_intercept() | ||
| self._push_extra_headers() |
Contributor
There was a problem hiding this comment.
3. Failed removal keeps headers active 🐞 Bug ☼ Reliability
remove_extra_header() deletes the stored entry before _push_extra_headers() sends the replacement list, and clear_extra_headers() likewise clears the store before sending it. If network.setExtraHeaders rejects either command, the browser retains the previously configured headers while the registry records them as removed; retrying a removal then raises ValueError.
Agent Prompt
## Issue description
A rejected browser command leaves the registry’s header store inconsistent with the headers still active in the browser.
## Fix Focus Areas
- py/private/_network_handlers.py[789-808]
## Recommended Fix
Build a proposed copy of the header store, send that copy to the browser, and commit it to `extra_headers` only after the command succeeds. Apply this ordering to add, remove, and clear, and test command failures followed by retries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
Please handle the qodo review comments and I'll be back to merge it . PR is definitely in the right direction so thank you! |
2 of 3 tasks
Member
|
/agentic_review |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 Related Issues
Fixes #18116
💥 What does this PR do?
add_extra_header()paused every request with a match-everything intercept and continued it with the header list rebuilt from thebeforeRequestSentevent. In Chrome that has two consequences:driver.get()hangs until the page load timeout, and headers the event does not list are dropped, so adding one header removesAcceptfrom navigation requests.test_handler_with_classic_navigationalready skips Chrome and Edge because interception does not work with classic navigation there, andadd_extra_header()put every caller into that state without them asking for interception.network.setExtraHeadersis in the spec andset_extra_headers()already wraps it, so the registry now keeps the store and pushes it to the browser. The browser adds the headers itself, nothing is paused, and the public API does not change.🔧 Implementation Notes
The extra-headers intercept, its subscription bookkeeping and the
_before_resolvemerging are gone, so the registry is about request handlers again. The store stays in the registry becauseclear_request_handlers()has to keep the headers.I checked the semantics of the native command in Chrome 154 and Firefox 154: adding a header of the same name replaces the previous one, an empty list clears them, and it composes with request handlers (they see the extra header in
beforeRequestSent, and it is still sent when a handler rewrites headers withset_headers).Verified locally on macOS:
bazel test //py:unitpasses (35/35), and so do thebidi/network_teststargets forchrome-bidiandfirefox-bidi. With the old implementation the new browser test fails on Chrome with a 60 second timeout, which is the hang it guards against.Cross-binding: .NET and Ruby expose
setExtraHeadersdirectly and have no intercept-based helper, so this brings Python in line with them.🤖 AI assistance
💡 Additional Considerations
AGENTS.mdlists as high risk.network.setExtraHeadershas to be supported by the browser. It works in Chrome 154 and Firefox 154, but the old implementation only needed interception, so on a browser without the commandadd_extra_header()now raises. Happy to add a fallback if you would rather keep that working.contexts/userContexts, which matches the previous session-wide behaviour. Exposing that scoping could be a follow-up.driver.get()still hang in Chrome ([🐛 Bug]: network.continueRequest() hangs navigation in Chrome #17373, https://issues.chromium.org/issues/425906330). This PR only stops extra headers from needing interception at all.🔄 Types of changes