diff --git a/app.py b/app.py index d4b798fd5..268eafc83 100644 --- a/app.py +++ b/app.py @@ -396,10 +396,18 @@ if AUTH_ENABLED: # bearer token still follows its token capability boundary; local # requests without one keep the documented bypass behavior. auth_header = request.headers.get("authorization", "") + auth_scheme, auth_separator, auth_credentials = auth_header.partition(" ") + raw_api_token = ( + auth_credentials.lstrip(" ") + if auth_separator and auth_scheme.casefold() == "bearer" + else "" + ) + if not raw_api_token.startswith("ody_"): + raw_api_token = "" if ( LOCALHOST_BYPASS and _is_trusted_loopback(request) - and not auth_header.startswith("Bearer ody_") + and not raw_api_token ): return await call_next(request) if not auth_manager.is_configured: @@ -409,8 +417,8 @@ if AUTH_ENABLED: return JSONResponse(status_code=401, content={"error": "Setup required"}) # --- Bearer token auth (API tokens for external integrations) --- - if auth_header.startswith("Bearer ody_"): - raw_token = auth_header[7:] + if raw_api_token: + raw_token = raw_api_token # Sanity check: tokens are "ody_" + 43 chars of base64 if len(raw_token) < 12 or len(raw_token) > 100: return JSONResponse(status_code=401, content={"error": "Invalid API token"}) diff --git a/routes/api_token_routes.py b/routes/api_token_routes.py index ac51169b2..8268f3495 100644 --- a/routes/api_token_routes.py +++ b/routes/api_token_routes.py @@ -54,7 +54,6 @@ def _normalize_scopes(scopes: str | list[str] | None = None, profile: str | None ensure_before("calendar:write", "calendar:read") ensure_before("memory:write", "memory:read") ensure_before("email:draft", "email:read") - ensure_before("cookbook:launch", "cookbook:read") return normalized or [DEFAULT_SCOPES] diff --git a/src/api_token_capabilities.py b/src/api_token_capabilities.py index be96ac260..21c5e97f7 100644 --- a/src/api_token_capabilities.py +++ b/src/api_token_capabilities.py @@ -36,7 +36,6 @@ ALL_API_TOKEN_SCOPES = frozenset().union( MEMORY_READ_SCOPES, CALENDAR_READ_SCOPES, DOCS_READ_SCOPES, - COOKBOOK_READ_SCOPES, ) @@ -95,16 +94,17 @@ def _capability( def _normalize_route_path(path: str) -> str | None: """Normalize the single trailing slash FastAPI redirects by default. - Everything else stays strict. Query strings and fragments do not belong in - ASGI ``scope['path']``; repeated separators, dot segments, backslashes, and - control characters are treated as malformed instead of being normalized - into a capability match. + Everything else stays strict. ASGI ``scope['path']`` is already decoded, so + question marks and hashes can be legitimate path-segment data rather than + query or fragment delimiters. Repeated separators, dot segments, + backslashes, and control characters are treated as malformed instead of + being normalized into a capability match. """ if not isinstance(path, str) or not path.startswith("/"): return None if any(ord(char) < 32 or ord(char) == 127 for char in path): return None - if "?" in path or "#" in path or "\\" in path: + if "\\" in path: return None if path != "/" and path.endswith("/"): path = path[:-1] @@ -139,9 +139,15 @@ def _route_path_from_scope(scope: Mapping[str, object]) -> str: return "" if not isinstance(root_path, str): root_path = "" - root_path = root_path.rstrip("/") - if root_path and (path == root_path or path.startswith(root_path + "/")): - path = path[len(root_path):] or "/" + if root_path: + if path == root_path: + path = "/" + elif ( + path.startswith(root_path) + and len(path) > len(root_path) + and path[len(root_path)] == "/" + ): + path = path[len(root_path):] return path @@ -221,27 +227,6 @@ API_TOKEN_ROUTE_CAPABILITIES: tuple[ApiTokenRouteCapability, ...] = ( _capability("GET", "/api/codex/documents/{doc_id}", DOCS_READ_SCOPES), _capability("POST", "/api/codex/documents", DOCS_WRITE_SCOPES), _capability("DELETE", "/api/codex/documents/{doc_id}", DOCS_WRITE_SCOPES), - _capability("GET", "/api/codex/cookbook/tasks", COOKBOOK_READ_SCOPES), - _capability("GET", "/api/codex/cookbook/servers", COOKBOOK_READ_SCOPES), - _capability( - "GET", - "/api/codex/cookbook/output/{session_id}", - COOKBOOK_READ_SCOPES, - ), - _capability("GET", "/api/codex/cookbook/cached", COOKBOOK_READ_SCOPES), - _capability("GET", "/api/codex/cookbook/presets", COOKBOOK_READ_SCOPES), - _capability("POST", "/api/codex/cookbook/serve", COOKBOOK_LAUNCH_SCOPES), - _capability( - "POST", - "/api/codex/cookbook/stop/{session_id}", - COOKBOOK_LAUNCH_SCOPES, - ), - _capability( - "POST", - "/api/codex/cookbook/preset/{name}", - COOKBOOK_LAUNCH_SCOPES, - ), - _capability("POST", "/api/codex/cookbook/adopt", COOKBOOK_LAUNCH_SCOPES), ) diff --git a/tests/test_api_token_capabilities.py b/tests/test_api_token_capabilities.py index 3f25f9e7f..5fb116288 100644 --- a/tests/test_api_token_capabilities.py +++ b/tests/test_api_token_capabilities.py @@ -64,30 +64,6 @@ def test_companion_bearer_reads_all_require_chat_scope(): "documents:write", "documents:read", ), - ("GET", "/api/codex/cookbook/tasks", "cookbook:read", "chat"), - ("GET", "/api/codex/cookbook/servers", "cookbook:launch", "chat"), - ( - "GET", - "/api/codex/cookbook/output/serve-1", - "cookbook:read", - "chat", - ), - ("GET", "/api/codex/cookbook/cached", "cookbook:read", "chat"), - ("GET", "/api/codex/cookbook/presets", "cookbook:read", "chat"), - ("POST", "/api/codex/cookbook/serve", "cookbook:launch", "chat"), - ( - "POST", - "/api/codex/cookbook/stop/serve-1", - "cookbook:launch", - "cookbook:read", - ), - ( - "POST", - "/api/codex/cookbook/preset/default", - "cookbook:launch", - "cookbook:read", - ), - ("POST", "/api/codex/cookbook/adopt", "cookbook:launch", "chat"), ], ) def test_codex_route_families_require_their_existing_scopes( @@ -152,6 +128,31 @@ def test_privileged_and_owner_attributing_ui_routes_remain_blocked(method, path) assert decision.error == API_TOKEN_FORBIDDEN_ERROR +@pytest.mark.parametrize( + ("method", "path"), + [ + ("GET", "/api/codex/cookbook/tasks"), + ("GET", "/api/codex/cookbook/servers"), + ("GET", "/api/codex/cookbook/output/serve-1"), + ("GET", "/api/codex/cookbook/cached"), + ("GET", "/api/codex/cookbook/presets"), + ("POST", "/api/codex/cookbook/serve"), + ("POST", "/api/codex/cookbook/stop/serve-1"), + ("POST", "/api/codex/cookbook/preset/default"), + ("POST", "/api/codex/cookbook/adopt"), + ], +) +def test_legacy_cookbook_scopes_are_inert_at_the_bearer_boundary(method, path): + decision = authorize_api_token_route( + method, + path, + ["cookbook:read", "cookbook:launch"], + ) + + assert decision.allowed is False + assert decision.error == API_TOKEN_FORBIDDEN_ERROR + + def test_method_matching_is_exact_and_case_insensitive(): assert _allowed("post", "/api/v1/chat", ["chat"]) is True assert _allowed("GET", "/api/v1/chat", ["chat"]) is False @@ -218,6 +219,31 @@ def test_asgi_root_path_is_removed_before_matching(): assert wrong_prefix.allowed is False +@pytest.mark.parametrize( + ("root_path", "path", "raw_path"), + [ + ("/odysseus/", "/odysseus//api/models", b"/odysseus//api/models"), + ("/", "//api/models", b"//api/models"), + ], +) +def test_asgi_root_path_with_trailing_slash_is_removed_before_matching( + root_path, + path, + raw_path, +): + decision = authorize_api_token_request( + "GET", + { + "root_path": root_path, + "path": path, + "raw_path": raw_path, + }, + ["chat"], + ) + + assert decision.allowed is True + + @pytest.mark.parametrize("encoded", [b"%2f", b"%2F", b"%5c", b"%5C", b"%00"]) def test_encoded_path_delimiters_fail_closed(encoded): decision = authorize_api_token_request( @@ -232,6 +258,26 @@ def test_encoded_path_delimiters_fail_closed(encoded): assert decision.allowed is False +@pytest.mark.parametrize( + ("decoded", "encoded"), + [("?", b"%3F"), ("#", b"%23")], +) +def test_encoded_calendar_uid_characters_follow_the_decoded_router_path( + decoded, + encoded, +): + decision = authorize_api_token_request( + "DELETE", + { + "path": f"/api/codex/calendar/events/team{decoded}primary", + "raw_path": b"/api/codex/calendar/events/team" + encoded + b"primary", + }, + ["calendar:write"], + ) + + assert decision.allowed is True + + def test_encoded_static_letters_follow_the_decoded_router_path(): decision = authorize_api_token_request( "GET", @@ -302,7 +348,24 @@ def test_manifest_matches_runtime_scoped_router_inventory(): ("POST", "/api/v1/chat"), ("GET", "/api/models"), } - expected.update(_router_inventory(setup_codex_routes())) + codex_inventory = _router_inventory(setup_codex_routes()) + cookbook_inventory = { + route + for route in codex_inventory + if route[1].startswith("/api/codex/cookbook/") + } + assert cookbook_inventory == { + ("GET", "/api/codex/cookbook/tasks"), + ("GET", "/api/codex/cookbook/servers"), + ("GET", "/api/codex/cookbook/output/{session_id}"), + ("GET", "/api/codex/cookbook/cached"), + ("GET", "/api/codex/cookbook/presets"), + ("POST", "/api/codex/cookbook/serve"), + ("POST", "/api/codex/cookbook/stop/{session_id}"), + ("POST", "/api/codex/cookbook/preset/{name}"), + ("POST", "/api/codex/cookbook/adopt"), + } + expected.update(codex_inventory - cookbook_inventory) expected.update(_router_inventory(setup_claude_routes())) expected.update(companion_inventory - companion_pairing) actual = { @@ -328,8 +391,6 @@ def test_accepted_scope_catalog_is_explicit_and_has_no_admin_scope(): "calendar:write", "memory:read", "memory:write", - "cookbook:read", - "cookbook:launch", } @@ -340,4 +401,5 @@ def test_token_minting_and_route_checks_share_the_scope_catalog(): assert ALLOWED_SCOPES is ALL_API_TOKEN_SCOPES assert codex_routes.TODO_READ_SCOPES <= ALL_API_TOKEN_SCOPES assert codex_routes.EMAIL_READ_SCOPES <= ALL_API_TOKEN_SCOPES - assert codex_routes.COOKBOOK_READ_SCOPES <= ALL_API_TOKEN_SCOPES + assert codex_routes.COOKBOOK_READ_SCOPES.isdisjoint(ALL_API_TOKEN_SCOPES) + assert codex_routes.COOKBOOK_LAUNCH_SCOPES.isdisjoint(ALL_API_TOKEN_SCOPES) diff --git a/tests/test_api_token_middleware_integration.py b/tests/test_api_token_middleware_integration.py index a2e026695..019ad098f 100644 --- a/tests/test_api_token_middleware_integration.py +++ b/tests/test_api_token_middleware_integration.py @@ -61,18 +61,28 @@ def test_production_auth_middleware_enforces_api_token_capabilities(tmp_path): app_module._bcrypt.checkpw = lambda *_args: True - def _request(method, path, *, loopback): + def _request( + method, + path, + *, + loopback, + authorization=None, + raw_path=None, + ): return Request({ "type": "http", "http_version": "1.1", "method": method, "scheme": "http", "path": path, - "raw_path": path.encode(), + "raw_path": raw_path or path.encode(), "root_path": "", "query_string": b"", "headers": [ - (b"authorization", f"Bearer {RAW_TOKEN}".encode()), + ( + b"authorization", + (authorization or f"Bearer {RAW_TOKEN}").encode(), + ), ], "client": ( "127.0.0.1" if loopback else "192.0.2.10", @@ -83,13 +93,28 @@ def test_production_auth_middleware_enforces_api_token_capabilities(tmp_path): }) - async def _case(path, scopes, *, loopback=False, localhost_bypass=False): + async def _case( + path, + scopes, + *, + loopback=False, + localhost_bypass=False, + authorization=None, + method="GET", + raw_path=None, + ): app_module.LOCALHOST_BYPASS = localhost_bypass app_module._token_cache.clear() app_module._token_cache[RAW_TOKEN[:8]] = [ ("token-1", "unused-hash", "alice", scopes), ] - request = _request("GET", path, loopback=loopback) + request = _request( + method, + path, + loopback=loopback, + authorization=authorization, + raw_path=raw_path, + ) calls = [] async def call_next(received): @@ -113,18 +138,73 @@ def test_production_auth_middleware_enforces_api_token_capabilities(tmp_path): try: forbidden = await _case("/api/unregistered", ["chat"]) allowed = await _case("/api/models", ["chat"]) - local_wrong_scope = await _case( - "/api/models", - ["todos:read"], - loopback=True, - localhost_bypass=True, - ) + encoded_calendar_uids = { + "question_mark": await _case( + "/api/codex/calendar/events/team?primary", + ["calendar:write"], + method="DELETE", + raw_path=( + b"/api/codex/calendar/events/team%3Fprimary" + ), + ), + "hash": await _case( + "/api/codex/calendar/events/team#primary", + ["calendar:write"], + method="DELETE", + raw_path=( + b"/api/codex/calendar/events/team%23primary" + ), + ), + } + cookbook_forbidden = {} + for label, (method, path) in { + "tasks": ("GET", "/api/codex/cookbook/tasks"), + "servers": ("GET", "/api/codex/cookbook/servers"), + "output": ( + "GET", + "/api/codex/cookbook/output/serve-1", + ), + "cached": ("GET", "/api/codex/cookbook/cached"), + "presets": ("GET", "/api/codex/cookbook/presets"), + "serve": ("POST", "/api/codex/cookbook/serve"), + "stop": ( + "POST", + "/api/codex/cookbook/stop/serve-1", + ), + "preset": ( + "POST", + "/api/codex/cookbook/preset/default", + ), + "adopt": ("POST", "/api/codex/cookbook/adopt"), + }.items(): + cookbook_forbidden[label] = await _case( + path, + ["cookbook:read", "cookbook:launch"], + method=method, + ) + local_wrong_scope = {} + for label, authorization in { + "canonical": f"Bearer {RAW_TOKEN}", + "lowercase": f"bearer {RAW_TOKEN}", + "uppercase": f"BEARER {RAW_TOKEN}", + "mixed_case": f"bEaReR {RAW_TOKEN}", + "multiple_spaces": f"Bearer {RAW_TOKEN}", + }.items(): + local_wrong_scope[label] = await _case( + "/api/models", + ["todos:read"], + loopback=True, + localhost_bypass=True, + authorization=authorization, + ) finally: app_module._asyncio.create_task = original_create_task print("RESULT=" + json.dumps({ "forbidden": forbidden, "allowed": allowed, + "cookbook_forbidden": cookbook_forbidden, + "encoded_calendar_uids": encoded_calendar_uids, "local_wrong_scope": local_wrong_scope, }, sort_keys=True)) @@ -165,10 +245,35 @@ def test_production_auth_middleware_enforces_api_token_capabilities(tmp_path): "api_token": True, "owner": "alice", } - assert observed["local_wrong_scope"] == { + assert observed["encoded_calendar_uids"] == { + "question_mark": observed["allowed"], + "hash": observed["allowed"], + } + expected_forbidden = { "status": 403, "body": generic_error, "called": 0, "api_token": None, "owner": None, } + assert observed["cookbook_forbidden"] == { + label: expected_forbidden + for label in ( + "tasks", + "servers", + "output", + "cached", + "presets", + "serve", + "stop", + "preset", + "adopt", + ) + } + assert observed["local_wrong_scope"] == { + "canonical": expected_forbidden, + "lowercase": expected_forbidden, + "uppercase": expected_forbidden, + "mixed_case": expected_forbidden, + "multiple_spaces": expected_forbidden, + } diff --git a/tests/test_api_token_routes.py b/tests/test_api_token_routes.py index 40afc2226..1f8d94398 100644 --- a/tests/test_api_token_routes.py +++ b/tests/test_api_token_routes.py @@ -192,7 +192,12 @@ def test_create_token_attributes_owner_hashes_secret_and_returns_raw_once(monkey invalidator.assert_called_once() -def test_create_token_accepts_cookbook_read_scope(monkeypatch, token_routes_mod): +@pytest.mark.parametrize("scope", ["cookbook:read", "cookbook:launch"]) +def test_create_token_rejects_retired_cookbook_scopes( + monkeypatch, + token_routes_mod, + scope, +): monkeypatch.setenv("AUTH_ENABLED", "true") mod = token_routes_mod @@ -202,24 +207,12 @@ def test_create_token_accepts_cookbook_read_scope(monkeypatch, token_routes_mod) req = _req("alice", is_admin=True) create_token = _get_handler(mod, "POST", "/tokens") - resp = create_token(request=req, name="cookbook-reader", scopes="cookbook:read") + with pytest.raises(HTTPException) as exc: + create_token(request=req, name="retired-cookbook", scopes=scope) - assert resp["scopes"] == ["cookbook:read"] - - -def test_cookbook_launch_scope_implies_read(monkeypatch, token_routes_mod): - monkeypatch.setenv("AUTH_ENABLED", "true") - mod = token_routes_mod - - fake_session = MagicMock() - monkeypatch.setattr(mod, "get_db_session", lambda: _db_ctx(fake_session)) - monkeypatch.setattr(mod, "get_current_user", lambda req: req.state.current_user) - - req = _req("alice", is_admin=True) - create_token = _get_handler(mod, "POST", "/tokens") - resp = create_token(request=req, name="cookbook-launcher", scopes="cookbook:launch") - - assert resp["scopes"] == ["cookbook:read", "cookbook:launch"] + assert exc.value.status_code == 400 + assert exc.value.detail == f"Unknown token scope: {scope}" + fake_session.add.assert_not_called() # ---------------------------------------------------------------------------