docs(specs): attachment target authorization — guards, feature split, one org scope

[bartek5412]

Summary

Design-only spec — no implementation in this PR.

POST /api/attachments carries the target it is about to write (entityId + recordId), but the only authorization it runs is requireFeatures: ['attachments.manage'] — a single tenant-wide feature that cannot tell "set my own avatar" from "replace an organization's logo". Both halves of the decision are the same request, so there is no seam at which "which target is this?" can be asked. An application is left choosing between granting attachments.manage to everyone (any signed-in account can then overwrite the organization logo, delete anyone's attachment and reconfigure storage partitions) and withholding it (invited staff can upload nothing at all, their own avatar included). The third option — an app-level target guard wired into the application's own API dispatcher, ~350 lines re-deriving core's route table — is what applications actually ship today.

The spec grounds four gaps against develop and proposes three changes plus one open question:

1. The attachments module never runs the mutation-guard registry. runRouteMutationGuards already models exactly this concern and is called by makeCrudRoute, communication_channels and warranty_claims; attachments has no data/guards.ts and no call anywhere. With no guards registered the registry returns ok: true, so wiring it in is behaviour-preserving for every existing install.
2. attachments.manage is one feature for five powers — upload, delete, metadata edit, transfer, and storage-partition administration. "Any user may attach a photo" therefore also means "may delete anyone's file and reconfigure storage".
3. The routes disagree about which organization they are in. Upload and DELETE /api/attachments use resolveAttachmentOrganizationId (selected org, RBAC-validated, added for bug: [Attachments] — POST /api/attachments silently uses the uploader's default/home organization instead of the currently selected organization, causing uploaded files to land in the wrong org open-mercato/open-mercato#3765); library/[id] and transfer filter on raw auth.orgId. A file uploaded under a non-home selected organization 404s from the library route — its owner cannot delete it. This is the one item an application cannot work around: it can refuse a request, but not redirect core's own query.
4. Open question: matchRoutePattern compares literal path segments case-insensitively, so /api/Attachments matches. Deliberate and worth documenting, or worth tightening?

Scope boundary. What this delivers is the seam, not avatar/logo rules themselves — core has no avatar feature and whether an admin may set someone else's avatar is product policy. After this lands an application expresses such a rule in ~20 lines in its own data/guards.ts.

Changes

• New spec: .ai/specs/2026-08-31-attachment-target-authorization.md (design-only, status: draft)

Specification

Does a spec exist for this feature/module?

• Yes
• No (created a new spec)
• N/A (minor change, no spec needed)

Spec file path:
.ai/specs/2026-08-31-attachment-target-authorization.md

Testing

Docs-only change (one new markdown file); no build or test surface is affected. Every load-bearing claim was verified against develop at 7f871e603 while drafting: the absent data/guards.ts and absent registry call under packages/core/src/modules/attachments, the five endpoints gated by acl.ts:3, the resolveAttachmentOrganizationId / auth.orgId split across api/route.ts:339,682 vs api/library/[id]/route.ts:159,241 and api/transfer/route.ts:49, the attachments.* wildcard already granted in setup.ts:5, and the live dependsOn contract in the customers reference module. Per-surface unit and integration coverage is enumerated in the spec's Test Plan and ships with the implementation PRs.

Checklist

• This pull request targets develop.
• I have read and accept the Open Mercato Contributor License Agreement (see docs/cla.md). (Left for the human author to tick personally.)
• I updated documentation, locales, or generators if the change requires it. (Docs-only; the spec itself flags a packages/core/AGENTS.md update as part of implementation Phase 1.)
• I added or adjusted tests that cover the change. (N/A — design-only spec; tests are enumerated in the Test Plan and ship with the implementation PRs.)
• I added or updated integration tests in .ai/qa/tests/ (or documented why integration coverage is not required). (Not required: no behaviour change in this PR. The spec specifies the integration coverage each phase must ship.)
• I created or updated the spec in .ai/specs/ with a changelog entry (if applicable).
• Priority set — see the label comment below (fork contributors cannot apply labels).
• Risk set — see the label comment below.
• QA routing set — see the label comment below.

Design System Compliance

N/A — no UI surface in this PR (one markdown file).

Linked issues

• Answers the open question in No avatar/photo field on auth.User — first-class column, or attachments? open-mercato/open-mercato#4717 (No avatar/photo field on auth.User — first-class column, or attachments?): the attachment route is viable, but not without gap 1; the spec's opt-in selfOwnedUserAttachmentGuard answers that issue's closing question about who may change whose avatar.
• Phase 2 absorbs feat(attachments): declare ACL feature dependencies open-mercato/open-mercato#2152 (feat(attachments): declare ACL feature dependencies) — same file, and the spec refines §6.12's proposed dependsOn table for the split features rather than colliding with it.
• Same defect class as bug(sync_excel): wire upload and import writes through mutation guards open-mercato/open-mercato#3312 (bug(sync_excel): wire upload and import writes through mutation guards), cited as evidence that the "future write route escapes the registry" risk is not hypothetical.
• Adjacent to docs(specs): RFC — actor-aware command guard policy (structural vs workflow guards) open-mercato/open-mercato#5726 (RFC — actor-aware command guard policy); its structural/workflow taxonomy is adopted — attachment target guards are structural.

[jtomaszewski]

@bartek5412 — we ran a zoom-out + prior-art pass on this spec (fresh-context second opinion, internal precedent sweep, and verified external research). Verdict: the direction is right — please keep going. Below is what confirmed it, four concrete improvements that would strengthen the spec, and context on where this lands strategically.

How the direction was validated

• Blind test: a fresh-context agent given only the problem (never shown this spec) independently converged on the same three moves — guard-registry adoption, feature split incl. attachments.partitions.manage, org-resolver unification — and independently rejected a two-phase redesign as out of scope for the authorization problem.
• Internal precedent: warranty_claims/api/portal/attachments/route.ts already implements exactly this pattern (load target claim → verify ownership → runRouteMutationGuards with the claim as resourceKind/resourceId). packages/core/AGENTS.md already mandates "Wire custom write routes through the mutation guard contract" — so gap 1 is compliance debt, not novel design. Adoption today: 339 hand-written write-route files in core, ~22 run the registry (~6%), attachments 0.
• The bug class is real in the wild: e.g. Vikunja GHSA-jfmm-mjcp-8wq2 (CVSS 8.1): "The permission check in CanRead() validates access to the task specified in the URL, but ReadOne() loads a different attachment that may belong to a task in another project." Your design point of resolving delete/transfer targets from the stored row (not the request) is exactly the fix for that class — worth keeping prominent.

Requested improvements to the spec

1. Name the deny-only limitation. Guards can only narrow; "grant by relationship alone, with zero feature" stays inexpressible at this seam (e.g. a caller without attachments.upload setting their own avatar). The realistic cases are covered by granting attachments.upload to employee + guards narrowing, and the repo's existing escape hatch — a module-owned route, warranty-portal style — covers relationship-only grants. Saying this explicitly (and why a tri-state allow/deny/abstain policy registry was not chosen: it forces feature checks out of declarative metadata into handlers) preempts the strongest review objection.
2. Cite AttachmentTargetAccessService (attachments/lib/target-access-service.ts, DI-registered, consumed by warranty_claims). It is the read-side "may this caller reach that target" primitive; Phase 1 is its missing write-side twin. Naming the symmetry strengthens the case and points implementers at existing conventions.
3. Add a forward-compat note for staged uploads. Maintainer direction is that a follow-up spec introduces two-phase upload (first-class unattached blobs; skeleton: .ai/specs/2026-09-01-staged-attachments-two-phase-upload.md). Under that design the upload-route guard call site moves to the attach mutation — the guard contract (resourceKind/resourceId/operation) is location-independent, so please keep the module-local guard helper target-shaped (not route-shaped) so the later move is mechanical. Everything else in this spec (feature split, org unification, delete/transfer guard sites) carries over unchanged.
4. Phase 4 has a stronger answer available. Whichever way the case-sensitivity decision goes, the docs should state that app-level logic keyed on request URLs MUST use the framework's own matchRoutePattern — URL-shape-keyed authz outside the router is exactly what gets bypassed (cf. Next.js GHSA-f82v-jwr5-mffw, CVSS 9.1: middleware-layer authz bypassed wholesale).

Optional: the Problem Statement can cite two more in-repo evidence points that the single-request contract leaks — AI chat uploads fake a target (entityId: 'ai-chat-draft' + minted per-batch UUID, apps/docs/docs/framework/ai-assistant/attachments.mdx) and the CrudForm field renders recordId ?? 'pending' on create.

Strategic framing (so the scope boundary is clear)

Stripe/Slack/Shopify solve this with two-phase upload — but two-phase relocates the target check to the attach step rather than eliminating it (Slack's files.completeUploadExternal fails with posting_to_channel_denied; Stripe attaches evidence by updating the dispute). So this PR is not an alternative to the industry pattern; it builds the authorization seam that pattern needs, at the only place the target is currently known. Sequenced this way there is exactly one behavior change ever (your Phase 3 org-scope fix, needed under any design) and everything else — including the later staged-upload work — lands additively.

One open point where your input is welcome: land Phase 1's guard call at the upload route as written (ships protection sooner, moves later), or aim it at the attach seam from the start? Current lean is the former; Jacek has the final call.

[jtomaszewski]

w8 with, this I may have more feedback

[jtomaszewski]

@bartek5412 follow-up from the maintainer discussion — the guards direction is confirmed (alternatives weighed below), but the discussion surfaced one real defect in the spec's design plus two structural amendments. Please fold these into the next revision.

1. Defect: guard dispatch collision between "attach to X" and "write X"

The registry dispatches on a single key — matchesEntity(guard.targetEntity, input.resourceKind) (packages/shared/src/lib/crud/mutation-guard-registry.ts:102) — and makeCrudRoute passes the entity being written as resourceKind. The spec has attachment routes call guards with resourceKind = target entityId (e.g. auth:user), which is the same key the target module's own CRUD writes use. Consequence: a guard meaning "who may attach a file to a user" also fires on every ordinary user-record update, and vice versa. The spec's Architecture example (resourceKind === 'auth:user' && resourceId !== userId → 403) would block an admin editing another user's profile through plain user CRUD wherever those writes run guards.

Proposed fix: an additive optional discriminator on the guard input — e.g. surface?: 'crud' | 'attachments' (default 'crud'), with MutationGuard optionally declaring surfaces?: string[]; guards that don't declare it keep today's fire-everywhere semantics, so every existing guard (optimistic-lock, customer_accounts domain rules) is unaffected. Additive optional fields on these types are compatible per BACKWARD_COMPATIBILITY.md. Payload-sniffing to tell the two apart should be explicitly rejected in the spec.

2. Amendment: enforce in the service, not per-route

Move the guard invocation from the six route call sites into attachmentService attach/create/detach/delete operations (single choke point). Wins:

• R5 dies structurally — a future route cannot forget the check, which beats the spec's manifest-enumeration unit test (that test can stay as belt-and-suspenders).
• It is the same choke point that owner-module flows will call once staged uploads land (follow-up spec): a deal route authorizes with its own RBAC (customers.deals.edit + ownership), then calls attachmentService.attach(...), which runs the same guard dispatch. Redundant for owner flows, load-bearing for the generic endpoint, generic UI, and user-defined custom entities (which have no module to host an attach endpoint).

3. Amendment: write the guard convention into the spec

This will be OM's first cross-module guard usage (existing guards are infrastructure — optimistic locking — or a module guarding its own entity), so this spec sets the precedent everyone copies. Please state:

• Guards are module-scoped: each module ships policy for its own entities in its own data/guards.ts (customers guards customers:deal, auth guards auth:user). No app-wide targetEntity: '*' switchboards for domain policy — '*' is reserved for infrastructure concerns (locking).
• The blessed primary path for domain flows, once staged uploads exist, is the owning module's mutation calling attachmentService.attach; the generic endpoint + guards is the safety net for generic UI and custom entities. (Replace/annotate the current app-wide example guard in Architecture accordingly — as written it both exhibits the collision above and models the switchboard anti-pattern.)

Alternatives that were weighed and lost (for the spec's "alternatives considered")

• Owner-module attach endpoints only, no guards — right as the primary path, insufficient alone: user-defined custom entities have no module for an endpoint, the shared attachment UI is generic over all entities, and the frozen generic endpoint stays alive either unguarded (the Vikunja IDOR class) or admin-locked (the under-grant returns).
• A dedicated AttachmentTargetPolicy interface — collision-free typing, but duplicates ~90% of the registry, adds a new frozen contract surface, and contradicts the existing mandate to wire custom write routes through the guard contract.
• Central policy engine / ReBAC — wrong weight class here.

[jtomaszewski]

@bartek5412 feedback given.

Most important points from me:

• long-term ideal design for attachments would be two-phase (first upload, later attach) - https://github.com/fullstackhouse/open-mercato/blob/jtomaszewski/pr-117-ddd-prior-art/.ai/specs/2026-09-01-staged-attachments-two-phase-upload.md - as seen e.g. in Stripe API, or other mature systems (e.g. I've seen that in Allegro)
• we don't have to the two-phase design yet (if all we need is only better authorization), but let's make sure that whatever we're doing now, won't need large refactor later, once two-phase upload is done
• if we have 100 modules using attachments, I dont want any single code bit to endlessly grow (e.g. a guard that contains logic for all 100 modules).