feat: add extra_headers to connect() and append client=dbapi to X-Wherobots-Client (WBC-179)#71
Conversation
…robots-Client (WBC-179) Add an optional `extra_headers` param to `connect()` and make the driver participate in the shared, cross-service `X-Wherobots-Client` header standard. `X-Wherobots-Client` is an ordered, append-only, comma-separated list of hops (`client=<token>[;ver=<v>][;plat=<p>]`); the leftmost is the origin and each component appends its own hop on the right. This driver appends `client=dbapi;ver=<dbapi version>`. - `connect()` gains `extra_headers`, merged into the request headers before the auth headers are applied, so `Authorization`/`X-API-Key` always win and the advisory header can never affect authentication. - The `X-Wherobots-Client` hop is appended case-insensitively (HTTP header names are case-insensitive) and any differently-cased inbound key is collapsed into the canonical `X-Wherobots-Client` key so exactly one such header is sent. - The merged headers already flow to the `/sql/session` POST, the status-polling GET, and `connect_direct()` -> the WebSocket upgrade, so the appended hop is present on every hop of the connection. - Absent/empty inbound chains are tolerated (bare `client=dbapi;ver=<v>`). Factored a small `_dbapi_version()` helper shared with `gen_user_agent_header()`. Claude-Session: https://claude.ai/code/session_01QP5saxNQTUTJpMRRxZXhzR
|
@salty-hambot rubric |
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂
Solid PR — the append-on-right, case-insensitive collapse, and auth-after-merge ordering are all correct and well-tested. One trivial double-_dbapi_version() call to tidy up and one missing test to nail down the 'extra_headers can't clobber auth' security claim; the reviewer's own self-retracted hand-wringing got left on the cutting room floor.
3 finding(s) posted.
💬 To request a re-review, comment @salty-hambot review
…ariant (WBC-179) Addresses review feedback on the X-Wherobots-Client work: - `importlib.metadata.version` scans the installed-package database on every call and the version is immutable within a process, so resolve it once at import into `_DBAPI_VERSION: Final[str]` and reference it from both `gen_user_agent_header()` and the client-hop builder instead of re-resolving per `connect()`. - Add two tests locking the security invariant that `extra_headers` can never override credentials: a hostile `X-API-Key` or `Authorization` in `extra_headers` is dropped because auth headers are applied after the merge, so the real credential always wins. Claude-Session: https://claude.ai/code/session_01QP5saxNQTUTJpMRRxZXhzR
|
@salty-hambot rubric |
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict: ✅ pass
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | ✅ pass | Append-on-right and case-insensitive collapse iterate a snapshot before mutating; empty inbound handled as falsy. |
| security | ✅ pass | Auth headers assigned after extra_headers.update(), so Authorization/X-API-Key always win; both spoof paths now tested. |
| privacy | ✅ pass | Only a static client=dbapi;ver= token emitted; no PII or credentials added by the driver. |
| reliability | ✅ pass | No new failure modes; helper handles absent, empty, and differently-cased inbound gracefully. |
| scalability | ✅ pass | O(n) over a small fixed headers dict; module-level constant eliminates the repeated metadata scan. |
| observability | ✅ pass | The header is itself the observability artifact; no new log/metric surface needed. |
| clarity/maintainability | ✅ pass | Module constant, canonical header name, and a well-documented helper keep connect() readable. |
| test quality | ✅ pass | Nine tests cover append, no-inbound, case-collapse, forwarding, auth non-override, both spoof paths, WS upgrade, and connect_direct forwarding. |
Both prior nits are buttoned up — double version call collapsed into a module-level _DBAPI_VERSION constant, and the auth-spoofing tests now lock down the security invariant for both X-API-Key and Bearer paths. Ship it.
Prior findings: ✅ 3 resolved
0 finding(s) posted.
💬 To request a re-review, comment @salty-hambot review
Summary
Adds an optional
extra_headersparam toconnect()and makes the driver participate in the shared, cross-serviceX-Wherobots-ClientHTTP header standard — the keystone for cross-service client-chain propagation.X-Wherobots-Clientis an ordered, append-only, comma-separated list of hops (client=<token>[;ver=<v>][;plat=<p>]); the leftmost is the origin and each component appends its own hop on the right (emitting,between hops). This driver appendsclient=dbapi;ver=<dbapi version>. The header is advisory and must never affect authentication.What changed:
connect()gainsextra_headers: Union[Dict[str, str], None] = None, merged into the request headers before the auth headers are applied, soAuthorization/X-API-Keyalways win and the advisory header can never override auth.X-Wherobots-Clienthop is looked up case-insensitively (HTTP header names are case-insensitive) and any differently-cased inbound key is collapsed into the canonicalX-Wherobots-Clientkey so exactly one such header is sent. If a chain is present,, client=dbapi;ver=<version>is appended; if absent, the header is set toclient=dbapi;ver=<version>./sql/sessionPOST, the status-polling GET, andconnect_direct()→ the WebSocket upgrade (additional_headers=headers), so the appended hop is present on every hop of the connection — including the WS upgrade.client=dbapi;ver=….gen_user_agent_header()(importlib.metadata.version("wherobots-python-dbapi"), falling back to"unknown"), factored into a shared_dbapi_version()helper.Related Issues
Implements WBC-179.
Rubric self-assessment
,separator; sets the bare hop when no inbound chain; version drawn from the same source as the user-agent. Covered by tests (a) and (b) below.extra_headersis merged before the auth headers, soAuthorization/X-API-Keyalways win; the advisory header never gates or influences authentication. Testtest_extra_headers_do_not_override_authproves an inboundX-Wherobots-Clientcannot clobberX-API-Key.client=dbapi;ver=<v>token is appended; no PII, credentials, or caller data is added to the header.,). No parsing of the opaque chain, so unexpected inbound content passes through untouched._dbapi_version()helper is reused bygen_user_agent_header(); aWHEROBOTS_CLIENT_HEADERcanonical constant and a documented_append_wherobots_client_hop()helper keepconnect()readable. New param is documented in the docstring.tests/test_driver.py::TestWherobotsClientHeader, all mockingrequests.post/.getand the websocket connect (no external HTTP mock lib), following existing patterns:test_appends_dbapi_hop_to_inbound_chain— inbound chain gets the dbapi hop appended on the right.test_sets_dbapi_hop_when_no_inbound_chain— with no inbound chain, header is exactlyclient=dbapi;ver=<version>.test_appends_case_insensitively— differently-cased inbound key is matched and collapsed to one canonical header.test_forwards_arbitrary_extra_headers— arbitraryextra_headerskeys are forwarded to the request.test_extra_headers_do_not_override_auth— advisory header never clobbers the auth header.test_ws_upgrade_carries_appended_header— the appended header also flows to the WebSocket upgrade path.test_connect_direct_forwards_headers_to_ws—connect_directforwards headers verbatim towebsockets.sync.client.connect(additional_headers=...).Visual Proof
https://claude.ai/code/session_01QP5saxNQTUTJpMRRxZXhzR