fix(security): KalshiConfig.extra_headers frozen post-construction + single header pipeline

[TexasCoding]

Summary

Freeze KalshiConfig.extra_headers at construction so it cannot be mutated after the #298 auth-header guard has run, and drop the duplicate header pipeline so _ci_merge is the single source of truth for per-request header layering. Together these close a forge surface that survived the #298 fix and remove the implementation lie in the documented header-precedence contract.

Issues closed

• Closes KalshiConfig.extra_headers is mutable post-construction, bypassing the #298 auth-header guard #313 — KalshiConfig.extra_headers is mutable post-construction, bypassing the #298 auth-header guard
• Closes config.extra_headers attached to httpx.Client AND re-merged per request #341 — config.extra_headers attached to httpx.Client AND re-merged per request

#298 lineage

#298 closed the auth-header forge surface at two boundaries: per-request extra_headers= (rejected in SyncTransport.request / AsyncTransport.request) and config construction (KalshiConfig.__post_init__ rejects KALSHI-ACCESS-* keys). But @dataclass(frozen=True) only forbids attribute reassignment — the dict stored in extra_headers was still mutable, so config.extra_headers["kalshi-access-key"] = "forged" after construction re-opened exactly the surface #298 had closed. The forged lowercase key would survive _ci_merge (no case-twin in base_headers at that point) and the final {**base_headers, **auth_headers} is case-sensitive, so on httpx 0.28 it would have shipped as two distinct raw header lines.

Behavioral changes

• KalshiConfig.extra_headers is now exposed as a types.MappingProxyType over a defensive copy. Read access is unchanged; attempting to mutate it raises TypeError, and mutating the dict the caller originally passed in no longer affects the config. Type annotation widened from dict[str, str] to Mapping[str, str].
• httpx.Client(headers=config.extra_headers, ...) is no longer set in either SyncTransport or AsyncTransport. The same headers are still applied to every request — _ci_merge(self._config.extra_headers, extra_headers, headers) was already merging them — but the duplicate httpx-side pipeline is gone, so the documented precedence (config defaults < per-call extras < signed auth) is now authoritative.

Tests

• tests/test_config.py
  • test_issue_313_extra_headers_immutable_post_construction — frozen view rejects __setitem__/__delitem__, and mutating the original passed-in dict after construction is a no-op on the config.
  • test_extra_headers_defaults_to_empty_mapping — replaces the prior mutate-bleed test (which mutated the dict directly, now a TypeError) with a per-instance isolation assertion that survives the freeze.
• tests/test_extra_headers_plumbing.py
  • test_issue_313_extra_headers_post_mutation_does_not_send_auth_forge — wire-level: post-construction mutation of a forged kalshi-access-key raises and never reaches the wire; exactly one signed KALSHI-ACCESS-KEY line ships and its value is the SDK signature, not the forged value.
  • test_issue_341_extra_headers_single_pipeline — wire-level: per-call x-trace-id overrides the config-default X-Trace-Id case-insensitively with exactly one raw header line, and config-only headers still reach the wire when no per-call override is present.

Full tests/test_config.py tests/test_extra_headers_plumbing.py tests/test_base_helpers.py tests/test_client.py tests/test_async_client.py: 524 passed.

Source

Round-3 independent audit closure plan, wave W1.

[claude]

Code Review — PR #361: KalshiConfig.extra_headers freeze + single header pipeline

Overview

This PR closes two related security issues: #313 (post-construction mutation of extra_headers bypasses the #298 auth-header guard) and #341 (duplicate header pipeline — config.extra_headers was attached to httpx.Client defaults and merged again per-request via _ci_merge). The approach is correct and the fix is minimal. Notes below, roughly in priority order.

---

Issues

1. Misleading variable name and comment in test_issue_313_extra_headers_post_mutation_does_not_send_auth_forge

# No forged lowercase pair ever reached the wire.
forged = [k for k, _ in wire.raw if k.lower() == b"kalshi-access-key"]
# Exactly one signed KALSHI-ACCESS-KEY line, never two.
assert len(forged) == 1, forged

The variable forged actually collects all raw kalshi-access-key lines (signed or otherwise), and the assertion checks there is exactly one — i.e., the SDK-signed key is present and no duplicate exists. The comment "No forged lowercase pair ever reached the wire" is the wrong framing; the actual not-forged check is assert wire["KALSHI-ACCESS-KEY"] != "forged" three lines later.

Suggested rename: access_key_lines and revise the first comment to "Exactly one KALSHI-ACCESS-KEY line on the wire — the SDK-signed one, not a forged duplicate."

2. _assert_no_auth_headers type annotation not widened (minor inconsistency)

def _assert_no_auth_headers(h: dict[str, str] | None) -> None:

This function is only called with per-call extra_headers (which is still dict[str, str]), so it won't cause a runtime issue. But given the PR's direction of widening Mapping acceptance, aligning this to Mapping[str, str] | None would be consistent and avoids a future surprise if someone passes a MappingProxyType to it.

3. default_factory=dict with Mapping[str, str] annotation — potential mypy strict concern

extra_headers: Mapping[str, str] = field(default_factory=dict)

dict as a factory produces dict[Any, Any], not dict[str, str]. mypy strict may warn here depending on version. A typed factory lambda: cast(dict[str, str], {}) or dict[str, str] (callable form) is the safer choice. Worth confirming CI mypy passes on this line specifically.

---

What's Done Well

• MappingProxyType(dict(self.extra_headers)) is the right pattern — the defensive copy before wrapping means the caller's original dict can't alias into the stored proxy, closing the aliasing vector.

• _ci_merge widened to Mapping[str, str] is correct and necessary; MappingProxyType doesn't satisfy dict, so without this change the type checker would reject the call site.

• Removing headers=config.extra_headers from both SyncTransport and AsyncTransport constructors is the right call. The httpx-client-default path was the source of the duplicate pipeline. With this removed, _ci_merge is unambiguously the single source of truth and the documented precedence order (config defaults < per-call extras < signed auth) is now structurally enforced rather than claimed.

• Wire-level tests using headers.raw to assert on raw line counts (not just logical header values) are the right way to prove the duplicate-pipeline fix. Checking len([k for k, _ in wire.raw if k.lower() == b"x-trace-id"]) == 1 is more meaningful than checking wire["x-trace-id"] == "per-call" alone.

• test_issue_313_extra_headers_post_mutation_does_not_send_auth_forge correctly combines two checks: (a) the TypeError fires so the forge attempt never enters the config, and (b) a wire-level assertion as defense-in-depth in case the TypeError somehow wasn't raised.

• test_issue_313_extra_headers_immutable_post_construction covers both __setitem__, __delitem__, and the aliasing path (mutating the caller's original dict after construction). Good exhaustive coverage.

---

Summary

The fix is correct and closes the identified forge surfaces. Two minor issues worth addressing before merge: the misleading forged variable/comment in the plumbing test, and a check that mypy strict is satisfied with default_factory=dict against the widened Mapping[str, str] annotation.

🤖 Generated with Claude Code

[TexasCoding]

Round-2 review addressed in 028c1f7:

• Renamed forged → access_key_lines + reworded comment in test_issue_313_extra_headers_post_mutation_does_not_send_auth_forge.
• Widened _assert_no_auth_headers parameter to Mapping[str, str] | None for consistency with the Mapping-acceptance widening across the PR.
  Deferring as non-blocking: default_factory=dict on Mapping-annotated field — mypy strict passes on the current CI; bot flagged as forecast concern only.

[claude]

Code Review — PR #361

Overview

This PR closes two related security/correctness issues in the header pipeline: it freezes KalshiConfig.extra_headers at construction time so the #298 auth-header guard cannot be bypassed post-construction (#313), and removes the duplicate httpx.Client(headers=...) path so _ci_merge is the single source of truth for per-request header layering (#341). The change is well-scoped and the motivating security argument is sound.

---

✅ What's done well

• Correct pattern for frozen dataclasses. object.__setattr__(self, "extra_headers", MappingProxyType(dict(self.extra_headers))) in __post_init__ is the right way to mutate a field on a frozen=True dataclass without resorting to dataclasses.FrozenInstanceError gymnastics.
• Defensive copy first. dict(self.extra_headers) creates a new dict before wrapping it, so the caller's original mapping is cleanly detached. No aliasing hazard.
• Correct signature widening. _assert_no_auth_headers and _ci_merge now accept Mapping[str, str] | None, which is the right generalization since MappingProxyType implements Mapping but not dict.
• Wire-level tests are solid. Asserting [k for k, _ in first.raw if k.lower() == b"x-trace-id"] checks the actual raw HTTP header list — the only reliable way to verify there's no duplicate line from a second pipeline.
• WS client is unaffected. kalshi/ws/client.py does not consume config.extra_headers directly, so no gap introduced there.

---

Issues

1. Multi-line comment blocks violate project style — _base_client.py and config.py

CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."

_base_client.py adds a 5-line comment block, and config.py adds a 9-line comment block. Both exceed the allowed single-line maximum. The WHY is meaningful (worth a comment), but each should be one line:

# _base_client.py (SyncTransport and AsyncTransport, both occurrences)
# #341: config.extra_headers is merged per-request via _ci_merge; attaching here would duplicate the pipeline.

# config.py (before the MappingProxyType assignment)
# #313: wrap a defensive copy so post-construction mutation raises TypeError and original dict is detached.

2. No async wire-level tests

TestExtraHeadersSecurityFreeze only exercises KalshiClient (sync). AsyncTransport.__init__ received the same "headers" removal, but there is no corresponding async variant of either test_issue_313_* or test_issue_341_*. Per CLAUDE.md conventions: "New function → write a test." The async transport behavior is untested at the wire level.

Suggested addition (mirrors the sync cases with pytest.mark.asyncio + AsyncKalshiClient):

@respx.mock
@pytest.mark.asyncio
async def test_issue_341_extra_headers_single_pipeline_async(
    self, test_auth: KalshiAuth
) -> None:
    ...

3. Cross-instance isolation assertion was silently dropped

test_extra_headers_defaults_to_empty_mapping replaces test_extra_headers_defaults_to_empty_dict. The old test explicitly asserted that mutating config_a.extra_headers did not bleed into config_b — a direct check of the default_factory isolation invariant. The new test only asserts both are empty; it does not verify that two independently-constructed configs cannot share state.

The invariant still holds (each __post_init__ wraps a freshly copied dict), but the explicit regression test is gone. Consider adding:

original_a = {"X-Foo": "a"}
cfg_a = KalshiConfig(extra_headers=original_a)
cfg_b = KalshiConfig(extra_headers={"X-Foo": "b"})
# Neither modifying the original dict nor config_b affects config_a.
original_a["X-Foo"] = "tampered"
assert cfg_a.extra_headers["X-Foo"] == "a"
assert cfg_b.extra_headers["X-Foo"] == "b"

4. Minor: extra_headers field annotation could be more precise

The field is declared as Mapping[str, str] but is always MappingProxyType[str, str] after construction. Using Mapping is the right input type (callers pass dicts), but since the stored value is always frozen, the public interface could document this more precisely with a property that returns MappingProxyType[str, str]. This is cosmetic / low-priority for now; keeping it as Mapping is defensible.

---

Summary

The security fix is correct and necessary. The two items that should be addressed before merge are the async wire-level test gap (issue #2 — the async path is the mirror of the sync path and should be covered) and the comment length violations (issue #1 — straightforward to fix). The cross-instance isolation regression (issue #3) is a nice-to-have.

[TexasCoding]

Round-3 review addressed in 00f8422: trimmed multi-line comment blocks to single lines per CLAUDE.md; added async wire-level test mirroring the sync single-pipeline check. Deferring: cross-instance isolation explicit regression test (low-priority); MappingProxyType field annotation precision (cosmetic, current Mapping[str,str] is defensible).

[claude]

Code Review — PR #361 (Round 4 / Final)

Three rounds of feedback have been addressed. This is a final pass to confirm the round-3 changes are correct and flag whether anything remains blocking.

---

Round-3 fixes verified

00f84228 — trim multi-line comments; add async wire-level test

• Both comment blocks are now single lines per CLAUDE.md (_base_client.py:221 and config.py:150). ✅
• test_issue_341_extra_headers_single_pipeline_async added and mirrors the sync test correctly. Decorator order (@respx.mock outer, @pytest.mark.asyncio inner) matches the pattern used elsewhere in the file for async respx tests. ✅

---

Remaining deferred items — appropriate to defer

1. Cross-instance isolation assertion (test_extra_headers_defaults_to_empty_mapping): the invariant is structurally enforced — every __post_init__ wraps a freshly copied dict in its own MappingProxyType, so no aliasing path exists. The deferred explicit regression test is nice-to-have, not blocking.

2. default_factory=dict vs Mapping[str, str]: CI mypy strict passes; dict[Any, Any] is compatible with Mapping[str, str] via the Any escape hatch. Not blocking.

3. extra_headers annotation precision (Mapping[str, str] vs MappingProxyType[str, str]): cosmetic. Mapping is the correct input contract and the right thing to expose in the public API surface. Defer is correct.

---

No new findings

The core fix is sound:

• MappingProxyType(dict(self.extra_headers)) correctly closes both the mutation vector and the aliasing vector in one step.
• Removing headers=config.extra_headers from both SyncTransport and AsyncTransport constructors eliminates the duplicate pipeline. _ci_merge is now the unambiguous single source of truth.
• Wire-level tests using headers.raw (raw line counts) rather than logical header lookup are the right way to prove the no-duplicate invariant — a logical read would collapse duplicates silently.

LGTM. No blocking issues. Ready to merge.

🤖 Generated with Claude Code