[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.

[AutomatedTester]

Blocking — this is where the leak reverses direction rather than closing.

The write moved per-instance but the read didn't. get_remote_connection_headers is a @classmethod still reading cls.extra_headers / cls.user_agent, and these lines then layer the instance config on top of its output. Trunk had one slot with replace semantics (cfg.extra_headers or RemoteConnection.extra_headers); this is two layers with union semantics. A process-wide class-level header now reaches every connection that has its own config — which trunk specifically prevented:

RemoteConnection.extra_headers = {"Authorization": "Bearer GLOBAL-SECRET"}
b = RemoteConnection(client_config=ClientConfig(url_b, extra_headers={"X-Tenant": "A"}))
b._request("GET", url_b + "/status")

	headers on the wire
trunk	X-Tenant: A
this branch	X-Tenant: A, Authorization: Bearer GLOBAL-SECRET

The description says class-level attributes "still work as process-wide defaults", but a default is what applies when nothing else is set — this makes them an unconditional floor.

Two further consequences of the same root cause, resolving after the hook instead of inside it:

1. get_remote_connection_headers() is public and no longer reports what goes on the wire. With ClientConfig(user_agent="my-ua/1", extra_headers={"X-Tenant": "A"}) the hook returns User-Agent: selenium/4.49.0… and no X-Tenant, while the request sends my-ua/1 and X-Tenant: A. On trunk the two agreed.

2. It breaks the downstream that the comment a few lines above names (#14694). appium-python-client's AppiumConnection sets user_agent = f'appium/{library_version()} ({RemoteConnection.user_agent})' and merges cls.extra_headers inside its override. Running that class verbatim against ClientConfig(user_agent="my-app/1.0", extra_headers={"X-Idempotency-Key": "user"}):

	User-Agent sent	X-Idempotency-Key
trunk	appium/5.2.6 (selenium/4.49.0 (python mac))	Appium's uuid
this branch	my-app/1.0	user

That's a public behaviour change in a PR labelled "Bug fix (backwards compatible)". Whether subclass class attributes should outrank ClientConfig is a fair question, but it should be a deliberate decision rather than a consequence of where the headers.update() landed.

Smallest fix — resolve once, with or (replace, matching trunk), into instance-owned state, and read it during header construction instead of patching over the result:

# in __init__, in place of the two removed lines
self.user_agent = self._client_config.user_agent or type(self).user_agent
self.extra_headers = dict(self._client_config.extra_headers or type(self).extra_headers or {}) or None

The obstacle is that get_remote_connection_headers is a @classmethod, so cls.* cannot see instance state — which I assume is why the merge ended up in _request. A small class-or-instance descriptor solves it, and usefully keeps Appium working unchanged: their override is a @classmethod, so its zero-arg super() is class-bound and still reads its own subclass attributes exactly as on trunk.

class _classorinstancemethod:
    def __init__(self, func):
        self.func = func
        functools.update_wrapper(self, func)

    def __get__(self, obj, cls=None):
        return functools.partial(self.func, obj if obj is not None else cls)

I prototyped that and re-ran every probe: the original two-driver leak stays fixed, the global-header leak above is gone, the hook matches the wire again, Appium's prefix survives, and RemoteConnection.get_remote_connection_headers(url) is still callable on the class. Adding keyword arguments to the classmethod instead is worse — it breaks Appium's override signature outright.

[AutomatedTester]

The comment overclaims — the three lines above it still mirror onto the class.

_timeout, _ca_certs and especially _client_config are still copied to the class, and _client_config carries username / password / token / auth_type as well as extra_headers / user_agent. So the description's "they outlived driver.quit()" is narrowed rather than fixed — the class still holds a live reference to the last-constructed connection's credentials after quit.

The deprecated class-level setters also still mutate whichever connection happened to be built last:

a = RemoteConnection(client_config=cfg_a)   # timeout 111
b = RemoteConnection(client_config=cfg_b)   # timeout 222
RemoteConnection.set_timeout(9)             # -> cfg_b.timeout == 9, cfg_a untouched

Confirmed on both trunk and this branch. Same bug class, same root cause. Either drop those three as well (with a deprecation pointer, since #14694 is why they exist) or keep them and reword this comment — as written it reads as though the whole block had been cleaned up.

[AutomatedTester]

This line nulls out the one interaction that is now broken.

Setting extra_headers to None and user_agent to a fixed default is precisely the configuration in which the layering problem cannot show up, so the regression is invisible to the suite and CI stays green.

The case worth adding: a class-level extra_headers and a per-connection ClientConfig(extra_headers=...) both set, asserting the class-level entry is not on the wire.

monkeypatch.setattr(RemoteConnection, "extra_headers", {"Authorization": "Bearer GLOBAL"})
conn = RemoteConnection(client_config=ClientConfig("http://localhost:4444", extra_headers={"X-Tenant": "A"}))
sent = _capture_sent_headers(conn)
conn._request("GET", "http://localhost:4444/status")
assert sent[0]["X-Tenant"] == "A"
assert "Authorization" not in sent[0]

One other thing worth guarding: _capture_sent_headers only replaces self._conn, which exists only when keep_alive=True. A config with keep_alive=False falls through to _get_connection_manager() and would attempt a real request instead of failing loudly.

[cgoldberg]

erm.. yeah what he said ^^ :)