From 5286022521aebf96c613e7770400630f73c4b4ad Mon Sep 17 00:00:00 2001 From: Ian Patrick Hines Date: Sat, 20 Jun 2026 21:58:28 -0400 Subject: [PATCH 1/3] feat(security): CSRF nonce + redirect_uri allowlist for NB OAuth (fixes #11) The NationBuilder OAuth callback previously trusted an unsigned, base64-only `state` ({user_id, nb_slug, redirect_uri}) and an unvalidated redirect_uri, enabling login-CSRF (attacker crafts state with their own user_id to capture a victim's nation tokens) and authorization-code interception. - Add shared `oauth_state` module: single-use, server-side nonce (DynamoDB w/ TTL) bound to the issued identity; validate-then-delete on callback (atomic conditional delete); created_at expiry; exact-match redirect_uri allowlist. Tampered/replayed/expired/forged state rejected. - Wire `validate_oauth_state` into the callback handler. - Add `nb_oauth_init` Lambda (issuance side) that mints the nonce and redirects to NationBuilder's authorize endpoint. - Infra: OAuthStateTable (TTL on expires_at), IAM, GET /auth/nationbuilder init route, env wiring for both lambdas. - Tests: shared module (round-trip, single-use, expiry, tamper, forged, allowlist), init handler, callback CSRF cases, updated integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- infrastructure/template.yaml | 133 +++++++++ src/lambdas/nb_oauth_callback/handler.py | 40 +-- src/lambdas/nb_oauth_init/__init__.py | 1 + src/lambdas/nb_oauth_init/handler.py | 150 ++++++++++ src/lambdas/shared/oauth_state.py | 262 ++++++++++++++++ .../integration/test_nb_oauth_integration.py | 62 +++- tests/test_nb_oauth_callback.py | 260 ++++++++++++---- tests/test_nb_oauth_init.py | 196 ++++++++++++ tests/test_oauth_state.py | 281 ++++++++++++++++++ 9 files changed, 1306 insertions(+), 79 deletions(-) create mode 100644 src/lambdas/nb_oauth_init/__init__.py create mode 100644 src/lambdas/nb_oauth_init/handler.py create mode 100644 src/lambdas/shared/oauth_state.py create mode 100644 tests/test_nb_oauth_init.py create mode 100644 tests/test_oauth_state.py diff --git a/infrastructure/template.yaml b/infrastructure/template.yaml index 6ba9042..fa3a191 100644 --- a/infrastructure/template.yaml +++ b/infrastructure/template.yaml @@ -126,6 +126,31 @@ Resources: - Key: Project Value: nat + # OAuthStateTable - single-use CSRF nonces for the NationBuilder OAuth flow. + # Each record is written at OAuth init and deleted on callback (single-use); + # the TTL on expires_at evicts any that are never consumed. + OAuthStateTable: + Type: AWS::DynamoDB::Table + DeletionPolicy: Retain + UpdateReplacePolicy: Retain + Properties: + TableName: !Sub 'nat-oauth-state-${Environment}' + BillingMode: PAY_PER_REQUEST + AttributeDefinitions: + - AttributeName: nonce + AttributeType: S + KeySchema: + - AttributeName: nonce + KeyType: HASH + TimeToLiveSpecification: + AttributeName: expires_at + Enabled: true + Tags: + - Key: Environment + Value: !Ref Environment + - Key: Project + Value: nat + # ============================================================================= # IAM Roles # ============================================================================= @@ -163,6 +188,7 @@ Resources: - !Sub '${TenantsTable.Arn}/index/*' - !GetAtt UsersTable.Arn - !Sub '${UsersTable.Arn}/index/*' + - !GetAtt OAuthStateTable.Arn - PolicyName: SecretsManagerAccess PolicyDocument: Version: '2012-10-17' @@ -355,6 +381,9 @@ Resources: Variables: NATIONS_TABLE: !Ref NationsTable USERS_TABLE: !Ref UsersTable + OAUTH_STATE_TABLE: !Ref OAuthStateTable + OAUTH_STATE_TTL_SECONDS: '600' + OAUTH_REDIRECT_URI_ALLOWLIST: !Sub 'https://${ApiGateway}.execute-api.${AWS::Region}.amazonaws.com/${Environment}/auth/nationbuilder/callback' NB_CLIENT_ID_SECRET: !Sub 'nat/nb-client-id-${Environment}' NB_CLIENT_SECRET_SECRET: !Sub 'nat/nb-client-secret-${Environment}' SESSION_JWT_SECRET_NAME: !Sub 'nat/session-jwt-secret-${Environment}' @@ -375,6 +404,45 @@ Resources: Principal: apigateway.amazonaws.com SourceArn: !Sub 'arn:aws:execute-api:${AWS::Region}:${AWS::AccountId}:${ApiGateway}/*/*/*' + # NationBuilder OAuth Init Lambda - issues the single-use CSRF state nonce and + # redirects the browser to NationBuilder's authorize endpoint. + NBOAuthInitFunction: + Type: AWS::Lambda::Function + Properties: + FunctionName: !Sub 'nat-nb-oauth-init-${Environment}' + Runtime: python3.11 + Handler: handler.handler + Role: !GetAtt LambdaExecutionRole.Arn + Code: + ZipFile: | + # Placeholder - actual code deployed separately + def handler(event, context): + return {"statusCode": 200, "body": "OK"} + Timeout: 30 + MemorySize: 256 + Environment: + Variables: + OAUTH_STATE_TABLE: !Ref OAuthStateTable + OAUTH_STATE_TTL_SECONDS: '600' + OAUTH_REDIRECT_URI_ALLOWLIST: !Sub 'https://${ApiGateway}.execute-api.${AWS::Region}.amazonaws.com/${Environment}/auth/nationbuilder/callback' + OAUTH_CALLBACK_URL: !Sub 'https://${ApiGateway}.execute-api.${AWS::Region}.amazonaws.com/${Environment}/auth/nationbuilder/callback' + NB_CLIENT_ID_SECRET: !Sub 'nat/nb-client-id-${Environment}' + ERROR_REDIRECT_URL: !Sub 'https://natassistant.com/connection-error?env=${Environment}' + Tags: + - Key: Environment + Value: !Ref Environment + - Key: Project + Value: nat + + # Permission for API Gateway to invoke NB OAuth Init Lambda + NBOAuthInitLambdaPermission: + Type: AWS::Lambda::Permission + Properties: + FunctionName: !Ref NBOAuthInitFunction + Action: lambda:InvokeFunction + Principal: apigateway.amazonaws.com + SourceArn: !Sub 'arn:aws:execute-api:${AWS::Region}:${AWS::AccountId}:${ApiGateway}/*/*/*' + # Token Refresh Lambda (EventBridge triggered) TokenRefreshFunction: Type: AWS::Lambda::Function @@ -560,6 +628,51 @@ Resources: ParentId: !Ref AuthResource PathPart: nationbuilder + # GET /auth/nationbuilder method - starts the OAuth flow (issues CSRF state) + AuthNBInitMethod: + Type: AWS::ApiGateway::Method + Properties: + RestApiId: !Ref ApiGateway + ResourceId: !Ref AuthNBResource + HttpMethod: GET + AuthorizationType: NONE + Integration: + Type: AWS_PROXY + IntegrationHttpMethod: POST + Uri: !Sub 'arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${NBOAuthInitFunction.Arn}/invocations' + MethodResponses: + - StatusCode: '302' + ResponseParameters: + method.response.header.Location: true + method.response.header.Access-Control-Allow-Origin: true + + # OPTIONS /auth/nationbuilder method for CORS + AuthNBInitOptionsMethod: + Type: AWS::ApiGateway::Method + Properties: + RestApiId: !Ref ApiGateway + ResourceId: !Ref AuthNBResource + HttpMethod: OPTIONS + AuthorizationType: NONE + Integration: + Type: MOCK + RequestTemplates: + application/json: '{"statusCode": 200}' + IntegrationResponses: + - StatusCode: '200' + ResponseParameters: + method.response.header.Access-Control-Allow-Headers: "'Content-Type,X-Amz-Date,Authorization,X-Api-Key,X-Amz-Security-Token'" + method.response.header.Access-Control-Allow-Methods: "'GET,OPTIONS'" + method.response.header.Access-Control-Allow-Origin: "'*'" + MethodResponses: + - StatusCode: '200' + ResponseParameters: + method.response.header.Access-Control-Allow-Headers: true + method.response.header.Access-Control-Allow-Methods: true + method.response.header.Access-Control-Allow-Origin: true + ResponseModels: + application/json: Empty + # /auth/nationbuilder/callback resource AuthNBCallbackResource: Type: AWS::ApiGateway::Resource @@ -813,6 +926,8 @@ Resources: - StripeWebhookOptionsMethod - StripeCheckoutMethod - StripeCheckoutOptionsMethod + - AuthNBInitMethod + - AuthNBInitOptionsMethod - AuthNBCallbackMethod - AuthNBCallbackOptionsMethod - AgentQueryMethod @@ -944,6 +1059,24 @@ Outputs: Export: Name: !Sub '${AWS::StackName}-NBOAuthCallbackUrl' + NBOAuthInitFunctionArn: + Description: ARN of the NationBuilder OAuth Init Lambda function + Value: !GetAtt NBOAuthInitFunction.Arn + Export: + Name: !Sub '${AWS::StackName}-NBOAuthInitFunctionArn' + + NBOAuthInitUrl: + Description: URL of the NationBuilder OAuth init endpoint (starts the flow) + Value: !Sub 'https://${ApiGateway}.execute-api.${AWS::Region}.amazonaws.com/${Environment}/auth/nationbuilder' + Export: + Name: !Sub '${AWS::StackName}-NBOAuthInitUrl' + + OAuthStateTableName: + Description: Name of the OAuth state (CSRF nonce) DynamoDB table + Value: !Ref OAuthStateTable + Export: + Name: !Sub '${AWS::StackName}-OAuthStateTableName' + TokenRefreshFunctionArn: Description: ARN of the Token Refresh Lambda function Value: !GetAtt TokenRefreshFunction.Arn diff --git a/src/lambdas/nb_oauth_callback/handler.py b/src/lambdas/nb_oauth_callback/handler.py index 73ed887..34fdb59 100644 --- a/src/lambdas/nb_oauth_callback/handler.py +++ b/src/lambdas/nb_oauth_callback/handler.py @@ -22,11 +22,19 @@ from botocore.exceptions import ClientError try: # Resolve in both pytest (repo root) and flattened Lambda packages. + from src.lambdas.shared.oauth_state import ( + OAuthStateError, + validate_oauth_state, + ) from src.lambdas.shared.session_token import ( get_session_secret, mint_session_token, ) except ModuleNotFoundError: # pragma: no cover - exercised only in Lambda + from shared.oauth_state import ( # type: ignore[no-redef] + OAuthStateError, + validate_oauth_state, + ) from shared.session_token import ( # type: ignore[no-redef] get_session_secret, mint_session_token, @@ -415,27 +423,25 @@ def handler(event: dict[str, Any], context: Any) -> LambdaResponse: error_url = f"{ERROR_REDIRECT_URL}?error=missing_state" return create_redirect_response(error_url) - # Parse state parameter + # Validate and consume the OAuth state. This enforces CSRF protection + # (single-use, server-side nonce bound to the issued identity) and the + # redirect_uri allowlist. Tampered, replayed, or expired state is + # rejected here. The returned fields come from the trusted server-side + # record, not the forgeable client copy. try: - import base64 - state_json = base64.urlsafe_b64decode(state).decode("utf-8") - state_data = json.loads(state_json) - except (json.JSONDecodeError, ValueError) as e: - logger.error(f"Invalid state parameter: {e}") - error_url = f"{ERROR_REDIRECT_URL}?error=invalid_state" + state_data = validate_oauth_state(state) + except OAuthStateError as e: + logger.error(f"Invalid OAuth state: {e}") + error_url = f"{ERROR_REDIRECT_URL}?error={e.error_slug}" return create_redirect_response(error_url) - user_id = state_data.get("user_id") - nb_slug = state_data.get("nb_slug") - redirect_uri = state_data.get("redirect_uri") - - if not user_id or not nb_slug or not redirect_uri: - logger.error("State missing required fields") - error_url = f"{ERROR_REDIRECT_URL}?error=invalid_state" - return create_redirect_response(error_url) + user_id = state_data["user_id"] + nb_slug = state_data["nb_slug"] + redirect_uri = state_data["redirect_uri"] - # Extract optional email from state for admin user creation - user_email = state_data.get("email") + # admin_email is no longer carried in the (now trusted, single-use) + # state. Populating it from NationBuilder userinfo is a follow-up. + user_email = None logger.info(f"Processing OAuth callback for user {user_id}, nation {nb_slug}") diff --git a/src/lambdas/nb_oauth_init/__init__.py b/src/lambdas/nb_oauth_init/__init__.py new file mode 100644 index 0000000..cee075e --- /dev/null +++ b/src/lambdas/nb_oauth_init/__init__.py @@ -0,0 +1 @@ +# NationBuilder OAuth Init Lambda diff --git a/src/lambdas/nb_oauth_init/handler.py b/src/lambdas/nb_oauth_init/handler.py new file mode 100644 index 0000000..0b2c8f0 --- /dev/null +++ b/src/lambdas/nb_oauth_init/handler.py @@ -0,0 +1,150 @@ +""" +NationBuilder OAuth Init Lambda Handler. + +Starts the NationBuilder OAuth connect flow. This is the *issuance* side of the +CSRF protection added in the OAuth callback: it mints a single-use, server-side +``state`` nonce (see :mod:`shared.oauth_state`) bound to the user and nation, +then redirects the browser to NationBuilder's authorize endpoint. + +Flow: + 1. Read ``nb_slug`` (the nation being connected) from the query string. + 2. Use the caller-supplied ``user_id`` or generate a fresh one (the NB connect + flow doubles as login, so a brand-new connect has no prior identity). + 3. Issue a single-use ``state`` bound to ``(user_id, nb_slug, redirect_uri)``, + persisting its nonce in DynamoDB with a TTL. + 4. 302-redirect to ``https://{nb_slug}.nationbuilder.com/oauth/authorize``. + +``redirect_uri`` is fixed server-side from ``OAUTH_CALLBACK_URL`` (which must be +on the allowlist) so it can never be influenced by the client. +""" + +from __future__ import annotations + +import json +import logging +import os +import uuid +from typing import Any, TypedDict +from urllib.parse import urlencode + +import boto3 +from botocore.exceptions import ClientError + +try: # Resolve in both pytest (repo root) and flattened Lambda packages. + from src.lambdas.shared.oauth_state import ( + OAuthStateError, + issue_oauth_state, + ) +except ModuleNotFoundError: # pragma: no cover - exercised only in Lambda + from shared.oauth_state import ( # type: ignore[no-redef] + OAuthStateError, + issue_oauth_state, + ) + +logger = logging.getLogger() +logger.setLevel(logging.INFO) + +NB_CLIENT_ID_SECRET = os.environ.get("NB_CLIENT_ID_SECRET", "nat/nb-client-id") +# The OAuth callback URI registered with NationBuilder. Must be on the +# OAUTH_REDIRECT_URI_ALLOWLIST so the callback accepts the resulting state. +OAUTH_CALLBACK_URL = os.environ.get("OAUTH_CALLBACK_URL", "") +ERROR_REDIRECT_URL = os.environ.get( + "ERROR_REDIRECT_URL", "https://natassistant.com/connection-error" +) + + +class LambdaResponse(TypedDict): + """Lambda response type.""" + + statusCode: int + body: str + headers: dict[str, str] + + +def get_secret(secret_name: str) -> str: + """Retrieve a secret value from Secrets Manager (mirrors callback handler).""" + client = boto3.client("secretsmanager") + response = client.get_secret_value(SecretId=secret_name) + secret: str = response.get("SecretString", "") + try: + secret_data = json.loads(secret) + for key in ["value", "secret", "token", "key"]: + if key in secret_data: + return str(secret_data[key]) + if secret_data: + return str(next(iter(secret_data.values()))) + return secret + except json.JSONDecodeError: + return secret + + +def create_redirect_response(url: str) -> LambdaResponse: + """Create a 302 redirect response.""" + return { + "statusCode": 302, + "body": "", + "headers": { + "Location": url, + "Access-Control-Allow-Origin": "*", + }, + } + + +def handler(event: dict[str, Any], context: Any) -> LambdaResponse: + """Lambda handler for starting the NationBuilder OAuth flow. + + Expected query parameters: + - ``nb_slug`` (required): the NationBuilder nation slug to connect. + - ``user_id`` (optional): existing user identity; generated if absent. + """ + try: + query_params = event.get("queryStringParameters") or {} + nb_slug = query_params.get("nb_slug") + if not nb_slug: + logger.error("Missing nb_slug") + return create_redirect_response( + f"{ERROR_REDIRECT_URL}?error=missing_nb_slug" + ) + + if not OAUTH_CALLBACK_URL: + logger.error("OAUTH_CALLBACK_URL is not configured") + return create_redirect_response( + f"{ERROR_REDIRECT_URL}?error=server_misconfigured" + ) + + # NB connect doubles as login; mint a new user_id when none is supplied. + user_id = query_params.get("user_id") or f"user-{uuid.uuid4().hex}" + + try: + state = issue_oauth_state( + user_id=user_id, + nb_slug=nb_slug, + redirect_uri=OAUTH_CALLBACK_URL, + ) + except OAuthStateError as e: + logger.error(f"Failed to issue OAuth state: {e}") + return create_redirect_response( + f"{ERROR_REDIRECT_URL}?error={e.error_slug}" + ) + + client_id = get_secret(NB_CLIENT_ID_SECRET) + authorize_query = urlencode( + { + "client_id": client_id, + "redirect_uri": OAUTH_CALLBACK_URL, + "response_type": "code", + "state": state, + } + ) + authorize_url = ( + f"https://{nb_slug}.nationbuilder.com/oauth/authorize?{authorize_query}" + ) + logger.info(f"Starting OAuth flow for nation {nb_slug}, user {user_id}") + return create_redirect_response(authorize_url) + + except ClientError as e: + logger.error(f"AWS service error: {e}") + return create_redirect_response(f"{ERROR_REDIRECT_URL}?error=service_error") + except Exception as e: + logger.error(f"Unexpected error: {e}") + return create_redirect_response(f"{ERROR_REDIRECT_URL}?error=unexpected") diff --git a/src/lambdas/shared/oauth_state.py b/src/lambdas/shared/oauth_state.py new file mode 100644 index 0000000..406d85c --- /dev/null +++ b/src/lambdas/shared/oauth_state.py @@ -0,0 +1,262 @@ +""" +OAuth ``state`` (CSRF) protection for the NationBuilder OAuth flow. + +The OAuth ``state`` parameter must be unforgeable and single-use to defend the +NationBuilder connect flow against two related attacks: + + - **CSRF / login-CSRF.** Previously ``state`` was just base64-encoded JSON + (``{user_id, nb_slug, redirect_uri}``) with no secret or nonce. An attacker + could craft a ``state`` carrying their *own* ``user_id``, lure a victim + through the flow, and have the victim's freshly minted nation tokens stored + under the attacker's account. + - **Authorization-code interception.** ``redirect_uri`` was taken from + ``state`` and used verbatim in the token exchange with no allowlist. + +Design +------ +At issuance (the OAuth init / authorize step) :func:`issue_oauth_state`: + + 1. Validates ``redirect_uri`` against an allowlist. + 2. Generates a cryptographically random, single-use ``nonce``. + 3. Persists the ``nonce`` server-side in DynamoDB with a TTL, binding it to + the ``{user_id, nb_slug, redirect_uri}`` it was issued for. + 4. Encodes ``{user_id, nb_slug, redirect_uri, nonce, created_at}`` as the + base64url ``state`` string handed to NationBuilder. + +At the callback :func:`validate_oauth_state`: + + 1. Decodes ``state`` and validates ``redirect_uri`` against the allowlist. + 2. Atomically claims-and-deletes the ``nonce`` from DynamoDB. A missing + record means the state is forged, already used (single-use), or + expired-and-evicted -> reject. + 3. Compares the *stored* ``{user_id, nb_slug, redirect_uri}`` against the + copy carried in ``state``. Any mismatch means the state was tampered with + after issuance -> reject. + 4. Rejects if older than ``OAUTH_STATE_TTL_SECONDS`` (using ``created_at``). + +The DynamoDB record is the trust anchor: because only the issuer writes nonces, +an attacker cannot fabricate a ``state`` that survives the lookup, and the +authoritative copy of the bound fields lives server-side where the client +cannot tamper with it. The delete is conditional (``attribute_exists``) so the +single-use guarantee holds even under concurrent replay. +""" + +from __future__ import annotations + +import base64 +import json +import logging +import os +import secrets +import time +from typing import Any + +import boto3 +from botocore.exceptions import ClientError + +logger = logging.getLogger() + +# DynamoDB table holding single-use OAuth state nonces (PK: ``nonce``). +OAUTH_STATE_TABLE = os.environ.get("OAUTH_STATE_TABLE", "nat-oauth-state-dev") + +# Maximum age of a state before it is rejected, in seconds. Also drives the +# DynamoDB TTL written on each record. Short: the connect round trip is quick. +OAUTH_STATE_TTL_SECONDS = int(os.environ.get("OAUTH_STATE_TTL_SECONDS", "600")) + +# Comma-separated allowlist of exact redirect_uri values the OAuth flow may use. +# Empty / unset means deny-all (secure default); production sets this via the +# Lambda environment. Whitespace around entries is ignored. +OAUTH_REDIRECT_URI_ALLOWLIST = os.environ.get("OAUTH_REDIRECT_URI_ALLOWLIST", "") + + +class OAuthStateError(Exception): + """Raised when an OAuth ``state`` is missing, malformed, forged, tampered, + replayed, expired, or carries a disallowed ``redirect_uri``. + + ``error_slug`` is a short, safe token surfaced in the user-facing error + redirect query string (never the raw exception detail). + """ + + def __init__(self, message: str, error_slug: str = "invalid_state") -> None: + self.error_slug = error_slug + super().__init__(message) + + +def get_dynamodb_resource() -> Any: + """Return a DynamoDB resource (indirection allows mocking in tests).""" + return boto3.resource("dynamodb") + + +def _get_table() -> Any: + return get_dynamodb_resource().Table(OAUTH_STATE_TABLE) + + +def _allowed_redirect_uris() -> set[str]: + return { + uri.strip() + for uri in OAUTH_REDIRECT_URI_ALLOWLIST.split(",") + if uri.strip() + } + + +def validate_redirect_uri(redirect_uri: str) -> bool: + """Return ``True`` iff ``redirect_uri`` exactly matches an allowlist entry. + + Exact matching (scheme + host + path) is intentional: prefix/substring + matching is a classic source of open-redirect and code-interception bugs. + """ + if not redirect_uri: + return False + return redirect_uri in _allowed_redirect_uris() + + +def _encode_state(payload: dict[str, Any]) -> str: + raw = json.dumps(payload, separators=(",", ":")).encode("utf-8") + return base64.urlsafe_b64encode(raw).decode("ascii") + + +def _decode_state(state: str) -> dict[str, Any]: + try: + raw = base64.urlsafe_b64decode(state) + data = json.loads(raw.decode("utf-8")) + except (ValueError, json.JSONDecodeError) as e: + raise OAuthStateError(f"Malformed state: {e}") from e + if not isinstance(data, dict): + raise OAuthStateError("State payload is not an object") + return data + + +def issue_oauth_state( + user_id: str, + nb_slug: str, + redirect_uri: str, + now: float | None = None, +) -> str: + """Mint a single-use OAuth ``state`` bound to ``(user_id, nb_slug, + redirect_uri)`` and persist its nonce server-side with a TTL. + + Args: + user_id: The user the flow is being initiated for. + nb_slug: The NationBuilder nation slug being connected. + redirect_uri: OAuth callback URI; must be on the allowlist. + now: Override for the current epoch time (testing). + + Returns: + The base64url-encoded ``state`` string to pass to NationBuilder. + + Raises: + OAuthStateError: If any field is missing or ``redirect_uri`` is not + allowlisted. + """ + if not user_id or not nb_slug or not redirect_uri: + raise OAuthStateError("user_id, nb_slug and redirect_uri are required") + if not validate_redirect_uri(redirect_uri): + raise OAuthStateError( + f"redirect_uri not allowlisted: {redirect_uri}", + error_slug="invalid_redirect_uri", + ) + + created_at = int(now if now is not None else time.time()) + nonce = secrets.token_urlsafe(32) + + _get_table().put_item( + Item={ + "nonce": nonce, + "user_id": user_id, + "nb_slug": nb_slug, + "redirect_uri": redirect_uri, + "created_at": created_at, + # DynamoDB TTL attribute (epoch seconds). Best-effort cleanup; the + # created_at age check below is the authoritative expiry enforcement. + "expires_at": created_at + OAUTH_STATE_TTL_SECONDS, + } + ) + + return _encode_state( + { + "user_id": user_id, + "nb_slug": nb_slug, + "redirect_uri": redirect_uri, + "nonce": nonce, + "created_at": created_at, + } + ) + + +def validate_oauth_state(state: str, now: float | None = None) -> dict[str, str]: + """Validate, atomically consume, and return the claims of an OAuth ``state``. + + Args: + state: The base64url ``state`` returned on the OAuth callback. + now: Override for the current epoch time (testing). + + Returns: + ``{"user_id", "nb_slug", "redirect_uri"}`` taken from the server-side + record (the trusted copy). + + Raises: + OAuthStateError: If the state is missing, malformed, carries a + disallowed ``redirect_uri``, is unknown / already used / expired, or + was tampered with after issuance. + """ + if not state: + raise OAuthStateError("Missing state", error_slug="missing_state") + + data = _decode_state(state) + nonce = data.get("nonce") + redirect_uri = data.get("redirect_uri") + if not nonce or not isinstance(nonce, str): + raise OAuthStateError("State missing nonce") + if not redirect_uri or not isinstance(redirect_uri, str): + raise OAuthStateError("State missing redirect_uri") + + # Reject disallowed redirect_uri before any DynamoDB work. + if not validate_redirect_uri(redirect_uri): + raise OAuthStateError( + f"redirect_uri not allowlisted: {redirect_uri}", + error_slug="invalid_redirect_uri", + ) + + # Atomically claim-and-delete the nonce. ConditionalCheckFailed means the + # nonce never existed or was already consumed -> forged / replayed / evicted. + table = _get_table() + try: + response = table.delete_item( + Key={"nonce": nonce}, + ConditionExpression="attribute_exists(nonce)", + ReturnValues="ALL_OLD", + ) + except ClientError as e: + if e.response["Error"]["Code"] == "ConditionalCheckFailedException": + raise OAuthStateError( + "State is unknown, already used, or expired" + ) from e + logger.error(f"DynamoDB error consuming OAuth state: {e}") + raise + + stored = response.get("Attributes") or {} + + # Tamper check: the state's own copy of the bound fields must match the + # authoritative server-side record exactly. + for field in ("user_id", "nb_slug", "redirect_uri"): + if str(data.get(field, "")) != str(stored.get(field, "")): + raise OAuthStateError(f"State {field} does not match issued value") + + # Expiry check against the authoritative created_at (defence in depth on top + # of the DynamoDB TTL, which is only best-effort/eventual). + created_at = stored.get("created_at") + current = int(now if now is not None else time.time()) + if created_at is None: + raise OAuthStateError("State record missing created_at") + try: + issued_at = int(created_at) + except (TypeError, ValueError) as e: + raise OAuthStateError("State has invalid created_at") from e + if current - issued_at > OAUTH_STATE_TTL_SECONDS: + raise OAuthStateError("State has expired", error_slug="expired_state") + + return { + "user_id": str(stored["user_id"]), + "nb_slug": str(stored["nb_slug"]), + "redirect_uri": str(stored["redirect_uri"]), + } diff --git a/tests/integration/test_nb_oauth_integration.py b/tests/integration/test_nb_oauth_integration.py index c1ce604..51801ef 100644 --- a/tests/integration/test_nb_oauth_integration.py +++ b/tests/integration/test_nb_oauth_integration.py @@ -16,8 +16,10 @@ from unittest.mock import MagicMock, patch import pytest +from botocore.exceptions import ClientError from src.lambdas.nb_oauth_callback.handler import handler +from src.lambdas.shared.oauth_state import issue_oauth_state # Test constants @@ -32,18 +34,64 @@ TEST_REFRESH_TOKEN = "nb_refresh_token_ghijkl" +class MockStateTable: + """In-memory stand-in for the DynamoDB OAuth state (CSRF nonce) table.""" + + def __init__(self) -> None: + self.items: dict[str, dict[str, Any]] = {} + + def put_item(self, Item: dict[str, Any]) -> None: + self.items[Item["nonce"]] = dict(Item) + + def delete_item( + self, + Key: dict[str, Any], + ConditionExpression: str | None = None, + ReturnValues: str | None = None, + ) -> dict[str, Any]: + nonce = Key["nonce"] + if nonce not in self.items: + raise ClientError( + {"Error": {"Code": "ConditionalCheckFailedException"}}, + "DeleteItem", + ) + old = self.items.pop(nonce) + return {"Attributes": old} if ReturnValues == "ALL_OLD" else {} + + +class MockStateResource: + def __init__(self, table: MockStateTable) -> None: + self._table = table + + def Table(self, name: str) -> MockStateTable: + return self._table + + +@pytest.fixture(autouse=True) +def oauth_state_backend() -> Any: + """Back the OAuth state store with an in-memory table and allow the test + redirect_uri for the whole module so issue/validate round-trip works.""" + table = MockStateTable() + with ( + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockStateResource(table), + ), + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + TEST_REDIRECT_URI, + ), + ): + yield table + + def create_oauth_state( user_id: str, nb_slug: str, redirect_uri: str, ) -> str: - """Create a valid OAuth state parameter.""" - state_data = { - "user_id": user_id, - "nb_slug": nb_slug, - "redirect_uri": redirect_uri, - } - return base64.urlsafe_b64encode(json.dumps(state_data).encode()).decode() + """Issue a valid, server-side-backed OAuth state (single-use nonce).""" + return issue_oauth_state(user_id, nb_slug, redirect_uri) class MockDynamoDBTable: diff --git a/tests/test_nb_oauth_callback.py b/tests/test_nb_oauth_callback.py index dc4e31d..089e62a 100644 --- a/tests/test_nb_oauth_callback.py +++ b/tests/test_nb_oauth_callback.py @@ -6,10 +6,12 @@ import base64 import json +from contextlib import ExitStack from typing import Any from unittest.mock import MagicMock, patch import pytest +from botocore.exceptions import ClientError from src.lambdas.nb_oauth_callback.handler import ( create_redirect_response, @@ -18,6 +20,7 @@ store_nb_tokens, update_user_nb_status, ) +from src.lambdas.shared.oauth_state import issue_oauth_state # Test data @@ -32,7 +35,10 @@ def create_state(user_id: str, nb_slug: str, redirect_uri: str) -> str: - """Create a base64-encoded state parameter.""" + """Create a legacy/plain base64-encoded state (no server-side nonce). + + Used only by tests that return before state validation (e.g. missing code). + """ state_data = { "user_id": user_id, "nb_slug": nb_slug, @@ -41,6 +47,56 @@ def create_state(user_id: str, nb_slug: str, redirect_uri: str) -> str: return base64.urlsafe_b64encode(json.dumps(state_data).encode()).decode() +class MockOAuthStateTable: + """In-memory stand-in for the DynamoDB OAuth state (CSRF nonce) table.""" + + def __init__(self) -> None: + self.items: dict[str, dict[str, Any]] = {} + + def put_item(self, Item: dict[str, Any]) -> None: + self.items[Item["nonce"]] = dict(Item) + + def delete_item( + self, + Key: dict[str, Any], + ConditionExpression: str | None = None, + ReturnValues: str | None = None, + ) -> dict[str, Any]: + nonce = Key["nonce"] + if nonce not in self.items: + raise ClientError( + {"Error": {"Code": "ConditionalCheckFailedException"}}, + "DeleteItem", + ) + old = self.items.pop(nonce) + if ReturnValues == "ALL_OLD": + return {"Attributes": old} + return {} + + +class MockOAuthStateResource: + def __init__(self, table: MockOAuthStateTable) -> None: + self._table = table + + def Table(self, name: str) -> MockOAuthStateTable: + return self._table + + +def oauth_state_patches(table: MockOAuthStateTable) -> list[Any]: + """Patches so issue_oauth_state / validate_oauth_state hit a shared mock + table and accept TEST_REDIRECT_URI.""" + return [ + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockOAuthStateResource(table), + ), + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + TEST_REDIRECT_URI, + ), + ] + + class MockDynamoDBTable: """Mock DynamoDB table for testing (per-nation model).""" @@ -367,33 +423,43 @@ def test_successful_oauth_flow(self) -> None: mock_http = MagicMock() mock_http.request.return_value = mock_token_response - state = create_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) - event = { - "queryStringParameters": { - "code": TEST_CODE, - "state": state, - }, - } + state_table = MockOAuthStateTable() - with ( - patch( - "src.lambdas.nb_oauth_callback.handler.get_dynamodb_resource", - return_value=mock_resource, - ), - patch( - "src.lambdas.nb_oauth_callback.handler.get_secrets_manager_client", - return_value=mock_sm_client, - ), - patch( - "src.lambdas.nb_oauth_callback.handler.get_secret", - side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], - ), - patch( - "src.lambdas.nb_oauth_callback.handler.get_session_secret", - return_value="test-session-secret", - ), - patch("urllib3.PoolManager", return_value=mock_http), - ): + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + # Issue a real, server-side-backed state, then run the callback. + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI + ) + event = { + "queryStringParameters": {"code": TEST_CODE, "state": state}, + } + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_dynamodb_resource", + return_value=mock_resource, + ) + ) + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_secrets_manager_client", + return_value=mock_sm_client, + ) + ) + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_secret", + side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], + ) + ) + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_session_secret", + return_value="test-session-secret", + ) + ) + stack.enter_context(patch("urllib3.PoolManager", return_value=mock_http)) response = handler(event, None) assert response["statusCode"] == 302 @@ -401,6 +467,8 @@ def test_successful_oauth_flow(self) -> None: assert TEST_USER_ID in response["headers"]["Location"] # A signed session token is returned to the extension via the fragment. assert "#session_token=" in response["headers"]["Location"] + # The state nonce was consumed (single-use). + assert state_table.items == {} # Verify nation connection record was created/updated assert len(nations_table.put_calls) == 1 @@ -424,24 +492,28 @@ def test_token_exchange_failure_redirects_to_error(self) -> None: mock_http = MagicMock() mock_http.request.return_value = mock_token_response - state = create_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) - event = { - "queryStringParameters": { - "code": TEST_CODE, - "state": state, - }, - } + state_table = MockOAuthStateTable() - with ( - patch( - "src.lambdas.nb_oauth_callback.handler.get_secret", - side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], - ), - patch("urllib3.PoolManager", return_value=mock_http), - ): + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI + ) + event = { + "queryStringParameters": {"code": TEST_CODE, "state": state}, + } + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_secret", + side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], + ) + ) + stack.enter_context(patch("urllib3.PoolManager", return_value=mock_http)) response = handler(event, None) assert response["statusCode"] == 302 + # Reached token exchange (state validated) and failed there. assert "error" in response["headers"]["Location"] def test_null_query_params_handled(self) -> None: @@ -467,22 +539,100 @@ def test_no_access_token_in_response(self) -> None: mock_http = MagicMock() mock_http.request.return_value = mock_token_response - state = create_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) - event = { - "queryStringParameters": { - "code": TEST_CODE, - "state": state, - }, - } + state_table = MockOAuthStateTable() - with ( - patch( - "src.lambdas.nb_oauth_callback.handler.get_secret", - side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], - ), - patch("urllib3.PoolManager", return_value=mock_http), - ): + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI + ) + event = { + "queryStringParameters": {"code": TEST_CODE, "state": state}, + } + stack.enter_context( + patch( + "src.lambdas.nb_oauth_callback.handler.get_secret", + side_effect=[TEST_CLIENT_ID, TEST_CLIENT_SECRET], + ) + ) + stack.enter_context(patch("urllib3.PoolManager", return_value=mock_http)) response = handler(event, None) assert response["statusCode"] == 302 assert "error=no_token" in response["headers"]["Location"] + + def test_csrf_tampered_state_rejected(self) -> None: + """An attacker who swaps user_id in the state is rejected (login-CSRF).""" + state_table = MockOAuthStateTable() + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI + ) + payload = json.loads(base64.urlsafe_b64decode(state).decode()) + payload["user_id"] = "attacker-999" + tampered = base64.urlsafe_b64encode( + json.dumps(payload).encode() + ).decode() + event = { + "queryStringParameters": {"code": TEST_CODE, "state": tampered}, + } + response = handler(event, None) + + assert response["statusCode"] == 302 + assert "error=invalid_state" in response["headers"]["Location"] + + def test_replayed_state_rejected(self) -> None: + """A state already consumed once cannot be reused (single-use).""" + state_table = MockOAuthStateTable() + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI + ) + # Consume the nonce out-of-band to simulate a prior successful use. + from src.lambdas.shared.oauth_state import validate_oauth_state + + validate_oauth_state(state) + event = { + "queryStringParameters": {"code": TEST_CODE, "state": state}, + } + response = handler(event, None) + + assert response["statusCode"] == 302 + assert "error=invalid_state" in response["headers"]["Location"] + + def test_disallowed_redirect_uri_rejected(self) -> None: + """A state carrying a redirect_uri off the allowlist is rejected.""" + state_table = MockOAuthStateTable() + # Issue with a permissive allowlist, validate under the strict default. + with ExitStack() as stack: + stack.enter_context( + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockOAuthStateResource(state_table), + ) + ) + stack.enter_context( + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + "https://evil.example.com/cb", + ) + ) + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, "https://evil.example.com/cb" + ) + + with ExitStack() as stack: + for p in oauth_state_patches(state_table): + stack.enter_context(p) + event = { + "queryStringParameters": {"code": TEST_CODE, "state": state}, + } + response = handler(event, None) + + assert response["statusCode"] == 302 + assert "error=invalid_redirect_uri" in response["headers"]["Location"] diff --git a/tests/test_nb_oauth_init.py b/tests/test_nb_oauth_init.py new file mode 100644 index 0000000..9c5126d --- /dev/null +++ b/tests/test_nb_oauth_init.py @@ -0,0 +1,196 @@ +""" +Unit tests for the NationBuilder OAuth Init Lambda Handler. + +The init endpoint is the issuance side of the CSRF protection: it mints a +single-use, server-side ``state`` nonce and redirects to NationBuilder's +authorize endpoint. +""" + +from __future__ import annotations + +import base64 +import json +from contextlib import ExitStack +from typing import Any +from unittest.mock import patch +from urllib.parse import parse_qs, urlparse + +from botocore.exceptions import ClientError + +from src.lambdas.nb_oauth_init.handler import handler + +TEST_CLIENT_ID = "client_id_123" +TEST_NB_SLUG = "testnation" +TEST_CALLBACK_URL = "https://api.example.com/auth/nationbuilder/callback" + + +class MockOAuthStateTable: + def __init__(self) -> None: + self.items: dict[str, dict[str, Any]] = {} + + def put_item(self, Item: dict[str, Any]) -> None: + self.items[Item["nonce"]] = dict(Item) + + def delete_item( + self, + Key: dict[str, Any], + ConditionExpression: str | None = None, + ReturnValues: str | None = None, + ) -> dict[str, Any]: + nonce = Key["nonce"] + if nonce not in self.items: + raise ClientError( + {"Error": {"Code": "ConditionalCheckFailedException"}}, + "DeleteItem", + ) + old = self.items.pop(nonce) + return {"Attributes": old} if ReturnValues == "ALL_OLD" else {} + + +class MockOAuthStateResource: + def __init__(self, table: MockOAuthStateTable) -> None: + self._table = table + + def Table(self, name: str) -> MockOAuthStateTable: + return self._table + + +def _patches( + table: MockOAuthStateTable, callback_url: str = TEST_CALLBACK_URL +) -> list[Any]: + return [ + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockOAuthStateResource(table), + ), + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + TEST_CALLBACK_URL, + ), + patch("src.lambdas.nb_oauth_init.handler.OAUTH_CALLBACK_URL", callback_url), + patch( + "src.lambdas.nb_oauth_init.handler.get_secret", + return_value=TEST_CLIENT_ID, + ), + ] + + +class TestInitHandler: + def test_missing_nb_slug_redirects_to_error(self) -> None: + event = {"queryStringParameters": {}} + response = handler(event, None) + assert response["statusCode"] == 302 + assert "error=missing_nb_slug" in response["headers"]["Location"] + + def test_null_query_params_handled(self) -> None: + response = handler({"queryStringParameters": None}, None) + assert response["statusCode"] == 302 + assert "error=missing_nb_slug" in response["headers"]["Location"] + + def test_missing_callback_url_misconfigured(self) -> None: + table = MockOAuthStateTable() + with ExitStack() as stack: + for p in _patches(table, callback_url=""): + stack.enter_context(p) + response = handler( + {"queryStringParameters": {"nb_slug": TEST_NB_SLUG}}, None + ) + assert response["statusCode"] == 302 + assert "error=server_misconfigured" in response["headers"]["Location"] + + def test_successful_init_redirects_to_authorize(self) -> None: + table = MockOAuthStateTable() + with ExitStack() as stack: + for p in _patches(table): + stack.enter_context(p) + response = handler( + {"queryStringParameters": {"nb_slug": TEST_NB_SLUG}}, None + ) + + assert response["statusCode"] == 302 + location = response["headers"]["Location"] + parsed = urlparse(location) + assert parsed.scheme == "https" + assert parsed.netloc == f"{TEST_NB_SLUG}.nationbuilder.com" + assert parsed.path == "/oauth/authorize" + + params = parse_qs(parsed.query) + assert params["client_id"] == [TEST_CLIENT_ID] + assert params["redirect_uri"] == [TEST_CALLBACK_URL] + assert params["response_type"] == ["code"] + assert "state" in params + + # The state nonce was persisted server-side, bound to a generated user. + assert len(table.items) == 1 + stored = next(iter(table.items.values())) + assert stored["nb_slug"] == TEST_NB_SLUG + assert stored["redirect_uri"] == TEST_CALLBACK_URL + assert stored["user_id"].startswith("user-") + + def test_state_matches_stored_nonce(self) -> None: + table = MockOAuthStateTable() + with ExitStack() as stack: + for p in _patches(table): + stack.enter_context(p) + response = handler( + {"queryStringParameters": {"nb_slug": TEST_NB_SLUG}}, None + ) + + location = response["headers"]["Location"] + state = parse_qs(urlparse(location).query)["state"][0] + payload = json.loads(base64.urlsafe_b64decode(state).decode()) + assert payload["nonce"] in table.items + + def test_supplied_user_id_preserved(self) -> None: + table = MockOAuthStateTable() + with ExitStack() as stack: + for p in _patches(table): + stack.enter_context(p) + response = handler( + { + "queryStringParameters": { + "nb_slug": TEST_NB_SLUG, + "user_id": "existing-user-7", + } + }, + None, + ) + assert response["statusCode"] == 302 + stored = next(iter(table.items.values())) + assert stored["user_id"] == "existing-user-7" + + def test_callback_url_off_allowlist_rejected(self) -> None: + """If the configured callback URL is not allowlisted, issuance fails.""" + table = MockOAuthStateTable() + with ExitStack() as stack: + stack.enter_context( + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockOAuthStateResource(table), + ) + ) + stack.enter_context( + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + "https://other.example.com/cb", + ) + ) + stack.enter_context( + patch( + "src.lambdas.nb_oauth_init.handler.OAUTH_CALLBACK_URL", + TEST_CALLBACK_URL, + ) + ) + stack.enter_context( + patch( + "src.lambdas.nb_oauth_init.handler.get_secret", + return_value=TEST_CLIENT_ID, + ) + ) + response = handler( + {"queryStringParameters": {"nb_slug": TEST_NB_SLUG}}, None + ) + + assert response["statusCode"] == 302 + assert "error=invalid_redirect_uri" in response["headers"]["Location"] + assert table.items == {} diff --git a/tests/test_oauth_state.py b/tests/test_oauth_state.py new file mode 100644 index 0000000..0c6c677 --- /dev/null +++ b/tests/test_oauth_state.py @@ -0,0 +1,281 @@ +""" +Unit tests for the OAuth state (CSRF) protection module. + +Covers the acceptance criteria for nat#11: + - single-use server-side nonce, validated then deleted + - created_at expiry enforcement + - redirect_uri allowlist + - rejection of tampered / replayed / expired / forged state +""" + +from __future__ import annotations + +import base64 +import json +from typing import Any +from unittest.mock import patch + +import pytest +from botocore.exceptions import ClientError + +from src.lambdas.shared.oauth_state import ( + OAuthStateError, + issue_oauth_state, + validate_oauth_state, + validate_redirect_uri, +) + +TEST_USER_ID = "user-123" +TEST_NB_SLUG = "testnation" +TEST_REDIRECT_URI = "https://api.example.com/auth/nationbuilder/callback" +ALLOWLIST = TEST_REDIRECT_URI + + +class MockOAuthStateTable: + """In-memory stand-in for the DynamoDB OAuth state table.""" + + def __init__(self) -> None: + self.items: dict[str, dict[str, Any]] = {} + + def put_item(self, Item: dict[str, Any]) -> None: + self.items[Item["nonce"]] = dict(Item) + + def delete_item( + self, + Key: dict[str, Any], + ConditionExpression: str | None = None, + ReturnValues: str | None = None, + ) -> dict[str, Any]: + nonce = Key["nonce"] + if nonce not in self.items: + # Mirror the conditional-delete failure on a missing item. + raise ClientError( + {"Error": {"Code": "ConditionalCheckFailedException"}}, + "DeleteItem", + ) + old = self.items.pop(nonce) + if ReturnValues == "ALL_OLD": + return {"Attributes": old} + return {} + + +class MockOAuthStateResource: + def __init__(self, table: MockOAuthStateTable) -> None: + self._table = table + + def Table(self, name: str) -> MockOAuthStateTable: + return self._table + + +def _patches(table: MockOAuthStateTable, allowlist: str = ALLOWLIST) -> list[Any]: + return [ + patch( + "src.lambdas.shared.oauth_state.get_dynamodb_resource", + return_value=MockOAuthStateResource(table), + ), + patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + allowlist, + ), + ] + + +def _decode(state: str) -> dict[str, Any]: + return json.loads(base64.urlsafe_b64decode(state).decode("utf-8")) + + +class TestValidateRedirectUri: + def test_exact_match_allowed(self) -> None: + with patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + ALLOWLIST, + ): + assert validate_redirect_uri(TEST_REDIRECT_URI) is True + + def test_unknown_rejected(self) -> None: + with patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + ALLOWLIST, + ): + assert validate_redirect_uri("https://evil.example.com/callback") is False + + def test_prefix_not_accepted(self) -> None: + """A URI that only shares a prefix must be rejected (no open redirect).""" + with patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", + ALLOWLIST, + ): + assert validate_redirect_uri(TEST_REDIRECT_URI + ".evil.com") is False + + def test_empty_allowlist_denies_all(self) -> None: + with patch( + "src.lambdas.shared.oauth_state.OAUTH_REDIRECT_URI_ALLOWLIST", "" + ): + assert validate_redirect_uri(TEST_REDIRECT_URI) is False + + +class TestIssueOAuthState: + def test_round_trip(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + result = validate_oauth_state(state) + assert result == { + "user_id": TEST_USER_ID, + "nb_slug": TEST_NB_SLUG, + "redirect_uri": TEST_REDIRECT_URI, + } + + def test_state_carries_created_at_and_nonce(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI, now=1000.0 + ) + payload = _decode(state) + assert payload["created_at"] == 1000 + assert isinstance(payload["nonce"], str) and len(payload["nonce"]) > 20 + + def test_nonce_stored_server_side(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + nonce = _decode(state)["nonce"] + assert nonce in table.items + assert table.items[nonce]["user_id"] == TEST_USER_ID + assert "expires_at" in table.items[nonce] + + def test_disallowed_redirect_uri_rejected_at_issuance(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + with pytest.raises(OAuthStateError) as exc: + issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, "https://evil.example.com/cb" + ) + assert exc.value.error_slug == "invalid_redirect_uri" + assert table.items == {} + + def test_missing_fields_rejected(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + with pytest.raises(OAuthStateError): + issue_oauth_state("", TEST_NB_SLUG, TEST_REDIRECT_URI) + + +class TestValidateOAuthState: + def test_single_use_replay_rejected(self) -> None: + """A state validated once cannot be validated again (single-use).""" + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + validate_oauth_state(state) # consumes it + with pytest.raises(OAuthStateError): + validate_oauth_state(state) + + def test_deleted_after_use(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + validate_oauth_state(state) + assert table.items == {} + + def test_expired_state_rejected(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI, now=1000.0 + ) + with pytest.raises(OAuthStateError) as exc: + # 601s later, past the 600s TTL + validate_oauth_state(state, now=1601.0) + assert exc.value.error_slug == "expired_state" + + def test_not_yet_expired_accepted(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI, now=1000.0 + ) + result = validate_oauth_state(state, now=1500.0) + assert result["user_id"] == TEST_USER_ID + + def test_tampered_user_id_rejected(self) -> None: + """Changing user_id in the client copy must not change the result.""" + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + payload = _decode(state) + payload["user_id"] = "attacker-999" + tampered = base64.urlsafe_b64encode( + json.dumps(payload).encode() + ).decode() + with pytest.raises(OAuthStateError): + validate_oauth_state(tampered) + + def test_tampered_nb_slug_rejected(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + state = issue_oauth_state(TEST_USER_ID, TEST_NB_SLUG, TEST_REDIRECT_URI) + payload = _decode(state) + payload["nb_slug"] = "othernation" + tampered = base64.urlsafe_b64encode( + json.dumps(payload).encode() + ).decode() + with pytest.raises(OAuthStateError): + validate_oauth_state(tampered) + + def test_forged_nonce_rejected(self) -> None: + """A state with a nonce that was never issued is rejected.""" + table = MockOAuthStateTable() + forged = base64.urlsafe_b64encode( + json.dumps( + { + "user_id": "attacker-999", + "nb_slug": TEST_NB_SLUG, + "redirect_uri": TEST_REDIRECT_URI, + "nonce": "totally-made-up-nonce", + "created_at": 1000, + } + ).encode() + ).decode() + with _patches(table)[0], _patches(table)[1]: + with pytest.raises(OAuthStateError): + validate_oauth_state(forged) + + def test_disallowed_redirect_uri_rejected_at_callback(self) -> None: + table = MockOAuthStateTable() + # Issue with a permissive allowlist, then validate under a strict one. + with _patches(table, allowlist="https://evil.example.com/cb")[0], _patches( + table, allowlist="https://evil.example.com/cb" + )[1]: + state = issue_oauth_state( + TEST_USER_ID, TEST_NB_SLUG, "https://evil.example.com/cb" + ) + with _patches(table)[0], _patches(table)[1]: + with pytest.raises(OAuthStateError) as exc: + validate_oauth_state(state) + assert exc.value.error_slug == "invalid_redirect_uri" + + def test_missing_state_rejected(self) -> None: + with pytest.raises(OAuthStateError) as exc: + validate_oauth_state("") + assert exc.value.error_slug == "missing_state" + + def test_malformed_state_rejected(self) -> None: + table = MockOAuthStateTable() + with _patches(table)[0], _patches(table)[1]: + with pytest.raises(OAuthStateError): + validate_oauth_state("not-valid-base64-json!!!") + + def test_state_without_nonce_rejected(self) -> None: + no_nonce = base64.urlsafe_b64encode( + json.dumps( + { + "user_id": TEST_USER_ID, + "nb_slug": TEST_NB_SLUG, + "redirect_uri": TEST_REDIRECT_URI, + } + ).encode() + ).decode() + with pytest.raises(OAuthStateError): + validate_oauth_state(no_nonce) From a37db8f74327ba155db41689b479298c8a37d616 Mon Sep 17 00:00:00 2001 From: Ian Patrick Hines Date: Sat, 20 Jun 2026 22:02:32 -0400 Subject: [PATCH 2/3] harden(oauth-init): validate nb_slug format before authorize redirect Reject nb_slug values that aren't lowercase-alphanumeric+hyphen so a crafted slug cannot distort the authorize host. Defense-in-depth from adversarial review; not a CSRF bypass (redirect_uri is fixed + allowlisted). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lambdas/nb_oauth_init/handler.py | 11 +++++++++++ tests/test_nb_oauth_init.py | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/lambdas/nb_oauth_init/handler.py b/src/lambdas/nb_oauth_init/handler.py index 0b2c8f0..1aa7ec9 100644 --- a/src/lambdas/nb_oauth_init/handler.py +++ b/src/lambdas/nb_oauth_init/handler.py @@ -23,6 +23,7 @@ import json import logging import os +import re import uuid from typing import Any, TypedDict from urllib.parse import urlencode @@ -52,6 +53,11 @@ "ERROR_REDIRECT_URL", "https://natassistant.com/connection-error" ) +# NationBuilder nation slugs are lowercase alphanumeric with hyphens. We embed +# the slug into the authorize host, so reject anything else to avoid steering +# the redirect to an attacker-influenced host. +NB_SLUG_RE = re.compile(r"^[a-z0-9][a-z0-9-]{0,62}$") + class LambdaResponse(TypedDict): """Lambda response type.""" @@ -105,6 +111,11 @@ def handler(event: dict[str, Any], context: Any) -> LambdaResponse: return create_redirect_response( f"{ERROR_REDIRECT_URL}?error=missing_nb_slug" ) + if not NB_SLUG_RE.match(nb_slug): + logger.error(f"Invalid nb_slug format: {nb_slug!r}") + return create_redirect_response( + f"{ERROR_REDIRECT_URL}?error=invalid_nb_slug" + ) if not OAUTH_CALLBACK_URL: logger.error("OAUTH_CALLBACK_URL is not configured") diff --git a/tests/test_nb_oauth_init.py b/tests/test_nb_oauth_init.py index 9c5126d..2d17dbe 100644 --- a/tests/test_nb_oauth_init.py +++ b/tests/test_nb_oauth_init.py @@ -87,6 +87,20 @@ def test_null_query_params_handled(self) -> None: assert response["statusCode"] == 302 assert "error=missing_nb_slug" in response["headers"]["Location"] + def test_malformed_nb_slug_rejected(self) -> None: + """A slug that would distort the authorize host is rejected.""" + table = MockOAuthStateTable() + for bad in ["evil.com/x", "Foo Bar", "slug?x=1", "-leading", "UPPER"]: + with ExitStack() as stack: + for p in _patches(table): + stack.enter_context(p) + response = handler( + {"queryStringParameters": {"nb_slug": bad}}, None + ) + assert response["statusCode"] == 302 + assert "error=invalid_nb_slug" in response["headers"]["Location"] + assert table.items == {} + def test_missing_callback_url_misconfigured(self) -> None: table = MockOAuthStateTable() with ExitStack() as stack: From 34a5cfb7927186c7c97f79fba6d41b083bd00fb2 Mon Sep 17 00:00:00 2001 From: Ian Patrick Hines Date: Mon, 22 Jun 2026 06:52:35 -0400 Subject: [PATCH 3/3] harden(oauth-init): never trust caller-supplied user_id (session-fixation) The init handler now always mints user_id server-side and ignores any query-string user_id. Trusting a caller-supplied user_id is an OAuth login-CSRF / session-fixation vector: an attacker could mint a state nonce bound to a user_id they control, lure a victim through authorize, and pin the victim's connection to that identity. The connect flow doubles as login, so a fresh connect has no prior trusted identity; the authoritative identity is the signed session token minted at the callback. Resolves the open security question that held #24 in needs-ian-judgment. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lambdas/nb_oauth_init/handler.py | 21 ++++++++++++++++----- tests/test_nb_oauth_init.py | 9 ++++++--- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/lambdas/nb_oauth_init/handler.py b/src/lambdas/nb_oauth_init/handler.py index 1aa7ec9..d6446b0 100644 --- a/src/lambdas/nb_oauth_init/handler.py +++ b/src/lambdas/nb_oauth_init/handler.py @@ -8,8 +8,15 @@ Flow: 1. Read ``nb_slug`` (the nation being connected) from the query string. - 2. Use the caller-supplied ``user_id`` or generate a fresh one (the NB connect - flow doubles as login, so a brand-new connect has no prior identity). + 2. Generate a fresh ``user_id`` server-side. We deliberately do NOT trust a + caller-supplied ``user_id``: doing so is an OAuth login-CSRF / session + fixation vector (an attacker could mint a nonce bound to a ``user_id`` they + control, lure a victim through authorize, and pin the victim's connection + to that identity). The connect flow doubles as login, so a fresh connect + has no prior trusted identity anyway; the authoritative identity is the + signed session token minted at the callback. Re-auth identity continuity, + if ever needed, must be derived from a *verified* session token, never a + query-string value. 3. Issue a single-use ``state`` bound to ``(user_id, nb_slug, redirect_uri)``, persisting its nonce in DynamoDB with a TTL. 4. 302-redirect to ``https://{nb_slug}.nationbuilder.com/oauth/authorize``. @@ -101,7 +108,10 @@ def handler(event: dict[str, Any], context: Any) -> LambdaResponse: Expected query parameters: - ``nb_slug`` (required): the NationBuilder nation slug to connect. - - ``user_id`` (optional): existing user identity; generated if absent. + + ``user_id`` is always generated server-side and is intentionally NOT read + from the request (see module docstring: trusting a client-supplied + ``user_id`` is a session-fixation vector). """ try: query_params = event.get("queryStringParameters") or {} @@ -123,8 +133,9 @@ def handler(event: dict[str, Any], context: Any) -> LambdaResponse: f"{ERROR_REDIRECT_URL}?error=server_misconfigured" ) - # NB connect doubles as login; mint a new user_id when none is supplied. - user_id = query_params.get("user_id") or f"user-{uuid.uuid4().hex}" + # NB connect doubles as login. Always mint the user_id server-side; a + # client-supplied value is never trusted (session-fixation defense). + user_id = f"user-{uuid.uuid4().hex}" try: state = issue_oauth_state( diff --git a/tests/test_nb_oauth_init.py b/tests/test_nb_oauth_init.py index 2d17dbe..a96adb0 100644 --- a/tests/test_nb_oauth_init.py +++ b/tests/test_nb_oauth_init.py @@ -155,7 +155,9 @@ def test_state_matches_stored_nonce(self) -> None: payload = json.loads(base64.urlsafe_b64decode(state).decode()) assert payload["nonce"] in table.items - def test_supplied_user_id_preserved(self) -> None: + def test_supplied_user_id_is_ignored(self) -> None: + """A caller-supplied user_id must NOT be trusted (session-fixation + defense): the issued state binds to a server-generated identity.""" table = MockOAuthStateTable() with ExitStack() as stack: for p in _patches(table): @@ -164,14 +166,15 @@ def test_supplied_user_id_preserved(self) -> None: { "queryStringParameters": { "nb_slug": TEST_NB_SLUG, - "user_id": "existing-user-7", + "user_id": "attacker-controlled-7", } }, None, ) assert response["statusCode"] == 302 stored = next(iter(table.items.values())) - assert stored["user_id"] == "existing-user-7" + assert stored["user_id"] != "attacker-controlled-7" + assert stored["user_id"].startswith("user-") def test_callback_url_off_allowlist_rejected(self) -> None: """If the configured callback URL is not allowlisted, issuance fails."""