Authorize and audit permanent comment deletion #780

Merged
timmy merged 1 commits from timmy/779-authorize-audit-comment-deletion into main 2026-08-13 22:58:02 +00:00
5 changed files with 285 additions and 6 deletions

View File

@ -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

View File

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

View File

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

View File

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

View File

@ -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