handleTransformAttachment opened with a flat requireMinRole("editor") and
never looked at the attachment's parent item at all. A restricted editor —
one whose collection access excludes that item — could transform an
attachment on an item they cannot see, given only the attachment id: the
handler read the source blob and returned output metadata plus a new row.
The output URL inherits ItemID and is gated by TASK-2401's read gate, so
this was not direct byte exfiltration, but it crossed the same boundary and
leaked processing behaviour and metadata for an invisible item.
The handler now authorizes per-attachment, in the order the read path uses
(PLAN-2391 DR-10): load the row -> workspace identity -> load the parent
with GetItem -> parent workspace identity -> checkItemVisible -> edit
permission -> transform. Every denial goes through writeAttachmentNotFound,
so a missing attachment, a foreign parent, a soft-deleted parent and an
invisible item are byte-identical; a distinguishable code or message would
be an existence oracle. Malformed non-null parents that resolve nowhere are
rejected by the same guard.
Edit permission is requireEditPermission rather than the flat editor role:
an item- or collection-grant editor can already attach to the item
(BUG-1661), so refusing them a rotate on their own upload would be an
inconsistency, not a boundary. Orphan rows keep the flat editor gate and,
matching the DELETE path (PLAN-2382 DR-4), require unrestricted workspace
access — the storage listing hides orphans from restricted members, so the
transform must not confirm one exists.
DR-14's race is closed, not narrowed. The parent check is point-in-time:
item deletion commits in its own transaction, and the blob read, decode,
transform, encode and Put in between are unbounded work, so the item can be
archived mid-flight and the insert then writes a quota-counted live row
against an archived item whose bytes DR-13 refuses to serve. The new
store.CreateAttachmentForLiveItem re-checks the parent under a row lock
inside the insert's own transaction: the row is written against a live item
or not written at all. FOR NO KEY UPDATE, not FOR UPDATE — DeleteItem's
UPDATE touches no key column so the archival still blocks, while the many
tables with a REFERENCES items(id) foreign key (comments, stars, the Yjs
op-log) keep taking FOR KEY SHARE on the parent uncontended. SQLite skips
the clause: _txlock=immediate already serializes writers there.
Tests fail against the pre-fix code: the restricted-editor transform
returns 404 with a body byte-identical to the missing-attachment body, and
the mid-flight test archives the item from inside the processor's Encode —
between the up-front check and the insert — asserting the hook actually ran
so it cannot pass vacuously. The Postgres lock test polls pg_stat_activity
until the statement is registered as lock-blocked rather than sleeping, and
watches the completion channel so a missing lock fails immediately. Both
were mutation-verified.
Recorded, not fixed here: a refused insert leaves a rowless blob on disk,
and the orphan GC is row-driven so nothing reclaims it. Pre-existing on the
upload and thumbnail paths too; filed as BUG-2406 with the dedupe guard a
correct fix needs. The comment claiming GC reclaims a transform's original
was wrong and is corrected — only an orphan original is GC-eligible.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
xarmian
380b75e12c