Merge pull request 'Authorize and audit permanent comment deletion' (#780) from timmy/779-authorize-audit-comment-deletion into main

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
+High-impact actions—merging a pull request, closing an assigned issue, permanently
-remote device, or signing out every device—require a passkey assertion or the
+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

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',
       };

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)

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()
 

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