From 745bf4324e36f7692728700128683c4849a79cad Mon Sep 17 00:00:00 2001 From: greatmengqi Date: Wed, 8 Apr 2026 13:18:44 +0800 Subject: [PATCH] security(auth): strict JWT validation in middleware (fix junk cookie bypass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AUTH_TEST_PLAN test 7.5.8 expects junk cookies to be rejected with 401. The previous middleware behaviour was "presence-only": check that some access_token cookie exists, then pass through. In combination with my Task-12 decision to skip @require_auth decorators on routes, this created a gap where a request with any cookie-shaped string (e.g. access_token=not-a-jwt) would bypass authentication on routes that do not touch the repository (/api/models, /api/mcp/config, /api/memory, /api/skills, …). Fix: middleware now calls get_current_user_from_request() strictly and catches the resulting HTTPException to render a 401 with the proper fine-grained error code (token_invalid, token_expired, user_not_found, …). On success it stamps request.state.user and the contextvar so repository-layer owner filters work downstream. The 4 old "_with_cookie_passes" tests in test_auth_middleware.py were written for the presence-only behaviour; they asserted that a junk cookie would make the handler return 200. They are renamed to "_with_junk_cookie_rejected" and their assertions flipped to 401. The negative path (no cookie → 401 not_authenticated) is unchanged. Verified: no cookie → 401 not_authenticated junk cookie → 401 token_invalid (the fixed bug) expired cookie → 401 token_expired Tests: 284 passed (auth + persistence + isolation) Lint: clean --- backend/app/gateway/auth_middleware.py | 54 ++++++++++++++++++-------- backend/tests/test_auth_middleware.py | 22 +++++++---- 2 files changed, 52 insertions(+), 24 deletions(-) diff --git a/backend/app/gateway/auth_middleware.py b/backend/app/gateway/auth_middleware.py index fda05e93d..a2ef846c0 100644 --- a/backend/app/gateway/auth_middleware.py +++ b/backend/app/gateway/auth_middleware.py @@ -47,12 +47,23 @@ def _is_public(path: str) -> bool: class AuthMiddleware(BaseHTTPMiddleware): - """Coarse-grained auth gate: reject requests without a valid session cookie. + """Strict auth gate: reject requests without a valid session. - This does NOT verify JWT signature or user existence — that is the job of - ``get_current_user_from_request`` in deps.py (called by ``@require_auth``). - The middleware only checks *presence* of the cookie so that new endpoints - that forget ``@require_auth`` are not completely exposed. + Two-stage check for non-public paths: + + 1. Cookie presence — return 401 NOT_AUTHENTICATED if missing + 2. JWT validation via ``get_optional_user_from_request`` — return 401 + TOKEN_INVALID if the token is absent, malformed, expired, or the + signed user does not exist / is stale + + On success, stamps ``request.state.user`` and the + ``deerflow.runtime.user_context`` contextvar so that repository-layer + owner filters work downstream without every route needing a + ``@require_auth`` decorator. Routes that need per-resource + authorization (e.g. "user A cannot read user B's thread by guessing + the URL") should additionally use ``@require_permission(..., + owner_check=True)`` for explicit enforcement — but authentication + itself is fully handled here. """ def __init__(self, app: ASGIApp) -> None: @@ -74,18 +85,29 @@ class AuthMiddleware(BaseHTTPMiddleware): }, ) - # Resolve the full user now so repository-layer owner filters - # can read from the contextvar. We use the "optional" flavour so - # middleware never raises on bad tokens — the cookie-presence - # check above plus the @require_auth decorator provide the - # strict gates. A stale/invalid token yields user=None here; - # the request continues without a contextvar, and any protected - # endpoint will still be rejected by @require_auth. - from app.gateway.deps import get_optional_user_from_request + # Strict JWT validation: reject junk/expired tokens with 401 + # right here instead of silently passing through. This closes + # the "junk cookie bypass" gap (AUTH_TEST_PLAN test 7.5.8): + # without this, non-isolation routes like /api/models would + # accept any cookie-shaped string as authentication. + # + # We call the *strict* resolver so that fine-grained error + # codes (token_expired, token_invalid, user_not_found, …) + # propagate from AuthErrorCode, not get flattened into one + # generic code. BaseHTTPMiddleware doesn't let HTTPException + # bubble up, so we catch and render it as JSONResponse here. + # + # On success we stamp request.state.user and the contextvar + # so repository-layer owner filters work downstream without + # every route needing a decorator. + from fastapi import HTTPException - user = await get_optional_user_from_request(request) - if user is None: - return await call_next(request) + from app.gateway.deps import get_current_user_from_request + + try: + user = await get_current_user_from_request(request) + except HTTPException as exc: + return JSONResponse(status_code=exc.status_code, content={"detail": exc.detail}) request.state.user = user token = set_current_user(user) diff --git a/backend/tests/test_auth_middleware.py b/backend/tests/test_auth_middleware.py index 64f8604f0..398f9cec6 100644 --- a/backend/tests/test_auth_middleware.py +++ b/backend/tests/test_auth_middleware.py @@ -161,9 +161,12 @@ def test_protected_path_no_cookie_returns_401(client): assert body["detail"]["code"] == "not_authenticated" -def test_protected_path_with_cookie_passes(client): +def test_protected_path_with_junk_cookie_rejected(client): + """Junk cookie → 401. Middleware strictly validates the JWT now + (AUTH_TEST_PLAN test 7.5.8); it no longer silently passes bad + tokens through to the route handler.""" res = client.get("/api/models", cookies={"access_token": "some-token"}) - assert res.status_code == 200 + assert res.status_code == 401 def test_protected_post_no_cookie_returns_401(client): @@ -189,16 +192,18 @@ def test_protected_patch_no_cookie(client): assert res.status_code == 401 -def test_put_with_cookie_passes(client): +def test_put_with_junk_cookie_rejected(client): + """Junk cookie on PUT → 401 (strict JWT validation in middleware).""" client.cookies.set("access_token", "tok") res = client.put("/api/mcp/config") - assert res.status_code == 200 + assert res.status_code == 401 -def test_delete_with_cookie_passes(client): +def test_delete_with_junk_cookie_rejected(client): + """Junk cookie on DELETE → 401 (strict JWT validation in middleware).""" client.cookies.set("access_token", "tok") res = client.delete("/api/threads/abc") - assert res.status_code == 200 + assert res.status_code == 401 # ── Fail-closed: unknown future endpoints ───────────────────────────────── @@ -210,7 +215,8 @@ def test_unknown_endpoint_no_cookie_returns_401(client): assert res.status_code == 401 -def test_unknown_endpoint_with_cookie_passes(client): +def test_unknown_endpoint_with_junk_cookie_rejected(client): + """New endpoints are also protected by strict JWT validation.""" client.cookies.set("access_token", "tok") res = client.get("/api/future-endpoint") - assert res.status_code == 200 + assert res.status_code == 401