Uh oh!
There was an error while loading. Please reload this page.
fix(security): guard the write-response internal-field property, not one class (#8497) - #8587
Conversation
…one class (#8497) Widen the #7823 write-response `internal: true` guard from "every `*Data` face on the protocol class" to the property it exists to hold: no response body an external caller receives from a write carries an `internal: true` value. Widening it found a live leak. `@objectstack/mcp`'s stdio bridge is engine-only by construction and its `create` arm echoed `engine.insert`'s result — whole since #7823 relocated the strip off the engine — straight back to the MCP caller. Measured, fixed, and pinned. - mcp: strip on `create` (the leak) and on `update` (so a caller cannot use their own patch as an oracle on an internal column). - core: the shared strip helper moves here from metadata-protocol, which re-exports it unchanged. `rest` and `mcp` both write through the engine directly and neither depends on metadata-protocol, so the old home forced every new mouth to duck-type or restate the rule. - rest + mcp: two new tripwires, same mechanism as the shipped one (runtime enumeration, total disposition map, negative control). The shipped protocol tripwire is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy
`packages/rest` hides its test files from its own `typecheck` script, so the package typecheck passed while `check:type-check-debt` measured +2 raw tsc errors against the frozen TEST_DEBT entry — both of them in the new tripwire: a relative import missing its `.js` extension under NodeNext, and `registry.registerObject` called without its required `packageId`. Fixed rather than ledgered: TEST_DEBT is a shrink-only ratchet and raising it is maintainer-only. Re-measured at 155, exactly the recorded value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 3 package(s): 28 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 5 release-owned page(s) also reference the affected code. These are read-only:
|
os-zhuang
commented
Aug 14, 2026
PM review (
⭐ Worth stating plainly, because it is the argument for this whole family of cards: the leak was found only by widening the guard. The card predicted an invisible-mouth risk and the enumeration turned up an actual one, in a file whose header asserted its divergences were "deliberate, filed, not security." Checked against the tree, not the description:
One boundary the PR states rather than hides, and I agree with recording it: of the 45 write routes, only the Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Closes#8497
Widens the #7823 write-response
internal: trueguard from "every*Dataface on the protocol class" to the property it exists to hold:⛔ Not a redesign. The shipped
protocol.write-response-internal-fields.tripwire.test.tsis untouched — its runtime prototype walk and itsleakyDatanegative control stay exactly as they are, and still pass (1245 metadata-protocol tests green). This adds scope, it does not replace mechanism.The card said the
rest-serverbatch mouth is covered by the fix and merely invisible to the guard — correct, and it still is. But the card's own sentence was broader:That sentence was already true of a second transport.
@objectstack/mcp's stdio bridge is engine-only by construction (the long-lived stdio host cannot reuse the runtime's request-shapedcallDatabuilder), and itscreatearm handedengine.insert's result straight back to the MCP caller. Since #7823 the engine deliberately keeps write results whole, so the flagged column rode the tool response verbatim. Measured before the fix, on the bridge the stdio transport actually serves:The file's own header listed its protocol-layer divergences as "deliberate, filed, not security". One limb of that list was security; the header now says so.
Fixing it is inside this card's scope rather than a follow-up: a guard shaped to exclude the one place the property is false would repeat exactly the mistake the card is about.
The three directions, evaluated by measurement
The body offered three. All three were measured before choosing.
1. Source-level check on call sites that write through the engine and return a body. ⛔ Rejected as the primary shape. Measured: 176 direct engine write call sites across 25 packages outside
metadata-protocol. Almost all are internal side-effect writes (plugins, services) that answer no external caller — and "returns a body to an external caller" is a semantic property no grep can decide. So this becomes a ~176-entry ledger in which ~175 entries say "not a response", each one a human judgement the check cannot verify, churning on every new plugin write. It guards a proxy (call-site shape), not the property.2. Assert at the response envelope, independent of which path produced it. ⭐ Picked — the triage default, and the measurement supports it. External write bodies are produced by transports, and both transports enumerate their own surface at runtime, so the property can be asserted where it is actually true using the same mechanism the shipped tripwire already uses:
RestServer.getRoutes()(88 routes, 45 of them write methods) and theMcpDataBridgeobject the stdio factory returns. A new route or a new bridge verb with no disposition fails the suite with instructions — the enumeration grows by itself, exactly as the prototype walk does.3. Write it down: direct-engine mouths outside
metadata-protocolare the author's responsibility. Adopted only as a complement, never as the mechanism — it is the convention-not-mechanism option this repo rejects. It survives as the module header on the relocated helper, which now names all three tripwires and what to do when adding a mouth.What changed
@objectstack/mcp— the stdio bridge'screatestrips (the leak).updatestrips too: that arm discards the engine's write result and echoes the read-path row plus the caller's own patch, so no stored value could reach it — but a caller who puts aninternal: truekey indatawould otherwise have it echoed back, using their own bytes as an oracle on a column the flag says is never returned. Read verbs untouched.@objectstack/core— the strip helper moved here from@objectstack/metadata-protocol. It shipped beside the protocol class when that class was its only caller, butrestandmcpboth reach the engine directly and neither depends onmetadata-protocol, so the old home forced every new mouth to choose between a duck-typed reach through a protocol instance (whatrestdoes, via an optional?.call that silently no-ops if the method ever disappears) and a private restatement of a security-relevant rule.coreis the floor all three already depend on and already hosts this exact class of shared write-path helper (bulk-write.ts, shared bymetadata-protocolandrest). No API change:metadata-protocolre-exports both names, verified against the rebuiltdist, not the source.rest,mcp), same mechanism as the shipped one: runtime enumeration, total disposition map, negative control.rest-server.tsitself is unchanged — its mouth already applies the strip correctly. The latent?.silent-noop seam is now covered by measurement rather than by reading.Reverse verification
Three experiments, each with the expected direction predicted before running (all three: RED on the driven route — not more-diagnostics, not inverted). The fix was committed first, so every restore came out of a real commit.
POST /batchupdate armPOST /api/v1/batchonlyql.insert)createstripcreateThe acceptance criterion inherited from the family is the middle row, and it discriminates precisely. With the strip deleted from the update arm, the create arm (protocol ingress) stayed clean and only the direct arm leaked:
With the second mouth added instead, the leak moved to the first element while the still-stripped update arm stayed clean — the guard names which mouth leaked, not merely that something did.
All three restores proven byte-identical with
git hash-object:packages/rest/src/rest-server.ts→6a58de8f15a9565f5628553666a4cff8796db305(baseline, twice)packages/mcp/src/stdio-data-bridge.ts→53e08e6aa6136169723b6b7ba7317b77a278a52a(baseline)Two anti-blindness assertions guard the guard: every driven case demands a control value in the body (so a refusal or a 501 cannot satisfy "no sentinel" by returning nothing), and one test asserts the fixture's stored row really does carry the flagged value — without it, every green could mean the secret was never stored.
Tests and gates
Tests — 4108 green:
core786,mcp182,metadata-protocol1245 (incl. the untouched #7823 tripwire),rest1895.Gates run locally — derived with
scripts/pm/dispatch-gates.mjsagainst the actual changed paths, plus those judged implicated:check:nul-bytes·check:changeset-gate-self-tests·check:cross-package-test-inputs·check:durability-log-level·check:kernel-hook-pairs·check:test-source-alias·check:engine-double-contract·check:query-options-erasure·check:type-check-coverage·check:type-check-debt·check:objectui-changeset·eslint·typecheck(mcp, rest) — all PASS.check:type-check-debtcaught a real regression and is worth naming.packages/resthides its test files from its owntypecheckscript, so the package typecheck passed green while the ratchet measured +2 raw tsc errors against the frozen TEST_DEBT entry — both in the new tripwire (a relative import missing its.jsextension under NodeNext, andregistry.registerObjectcalled without its requiredpackageId). Fixed rather than ledgered: TEST_DEBT is shrink-only and raising it is maintainer-only. Re-measured at 155, exactly the recorded value.check:objectui-pin-freshis pre-existing RED on this repo (.objectui-shabehindobjectuimain, #3340) and fires on any.changeset/*touch. Not caused by this branch and deliberately not "fixed" here.Scope
content/docs/releases/untouched; the changeset is this PR's input to the release notes.no-record-echo/protocol-ingress) rather than drives. That boundary is stated plainly in the file: onlydrivenis a measurement, the rest are reviewed claims, and the enumeration's job is to force an explicit decision when a route is added. Riskier claims were checked rather than asserted — the import runner's per-row results spread onlywarnings, never the written row, and the sharing routes echo a grant row from the sharing service, not a row of the:objectin the path.Generated by Claude Code