[py]: Apply ClientConfig user_agent and extra_headers per connection

[navin772]

🔗 Related Issues

💥 What does this PR do?

RemoteConnection.__init__ copied the connection's user_agent and extra_headers onto the class, not the instance:

RemoteConnection.extra_headers = self._client_config.extra_headers or RemoteConnection.extra_headers
RemoteConnection.user_agent = self._client_config.user_agent or RemoteConnection.user_agent

Every RemoteConnection in the process then shared them, and they outlived driver.quit(). A driver pointed at an authenticated Grid followed by a local driver meant the local driver sent the Grid's Authorization header:

a = webdriver.Remote(client_config=ClientConfig(url_a, extra_headers={"Authorization": "Bearer SECRET"}))
b = webdriver.Remote(client_config=ClientConfig(url_b))  # also sends Bearer SECRET

Now they're applied per-instance in _request() from self._client_config, so one connection's headers can't reach another. Class-level RemoteConnection.extra_headers / user_agent still work as process-wide defaults.

🔧 Implementation Notes

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude code
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

[py] Prevent RemoteConnection user-agent/header leakage across instances

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Stop mirroring per-connection headers onto RemoteConnection class attributes.
• Apply ClientConfig user_agent and extra_headers only within each request.
• Add unit tests asserting headers never leak between separate RemoteConnection instances.

Diagram

graph TD
  A((Caller code)) --> B[/"ClientConfig"/] --> C["RemoteConnection instance"] --> D["_request()"] --> E["urllib3 PoolManager"] --> F{{"Remote Server"}}

  subgraph Legend
    direction LR
    _a((Caller)) ~~~ _b[/Config/] ~~~ _c[Component] ~~~ _d{{External}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Cache computed headers on the instance in __init__

• ➕ Avoids recomputing/merging headers on every request
• ➕ Makes the per-instance nature explicit (self._headers)
• ➖ Must keep cache in sync with any runtime mutations (e.g., changing auth/proxy/keep-alive)
• ➖ More state to maintain and reason about than reading from ClientConfig at request time

2. Extend get_remote_connection_headers() to accept per-instance overrides

• ➕ Keeps all header construction in one method
• ➕ Makes override points explicit via parameters rather than post-merge logic
• ➖ Requires changing a widely-used classmethod signature or adding a parallel API
• ➖ Still needs careful ordering rules (defaults vs overrides vs auth) to avoid regressions

Recommendation: The chosen approach (apply ClientConfig.user_agent/extra_headers inside _request() per instance) is the best balance of safety and compatibility: it eliminates cross-instance leakage without changing the public header-building API, while preserving RemoteConnection class attributes as process-wide defaults.

Files changed (2) +78 / -29

Bug fix (1) +9 / -2

remote_connection.pyApply ClientConfig user_agent/extra_headers per request (no class-level mirroring) +9/-2

Apply ClientConfig user_agent/extra_headers per request (no class-level mirroring)

• Removes copying ClientConfig.user_agent and extra_headers onto RemoteConnection class attributes during initialization. Instead, _request() now overlays per-instance user_agent and extra_headers onto the computed request headers, preventing cross-connection leakage while keeping class attributes as defaults.

py/selenium/webdriver/remote/remote_connection.py

Tests (1) +69 / -27

remote_connection_tests.pyAssert per-connection headers reach requests and never leak between connections +69/-27

Assert per-connection headers reach requests and never leak between connections

• Refactors tests to capture actual headers passed to the underlying connection request rather than mocking header construction. Adds a regression test proving that a connection with authenticated/extra headers does not contaminate a subsequently created plain RemoteConnection instance.

py/test/unit/selenium/webdriver/remote/remote_connection_tests.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

No code changes since the last review — review skipped

[qodo-code-review]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again manually by commenting /agentic_review on this PR.

[navin772]

@cgoldberg could you review this PR?

[cgoldberg]

nice. LGTM

[AutomatedTester]

Thanks for digging into this — the leak is real and the two-driver repro in the description is exactly right. But I don't think this closes it, I think it reverses its direction.

I checked the claims below by running trunk, this branch (83bd329) and a prototype side by side rather than by reading the diff, so the tables are measured rather than inferred.

The framing I'd suggest: RemoteConnection.extra_headers / user_agent want to be class-level templates, handled the way capabilities already are in this codebase — copy.deepcopy(caps) in _create_caps, DesiredCapabilities.CHROME.copy() in ChromiumOptions.default_capabilities. A template gets copied by every consumer at construction and is never written back to. Trunk writes back to the template — that's the leak you found. This PR stops writing back, but leaves the template as a live layer underneath every request. Copying at construction takes it out of the request path altogether.

The copy is missing in a second place too, and it isn't in this diff — ClientConfig.__init__ stores the caller's dicts by reference:

self.init_args_for_pool_manager = init_args_for_pool_manager or {}
self.extra_headers = extra_headers

So two connections handed the same ClientConfig share one extra_headers dict, and anything that mutates it in place reaches back into the user's own dict. Appium does precisely that: cls.extra_headers[HEADER_IDEMOTENCY_KEY] = str(uuid.uuid4()). dict(extra_headers) if extra_headers is not None else None plus dict(init_args_for_pool_manager or {}) covers it.

Two things I'd file separately rather than grow this PR:

• ClientConfig.__init__ has proxy: Proxy | None = Proxy(raw={"proxyType": ProxyType.SYSTEM}) — one instance, evaluated at import, shared by every ClientConfig ever constructed.
• _request follows a 3xx Location to an arbitrary host and re-sends extra_headers plus get_auth_header(). I confirmed Authorization: Bearer SECRET is sent on to the redirect target, on both trunk and this branch.

[cgoldberg]

erm.. yeah what he said ^^ :)

