From d9c182a09b3ddf523112422517f86b610859d9a6 Mon Sep 17 00:00:00 2001 From: jzhao234 Date: Fri, 11 Sep 2026 17:29:03 +0000 Subject: [PATCH] fix(auth): validate issuer, audience and expiry on the token response; tell users to retry a rejected code TL;DR: Review follow-ups on the central-auth client. Explorer now checks that the identity it receives was minted by the issuer it asked, for this client, and is still within its window; a consumed or superseded sign-in code tells the user to try again instead of blaming an outage; and two constant-time compares can no longer crash on non-ASCII input. Problem: - exchange() trusted sub, email and nonce from the token response and ignored iss, aud and exp. The response arrives over an authenticated TLS backchannel, so this was a misconfiguration risk rather than an attack path, but a response minted for another client or replayed after its window would still have created a session. - Every token-endpoint failure became a 502 "authentication service is unavailable". The auth service answers 400 invalid_grant for a consumed, expired, or superseded code, which happens whenever a user has two tabs sign in at once (the newer /authorize invalidates the older code). That is a retry, not an outage, and the message sent people looking for a problem that did not exist. - hmac.compare_digest raises TypeError on str arguments containing non-ASCII characters. The state value comes from the callback query string, so any visitor could turn the callback into a 500. The nonce compare had the same shape. - The bootstrap wrote .env with mode 0640 although it now holds the central-auth client secret alongside the session secret and AWS keys. Fix: - exchange() requires iss to equal the configured issuer origin, aud to equal the client id, and exp to be an integer not more than 60 seconds in the past (assertions live five minutes; the skew tolerance covers clock drift between hosts). Booleans are rejected as exp even though they are ints in Python. - New CodeExchangeRejectedError for HTTP 400 from the token endpoint; the callback maps it to 400 "Sign-in expired. Try again." 401 (invalid_client) and 5xx stay 502 because they are deployment or availability problems. - State and nonce compares operate on UTF-8 bytes. - bootstrap.sh writes .env with mode 0600. Tests: - New: wrong aud, wrong iss, expired exp, string exp, and boolean exp are all rejected; 10 seconds of skew is tolerated; a non-ASCII nonce is rejected rather than raising; HTTP 400 from the token endpoint is CodeExchangeRejectedError while 401 stays a plain CentralAuthError; the callback returns 400 with "Try again" for a rejected code and never exchanges a code when the state is non-ASCII. - Updated: the two unit-test fakes now return iss, aud and exp like the real service. - Ran: .venv/bin/python -m pytest -q (98 passed), ruff check, ruff format --check, bash -n and shellcheck on bootstrap.sh, update.sh and explorer-cli. (cherry picked from commit 5cb7fc346419f3c8fd109d1447b126869d34a662) --- app/auth.py | 4 +- app/central_auth.py | 55 ++++++++++-- app/main.py | 7 ++ scripts/bootstrap.sh | 4 +- tests/test_central_auth.py | 122 ++++++++++++++++++++++++++- tests/test_central_auth_endpoints.py | 26 ++++++ 6 files changed, 208 insertions(+), 10 deletions(-) diff --git a/app/auth.py b/app/auth.py index 7fd02a0..47af2d1 100644 --- a/app/auth.py +++ b/app/auth.py @@ -275,7 +275,9 @@ def complete_login( or not isinstance(nonce, str) or not isinstance(next_path, str) or not isinstance(created_at, int) - or not hmac.compare_digest(expected_state, state) + or not hmac.compare_digest( + expected_state.encode("utf-8"), state.encode("utf-8") + ) or now < created_at or now - created_at > LOGIN_TRANSACTION_SECONDS ): diff --git a/app/central_auth.py b/app/central_auth.py index cf5369f..c3e99e9 100644 --- a/app/central_auth.py +++ b/app/central_auth.py @@ -57,6 +57,25 @@ class AuthTransactionError(CentralAuthError): """The browser callback did not match a live login transaction.""" +class CodeExchangeRejectedError(CentralAuthError): + """The auth service refused the code: expired, replayed, or superseded. + + This is the user's problem to retry, not an outage: a second tab, a slow + click, or a stale bookmark produces it. Callers should say "try again", + not "the service is unavailable". + """ + + +# Tolerated skew between this host's clock and the auth service's when +# checking the assertion expiry. The assertion lives five minutes. +CLOCK_SKEW_SECONDS = 60 + + +def _constant_time_equal(expected: str, actual: str) -> bool: + """compare_digest on str raises TypeError for non-ASCII; compare bytes.""" + return hmac.compare_digest(expected.encode("utf-8"), actual.encode("utf-8")) + + @dataclass(frozen=True) class AuthenticatedPrincipal: subject: str @@ -229,12 +248,19 @@ def exchange( try: with _open_token_request(request, self.timeout_seconds) as response: raw = response.read(MAX_TOKEN_RESPONSE_BYTES + 1) - except ( - urllib.error.HTTPError, - urllib.error.URLError, - TimeoutError, - OSError, - ) as exc: + except urllib.error.HTTPError as exc: + # 400 is the auth service's invalid_grant: the code was consumed, + # expired, or superseded by a newer /authorize for this browser. + # Anything else (401 invalid_client, 5xx) is a deployment or + # availability problem. + if exc.code == 400: + raise CodeExchangeRejectedError( + "The authentication service rejected the sign-in code" + ) from exc + raise CentralAuthError( + "The authentication service rejected the code exchange" + ) from exc + except (urllib.error.URLError, TimeoutError, OSError) as exc: raise CentralAuthError( "The authentication service rejected the code exchange" ) from exc @@ -250,13 +276,28 @@ def exchange( subject = payload.get("sub") email = payload.get("email") nonce = payload.get("nonce") + issuer = payload.get("iss") + audience = payload.get("aud") + expires_at = payload.get("exp") + now = int(time.time()) + # The response arrives over an authenticated TLS backchannel, so these + # checks defend against misconfiguration rather than an attacker: a + # response minted by a different issuer, for a different client, or + # replayed after its assertion window must not create a session. if ( not isinstance(subject, str) or not subject or len(subject) > 255 or not isinstance(email, str) or not isinstance(nonce, str) - or not hmac.compare_digest(nonce, expected_nonce) + or not _constant_time_equal(expected_nonce, nonce) + or not isinstance(issuer, str) + or issuer.rstrip("/") != self.issuer_url + or not isinstance(audience, str) + or audience != self.client_id + or isinstance(expires_at, bool) + or not isinstance(expires_at, int) + or expires_at + CLOCK_SKEW_SECONDS <= now ): raise CentralAuthError("The authentication response was invalid") try: diff --git a/app/main.py b/app/main.py index ebdc16d..0461dd5 100644 --- a/app/main.py +++ b/app/main.py @@ -33,6 +33,7 @@ AccessDeniedError, AuthTransactionError, CentralAuthError, + CodeExchangeRejectedError, verify_csrf_token, ) from app.config import settings @@ -616,6 +617,12 @@ def auth_callback( status_code=403, detail="Your account does not have access to this Explorer instance.", ) from exc + except CodeExchangeRejectedError as exc: + # A consumed, expired, or superseded code (for example a second tab + # that started its own sign-in) is a retry, not an outage. + raise HTTPException( + status_code=400, detail="Sign-in expired. Try again." + ) from exc except CentralAuthError as exc: raise HTTPException( status_code=502, detail="The authentication service is unavailable." diff --git a/scripts/bootstrap.sh b/scripts/bootstrap.sh index 71b7f2f..ff24cf4 100755 --- a/scripts/bootstrap.sh +++ b/scripts/bootstrap.sh @@ -237,7 +237,9 @@ EMAIL_S3_BUCKET="$EMAIL_S3_BUCKET" # AWS_SECRET_ACCESS_KEY= EOF chown "$APP_USER:$APP_USER" "$ENV_FILE" -chmod 0640 "$ENV_FILE" +# Owner-only: this file carries the session secret, the central-auth client +# secret, and any AWS keys. +chmod 0600 "$ENV_FILE" ok "env seeded at $ENV_FILE" # ── step 4b: optional encrypted config bundle ─────────────────────────── diff --git a/tests/test_central_auth.py b/tests/test_central_auth.py index 59b9b75..562d1ca 100644 --- a/tests/test_central_auth.py +++ b/tests/test_central_auth.py @@ -9,6 +9,8 @@ import json import sqlite3 import stat +import time +import urllib.error import urllib.parse import pytest @@ -18,6 +20,7 @@ CentralAuthClient, CentralAuthError, CentralAuthStore, + CodeExchangeRejectedError, csrf_token_for_session, verify_csrf_token, ) @@ -117,6 +120,9 @@ def read(self, _limit): "sub": "account-123", "email": "Alice@Example.com", "nonce": "expected-nonce", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, } ).encode() @@ -192,7 +198,16 @@ def __exit__(self, *args): return None def read(self, _limit): - return b'{"sub":"account-123","email":"alice@example.com","nonce":"wrong"}' + return json.dumps( + { + "sub": "account-123", + "email": "alice@example.com", + "nonce": "wrong", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, + } + ).encode() monkeypatch.setattr( "app.central_auth._open_token_request", lambda *_args: Response() @@ -210,3 +225,108 @@ def read(self, _limit): code_verifier="pkce-verifier", expected_nonce="expected-nonce", ) + + +def _client() -> CentralAuthClient: + return CentralAuthClient( + issuer_url="https://auth.example.com", + public_url="https://explorer.example.com", + client_id="explorer", + client_secret="a-random-client-secret-with-32-bytes", + ) + + +def _fake_response(payload: dict): + class Response: + def __enter__(self): + return self + + def __exit__(self, *_args): + return None + + def read(self, _limit): + return json.dumps(payload).encode() + + return Response() + + +def _valid_payload() -> dict: + return { + "sub": "account-123", + "email": "alice@example.com", + "nonce": "expected-nonce", + "iss": "https://auth.example.com", + "aud": "explorer", + "exp": int(time.time()) + 300, + } + + +@pytest.mark.parametrize( + "mutation", + [ + {"aud": "lens"}, + {"iss": "https://other-auth.example.com"}, + {"exp": int(time.time()) - 120}, + {"exp": "soon"}, + {"exp": True}, + ], +) +def test_code_exchange_rejects_wrong_audience_issuer_or_expired( + monkeypatch, mutation +) -> None: + payload = _valid_payload() | mutation + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + with pytest.raises(CentralAuthError, match="invalid"): + _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + + +def test_code_exchange_tolerates_small_clock_skew(monkeypatch) -> None: + payload = _valid_payload() | {"exp": int(time.time()) - 10} + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + principal = _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + assert principal.subject == "account-123" + + +def test_non_ascii_nonce_from_issuer_is_rejected_not_crashed(monkeypatch) -> None: + payload = _valid_payload() | {"nonce": "expected-nonc\u00e9"} + monkeypatch.setattr( + "app.central_auth._open_token_request", + lambda *_args: _fake_response(payload), + ) + with pytest.raises(CentralAuthError, match="invalid"): + _client().exchange( + code="code", code_verifier="v" * 43, expected_nonce="expected-nonce" + ) + + +def test_http_400_from_token_endpoint_is_a_rejected_code(monkeypatch) -> None: + def refuse(*_args): + raise urllib.error.HTTPError( + "https://auth.example.com/token", 400, "Bad Request", {}, None + ) + + monkeypatch.setattr("app.central_auth._open_token_request", refuse) + with pytest.raises(CodeExchangeRejectedError): + _client().exchange(code="code", code_verifier="v" * 43, expected_nonce="n") + + +def test_http_401_from_token_endpoint_is_an_outage_not_a_retry(monkeypatch) -> None: + def refuse(*_args): + raise urllib.error.HTTPError( + "https://auth.example.com/token", 401, "Unauthorized", {}, None + ) + + monkeypatch.setattr("app.central_auth._open_token_request", refuse) + with pytest.raises(CentralAuthError) as excinfo: + _client().exchange(code="code", code_verifier="v" * 43, expected_nonce="n") + assert not isinstance(excinfo.value, CodeExchangeRejectedError) diff --git a/tests/test_central_auth_endpoints.py b/tests/test_central_auth_endpoints.py index 0e211b5..40d619a 100644 --- a/tests/test_central_auth_endpoints.py +++ b/tests/test_central_auth_endpoints.py @@ -17,6 +17,7 @@ CentralAuthClient, CentralAuthError, CentralAuthStore, + CodeExchangeRejectedError, ) @@ -191,6 +192,31 @@ def test_callback_fails_closed_when_code_exchange_fails(central_client) -> None: assert CENTRAL_AUTH_COOKIE_NAME not in client.cookies +def test_rejected_code_is_a_retry_not_an_outage(central_client) -> None: + client, _store = central_client + FakeAuthClient.exchange_error = CodeExchangeRejectedError("code consumed") + + response = complete_login(client) + + assert response.status_code == 400 + assert "Try again" in response.text + assert CENTRAL_AUTH_COOKIE_NAME not in client.cookies + + +def test_non_ascii_state_is_rejected_not_crashed(central_client) -> None: + client, _store = central_client + begin_login(client) + + response = client.get( + "/auth/callback", + params={"code": "irrelevant", "state": "\u00e9tat"}, + follow_redirects=False, + ) + + assert response.status_code == 400 + assert FakeAuthClient.exchanged_codes == [] + + def test_external_next_url_is_not_used_after_callback(central_client) -> None: client, _store = central_client