From d0550079f7eee1924b1cb5031899e6d58d11ea5f Mon Sep 17 00:00:00 2001 From: Ejiro Asiuwhu Date: Thu, 26 Mar 2026 09:01:06 +0100 Subject: [PATCH] fix: address PR review findings from bot analysis - Fix incorrect docstring method names in get_item (set_quantity -> increase_quantity, decrease_quantity, try_decrease_quantity) - Add asyncio.Lock to AsyncJWKSFetcher._fetch_jwks to deduplicate concurrent JWKS fetches on cold/expired cache - Add threading.Lock to SyncJWKSFetcher._fetch_jwks for same reason - Add threading.Lock to _get_or_create_memory_store to prevent race condition in token store registry - Fix README clone URL to point to upstream repo - Fix E2E test SyncJWKSFetcher constructor to use correct signature (jwks_url + http_client instead of project_id + base_url) --- sdks/implementations/python/README.md | 2 +- .../python/src/stack_auth/_app.py | 6 ++- .../python/src/stack_auth/_jwt.py | 50 ++++++++++++------- .../python/src/stack_auth/_token_store.py | 16 ++++-- sdks/implementations/python/tests/test_e2e.py | 14 ++++-- 5 files changed, 59 insertions(+), 29 deletions(-) diff --git a/sdks/implementations/python/README.md b/sdks/implementations/python/README.md index c1cb25cb7..af23c5134 100644 --- a/sdks/implementations/python/README.md +++ b/sdks/implementations/python/README.md @@ -192,7 +192,7 @@ app = StackServerApp( ```bash # Clone the repo -git clone https://github.com/ejirocodes/stack-auth.git +git clone https://github.com/stack-auth/stack-auth.git cd stack-auth/sdks/implementations/python # Install in dev mode diff --git a/sdks/implementations/python/src/stack_auth/_app.py b/sdks/implementations/python/src/stack_auth/_app.py index 3f8b6426d..29ee9bf84 100644 --- a/sdks/implementations/python/src/stack_auth/_app.py +++ b/sdks/implementations/python/src/stack_auth/_app.py @@ -1235,7 +1235,8 @@ class StackServerApp: Returns: A :class:`ServerItem` wrapping the :class:`Item` data and - providing ``set_quantity`` and ``increment_quantity`` methods. + providing ``increase_quantity``, ``decrease_quantity``, and + ``try_decrease_quantity`` methods. Raises: ValueError: If not exactly one customer identifier is provided. @@ -2570,7 +2571,8 @@ class AsyncStackServerApp: Returns: An :class:`AsyncServerItem` wrapping the :class:`Item` data and - providing ``set_quantity`` and ``increment_quantity`` coroutines. + providing ``increase_quantity``, ``decrease_quantity``, and + ``try_decrease_quantity`` coroutines. Raises: ValueError: If not exactly one customer identifier is provided. diff --git a/sdks/implementations/python/src/stack_auth/_jwt.py b/sdks/implementations/python/src/stack_auth/_jwt.py index 04be2eb18..121d9703a 100644 --- a/sdks/implementations/python/src/stack_auth/_jwt.py +++ b/sdks/implementations/python/src/stack_auth/_jwt.py @@ -7,6 +7,8 @@ to prevent CVE-2022-29217 style algorithm confusion attacks. from __future__ import annotations +import asyncio +import threading import time from typing import Any @@ -34,6 +36,7 @@ class AsyncJWKSFetcher: self._http_client = http_client self._cache: dict[str, Any] | None = None self._cache_time: float = 0.0 + self._fetch_lock = asyncio.Lock() async def get_signing_key(self, kid: str) -> Any: """Return the RSA public key for the given key ID. @@ -55,16 +58,21 @@ class AsyncJWKSFetcher: return RSAAlgorithm.from_jwk(key_data) async def _fetch_jwks(self, force: bool = False) -> dict[str, Any]: - """Fetch JWKS from the endpoint, using cache if fresh.""" - now = time.monotonic() - if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL: - return self._cache + """Fetch JWKS from the endpoint, using cache if fresh. - response = await self._http_client.get(self._jwks_url) - response.raise_for_status() - self._cache = response.json() - self._cache_time = time.monotonic() - return self._cache # type: ignore[return-value] + Uses an asyncio.Lock to deduplicate concurrent fetches so only one + HTTP request is made when multiple coroutines hit a cold or expired cache. + """ + async with self._fetch_lock: + now = time.monotonic() + if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL: + return self._cache + + response = await self._http_client.get(self._jwks_url) + response.raise_for_status() + self._cache = response.json() + self._cache_time = time.monotonic() + return self._cache # type: ignore[return-value] class SyncJWKSFetcher: @@ -80,6 +88,7 @@ class SyncJWKSFetcher: self._http_client = http_client self._cache: dict[str, Any] | None = None self._cache_time: float = 0.0 + self._fetch_lock = threading.Lock() def get_signing_key(self, kid: str) -> Any: """Return the RSA public key for the given key ID. @@ -100,16 +109,21 @@ class SyncJWKSFetcher: return RSAAlgorithm.from_jwk(key_data) def _fetch_jwks(self, force: bool = False) -> dict[str, Any]: - """Fetch JWKS from the endpoint, using cache if fresh.""" - now = time.monotonic() - if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL: - return self._cache + """Fetch JWKS from the endpoint, using cache if fresh. - response = self._http_client.get(self._jwks_url) - response.raise_for_status() - self._cache = response.json() - self._cache_time = time.monotonic() - return self._cache # type: ignore[return-value] + Uses a threading.Lock to deduplicate concurrent fetches so only one + HTTP request is made when multiple threads hit a cold or expired cache. + """ + with self._fetch_lock: + now = time.monotonic() + if not force and self._cache is not None and (now - self._cache_time) < JWKS_CACHE_TTL: + return self._cache + + response = self._http_client.get(self._jwks_url) + response.raise_for_status() + self._cache = response.json() + self._cache_time = time.monotonic() + return self._cache # type: ignore[return-value] def _find_key(jwks: dict[str, Any], kid: str) -> dict[str, Any] | None: diff --git a/sdks/implementations/python/src/stack_auth/_token_store.py b/sdks/implementations/python/src/stack_auth/_token_store.py index 9b0d2211d..6f38ddfe8 100644 --- a/sdks/implementations/python/src/stack_auth/_token_store.py +++ b/sdks/implementations/python/src/stack_auth/_token_store.py @@ -177,12 +177,20 @@ ensures that invariant. To reset in tests: ``stack_auth._token_store._token_store_registry.clear()`` """ +_registry_lock = threading.Lock() +"""Guards _token_store_registry against concurrent check-then-set races.""" + def _get_or_create_memory_store(project_id: str) -> MemoryTokenStore: - """Return the shared MemoryTokenStore for a project, creating one if needed.""" - if project_id not in _token_store_registry: - _token_store_registry[project_id] = MemoryTokenStore() - return _token_store_registry[project_id] + """Return the shared MemoryTokenStore for a project, creating one if needed. + + Thread-safe: uses _registry_lock to prevent two threads from creating + duplicate stores for the same project_id. + """ + with _registry_lock: + if project_id not in _token_store_registry: + _token_store_registry[project_id] = MemoryTokenStore() + return _token_store_registry[project_id] # --------------------------------------------------------------------------- diff --git a/sdks/implementations/python/tests/test_e2e.py b/sdks/implementations/python/tests/test_e2e.py index fe64a6483..9ab492594 100644 --- a/sdks/implementations/python/tests/test_e2e.py +++ b/sdks/implementations/python/tests/test_e2e.py @@ -302,12 +302,15 @@ class TestAuthenticateRequestE2E: """ def test_unauthenticated_request_without_token(self, app: StackServerApp) -> None: + import httpx as httpx_client + from stack_auth._auth import sync_authenticate_request from stack_auth._jwt import SyncJWKSFetcher + jwks_url = f"{BASE_URL}/api/v1/projects/{PROJECT_ID}/.well-known/jwks.json" fetcher = SyncJWKSFetcher( - project_id=PROJECT_ID, - base_url=BASE_URL, + jwks_url=jwks_url, + http_client=httpx_client.Client(), ) class FakeRequest: @@ -320,12 +323,15 @@ class TestAuthenticateRequestE2E: assert result.status == "unauthenticated" def test_invalid_token_returns_unauthenticated(self, app: StackServerApp) -> None: + import httpx as httpx_client + from stack_auth._auth import sync_authenticate_request from stack_auth._jwt import SyncJWKSFetcher + jwks_url = f"{BASE_URL}/api/v1/projects/{PROJECT_ID}/.well-known/jwks.json" fetcher = SyncJWKSFetcher( - project_id=PROJECT_ID, - base_url=BASE_URL, + jwks_url=jwks_url, + http_client=httpx_client.Client(), ) class FakeRequest: