From d8216004deb6b97b0b7345261f6c9d4d0c3f7c82 Mon Sep 17 00:00:00 2001 From: Jeffrey West Date: Fri, 22 May 2026 07:36:29 -0500 Subject: [PATCH] fix(auth): preserve caller-owned KalshiAuth in from_env; lazy env eval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two overlapping integrity bugs in KalshiClient.from_env / AsyncKalshiClient.from_env: #311 (HIGH): from_env recomputed _auth_owned from the input kwarg after __init__ had already encoded the correct ownership. Case C — caller passes auth=my_auth with env vars set — flipped ownership to True and client.close() then closed the caller's still-live shared KalshiAuth. Case B — caller passes key_id+private_key with no env vars — flipped ownership to False and leaked the sign-offload ThreadPoolExecutor. #316 (MEDIUM): from_env eagerly evaluated KalshiAuth.try_from_env() through kwargs.setdefault, so a malformed KALSHI_PRIVATE_KEY lingering in the process env raised KalshiAuthError even when the caller supplied an explicit auth=, key_id, private_key, or private_key_path. Fix: - Tighten __init__ ownership invariant: _auth_owned is True iff the constructor built the KalshiAuth from key_id+private-key kwargs; False for caller-supplied auth and for unauthenticated clients. - Rewrite from_env to consult the env lazily — only when the caller did not supply any explicit credential source. When from_env builds the auth itself from env vars, it explicitly claims ownership on the resulting client. Adds TestFromEnvOwnershipAndLazyEval / TestAsyncFromEnvOwnershipAndLazyEval covering both #311 cases and both #316 short-circuits, sync and async. Closes #311, #316 --- kalshi/async_client.py | 44 ++++++++++++++++----- kalshi/client.py | 44 ++++++++++++++++----- tests/test_async_client.py | 73 ++++++++++++++++++++++++++++++++++ tests/test_client.py | 80 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 20 deletions(-) diff --git a/kalshi/async_client.py b/kalshi/async_client.py index 3f43a259..f089a880 100644 --- a/kalshi/async_client.py +++ b/kalshi/async_client.py @@ -91,18 +91,25 @@ def __init__( # silently signing with the wrong key. if private_key_path is not None and private_key is not None: raise ValueError("Provide either private_key_path or private_key, not both.") - # #210: only shut down auth on close() when WE built it. If the - # caller passed in their own KalshiAuth, they keep ownership. - self._auth_owned: bool = auth is None + # #210/#311: ``_auth_owned`` means "this client built the auth and is + # responsible for closing it". True only when we constructed the + # ``KalshiAuth`` from ``key_id``+private-key kwargs; never when the + # caller passed a ``KalshiAuth`` and never when there is no auth at + # all (nothing to own). self._auth: KalshiAuth | None + self._auth_owned: bool if auth is not None: self._auth = auth + self._auth_owned = False elif key_id and private_key_path: self._auth = KalshiAuth.from_key_path(key_id, private_key_path) + self._auth_owned = True elif key_id and private_key: self._auth = KalshiAuth.from_pem(key_id, private_key) + self._auth_owned = True else: self._auth = None + self._auth_owned = False # Build config if config is not None: @@ -207,16 +214,33 @@ def from_env(cls, **kwargs: Unpack[ClientInitKwargs]) -> AsyncKalshiClient: Returns an unauthenticated client if no credentials are configured. """ - # Caller-supplied kwargs win over env-derived values (legacy behaviour - # via ``cls(auth=..., demo=..., base_url=..., **kwargs)`` would have - # TypeError'd on duplicates; setdefault preserves env semantics while - # giving the static signature a clean shape). See #266. - kwargs.setdefault("auth", KalshiAuth.try_from_env()) + # #316: only consult env for credentials when the caller did not supply + # any explicit credential source. ``setdefault("auth", try_from_env())`` + # always evaluates the default, so a malformed ``KALSHI_PRIVATE_KEY`` in + # the process env would raise even when the caller passed ``auth=`` or + # ``key_id``+``private_key``. + caller_supplied_auth = ( + "auth" in kwargs + or kwargs.get("key_id") is not None + or kwargs.get("private_key") is not None + or kwargs.get("private_key_path") is not None + ) + env_built_auth: KalshiAuth | None = None + if not caller_supplied_auth: + env_built_auth = KalshiAuth.try_from_env() + if env_built_auth is not None: + kwargs["auth"] = env_built_auth kwargs.setdefault("demo", os.environ.get("KALSHI_DEMO", "").lower() == "true") kwargs.setdefault("base_url", os.environ.get("KALSHI_API_BASE_URL")) client = cls(**kwargs) - # from_env constructs auth locally — caller never owned it. - client._auth_owned = kwargs["auth"] is not None + # #311: ``__init__`` treats any passed-in ``auth`` as caller-owned. When + # ``from_env`` itself built the auth from env vars, the client (not the + # caller) owns it and must shut it down on ``close()``. The constructor + # is the source of truth for every other path — caller passed ``auth=`` + # (caller keeps ownership), caller passed ``key_id``+``private_key`` + # (client owns), or no credentials at all (nothing to own). + if env_built_auth is not None: + client._auth_owned = True return client async def close(self) -> None: diff --git a/kalshi/client.py b/kalshi/client.py index 31e65756..1bac1152 100644 --- a/kalshi/client.py +++ b/kalshi/client.py @@ -93,18 +93,25 @@ def __init__( # silently signing with the wrong key. if private_key_path is not None and private_key is not None: raise ValueError("Provide either private_key_path or private_key, not both.") - # #210: only shut down auth on close() when WE built it. If the - # caller passed in their own KalshiAuth, they keep ownership. - self._auth_owned: bool = auth is None + # #210/#311: ``_auth_owned`` means "this client built the auth and is + # responsible for closing it". True only when we constructed the + # ``KalshiAuth`` from ``key_id``+private-key kwargs; never when the + # caller passed a ``KalshiAuth`` and never when there is no auth at + # all (nothing to own). self._auth: KalshiAuth | None + self._auth_owned: bool if auth is not None: self._auth = auth + self._auth_owned = False elif key_id and private_key_path: self._auth = KalshiAuth.from_key_path(key_id, private_key_path) + self._auth_owned = True elif key_id and private_key: self._auth = KalshiAuth.from_pem(key_id, private_key) + self._auth_owned = True else: self._auth = None + self._auth_owned = False # Build config if config is not None: @@ -175,16 +182,33 @@ def from_env(cls, **kwargs: Unpack[ClientInitKwargs]) -> KalshiClient: Returns an unauthenticated client if no credentials are configured. """ - # Caller-supplied kwargs win over env-derived values (legacy behaviour - # via ``cls(auth=..., demo=..., base_url=..., **kwargs)`` would have - # TypeError'd on duplicates; setdefault preserves env semantics while - # giving the static signature a clean shape). See #266. - kwargs.setdefault("auth", KalshiAuth.try_from_env()) + # #316: only consult env for credentials when the caller did not supply + # any explicit credential source. ``setdefault("auth", try_from_env())`` + # always evaluates the default, so a malformed ``KALSHI_PRIVATE_KEY`` in + # the process env would raise even when the caller passed ``auth=`` or + # ``key_id``+``private_key``. + caller_supplied_auth = ( + "auth" in kwargs + or kwargs.get("key_id") is not None + or kwargs.get("private_key") is not None + or kwargs.get("private_key_path") is not None + ) + env_built_auth: KalshiAuth | None = None + if not caller_supplied_auth: + env_built_auth = KalshiAuth.try_from_env() + if env_built_auth is not None: + kwargs["auth"] = env_built_auth kwargs.setdefault("demo", os.environ.get("KALSHI_DEMO", "").lower() == "true") kwargs.setdefault("base_url", os.environ.get("KALSHI_API_BASE_URL")) client = cls(**kwargs) - # from_env constructs auth locally — caller never owned it. - client._auth_owned = kwargs["auth"] is not None + # #311: ``__init__`` treats any passed-in ``auth`` as caller-owned. When + # ``from_env`` itself built the auth from env vars, the client (not the + # caller) owns it and must shut it down on ``close()``. The constructor + # is the source of truth for every other path — caller passed ``auth=`` + # (caller keeps ownership), caller passed ``key_id``+``private_key`` + # (client owns), or no credentials at all (nothing to own). + if env_built_auth is not None: + client._auth_owned = True return client def close(self) -> None: diff --git a/tests/test_async_client.py b/tests/test_async_client.py index eb54d594..299bcaac 100644 --- a/tests/test_async_client.py +++ b/tests/test_async_client.py @@ -746,6 +746,79 @@ async def test_from_env_owns_auth( await client.close() +class TestAsyncFromEnvOwnershipAndLazyEval: + """#311 / #316: async mirror — from_env must (a) honor ``__init__``'s + ownership invariant instead of recomputing from the input kwarg, and (b) + only consult ``KALSHI_PRIVATE_KEY`` lazily when the caller did not supply + an explicit credential source. + """ + + @pytest.mark.asyncio + async def test_issue_311_from_env_preserves_caller_owned_auth( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str, test_auth: KalshiAuth + ) -> None: + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", pem_string) + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = AsyncKalshiClient.from_env(auth=test_auth) + assert client._auth is test_auth + assert client._auth_owned is False + await client.close() + headers = test_auth.sign_request("GET", "/trade-api/v2/markets") + assert "KALSHI-ACCESS-KEY" in headers + + @pytest.mark.asyncio + async def test_issue_311_from_env_owns_auth_built_from_key_id( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str + ) -> None: + monkeypatch.delenv("KALSHI_KEY_ID", raising=False) + monkeypatch.delenv("KALSHI_PRIVATE_KEY", raising=False) + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = AsyncKalshiClient.from_env(key_id="caller-key", private_key=pem_string) + assert client._auth is not None + assert client._auth.key_id == "caller-key" + assert client._auth_owned is True + auth = client._auth + await client.close() + # SDK-owned auth has been shut down; ``_closed`` flips True so a + # subsequent ``sign_request_async`` raises (per #267). + assert auth._closed is True + + @pytest.mark.asyncio + async def test_issue_316_from_env_does_not_eagerly_evaluate_env_when_auth_provided( + self, monkeypatch: pytest.MonkeyPatch, test_auth: KalshiAuth + ) -> None: + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", "-----BEGIN GARBAGE-----\nnotapem\n") + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = AsyncKalshiClient.from_env(auth=test_auth) + assert client._auth is test_auth + assert client._auth_owned is False + await client.close() + + @pytest.mark.asyncio + async def test_issue_316_from_env_does_not_eagerly_evaluate_env_when_key_id_provided( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str + ) -> None: + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", "-----BEGIN GARBAGE-----\nnotapem\n") + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = AsyncKalshiClient.from_env(key_id="caller-key", private_key=pem_string) + assert client._auth is not None + assert client._auth.key_id == "caller-key" + assert client._auth_owned is True + await client.close() + + + class TestAsyncTransportNetworkRetry: """#240: async httpx.TransportError retry policy. Mirrors sync tests.""" diff --git a/tests/test_client.py b/tests/test_client.py index fed5ba2c..f5054b76 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1312,6 +1312,86 @@ def test_from_env_no_credentials_owns_nothing(self, monkeypatch: pytest.MonkeyPa client.close() +class TestFromEnvOwnershipAndLazyEval: + """#311 / #316: from_env must (a) honor ``__init__``'s ownership invariant + instead of recomputing from the input kwarg, and (b) only consult + ``KALSHI_PRIVATE_KEY`` lazily when the caller did not supply an explicit + credential source. + """ + + def test_issue_311_from_env_preserves_caller_owned_auth( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str, test_auth: KalshiAuth + ) -> None: + # Case C: env vars set + caller passes auth=. The previous bug flipped + # ``_auth_owned`` to True off the input kwarg and ``close()`` would + # tear down the caller's auth, breaking every subsequent sign request. + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", pem_string) + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = KalshiClient.from_env(auth=test_auth) + assert client._auth is test_auth + assert client._auth_owned is False + client.close() + # Caller-owned auth survives client.close() and can still sign. + headers = test_auth.sign_request("GET", "/trade-api/v2/markets") + assert "KALSHI-ACCESS-KEY" in headers + + def test_issue_311_from_env_owns_auth_built_from_key_id( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str + ) -> None: + # Case B: no env vars, caller passes key_id+private_key. __init__ + # builds auth and is the legitimate owner; the previous override + # recomputed ``_auth_owned`` as ``(None is not None) == False`` and + # leaked the sign ThreadPoolExecutor on close(). + monkeypatch.delenv("KALSHI_KEY_ID", raising=False) + monkeypatch.delenv("KALSHI_PRIVATE_KEY", raising=False) + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = KalshiClient.from_env(key_id="caller-key", private_key=pem_string) + assert client._auth is not None + assert client._auth.key_id == "caller-key" + assert client._auth_owned is True + auth = client._auth + client.close() + # SDK-owned auth has been shut down; ``_closed`` flips True so a + # subsequent ``sign_request_async`` raises (per #267). The sync + # ``sign_request`` path intentionally still works on a retired auth. + assert auth._closed is True + + def test_issue_316_from_env_does_not_eagerly_evaluate_env_when_auth_provided( + self, monkeypatch: pytest.MonkeyPatch, test_auth: KalshiAuth + ) -> None: + # Malformed PEM lingering in env must not block an explicit ``auth=``. + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", "-----BEGIN GARBAGE-----\nnotapem\n") + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = KalshiClient.from_env(auth=test_auth) + assert client._auth is test_auth + assert client._auth_owned is False + client.close() + + def test_issue_316_from_env_does_not_eagerly_evaluate_env_when_key_id_provided( + self, monkeypatch: pytest.MonkeyPatch, pem_string: str + ) -> None: + # A malformed env PEM must not block explicit ``key_id``+``private_key``. + monkeypatch.setenv("KALSHI_KEY_ID", "env-key") + monkeypatch.setenv("KALSHI_PRIVATE_KEY", "-----BEGIN GARBAGE-----\nnotapem\n") + monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) + monkeypatch.delenv("KALSHI_DEMO", raising=False) + monkeypatch.delenv("KALSHI_API_BASE_URL", raising=False) + client = KalshiClient.from_env(key_id="caller-key", private_key=pem_string) + assert client._auth is not None + assert client._auth.key_id == "caller-key" + assert client._auth_owned is True + client.close() + + + class TestExtraHeadersPerRequest: """P1.2: SyncTransport.request accepts per-call ``extra_headers`` that layer on top of config-level ``extra_headers`` but never override the