Merge pull request 'Make consequential pull reviews passkey-native and auditable' (#510) from timmy/509-passkey-review-activity into main

frontend/session.js

@@ -149,6 +149,8 @@
             all_sessions_revoked: 'All device access revoked',
             issue_closed: 'Issue closed',
             pull_merged: 'Pull request merged',
+            pull_review_approved: 'Pull request approved',
+            pull_review_changes_requested: 'Changes requested',
           };
           page.events.forEach(event => {
             const row = root.document.createElement('article');

src/main.py

@@ -200,9 +200,7 @@ class DashboardSignIn(BaseModel):
         return normalized
 
 
-class FreshAuthorization(BaseModel):
+StepUpAction = Literal[
-    access_token: str = Field(min_length=1, max_length=1_024)
-    action: Literal[
     "merge_pull",
     "submit_pull_review",
     "close_issue",
@@ -211,6 +209,11 @@ class FreshAuthorization(BaseModel):
     "enroll_passkey",
     "revoke_passkey",
 ]
+
+
+class FreshAuthorization(BaseModel):
+    access_token: str = Field(min_length=1, max_length=1_024)
+    action: StepUpAction
     target: str = Field(min_length=1, max_length=255)
 
 
@@ -228,9 +231,7 @@ class PasskeyAuthentication(PasskeyCeremony):
 
 
 class PasskeyAuthorizationTarget(BaseModel):
-    action: Literal[
+    action: StepUpAction
-        "merge_pull", "close_issue", "revoke_device", "revoke_all_sessions", "revoke_passkey"
-    ]
     target: str = Field(min_length=1, max_length=255)
 
 
@@ -3825,12 +3826,50 @@ async def submit_review(
             submission.decision,
             submission.body,
         )
+        journal = None
+        operation_id = None
+        if submission.decision != "comment":
+            journal = _security_event_store()
+            try:
+                operation_id = await asyncio.to_thread(
+                    journal.reserve,
+                    (
+                        "pull_review_approved"
+                        if submission.decision == "approve"
+                        else "pull_review_changes_requested"
+                    ),
+                    target=f"{repository}#{number}",
+                )
+            except SecurityEventStoreError:
+                raise HTTPException(
+                    status_code=503,
+                    detail="Security activity is temporarily unavailable",
+                )
+        try:
             if submission.comments:
-            return await gitea_proxy.submit_pull_review(
+                result = await gitea_proxy.submit_pull_review(
                     *args,
                     [comment.model_dump() for comment in submission.comments],
                 )
-        return await gitea_proxy.submit_pull_review(*args)
+            else:
+                result = await gitea_proxy.submit_pull_review(*args)
+        except (
+            HTTPException,
+            gitea_proxy.StaleReviewError,
+            gitea_proxy.InvalidReviewCommentError,
+        ):
+            if journal is not None and operation_id is not None:
+                try:
+                    await asyncio.to_thread(journal.discard, operation_id)
+                except SecurityEventStoreError:
+                    pass
+            raise
+        if journal is not None and operation_id is not None:
+            try:
+                await asyncio.to_thread(journal.finalize, operation_id)
+            except SecurityEventStoreError:
+                pass
+        return result
 
     try:
         comment_fingerprint = tuple(

tests/test_dashboard_auth.py

@@ -201,6 +201,14 @@ async def test_passkey_fresh_authorization_is_exact_target_bound_and_single_use(
             json={"action": "close_issue", "target": "stackchain/api#7"},
             headers=headers,
         )
+        review_options = await client.post(
+            "/api/v1/passkeys/authorization/options",
+            json={
+                "action": "submit_pull_review",
+                "target": "stackchain/api#7@abc123:approve",
+            },
+            headers=headers,
+        )
         ceremony = {
             "challenge": options.json()["challenge"],
             "credential": {"id": "cGhvbmUtY3JlZGVudGlhbA"},
@@ -220,6 +228,8 @@ async def test_passkey_fresh_authorization_is_exact_target_bound_and_single_use(
         )
 
     assert options.status_code == 200
+    assert review_options.status_code == 200
+    assert review_options.json()["challenge"]
     assert authorized.status_code == 201
     assert authorized.json()["grant"]
     assert authorized.json()["expires_in"] == 90

tests/test_dashboard_session_frontend.py

@@ -655,6 +655,13 @@ def test_security_activity_explains_pending_outcome_confirmation():
     assert "event.status === 'pending'" in source
 
 
+def test_security_activity_labels_consequential_pull_review_decisions():
+    source = SESSION_JS.read_text()
+
+    assert "pull_review_approved: 'Pull request approved'" in source
+    assert "pull_review_changes_requested: 'Changes requested'" in source
+
+
 def test_authenticated_dashboard_load_requests_queued_delivery_resume():
     result = run_session_scenario(
         """

tests/test_security_activity.py

@@ -234,6 +234,117 @@ async def test_successful_protected_actions_record_only_bounded_targets(
     assert b"abc123" not in persisted
 
 
+@pytest.mark.anyio
+async def test_approved_pull_review_records_a_privacy_safe_completed_event(
+    security_access, monkeypatch
+):
+    async def requested(*_args):
+        return True
+
+    async def submit(*_args):
+        return {"id": 91, "state": "APPROVED"}
+
+    monkeypatch.setattr(main, "is_requested_review", requested)
+    monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="https://test") as client:
+        await client.post(
+            "/api/v1/session",
+            json={"access_token": "correct horse battery staple", "device_label": "Phone"},
+        )
+        headers = {
+            "Origin": "https://test",
+            "X-CSRF-Token": client.cookies["stackchain_csrf"],
+        }
+        grant = await client.post(
+            "/api/v1/fresh-authorization",
+            json={
+                "access_token": "correct horse battery staple",
+                "action": "submit_pull_review",
+                "target": "stackchain/api#7@abc123:approve",
+            },
+            headers=headers,
+        )
+        response = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/review",
+            json={
+                "expected_head_sha": "abc123",
+                "decision": "approve",
+                "body": "Ready to ship with private review notes.",
+            },
+            headers={**headers, "X-Step-Up-Grant": grant.json()["grant"]},
+        )
+        events = (await client.get("/api/v1/security-events")).json()["events"]
+
+    assert response.status_code == 201
+    assert events[0] == {
+        "id": events[0]["id"],
+        "kind": "pull_review_approved",
+        "method": None,
+        "device_label": None,
+        "target": "stackchain/api#7",
+        "created_at": events[0]["created_at"],
+        "status": "completed",
+    }
+    persisted = (security_access / "security.sqlite3").read_bytes()
+    assert b"abc123" not in persisted
+    assert b"private review notes" not in persisted
+
+
+@pytest.mark.anyio
+async def test_review_reservation_failure_prevents_the_gitea_mutation(
+    security_access, monkeypatch
+):
+    submit_calls = []
+
+    async def requested(*_args):
+        return True
+
+    async def submit(*args):
+        submit_calls.append(args)
+        return {"id": 91, "state": "APPROVED"}
+
+    class UnavailableJournal:
+        def reserve(self, *_args, **_kwargs):
+            raise SecurityEventStoreError("unavailable")
+
+    monkeypatch.setattr(main, "is_requested_review", requested)
+    monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="https://test") as client:
+        await client.post(
+            "/api/v1/session",
+            json={"access_token": "correct horse battery staple"},
+        )
+        headers = {
+            "Origin": "https://test",
+            "X-CSRF-Token": client.cookies["stackchain_csrf"],
+        }
+        grant = await client.post(
+            "/api/v1/fresh-authorization",
+            json={
+                "access_token": "correct horse battery staple",
+                "action": "submit_pull_review",
+                "target": "stackchain/api#7@abc123:approve",
+            },
+            headers=headers,
+        )
+        monkeypatch.setattr(main, "_security_event_store", lambda: UnavailableJournal())
+        response = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/review",
+            json={
+                "expected_head_sha": "abc123",
+                "decision": "approve",
+                "body": "Ready.",
+            },
+            headers={**headers, "X-Step-Up-Grant": grant.json()["grant"]},
+        )
+
+    assert response.status_code == 503
+    assert response.json()["detail"] == "Security activity is temporarily unavailable"
+    assert submit_calls == []
+
+
 @pytest.mark.anyio
 async def test_issue_close_reservation_failure_prevents_the_gitea_mutation(
     security_access, monkeypatch