[Backend] Approving a lore proposal does not check deleted_at, so a trashed proposal can be resurrected #630

New issue

Closed

opened 2026-09-12 18:14:10 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-09-12 18:14:10 +00:00

Collaborator

Found while fixing #524, and deliberately left out of that PR (#629) because it is a write path with different consequences, not a count.

POST /campaigns/{campaign_id}/lore/{entry_id}/approve (routers/campaigns.py, around line 3449) looks the proposal up with:

entry = await db.scalar(
    sel(LoreEntry)
    .options(selectinload(LoreEntry.session_appearances))
    .where(
        LoreEntry.id == entry_id,
        LoreEntry.campaign_id == campaign_id,
        LoreEntry.proposed_by_bot == True,  # noqa: E712
    )
)

No deleted_at IS NULL. approve_lore_proposal then flips proposed_by_bot to False, which makes it a normal entry — while deleted_at stays set, so it is now a trashed approved entry: invisible in the wiki, sitting in the trash with a deleted_at it never earned, and due to be hard-deleted by _purge_trashed_lore_entries_async when its grace period expires. The GM's approval silently does nothing they can see.

Reachable how. Not from a freshly loaded proposals page — the listing filters correctly. It needs the id to come from somewhere staler: a browser tab left open across a delete, a double submit, a back-button, or anything replaying the request. Low frequency, but the failure is silent in both directions (nothing appears in the wiki, no error is shown).

Expected: approving an entry that is in the trash is a 404, the same answer every other read path gives for a trashed entry — or, if a GM approving something they had deleted should really mean "undelete and approve", that should be a deliberate decision with deleted_at cleared, not an accident of a missing predicate.

Acceptance:

The approve lookup filters deleted_at IS NULL (or explicitly clears it, if the decision goes the other way).
A test: trashing a proposal and then approving it by id returns 404 and leaves the entry trashed and still proposed_by_bot.
Check the reject/dismiss counterpart and the relationship-proposal approve path for the same gap while in there.

Related: #524 (the count), #408 (soft delete). Worth noting the model comment that claimed all this filtering went through a single live_lore_entries helper was wrong — there is no such helper, every query carries its own predicate, and #629 corrected the comment.

Found while fixing #524, and deliberately left out of that PR (#629) because it is a write path with different consequences, not a count. `POST /campaigns/{campaign_id}/lore/{entry_id}/approve` (`routers/campaigns.py`, around line 3449) looks the proposal up with: ```python entry = await db.scalar( sel(LoreEntry) .options(selectinload(LoreEntry.session_appearances)) .where( LoreEntry.id == entry_id, LoreEntry.campaign_id == campaign_id, LoreEntry.proposed_by_bot == True, # noqa: E712 ) ) ``` No `deleted_at IS NULL`. `approve_lore_proposal` then flips `proposed_by_bot` to False, which makes it a normal entry — while `deleted_at` stays set, so it is now a *trashed approved entry*: invisible in the wiki, sitting in the trash with a deleted_at it never earned, and due to be hard-deleted by `_purge_trashed_lore_entries_async` when its grace period expires. The GM's approval silently does nothing they can see. **Reachable how.** Not from a freshly loaded proposals page — the listing filters correctly. It needs the id to come from somewhere staler: a browser tab left open across a delete, a double submit, a back-button, or anything replaying the request. Low frequency, but the failure is silent in both directions (nothing appears in the wiki, no error is shown). **Expected:** approving an entry that is in the trash is a 404, the same answer every other read path gives for a trashed entry — or, if a GM approving something they had deleted should really mean "undelete and approve", that should be a deliberate decision with `deleted_at` cleared, not an accident of a missing predicate. **Acceptance:** - [ ] The approve lookup filters `deleted_at IS NULL` (or explicitly clears it, if the decision goes the other way). - [ ] A test: trashing a proposal and then approving it by id returns 404 and leaves the entry trashed and still `proposed_by_bot`. - [ ] Check the reject/dismiss counterpart and the relationship-proposal approve path for the same gap while in there. Related: #524 (the count), #408 (soft delete). Worth noting the model comment that claimed all this filtering went through a single `live_lore_entries` helper was wrong — there is no such helper, every query carries its own predicate, and #629 corrected the comment.