diff --git a/sdk-compliance.yaml b/sdk-compliance.yaml index 9345a3944..ce728681c 100644 --- a/sdk-compliance.yaml +++ b/sdk-compliance.yaml @@ -68,6 +68,12 @@ features: symbols: - AsyncGoTrueClient.exchange_code_for_session - SyncGoTrueClient.exchange_code_for_session + auth.sign_in.concurrent_pkce_flows: + status: implemented + symbols: + - OAuthResponse.flow_id + - AsyncGoTrueClient.exchange_code_for_session + - SyncGoTrueClient.exchange_code_for_session auth.session.get_session: status: implemented diff --git a/src/auth/scripts/run-unasync.py b/src/auth/scripts/run-unasync.py index 73f8986b4..360f6f718 100644 --- a/src/auth/scripts/run-unasync.py +++ b/src/auth/scripts/run-unasync.py @@ -9,7 +9,12 @@ unasync.Rule( fromdir="/_async/", todir="/_sync/", - additional_replacements={"AsyncClient": "Client", "aclose": "close"}, + additional_replacements={ + "AsyncClient": "Client", + "aclose": "close", + # asyncio is only used for the Lock in the PKCE verifier store + "asyncio": "threading", + }, ), unasync._DEFAULT_RULE, ) diff --git a/src/auth/src/supabase_auth/_async/gotrue_client.py b/src/auth/src/supabase_auth/_async/gotrue_client.py index d7facd8cc..dd091abe1 100644 --- a/src/auth/src/supabase_auth/_async/gotrue_client.py +++ b/src/auth/src/supabase_auth/_async/gotrue_client.py @@ -24,6 +24,7 @@ AuthImplicitGrantRedirectError, AuthInvalidCredentialsError, AuthInvalidJwtError, + AuthPKCECodeVerifierMissingError, AuthRetryableError, AuthSessionMissingError, UserDoesntExist, @@ -31,6 +32,7 @@ from ..helpers import ( decode_jwt, generate_pkce_challenge, + generate_pkce_flow_id, generate_pkce_verifier, model_dump_json, model_validate, @@ -41,6 +43,7 @@ parse_sso_response, parse_user_response, validate_exp, + validate_pkce_flow_id, ) from ..timer import Timer from ..types import ( @@ -94,6 +97,7 @@ from .gotrue_admin_api import AsyncGoTrueAdminAPI from .gotrue_base_api import AsyncGoTrueBaseAPI from .gotrue_mfa_api import AsyncGoTrueMFAAPI +from .pkce_verifier_store import AsyncPKCEVerifierStore from .storage import AsyncMemoryStorage, AsyncSupportedStorage @@ -153,6 +157,9 @@ def __init__( self._network_retries = 0 self._state_change_emitters: Dict[str, Subscription] = {} self._flow_type = flow_type + self._pkce_verifier_store = AsyncPKCEVerifierStore( + self._storage, self._storage_key + ) self.admin = AsyncGoTrueAdminAPI( url=self._url, @@ -443,6 +450,11 @@ async def sign_in_with_oauth( ) -> OAuthResponse: """ Log in an existing user via a third-party provider. + + On the PKCE flow the returned ``flow_id`` identifies the code verifier + stored for this call. Pass it to ``exchange_code_for_session`` so that + the exchange keeps working even when other PKCE flows are started before + this one completes. """ await self._remove_session() @@ -455,10 +467,10 @@ async def sign_in_with_oauth( params["redirect_to"] = redirect_to if scopes: params["scopes"] = scopes - url_with_qs, _ = await self._get_url_for_provider( + url_with_qs, _, flow_id = await self._get_url_for_provider( f"{self._url}/authorize", provider, params ) - return OAuthResponse(provider=provider, url=url_with_qs) + return OAuthResponse(provider=provider, url=url_with_qs, flow_id=flow_id) async def link_identity( self, credentials: SignInWithOAuthCredentials @@ -474,7 +486,7 @@ async def link_identity( params["scopes"] = scopes params["skip_http_redirect"] = "true" url = "user/identities/authorize" - _, query = await self._get_url_for_provider(url, provider, params) + _, query, flow_id = await self._get_url_for_provider(url, provider, params) session = await self.get_session() if not session: @@ -487,7 +499,7 @@ async def link_identity( jwt=session.access_token, ) link_identity = parse_link_identity_response(response) - return OAuthResponse(provider=provider, url=link_identity.url) + return OAuthResponse(provider=provider, url=link_identity.url, flow_id=flow_id) async def get_user_identities(self) -> IdentitiesResponse: response = await self.get_user() @@ -801,6 +813,7 @@ async def sign_out(self, options: Optional[SignOutOptions] = None) -> None: if signout_options["scope"] != "others": await self._remove_session() + await self._pkce_verifier_store.remove_all() self._notify_all_subscribers("SIGNED_OUT", None) def on_auth_state_change( @@ -1173,14 +1186,22 @@ async def _get_url_for_provider( url: str, provider: Provider, params: Dict[str, str], - ) -> Tuple[str, QueryParams]: + ) -> Tuple[str, QueryParams, Optional[str]]: + """ + Build the authorize URL for ``provider``. + + On the PKCE flow a fresh code verifier is generated and stored in a + slot of its own, identified by the returned flow id, so that several + flows can be pending at the same time. Returns ``None`` as the flow id + on the implicit flow. + """ query = QueryParams(params) + flow_id: Optional[str] = None if self._flow_type == "pkce": code_verifier = generate_pkce_verifier() code_challenge = generate_pkce_challenge(code_verifier) - await self._storage.set_item( - f"{self._storage_key}-code-verifier", code_verifier - ) + flow_id = generate_pkce_flow_id() + await self._pkce_verifier_store.store(flow_id, code_verifier) code_challenge_method = ( "plain" if code_verifier == code_challenge else "s256" ) @@ -1188,26 +1209,48 @@ async def _get_url_for_provider( "code_challenge_method", code_challenge_method ) query = query.set("provider", provider) - return f"{url}?{query}", query + return f"{url}?{query}", query, flow_id async def exchange_code_for_session( self, params: CodeExchangeParams ) -> AuthResponse: - code_verifier = params.get("code_verifier") or await self._storage.get_item( - f"{self._storage_key}-code-verifier" - ) - response = await self._request( - "POST", - "token", - query=QueryParams(grant_type="pkce"), - body={ - "auth_code": params.get("auth_code"), - "code_verifier": code_verifier, - }, - redirect_to=params.get("redirect_to"), - ) + """ + Exchange an authorization code obtained through the PKCE flow for a session. + + Pass the ``flow_id`` returned by ``sign_in_with_oauth`` or + ``link_identity`` to use the code verifier of that specific flow. With a + flow id, only that flow's verifier is used; if it is missing the + exchange fails with ``AuthPKCECodeVerifierMissingError`` rather than + spending the single-use code on another flow's verifier. Without a + flow id the verifier of the most recently started flow is used. + """ + flow_id: Optional[str] = None + requested_flow_id = params.get("flow_id") + if requested_flow_id is not None: + flow_id = validate_pkce_flow_id(requested_flow_id) + if flow_id is None: + raise AuthPKCECodeVerifierMissingError() + + code_verifier = params.get("code_verifier") + if not code_verifier: + code_verifier = await self._pkce_verifier_store.retrieve(flow_id) + if flow_id is not None and code_verifier is None: + raise AuthPKCECodeVerifierMissingError() + + try: + response = await self._request( + "POST", + "token", + query=QueryParams(grant_type="pkce"), + body={ + "auth_code": params.get("auth_code"), + "code_verifier": code_verifier, + }, + redirect_to=params.get("redirect_to"), + ) + finally: + await self._pkce_verifier_store.remove(flow_id) auth_response = parse_auth_response(response) - await self._storage.remove_item(f"{self._storage_key}-code-verifier") if auth_response.session: await self._save_session(auth_response.session) self._notify_all_subscribers("SIGNED_IN", auth_response.session) diff --git a/src/auth/src/supabase_auth/_async/pkce_verifier_store.py b/src/auth/src/supabase_auth/_async/pkce_verifier_store.py new file mode 100644 index 000000000..1d3b9d158 --- /dev/null +++ b/src/auth/src/supabase_auth/_async/pkce_verifier_store.py @@ -0,0 +1,147 @@ +from __future__ import annotations + +import json +from asyncio import Lock +from typing import List, Optional + +from ..constants import PKCE_MAX_CONCURRENT_FLOWS +from ..helpers import ( + pkce_flow_index_key, + pkce_legacy_verifier_key, + pkce_verifier_slot_key, + validate_pkce_flow_id, +) +from .storage import AsyncSupportedStorage + + +class AsyncPKCEVerifierStore: + """Keeps one PKCE code verifier per pending flow. + + Each flow gets its own storage slot keyed by a flow id, so starting a + second PKCE flow no longer overwrites the verifier of the first one. The + store is bounded: an ordered index of pending flow ids is kept in storage + (storage backends cannot enumerate keys) and the oldest flow is evicted + once more than ``PKCE_MAX_CONCURRENT_FLOWS`` are pending. + + The single legacy key (``{storage_key}-code-verifier``) is still written + with the verifier of the most recently started flow and is used whenever a + caller does not know its flow id, which keeps older call sites working. + """ + + def __init__(self, storage: AsyncSupportedStorage, storage_key: str) -> None: + self._storage = storage + self._storage_key = storage_key + # The index is read, modified and written back across several awaits; + # without serialization two concurrent flow starts could each write a + # different index and one slot would become unreachable for eviction + # and clean-up. + self._lock = Lock() + + @property + def legacy_key(self) -> str: + return pkce_legacy_verifier_key(self._storage_key) + + @property + def index_key(self) -> str: + return pkce_flow_index_key(self._storage_key) + + def slot_key(self, flow_id: str) -> str: + return pkce_verifier_slot_key(self._storage_key, flow_id) + + async def _read_index(self) -> List[str]: + raw = await self._storage.get_item(self.index_key) + if not raw: + return [] + try: + parsed = json.loads(raw) + except ValueError: + return [] + if not isinstance(parsed, list): + return [] + # Ids read back from storage are as untrusted as caller input. + return [ + flow_id + for flow_id in (validate_pkce_flow_id(item) for item in parsed) + if flow_id is not None + ] + + async def _write_index(self, index: List[str]) -> None: + if index: + await self._storage.set_item(self.index_key, json.dumps(index)) + else: + await self._storage.remove_item(self.index_key) + + async def store(self, flow_id: str, code_verifier: str) -> List[str]: + """Store ``code_verifier`` for ``flow_id`` and return any evicted flow ids. + + Raises ``ValueError`` for a malformed flow id: a slot the index could + not validate on the way back out could never be evicted or removed. + """ + if validate_pkce_flow_id(flow_id) is None: + raise ValueError("Invalid PKCE flow id") + + async with self._lock: + await self._storage.set_item(self.slot_key(flow_id), code_verifier) + + index = [item for item in await self._read_index() if item != flow_id] + index.append(flow_id) + evicted: List[str] = [] + while len(index) > PKCE_MAX_CONCURRENT_FLOWS: + oldest = index.pop(0) + await self._storage.remove_item(self.slot_key(oldest)) + evicted.append(oldest) + await self._write_index(index) + + # Mirror the newest verifier into the legacy key for callers that + # never learned their flow id. + await self._storage.set_item(self.legacy_key, code_verifier) + return evicted + + async def retrieve(self, flow_id: Optional[str]) -> Optional[str]: + """Return the verifier for ``flow_id``, or the legacy verifier if it is ``None``. + + With a flow id only that flow's slot is consulted. Falling back to + another flow's verifier would spend the single-use auth code on a + request that cannot succeed, so a miss is reported as ``None`` instead. + """ + if flow_id is None: + return await self._storage.get_item(self.legacy_key) + if validate_pkce_flow_id(flow_id) is None: + return None + return await self._storage.get_item(self.slot_key(flow_id)) + + async def remove(self, flow_id: Optional[str]) -> None: + """Forget the verifier of ``flow_id``, or only the legacy verifier if it is ``None``. + + The legacy key is cleared alongside a slot only when it still holds + that slot's verifier, so removing a finished flow never destroys the + fallback of a newer pending flow. + """ + async with self._lock: + if flow_id is None: + await self._storage.remove_item(self.legacy_key) + return + if validate_pkce_flow_id(flow_id) is None: + return + + slot_key = self.slot_key(flow_id) + removed_verifier = await self._storage.get_item(slot_key) + await self._storage.remove_item(slot_key) + + index = await self._read_index() + remaining = [item for item in index if item != flow_id] + if len(remaining) != len(index): + await self._write_index(remaining) + + if removed_verifier is not None: + legacy_verifier = await self._storage.get_item(self.legacy_key) + if legacy_verifier == removed_verifier: + await self._storage.remove_item(self.legacy_key) + + async def remove_all(self) -> None: + """Forget every pending verifier, the index and the legacy verifier.""" + async with self._lock: + for flow_id in await self._read_index(): + await self._storage.remove_item(self.slot_key(flow_id)) + await self._storage.remove_item(self.index_key) + await self._storage.remove_item(self.legacy_key) diff --git a/src/auth/src/supabase_auth/_sync/gotrue_client.py b/src/auth/src/supabase_auth/_sync/gotrue_client.py index b3852aa5a..3384ecd13 100644 --- a/src/auth/src/supabase_auth/_sync/gotrue_client.py +++ b/src/auth/src/supabase_auth/_sync/gotrue_client.py @@ -24,6 +24,7 @@ AuthImplicitGrantRedirectError, AuthInvalidCredentialsError, AuthInvalidJwtError, + AuthPKCECodeVerifierMissingError, AuthRetryableError, AuthSessionMissingError, UserDoesntExist, @@ -31,6 +32,7 @@ from ..helpers import ( decode_jwt, generate_pkce_challenge, + generate_pkce_flow_id, generate_pkce_verifier, model_dump_json, model_validate, @@ -41,6 +43,7 @@ parse_sso_response, parse_user_response, validate_exp, + validate_pkce_flow_id, ) from ..timer import Timer from ..types import ( @@ -94,6 +97,7 @@ from .gotrue_admin_api import SyncGoTrueAdminAPI from .gotrue_base_api import SyncGoTrueBaseAPI from .gotrue_mfa_api import SyncGoTrueMFAAPI +from .pkce_verifier_store import SyncPKCEVerifierStore from .storage import SyncMemoryStorage, SyncSupportedStorage @@ -153,6 +157,9 @@ def __init__( self._network_retries = 0 self._state_change_emitters: Dict[str, Subscription] = {} self._flow_type = flow_type + self._pkce_verifier_store = SyncPKCEVerifierStore( + self._storage, self._storage_key + ) self.admin = SyncGoTrueAdminAPI( url=self._url, @@ -441,6 +448,11 @@ def sign_in_with_oauth( ) -> OAuthResponse: """ Log in an existing user via a third-party provider. + + On the PKCE flow the returned ``flow_id`` identifies the code verifier + stored for this call. Pass it to ``exchange_code_for_session`` so that + the exchange keeps working even when other PKCE flows are started before + this one completes. """ self._remove_session() @@ -453,10 +465,10 @@ def sign_in_with_oauth( params["redirect_to"] = redirect_to if scopes: params["scopes"] = scopes - url_with_qs, _ = self._get_url_for_provider( + url_with_qs, _, flow_id = self._get_url_for_provider( f"{self._url}/authorize", provider, params ) - return OAuthResponse(provider=provider, url=url_with_qs) + return OAuthResponse(provider=provider, url=url_with_qs, flow_id=flow_id) def link_identity(self, credentials: SignInWithOAuthCredentials) -> OAuthResponse: provider = credentials["provider"] @@ -470,7 +482,7 @@ def link_identity(self, credentials: SignInWithOAuthCredentials) -> OAuthRespons params["scopes"] = scopes params["skip_http_redirect"] = "true" url = "user/identities/authorize" - _, query = self._get_url_for_provider(url, provider, params) + _, query, flow_id = self._get_url_for_provider(url, provider, params) session = self.get_session() if not session: @@ -483,7 +495,7 @@ def link_identity(self, credentials: SignInWithOAuthCredentials) -> OAuthRespons jwt=session.access_token, ) link_identity = parse_link_identity_response(response) - return OAuthResponse(provider=provider, url=link_identity.url) + return OAuthResponse(provider=provider, url=link_identity.url, flow_id=flow_id) def get_user_identities(self) -> IdentitiesResponse: response = self.get_user() @@ -795,6 +807,7 @@ def sign_out(self, options: Optional[SignOutOptions] = None) -> None: if signout_options["scope"] != "others": self._remove_session() + self._pkce_verifier_store.remove_all() self._notify_all_subscribers("SIGNED_OUT", None) def on_auth_state_change( @@ -1167,12 +1180,22 @@ def _get_url_for_provider( url: str, provider: Provider, params: Dict[str, str], - ) -> Tuple[str, QueryParams]: + ) -> Tuple[str, QueryParams, Optional[str]]: + """ + Build the authorize URL for ``provider``. + + On the PKCE flow a fresh code verifier is generated and stored in a + slot of its own, identified by the returned flow id, so that several + flows can be pending at the same time. Returns ``None`` as the flow id + on the implicit flow. + """ query = QueryParams(params) + flow_id: Optional[str] = None if self._flow_type == "pkce": code_verifier = generate_pkce_verifier() code_challenge = generate_pkce_challenge(code_verifier) - self._storage.set_item(f"{self._storage_key}-code-verifier", code_verifier) + flow_id = generate_pkce_flow_id() + self._pkce_verifier_store.store(flow_id, code_verifier) code_challenge_method = ( "plain" if code_verifier == code_challenge else "s256" ) @@ -1180,24 +1203,46 @@ def _get_url_for_provider( "code_challenge_method", code_challenge_method ) query = query.set("provider", provider) - return f"{url}?{query}", query + return f"{url}?{query}", query, flow_id def exchange_code_for_session(self, params: CodeExchangeParams) -> AuthResponse: - code_verifier = params.get("code_verifier") or self._storage.get_item( - f"{self._storage_key}-code-verifier" - ) - response = self._request( - "POST", - "token", - query=QueryParams(grant_type="pkce"), - body={ - "auth_code": params.get("auth_code"), - "code_verifier": code_verifier, - }, - redirect_to=params.get("redirect_to"), - ) + """ + Exchange an authorization code obtained through the PKCE flow for a session. + + Pass the ``flow_id`` returned by ``sign_in_with_oauth`` or + ``link_identity`` to use the code verifier of that specific flow. With a + flow id, only that flow's verifier is used; if it is missing the + exchange fails with ``AuthPKCECodeVerifierMissingError`` rather than + spending the single-use code on another flow's verifier. Without a + flow id the verifier of the most recently started flow is used. + """ + flow_id: Optional[str] = None + requested_flow_id = params.get("flow_id") + if requested_flow_id is not None: + flow_id = validate_pkce_flow_id(requested_flow_id) + if flow_id is None: + raise AuthPKCECodeVerifierMissingError() + + code_verifier = params.get("code_verifier") + if not code_verifier: + code_verifier = self._pkce_verifier_store.retrieve(flow_id) + if flow_id is not None and code_verifier is None: + raise AuthPKCECodeVerifierMissingError() + + try: + response = self._request( + "POST", + "token", + query=QueryParams(grant_type="pkce"), + body={ + "auth_code": params.get("auth_code"), + "code_verifier": code_verifier, + }, + redirect_to=params.get("redirect_to"), + ) + finally: + self._pkce_verifier_store.remove(flow_id) auth_response = parse_auth_response(response) - self._storage.remove_item(f"{self._storage_key}-code-verifier") if auth_response.session: self._save_session(auth_response.session) self._notify_all_subscribers("SIGNED_IN", auth_response.session) diff --git a/src/auth/src/supabase_auth/_sync/pkce_verifier_store.py b/src/auth/src/supabase_auth/_sync/pkce_verifier_store.py new file mode 100644 index 000000000..3d8309247 --- /dev/null +++ b/src/auth/src/supabase_auth/_sync/pkce_verifier_store.py @@ -0,0 +1,147 @@ +from __future__ import annotations + +import json +from threading import Lock +from typing import List, Optional + +from ..constants import PKCE_MAX_CONCURRENT_FLOWS +from ..helpers import ( + pkce_flow_index_key, + pkce_legacy_verifier_key, + pkce_verifier_slot_key, + validate_pkce_flow_id, +) +from .storage import SyncSupportedStorage + + +class SyncPKCEVerifierStore: + """Keeps one PKCE code verifier per pending flow. + + Each flow gets its own storage slot keyed by a flow id, so starting a + second PKCE flow no longer overwrites the verifier of the first one. The + store is bounded: an ordered index of pending flow ids is kept in storage + (storage backends cannot enumerate keys) and the oldest flow is evicted + once more than ``PKCE_MAX_CONCURRENT_FLOWS`` are pending. + + The single legacy key (``{storage_key}-code-verifier``) is still written + with the verifier of the most recently started flow and is used whenever a + caller does not know its flow id, which keeps older call sites working. + """ + + def __init__(self, storage: SyncSupportedStorage, storage_key: str) -> None: + self._storage = storage + self._storage_key = storage_key + # The index is read, modified and written back across several awaits; + # without serialization two concurrent flow starts could each write a + # different index and one slot would become unreachable for eviction + # and clean-up. + self._lock = Lock() + + @property + def legacy_key(self) -> str: + return pkce_legacy_verifier_key(self._storage_key) + + @property + def index_key(self) -> str: + return pkce_flow_index_key(self._storage_key) + + def slot_key(self, flow_id: str) -> str: + return pkce_verifier_slot_key(self._storage_key, flow_id) + + def _read_index(self) -> List[str]: + raw = self._storage.get_item(self.index_key) + if not raw: + return [] + try: + parsed = json.loads(raw) + except ValueError: + return [] + if not isinstance(parsed, list): + return [] + # Ids read back from storage are as untrusted as caller input. + return [ + flow_id + for flow_id in (validate_pkce_flow_id(item) for item in parsed) + if flow_id is not None + ] + + def _write_index(self, index: List[str]) -> None: + if index: + self._storage.set_item(self.index_key, json.dumps(index)) + else: + self._storage.remove_item(self.index_key) + + def store(self, flow_id: str, code_verifier: str) -> List[str]: + """Store ``code_verifier`` for ``flow_id`` and return any evicted flow ids. + + Raises ``ValueError`` for a malformed flow id: a slot the index could + not validate on the way back out could never be evicted or removed. + """ + if validate_pkce_flow_id(flow_id) is None: + raise ValueError("Invalid PKCE flow id") + + with self._lock: + self._storage.set_item(self.slot_key(flow_id), code_verifier) + + index = [item for item in self._read_index() if item != flow_id] + index.append(flow_id) + evicted: List[str] = [] + while len(index) > PKCE_MAX_CONCURRENT_FLOWS: + oldest = index.pop(0) + self._storage.remove_item(self.slot_key(oldest)) + evicted.append(oldest) + self._write_index(index) + + # Mirror the newest verifier into the legacy key for callers that + # never learned their flow id. + self._storage.set_item(self.legacy_key, code_verifier) + return evicted + + def retrieve(self, flow_id: Optional[str]) -> Optional[str]: + """Return the verifier for ``flow_id``, or the legacy verifier if it is ``None``. + + With a flow id only that flow's slot is consulted. Falling back to + another flow's verifier would spend the single-use auth code on a + request that cannot succeed, so a miss is reported as ``None`` instead. + """ + if flow_id is None: + return self._storage.get_item(self.legacy_key) + if validate_pkce_flow_id(flow_id) is None: + return None + return self._storage.get_item(self.slot_key(flow_id)) + + def remove(self, flow_id: Optional[str]) -> None: + """Forget the verifier of ``flow_id``, or only the legacy verifier if it is ``None``. + + The legacy key is cleared alongside a slot only when it still holds + that slot's verifier, so removing a finished flow never destroys the + fallback of a newer pending flow. + """ + with self._lock: + if flow_id is None: + self._storage.remove_item(self.legacy_key) + return + if validate_pkce_flow_id(flow_id) is None: + return + + slot_key = self.slot_key(flow_id) + removed_verifier = self._storage.get_item(slot_key) + self._storage.remove_item(slot_key) + + index = self._read_index() + remaining = [item for item in index if item != flow_id] + if len(remaining) != len(index): + self._write_index(remaining) + + if removed_verifier is not None: + legacy_verifier = self._storage.get_item(self.legacy_key) + if legacy_verifier == removed_verifier: + self._storage.remove_item(self.legacy_key) + + def remove_all(self) -> None: + """Forget every pending verifier, the index and the legacy verifier.""" + with self._lock: + for flow_id in self._read_index(): + self._storage.remove_item(self.slot_key(flow_id)) + self._storage.remove_item(self.index_key) + self._storage.remove_item(self.legacy_key) diff --git a/src/auth/src/supabase_auth/constants.py b/src/auth/src/supabase_auth/constants.py index 538cea4c2..1ab48d6bf 100644 --- a/src/auth/src/supabase_auth/constants.py +++ b/src/auth/src/supabase_auth/constants.py @@ -8,6 +8,15 @@ RETRY_INTERVAL = 2 # deciseconds STORAGE_KEY = "supabase.auth.token" +# PKCE code verifiers are stored in one slot per flow so that overlapping flows +# (two OAuth sign-ins, or an OAuth flow started while another is pending) do +# not overwrite each other. The ring is bounded; the oldest pending flow is +# evicted once this many flows are in progress. +PKCE_MAX_CONCURRENT_FLOWS = 5 +# Flow ids are concatenated into storage keys, so anything that reaches a key +# (caller-supplied ids and ids read back from storage) must match this shape. +PKCE_FLOW_ID_PATTERN = r"^[a-zA-Z0-9_-]{8,64}$" + API_VERSION_HEADER_NAME = "X-Supabase-Api-Version" API_VERSIONS_2024_01_01_TIMESTAMP = datetime.timestamp( datetime.strptime("2024-01-01", "%Y-%m-%d") diff --git a/src/auth/src/supabase_auth/errors.py b/src/auth/src/supabase_auth/errors.py index e7a105396..2c0c91634 100644 --- a/src/auth/src/supabase_auth/errors.py +++ b/src/auth/src/supabase_auth/errors.py @@ -70,6 +70,7 @@ "over_email_send_rate_limit", "over_sms_send_rate_limit", "bad_code_verifier", + "pkce_code_verifier_not_found", "anonymous_provider_disabled", "hook_timeout", "hook_timeout_after_retry", @@ -161,6 +162,20 @@ def __init__(self) -> None: ) +class AuthPKCECodeVerifierMissingError(CustomAuthError): + def __init__(self) -> None: + CustomAuthError.__init__( + self, + "PKCE code verifier not found in storage. This can happen if the " + "auth flow was started with a different client or storage, if the " + "storage was cleared, or if the flow id does not match a pending " + "flow.", + "AuthPKCECodeVerifierMissingError", + 400, + "pkce_code_verifier_not_found", + ) + + class AuthInvalidCredentialsError(CustomAuthError): def __init__(self, message: str) -> None: CustomAuthError.__init__( diff --git a/src/auth/src/supabase_auth/helpers.py b/src/auth/src/supabase_auth/helpers.py index fbaf4daeb..87a166a27 100644 --- a/src/auth/src/supabase_auth/helpers.py +++ b/src/auth/src/supabase_auth/helpers.py @@ -18,6 +18,7 @@ from .constants import ( API_VERSION_HEADER_NAME, API_VERSIONS_2024_01_01_TIMESTAMP, + PKCE_FLOW_ID_PATTERN, ) from .errors import ( AuthApiError, @@ -260,6 +261,42 @@ def generate_pkce_challenge(code_verifier) -> str: return base64.urlsafe_b64encode(sha256_hash).rstrip(b"=").decode("utf-8") +def generate_pkce_flow_id() -> str: + """Generate an identifier for one PKCE flow (32 lowercase hex characters).""" + return secrets.token_hex(16) + + +def validate_pkce_flow_id(flow_id: Any) -> Optional[str]: + """Return ``flow_id`` if it is safe to embed in a storage key, else ``None``. + + Flow ids come from callers and from values read back out of storage, so + they are treated as untrusted input everywhere. + """ + if isinstance(flow_id, str) and re.match(PKCE_FLOW_ID_PATTERN, flow_id): + return flow_id + return None + + +def pkce_legacy_verifier_key(storage_key: str) -> str: + """Storage key of the single shared code verifier used before per-flow slots.""" + return f"{storage_key}-code-verifier" + + +def pkce_verifier_slot_key(storage_key: str, flow_id: str) -> str: + """Storage key holding the code verifier of one PKCE flow. + + The key deliberately ends in ``-code-verifier`` and contains no dot so that + it is handled like the legacy key by storage adapters that special-case + verifier keys (for example cookie based adapters). + """ + return f"{storage_key}-flow-{flow_id}-code-verifier" + + +def pkce_flow_index_key(storage_key: str) -> str: + """Storage key of the ordered list of pending PKCE flow ids (oldest first).""" + return f"{storage_key}-flows-code-verifier" + + API_VERSION_REGEX = r"^2[0-9]{3}-(0[1-9]|1[0-2])-(0[1-9]|1[0-9]|2[0-9]|3[0-1])$" diff --git a/src/auth/src/supabase_auth/types.py b/src/auth/src/supabase_auth/types.py index 1968c9960..cc76c713d 100644 --- a/src/auth/src/supabase_auth/types.py +++ b/src/auth/src/supabase_auth/types.py @@ -116,6 +116,13 @@ class AuthOtpResponse(BaseModel): class OAuthResponse(BaseModel): provider: Provider url: str + flow_id: Optional[str] = None + """ + Identifier of the PKCE flow started by this call. Pass it as ``flow_id`` to + ``exchange_code_for_session`` so the exchange uses this flow's code verifier + even when other PKCE flows were started in between. ``None`` on the implicit + flow. + """ class SSOResponse(BaseModel): @@ -562,18 +569,25 @@ class MFAUnenrollParams(TypedDict): class CodeExchangeParams(TypedDict): - code_verifier: str + code_verifier: NotRequired[str] """ - Randomly generated string + Randomly generated string. Read from storage when omitted. """ auth_code: str """ Code returned after completing one of the authorization flows """ - redirect_to: str + redirect_to: NotRequired[str] """ The URL to route to after a session is successfully obtained """ + flow_id: NotRequired[str] + """ + Identifier of the PKCE flow that produced ``auth_code``, as returned in + ``OAuthResponse.flow_id``. Selects that flow's stored code verifier so that + overlapping PKCE flows do not interfere. When omitted, the verifier of the + most recently started flow is used. + """ class MFAVerifyParams(TypedDict): diff --git a/src/auth/tests/_async/test_gotrue.py b/src/auth/tests/_async/test_gotrue.py index b6ea23864..1a3079af2 100644 --- a/src/auth/tests/_async/test_gotrue.py +++ b/src/auth/tests/_async/test_gotrue.py @@ -1,8 +1,10 @@ +import re import time from uuid import uuid4 import pytest from jwt import encode +from supabase_auth import AsyncMemoryStorage from supabase_auth.errors import ( AuthApiError, AuthInvalidJwtError, @@ -337,7 +339,7 @@ async def test_exchange_code_for_session() -> None: client._flow_type = "pkce" # Test the PKCE URL generation which is needed for exchange_code_for_session - url, params = await client._get_url_for_provider( + url, params, flow_id = await client._get_url_for_provider( f"{client._url}/authorize", "github", {} ) @@ -345,9 +347,191 @@ async def test_exchange_code_for_session() -> None: assert "code_challenge" in params assert "code_challenge_method" in params - # Verify the code verifier was stored + # Verify the code verifier was stored under the legacy key and the flow slot code_verifier = await client._storage.get_item(storage_key) assert code_verifier is not None + assert flow_id is not None + assert re.match(r"^[a-f0-9]{32}$", flow_id) + slot_verifier = await client._storage.get_item( + f"{client._storage_key}-flow-{flow_id}-code-verifier" + ) + assert slot_verifier == code_verifier + + +async def test_get_url_for_provider_has_no_flow_id_on_implicit_flow() -> None: + client = auth_client() + client._flow_type = "implicit" + + url, params, flow_id = await client._get_url_for_provider( + f"{client._url}/authorize", "github", {} + ) + + assert flow_id is None + assert "code_challenge" not in params + assert _memory_storage(client) == {} + + +async def test_overlapping_pkce_flows_keep_distinct_verifiers() -> None: + client = auth_client() + client._flow_type = "pkce" + + first = await client.sign_in_with_oauth({"provider": "github"}) + second = await client.sign_in_with_oauth({"provider": "google"}) + + assert first.flow_id is not None and second.flow_id is not None + assert first.flow_id != second.flow_id + + first_verifier = await client._pkce_verifier_store.retrieve(first.flow_id) + second_verifier = await client._pkce_verifier_store.retrieve(second.flow_id) + assert first_verifier is not None and second_verifier is not None + assert first_verifier != second_verifier + # the legacy key follows the most recent flow + assert await client._pkce_verifier_store.retrieve(None) == second_verifier + + +def _mock_auth_response_json() -> str: + from datetime import datetime + + from supabase_auth.types import Session, User + + date = datetime(year=2023, month=1, day=1) + user = User( + id="user123", + email="test@example.com", + app_metadata={}, + user_metadata={}, + aud="authenticated", + created_at=date, + confirmed_at=date, + last_sign_in_at=date, + role="authenticated", + updated_at=date, + ) + session = Session( + access_token="mock_access_token", + refresh_token="mock_refresh_token", + expires_in=3600, + token_type="bearer", + user=user, + ) + # the token endpoint answers with the session itself, which embeds the user + return session.model_dump_json() + + +def _memory_storage(client) -> dict: + storage = client._storage + assert isinstance(storage, AsyncMemoryStorage) + return storage.storage + + +async def test_exchange_code_for_session_uses_verifier_of_given_flow() -> None: + from unittest.mock import patch + + from httpx import Response + + client = auth_client() + client._flow_type = "pkce" + + first = await client.sign_in_with_oauth({"provider": "github"}) + second = await client.sign_in_with_oauth({"provider": "google"}) + assert first.flow_id is not None and second.flow_id is not None + first_verifier = await client._pkce_verifier_store.retrieve(first.flow_id) + second_verifier = await client._pkce_verifier_store.retrieve(second.flow_id) + + with patch.object(client, "_request") as mock_request: + mock_request.return_value = Response( + content=_mock_auth_response_json(), status_code=200 + ) + + # the first flow completes after the second one overwrote the legacy key + response = await client.exchange_code_for_session( + {"auth_code": "code-1", "flow_id": first.flow_id} + ) + + mock_request.assert_called_once() + args, kwargs = mock_request.call_args + assert args[0] == "POST" + assert args[1] == "token" + assert kwargs["body"] == { + "auth_code": "code-1", + "code_verifier": first_verifier, + } + + assert response.session is not None + assert response.session.access_token == "mock_access_token" + + # only the first flow's slot is consumed + assert await client._pkce_verifier_store.retrieve(first.flow_id) is None + assert await client._pkce_verifier_store.retrieve(second.flow_id) == second_verifier + assert await client._pkce_verifier_store.retrieve(None) == second_verifier + + +async def test_exchange_code_for_session_fails_fast_for_unknown_flow_id() -> None: + from unittest.mock import patch + + from supabase_auth.errors import AuthPKCECodeVerifierMissingError + + client = auth_client() + client._flow_type = "pkce" + + pending = await client.sign_in_with_oauth({"provider": "github"}) + assert pending.flow_id is not None + storage_before = dict(_memory_storage(client)) + + with patch.object(client, "_request") as mock_request: + for flow_id in ["flow-id-gone0000", "slash/../evil"]: + with pytest.raises(AuthPKCECodeVerifierMissingError): + await client.exchange_code_for_session( + {"auth_code": "code-1", "flow_id": flow_id} + ) + # no request goes out and the pending flow is untouched + mock_request.assert_not_called() + + assert _memory_storage(client) == storage_before + + +async def test_exchange_code_for_session_falls_back_to_legacy_key() -> None: + from unittest.mock import patch + + from httpx import Response + + client = auth_client() + client._flow_type = "pkce" + + # a verifier stored by an older client version under the legacy key only + await client._storage.set_item( + f"{client._storage_key}-code-verifier", "legacy-verifier" + ) + + with patch.object(client, "_request") as mock_request: + mock_request.return_value = Response( + content=_mock_auth_response_json(), status_code=200 + ) + + response = await client.exchange_code_for_session({"auth_code": "code-1"}) + + _, kwargs = mock_request.call_args + assert kwargs["body"]["code_verifier"] == "legacy-verifier" + + assert response.session is not None + assert ( + await client._storage.get_item(f"{client._storage_key}-code-verifier") is None + ) + + +async def test_sign_out_clears_every_pending_pkce_flow() -> None: + client = auth_client() + client._flow_type = "pkce" + credentials = mock_user_credentials() + await client.sign_up({"email": credentials.email, "password": credentials.password}) + + await client.sign_in_with_oauth({"provider": "github"}) + await client.sign_in_with_oauth({"provider": "google"}) + assert any("-flow-" in key for key in _memory_storage(client)) + + await client.sign_out() + + assert not any("code-verifier" in key for key in _memory_storage(client)) async def test_get_authenticator_assurance_level() -> None: @@ -396,7 +580,8 @@ async def test_link_identity() -> None: with patch.object(client, "_get_url_for_provider") as mock_url_provider: mock_url = "http://example.com/authorize?provider=github" mock_params = {"provider": "github"} - mock_url_provider.return_value = (mock_url, mock_params) + mock_flow_id = "0123456789abcdef0123456789abcdef" + mock_url_provider.return_value = (mock_url, mock_params, mock_flow_id) # Also mock the _request method since the server would reject it with patch.object(client, "_request") as mock_request: @@ -410,6 +595,7 @@ async def test_link_identity() -> None: # Verify the response assert response.provider == "github" assert response.url == mock_url + assert response.flow_id == mock_flow_id async def test_get_user_identities() -> None: diff --git a/src/auth/tests/_async/test_pkce_verifier_store.py b/src/auth/tests/_async/test_pkce_verifier_store.py new file mode 100644 index 000000000..82623997c --- /dev/null +++ b/src/auth/tests/_async/test_pkce_verifier_store.py @@ -0,0 +1,179 @@ +import json +import re + +import pytest +from supabase_auth import AsyncMemoryStorage +from supabase_auth.constants import PKCE_MAX_CONCURRENT_FLOWS + +from .clients import auth_client + + +def _slot_keys(storage: dict) -> list: + return sorted(key for key in storage if "-flow-" in key) + + +def _store() -> tuple: + """A verifier store bound to a fresh in-memory storage.""" + client = auth_client() + storage = client._storage + assert isinstance(storage, AsyncMemoryStorage) + return client._pkce_verifier_store, storage.storage + + +async def test_store_keeps_one_slot_per_flow_plus_legacy_key() -> None: + store, storage = _store() + + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a" + assert storage[store.slot_key("flow-id-bbbbbbbb")] == "verifier-b" + # the legacy key mirrors the most recently started flow + assert storage[store.legacy_key] == "verifier-b" + assert json.loads(storage[store.index_key]) == [ + "flow-id-aaaaaaaa", + "flow-id-bbbbbbbb", + ] + + +async def test_store_evicts_oldest_flow_beyond_bound_and_reports_it() -> None: + store, storage = _store() + flow_ids = [f"flow-id-{i:08d}" for i in range(PKCE_MAX_CONCURRENT_FLOWS + 1)] + + evicted = [] + for flow_id in flow_ids: + evicted.extend(await store.store(flow_id, f"verifier-{flow_id}")) + + assert evicted == [flow_ids[0]] + assert store.slot_key(flow_ids[0]) not in storage + assert json.loads(storage[store.index_key]) == flow_ids[1:] + assert len(_slot_keys(storage)) == PKCE_MAX_CONCURRENT_FLOWS + + +async def test_store_same_flow_again_does_not_consume_another_slot() -> None: + store, storage = _store() + + await store.store("flow-id-aaaaaaaa", "verifier-a1") + await store.store("flow-id-bbbbbbbb", "verifier-b") + await store.store("flow-id-aaaaaaaa", "verifier-a2") + + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a2" + assert len(_slot_keys(storage)) == 2 + # re-storing moves the flow to the newest position + assert json.loads(storage[store.index_key]) == [ + "flow-id-bbbbbbbb", + "flow-id-aaaaaaaa", + ] + + +async def test_store_rejects_flow_id_it_could_not_evict_later() -> None: + store, storage = _store() + + with pytest.raises(ValueError): + await store.store("slash/../evil", "verifier") + + assert storage == {} + + +async def test_store_tolerates_corrupt_or_hostile_index() -> None: + store, storage = _store() + + for raw_index in [ + "not-json{", + '{"a": 1}', + '[42, null, "short", "../etc/passwd", "ok_flow-id"]', + ]: + storage[store.index_key] = raw_index + await store.store("flow-id-aaaaaaaa", "verifier-a") + + index = json.loads(storage[store.index_key]) + assert index[-1] == "flow-id-aaaaaaaa" + assert all(re.match(r"^[a-zA-Z0-9_-]{8,64}$", item) for item in index) + + +async def test_retrieve_is_slot_only_when_flow_id_is_given() -> None: + store, _ = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + assert await store.retrieve("flow-id-aaaaaaaa") == "verifier-a" + # an unknown flow must not borrow another flow's verifier + assert await store.retrieve("flow-id-gone0000") is None + assert await store.retrieve("slash/../evil") is None + # without a flow id the legacy (most recent) verifier is used + assert await store.retrieve(None) == "verifier-b" + + +async def test_remove_only_touches_its_own_flow() -> None: + store, storage = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + await store.remove("flow-id-aaaaaaaa") + + assert store.slot_key("flow-id-aaaaaaaa") not in storage + assert storage[store.slot_key("flow-id-bbbbbbbb")] == "verifier-b" + # the legacy key belongs to flow b and survives + assert storage[store.legacy_key] == "verifier-b" + assert json.loads(storage[store.index_key]) == ["flow-id-bbbbbbbb"] + + +async def test_remove_drops_legacy_key_when_it_belongs_to_removed_flow() -> None: + store, storage = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + await store.remove("flow-id-bbbbbbbb") + + assert store.legacy_key not in storage + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a" + assert json.loads(storage[store.index_key]) == ["flow-id-aaaaaaaa"] + + +async def test_remove_without_flow_id_only_touches_legacy_key() -> None: + store, storage = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + await store.remove(None) + + assert store.legacy_key not in storage + assert len(_slot_keys(storage)) == 2 + assert json.loads(storage[store.index_key]) == [ + "flow-id-aaaaaaaa", + "flow-id-bbbbbbbb", + ] + + +async def test_remove_unknown_flow_leaves_index_untouched() -> None: + store, storage = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + before = dict(storage) + + await store.remove("flow-id-gone0000") + + assert storage == before + + +async def test_remove_all_clears_every_slot_the_index_and_legacy_key() -> None: + store, storage = _store() + await store.store("flow-id-aaaaaaaa", "verifier-a") + await store.store("flow-id-bbbbbbbb", "verifier-b") + + await store.remove_all() + + assert storage == {} + + +async def test_keys_end_in_code_verifier_and_contain_no_dot() -> None: + store, _ = _store() + # the default storage key itself contains dots; only the suffix we add must not + prefix = store._storage_key + for key in [ + store.slot_key("flow-id-aaaaaaaa"), + store.index_key, + store.legacy_key, + ]: + assert key.startswith(prefix) + assert key.endswith("-code-verifier") + assert "." not in key[len(prefix) :] diff --git a/src/auth/tests/_sync/test_gotrue.py b/src/auth/tests/_sync/test_gotrue.py index 5776f78ff..1f25e13b5 100644 --- a/src/auth/tests/_sync/test_gotrue.py +++ b/src/auth/tests/_sync/test_gotrue.py @@ -1,8 +1,10 @@ +import re import time from uuid import uuid4 import pytest from jwt import encode +from supabase_auth import SyncMemoryStorage from supabase_auth.errors import ( AuthApiError, AuthInvalidJwtError, @@ -337,15 +339,195 @@ def test_exchange_code_for_session() -> None: client._flow_type = "pkce" # Test the PKCE URL generation which is needed for exchange_code_for_session - url, params = client._get_url_for_provider(f"{client._url}/authorize", "github", {}) + url, params, flow_id = client._get_url_for_provider( + f"{client._url}/authorize", "github", {} + ) # Verify PKCE parameters were added assert "code_challenge" in params assert "code_challenge_method" in params - # Verify the code verifier was stored + # Verify the code verifier was stored under the legacy key and the flow slot code_verifier = client._storage.get_item(storage_key) assert code_verifier is not None + assert flow_id is not None + assert re.match(r"^[a-f0-9]{32}$", flow_id) + slot_verifier = client._storage.get_item( + f"{client._storage_key}-flow-{flow_id}-code-verifier" + ) + assert slot_verifier == code_verifier + + +def test_get_url_for_provider_has_no_flow_id_on_implicit_flow() -> None: + client = auth_client() + client._flow_type = "implicit" + + url, params, flow_id = client._get_url_for_provider( + f"{client._url}/authorize", "github", {} + ) + + assert flow_id is None + assert "code_challenge" not in params + assert _memory_storage(client) == {} + + +def test_overlapping_pkce_flows_keep_distinct_verifiers() -> None: + client = auth_client() + client._flow_type = "pkce" + + first = client.sign_in_with_oauth({"provider": "github"}) + second = client.sign_in_with_oauth({"provider": "google"}) + + assert first.flow_id is not None and second.flow_id is not None + assert first.flow_id != second.flow_id + + first_verifier = client._pkce_verifier_store.retrieve(first.flow_id) + second_verifier = client._pkce_verifier_store.retrieve(second.flow_id) + assert first_verifier is not None and second_verifier is not None + assert first_verifier != second_verifier + # the legacy key follows the most recent flow + assert client._pkce_verifier_store.retrieve(None) == second_verifier + + +def _mock_auth_response_json() -> str: + from datetime import datetime + + from supabase_auth.types import Session, User + + date = datetime(year=2023, month=1, day=1) + user = User( + id="user123", + email="test@example.com", + app_metadata={}, + user_metadata={}, + aud="authenticated", + created_at=date, + confirmed_at=date, + last_sign_in_at=date, + role="authenticated", + updated_at=date, + ) + session = Session( + access_token="mock_access_token", + refresh_token="mock_refresh_token", + expires_in=3600, + token_type="bearer", + user=user, + ) + # the token endpoint answers with the session itself, which embeds the user + return session.model_dump_json() + + +def _memory_storage(client) -> dict: + storage = client._storage + assert isinstance(storage, SyncMemoryStorage) + return storage.storage + + +def test_exchange_code_for_session_uses_verifier_of_given_flow() -> None: + from unittest.mock import patch + + from httpx import Response + + client = auth_client() + client._flow_type = "pkce" + + first = client.sign_in_with_oauth({"provider": "github"}) + second = client.sign_in_with_oauth({"provider": "google"}) + assert first.flow_id is not None and second.flow_id is not None + first_verifier = client._pkce_verifier_store.retrieve(first.flow_id) + second_verifier = client._pkce_verifier_store.retrieve(second.flow_id) + + with patch.object(client, "_request") as mock_request: + mock_request.return_value = Response( + content=_mock_auth_response_json(), status_code=200 + ) + + # the first flow completes after the second one overwrote the legacy key + response = client.exchange_code_for_session( + {"auth_code": "code-1", "flow_id": first.flow_id} + ) + + mock_request.assert_called_once() + args, kwargs = mock_request.call_args + assert args[0] == "POST" + assert args[1] == "token" + assert kwargs["body"] == { + "auth_code": "code-1", + "code_verifier": first_verifier, + } + + assert response.session is not None + assert response.session.access_token == "mock_access_token" + + # only the first flow's slot is consumed + assert client._pkce_verifier_store.retrieve(first.flow_id) is None + assert client._pkce_verifier_store.retrieve(second.flow_id) == second_verifier + assert client._pkce_verifier_store.retrieve(None) == second_verifier + + +def test_exchange_code_for_session_fails_fast_for_unknown_flow_id() -> None: + from unittest.mock import patch + + from supabase_auth.errors import AuthPKCECodeVerifierMissingError + + client = auth_client() + client._flow_type = "pkce" + + pending = client.sign_in_with_oauth({"provider": "github"}) + assert pending.flow_id is not None + storage_before = dict(_memory_storage(client)) + + with patch.object(client, "_request") as mock_request: + for flow_id in ["flow-id-gone0000", "slash/../evil"]: + with pytest.raises(AuthPKCECodeVerifierMissingError): + client.exchange_code_for_session( + {"auth_code": "code-1", "flow_id": flow_id} + ) + # no request goes out and the pending flow is untouched + mock_request.assert_not_called() + + assert _memory_storage(client) == storage_before + + +def test_exchange_code_for_session_falls_back_to_legacy_key() -> None: + from unittest.mock import patch + + from httpx import Response + + client = auth_client() + client._flow_type = "pkce" + + # a verifier stored by an older client version under the legacy key only + client._storage.set_item(f"{client._storage_key}-code-verifier", "legacy-verifier") + + with patch.object(client, "_request") as mock_request: + mock_request.return_value = Response( + content=_mock_auth_response_json(), status_code=200 + ) + + response = client.exchange_code_for_session({"auth_code": "code-1"}) + + _, kwargs = mock_request.call_args + assert kwargs["body"]["code_verifier"] == "legacy-verifier" + + assert response.session is not None + assert client._storage.get_item(f"{client._storage_key}-code-verifier") is None + + +def test_sign_out_clears_every_pending_pkce_flow() -> None: + client = auth_client() + client._flow_type = "pkce" + credentials = mock_user_credentials() + client.sign_up({"email": credentials.email, "password": credentials.password}) + + client.sign_in_with_oauth({"provider": "github"}) + client.sign_in_with_oauth({"provider": "google"}) + assert any("-flow-" in key for key in _memory_storage(client)) + + client.sign_out() + + assert not any("code-verifier" in key for key in _memory_storage(client)) def test_get_authenticator_assurance_level() -> None: @@ -394,7 +576,8 @@ def test_link_identity() -> None: with patch.object(client, "_get_url_for_provider") as mock_url_provider: mock_url = "http://example.com/authorize?provider=github" mock_params = {"provider": "github"} - mock_url_provider.return_value = (mock_url, mock_params) + mock_flow_id = "0123456789abcdef0123456789abcdef" + mock_url_provider.return_value = (mock_url, mock_params, mock_flow_id) # Also mock the _request method since the server would reject it with patch.object(client, "_request") as mock_request: @@ -408,6 +591,7 @@ def test_link_identity() -> None: # Verify the response assert response.provider == "github" assert response.url == mock_url + assert response.flow_id == mock_flow_id def test_get_user_identities() -> None: diff --git a/src/auth/tests/_sync/test_pkce_verifier_store.py b/src/auth/tests/_sync/test_pkce_verifier_store.py new file mode 100644 index 000000000..0316575b3 --- /dev/null +++ b/src/auth/tests/_sync/test_pkce_verifier_store.py @@ -0,0 +1,179 @@ +import json +import re + +import pytest +from supabase_auth import SyncMemoryStorage +from supabase_auth.constants import PKCE_MAX_CONCURRENT_FLOWS + +from .clients import auth_client + + +def _slot_keys(storage: dict) -> list: + return sorted(key for key in storage if "-flow-" in key) + + +def _store() -> tuple: + """A verifier store bound to a fresh in-memory storage.""" + client = auth_client() + storage = client._storage + assert isinstance(storage, SyncMemoryStorage) + return client._pkce_verifier_store, storage.storage + + +def test_store_keeps_one_slot_per_flow_plus_legacy_key() -> None: + store, storage = _store() + + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a" + assert storage[store.slot_key("flow-id-bbbbbbbb")] == "verifier-b" + # the legacy key mirrors the most recently started flow + assert storage[store.legacy_key] == "verifier-b" + assert json.loads(storage[store.index_key]) == [ + "flow-id-aaaaaaaa", + "flow-id-bbbbbbbb", + ] + + +def test_store_evicts_oldest_flow_beyond_bound_and_reports_it() -> None: + store, storage = _store() + flow_ids = [f"flow-id-{i:08d}" for i in range(PKCE_MAX_CONCURRENT_FLOWS + 1)] + + evicted = [] + for flow_id in flow_ids: + evicted.extend(store.store(flow_id, f"verifier-{flow_id}")) + + assert evicted == [flow_ids[0]] + assert store.slot_key(flow_ids[0]) not in storage + assert json.loads(storage[store.index_key]) == flow_ids[1:] + assert len(_slot_keys(storage)) == PKCE_MAX_CONCURRENT_FLOWS + + +def test_store_same_flow_again_does_not_consume_another_slot() -> None: + store, storage = _store() + + store.store("flow-id-aaaaaaaa", "verifier-a1") + store.store("flow-id-bbbbbbbb", "verifier-b") + store.store("flow-id-aaaaaaaa", "verifier-a2") + + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a2" + assert len(_slot_keys(storage)) == 2 + # re-storing moves the flow to the newest position + assert json.loads(storage[store.index_key]) == [ + "flow-id-bbbbbbbb", + "flow-id-aaaaaaaa", + ] + + +def test_store_rejects_flow_id_it_could_not_evict_later() -> None: + store, storage = _store() + + with pytest.raises(ValueError): + store.store("slash/../evil", "verifier") + + assert storage == {} + + +def test_store_tolerates_corrupt_or_hostile_index() -> None: + store, storage = _store() + + for raw_index in [ + "not-json{", + '{"a": 1}', + '[42, null, "short", "../etc/passwd", "ok_flow-id"]', + ]: + storage[store.index_key] = raw_index + store.store("flow-id-aaaaaaaa", "verifier-a") + + index = json.loads(storage[store.index_key]) + assert index[-1] == "flow-id-aaaaaaaa" + assert all(re.match(r"^[a-zA-Z0-9_-]{8,64}$", item) for item in index) + + +def test_retrieve_is_slot_only_when_flow_id_is_given() -> None: + store, _ = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + assert store.retrieve("flow-id-aaaaaaaa") == "verifier-a" + # an unknown flow must not borrow another flow's verifier + assert store.retrieve("flow-id-gone0000") is None + assert store.retrieve("slash/../evil") is None + # without a flow id the legacy (most recent) verifier is used + assert store.retrieve(None) == "verifier-b" + + +def test_remove_only_touches_its_own_flow() -> None: + store, storage = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + store.remove("flow-id-aaaaaaaa") + + assert store.slot_key("flow-id-aaaaaaaa") not in storage + assert storage[store.slot_key("flow-id-bbbbbbbb")] == "verifier-b" + # the legacy key belongs to flow b and survives + assert storage[store.legacy_key] == "verifier-b" + assert json.loads(storage[store.index_key]) == ["flow-id-bbbbbbbb"] + + +def test_remove_drops_legacy_key_when_it_belongs_to_removed_flow() -> None: + store, storage = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + store.remove("flow-id-bbbbbbbb") + + assert store.legacy_key not in storage + assert storage[store.slot_key("flow-id-aaaaaaaa")] == "verifier-a" + assert json.loads(storage[store.index_key]) == ["flow-id-aaaaaaaa"] + + +def test_remove_without_flow_id_only_touches_legacy_key() -> None: + store, storage = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + store.remove(None) + + assert store.legacy_key not in storage + assert len(_slot_keys(storage)) == 2 + assert json.loads(storage[store.index_key]) == [ + "flow-id-aaaaaaaa", + "flow-id-bbbbbbbb", + ] + + +def test_remove_unknown_flow_leaves_index_untouched() -> None: + store, storage = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + before = dict(storage) + + store.remove("flow-id-gone0000") + + assert storage == before + + +def test_remove_all_clears_every_slot_the_index_and_legacy_key() -> None: + store, storage = _store() + store.store("flow-id-aaaaaaaa", "verifier-a") + store.store("flow-id-bbbbbbbb", "verifier-b") + + store.remove_all() + + assert storage == {} + + +def test_keys_end_in_code_verifier_and_contain_no_dot() -> None: + store, _ = _store() + # the default storage key itself contains dots; only the suffix we add must not + prefix = store._storage_key + for key in [ + store.slot_key("flow-id-aaaaaaaa"), + store.index_key, + store.legacy_key, + ]: + assert key.startswith(prefix) + assert key.endswith("-code-verifier") + assert "." not in key[len(prefix) :] diff --git a/src/auth/tests/test_helpers.py b/src/auth/tests/test_helpers.py index e8ee6635e..dbb4cfb92 100644 --- a/src/auth/tests/test_helpers.py +++ b/src/auth/tests/test_helpers.py @@ -1,3 +1,4 @@ +import re from datetime import datetime from unittest.mock import MagicMock, patch @@ -19,6 +20,7 @@ from supabase_auth.helpers import ( decode_jwt, generate_pkce_challenge, + generate_pkce_flow_id, generate_pkce_verifier, handle_exception, model_dump, @@ -26,7 +28,11 @@ model_validate, parse_link_identity_response, parse_response_api_version, + pkce_flow_index_key, + pkce_legacy_verifier_key, + pkce_verifier_slot_key, validate_exp, + validate_pkce_flow_id, ) from ._sync.clients import mock_access_token @@ -142,6 +148,29 @@ def test_generate_pkce_challenge() -> None: assert isinstance(generate_pkce_challenge(pkce), str) +def test_generate_pkce_flow_id() -> None: + flow_id = generate_pkce_flow_id() + assert re.match(r"^[a-f0-9]{32}$", flow_id) + assert flow_id != generate_pkce_flow_id() + assert validate_pkce_flow_id(flow_id) == flow_id + + +@pytest.mark.parametrize( + "flow_id", + [None, 42, "", "short", "has spaces here", "a" * 65, "slash/../evil", "dot.ted"], +) +def test_validate_pkce_flow_id_rejects_unsafe_values(flow_id) -> None: + assert validate_pkce_flow_id(flow_id) is None + + +def test_pkce_storage_keys() -> None: + assert pkce_legacy_verifier_key("sb") == "sb-code-verifier" + assert pkce_verifier_slot_key("sb", "flow-id-aaaaaaaa") == ( + "sb-flow-flow-id-aaaaaaaa-code-verifier" + ) + assert pkce_flow_index_key("sb") == "sb-flows-code-verifier" + + def test_parse_response_api_version_invalid_date() -> None: mock_response = MagicMock(spec=Response) mock_response.headers = {API_VERSION_HEADER_NAME: "2023-02-30"} # Invalid date diff --git a/src/auth/tests/test_pkce_verifier_store_concurrency.py b/src/auth/tests/test_pkce_verifier_store_concurrency.py new file mode 100644 index 000000000..85c32814b --- /dev/null +++ b/src/auth/tests/test_pkce_verifier_store_concurrency.py @@ -0,0 +1,33 @@ +import asyncio +import json + +from supabase_auth import AsyncMemoryStorage +from supabase_auth._async.pkce_verifier_store import AsyncPKCEVerifierStore +from supabase_auth.constants import PKCE_MAX_CONCURRENT_FLOWS + + +async def test_concurrent_stores_preserve_the_bound_and_index_every_flow() -> None: + """Interleaved flow starts must not lose index entries. + + The index is read, modified and written back across awaits; without + serialization two starts could each write a different index and leave a + slot that can never be evicted or cleaned up. + """ + storage = AsyncMemoryStorage() + store = AsyncPKCEVerifierStore(storage, "test-storage-key") + flow_ids = [f"flow-id-{i:08d}" for i in range(20)] + + await asyncio.gather( + *(store.store(flow_id, f"verifier-{flow_id}") for flow_id in flow_ids) + ) + + slot_keys = [key for key in storage.storage if "-flow-" in key] + index = json.loads(storage.storage[store.index_key]) + + assert len(slot_keys) == PKCE_MAX_CONCURRENT_FLOWS + assert len(index) == PKCE_MAX_CONCURRENT_FLOWS + # every surviving slot is reachable through the index + assert sorted(store.slot_key(flow_id) for flow_id in index) == sorted(slot_keys) + + await store.remove_all() + assert storage.storage == {}