Skip to content

fix(service-storage): a member attaches a file from the browser again — the upload's own completion attaches it as them, after #31031 - #31072

Merged
objectstack-fleet[bot] merged 24 commits into
mainfrom
claude/sec-hardening-r17
Nov 7, 2026
Merged

objectstack-fleet[bot] merged 24 commits into
mainfrom
claude/sec-hardening-r17

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

security#1 · os-tesla · session_01L9qibvynRbCtMndW8EHNGc

Clause-②: yes (widening)
Metadata-protocol: added
Skills-docs: updated here — content/docs/permissions/attachments-access.mdx and content/docs/api/plugin-endpoints.mdx state the attachTo key of the upload-complete request and its refusals; no file under skills/** teaches the upload-complete request (a git grep of skills/ finds no upload/complete and no CompleteUploadRequest).

维护者速读

  • 改了什么:记录附件面板在"完成上传"的同一次调用里,以成员本人身份把文件挂到记录上(attachTo),不再走第二步通用数据接口。
  • 为什么改:fix(objectql)!: a non-system write may name, in a lookup or user field, only a record the caller can read #31031 之后,没有 sys_file 读权限的成员从浏览器挂附件,第二步 POST /data/sys_attachment 被拒(400 reference_not_found)。
  • 风险与回滚:担保只覆盖调用者本人上传、仍为 pending、尚未被任何记录持有的那一个文件,并且先过挂载授权;通用数据接口照旧拒绝;担保在本次写入后即失效,钩子拿不到。回滚 = revert 本 PR,面板回到被拒状态。
  • 先看:file-attachment-service.ts 的 attachUploadAs(担保前的各道检查)、storage-routes.ts 完成上传门的 attachTo 分支、engine-defaults-and-autonumbers.ts 的 buildHookApi(剥除担保)。
  • 两分钟验证:pnpm --filter @objectstack/dogfood exec vitest run test/attachments-browser-attach.dogfood.test.ts(先构建依赖闭包),预期 8 passed。
  • 需要你做的:无,席位落地。

What changes

The rule. The upload-complete door (POST /api/v1/storage/upload/complete) takes an optional attachTo: { parentObject, parentId }. With it, the door first runs every gate it already ran: the session, a malformed attachTo (400), an id that names no file (404), the upload ownership rule (403), the starting organization, and the held bytes (409). It then hands the upload to the file-attachments service's new attachUploadAs, running as the caller. The answer is the committed file plus its new attachmentId.

attachUploadAs vouches for one sys_file and nothing else. Every condition below must hold, in this order, before the vouch is spent:

  • The file is the caller's own upload. Another user's id and an id that names no file get the same answer.
  • It is pending.
  • It is in the attachments scope.
  • It is within the caller's write reach.
  • No record holds it yet (findFileHolder: no sys_attachment row and no ref_* column).
  • Its bytes are held.
  • The caller may attach to the parent: the sys_attachment create permission, the parent-edit attach rule, and enable.files.
  • The parent is in the file's organization.

The verb then writes the sys_attachment row (vouching for that file_id) and commits the file, as one unit. If the write fails, the upload stays pending.

One-shot vouch. The vouch is spent on the verb's own write. buildHookApi builds every hook's ctx.api, plus withRunAs('user') and sudo derived from it, from a context with the mark stripped (withoutVouchedReferences, @objectstack/core). A hook running under the vouched insert that names the same file is judged through the caller's read door.

Protocol. The spec's CompleteUploadRequestSchema gains the optional attachTo key, and FileUploadResponseSchema gains an optional attachmentId. So Metadata-protocol: is added, and @objectstack/spec is graded minor.

Unchanged.

  • No caller gains a read of sys_file.
  • POST /api/v1/data/sys_attachment naming a sys_file the caller cannot read is still refused reference_not_found.
  • A completion without attachTo commits exactly as before.
  • A mount with no file-attachments service refuses attachTo with 501.

Console.

  • The Attachments panel completes its upload with attachTo and makes no second call. It keeps its refusal messages.
  • The upload adapter gains attachTo and returns the attachmentId.

Why the complete door, not a new route.

  • pending is the proof that the file came from this flow and was never attached.
  • The commit and the attach are one unit, so no committed-but-unattached file is left behind.
  • The panel already calls this door, so its flow loses a step instead of gaining one.

Other paths checked.

  • The SDK's storage.upload has no attach step.
  • MCP has no attach tool.
  • No other console path creates sys_attachment.

None of them needed a change.

Docs.

  • content/docs/permissions/attachments-access.mdx
  • content/docs/api/plugin-endpoints.mdx
  • the service-storage README
  • the generated storage API reference

Recounted. The tenant audit census counts the verb's new sys_attachment insert (265 to 266 write call sites), and the strictness ledger counts the new attachTo object.

Tests

  • file-attachment-service.test.ts, 13 new attachUploadAs cases.
    • Green leg: a member without sys_file read attaches. The file is committed, uploaded_by is the member, and sys_file stays unreadable to them.
    • Refused:
      • another user's id and a nonexistent id (same answer),
      • a committed upload, an already held upload,
      • the user scope, missing bytes,
      • a sharing deny, an explain deny,
      • a parent without enable.files, an unreadable parent,
      • a system context.
    • After the verb, the member's own direct insert naming the same file is still refused reference_not_found.
  • storage-routes.complete-attach.test.ts (new, 15 cases).
    • The door's own gates run first, and a refused request never reaches the attacher: 401, 403, 404, 409, and 400 for five malformed attachTo shapes.
    • The attacher's declared refusals are relayed with their code. A validation refusal becomes VALIDATION_FAILED. An undeclared fault is a generic 500.
    • A mount with no attacher answers 501.
    • A completion without attachTo is unchanged.
  • engine-unreadable-reference-target.test.ts, 3 new cases. A hook writing through ctx.api, or through ctx.api.withRunAs('user'), under the vouched insert, naming the same unreadable target, is refused. The control: under a system write, the same hook's write lands.
  • attachments-browser-attach.dogfood.test.ts (new, 8 cases), at the HTTP level on the showcase boot, as a member holding showcase_contributor plus sys_attachment read/create and nothing on sys_file.
    • Green leg: presign, then PUT, then complete with attachTo gives 200. One committed file, one attachment in the member's name. The panel's list read and the download URL work, and sys_file/:id stays 403.
    • Controls:
      • the generic two-step is refused 400 reference_not_found;
      • another user's upload id is refused 403 and stays pending;
      • a nonexistent id gets 404;
      • another user's invoice is refused and the upload stays pending;
      • re-attaching gets 409;
      • a '*' reader works through both flows.
  • Console:
    • RecordAttachmentsPanel.test.tsx uses the real adapter with a stubbed fetch. The complete body carries attachTo, a refusal maps to friendly copy, and a missing attachmentId is an error.
    • The affordanceGrant-12047 control asserts the adapter gets attachTo and that dataSource.create is never called.
    • The write census in affordanceGrantMap-12082.test.tsx drops the panel's handleFiles :: create site, since the panel no longer calls create. Its attachmentUpload row is still read where the upload is offered, and its delete site is unchanged.

Acceptance notes

  • The order's controls are all pinned in the dogfood:
    • another user's id,
    • a nonexistent id,
    • no attach right on the parent,
    • the direct generic POST still refused,
    • the '*' reader working.
  • The vouch premise is pinned in the unit tests: pending, the caller's own upload, through this flow, not yet attached.
  • One-shot: pinned in objectql; red under the ablation below.
  • The door answers 404 for an id that names no file and 403 for another user's id. That distinction predates this change: the door's existing requireUploader path already makes it on completion without attachTo. The verb's own answer is identical for both.

Follow-up. The call-site census gate (check:vouch-for-reference-callers) has moved out of this PR. It pins the code that sets the vouched-reference mark to an allowlist: file-attachment-service.ts, 2 calls. Its parts:

  • the script and its root alias,
  • a core repo vitest pin,
  • the Test Core input wiring,
  • a lint.yml step.

It lands in its own PR after this one merges. That PR is Tier H, through the maintainer, because of the workflow step. This PR touches no governed surface.

Verification

Ablation: the vouch in attachUploadAs

The verb's insert context vouchForReference(unitContext as ExecutionContext, …) was replaced with a plain unitContext through scripts/ablation-replace.mjs. service-storage was rebuilt, and ablation-dist-preflight confirmed the marker is present in 2 built files.

  • Unit (-t attachUploadAs): 2 failed, 11 passed. Red: the member attach, and the case that first attaches through the verb and then tries a direct insert. Its setup attach now fails.
  • Dogfood: 2 failed, 6 passed. Red: the member end-to-end attach, and the re-attach control, whose first completion must succeed. Green: the generic door refusal, another user's id, the nonexistent id, another user's invoice, and the '*' reader through both flows.
  • Restore: blob == HEAD (24e738f31e20), source clean. Rebuilt, and preflight --absent confirmed the marker is absent from all 6 built files.
Ablation: the strip in buildHookApi

withoutVouchedReferences(...) was removed from buildHookApi. Result: engine-unreadable-reference-target.test.ts 2 failed, 122 passed. Red: the ctx.api and withRunAs('user') hook cases. Green: the system-write control. Restore: blob == HEAD.

Suites at head cb31937 (main merged at 5210acf), under the verify lock
  • pnpm install --frozen-lockfile && pnpm build: 119 of 119 tasks.
  • @objectstack/core: test 3143 passed; test:repo 94 passed.
  • @objectstack/objectql: engine-unreadable-reference-target.test.ts 124 passed. The whole package (11372 passed) was run before the census gate was split out; that commit does not touch objectql.
  • @objectstack/service-storage: test 1388 passed; typecheck clean.
  • Dogfood:
    • files: attachments-browser-attach, attachments-permission-matrix, print-service-archive.
    • result: 46 passed, 1 skipped.
    • the skip is the existing skipIf(!organizationsAvailable) block.
  • UI:
    • affordanceGrantMap-12082.test.tsx: 55 passed.
    • plugin-form, app-shell and data-objectstack whole packages: 23858 passed, 10 skipped.
    • turbo run type-check for @object-ui/app-shell and @object-ui/data-objectstack: 68 of 68 tasks.
  • @objectstack/spec check:generated: all 14 artifacts current.
  • The full dogfood suite was not run: the title has no ! and Clause-② carries no (narrowing) arm.
Gates
  • dispatch-gates --commands at cb31937 derives 117 commands, and all 117 were run and exited 0. --ran reconciles them: 117 derived, 117 run, 0 not measured, 0 unrun.
  • The three that need the verify lock ran under it, exactly as printed: pnpm --dir ui exec vitest run scripts/, spec check:generated and check:type-check-debt.
  • check:self-test-wired: green.
  • check-governed-merges.mjs --pr 31072: NOT governed. 0 of 26 paths hit the register, and git diff origin/main...HEAD -- .github/ is empty.
  • The changeset gates were run against the merge base 5210acf: check-changeset-no-major, check-adr-0087-registration (non-breaking) and check-empty-changeset, all green. check-changeset-no-major was also fed a pull_request payload carrying this body: its level axis passes, reading clause-② yes (widening), metadata-protocol added, and @objectstack/spec: minor.

https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc

claude added 21 commits November 6, 2026 21:37
…ys_file stays unreadable

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…he request names attachTo

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…ouches only for an unheld upload

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
… upload, and refuses every other file

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
… only after its own gates

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…nel's flow on the showcase boot

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…e from the browser

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…r guard; the tenant audit census counts the attach verb's insert

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
# Conflicts:
#	packages/core/src/security/index.ts
#	packages/core/src/security/vouched-reference.ts
#	packages/objectql/src/engine-unreadable-reference-target.test.ts
#	packages/services/service-storage/src/file-attachment-service.test.ts
#	packages/services/service-storage/src/file-attachment-service.ts
@github-actions github-actions Bot added size/xl ci/cd dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation tests tooling Fix lands in tooling, not a product package — enters pm:queue only with Unblocks: or a named surface labels Nov 7, 2026
The census script, its root alias, the core repo pin, the Test Core input
wiring and the lint workflow step move to a separate change. The attach on
upload completion, the one-shot vouch and their pins stay.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 4 package(s): @objectstack/core, @objectstack/objectql, @objectstack/service-storage, @objectstack/spec, touching 46 documentable anchor(s). ⚠️ 3 changed file(s) yielded no anchor (packages/core/src/security/index.ts, packages/services/service-storage/README.md, packages/spec/authorable-surface/api.json), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

57 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 3f61e4f8fb567160f839c3269ee95aa89ee4f1db.

⛔ 15 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 3 changed file(s) yielded no anchor (packages/core/src/security/index.ts, packages/services/service-storage/README.md, packages/spec/authorable-surface/api.json) — pages documenting those are invisible to this run
  • 1 anchor(s) matched too much of the corpus to be a work list: PERMISSION_DENIED (literal, 51 pages)
  • 3 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 63 of 224 client-bound route-ledger rows — the other 161 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 161: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 57 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 104 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 182 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 3f61e4f8fb567160f839c3269ee95aa89ee4f1db → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 35e85c0824c4e910e1e037261a62ca069be01426 — the merge of head cb31937dcc5e30cb340f9bb565236a87d0c5da1b into base 3f61e4f8fb567160f839c3269ee95aa89ee4f1db, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 35e85c0824c4e910e1e037261a62ca069be01426 && git checkout 35e85c0824c4e910e1e037261a62ca069be01426
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3f61e4f8fb567160f839c3269ee95aa89ee4f1db cb31937dcc5e30cb340f9bb565236a87d0c5da1b && git checkout -B drift-repro 3f61e4f8fb567160f839c3269ee95aa89ee4f1db && git merge --no-ff cb31937dcc5e30cb340f9bb565236a87d0c5da1b

node scripts/docs-audit/affected-docs.mjs --json 3f61e4f8fb567160f839c3269ee95aa89ee4f1db

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 3f61e4f8fb567160f839c3269ee95aa89ee4f1db → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…protocol line is added and spec takes minor

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
…— the upload's own completion attaches it

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-tesla · session_01L9qibvynRbCtMndW8EHNGc

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: cb31937dcc5e30cb340f9bb565236a87d0c5da1b
Local-runs: none
Scope: packages/spec/src/api/storage.zod.ts + packages/objectql/src/engine-referential-integrity.ts, packages/objectql/src/engine.ts, packages/services/service-storage/src/attachment-lifecycle.ts, packages/services/service-storage/src/upload-ownership.ts, packages/services/service-storage/src/metadata-store.ts, packages/client/src/index.ts, ui/apps/console/src/App.tsx

Read at 2026-11-07T03:35Z: the PR body as rewritten, its 26-file list, the three-dot diff against the merge base 5210acfbae (origin/main has since moved two commits, to 0f920b941f, touching none of the PR's 26 paths), the check-runs on this head, and the cited code on main. Nothing was built, run or re-run.

Check-runs on this head. All ten required contexts are success: Lint & Repo Gates, TypeScript Type Check, Test Core, Build Core, Governed Surface Queue Guard, Lint UI, Type Check UI, Test UI, Build & E2E UI, Live E2E UI. Check Changeset went red on its first run (2026-11-07T02:35:19Z) because the body then read Metadata-protocol: none while the diff moves packages/spec/authorable-surface/api.json; the re-run after the body edit (2026-11-07T03:26:45Z) is success, as is the re-run of the PR-body guards. Dogfood Regression Gate, Temporal Conformance, Dogfood Verify CLI and UI Published Artifacts are skipped on this PR run as the two-speed battery declares. Nothing is still running.

① Derived judgments

The accept set moves in two published places and one door. Each change, judged:

  1. CompleteUploadRequestSchema.attachTo — optional { parentObject, parentId }, both non-empty strings. Right. The door reads it through a closed set (attachTargetOf): an array, null, a non-object, an empty string or any key outside the two declared is refused 400 INVALID_REQUEST before the file row is read, so a malformed target learns nothing about any file. Pinned at the door for five malformed shapes with the attacher never called.

  2. FileUploadResponseSchema.data.attachmentId — optional, present only when the request carried attachTo. Right. A completion without attachTo answers exactly the prior shape through the same committedFileMetadata projection; pinned.

  3. The door's attach arm, and its order. The gates the door already asked run first and unchanged: session (401), fileId (400), target shape (400), the row (404), the uploader rule (403), the starting organization (409), the held bytes (409). Only then is the attacher asked, and only in a mount that bound one; the public registrar and the open mode (session === null) answer 501 NOT_IMPLEMENTED and commit nothing. Right: a request refused by any prior gate never reaches the verb (pinned for 401, 403, 404, 409 and the five 400s), and the no-attacher mount keeps the plain commit unchanged.

  4. The verb's premise, re-asked inside the verb. attachUploadAs reads the row as the system by id and asks, in order: isFileUploader (an empty owner, another owner and no row all take the uploader rule's one answer), status === 'pending', scope === 'attachments', organizationOutOfWriteReach against the caller's active organization, findFileHolder (a sys_attachment row naming the file, or the row's own ref_* columns), heldBytesRefusal. Right, and each is the existing definition, not a copy: the uploader rule is the commit door's, the holder question is the reap guard's, the bytes question is the commit door's own. So the set of files the vouch can ever name is: a row the caller's own presign created in the attachments scope, still pending, held by nothing, with bytes present, in the caller's write reach. A row the system attachFile created has no owner and is refused; a row attachFileAs created is committed and is refused; a user-scope upload is refused.

  5. The parent authorization, before any write. assertMayAttach asks the security service's explain for sys_attachment create and the parent-EDIT attach rule with the parent read as the caller; organizationOfParent reads the parent as the caller again and refuses a parent that does not exist, that the caller cannot read, or that is walled into another organization with one answer; then the file's organization must equal the organization the attachment is written in. Right. The insert itself runs as the caller, so the engine's own gates judge the caller too: enable.files (pinned FILES_DISABLED), the attach-rule hook, the uploaded_by stamp, RLS and FLS. The row's file_name, mime_type and size come from the sys_file row the verb read, never from the request.

  6. What the vouch admits. One (sys_attachment, file_id, file.id) triple, on the verb's own insert; the existence probe still runs. Right, and unchanged from the mark's published contract: vouchForReference and isVouchedReference keep their signatures, and the reference check's one consultation of the mark is untouched. The generic data door carries no mark; pinned twice — a unit case where the same member's direct insert naming the file the verb just committed is refused reference_not_found, and the dogfood's two-step control.

  7. The one-shot strip. withoutVouchedReferences runs where buildHookApi builds every hook's api, so ctx.api, withRunAs('user') and sudo all derive from a context without the mark. Right. Pinned in objectql both ways: a hook naming the vouched id again through either api is refused, and the same hook under an unmarked system write lands (so the refusal is the mark being spent, not the hook failing). The strip is own-property based; the mark is set only by spread, so there is no inherited-property shape for it to miss in the engine (no Object.create of a context anywhere in objectql's source).

  8. Error paths. The insert and the commit run in one inOneUnit: an owned transaction rolls both back; a unit the call does not own removes the inserted row best-effort and logs the row id when it cannot. The upload itself is never touched by a refusal: it stays pending, bytes stored, attached to nothing, and the pending-upload retention reclaims it. Right. The door relays a declared 4xx with its code, status, message and details, a declared 5xx with its code and status and generic text, and anything undeclared as a generic 500; pinned for all three.

  9. Disclosure. For a file that is not the caller's upload, the only answers the door ever gives are its pre-existing 404 FILE_NOT_FOUND and 403 PERMISSION_DENIED, both before any fact about the row; every 409 and 400 that describes a file's state is reached only by its uploader. The verb's own answer for another user's row and for no row is one body (pinned byte-equal). Right; see ③ for the door's two codes.

  10. Console. The panel sends attachTo with the completion and makes no second call; a 200 without an attachmentId is an error, never a silent success; the adapter puts the server's code and status on the thrown error so the panel maps refusals without parsing text; uploaded_by is no longer sent (the server stamps it). Right. The write census drops the panel's create site and keeps its delete site; the attachmentUpload affordance row is still read where Upload is offered.

  11. Counts. The tenant audit census moves 265 to 266 for the verb's new insert; the strictness ledger moves api 500 to 501 for the new attachTo object; the authorable-surface shard gains api/CompleteUploadRequest:attachTo; the generated storage reference carries the new key, its nested shape and the response field. Mechanical and consistent with the diff.

Two things the rule leaves open, neither a widening of any caller's rights:

  • (should-fix) The commit is not conditional on the state the verb read. The pending and unheld checks are reads, and the commit writes status: 'committed' by id without a where status = 'pending' arm. Two completions of the same upload racing through the arm can each write a sys_attachment row for the caller's own file on parents the caller may attach to. Every row so written is one the caller is entitled to create, and the surface already admits one file on more than one record, so the accept set does not grow; but the invariant the header states ("held by nothing") is held by a read. The fix is one condition on the commit (or refusing when updateFile reports the state moved), answered 409 RESOURCE_CONFLICT.

  • (should-fix) The mark still rides ctx.input.options.context. The engine builds each insert hook's input.options from the operation's options, whose context is the vouched one, so a platform hook that forwards ctx.input.options into a nested write carries the mark. It is admitted only for the same triple, which the verb has already judged, and no platform hook on sys_attachment forwards its options today (the lifecycle hooks write sys_file under their own system context). But the module header now says "no hook inherits it", which is wider than the code: either strip at the hook's options view too, or narrow the sentence to the api. The dev flagged this one.

② Semver level

  • Clause-②: yes (widening) — right. One optional request key and one optional response field enter; nothing is removed, refused or re-meant.
  • Metadata-protocol: added — right. The authorable-surface shard moves by one added key, which is what the gate reads; the first Check Changeset run was red on none, and the body and changeset now both read added, re-run green.
  • Levels: @objectstack/spec minor; core, objectql, service-storage, @object-ui/data-objectstack, @object-ui/app-shell patch — right under the kind-decides-the-level rule. No BREAKING banner is owed and none is written; no ADR-0087 disposition is owed.
  • Skills-docs: updated here — right. skills/ and .claude/skills/ on main carry no upload/complete, CompleteUploadRequest or attachTo; the two hand-written docs state the key and its refusals; the route ledger and the service README say what the door now does; the generated reference is regenerated.

③ Boundary flags

  1. The door answers 404 for an id that names no file and 403 for another user's id. Pre-existing: the commit door's getFile 404 has preceded the uploader rule since that rule landed, on completion with or without attachTo, and the two chunked doors do the same. The verb's own answer is one body for both. Ids are server-minted random UUIDs. Judgment: follow-up — a card on the three upload doors to give the uploader rule's one body for a missing row too, as the rule's own text already says; not this PR's to change, and the doc table here states both codes honestly.
  2. A platform hook forwarding ctx.input.options into a nested write carries the vouched context. Judgment: should-fix (①, second open item). Admitted only for the triple the verb judged; the fix is a strip at the options view or a narrower header sentence.
  3. Concurrent completions of one upload through the arm. Judgment: should-fix (①, first open item). No rights gained; the state invariant should be a write condition.
  4. Version skew between console and server. A panel carrying attachTo against a server without the arm gets a 200 with no attachmentId; the panel reports it as an error, and the server has committed an unattached file. Judgment: follow-up note only — the console and the server ship from one tree, and the panel's answer is loud.

④ Design judgment

The seam. Putting the attach on the complete door is the right seam, and the body's three reasons hold against the code: pending plus the uploader rule is exactly the proof the vouch rests on, and a separate attach route after a commit would have to vouch for a committed file a member cannot read, a wider premise; the commit and the row are one unit, so no committed-but-unattached file is left for a refused attach; and the panel loses a round trip rather than gaining a route. Route ownership holds: one route, one owner, the attacher a parameter of registerStorageFamily bound only by composeStorageRoutes, so no host hands the door an attacher of its own, and the public registrar refuses loudly (501) rather than serving a bare miss. The one tension is that the arm resolves the request's full execution context a second time after requireUploadSession has already resolved a session; the verb needs the full context (RLS, sharing, grants) and the door's session gate predates it, so both are correct, and a single resolution is a tidy-up for later, not a defect.

The mark. The PR reuses the one-file vouch without widening it: no new signature, no new field on the context, one new call site vouching the same triple on the same kind of insert (the caller's own sys_attachment row for a file the same call authorized), and the existence probe untouched. withoutVouchedReferences narrows the mark's reach rather than widens it, and the module header is rewritten to say so; the census gate that pins the mark's call sites moved to its own Tier H PR because of its workflow step, which is the right split — this PR touches no governed path (0 of 26 against the register).

Consumer hunt — every caller of the complete door and every reader of its answer, on main:

  • packages/client/src/index.ts storage.upload — sends { fileId, eTag }, never attachTo, and reads FileUploadResponse; the optional field is absent for it. Unchanged.
  • CLI — no caller of the door. MCP — no attach tool and no caller. Unchanged.
  • Console: ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx is now the one sender of attachTo; ui/apps/console/src/App.tsx builds the same adapter for file fields without attachTo, so its completion body stays { fileId } and the response body is not read. The adapter's thrown error gains status and code (additive; the message text is unchanged). The currentUserId prop stays on the panel's interface, so a host passing it still compiles.
  • Tests on main that call the door without attachTo: dogfood anonymous-door-sweep, attachments-permission-matrix, cbp-parent-attachment-comment-gates, field-file-collection, field-zoo-roundtrip, file-field-hydration-no-denial, fixtures/attachments-authz-harness, flow-screen-file-intake, predicate-write-unreadable-not-matched, storage-unclaimed-download, write-door-unreadable-is-not-found; organizations storage-upload-door-ownership.wall and storage-upload-organization-change.wall. Their request and answer shapes are unchanged.
  • Readers of an attachmentId: the print service answers its own { fileId, attachmentId } from attachFileAs, a different surface; unchanged.
  • Docs naming the door: content/docs/api/plugin-endpoints.mdx, the service README and the route ledger are updated in the diff; content/docs/references/api/storage.mdx is regenerated.

Nothing breaks; the one behaviour change for an existing caller is the panel's, which is the fix.

Implemented-by: claude/sec-hardening-r17
Reviewed-by: session_01L9qibvynRbCtMndW8EHNGc

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-tesla · session_01L9qibvynRbCtMndW8EHNGc

席位核对

ACCEPT · 2026-11-07T03:36Z · head cb31937dcc5e30cb340f9bb565236a87d0c5da1b

  • Contract review: PASS at this head, served at CONTRACT_REVIEW_TIER (above the default for its face, as a security seam), record 6701728749. No must-fix. Should-fixes go into this seat's follow-up list: the upload's commit made conditional on the state it was judged in (refused 409 otherwise), and the vouched mark stripped from the hook's options view as well, or the module's sentence narrowed to the api. Follow-ups: one answer for a missing row on the three upload doors, as the uploader rule already says, and a note on console/server version skew. None changes what lands here.
  • What lands: the maintainer's choice A (2026-11-07, this seat's thread). The upload-complete door takes an optional attachTo and, after its own gates, attaches the caller's own pending, unheld upload as the caller, in the same call, through attachUploadAs, which reuses fix(service-storage): the attach verb vouches for the one file it created, so members attach files again after #31031 #31061's one-file vouch. The Attachments panel no longer makes a second call. Every other door is unchanged: no caller gains a read of sys_file, and the generic insert naming an unreadable file is still refused. The vouch is spent on the verb's own write and stripped from every hook api.
  • Consumer hunt (record ④): the SDK, CLI, MCP and the console's file-field adapter call the door without attachTo and keep their request and response shapes; every in-tree test calling the door does the same. The panel is the only sender of attachTo.
  • Checks: every check-run on this head is success or skipped by its own conditions; Check Changeset went green on its re-run after the body was corrected to Metadata-protocol: added. git merge-tree against current main is clean.
  • Gates (dev, at this head): dispatch-gates --commands 117 derived / 117 run / 0 NOT-MEASURED; core, objectql, service-storage and the touched ui packages green; the three dogfood files 46 passed; ablations of the vouch and of the hook strip red as predicted and restored.
  • Shape: base main, no closing keyword. @objectstack/spec minor, the other five packages patch, with Clause-②: yes (widening), Metadata-protocol: added and Skills-docs: updated here. The vouched-reference call-site census gate moved to its own later PR, which will touch a workflow file and go to the maintainer.
  • Governed: check-governed-merges.mjs --pr 31072: not governed; 1568 changed lines ≤ 3000 ⇒ Tier S.
  • Landing authority: the maintainer's standing approval for this seat's security PRs at Tier S (2026-11-06 card answer in the seat's thread) and 「你应该是使用 pm-dispatch 技能自主审核和落地 pr」.
  • Next: relay pr_ready + automerge_enable; on merge, tell domain:server#2 on pm:mailbox — security#1 · os-tesla #30818 that the panel attach is back.

Generated by Claude Code

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review November 7, 2026 03:38
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge November 7, 2026 03:38
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Nov 7, 2026
Merged via the queue into main with commit d6b88f1 Nov 7, 2026
69 of 70 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/sec-hardening-r17 branch November 7, 2026 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/xl tests tooling Fix lands in tooling, not a product package — enters pm:queue only with Unblocks: or a named surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants