From 3896f17a58434001be056a5ea638bdba7407d71b Mon Sep 17 00:00:00 2001 From: timmy Date: Thu, 13 Aug 2026 22:55:47 +0000 Subject: [PATCH] feat: authorize and audit comment deletion (Closes #779) --- README.md | 5 +- frontend/security-center.js | 1 + src/main.py | 68 +++++++- tests/test_dashboard_session_frontend.py | 6 + tests/test_security_activity.py | 211 +++++++++++++++++++++++ 5 files changed, 285 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 581e635..50596b1 100644 --- a/README.md +++ b/README.md @@ -289,8 +289,9 @@ remain compatible. The bounded **Passkey counter anomaly** Security activity ent only the device label and attempted action; repeated alerts should prompt removal and re-enrollment of that passkey. -High-impact actions—merging a pull request, closing an assigned issue, revoking a -remote device, or signing out every device—require a passkey assertion or the +High-impact actions—merging a pull request, closing an assigned issue, permanently +deleting an authored comment, revoking a remote device, or signing out every device— +require a passkey assertion or the operator access token again. The server issues a random 90-second grant bound to the active session, exact action, and exact target. Only its digest is stored, and the grant is consumed atomically on diff --git a/frontend/security-center.js b/frontend/security-center.js index c29776d..805d191 100644 --- a/frontend/security-center.js +++ b/frontend/security-center.js @@ -131,6 +131,7 @@ passkey_counter_anomaly: 'Passkey counter anomaly', device_revoked: 'Device access revoked', all_sessions_revoked: 'All device access revoked', issue_closed: 'Issue closed', pull_merged: 'Pull request merged', + comment_deleted: 'Comment deleted', pull_review_approved: 'Pull request approved', pull_review_changes_requested: 'Changes requested', gitea_time_logged: 'Gitea time logged', }; diff --git a/src/main.py b/src/main.py index 4352f1f..59918cd 100644 --- a/src/main.py +++ b/src/main.py @@ -412,6 +412,7 @@ StepUpAction = Literal[ "merge_pull", "submit_pull_review", "close_issue", + "delete_comment", "log_recap_time", "revoke_device", "revoke_all_sessions", @@ -4452,25 +4453,66 @@ async def _edit_conversation_comment( async def _delete_conversation_comment( - repository: str, number: int, comment_id: int + request: Request, + step_up_grant: str | None, + repository: str, + number: int, + comment_id: int, ) -> JSONResponse: + target = f"{repository}#{number}:{comment_id}" + await _require_step_up( + request, + step_up_grant, + action="delete_comment", + target=target, + ) + try: + journal = _security_event_store() + operation_id = await asyncio.to_thread( + journal.reserve, + "comment_deleted", + target=target, + ) + except SecurityEventStoreError: + return JSONResponse( + {"error": "Security activity is temporarily unavailable. The comment was not deleted."}, + status_code=503, + headers={"Retry-After": "1"}, + ) try: result = await asyncio.wait_for( gitea_proxy.delete_owned_comment(repository, number, comment_id), timeout=ISSUE_ACTION_TIMEOUT_SECONDS, ) except gitea_proxy.CommentMutationForbiddenError as exc: + try: + await asyncio.to_thread(journal.discard, operation_id) + except SecurityEventStoreError: + pass raise HTTPException( status_code=403, detail="You can only change your own comments" ) from exc except HTTPException: + try: + await asyncio.to_thread(journal.discard, operation_id) + except SecurityEventStoreError: + pass raise except Exception: + try: + await asyncio.to_thread(journal.discard, operation_id) + except SecurityEventStoreError: + pass return JSONResponse( {"error": "The comment deletion could not be confirmed. Please retry."}, status_code=503, headers={"Retry-After": "1"}, ) + try: + await asyncio.to_thread(journal.finalize, operation_id) + except SecurityEventStoreError: + # Upstream deletion is authoritative; pending evidence remains truthful. + pass return JSONResponse(result) @@ -4490,15 +4532,21 @@ async def edit_assigned_issue_comment( @app.delete("/api/v1/repos/{owner}/{repo}/issues/{number}/comments/{comment_id}") async def delete_assigned_issue_comment( + request: Request, owner: str, repo: str, number: int = PathParam(gt=0), comment_id: int = PathParam(gt=0), + step_up_grant: str | None = Header( + default=None, alias="X-Step-Up-Grant", max_length=128 + ), ): repository = f"{owner}/{repo}" if not await gitea_proxy.is_assigned_issue(repository, number): raise HTTPException(status_code=404, detail="Assigned issue not found") - return await _delete_conversation_comment(repository, number, comment_id) + return await _delete_conversation_comment( + request, step_up_grant, repository, number, comment_id + ) @app.patch("/api/v1/repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}") @@ -4517,15 +4565,21 @@ async def edit_assigned_pull_comment( @app.delete("/api/v1/repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}") async def delete_assigned_pull_comment( + request: Request, owner: str, repo: str, number: int = PathParam(gt=0), comment_id: int = PathParam(gt=0), + step_up_grant: str | None = Header( + default=None, alias="X-Step-Up-Grant", max_length=128 + ), ): repository = f"{owner}/{repo}" if not await gitea_proxy.is_assigned_pull(repository, number): raise HTTPException(status_code=404, detail="Assigned pull request not found") - return await _delete_conversation_comment(repository, number, comment_id) + return await _delete_conversation_comment( + request, step_up_grant, repository, number, comment_id + ) @app.patch("/api/v1/notifications/{thread_id}/comments/{comment_id}") @@ -4543,14 +4597,20 @@ async def edit_notification_comment( @app.delete("/api/v1/notifications/{thread_id}/comments/{comment_id}") async def delete_notification_comment( + request: Request, thread_id: int = PathParam(gt=0), comment_id: int = PathParam(gt=0), + step_up_grant: str | None = Header( + default=None, alias="X-Step-Up-Grant", max_length=128 + ), ): try: repository, number = await gitea_proxy.notification_conversation_target(thread_id) except Exception as exc: raise HTTPException(status_code=404, detail="Notification conversation not found") from exc - return await _delete_conversation_comment(repository, number, comment_id) + return await _delete_conversation_comment( + request, step_up_grant, repository, number, comment_id + ) @app.post("/api/v1/repos/{owner}/{repo}/issues/{number}/attachments", status_code=201) diff --git a/tests/test_dashboard_session_frontend.py b/tests/test_dashboard_session_frontend.py index 7965d19..729566c 100644 --- a/tests/test_dashboard_session_frontend.py +++ b/tests/test_dashboard_session_frontend.py @@ -777,6 +777,12 @@ def test_security_activity_explains_pending_outcome_confirmation(): assert "event.status === 'pending'" in source +def test_security_activity_labels_permanent_comment_deletion(): + source = SECURITY_CENTER_JS.read_text() + + assert "comment_deleted: 'Comment deleted'" in source + + def test_security_activity_labels_passkey_enrollment_without_html_rendering(): source = SECURITY_CENTER_JS.read_text() diff --git a/tests/test_security_activity.py b/tests/test_security_activity.py index 63afa22..193345b 100644 --- a/tests/test_security_activity.py +++ b/tests/test_security_activity.py @@ -401,6 +401,217 @@ async def test_successful_protected_actions_record_only_bounded_targets( assert b"abc123" not in persisted +@pytest.mark.anyio +@pytest.mark.parametrize( + "path", + [ + "/api/v1/repos/stackchain/api/issues/7/comments/42", + "/api/v1/repos/stackchain/api/pulls/7/comments/42", + "/api/v1/notifications/91/comments/42", + ], +) +async def test_comment_deletion_requires_exact_fresh_authorization_before_gitea( + security_access, monkeypatch, path +): + calls = [] + + async def assigned(*_args): + return True + + async def notification_target(thread_id): + assert thread_id == 91 + return "stackchain/api", 7 + + async def delete(*args): + calls.append(args) + return {"id": 42, "deleted": True} + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_issue", assigned) + monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned) + monkeypatch.setattr(main.gitea_proxy, "notification_conversation_target", notification_target) + monkeypatch.setattr(main.gitea_proxy, "delete_owned_comment", delete) + 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"], + } + response = await client.delete(path, headers=headers) + + assert response.status_code == 428 + assert response.json()["detail"] == { + "detail": "Fresh authorization required", + "code": "step_up_required", + "action": "delete_comment", + "target": "stackchain/api#7:42", + } + assert calls == [] + + +@pytest.mark.anyio +async def test_authorized_comment_deletion_records_only_privacy_safe_target( + security_access, monkeypatch +): + async def assigned(*_args): + return True + + async def delete(*_args): + return {"id": 42, "deleted": True} + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_issue", assigned) + monkeypatch.setattr(main.gitea_proxy, "delete_owned_comment", delete) + 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": "delete_comment", + "target": "stackchain/api#7:42", + }, + headers=headers, + ) + response = await client.delete( + "/api/v1/repos/stackchain/api/issues/7/comments/42", + headers={**headers, "X-Step-Up-Grant": grant.json()["grant"]}, + ) + events = (await client.get("/api/v1/security-events")).json()["events"] + + assert response.status_code == 200 + assert (events[0]["kind"], events[0]["target"], events[0]["status"]) == ( + "comment_deleted", "stackchain/api#7:42", "completed" + ) + persisted = (security_access / "security.sqlite3").read_bytes() + assert b"private comment body" not in persisted + assert b"correct horse battery staple" not in persisted + + +@pytest.mark.anyio +async def test_comment_deletion_reservation_failure_prevents_gitea_mutation( + security_access, monkeypatch +): + calls = [] + + async def assigned(*_args): + return True + + async def delete(*args): + calls.append(args) + return {"id": 42, "deleted": True} + + class UnavailableJournal: + def reserve(self, *_args, **_kwargs): + raise SecurityEventStoreError("unavailable") + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_issue", assigned) + monkeypatch.setattr(main.gitea_proxy, "delete_owned_comment", delete) + 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": "delete_comment", + "target": "stackchain/api#7:42", + }, + headers=headers, + ) + monkeypatch.setattr(main, "_security_event_store", lambda: UnavailableJournal()) + response = await client.delete( + "/api/v1/repos/stackchain/api/issues/7/comments/42", + headers={**headers, "X-Step-Up-Grant": grant.json()["grant"]}, + ) + + assert response.status_code == 503 + assert calls == [] + + +@pytest.mark.anyio +@pytest.mark.parametrize("finalize_fails", [False, True]) +async def test_comment_deletion_journal_tracks_failed_and_confirmed_outcomes( + security_access, monkeypatch, finalize_fails +): + journal_calls = [] + + async def assigned(*_args): + return True + + async def delete(*_args): + if not finalize_fails: + raise RuntimeError("upstream rejected deletion") + return {"id": 42, "deleted": True} + + class Journal: + def reserve(self, kind, *, target): + journal_calls.append(("reserve", kind, target)) + return "operation-42" + + def discard(self, operation_id): + journal_calls.append(("discard", operation_id)) + + def finalize(self, operation_id): + journal_calls.append(("finalize", operation_id)) + raise SecurityEventStoreError("unavailable") + + monkeypatch.setattr(main.gitea_proxy, "is_assigned_issue", assigned) + monkeypatch.setattr(main.gitea_proxy, "delete_owned_comment", delete) + 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": "delete_comment", + "target": "stackchain/api#7:42", + }, + headers=headers, + ) + monkeypatch.setattr(main, "_security_event_store", lambda: Journal()) + response = await client.delete( + "/api/v1/repos/stackchain/api/issues/7/comments/42", + headers={**headers, "X-Step-Up-Grant": grant.json()["grant"]}, + ) + + assert journal_calls[0] == ( + "reserve", "comment_deleted", "stackchain/api#7:42" + ) + if finalize_fails: + assert response.status_code == 200 + assert response.json() == {"id": 42, "deleted": True} + assert journal_calls[1] == ("finalize", "operation-42") + else: + assert response.status_code == 503 + assert journal_calls[1] == ("discard", "operation-42") + + @pytest.mark.anyio async def test_approved_pull_review_records_a_privacy_safe_completed_event( security_access, monkeypatch -- 2.43.0