[TexasCoding]

Closes #298.

What

Two wire-level header bugs surfaced in the post-v2.5 audit:

1. Auth-header forge surface. The per-request header merge in Sync/AsyncTransport.request used plain dict-unpacking, which is case-sensitive on keys. A caller-supplied kalshi-access-key (lowercase) co-existed with the SDK-signed KALSHI-ACCESS-KEY and httpx 0.28 shipped both raw header lines. RFC 7230 leaves multi-instance handling implementation-defined, so a tolerant intermediary could observe the user-supplied value — a forge surface even though the documented contract promises auth headers are SDK-managed.
2. Content-Type flap on JSON helpers. _post(json=...) / _put(json=...) (and async mirrors) relied on httpx's implicit Content-Type inference. httpx only infers when no Content-Type is present, so extra_headers={'content-type': 'text/plain'} shipped a JSON body labelled text/plain. _post_json / _delete_with_body_json already pinned Content-Type explicitly; the inferring siblings now match.

Fix

• New module-level helpers in kalshi/_base_client.py:
  • _assert_no_auth_headers(h) — raises ValueError naming the leaked key when extra_headers contains any case-form of kalshi-access-*.
  • _ci_merge(*layers) — last-layer-wins, case-insensitive on key.
• Both transports route extra_headers through _assert_no_auth_headers and build base_headers via _ci_merge. Auth merge is unchanged (auth keys are always canonical uppercase; base keys are now case-deduped).
• _post / _put and async mirrors now pin headers={"Content-Type": "application/json"} alongside json=....

Tests

Three new wire-assertion tests in tests/test_extra_headers_plumbing.py:

1. extra_headers={'kalshi-access-key': 'X'} → ValueError naming the leaked key.
2. extra_headers={'content-type': 'text/plain'} on a POST with JSON body → wire Content-Type is application/json (verified via respx route inspection).
3. extra_headers={'x-request-id': 'abc'} → round-trips unchanged on the wire.

Four previously-passing tests asserted the silent-override behavior and were rewritten to assert the new ValueError contract. All 437 transport/header tests pass.

Behavioral fence

extra_headers keys starting with kalshi-access- (any case) now raise ValueError where they previously silently leaked. Worth a "Breaking" entry in the v2.6.0 release notes.

Part of the post-v2.5 audit closure (Wave W1).

[claude]

Code Review — PR #308

Good fix overall. The two bugs are real and the core implementation is sound. Three issues worth addressing before merge:

---

Bug: config.extra_headers bypasses _strip_auth_headers

_strip_auth_headers is only called on the per-request extra_headers argument, not on self._config.extra_headers. A caller can still forge auth headers via:

KalshiConfig(extra_headers={"kalshi-access-key": "injected"})

config.extra_headers flows directly into _ci_merge with no validation. This leaves the same forge surface open through the config path, contradicting the stated goal of closing it completely. The fix should also call _strip_auth_headers(self._config.extra_headers) — either at KalshiConfig.__init__ time (preferred: fails early) or at the top of each request() method.

---

Bug: _delete_with_body missing the Content-Type pin

The PR description says the fix makes "the inferring siblings" match the already-pinned helpers, but _delete_with_body (both sync resources/_base.py:256 and async resources/_base.py:496) still uses json=json without headers={"Content-Type": "application/json"}. _delete_with_body_json was already pinned; _delete_with_body is the missed sibling.

---

Test indentation inconsistency

In every rewritten compound with block, the body is over-indented by one level (8 spaces instead of 4 after the :) — e.g. test_extra_headers_plumbing.py:78-89. This is stylistically inconsistent with the rest of the file and will likely trigger a ruff E131 on CI. Example (as written):

        with (
            KalshiClient(auth=test_auth, config=_config()) as client,
            pytest.raises(ValueError, match="KALSHI-ACCESS-"),
        ):
                client.orders.create(   # ← 16-space indent instead of 12

---

Minor observations (non-blocking)

• Naming: _strip_auth_headers raises, it doesn't strip. _reject_auth_headers or _assert_no_auth_headers would be less surprising when reading the call site.
• config.extra_headers also needs a test: There's currently no test asserting that KalshiConfig(extra_headers={"kalshi-access-key": "x"}) raises at request time (or config time, once the gap above is fixed).
• _delete_with_body Content-Type test: Once the fix lands, a matching wire-assertion test would close the coverage gap symmetrically with the new _post test.
• Async Content-Type test: The new test_post_json_body_pins_content_type_against_caller_override is sync-only; the async transport has identical changes but no coverage. Optional, but worth noting.

---

What's solid

• _ci_merge is a clean, correct implementation — the lower_to_canonical approach properly handles the case where later layers use different casing for the same key.
• Raising ValueError (hard error at the boundary) instead of silently overriding is the right call; the PR description's "behavioral fence" framing is accurate.
• The Content-Type pinning in _post/_put (sync+async) and the wire.raw assertion verifying no duplicate headers are both good additions.
• Rewriting the four previously-passing "silent override" tests to assert the new ValueError contract is correct — those tests were asserting wrong behavior.

The two bugs above are the only blockers. Happy to approve once they're addressed.

[TexasCoding]

Follow-up commit 80e4014 addresses bot review:

• config.extra_headers gap: KalshiConfig.__post_init__ now rejects KALSHI-ACCESS-* keys at construction, closing the forge surface that bypassed the per-request guard. Prefix check is inlined to avoid a circular import with _base_client. Tests in tests/test_config.py pin the uppercase, lowercase, KALSHI-ACCESS-SIGNATURE, and KALSHI-ACCESS-TIMESTAMP cases plus a non-auth sanity check.
• _delete_with_body Content-Type: Sync + async _delete_with_body in kalshi/resources/_base.py now pin Content-Type: application/json, matching _post/_put/_delete_with_body_json. New wire-assertion tests via batch_cancel_v2 (sync + async) confirm a caller-supplied content-type: text/plain cannot win.
• Rename: _strip_auth_headers → _assert_no_auth_headers (definition + both call sites). The helper raises; the new name matches.
• Test indent: four with (...) blocks in tests/test_extra_headers_plumbing.py had bodies at 16 spaces instead of 12. Normalized.

Verified: uv run ruff check . clean, uv run mypy kalshi/ clean, uv run pytest tests/test_extra_headers_plumbing.py tests/test_config.py tests/test_client.py tests/test_async_client.py → 480 passed.

[claude]

Code Review PR 308

TL;DR: Solid fix for a real security surface. The approach (early reject + case-insensitive merge + Content-Type pinning) is correct. A few small issues worth addressing before merge.

Overview:
Addresses two wire-level bugs: (1) Case-mismatched kalshi-access-* in extra_headers could co-exist with SDK-signed headers (httpx 0.28 ships both raw lines). (2) A mismatched-case content-type in extra_headers could suppress httpx JSON inference, labelling a JSON body as text/plain. The layered fix is correct: hard-reject at the call boundary (_assert_no_auth_headers), case-insensitive merge (_ci_merge), Content-Type pinning in body helpers, and config-level validation to close the construction-time bypass.

ISSUE 1 - Misleading return type on _assert_no_auth_headers: The signature is -> dict[str, str] | None and it ends with return h, implying a transform. But both call sites ignore the return value and the function only ever raises or does nothing. The return type should be -> None. A future caller could mistakenly rely on the return being a filtered dict, and mypy would not catch it.

ISSUE 2 - PR description names a function that does not exist: The Fix section says _strip_auth_headers(h) but the implementation is _assert_no_auth_headers(h). The code name is better; the description just needs a quick update before landing in history.

ISSUE 3 - Duplicated kalshi-access- literal in config.py: The prefix is inlined rather than importing _AUTH_HEADER_PREFIX from _base_client.py. The comment explains the circular-import blocker, which is fair. A follow-up extracting the constant to a minimal kalshi/_constants.py (no imports from either file) would eliminate drift risk.

ISSUE 4 - Dead mock registration in test_extra_headers_rejects_KALSHI_ACCESS_headers: The ValueError is raised before any request is sent, so the respx.get().mock() inside the test body is never triggered. The @respx.mock decorator should stay to sandbox real calls, but the inline mock registration is dead code and should be removed.

ISSUE 5 - Async test uses assert_all_called=False on a route expected to be called: test_delete_with_body_async_pins_content_type sets assert_all_called=False but then accesses route.calls.last, implying it expects the route to be hit. If the request is ever silently skipped, route.calls.last raises AttributeError instead of a clean assertion failure. The sync mirror uses @respx.mock (strict default). Either drop the flag or set assert_all_called=True explicitly.

Minor Notes: Post-construction mutation bypass: KalshiConfig.extra_headers is a plain dict; a caller can mutate it after init and bypass validation. Low-impact but worth a follow-up. _ci_merge algorithm traced through same-layer case conflicts and cross-layer last-wins scenarios - correct. Content-Type ordering in _base.py: the Content-Type pin is the rightmost non-auth layer in _ci_merge so it correctly wins over a caller-supplied content-type in extra_headers; the ct_lines assertion verifies exactly one header line on the wire.

Summary: Correctness=sound, Security=hard-reject closes the forge surface, Test coverage=wire-level assertions for all three scenarios, Blocking issues=none, Recommended before merge=Issues 1, 4, 5.

[TexasCoding]

Round-2 fixes pushed (957efb4):

• Issue 1: _assert_no_auth_headers return type is now -> None (drops misleading dict return).
• Issue 2: PR description updated to reference _assert_no_auth_headers (the actual function name).
• Issue 3: Extracted AUTH_HEADER_PREFIX to new kalshi/_constants.py (no imports, no circular hazard); both _base_client.py and config.py now import from it.
• Issue 4: Dropped dead respx.get().mock() registration in test_extra_headers_rejects_KALSHI_ACCESS_headers — ValueError fires before any request is sent. @respx.mock decorator kept as sandbox.
• Issue 5: Dropped assert_all_called=False on test_delete_with_body_async_pins_content_type to mirror the sync test (route is expected to be called).

Not addressed — post-construction mutation of extra_headers: KalshiConfig is @dataclass(frozen=True), but extra_headers is a plain dict that a caller can mutate after __init__ to bypass the validation in __post_init__. This is the standard frozen-dataclass-mutable-default caveat (also applies to limits, etc.). Bot flagged it as low-impact; deferring to a separate issue if we want to harden — likely via MappingProxyType or copy-on-construct.

Verify: ruff check clean, mypy kalshi/ clean, 480 tests pass (test_extra_headers_plumbing.py + test_config.py + test_client.py + test_async_client.py).

[claude]

Code Review — PR #308 (updated state)

This is a review of the current diff. The previous bot comment flagged three issues (config bypass, missing _delete_with_body pin, naming) — all three are addressed in the current revision, so this review focuses on what's here now.

---

Overview

Two real wire-level bugs, both fixed cleanly:

1. Auth-header forge surface — case-mismatched kalshi-access-key co-existed with the SDK-signed KALSHI-ACCESS-KEY on the wire because the header merge was case-sensitive dict unpacking. Fixed with _ci_merge + _assert_no_auth_headers rejection at both the per-request and config construction boundaries.
2. Content-Type flap — _post/_put/_delete_with_body now pin Content-Type: application/json explicitly, preventing a caller-supplied content-type: text/plain from riding on a JSON body.

---

What's correct

• _assert_no_auth_headers is called at both entry points (SyncTransport.request, AsyncTransport.request), and also at KalshiConfig.__post_init__ — no bypass path remains.
• _ci_merge correctly handles cross-layer case deduplication. The lower_to_canonical bookkeeping ensures the old canonical key is evicted from out before the new one is inserted, so no duplicate lines reach httpx.
• The SDK-pinned Content-Type is layer 3 in every _ci_merge call, so it cleanly wins over a caller-supplied layer-2 content-type. The wire-assertion tests verify this with .raw inspection.
• asyncio_mode = "auto" in pyproject.toml covers test_delete_with_body_async_pins_content_type — the missing @pytest.mark.asyncio is not a defect.
• _constants.py for AUTH_HEADER_PREFIX correctly solves the circular-import risk that would arise from putting it in _base_client.py or config.py.

---

Remaining observations (non-blocking)

1. _ci_merge: intra-layer case-variant keys

If a single dict layer contains two keys that are case-variants of the same logical header (e.g. {"content-type": "text/plain", "Content-Type": "application/json"}), the last one by Python dict iteration order wins — the dedup only fires across layers, not within a single layer. This is an extremely unlikely caller mistake and irrelevant to the security fix, but worth a one-liner note in the docstring if you want the contract to be airtight.

2. _assert_no_auth_headers is not called on the headers parameter

SyncTransport.request takes both extra_headers (public-facing) and headers (internal, set by _post/_put/_delete_with_body). Validation only fires on extra_headers. The headers arg is SDK-internal, so this is fine in practice — but the boundary is implicit. A comment at the headers parameter saying "SDK-internal; not validated" would prevent future confusion.

3. config.extra_headers is both a httpx client default and explicitly merged per-request

SyncTransport.__init__ passes config.extra_headers to httpx.Client(headers=...), and request() also feeds it into _ci_merge. httpx deduplicates case-insensitively when merging client defaults with per-request headers, so there's no wire duplication — but the redundancy is pre-existing and not introduced here. Worth filing a follow-up to pick one path, or at least add a comment explaining the intent.

4. Double blank line at _base_client.py:164-165

Not a ruff violation (two blank lines between top-level defs is PEP 8 compliant), but there appear to be two consecutive blank lines after _would_exceed_budget making it three total (the existing one before the new functions plus the two added). Run ruff check --select E303 to verify CI won't flag it.

---

Test coverage

• Wire-assertion tests using .raw for both Content-Type and header count are the right way to test this; they catch silent multi-header bugs that header-dict lookups miss.
• test_extra_headers_rejects_KALSHI_ACCESS_headers correctly verifies both canonical and lowercase variants.
• The config.py guard has its own dedicated test class in test_config.py — four focused tests covering uppercase, lowercase, other KALSHI-ACCESS-* prefixes, and a clean passthrough.
• TestHeaderCollision298 closes the coverage gap for the _delete_with_body sync and async Content-Type pin.

---

Verdict

The security fix is correct and complete. The Content-Type pin closes the stated gap. Tests are thorough and verify at the wire level. The observations above are minor and none block merge. Approves.

[TexasCoding]

Round-2 bot review: all 5 items addressed in 957efb4. Remaining 4 observations are explicitly non-blocking ("Approves"). The double blank line at _base_client.py:164-165 is the standard PEP 8 separation between top-level defs (ruff E303 is for >2 blanks, not for the 2 we have); confirmed locally. Merging on green.