fix(core): permanent delete is missing trash guard and uses unrelated permission#912
Conversation
🦋 Changeset detectedLatest commit: bf384d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid bug fix. Both issues are real and the fix is minimal and correct:
- The added
AND deleted_at IS NOT NULLclause is standard ANSI SQL, dialect-safe, and properly enforces the "empty trash, not skip soft-delete" contract. The pre-fix SQL really would let a caller wipe live rows without going through trash — verified by the new integration test that fails without the guard. - Replacing
import:executewith a dedicatedcontent:delete_permanentis the right shape. Both gate atRole.ADMIN, so audience is unchanged. The permission lives in the right place in the ladder (next tocontent:delete_any) andPermissioniskeyof typeof Permissions, so type checking flows through cleanly. No other route was usingimport:executefor non-import work, so nothing else needs untangling.
Test coverage is appropriate:
- Integration test exercises the three behavioral paths (live refusal, missing id, trashed happy path) and verifies the row actually survives in the live-refusal case rather than just trusting the error code.
- Unit test covers the full role ladder (ADMIN allowed; EDITOR / AUTHOR / CONTRIBUTOR / SUBSCRIBER denied, handler not called).
One small nit inline. There's also a follow-up worth doing (out of scope here): with the new guard, permanentDelete returns false for both "row doesn't exist" and "row exists but is live", and the handler surfaces both as NOT_FOUND. The security invariant is intact (the row is preserved), but the second case is slightly misleading — the row IS found, just not eligible for permanent deletion. Worth a follow-up to distinguish NOT_FOUND vs NOT_TRASHED (probably 409) so the admin UI can render a clearer error if a stale tab tries to permanent-delete a restored item. The repository docstring in this PR already points at findByIdOrSlugIncludingTrashed as the discriminator.
Nothing blocking — happy to see this merge.
| * the caller is responsible for distinguishing these cases (typically via | ||
| * a follow-up `findByIdOrSlugIncludingTrashed` to surface NOT_FOUND vs | ||
| * NOT_TRASHED). The `AND deleted_at IS NOT NULL` clause is the safety net | ||
| * that prevents permanent delete from bypassing the trash workflow. |
There was a problem hiding this comment.
Nit: there are now two consecutive JSDoc blocks on permanentDelete. The original one-liner (Permanently delete content (cannot be undone)) was kept above the new detailed block. Worth collapsing into one — IDE hover and TSDoc tooling will only pick up the second block anyway, and the duplicate looks like a merge accident on a quick read.
| * that prevents permanent delete from bypassing the trash workflow. | |
| /** | |
| * Permanently delete a soft-deleted content row. | |
| * | |
| * Returns `true` only when a soft-deleted (trashed) row was removed. | |
| * Returns `false` when no row exists OR when the row exists but is live — | |
| * the caller is responsible for distinguishing these cases (typically via | |
| * a follow-up `findByIdOrSlugIncludingTrashed` to surface NOT_FOUND vs | |
| * NOT_TRASHED). The `AND deleted_at IS NOT NULL` clause is the safety net | |
| * that prevents permanent delete from bypassing the trash workflow. | |
| */ |
|
Review posted successfully. Summary:
|
2 participants
What does this PR do?
DELETE /content/:collection/:id/permanenthad two issues:No trash-state guard. The SQL was
DELETE FROM ${table} WHERE id = ?, so live rows could be permanently removed without going through soft-delete first. The query now requiresdeleted_at IS NOT NULL.Wrong permission domain. The route gated on
import:execute, an unrelated permission that happens to be ADMIN-only. Replaced with a newcontent:delete_permanent(also ADMIN-only — no audience change).Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output