Media delete reports success when the storage delete failed, and the public file route serves objects with no DB row, so a "deleted" file can stay downloadable forever

[marks-zyz]

Description

DELETE /_emdash/api/media/:id reports {"deleted": true} even when the object was never removed from storage, and the public file route serves objects with no database lookup. Together these mean a "deleted" media file can stay publicly downloadable forever, while the admin shows it as gone.

Two halves of the problem, both on 0.37.0:

1. The storage delete failure is swallowed. In packages/core/src/astro/routes/api/media/[id].ts the DB row is deleted first, then:

if (emdash.storage) {
  const repo = new MediaRepository(emdash.db);
  await removeUploadAttempt(emdash.storage, repo, result.data.storageKey, {
    allowUntracked: true,
  });          // line 160 — return value ignored
}
return apiSuccess({ deleted: true });   // line 165 — unconditional

removeUploadAttempt (packages/core/src/media/upload-attempts.ts:20-25) catches every storage error, console.errors it and returns false:

try {
  await storage.delete(storageKey);
} catch (error) {
  console.error("[media] upload cleanup failed:", error);
  return false;
}

The route never reads that boolean, so any storage-side failure (R2 incident, bucket lock, credentials, S3 endpoint error) produces a success response and a deleted DB row.

2. The public file route needs no DB row. packages/core/src/astro/routes/api/media/file/[...key].ts (90 lines) resolves the key straight against storage. It has zero references to emdash.db or MediaRepository; the only key-based rejection is the backups/ prefix. So an object with no media row is still served at /_emdash/api/media/file/<storageKey> with 200.

Why this is worse than an orphan-storage-cost issue: nothing ever reclaims these objects. MediaRepository.cleanupPendingUploads (packages/core/src/database/repositories/media.ts:617-623) starts from the database (where status = 'pending'), so an object whose row is gone is unreachable by the cleanup. The file is invisible in the admin, unreferenced by the DB, permanently public, and only its URL is needed to fetch it. For a site that deletes a file precisely because it should not have been published (personal data, a wrong document, a takedown request), "delete" that silently does nothing is a privacy/GDPR problem, not a housekeeping one.

Suggested fix:

1. Make the route honour the result: if removeUploadAttempt returns false, respond with an error (or a success that explicitly reports storageDeleted: false) instead of a bare deleted: true, so callers and operators learn the object survived.
2. Consider deleting from storage before dropping the DB row, or recording the orphan (e.g. re-inserting/keeping a row marked for retry) so a later sweep can finish the job. Today the row is destroyed first, which is what makes the orphan unreachable.
3. Optionally, have the public file route reject keys with no corresponding media row, so an orphan is not served even if one exists.

I am happy to send a PR for (1) plus (2) if the direction sounds right.

Steps to reproduce

Part 2 is directly reproducible on any EmDash site with R2 storage, no failure injection needed. It shows that an object with no DB row is public:

# 1. put an object straight into the site's bucket, so no media row exists
echo hello > orphan.txt
npx wrangler r2 object put "<bucket>/orphan-test.txt" --file orphan.txt \
  --content-type text/plain --remote

# 2. confirm the CMS has no row for that key
curl -s -H "Authorization: Bearer <admin token>" \
  "https://<site>/_emdash/api/media?limit=50" | jq '[.data.items[]
  | select((.storageKey // .meta.storageKey // "") == "orphan-test.txt")] | length'
# -> 0

# 3. fetch it through the public media route
curl -s -o /dev/null -w '%{http_code}\n' \
  "https://<site>/_emdash/api/media/file/orphan-test.txt"
# -> 200, and the body is the file

Observed on a production site running 0.37.0: step 2 returned 0 and step 3 returned 200 with the exact 25-byte body. After wrangler r2 object delete, the same URL returned 404, confirming the route was serving purely from storage.

For part 1, make storage.delete fail (an R2 bucket lock, a storage binding without delete permission, or an R2 incident like the 2025-02-06 and 2025-03-21 outages where R2 returned errors for 100% of operations) and delete a media item from the admin: the UI reports success, the item disappears from the library, and the file is still served at its URL.

Environment

• emdash 0.37.0, @emdash-cms/admin 0.37.0
• Astro 7 on Cloudflare Workers, D1 + R2 storage
• Reproduced against a production deployment; line numbers taken from the published 0.37.0 tarball.

Screenshots

Not applicable — the evidence is the HTTP status codes above.

Logs / error output

On a swallowed storage failure the only trace is a Worker log line, and nothing reaches the API caller:

[media] upload cleanup failed: <error>

Related

• Discussion #1503 proposes a usage index for safe deletes (blocking deletion of media still referenced in content). That is the opposite end of the same flow and does not cover this case: here the delete is intended, and the failure is that it silently does not happen.
• Issue #2689 is about the same file route ignoring Range requests. Unrelated cause, same handler.

[emdashbot]

Completed

Investigate whether the media delete and file-serving behavior matches the reported bug, and whether a fix can proceed automatically.

• Verify the reported behavior in source code
• Assess security/privacy and backwards-compat impact
• Report triage disposition

Outcome: Both reported behaviors are present in the current source:

1. **Delete reports success when storage delete fails.** `packages/core/src/astro/routes/api/media/[id].ts` calls `removeUploadAttempt` (which catches storage errors and returns `false`) but never reads that boolean, so it unconditionally returns `{ deleted: true }` after the DB row is already gone.

2. **Public file route serves unreferenced objects.** `packages/core/src/astro/routes/api/media/file/[...key].ts` reads straight from `emdash.storage` with no DB lookup; only the `backups/` prefix is rejected. Anything that exists in the bucket is publicly fetchable.

Together this is a privacy/security bug: a media delete can claim success while the file remains publicly downloadable indefinitely, and the CMS has no DB row to find or clean it up (`cleanupPendingUploads` starts from the DB).

But the fix needs maintainer decisions, so it should not be auto-worked:

• Making the file route require a DB `media` row adds a Kysely lookup to a logged-out hot path; per the repo guidelines that needs a strong justification and a query-count review.
• Reordering delete so storage is removed before the DB row changes the failure mode (a failed delete would now leave a DB row pointing at nothing, which is arguably better but still needs handling).
• The route's response contract would change, so the admin and any API consumers need to know the new shape.
• It calls for reproducing tests first (TDD for bugs).

Recommended next step: approve a focused change that adds failing unit/integration tests for both halves, then:

• surface `removeUploadAttempt`'s result (return an error or `storageDeleted: false`) instead of unconditional `{ deleted: true }`, and
• either require a `media` row in the file route or record the orphan so cleanup can retry.

Mode: triage

View live dashboard · Run: c9286a9f-e93f-4b76-89af-2e401aabbb6a

[swissky]

Thanks for the precise report — fix for the delete side in #3067 (storage deletion moved into the shared handler, failed deletes are retried by the cleanup sweep and reported as storageDeleted: false). The file-route half is left for a maintainer call, since gating on a media row adds a query to a logged-out route.