mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-08-05 02:45:28 +00:00
fix(auth): normalize trailing ASGI root paths
This commit is contained in:
parent
eccff78e4d
commit
5a1a5332f9
6 changed files with 244 additions and 92 deletions
14
app.py
14
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"})
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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,11 +138,64 @@ 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(
|
||||
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
|
||||
|
|
@ -125,6 +203,8 @@ def test_production_auth_middleware_enforces_api_token_capabilities(tmp_path):
|
|||
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,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue