From 1500adddface3ff579de1159d4e74f4674a11783 Mon Sep 17 00:00:00 2001 From: Perseus Date: Sun, 5 Jul 2026 12:57:52 -0500 Subject: [PATCH] security: bind OIDC login flow to the browser (login-CSRF guard) The OAuth state lived only in a process-global pool, so an attacker could replay their own code+state into a victim's browser and plant an attacker-owned session. /auth/login now sets a short-lived HttpOnly; SameSite=Lax plutus_oauth_state cookie and handle_callback requires the callback state to match it (constant-time) on top of the _pending nonce check. Fixes the HIGH login-CSRF finding from the 2026-07-05 security review. 254 tests pass (login_url now returns (url, state); added a state-cookie-mismatch test). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 11 ++++++++++ plutus_agent/server/app.py | 13 ++++++++---- plutus_agent/server/auth.py | 40 +++++++++++++++++++++++++++++-------- tests/test_auth.py | 40 ++++++++++++++++++++++++++++++------- 4 files changed, 85 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b92552a..2b40a92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to Plutus are documented here. ## [Unreleased] +### Security +- **OIDC login-CSRF: bind the OAuth flow to the browser that started it.** The + authorization `state` was held only in a process-global pool, so an attacker + could complete their own Google sign-in, capture their `code`+`state`, and feed + a victim's browser a `/auth/callback?code=…&state=…` link — planting an + attacker-owned session in the victim's browser (the victim would then operate + inside, and leak API keys to, the attacker's tenant). `/auth/login` now sets a + short-lived `HttpOnly; SameSite=Lax` `plutus_oauth_state` cookie and + `handle_callback` requires the callback `state` to match it (constant-time), + in addition to the existing `_pending` nonce check. (2026-07-05 security review) + ## [1.0.0] — 2026-06-27 **Plutus 1.0 — the billing loop is closed and the contract is frozen.** The diff --git a/plutus_agent/server/app.py b/plutus_agent/server/app.py index 9be999a..99904f5 100644 --- a/plutus_agent/server/app.py +++ b/plutus_agent/server/app.py @@ -255,11 +255,14 @@ def _authz_org(self, conn, requested, *, strict=False): def _auth_login(self): try: - url = authmod.login_url(self.ctx.cfg) + url, state = authmod.login_url(self.ctx.cfg) except authmod.AuthError as e: return self._send(500, views.simple_page( "Sign in", "Sign-in is misconfigured", html.escape(str(e)), ok=False)) - return self._send(200, views.login_page(url)) + # Bind this login attempt to the browser (login-CSRF guard): the callback + # state must match this cookie. + return self._send(200, views.login_page(url), headers={ + "Set-Cookie": authmod.set_state_cookie_header(state, self.ctx.cfg)}) def _client_ip(self) -> str: """Best-effort client IP for rate limiting (fix #59). Honors the first @@ -277,14 +280,16 @@ def _client_ip(self) -> str: def _auth_callback(self, conn, q): flat = {k: (v[0] if isinstance(v, list) else v) for k, v in q.items()} try: - token = authmod.handle_callback(conn, self.ctx.cfg, flat, - client_ip=self._client_ip()) + token = authmod.handle_callback( + conn, self.ctx.cfg, flat, client_ip=self._client_ip(), + cookie_state=authmod.read_cookie(self, authmod.STATE_COOKIE)) except authmod.AuthError as e: return self._send(403, views.simple_page( "Sign in", "Could not sign you in", html.escape(str(e)), ok=False)) self.send_response(303) self.send_header("Location", "/") self.send_header("Set-Cookie", authmod.set_cookie_header(token, self.ctx.cfg)) + self.send_header("Set-Cookie", authmod.clear_state_cookie_header(self.ctx.cfg)) self.end_headers() def _auth_logout(self, conn): diff --git a/plutus_agent/server/auth.py b/plutus_agent/server/auth.py index 99f1edd..9025749 100644 --- a/plutus_agent/server/auth.py +++ b/plutus_agent/server/auth.py @@ -39,6 +39,7 @@ JWKS_ENDPOINT = "https://www.googleapis.com/oauth2/v3/certs" SCOPES = "openid email profile" COOKIE = "plutus_session" +STATE_COOKIE = "plutus_oauth_state" # binds an in-flight OAuth login to the browser that began it (login-CSRF guard) _STATE_TTL = 600 # how long a login attempt's state/nonce stays valid (seconds) # Self-serve signup rate limiting (per hour globally) @@ -240,8 +241,12 @@ def _take_state(state: str) -> Optional[str]: # ---------------------------------------------------------------- the flow --- -def login_url(cfg) -> str: - """Build the Google authorization URL and stash its state/nonce.""" +def login_url(cfg) -> tuple[str, str]: + """Build the Google authorization URL and stash its state/nonce. + + Returns ``(url, state)``; the caller sets ``state`` as a short-lived cookie + on the browser so :func:`handle_callback` can bind the callback to it. + """ state = secrets.token_urlsafe(24) nonce = secrets.token_urlsafe(24) _remember_state(state, nonce) @@ -255,7 +260,7 @@ def login_url(cfg) -> str: "access_type": "online", "prompt": "select_account", } - return AUTH_ENDPOINT + "?" + urllib.parse.urlencode(params) + return AUTH_ENDPOINT + "?" + urllib.parse.urlencode(params), state def _exchange_code(cfg, code: str) -> dict: @@ -405,11 +410,14 @@ def _authorize_email(conn, cfg, email: str, name: Optional[str] = None, return None -def handle_callback(conn, cfg, q: dict, client_ip: Optional[str] = None) -> str: +def handle_callback(conn, cfg, q: dict, client_ip: Optional[str] = None, + cookie_state: Optional[str] = None) -> str: """Process /auth/callback query params; return a fresh session token. - ``client_ip`` (fix #59) feeds the per-IP signup throttle. Raises - :class:`AuthError` on any problem. + ``client_ip`` (fix #59) feeds the per-IP signup throttle. ``cookie_state`` is + the value of the ``plutus_oauth_state`` cookie set on this browser at + ``/auth/login``; the callback ``state`` must match it (login-CSRF guard). + Raises :class:`AuthError` on any problem. """ if q.get("error"): raise AuthError(f"Google returned an error: {q['error']}") @@ -417,6 +425,12 @@ def handle_callback(conn, cfg, q: dict, client_ip: Optional[str] = None) -> str: state = q.get("state") if not code or not state: raise AuthError("missing code or state") + # Login-CSRF guard: the callback state must match the state cookie this + # browser received at /auth/login. Without this an attacker could feed a + # victim's browser their own code+state and plant an attacker-owned session + # (the global _pending pool alone does not bind the flow to a browser). + if not cookie_state or not secrets.compare_digest(str(state), str(cookie_state)): + raise AuthError("sign-in state did not match this browser — start again") nonce = _take_state(state) if nonce is None: raise AuthError("invalid or expired sign-in state — try again") @@ -454,7 +468,7 @@ def csrf_token(session_token: str) -> str: # ----------------------------------------------------------- cookie helpers --- -def read_cookie(handler) -> str: +def read_cookie(handler, name: str = COOKIE) -> str: raw = handler.headers.get("Cookie", "") if not raw: return "" @@ -463,7 +477,7 @@ def read_cookie(handler) -> str: jar.load(raw) except Exception: return "" - m = jar.get(COOKIE) + m = jar.get(name) return m.value if m else "" @@ -483,6 +497,16 @@ def clear_cookie_header(cfg) -> str: return f"{COOKIE}=; Path=/; HttpOnly; SameSite=Lax{_secure_attr(cfg)}; Max-Age=0" +def set_state_cookie_header(state: str, cfg) -> str: + """Short-lived cookie binding an in-flight OAuth login to this browser.""" + return (f"{STATE_COOKIE}={state}; Path=/; HttpOnly; SameSite=Lax" + f"{_secure_attr(cfg)}; Max-Age={int(_STATE_TTL)}") + + +def clear_state_cookie_header(cfg) -> str: + return f"{STATE_COOKIE}=; Path=/; HttpOnly; SameSite=Lax{_secure_attr(cfg)}; Max-Age=0" + + def current_user(handler, conn): """The signed-in user row for this request, or None.""" return db.session_user(conn, read_cookie(handler)) diff --git a/tests/test_auth.py b/tests/test_auth.py index 1c1f751..924bceb 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -177,36 +177,62 @@ def test_claims_validation_rejects_bad_token(self): authmod._claims_from_id_token(_jwt(claims), self.cfg, nonce) def test_callback_creates_session(self): - url = authmod.login_url(self.cfg) + url, state = authmod.login_url(self.cfg) qs = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) - state, nonce = qs["state"][0], qs["nonce"][0] + nonce = qs["nonce"][0] orig = authmod._exchange_code authmod._exchange_code = lambda cfg, code: {"id_token": _jwt(self._good_claims(nonce))} try: token = authmod.handle_callback( - self.conn, self.cfg, {"code": "abc", "state": state}) + self.conn, self.cfg, {"code": "abc", "state": state}, + cookie_state=state) finally: authmod._exchange_code = orig self.assertEqual(db.session_user(self.conn, token)["email"], "owner@example.com") def test_callback_rejects_unknown_state(self): + # cookie_state matches the (never-issued) callback state, so this exercises + # the _pending lookup rejection rather than the login-CSRF guard. with self.assertRaises(authmod.AuthError): authmod.handle_callback(self.conn, self.cfg, - {"code": "abc", "state": "never-issued"}) + {"code": "abc", "state": "never-issued"}, + cookie_state="never-issued") + + def test_callback_rejects_state_cookie_mismatch(self): + """Login-CSRF guard: a valid pending state with a missing/mismatched + browser cookie is rejected, so an attacker's code+state replayed into a + victim's browser cannot plant an attacker-owned session.""" + url, state = authmod.login_url(self.cfg) + qs = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) + nonce = qs["nonce"][0] + orig = authmod._exchange_code + authmod._exchange_code = lambda cfg, code: {"id_token": _jwt(self._good_claims(nonce))} + try: + with self.assertRaises(authmod.AuthError): + authmod.handle_callback(self.conn, self.cfg, + {"code": "abc", "state": state}, + cookie_state="attacker-different") + with self.assertRaises(authmod.AuthError): + authmod.handle_callback(self.conn, self.cfg, + {"code": "abc", "state": state}, + cookie_state=None) + finally: + authmod._exchange_code = orig def test_callback_open_signup_creates_session_for_stranger(self): cfg = _auth_cfg(allow_signup=True) - url = authmod.login_url(cfg) + url, state = authmod.login_url(cfg) qs = urllib.parse.parse_qs(urllib.parse.urlparse(url).query) - state, nonce = qs["state"][0], qs["nonce"][0] + nonce = qs["nonce"][0] claims = self._good_claims(nonce, email="newbie@elsewhere.com", name="Newbie") orig = authmod._exchange_code authmod._exchange_code = lambda c, code: {"id_token": _jwt(claims)} try: token = authmod.handle_callback(self.conn, cfg, - {"code": "abc", "state": state}) + {"code": "abc", "state": state}, + cookie_state=state) finally: authmod._exchange_code = orig self.assertEqual(db.session_user(self.conn, token)["email"], "newbie@elsewhere.com")