From 877dc475ddf499ddb5e7ebd0105adf9478ebc5c7 Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 10 Aug 2026 19:56:12 +0000 Subject: [PATCH] feat: make consequential reviews passkey-native (Closes #509) --- frontend/session.js | 2 + src/main.py | 75 +++++++++++---- tests/test_dashboard_auth.py | 10 ++ tests/test_dashboard_session_frontend.py | 7 ++ tests/test_security_activity.py | 111 +++++++++++++++++++++++ 5 files changed, 187 insertions(+), 18 deletions(-) diff --git a/frontend/session.js b/frontend/session.js index 4402f3e..df1d638 100644 --- a/frontend/session.js +++ b/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'); diff --git a/src/main.py b/src/main.py index 986abf2..1e936ae 100644 --- a/src/main.py +++ b/src/main.py @@ -200,17 +200,20 @@ class DashboardSignIn(BaseModel): return normalized +StepUpAction = Literal[ + "merge_pull", + "submit_pull_review", + "close_issue", + "revoke_device", + "revoke_all_sessions", + "enroll_passkey", + "revoke_passkey", +] + + class FreshAuthorization(BaseModel): access_token: str = Field(min_length=1, max_length=1_024) - action: Literal[ - "merge_pull", - "submit_pull_review", - "close_issue", - "revoke_device", - "revoke_all_sessions", - "enroll_passkey", - "revoke_passkey", - ] + action: StepUpAction target: str = Field(min_length=1, max_length=255) @@ -228,9 +231,7 @@ class PasskeyAuthentication(PasskeyCeremony): class PasskeyAuthorizationTarget(BaseModel): - action: Literal[ - "merge_pull", "close_issue", "revoke_device", "revoke_all_sessions", "revoke_passkey" - ] + action: StepUpAction target: str = Field(min_length=1, max_length=255) @@ -3825,12 +3826,50 @@ async def submit_review( submission.decision, submission.body, ) - if submission.comments: - return await gitea_proxy.submit_pull_review( - *args, - [comment.model_dump() for comment in submission.comments], - ) - return await gitea_proxy.submit_pull_review(*args) + 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: + result = await gitea_proxy.submit_pull_review( + *args, + [comment.model_dump() for comment in submission.comments], + ) + 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( diff --git a/tests/test_dashboard_auth.py b/tests/test_dashboard_auth.py index b9f37c9..cd88af3 100644 --- a/tests/test_dashboard_auth.py +++ b/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 diff --git a/tests/test_dashboard_session_frontend.py b/tests/test_dashboard_session_frontend.py index 7d55124..832c411 100644 --- a/tests/test_dashboard_session_frontend.py +++ b/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( """ diff --git a/tests/test_security_activity.py b/tests/test_security_activity.py index 597c31f..399ddad 100644 --- a/tests/test_security_activity.py +++ b/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 -- 2.43.0