Skip to content

Commit 00aa25b

Browse files
authored
fix(auth): enforce API-key scopes on v2 routes (#717) (#788)
* fix(auth): enforce API-key scopes on v2 routes (#717) require_scope existed but was wired to zero routes: every v2 router mounted a blanket require_auth, so a read-scoped key could POST/PUT/DELETE, store machine-wide credentials, and merge PRs. A leaked read-only CI token was an admin token. - add require_method_scope: safe methods (GET/HEAD/OPTIONS) need read, mutating methods need write; swap it in for the blanket _AUTH dependency so all 22 routers are covered in one place - require admin on credential storage/deletion (settings /keys PUT+DELETE) and PR merge (pr /{n}/merge) - JWT principals and the auth-disabled synthetic principal keep all scopes, so the web UI, local dev, and existing tests are unaffected; only scoped API keys are constrained Closes #717 * test(auth): admin-merge positive + PATCH coverage; comment require_scope chain (#717 review)
1 parent 5ba826e commit 00aa25b

5 files changed

Lines changed: 196 additions & 11 deletions

File tree

codeframe/auth/dependencies.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,31 @@ async def require_auth(
327327
)
328328

329329

330+
# HTTP methods that only read state — everything else mutates (#717).
331+
_SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"})
332+
333+
334+
async def require_method_scope(
335+
request: Request,
336+
auth: Dict[str, Any] = Depends(require_auth),
337+
) -> Dict[str, Any]:
338+
"""Router-level guard that enforces scope by HTTP method (issue #717).
339+
340+
Safe methods (GET/HEAD/OPTIONS) require the ``read`` scope; mutating
341+
methods (POST/PUT/PATCH/DELETE) require ``write``. Admin-only routes layer
342+
their own ``Depends(require_scope("admin"))`` on top. JWT principals and the
343+
auth-disabled synthetic principal both carry all scopes, so this only
344+
constrains scoped API keys — a read-only key can no longer mutate state.
345+
"""
346+
required = SCOPE_READ if request.method.upper() in _SAFE_METHODS else SCOPE_WRITE
347+
if not has_scope(auth, required):
348+
raise HTTPException(
349+
status_code=status.HTTP_403_FORBIDDEN,
350+
detail=f"Insufficient permissions: '{required}' scope required",
351+
)
352+
return auth
353+
354+
330355
async def authenticate_websocket(
331356
websocket: WebSocket,
332357
*,
@@ -429,6 +454,10 @@ async def create_resource(auth: dict = Depends(require_scope("write"))):
429454
Returns:
430455
Dependency function that validates scope
431456
"""
457+
# Depends on require_auth directly (not require_method_scope): on admin
458+
# routes both run, but FastAPI caches require_auth within a request, so
459+
# there is exactly one authentication call — the method guard checks
460+
# write and this checks admin against the same principal (#717).
432461
async def check_scope(auth: Dict[str, Any] = Depends(require_auth)) -> Dict[str, Any]:
433462
"""Verify principal has required scope."""
434463
if not has_scope(auth, required_scope):

codeframe/ui/routers/pr_v2.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
from fastapi import APIRouter, Depends, HTTPException, Query, Request
1919
from pydantic import BaseModel, Field
2020

21+
from codeframe.auth.api_keys import SCOPE_ADMIN
22+
from codeframe.auth.dependencies import require_scope
2123
from codeframe.core.workspace import Workspace
2224
from codeframe.lib.rate_limiter import rate_limit_standard
2325
from codeframe.git.github_integration import GitHubIntegration, GitHubAPIError, PRDetails
@@ -599,7 +601,11 @@ async def create_pull_request(
599601
await client.close()
600602

601603

602-
@router.post("/{pr_number}/merge", response_model=MergeResponse)
604+
@router.post(
605+
"/{pr_number}/merge",
606+
response_model=MergeResponse,
607+
dependencies=[Depends(require_scope(SCOPE_ADMIN))], # merging to the repo is admin-only (#717)
608+
)
603609
@rate_limit_standard()
604610
async def merge_pull_request(
605611
request: Request,

codeframe/ui/routers/settings_v2.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
from anthropic import AuthenticationError as _AnthropicAuthError
2828
from fastapi import APIRouter, Depends, HTTPException, Request, Response
2929
from fastapi.concurrency import run_in_threadpool
30+
31+
from codeframe.auth.api_keys import SCOPE_ADMIN
32+
from codeframe.auth.dependencies import require_scope
3033
from openai import AuthenticationError as _OpenAIAuthError
3134
from openai import OpenAI as _OpenAIClient
3235
from pydantic import BaseModel, Field
@@ -235,7 +238,11 @@ async def list_key_status(
235238
return [_build_status(p, manager) for p in KEY_PROVIDERS]
236239

237240

238-
@router.put("/keys/{provider}", response_model=KeyStatusResponse)
241+
@router.put(
242+
"/keys/{provider}",
243+
response_model=KeyStatusResponse,
244+
dependencies=[Depends(require_scope(SCOPE_ADMIN))], # credential storage is admin-only (#717)
245+
)
239246
@rate_limit_standard()
240247
async def store_key(
241248
provider: str,
@@ -268,7 +275,11 @@ async def store_key(
268275
return _build_status(cast(KeyProvider, provider), manager)
269276

270277

271-
@router.delete("/keys/{provider}", status_code=204)
278+
@router.delete(
279+
"/keys/{provider}",
280+
status_code=204,
281+
dependencies=[Depends(require_scope(SCOPE_ADMIN))], # credential deletion is admin-only (#717)
282+
)
272283
@rate_limit_standard()
273284
async def delete_key(
274285
provider: str,

codeframe/ui/server.py

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
workspace_v2,
4646
)
4747
from codeframe.auth import router as auth_router
48-
from codeframe.auth.dependencies import require_auth
48+
from codeframe.auth.dependencies import require_auth, require_method_scope
4949
from codeframe.platform_store.database import Database
5050
from codeframe.lib.rate_limiter import (
5151
get_rate_limiter,
@@ -742,13 +742,15 @@ async def test_broadcast(message: dict, project_id: int = None):
742742
# v2 API routers - all delegate to codeframe.core modules
743743
#
744744
# Auth enforcement (issue #336): every v2 REST router is mounted with a
745-
# blanket ``require_auth`` dependency. With CODEFRAME_AUTH_REQUIRED disabled
746-
# (local opt-out) require_auth returns a synthetic principal instead of
747-
# raising, so behavior is unchanged for local/dev use. The two WebSocket
748-
# routers (session_chat_ws, terminal_ws) are intentionally excluded — they
749-
# perform their own ?token= JWT auth and cannot use an HTTP 401 dependency.
750-
# The auth_router (login/register) stays public.
751-
_AUTH = [Depends(require_auth)]
745+
# blanket scope-aware auth dependency. ``require_method_scope`` first resolves
746+
# ``require_auth`` (honoring CODEFRAME_AUTH_REQUIRED — disabled returns a
747+
# synthetic all-scopes principal, so local/dev/tests are unchanged), then
748+
# enforces scope by HTTP method (#717): safe methods need ``read``, mutating
749+
# methods need ``write``. Admin-only routes (credential storage, PR merge) add
750+
# their own ``Depends(require_scope("admin"))``. The two WebSocket routers
751+
# (session_chat_ws, terminal_ws) are intentionally excluded — they perform
752+
# their own ?token= JWT auth. The auth_router (login/register) stays public.
753+
_AUTH = [Depends(require_method_scope)]
752754
app.include_router(batches_v2.router, dependencies=_AUTH) # /api/v2/batches
753755
app.include_router(blockers_v2.router, dependencies=_AUTH) # /api/v2/blockers
754756
app.include_router(checkpoints_v2.router, dependencies=_AUTH) # /api/v2/checkpoints
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
"""API-key scope enforcement across the v2 API (issue #717 / P0.6).
2+
3+
Before this, every v2 router mounted a blanket ``require_auth`` that proved
4+
*authentication* only — a read-scoped key could POST/PUT/DELETE and even store
5+
credentials or merge PRs. Now:
6+
7+
- safe methods (GET) need ``read``
8+
- mutating methods (POST/PUT/PATCH/DELETE) need ``write``
9+
- credential storage/deletion and PR-merge need ``admin``
10+
11+
JWT principals and the auth-disabled synthetic principal carry all scopes, so
12+
this only constrains scoped API keys.
13+
"""
14+
15+
import importlib
16+
17+
import pytest
18+
from fastapi.testclient import TestClient
19+
20+
from codeframe.auth.api_keys import SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN
21+
from codeframe.auth.manager import reset_auth_engine
22+
from codeframe.core.api_key_service import ApiKeyService
23+
from codeframe.platform_store.database import Database
24+
25+
pytestmark = pytest.mark.v2
26+
27+
28+
@pytest.fixture
29+
def scoped_app(tmp_path, monkeypatch):
30+
"""Real server app with auth ON and three API keys (read/write/admin)."""
31+
db_path = tmp_path / "state.db"
32+
monkeypatch.setenv("DATABASE_PATH", str(db_path))
33+
monkeypatch.setenv("CODEFRAME_AUTH_REQUIRED", "true")
34+
reset_auth_engine()
35+
36+
db = Database(db_path)
37+
db.initialize()
38+
db.conn.execute(
39+
"""
40+
INSERT OR REPLACE INTO users (
41+
id, email, name, hashed_password,
42+
is_active, is_superuser, is_verified, email_verified
43+
) VALUES (1, 'test@example.com', 'Test', '!DISABLED!', 1, 0, 1, 1)
44+
"""
45+
)
46+
db.conn.commit()
47+
48+
svc = ApiKeyService(db)
49+
keys = {
50+
"read": svc.create_api_key(user_id=1, name="r", scopes=[SCOPE_READ]).key,
51+
"write": svc.create_api_key(user_id=1, name="w", scopes=[SCOPE_READ, SCOPE_WRITE]).key,
52+
"admin": svc.create_api_key(user_id=1, name="a", scopes=[SCOPE_ADMIN]).key,
53+
}
54+
db.close()
55+
56+
from codeframe.ui import server
57+
58+
importlib.reload(server)
59+
yield server.app, keys
60+
reset_auth_engine()
61+
62+
63+
def _hdr(key: str) -> dict:
64+
return {"X-API-Key": key}
65+
66+
67+
class TestMethodScope:
68+
def test_read_key_allowed_on_get(self, scoped_app):
69+
app, keys = scoped_app
70+
r = TestClient(app).get("/api/v2/settings", headers=_hdr(keys["read"]))
71+
assert r.status_code != 403 # read scope satisfies a GET
72+
assert r.status_code != 401
73+
74+
def test_read_key_forbidden_on_write(self, scoped_app):
75+
app, keys = scoped_app
76+
# PUT /api/v2/settings mutates → needs write; read key must 403 before
77+
# any body validation (router-level dependency fires first).
78+
r = TestClient(app).put("/api/v2/settings", headers=_hdr(keys["read"]), json={})
79+
assert r.status_code == 403
80+
81+
def test_write_key_allowed_on_write(self, scoped_app):
82+
app, keys = scoped_app
83+
r = TestClient(app).put("/api/v2/settings", headers=_hdr(keys["write"]), json={})
84+
assert r.status_code != 403 # write scope passes the method guard
85+
assert r.status_code != 401
86+
87+
88+
class TestAdminScope:
89+
def test_read_key_forbidden_on_credential_storage(self, scoped_app):
90+
app, keys = scoped_app
91+
r = TestClient(app).put(
92+
"/api/v2/settings/keys/openai", headers=_hdr(keys["read"]), json={"value": "sk-x"}
93+
)
94+
assert r.status_code == 403
95+
96+
def test_write_key_forbidden_on_credential_storage(self, scoped_app):
97+
app, keys = scoped_app
98+
# write is not enough — credential storage is admin-only.
99+
r = TestClient(app).put(
100+
"/api/v2/settings/keys/openai", headers=_hdr(keys["write"]), json={"value": "sk-x"}
101+
)
102+
assert r.status_code == 403
103+
104+
def test_admin_key_allowed_on_credential_storage(self, scoped_app):
105+
app, keys = scoped_app
106+
r = TestClient(app).put(
107+
"/api/v2/settings/keys/openai", headers=_hdr(keys["admin"]), json={"value": "sk-x"}
108+
)
109+
# admin passes the scope gate; may 400 on value format, but never 403.
110+
assert r.status_code != 403
111+
assert r.status_code != 401
112+
113+
def test_write_key_forbidden_on_pr_merge(self, scoped_app):
114+
app, keys = scoped_app
115+
r = TestClient(app).post(
116+
"/api/v2/pr/1/merge", headers=_hdr(keys["write"]), json={}
117+
)
118+
assert r.status_code == 403
119+
120+
def test_admin_key_allowed_on_pr_merge(self, scoped_app):
121+
app, keys = scoped_app
122+
r = TestClient(app).post(
123+
"/api/v2/pr/1/merge", headers=_hdr(keys["admin"]), json={}
124+
)
125+
# admin passes the scope gate; may 400/404/422 on body/logic, never 403.
126+
assert r.status_code != 403
127+
assert r.status_code != 401
128+
129+
130+
class TestPatchIsMutating:
131+
def test_read_key_forbidden_on_patch(self, scoped_app):
132+
app, keys = scoped_app
133+
# PATCH is not a safe method → needs write; a read key must 403.
134+
r = TestClient(app).patch(
135+
"/api/v2/tasks/abc", headers=_hdr(keys["read"]), json={}
136+
)
137+
assert r.status_code == 403

0 commit comments

Comments
 (0)