Skip to content

fix(platform): blob refs grant nothing; upload and import lanes gated - #3160

Merged
larryro merged 8 commits into
mainfrom
fix/upload-blob-authz
Sep 3, 2026
Merged

fix(platform): blob refs grant nothing; upload and import lanes gated#3160
larryro merged 8 commits into
mainfrom
fix/upload-blob-authz

Conversation

@larryro

@larryro larryro commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

The invariant

Holding or naming a blob reference grants nothing. buildObjectKey mints every org blob as <prefix>/<orgSlug>/<uuid>, so the org prefix on a key (s3KeyBelongsToOrg) proves tenancy, never ownership — and a document's s3: ref is served to every reader of that document. Authority must come from two places only:

  • Bind / consume: a server-recorded, single-use, purpose-scoped upload intent for the caller (app.upload_intents, migration 0067 — the session-lane twin of the REST door's rest_upload_intents).
  • Read: the ACL of the bound parent the row belongs to — uploader, document (every document sharing the blob), thread owner / project share, conversation assignment, or a readable task that lists the ref (domains/files/access.ts). Denial is 404-shaped.

Threat framing: any authenticated member of an organization. Every lane below could be driven with a ref obtained legitimately (a document preview, a shared thread, a task deliverable) or guessed from a leaked URL.

Findings

1. Blob ref possession grants read and destroy — fixed

registerUpload inserted a row for any org-prefixed key; GET /files/:ref/url, /urls, /statuses, GET /files/:id and the transcription skip/retry verbs answered any row of the org. A member holding a document's ref could register it as their upload, then DELETE /files/:id passed the uploader gate and deleted the shared blob.

  • Every browser-minted key (/files/upload, /blob-upload, /upload-handoff) is recorded as the caller's intent; /files/register consumes it (UPLOAD_NOT_OWNED 403) and refuses a second row per blob (BLOB_ALREADY_REGISTERED 409). The REST door keeps rest_upload_intents and declares { kind: 'external' } at the call site — every registerUpload caller must now say who vouches.
  • All read doors resolve the bound parent (access.ts); the document probe walks every document with file_ref = ref because a multi-team upload is one blob and one document per team (the row's single document_id names only the last).
  • deleteFile refuses document-bound rows (the documents domain cascades) and keeps bytes another row or document still references; /files/reject-blob consumes the caller's own intent; the document bind lane (/documents/from-blob-upload) proves ownership without consuming.
  • Tests: domains/files/access.test.ts (8 — the decision core with injected probes: deny-by-default, uploader short-circuit, document ACL exclusive, thread/conversation/task grants, fail-closed on probe error) + checkBlobRefAuthority (integration; see below).

2. Skill upload deletes any org blob passed as storageId — fixed (incl. the automations twin)

Both bundle lanes trusted s3KeyBelongsToOrg and deleted the caller-named key on every path. The zip is now staged via POST /files/upload?purpose=skill_bundle|automation_bundle; the lane consumes that purpose-scoped intent before reading a byte; the automation host's readStagedZip/cleanupStagedZip act only on the key verifyStagedZip admitted (the impl runs cleanup in finally, refusal included). A registered file, a foreign ref, or a zip staged for the other lane is STORAGE_NOT_OWNED and untouched. rg s3KeyBelongsToOrg: the remaining callers (files/service.ts requireOrgScopedKey, sandbox-blob-routes.ts behind an HMAC stage token, documents/replacement.ts behind its own intent table, core/lib/storage/blob_access.ts requireS3) use it as the tenancy check it is, behind another authority — no other caller trusts it alone.

3. Zip entries fully decompressed before size caps — fixed

parseSkillBundleZip now gates on the central directory's declared uncompressed size per entry and in total before any inflation (JSZip keeps it on the loaded entry's CompressedObject), and every entry inflates through a size-limited sink (nodeStream + pause/destroy at the cap) so a lying header is cut off rather than trusted. Tests (bundle_zip.test.ts, +5): declared size read; a 4 MB+1 zero bomb and an over-total bundle refused with JSZip's inflate seam (internalStream) never called (spy); a hand-patched central directory declaring 10 bytes for a 4.5 MB entry refused FILE_TOO_LARGE by the sink; a bundle exactly at the caps still accepted.

4. Import endpoints skip the knowledgeWrite gate — fixed

requireOrgAbility('write', 'knowledgeWrite') (new in auth/org.ts, the server twin of the UI's ability.can) is mounted on both /api/app/onedrive and /api/app/google-drive routers — the whole surface exists to write Knowledge, matching the cloud-import OAuth start and the UI. A member gets 403 on import / list / cancel of both providers; an editor passes the gate (cancel of an unknown config → 404).

Same class, found while verifying (fixed in the same PR)

  • Chat attachments (filterStorageIdsInOrgfilterStorageIdsReadable): the turn accepted any org row as an attachment (the model reads it out to the sender) and bindStorageIdsToThread then bound every unbound row — a document's included — into the sender's thread. The gate now runs the files read resolver for the sender; the bind claims only the sender's own unbound, non-document rows.
  • Outbound mail attachments (/conversations/:id/reply, /compose): client-named refs were registered and mailed out of the org. Each must now be the sender's own upload (attachment_not_owned 403).

Verification

  • bunx vitest --run --project server: 367 files / 5074 tests passed on the branch (access.test.ts 8 + bundle_zip.test.ts 14 included).
  • bunx tsc --noEmit (platform): clean. bunx oxlint --type-aware on touched files: clean. bun run lint:sast (opengrep, 467 rules / 3397 files): 0 findings.
  • backend:integration on throwaway tale-db + MinIO — branch: 289/290 (the one red is the object-store seed probe's bucket delete failing on a MinIO container reused from the previous run — a non-empty bucket, not code; the run before it, on fresh containers, passed that probe and showed 288/290 where its two reds were harness probes seeding file_metadata rows by raw SQL without uploaded_by, fixed in the last commit). All 11 new class probes PASS, and the pre-existing skill/automation upload probes PASS with the purpose param.
  • base (main + only this PR's test files, fresh containers): 280/290 — the 10 base reds are exactly the new class probes, and they show the damage, not just a status code: skill upload blob survived=false, automation upload blob survived=false, document ACL stranger reads team=LEAKED, chat bind stranger-thread:DOCUMENT (a document row hijacked into a stranger's thread), reclaim deleted=true for another member's staged blob, register rows=3 (duplicate rows for one blob), import doors 200/200/404 for a read-only member. The one class probe green on base is the direct-delete gate, which existed — the attack there was through the duplicate registration.
  • New unit tests on base: access.test.ts cannot load (module absent); bundle_zip.test.ts 4 of 5 new tests red (declared-size helper absent; both pre-inflation spies see 2 and 9 inflate calls; the lying header is inflated whole and rejected by JSZip as a corrupted archive instead of refused at the cap). The "bundle exactly at the caps still accepted" guard is green on both.
  • New integration probe checkBlobRefAuthority (11 checks): register IDOR + single-use; every read door refuses a bare ref; delete stays with the uploader; skill upload refuses foreign/registered/unpurposed refs and leaves the blob, installs its own once; automation upload refuses an unstaged ref and leaves the blob; the document bind lane needs the uploader; reclaim consumes only the minter's intent; a document blob follows the document ACL (team-scoped hidden, org-wide served); chat attaches only readable refs and binds only own staging rows; outbound mail attaches only the sender's uploads; cloud-import doors 403 for member, pass for editor.

Migration

0067_upload_intents.sql — new table + unique index on s3_ref, forward-only, no backfill, rolling-safe (old code never touches it). Two harness probes that seeded file_metadata rows by raw SQL now seed them the way a real upload looks (uploaded_by).

Not changed / noted

  • tasks.attachments has no 0.5 server write path (routes strip it; the UI's task-attachment zone silently drops); only the trusted run harvest writes tasks.outputs, so the task binding in the read gate is trustworthy today — a future attachment writer must validate readability (comment in access.ts).
  • Product image upload (use-product-image-upload.ts) POSTs bytes and asks getFileUrl for a never-registered ref — it already 404s on 0.5 and stores a 15-minute presigned URL as a "stable" imageUrl; pre-existing, out of class.
  • The branding-SVG stored-XSS finding is fixed on fix(platform): harden untrusted content serving and email HTML escaping #3133 and untouched here.

Holding an s3: blob ref grants nothing. `buildObjectKey` mints every org
blob as `<prefix>/<orgSlug>/<uuid>`, so `s3KeyBelongsToOrg` proves
tenancy, never ownership — yet `registerUpload` inserted a row for any
org-prefixed key, and the serve/status/transcription verbs answered any
row of the org. A member holding a document's ref (served to every reader)
could register it as "their" upload and delete the shared blob through
the uploader gate, or presign, read and steer rows they had no access to.

- `app.upload_intents` (0067): every browser-minted key (`/files/upload`,
  `/blob-upload`, `/upload-handoff`) is recorded as the caller's
  single-use, purpose-scoped intent; `/files/register` consumes it and
  refuses a second row per blob. The REST door keeps its own
  `rest_upload_intents` and says so at the call site.
- `files/access.ts`: reads resolve the row's bound parent — uploader,
  document ACL (every document sharing the blob; a multi-team upload is
  one blob, one document per team), thread owner / project share,
  conversation assignment, or a readable task listing the ref. Denial is
  404-shaped. `/files/:id`, `/:id/url`, `/urls`, `/statuses` and the
  transcription skip/retry verbs ride it.
- `deleteFile` refuses document-bound rows (the documents domain
  cascades) and keeps bytes another row or document still references;
  `/files/reject-blob` consumes the caller's own intent.
- The document bind lane proves ownership without consuming (one blob
  legitimately becomes several documents).
Both bundle lanes trusted `s3KeyBelongsToOrg` as ownership and deleted the
caller-named key on every path (parse failure, needs_confirm, forbidden,
success). Every document blob in the org carries the same prefix, so any
member could pass another document's storageId and have the lane destroy
its bytes.

The zip is now staged through `POST /files/upload?purpose=skill_bundle`
(or `automation_bundle`) and the lane consumes that purpose-scoped intent
before reading a byte; the automation host's read/cleanup act only on the
key the consume admitted (the impl runs cleanup in `finally`, refusal
included). A registered file, a foreign ref, or a zip staged for the other
lane is refused as STORAGE_NOT_OWNED and left untouched.
`parseSkillBundleZip` checked only the 32 MB compressed upload before
`entry.async()` materialized each entry; the 4 MB per-file cap ran after
decompression. DEFLATE runs past 1000:1 on repetitive input, so one
authenticated member could OOM the shared backend with a small zip.

The central directory's declared uncompressed size now gates each entry
and the running total before any inflation, and every entry inflates
through a size-limited sink that stops the stream at the cap — so a header
that lies about its size is cut off instead of trusted. Tests prove the
refusal happens without touching JSZip's inflate seam, and that a patched
header still cannot get past the sink.
`/api/app/onedrive/*` and `/api/app/google-drive/*` required only org
membership, while the cloud-import OAuth start and the UI gate the import
on knowledgeWrite. A read-only member holding a usable Graph token (the
login-account lane) could import documents and cancel other users' syncs
through the API.

`requireOrgAbility(action, subject)` is the server twin of the UI's
`ability.can` for a router whose whole surface sits behind one
capability; both import routers mount it.
Two more doors trusted a client-named blob ref: the chat turn accepted any
org row as an attachment (the model reads it out to the sender) and then
bound every unbound row — a document's included — into the sender's
thread; outbound mail attached and sent any org blob.

`filterStorageIdsReadable` runs the files read gate for the sender, the
thread bind claims only the sender's own unbound, non-document rows, and
the reply/compose doors require each attachment to be the sender's upload.
`checkBlobRefAuthority`: a second, read-only member drives every lane that
used to trust the ref alone — register, serve, statuses, transcription
verbs, the skill and automation bundle uploads, the document bind lane,
the reclaim lane, the chat attachment gate + thread bind, the outbound
mail attachment — and both cloud-import routers; the owner's own flows,
a document blob following its document's ACL, and an editor passing the
import gate prove the gate is about authority, not a blanket refusal.

The skill/automation upload probes stage with the lane's purpose, and the
two probes that seed file rows by hand now seed them the way a registered
upload looks (`uploaded_by`).
# Conflicts:
#	services/platform/backend/domains/documents/service.ts
#	services/platform/backend/domains/files/service.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant