Repository navigation
fix(mcp): run_action and resume_run withhold a PERMISSION_DENIED's details, as the data tools do (#23529) - #23544
Conversation
…efusal's details over HTTP Drives the MCP HTTP face through the real Streamable HTTP transport, with a bridge double that refuses both action tools with the security gate's PermissionDeniedError shape. Asserts the tool error carries PERMISSION_DENIED and 403 and that no position or permission-set name appears anywhere in the HTTP response body. A control keeps the confirmation refusal's details served on both tools. Red on its parent: errorResultFromThrown spreads the refusal's details into the tool result. Claude-Session: https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm Co-authored-by: Claude <noreply@anthropic.com>
…tails, as the data tools do errorResultFromThrown now answers a refusal whose declared code is the catalogue's PERMISSION_DENIED through errorResultWithCode, the function the data tools already render through, so the security gate's diagnostics stay off the wire on the two action tools as they do on REST. Every other coded refusal keeps its details, so the confirmation refusal's retry contract and resume_run's REST-built details are unchanged. Claude-Session: https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm Co-authored-by: Claude <noreply@anthropic.com>
…sion refusal's details Claude-Session: https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift Check1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 12 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 1039243c64b5236b485883dfe52c722270e49d57 && git checkout 1039243c64b5236b485883dfe52c722270e49d57
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5fd2c76d6ce7ee8cd3267b3e026a1d48c400a73d 03c6a098a3993d02d9a8148520373430650de9d1 && git checkout -B drift-repro 5fd2c76d6ce7ee8cd3267b3e026a1d48c400a73d && git merge --no-ff 03c6a098a3993d02d9a8148520373430650de9d1
node scripts/docs-audit/affected-docs.mjs --json 5fd2c76d6ce7ee8cd3267b3e026a1d48c400a73d |
Contract reviewServed-tier: Inputs read: card #23529 body; the reading ① Derived judgments
② Semver level
③ VerdictPASS. No blocking item. Acceptance notes, none blocking: (a) the residual fail-open (a future coded refusal carrying principal data would still render its |
席位核对
Verdict: ACCEPT. Not governed (
|
Fixes #23529
Clause-②: no
run_actionandresume_runnow withhold aPERMISSION_DENIEDrefusal'sdetails, as the data tools, the record resource, the runtime dispatcher and the REST actions door already do (the ruling on #7450). Every other coded refusal on the two tools keeps itsdetails, so #15942's confirmation contract is unchanged.Dispatched by the PM loop,
domain:specseat 4, building round after triage's regrade (comment 6177046363), claim 6177192097, sessionsession_0121ooBYRmS8ziFXm9ai5YHm.What changed
packages/mcp/src/mcp-http-tools.ts,errorResultFromThrownonly (plus the one import it reads). A thrown refusal whose declared code is the standard catalogue'sPERMISSION_DENIEDnow renders througherrorResultWithCode, the function the data tools already render through, so itsdetailsare withheld. The code is spelled fromStandardErrorCode(@objectstack/spec/api, already a dependency), not by hand. Every other coded refusal keeps itsdetails. Therun_actionandresume_runcatches are unchanged: both already callerrorResultFromThrown.packages/mcp/src/mcp-action-refusal-details.test.ts: the pin, on the MCP HTTP face over the real Streamable HTTP transport, the waymcp-coded-refusal.test.tsdrives it..changeset/23529-mcp-action-tools-withhold-permission-details.md:@objectstack/mcppatch.Why
The security gate's
PERMISSION_DENIEDcarries the caller's own positions and permission-set names indetails. The ruling on #7450 keeps those off the wire. The dispatcher drops them, the REST actions door answers throughresolveThrownHttpError's closeddetailslist, and since #23509 the data tools render througherrorResultWithCode, which never readsdetails. The two action tools rendered througherrorResultFromThrown, which spreaddetailsverbatim into a tool result. That tool result rides a JSON-RPC 200, so the dispatcher's withholding never saw it.Reach, read from source (reading 6176605181, re-read on this tree):
executeDeclarativeUpdateActionwrites throughcallData('update', …, ec)as the caller, and nothing between the security middleware anderrorResultFromThrowncatches the gate's write-side refusal. The subject load does not carry it:loadActionSubjectRecord's bare catch drops the throw andrefuseDeniedSubjectLoadanswersRECORD_NOT_FOUNDwith nodetails.Route: the first choice measured, then the second
First choice, measured and not taken.
resolveThrownHttpError(@objectstack/types) buildsdetailsfrom a closed list (non-stringcode,issues, validationfields) and never reads the throwndetails. A probe on its built output:It withholds the denial, but it also drops the confirmation refusal's retry
details, whichmcp-action-confirmation-member.test.tsandmcp-resume-run-tool.test.tspin. So it would break MCPrun_actionnever enforcesai.requiresConfirmation— it is surfaced inlist_actionsonly, while@objectstack/specguidance tells authors it stops execution #15942.Taken: the withholding keyed on the declared code, through an existing export. No key-name list. No second filter of
details: the class's answer iserrorResultWithCode's, reused. EveryPermissionDeniedErrordeclares this code, because it is a class constant. Every throw inplugin-securitythat puts positions or permission sets indetailsis that class. A throw with no code never reaches thedetailsread.No other package is edited:
packages/runtime,packages/typesandplugin-securityare untouched, and so ispackages/mcp/src/plugin.ts([Decision] API key 的生命周期与存放:到期上限与不活动失效取多长,key 继续放自有 sys_api_key 还是迁到 @better-auth/api-key(#23287 裁决 C′ 的留项) #23367 has it in flight).The pin: red on
main, green afterThe bridge is a double, because
@objectstack/mcpdoes not depend on@objectstack/runtime. It lets the caller read the subject row, and it refuses both action tools with the gate'sPermissionDeniedErrorshape. The assertions: the tool error carriescodePERMISSION_DENIEDand status 403. Then each position and permission-set name is checked against the whole HTTP response body, not one key. A control row on each tool keepsACTION_CONFIRMATION_REQUIRED'sdetailsserved.Run against
main's renderer (basee1cefa58e3, the pin added, no fix):After the fix, the pin and the three suites that must not move, unchanged:
mcp-action-confirmation-member.test.tsandmcp-coded-refusal.test.ts(itsRECORD_NOT_FOUNDrows included) are byte-unchanged by this PR.resume_runis covered by a pin, at the renderer. It calls the sameerrorResultFromThrown, and its row fails and passes withrun_action's. By reading, nothing reaches it today: it makes no caller-scope write of its own, and its subject re-read ends atRECORD_NOT_FOUND. So its row guards the shared renderer, not a measured producer.Ablation
node scripts/ablation-replace.mjs, wrap mode, under the verify lock. The mutation deletes the guard line and the pin runs on the mutated file. The subject resolves through the package's ownsrc/, so no build ordist/leg applies.The direction is the ordinary one: the two denial rows turn red and the two confirmation controls stay green. That shows the guard is keyed on the code and is not a blanket withhold.
git statuswas clean after the restore.232f1b56c0is still this file's blob at the head.Tests and gates, at head
03c6a098a3pnpm --filter @objectstack/mcp test:Test Files 39 passed (39),Tests 467 passed (467).pnpm --filter @objectstack/mcp typecheck: exit 0, andcheck:test-typecheck: OKwith no new debt. The new test file compiles undertsconfig.test.json.pnpm --filter @objectstack/mcp build: exit 0. The CJS and ESM builds both load. The CJS bundle requires@objectstack/spec/api, and it resolves.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 63 commands. All 63 were run, with each exit code captured before any pipe. Reconciliation:✓ dispatch-gates --ran: 63 derived famil(ies) accounted for — 63 run, 0 NOT-MEASURED (a DERIVED zero — all 63 recorded an exit code and none of them is 3).PREREQUISITE NOT MET(exit 3):check:lean-entry-closureandcheck:dual-build-cjs-loads. I built their prerequisites (turbo run build, all but one task a cache hit) and re-ran them. Both exit 0, and those are the readings recorded.mainwas merged once before this PR opened. The one incoming commit touchedpackages/lintonly, and@objectstack/lintwas rebuilt.Acceptance notes
detailsstill render verbatim on these two tools. REST passes a thrown refusal'sdetailsonly through the closed list. No other producer that reaches these tools puts caller authorization data indetailstoday. A fail-closed alternative is to serve only the confirmation refusal'sdetails. It was not taken because it would also dropresume_run's REST-built run artefacts on a failed resume, which narrows more than this card asks.executeRegisteredActionrethrows a handler's own error raw. A rethrownPermissionDeniedErrorkeeps its code, so this renderer-level fix covers it with no producer change. No in-tree producer was found.errorResultWithCodeis untouched. It sits outside the claimed surface, and it still holds for that function.errorResultFromThrown's own doc names the one code-keyed exception and why it is not a second copy of the policy.detailsreach the MCP HTTP face intact for a data verb.Generated by Claude Code