feat(micropub): implement proposed media-endpoint extensions - #375
Conversation
davidwkeith
left a comment
There was a problem hiding this comment.
Reviewed against spec/packages/micropub.md's new "Proposed media-endpoint extensions" section and CONTRIBUTING.md.
CONTRIBUTING.md conformance: looks good — PR opened from the template with headings intact, checklist items ticked or left with a one-line reason, Conventional Commits title (feat(micropub): ...), colocated tests in src/*.test.ts, a changeset (.changeset/media-endpoint-extensions.md, correctly scoped @dwk/micropub: minor), config threaded through MicropubConfig/ResolvedConfig rather than read from env, metadata stored in D1 (MICROPUB_DB) not KV, and R2 trash-copy is streamed (body.body) rather than buffered. pnpm --filter @dwk/micropub typecheck, pnpm test --project @dwk/micropub (232/232), eslint, and prettier --check all pass clean on the touched files.
Spec conformance: verified against the design doc — disabled-path byte-identical behavior, URL ownership validation (rejects foreign origin/wrong prefix/traversal/encoded slash/trash prefix/query-fragment, all before any storage access), scope enforcement (media for q=source; both action-scope + media for delete/undelete), the delete/undelete state machine's four rows, fail-closed vs. best-effort metadata handling, the public GET route never serving .trash/, and q=source ordering/pagination — all match the spec and are backed by tests (the URL-ownership rejection matrix in media.test.ts is particularly thorough).
Left three inline notes — none are blockers, but the first is worth a look before merge:
- Should-fix:
foldUploadedMedia's multi-file loop doesn't roll back earlier-file blobs+rows when a later file's metadata insert fails mid-loop, leaving orphaned listed media after a failed create — inconsistent with the single-upload path's stated fail-closed guarantee. - Should-fix: the scope-pair test only covers
action=delete, notaction=undelete— a hardcoded-scope bug in the undelete path wouldn't be caught. - Nit: cosmetic wording inconsistency between the two 403
insufficient_scopepaths.
Nice work on the URL-ownership defense and the delete state machine — the resumed-partial-failure and post-purge-permanence test cases in particular give good confidence in the recoverable-delete design.
Generated by Claude Code
Implements the #363 design (roadmap #354), gated behind extensions.proposed: - Media q=source: newest-first listing (limit/offset) and by-URL lookup over a new micropub_media D1 metadata table, requiring the media scope. - Upload response body: 201 + Location gains the upstream-minimum { url } JSON body when enabled. - action=delete/undelete at the media endpoint: recoverable soft delete via the R2 .trash/ prefix (copy-then-delete ordering, convergent retries), scope-pair enforcement (delete+media / undelete+media), and load-bearing URL ownership validation before any storage access. - Metadata recording is unconditional; insert failure is fail-closed (rollback + 500) only when the group is on, best-effort (log, keep 201) when off, so disabled-path behaviour stays byte-identical. - Public blob GET now serves only single-segment keys, so the trash prefix is never servable; expired trash rows are pruned opportunistically using the new mediaTrashRetentionDays config (default 30, blob purge delegated to an R2 lifecycle rule). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5e189d9 to
26d78b9
Compare
Address review on #375: in foldUploadedMedia a later file's failure (metadata insert with the proposed group on, or an oversized file) left earlier files' R2 blobs and micropub_media rows committed with no post to reference them — servable, listed orphans from a request that 500ed. Track the URLs stored so far and best-effort delete blob + row on failure before rethrowing (the URLs were never handed to the client). Adds MicropubMediaStore.remove() for the row rollback and the missing undelete scope-pair test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Implements the proposed media-endpoint extensions designed in #363 (roadmap #354), completing the last outstanding item of that roadmap. Everything is gated behind
extensions.proposed(off by default; disabled-path media behaviour stays byte-identical, including the metadata-insert failure surface):q=source— newest-first listing (limit/offset, matching the post list) and by-URL lookup at the media endpoint, backed by a newmicropub_mediaD1 metadata table (src/media.ts); requires themediascope.201+Locationgains the upstream-minimum{ "url": ... }JSON body when enabled.action=delete/action=undelete— recoverable soft delete via the R2.trash/prefix with the copy-then-delete ordering and convergent retry state machine from the spec, scope-pair enforcement (delete+media/undelete+media), and load-bearing URL ownership validation before any storage access.500) only when the group is on, and best-effort (structured warning,201preserved) when off — per the amended spec from docs(micropub): design proposed media-endpoint extensions #374.GETroute now serves only single-segment keys (the trash prefix is never servable), and expired trash rows are pruned opportunistically during media-endpoint writes via the newmediaTrashRetentionDaysconfig (default 30; blob purge is delegated to an R2 lifecycle rule).Includes the spec design from #374 (this branch contains those commits; land #374 first and this PR's diff reduces to the implementation + the spec status-line flip to "implemented").
Closes #354's remaining item; depends on #374.
Packages affected
@dwk/micropub
Checklist
spec/packages/and updated them ifbehaviour changed
src/*.test.ts)pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm testpnpm changeset) if this touches a publishablepackage
catalog.json/conformance/status.json— not applicable, no new worker and no conformance status change🤖 Generated with Claude Code