diff --git a/app.py b/app.py index 6a0dd16c9..75a7da875 100644 --- a/app.py +++ b/app.py @@ -362,7 +362,7 @@ if AUTH_ENABLED: path = request.url.path # A genuine CORS preflight (OPTIONS + Access-Control-Request-Method) # carries no credentials by design and must reach CORSMiddleware to be - # answered. AuthMiddleware is the outermost middleware, so gating the + # answered. AuthMiddleware runs outside CORSMiddleware, so gating the # preflight on auth 401s it before CORS can respond -- which blocks # every cross-origin browser/WebView client before the real request # is sent. Let real preflights through (only OPTIONS w/ the ACRM diff --git a/core/middleware.py b/core/middleware.py index 3435e9f27..3834bbe72 100644 --- a/core/middleware.py +++ b/core/middleware.py @@ -38,32 +38,66 @@ def is_codex_cookbook_path(path: str) -> bool: def is_odysseus_bearer_authorization(value: str | None) -> bool: - """Recognize the Bearer scheme with normal case and whitespace freedom.""" + """Recognize an Odysseus Bearer value, including proxy-combined fields.""" if not isinstance(value, str): return False - parts = value.strip().split(None, 1) - return ( - len(parts) == 2 - and parts[0].casefold() == "bearer" - and parts[1].startswith("ody_") - ) + for candidate in value.split(","): + parts = candidate.strip().split(None, 1) + if ( + len(parts) == 2 + and parts[0].casefold() == "bearer" + and parts[1].startswith("ody_") + ): + return True + return False + + +def _header_values(headers, name: str) -> list[str]: + """Return every field value, with a mapping fallback for direct callers.""" + getlist = getattr(headers, "getlist", None) + if callable(getlist): + values = getlist(name) + else: + value = headers.get(name) + values = value if isinstance(value, (list, tuple)) else [value] + return [value for value in values if isinstance(value, str)] + + +def _internal_header_matches(value: str) -> bool: + """Compare raw or proxy-combined values without obs-text type failures.""" + candidates = [value] + if "," in value: + candidates.extend(part.strip() for part in value.split(",")) + try: + expected = INTERNAL_TOOL_TOKEN.encode("utf-8") + except (AttributeError, UnicodeError): + return False + for candidate in candidates: + try: + if secrets.compare_digest(candidate.encode("utf-8"), expected): + return True + except (AttributeError, TypeError, UnicodeError): + continue + return False def require_codex_cookbook_browser(request: Request) -> None: """Reject bearer and internal-tool principals at the shared boundary.""" current_user = getattr(request.state, "current_user", None) - authorization = request.headers.get("authorization", "") - internal_header = request.headers.get(INTERNAL_TOOL_HEADER) - has_internal_header = bool( - internal_header - and secrets.compare_digest(internal_header, INTERNAL_TOOL_TOKEN) - ) if ( getattr(request.state, "api_token", False) or current_user == "api" or current_user == INTERNAL_TOOL_USER - or is_odysseus_bearer_authorization(authorization) - or has_internal_header + ): + raise HTTPException(403, "Forbidden") + if any( + is_odysseus_bearer_authorization(value) + for value in _header_values(request.headers, "authorization") + ): + raise HTTPException(403, "Forbidden") + if any( + _internal_header_matches(value) + for value in _header_values(request.headers, INTERNAL_TOOL_HEADER) ): raise HTTPException(403, "Forbidden") diff --git a/tests/test_codex_cookbook_admin_gate.py b/tests/test_codex_cookbook_admin_gate.py index bb4f43e15..0921d1a89 100644 --- a/tests/test_codex_cookbook_admin_gate.py +++ b/tests/test_codex_cookbook_admin_gate.py @@ -84,6 +84,61 @@ PRE_BODY_CREDENTIALS = [ ), ] +_INTERNAL_HEADER_BYTES = INTERNAL_TOOL_HEADER.lower().encode("ascii") +_INTERNAL_TOKEN_BYTES = INTERNAL_TOOL_TOKEN.encode("utf-8") +DUPLICATE_PRE_BODY_HEADERS = [ + pytest.param( + [ + (b"authorization", b"Basic placeholder"), + (b"authorization", b"Bearer ody_second_value"), + ], + id="bearer-second", + ), + pytest.param( + [ + (b"authorization", b"Bearer ody_first_value"), + (b"authorization", b"Basic placeholder"), + ], + id="bearer-first", + ), + pytest.param( + [ + (_INTERNAL_HEADER_BYTES, b"invalid"), + (_INTERNAL_HEADER_BYTES, _INTERNAL_TOKEN_BYTES), + ], + id="internal-second", + ), + pytest.param( + [ + (_INTERNAL_HEADER_BYTES, _INTERNAL_TOKEN_BYTES), + (_INTERNAL_HEADER_BYTES, b"invalid"), + ], + id="internal-first", + ), + pytest.param( + [(b"authorization", b"Basic placeholder, Bearer ody_combined")], + id="bearer-proxy-combined", + ), + pytest.param( + [(_INTERNAL_HEADER_BYTES, b"invalid, " + _INTERNAL_TOKEN_BYTES)], + id="internal-proxy-combined", + ), + pytest.param( + [ + (_INTERNAL_HEADER_BYTES, b"\xff"), + (b"authorization", b"Bearer ody_after_obs_text"), + ], + id="non-ascii-before-bearer", + ), + pytest.param( + [ + (_INTERNAL_HEADER_BYTES, b"\xff"), + (_INTERNAL_HEADER_BYTES, _INTERNAL_TOKEN_BYTES), + ], + id="non-ascii-before-internal", + ), +] + class _PoisonPath: def __fspath__(self): @@ -195,6 +250,24 @@ def test_gate_precedes_json_body_validation(blocked_client, headers): assert side_effects == [] +@pytest.mark.parametrize("raw_headers", DUPLICATE_PRE_BODY_HEADERS) +def test_all_duplicate_and_obs_text_credentials_precede_body_validation( + blocked_client, + raw_headers, +): + client, side_effects = blocked_client + + response = client.post( + "/api/codex/cookbook/serve", + headers=[*raw_headers, (b"content-type", b"application/json")], + content=b"{not-json", + ) + + assert response.status_code == 403 + assert response.json() == {"detail": "Forbidden"} + assert side_effects == [] + + @pytest.mark.parametrize( "value", [ @@ -203,6 +276,7 @@ def test_gate_precedes_json_body_validation(blocked_client, headers): "BEARER ody_token", "BeArEr\tody_token", " bearer \t ody_token ", + "Basic placeholder, Bearer ody_token", ], ) def test_odysseus_bearer_parser_accepts_scheme_case_and_sp_htab(value): diff --git a/tests/test_cors_preflight.py b/tests/test_cors_preflight.py index 24f69290b..0a834678b 100644 --- a/tests/test_cors_preflight.py +++ b/tests/test_cors_preflight.py @@ -1,10 +1,10 @@ """Regression test for the CORS-preflight auth bypass. -AuthMiddleware is the outermost middleware, so it used to 401 the credential-less -OPTIONS preflight before CORSMiddleware could answer it -- which blocks every -cross-origin browser/WebView client before the real request is ever sent. The -fix lets a genuine preflight through; `is_cors_preflight` is the pure predicate -it uses. Guard it so the bypass can't silently regress. +AuthMiddleware runs outside CORSMiddleware, so it used to 401 the credential-less +OPTIONS preflight before CORS could answer it -- which blocks every cross-origin +browser/WebView client before the real request is ever sent. The fix lets a +genuine preflight through; `is_cors_preflight` is the pure predicate it uses. +Guard it so the bypass can't silently regress. """ import os