Uh oh!
There was an error while loading. Please reload this page.
fix(runtime): tell an action handler when its caller-scope record load was refused - #14247
Conversation
…d was refused Add `ctx.recordLoadDenied` — true exactly when a caller-scope subject-record load was attempted and did not deliver the row, absent otherwise. Both action doors (REST /actions and the MCP run_action bridge) now share one producer, `loadActionSubjectRecord`, and the flag is marshalled explicitly into the body sandbox so an inline body reads it as a registered handler does. The `recordId` stamp is deliberately kept: record-less / new-record actions depend on it, which is why the stamp condition and the load-failure condition coincided and `if (!ctx.record?.id)` never refused anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ea206df91049f4659826d26335bbe91463109f9c && git checkout ea206df91049f4659826d26335bbe91463109f9c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b360cc7d5ca2e9cdb60a12018af20cbc3470cafa 603d12366c73f04ba6b38174be6e9a8b18291786 && git checkout -B drift-repro b360cc7d5ca2e9cdb60a12018af20cbc3470cafa && git merge --no-ff 603d12366c73f04ba6b38174be6e9a8b18291786
node scripts/docs-audit/affected-docs.mjs --json b360cc7d5ca2e9cdb60a12018af20cbc3470cafa
|
Uh oh!
There was an error while loading. Please reload this page.
…ve published surface The diff widens the published surface additively: `@objectstack/metadata`'s entry gains a named type (`MetadataKeyedItem`) and `MetadataLoader` gains an optional member (`loadManyKeyed?`). This repo's precedent for additive public-surface widening is `minor`, not `patch` (R12: #14262's `job-handler-data-reach.md` and #14247, both `"@objectstack/runtime": minor`). Front matter only; the changeset body is byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68
Closes#14143
An action handler could not tell "the caller cannot read this row" from "this action legitimately has no record". This PR gives it a signal that can, additively, and writes it into the action-authoring docs — half the defect was that none of it was written down.
⛔ Not an exploitation claim. No exploitability work was done and none is claimed. This is a predicate defect: the platform's most natural authorization predicate for an action handler was structurally always-true. The
isSystem: trueelevation is settled design (#3914) and is untouched.The mechanism, re-derived on the current tree
Anchors were taken by triage on
20b79bea; re-derived here on66ecc50aand they have not drifted —packages/runtime/src/action-execution.tsstill carries:1117(the elevation),:1282/:1283(the caller-scope load),:1284(the swallow),:1285(the comment) and:1288(the stamp) at those exact lines.A refused or empty load leaves
recordas{}, sorecord.idis exactlynull— which is the stamp's own condition. The stamp condition and the load-failure condition coincide. The body then runs elevated, so authorization has to be re-established inside the handler, and:⛔ The stamp is not deleted. Record-less / new-record actions legitimately depend on it, and that dependency is pinned in this PR in both directions.
Measured first: can the caught error already tell refusal from absence?
No — and the collapse is deliberate, not an oversight. Traced end to end:
callData('get', ...)prefersprotocol.getData, which callsengine.findOneunder the caller's context and, when nothing comes back, throwsrecordNotFoundError(packages/metadata-protocol/src/protocol.ts);packages/core/src/utils/record-not-found.ts:code: 'RECORD_NOT_FOUND',status: 404);:1284as the same error object shape. The call site's own comment already says it: "engages the same permission path as get_record — an unseen record reads as not-found". That is existence non-disclosure working as designed.⇒ The repair is not smaller than the card assumed, and it deliberately is not an inspection of the caught error: there is nothing there to inspect. It is a separate channel. Consequences for the design, both taken:
What was built — direction 1 (additive), as ruled
ctx.recordLoadDenied—trueexactly when a caller-scope load was attempted and did not deliver the row; absent, neverfalse, otherwise. Same absence semantics as thereferentialFieldClearmarker already on this seam, so a handler readsctx.recordLoadDenied === true.Nothing that reaches a handler today stops reaching it. No existing key changes value. Direction 2 (pre-dispatch refusal) was not taken and no published accept set is narrowed.
Files, and why each one is required rather than scope creep
packages/runtime/src/action-execution.tsloadActionSubjectRecord— one producer for the load and its verdict — plus the signal on the MCPrun_actioncontext.packages/runtime/src/domains/actions.ts/actionsdoor carries the identical defect, byte-for-byte:git grepfor the stamp returns exactly two hits, this one and the above. A documented guard that only one of two doors sets is an authorization guard silently inert on the other — the same defect one door over. Converted to call the same producer; no other behaviour changed.packages/runtime/src/sandbox/script-runner.ts,body-runner.ts,quickjs-runner.tsctxis a fixed key set — a key the dispatcher sets but the sandbox never marshals reads asundefinedinside every inlinebody. Without these three edits the documented guard would be false on the surface an AI author writes most. Declared onScriptContext, projected inbuildActionSandboxContext, installed true-only ininstallCtx— the exact shapereferentialFieldClearalready uses two lines above.content/docs/ui/actions.mdx,content/docs/automation/hook-bodies.mdxctxparagraph in the hook-bodies reference.packages/runtime/src/action-record-load-denied.test.ts,.changeset/action-record-load-denied-signal.mdpackages/specis not in the diff — the Clause ② path limb does not fire (the engine contract typesexecuteAction's ctx asany, and noActionContextschema exists in spec).Verification
Everything below was run on
603d1236, this branch's head — the tree was clean and unchanged from that commit for the whole measurement.Whole affected package, green:
Tests —
packages/runtime/src/action-record-load-denied.test.ts, 13 cases. The engine double is row-scoped on the one point that matters:findhonoursoptions.context.userId, so the row is returned to its owner and is invisible to anyone else — which is how row-level security actually manifests tocallData('get', ...). The MCP leg is wired to the realcallData, so the realrecordNotFoundErroris what the dispatcher catches; nothing about the refused/absent collapse is mocked away.Both directions pinned, on both doors:
recordLoadDenied === true— andctx.record.idis still there, which is the prohibited regression, asserted rather than assumed;'recordLoadDenied' in ctx === false);recordIdattempts no load and still gets the stamp;recordId) attempts no load, gets no flag, and no subject-row read is issued at all.Every absence assertion has a firing positive control on the same rig: the identical expectation shape reports
truefor the unauthorized caller.Reverse verification — three ablations, each on the committed tree, each proving the mutation landed on disk by occurrence counts (not by an editor's exit code) and each restoring under a trap whose success is proven by comparing
git hash-objectagainst the HEAD blob:actionRecordLoadSignalreturns{}(kills the signal at both doors)recordLoadDeniedprojection removed frombuildActionSandboxContextvm.setProp(ctxObj, 'recordLoadDenied', ...)removed frominstallCtxNo rebuild is needed for these: the test imports its subjects by relative specifier (
./action-execution.js,./sandbox/body-runner.js), which vite resolves topackages/runtime/src/*.ts— the package'sdistis never on the path, andpackages/runtime/vitest.config.tsdeclares no alias that touches a relative import.Typecheck —
pnpm --filter @objectstack/runtime typecheckgreen, and measured rather than assumed:tsc --listFilesreports 5 of 5 edited source files inside that program. That config excludes**/*.test.ts, so the new test file was type-checked separately against the same compiler options with the exclusion lifted — also green.Gates run locally (the farm runs in CI; these are the ones this diff implicates):
check:nul-bytes·check:doc-anchors(295 fragment links resolve, including the two added here) ·check:docs-single-h1·check-system-context-census(109 elevation read sites, all anchored — this gate reads both edited dispatcher files by exact path) ·check:route-envelope·check:doc-authoring·check:cross-package-test-inputs·check:test-source-alias·check-doc-frontmatter·check-doc-route-spelling·check-docs-section-name·check-keyed-text-bounds·check-comment-mask-adoption·check-undeclared-dep-imports·check:type-source-resolution·check:objectql-double-limit·check:where-matcher·check-empty-changeset·check-adr-0087-registration·check-changeset-no-major·check:changeset-gate-self-tests·check:objectui-changeset·check:docs-audit-scope·check:corpus-claim-drift·check:doc-security-posture·check:role-word·docs-audit/check-affected-docs·pm/check-half-states·check:published-files·check-ci-filter-parity— all green.check-test-completenessexits 3 (PREREQUISITE NOT MET) with no argument: it grades a savedturbo run testlog that only CI produces. Recorded as NOT MEASURED, per that script's own instruction.Stop conditions checked
privateeven once checkAuthoredRowWrite admits it #7401's site? No. That card is the write-by-id pre-image door inpackages/metadata-protocol; this diff does not touch that package. Same pattern (a row resolved in the caller's scope), opposite direction, different site.action-execution.ts? No. Checked at dispatch and again before pushing: none of the 9 open PRs touchesaction-execution.ts,domains/actions.ts,sandbox/, orcontent/docs/ui/actions.mdx.packages/specin the diff? No.Out of scope, filed rather than fixed
The flow branch of the same two doors hands the same stamped
recordstub toAutomationContext, and the flow face has no counterpart signal. It is a narrower and conditional claim (a flow action is not system-elevated by default — the flow engine honoursrunAs, ADR-0049), and ruling it is a design question this card does not own. Filed unassigned as #14244.Generated by Claude Code