The upload handler read item_id from two places with different rules:
authorization resolved only the query-string value, while the association
step fell back to the multipart-form value and persisted it verbatim. Since
ResolveItem accepts a UUID, a ref, or a slug, a form-supplied ref or a
foreign-workspace id could land in attachments.item_id unauthorized and
unresolvable — the malformed-row invariant BUG-2387's cross-workspace leak
rests on.
Three coupled changes (PLAN-2391 DR-2):
1. One effective item_id. Each non-empty channel is resolved in the request
workspace and the RESOLVED canonical ids are compared — not the caller's
spelling, so query "TASK-12" + form "<uuid>" is agreement, not conflict.
Absent and explicitly-empty both mean "no value" (compared after
TrimSpace). item.ID is what gets persisted. The form value is read from
r.MultipartForm.Value rather than r.FormValue, which merges the query
string back in and would collapse the two channels into one. A channel
that repeats item_id has every value resolved rather than first-wins,
since net/http otherwise silently discards the rest; the value count per
channel is capped, because exact-string dedup can't bound the lookups on
its own (TASK-7 / task-7 / TASK-0007 resolve alike).
2. Auth ordering. The no-item workspace-editor gate is deferred until after
multipart parsing; firing it pre-parse 403'd a form-only item-grant guest
(the CLI's shape) before the association that authorizes them was read.
The query channel is still resolved and authorized pre-parse so a doomed
upload never spools. The route's auth/workspace-access middleware chain
is unchanged.
3. Spool cleanup. file.Close() closes the spooled multipart temp file but
never removes it; added r.MultipartForm.RemoveAll() on every exit path,
including success, where it leaked today too.
Status codes (the pinned contract): an item_id that does not resolve in the
request workspace → 404 item_not_found on either channel, cross-workspace
UUIDs included; two channels — or two values on one channel — that each
resolve but to different items → 400 item_id_conflict.
Folded in from review: each resolved item is gated on requireItemVisible
(404) before the values are compared and before requireEditPermission (403).
Without that, the status split is an existence oracle for items a restricted
member or ungranted guest can't see — directly via 404-vs-403, or by pairing
a visible id with the id being probed and reading 400-vs-404. It also closes
requireEditPermission's editor/owner fast path, which never consults
collection visibility, so a collection_access="specific" member could
otherwise attach to an item in a collection hidden from them.
Two intentional behaviour narrowings, both following from DR-2's "reject a
non-empty value that does not resolve": an item_id for a soft-deleted item
now 404s where a workspace editor previously got a 201 (ResolveItem is
live-only) — consistent with DR-13/DR-14 keeping archived parents from
accruing new bytes; and an unresolvable item_id no longer falls back to the
flat editor gate and silently stores the caller's string.
Tests: extends TestUpload_GrantBasedEditorCanAttach with the form-only and
both-channel grant-guest cases, the ungranted-item 404, and the paired-probe
oracle check; adds canonical-UUID persistence, 404/400 rejection with no row
written, repeated conflicting values, the value-count cap, and a >1 MiB
isolated-TMPDIR fixture for the spool (a tiny in-memory body never spills to
disk, so it would pass either way). The auth-ordering and spool tests were
mutation-checked against the pre-fix behaviour.
Gates: make check (exit 0), make test-pg (exit 0).
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
xarmian
27b71fe4f6