[AutomatedTester]

Thanks — this addresses all three findings from my earlier review. I re-ran the same probes
against trunk and this head rather than reading the diff, plus the module's own unit tests.

behaviour	trunk	previous head	this head
conn A's Authorization reaches conn B	yes ✗	no	no ✓
process-wide class header forced onto a conn with its own config	no	yes ✗	no ✓
class-level header still applies when a conn sets none	yes	yes	yes ✓
conn.get_remote_connection_headers() matches the wire	yes	no ✗	yes ✓
real AppiumConnection: appium/ UA prefix + its idempotency key	yes	no ✗	yes ✓
caller's extra_headers / init_args_for_pool_manager aliased	yes ✗	yes ✗	no ✓
two conns sharing one ClientConfig share one headers dict	yes ✗	n/a	no ✓
mock.patch.object on the hook still intercepts	yes	yes	yes ✓

I checked the Appium row against the real appium-python-client source rather than the
synthetic subclass in the test: AppiumConnection declares its own extra_headers = {}, so
its classmethod super() stays class-bound and reads its own attributes — trunk parity exactly.

Test suite: 94/94 pass on this head. Of the nine added/rewritten tests, four fail against
trunk (test_client_config_headers_do_not_leak_across_connections,
test_class_level_extra_headers_are_a_default_not_a_floor, and both client_config copy
tests), so they are genuine regression coverage; the other five pass on trunk as parity locks
against the previous approach. Nothing tautological.

Six small things below. None of them are blocking.

1. functools.partial loses five bits of the method's API surface

_classorinstancemethod.__get__ returns a functools.partial, which is not a bound method:

	trunk	this head
type(...)	method	partial
inspect.ismethod(...)	True	False
__func__ / __self__	present	absent
__name__	get_remote_connection_headers	absent
__doc__	"Get headers for remote request."	"partial(func, *args, **keywords) - new function with partial application..."

The __doc__ one is user-visible: help(RemoteConnection.get_remote_connection_headers) now
prints functools.partial's docstring instead of the method's. functools.update_wrapper on
the descriptor doesn't help, because the wrapper lives on the descriptor and the partial is
what callers actually receive.

One-line fix — types.MethodType is what classmethod itself hands back, so all five rows
return to the trunk column:

+import types
@@
     def __get__(self, obj, cls=None):
-        return functools.partial(self.func, obj if obj is not None else cls)
+        return types.MethodType(self.func, obj if obj is not None else cls)

