Files
pad/internal/server/attachments_authz.go
xarmian 31075d996a fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409) (#1136)
* fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409)

The copy transaction holds advisory locks on BOTH workspaces, but the
attachment planner (PlanAttachmentCopy) and the server's per-row
attachment authorizer read through the connection pool. Under enough
concurrent copies every pooled connection can be occupied by a
lock-waiter while the lock holder waits for a spare connection —
starvation presenting as a hang.

Fix: a store.Queryer interface (satisfied by *sql.DB and *sql.Tx)
threaded through the planner and the AttachmentAuthorizer callback, so
the mutating copy plans and authorizes on its own transaction's
connection while the preflight keeps planning through the pool — one
implementation, two executors, preserving TASK-2354's no-drift shape.
Mechanical *Q variants added for the store reads the authorizer
transitively needs (GetItem, GetUser, GetWorkspaceMember,
VisibleCollectionIDs, GetMemberCollectionAccess,
ListSystemCollectionIDs, GuestVisibleCollectionIDs,
GuestVisibleResources(+IncludeDeleted), ResolveBacklinksVisibility) and
Q-cores behind existing-signature server wrappers (checkItemVisible,
guestResourceFilterCore, resolveAttachmentParentItem,
attachmentCallerIsRestricted). No decision logic changed anywhere —
executor threading only. GetItem/getItemTx/GetItemIncludeDeleted's three
duplicate scan bodies collapse into one getItemScanQ.

Regression test: TestCopyItemAcrossWorkspaces_NoPoolIOUnderLocks pins
the invariant deterministically — with MaxOpenConns(1) the transaction
owns the only connection, so ANY pool read under the locks deadlocks.
Fails by timeout on the pre-fix executor (verified); passes in 0.16s
fixed. The test's authorizer performs a real read through the handed
Queryer, pinning the callback leg too.

Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V

* fix(store): quota check reads through the copy transaction too, per Codex review (round 2)

Codex's targeted round found the third lock-held pool-read leg: CheckLimitTx
routed only the feature COUNT through the caller's transaction while
checkLimitOn's owner lookup, GetUser, and resolveLimit's platform-setting
read stayed on the pool — the same starvation shape under the copy's
advisory locks. checkLimitOn is now parameterized over a single Queryer for
every read (CheckLimit passes the pool, CheckLimitTx the transaction), with
resolveLimitQ / GetPlatformSettingQ variants behind existing-signature
wrappers. The regression test now arms this leg deliberately: a FREE-plan
owner with EnforceItemLimit and no plan override drives the full quota read
chain under MaxOpenConns(1) — verified deadlocking before this commit,
0.16s after.

Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
2026-08-17 08:16:02 -04:00

270 lines
13 KiB
Go

package server
import (
"net/http"
"github.com/PerpetualSoftware/pad/internal/models"
"github.com/PerpetualSoftware/pad/internal/store"
)
// attachmentParentOutcome classifies the result of resolving an attachment
// row's parent item.
//
// The four cases exist because attachments.item_id carries NO foreign key and
// NO same-workspace constraint (fixed at the source by TASK-2400, repaired for
// existing rows by PLAN-2397). "Resolves to an item" and "is a legitimate
// parent for THIS row" are therefore different questions, and every caller has
// to ask both. Splitting the outcomes rather than returning a bare bool keeps
// the malformed cases distinguishable for callers that want to log them
// (derivation does) without forcing that distinction onto callers that must
// NOT surface it (the HTTP paths, where any split is an existence oracle).
type attachmentParentOutcome int
const (
// attachmentParentOrphan — item_id IS NULL. There is no parent to
// authorize against, so the caller applies its own orphan rule; see
// attachmentCallerIsRestricted for the one the HTTP paths share.
attachmentParentOrphan attachmentParentOutcome = iota
// attachmentParentOK — resolved to an item of the attachment's own
// workspace. Live, unless the caller passed includeArchived.
attachmentParentOK
// attachmentParentGone — item_id names nothing the lookup returned:
// soft-deleted (when includeArchived is false), hard-deleted, or a
// dangling id that never named an item.
attachmentParentGone
// attachmentParentForeign — resolves to an item in ANOTHER workspace.
// Malformed for this row: a grant on that foreign item must never
// authorize anything against this workspace's bytes.
attachmentParentForeign
)
// resolveAttachmentParentItem loads and validates the parent item of an
// attachment row that has ALREADY been established as live and belonging to
// the request workspace.
//
// This is the one place the attachment-parent invariant lives. Before it, the
// blob read, the transform, thumbnail derivation and the delete path each
// hand-rolled "load the parent, check workspace identity, check liveness" in
// its own shape, and the three implementations drifted apart far enough to
// open two real gaps (see the commit that introduced this helper). The
// invariant is now shared; the DENIAL behaviour deliberately is not.
//
// What the callers keep for themselves, and why:
//
// - the blob read (handleGetAttachment) and transform
// (handleTransformAttachment) funnel every outcome except OK through
// writeAttachmentNotFound, so a missing attachment, a foreign parent, an
// archived parent and an invisible item are byte-identical;
// - derivation (deriveThumbnails) has no HTTP response to shape at all: it
// logs a distinct WARN per outcome — the malformed cases are meant to be
// greppable ahead of PLAN-2397's repair — and skips;
// - the delete path passes includeArchived, because the storage listing
// intentionally surfaces attachments whose parent item is soft-deleted:
// they still consume quota and the user needs a path to delete the blob.
//
// includeArchived selects GetItemIncludeDeleted over GetItem. With it false,
// "live" means exactly what the read gate (PLAN-2391 DR-13) means by it, and
// an archived parent reports Gone.
//
// Point-in-time, by construction: this is a read, and item deletion commits in
// its own transaction, so a caller that does unbounded work between this call
// and a write has a window. Transform closes its window with
// store.CreateAttachmentForLiveItem; derivation deliberately does not (see the
// comment on deriveThumbnails). Do not read a passing check here as a lock.
func (s *Server) resolveAttachmentParentItem(att *models.Attachment, includeArchived bool) (*models.Item, attachmentParentOutcome, error) {
return s.resolveAttachmentParentItemQ(s.store.Q(), att, includeArchived)
}
// resolveAttachmentParentItemQ is resolveAttachmentParentItem parameterized
// over its executor — the cross-workspace copy's attachment authorizer runs
// it on the copy transaction's connection (BUG-2409); every other caller
// uses the pool wrapper above. One body, two executors, so the invariant
// stays in one place.
func (s *Server) resolveAttachmentParentItemQ(q store.Queryer, att *models.Attachment, includeArchived bool) (*models.Item, attachmentParentOutcome, error) {
if att.ItemID == nil || *att.ItemID == "" {
return nil, attachmentParentOrphan, nil
}
var (
item *models.Item
err error
)
if includeArchived {
item, err = s.store.GetItemIncludeDeletedQ(q, *att.ItemID)
} else {
item, err = s.store.GetItemQ(q, *att.ItemID)
}
if err != nil {
return nil, attachmentParentGone, err
}
if item == nil {
return nil, attachmentParentGone, nil
}
// Workspace identity is checked HERE, ahead of every caller's own
// visibility/permission logic, because none of those catch a foreign
// parent on their own: checkItemVisible admits any collection id when
// the caller is unrestricted, and both the item-grant lookup and
// requireEditPermission match by item id. Without this guard a grant on
// a foreign item would authorize acting on THIS workspace's bytes.
if item.WorkspaceID != att.WorkspaceID {
return item, attachmentParentForeign, nil
}
return item, attachmentParentOK, nil
}
// attachmentCallerIsRestricted reports whether the caller's access to the
// workspace is narrowed to specific collections or items rather than being
// full workspace access.
//
// It is the orphan-row gate (PLAN-2382 DR-4), shared by the read, transform
// and delete paths. An orphan attachment (item_id IS NULL) belongs to no
// collection, so collection visibility cannot gate it — and the storage
// LISTING already hides orphans from restricted members. Without this, a
// restricted member who guesses an orphan's UUID gets confirmation it exists,
// which is the same disclosure the item-bound paths refuse.
//
// Callers MUST apply it AHEAD of any role gate that would answer 403: a 403
// reached only for rows that exist is itself the oracle.
func (s *Server) attachmentCallerIsRestricted(r *http.Request, workspaceID string) (bool, error) {
return s.attachmentCallerIsRestrictedQ(s.store.Q(), r, workspaceID)
}
// attachmentCallerIsRestrictedQ is attachmentCallerIsRestricted
// parameterized over its executor (see resolveAttachmentParentItemQ).
func (s *Server) attachmentCallerIsRestrictedQ(q store.Queryer, r *http.Request, workspaceID string) (bool, error) {
fullCollIDs, grantedItemIDs, err := s.guestResourceFilterCoreQ(q, r, workspaceID, false)
if err != nil {
return false, err
}
return fullCollIDs != nil || grantedItemIDs != nil, nil
}
// attachmentCopyAuthorizer builds the per-row visibility check the
// cross-workspace copy planner applies to every attachment reference it
// resolves in the SOURCE workspace (TASK-2408 / BUG-2407).
//
// It is the read path's rule, not a weaker cousin of it: the same
// resolveAttachmentParentItem outcomes, the same includeArchived=false
// (DR-13 — an archived parent is not readable, so it is not copyable), the
// same checkItemVisible, and the same orphan gate in the same ORDER, ahead
// of the role check. handleGetAttachment is the sibling to compare against
// line for line; the only difference is the shape of the denial, and that
// difference is forced: this one has no response to write, so it answers
// false and the planner turns that into "unresolvable".
//
// workspaceID is the SOURCE workspace, which is also the request's own
// workspace — the planner never resolves anything outside it, so
// guestResourceFilter and workspaceRole(r), both of which describe the
// caller's standing in the request workspace, are the right inputs. The
// destination side is authorized separately and earlier, by the ladder in
// resolveAuthorizedCopy.
//
// MEMOIZED PER PARENT ITEM, which is not an optimization detail but the
// difference between one query and N. With workspaceID and the caller
// fixed, the verdict is a pure function of the row's item_id: nothing
// below reads any other column. So the ordinary body — one item's images
// plus two thumbnail variants each — costs ONE item load and ONE
// visibility query instead of three per image, and the planner cannot turn
// a long reference list into an N+1 storm inside the copy's transaction,
// where both workspace advisory locks are held (Codex round 4). The
// restricted-ness verdict, a membership query used only by the orphan
// branch, is resolved once for the same reason. Errors are memoized too: a
// database that is failing should be asked once, not once per row.
//
// The memo therefore pins the caller's ACLs for the duration of ONE
// planner pass, which is the same point-in-time semantics the rest of this
// file has (resolveAttachmentParentItem's "do not read a passing check as
// a lock") and the same the plan itself has (PlanAttachmentCopy's
// STALENESS CONTRACT): a grant revoked mid-pass may not be observed by the
// remaining rows. Nothing is cached ACROSS requests — the closure is built
// per call of resolveAuthorizedCopy, so the preflight and the copy each
// re-derive every verdict from scratch.
//
// RESIDUAL: the query count is still linear in the number of DISTINCT
// parent items referenced. Bounding that would mean batching the item
// loads, which the per-row callback shape cannot express; it is bounded in
// practice by the attachments that actually exist in the source workspace,
// since an unresolvable reference never reaches this function at all.
//
// TRANSACTION-BOUND READS (BUG-2409). Every read below runs on the Queryer
// the PLANNER passes in — the pool on the preflight path, the copy's own
// in-flight transaction on the mutating path. That matters because the
// mutating planner runs inside a transaction holding advisory locks on
// BOTH workspaces: a read routed through the connection pool there could
// wait for a free connection while every pooled connection is itself
// occupied by a copy waiting on those locks — starvation presenting as a
// hang. On the transaction's own connection the reads cannot wait on the
// pool at all. The memo is what keeps the read count bounded rather than
// per-row; the q-threading is what keeps the bounded reads off the pool.
// NOTE the memo is keyed per parent item, not per (q, item) — sound
// because one closure only ever sees one executor: it is built per
// resolveAuthorizedCopy call and consumed by exactly one planner pass.
func (s *Server) attachmentCopyAuthorizer(r *http.Request, workspaceID string) store.AttachmentAuthorizer {
type verdict struct {
allowed bool
err error
}
var (
restricted bool
restrictedKnown bool
byParentItem = map[string]verdict{}
)
decide := func(q store.Queryer, att models.Attachment) (bool, error) {
item, outcome, err := s.resolveAttachmentParentItemQ(q, &att, false)
if err != nil {
return false, err
}
switch outcome {
case attachmentParentOK:
return s.checkItemVisibleQ(q, workspaceID, item, currentUser(r), workspaceRole(r), isBearerAuth(r))
case attachmentParentOrphan:
// The orphan rule (PLAN-2382 DR-4), and the restriction check
// BEFORE the role check for the reason every sibling path
// documents: a denial reachable only for rows that exist is
// itself the oracle. Here both denials collapse into the same
// "unresolvable" count anyway, but keeping the order identical
// to the read path is what makes the two comparable.
if !restrictedKnown {
isRestricted, err := s.attachmentCallerIsRestrictedQ(q, r, workspaceID)
if err != nil {
return false, err
}
restricted = isRestricted
restrictedKnown = true
}
if restricted {
return false, nil
}
return requireRole(r, "viewer"), nil
default: // Gone (missing, hard-deleted, or ARCHIVED parent), Foreign
return false, nil
}
}
return func(q store.Queryer, att models.Attachment) (bool, error) {
// Defense in depth: the planner scopes every query to the source
// workspace already, so this cannot fire today. It costs nothing
// and means the authorizer is safe to hand to any future caller
// without re-deriving that guarantee.
//
// Checked BEFORE the memo, because the memo is keyed by item_id
// alone and is only sound while every row belongs to workspaceID.
if att.WorkspaceID != workspaceID {
return false, nil
}
// Orphans are not memoized: they have no item_id to key on, and
// their branch runs no per-row query anyway.
if att.ItemID == nil || *att.ItemID == "" {
return decide(q, att)
}
if v, ok := byParentItem[*att.ItemID]; ok {
return v.allowed, v.err
}
allowed, err := decide(q, att)
byParentItem[*att.ItemID] = verdict{allowed: allowed, err: err}
return allowed, err
}
}