I ran this variant: every behaviour the new tests assert still holds, including the
class-bound super() path for the Appium-shaped subclass, and inspect.signature stays
(parsed_url, keep_alive=False).

2. Class-level headers set after a connection exists no longer reach it

__init__ now always writes self.extra_headers / self.user_agent, so an instance
attribute exists from construction and shadows the class attribute for that connection's
lifetime:

conn = RemoteConnection(client_config=ClientConfig(url))
RemoteConnection.extra_headers = {"Foo": "bar"}
conn._request("GET", url + "/status")   # trunk: sends Foo. this head: does not.

Set-before-construct still works, which is the documented pattern, so this may well be fine —
but it silently breaks refreshing a class-level token mid-session. Worth either a deliberate
note in the changelog/docstring or a test pinning whichever semantics you want. Right now
nothing covers it.

3. The ClientConfig copy has a second door

__init__ copies, but assignment through _ClientConfigDescriptor does not, so the invariant
the two new tests assert only holds via the constructor:

caller = {"X-Api-Key": "secret"}
cfg = ClientConfig(url)
cfg.extra_headers = caller
assert cfg.extra_headers is not caller   # fails, on trunk and on this head

Not a regression, and arguably out of scope — but if the goal is "caller-owned dicts can't be
aliased in", the setter is the other way in.

4. Stale comment from the previous approach

py/test/unit/selenium/webdriver/remote/remote_connection_tests.py:475-477 still says
extra_headers "are applied per-connection in _request". They're now resolved in __init__
and read through the descriptor; _request no longer touches them.

5. Needs a rebase — one trivial conflict, and the description now describes the old approach

The branch is CONFLICTING against trunk. Both source files auto-merge; the only conflict is
in client_config_tests.py, where this PR's new tests and trunk's
test_default_proxy_is_not_shared_between_instances (56e5c616d8) were added at the same
spot. Keeping both blocks is the whole resolution — I merged origin/trunk locally that way
and the module is 95/95 green, with trunk's proxy is None change and this PR's
init_args_for_pool_manager copy sitting happily in the same __init__.

Two things worth knowing about the rebase:

• Trunk also enabled disallow_untyped_decorators in py/pyproject.toml (34c744284a), which
  landed after this branch's base. I checked whether the new _classorinstancemethod decorator
  trips it: it doesn't — mypy reports zero errors in remote_connection.py and
  client_config.py under the stricter config (the only untyped-decorator hits are the
  pre-existing webdriver.py ones that 34c744284a already fixed). So the green mypy check
  here holds after the rebase.
• The PR description still describes the previous implementation — "applied per-instance in
  _request()" and "class-level RemoteConnection.extra_headers / user_agent still work as
  process-wide defaults". Neither is true of this head any more: they're resolved once in
  __init__, and class-level values are now genuine defaults rather than a floor merged under
  every request. Worth rewriting before merge, since the description is what ends up in the
  squashed commit message.

6. Scope note on the __init__ comment

The new comment is accurate and correctly scoped to headers now. The three lines directly
above it still mirror onto the class, though:

RemoteConnection._timeout = self._client_config.timeout
RemoteConnection._ca_certs = self._client_config.ca_certs
RemoteConnection._client_config = self._client_config

So RemoteConnection._client_config.password still returns the last-constructed connection's
credentials after quit(), and RemoteConnection.set_timeout(9) still mutates the
last-built config rather than the class. Both unchanged from trunk and reasonable to leave
here — but it does mean the description's "they outlived driver.quit()" only holds for the
header half now, which folds into the rewrite in (5). A follow-up issue for the
_client_config mirroring would be worth filing.

One separate issue, pre-existing

_request follows a 3xx Location to an arbitrary host and re-sends extra_headers plus
get_auth_header(), so Authorization can reach a redirect target. Unchanged on both trees
and out of scope here, but same "who is allowed to see this header" question.

(The other one I mentioned last time — the shared Proxy() default argument — is already
fixed on trunk by 56e5c616d8, so ignore that.)

[AutomatedTester]

Looks good now, please rebase/fix merge conflicts so we can merge this