* feat(#2796): reviewer lane as a fourth trust-disclosure class Phase 3 of epic #2782, delivering ADR-2782 D5. A reviewer lane is piped the plan text, requirements, research findings and CONTEXT.md decisions, and its output is read back into REVIEWS.md -- an egress channel for the most sensitive artifacts GSD produces. Making lanes pluggable WITHOUT a disclosure class would open a data-exfiltration path behind a manifest field, which is why this gates the feature rather than following it. - discloseExecutableSurfaces was cyclomatic 51 / cognitive 99 / 110 lines with risk_level critical. Rather than grow it, it is now a short orchestrator over four extracted collectors (hooks, commands, mcp -- behaviour-preserving -- plus the new lane collector), each independently testable. That is also what makes the 80% mutation threshold survivable: 51 branches in one function cannot be mutation-covered by whole-function tests. - A spawn lane discloses its binary AND its full declared args, in rendered and raw form. Binary-only disclosure would be insufficient and not hypothetically: a lane declaring python3 with innocuous args could later change them to ['-c', '<program>'] without the binary changing. That is the bug class #1459 already fixed for MCP servers. - An openai-http lane has no binary, so it discloses the destination host and the config key naming it. A localhost destination is disclosed and distinguished from a remote one. Both forms name the egress payload classes. THE CONSTRAINT THAT SHAPED THE DESIGN: the lane element is appended to the disclosure signature ONLY when at least one lane is declared. signatureForManifest is the consent key both the loader and the lifecycle compare, so appending unconditionally would have changed every installed capability's signature and re-prompted every user for every capability on their next upgrade -- for a feature they do not use. Two pre-change goldens are asserted byte-for-byte as the tripwire. The resolved host is deliberately NOT in the signature. The loader has no config resolver, so including it would make the loader and the lifecycle compute different signatures for the same manifest and produce a permanent false-mismatch loop. It is disclosed and recorded instead; Phase 5b re-resolves and compares at invocation, which is D5 rule 4's own placement. reviewsSection and timeoutFloorMs are also excluded from the signature: a cosmetic change must not force re-consent, because a prompt carrying no security information is how users learn to click through. A lane's binary is NOT existence-checked against the staged bundle. It is a PATH tool, never a bundle artifact; treating it like a hook script would add every lane to missingArtifacts and block every lane install. Two defects fixed beyond the fourth class: - isLocalHostValue mis-parsed a scheme-less host: new URL('localhost:1234') does NOT throw, it reads 'localhost' as the URL scheme and yields an empty hostname, so a bare host:port would have been reported as non-local. Now falls back on an empty hostname rather than only on a caught throw. - The orchestrator's safeCollect closes a PRE-EXISTING totality gap in the other three classes: a null manifest, or one with a throwing getter or Proxy trap, previously threw out of disclosure -- which runs on an UNVALIDATED manifest at install time. No well-formed input changes; all 51 existing trust tests pass. Closes #2796 * fix(#2796): close four disclosure gaps found by the isolated security review All four were REPRODUCED by execution against the shipped module, and all four passed the existing 41-test suite while live -- each exists because the matrix did not think to ask. B (MEDIUM, reachable via plain JSON). Non-string argv members were folded into the consent SIGNATURE but dropped from the human-facing text, because the summary rendered the string-filtered args rather than the raw declared array. A manifest declaring args ['--json', 7, {mode:'exfiltrate-everything'}, true] printed as '--json' alone -- the host still receives the rest, so the user consented to a surface never shown. That directly contradicts this design's own Kerckhoffs claim that nothing about a lane is hidden. The summary now renders the raw array, with non-strings shown in a visible form, and never throws on a circular or BigInt member. F (MEDIUM, reachable). The [local] flag is design-load-bearing, and it was dropped for every loopback form except the dotted quad and the bare hostname. Bracketed IPv6 was mangled by splitting on the address's own colons ([::1]:8080 became '['), and legacy IPv4 encodings were not recognised at all. A browser, curl and the OS resolver all treat 127.1, 2130706433, 0x7f000001 and 0177.0.0.1 as loopback. isLocalHostValue now handles bracketed and bare IPv6, IPv4-mapped loopback, and inet_aton shorthand/decimal/hex/octal. The dangerous direction was already clean and is now pinned by tests: localhost.evil.com, http://user@localhost@evil.com and friends stay REMOTE. C (LOW). An empty reviewer body flipped hasExecutable true and perturbed the disclosure signature, producing a re-consent prompt whose only content was '(no binary declared)'. A prompt carrying no security information is the click-through-training harm this design explicitly refuses for reviewsSection and timeoutFloorMs; refusing it there and permitting it here was inconsistent. A body declaring nothing recognised is no longer a lane. The test is deliberately broad -- any ONE recognised field suffices -- because requiring specifically a binary, or specifically a slug, would let a lane declaring only the other slip through unconsented, which is the far worse failure. Pinned in both directions. D (LOW-MEDIUM). Disclosure runs BEFORE validation, so a mis-cased or unrecognised transport reaches this code. Keying on an exact string sent a lane that plainly declares a hostConfigKey down the spawn branch, printing '(no binary declared)' for a lane egressing to a live remote host, and left its resolvedHost blank -- which reads as 'no destination', the precise thing the design forbids. Both the collector and the summary now branch on the declared SHAPE, so such a lane discloses its key and either a resolved host or the explicit unresolved marker. Two further findings were reproduced but confirmed NOT reachable through the real pipeline and are recorded as known limits rather than fixed: a selective-throw Proxy blanking a whole lane, and NaN/Infinity/undefined colliding to 'null' in a signature. Every production manifest reaches disclosure through readManifestBounded's strict JSON.parse, which cannot produce a Proxy, a getter, a BigInt, a circular reference, NaN or Infinity. The 0/-0 sub-case IS reachable via valid JSON but is inert -- String(0) === String(-0), so a spawned process receives identical argv. 9 regression tests added (50 total in this file, up from 41). * chore(#2796): backfill changeset pr number to 2826
This commit is contained in:
@@ -15,8 +15,9 @@
|
||||
|
||||
GSD 1.6.0 opens the capability platform to third-party authors with **full
|
||||
artifact parity**: a third-party capability may ship the same executable
|
||||
surfaces that GSD Core ships — hooks, MCP servers, command modules. This is a
|
||||
deliberate product choice, and it carries real security weight.
|
||||
surfaces that GSD Core ships — hooks, MCP servers, command modules, and
|
||||
reviewer lanes. This is a deliberate product choice, and it carries real
|
||||
security weight.
|
||||
|
||||
Full parity means a third-party capability, once installed, can execute code
|
||||
the next time a relevant loop event fires. There is no "first use" gate.
|
||||
@@ -58,9 +59,9 @@ contains.
|
||||
|
||||
GSD's response: auto-update is **off by default** for third-party capabilities.
|
||||
When it is enabled, a change to the *executable set* (the set of hooks, MCP
|
||||
servers, or command modules the capability declares) triggers a re-consent
|
||||
prompt before the update applies. Updating a non-executable capability
|
||||
(documentation, agents, skills) does not require re-consent.
|
||||
servers, command modules, or reviewer lanes the capability declares) triggers a
|
||||
re-consent prompt before the update applies. Updating a non-executable
|
||||
capability (documentation, agents, skills) does not require re-consent.
|
||||
|
||||
VS Code also has no signature check on VSIX packages. GSD requires an
|
||||
`integrity` SHA-512 pin in the ledger, verified before extraction.
|
||||
@@ -75,11 +76,57 @@ recommendation for sensitive environments is `--ignore-scripts`.
|
||||
|
||||
GSD takes a stronger position: **install never executes capability code**,
|
||||
full stop. Installation is a copy-only staging operation. There is no
|
||||
`postinstall`-equivalent. A capability's hooks, MCP server, and command
|
||||
modules are not invoked during install; they are first invoked when the loop
|
||||
fires after install. This means a malicious payload in an executable surface
|
||||
cannot be triggered by the act of downloading it — the user has a window
|
||||
between install and first use to verify what they consented to.
|
||||
`postinstall`-equivalent. A capability's hooks, MCP server, command
|
||||
modules, and reviewer lanes are not invoked during install; they are first
|
||||
invoked when the loop fires after install. This means a malicious payload in an
|
||||
executable surface cannot be triggered by the act of downloading it — the user
|
||||
has a window between install and first use to verify what they consented to.
|
||||
|
||||
### The reviewer lane: the one surface that *receives* data
|
||||
|
||||
Three of the four disclosure classes are about code the capability gets to
|
||||
**run**. A reviewer lane — one external CLI or model endpoint that `/gsd:review`
|
||||
hands a plan to — is different in kind, and the difference is the reason it is
|
||||
disclosed at all.
|
||||
|
||||
A lane is piped the plan text, the requirements, the research findings, and the
|
||||
`CONTEXT.md` decisions, and its output is read back into `REVIEWS.md`. That is an
|
||||
**egress channel for the most sensitive artifacts GSD produces**. Making lanes
|
||||
pluggable without a disclosure class would have opened a data-exfiltration path
|
||||
behind a manifest field, which is why the trust work gates the feature rather
|
||||
than following it.
|
||||
|
||||
What is disclosed depends on how the lane is reached:
|
||||
|
||||
- A **spawned** lane discloses its binary **and its full declared arguments**, in
|
||||
both rendered and raw form. Disclosing the binary alone would be insufficient,
|
||||
and not hypothetically: a lane declaring `python3` with innocuous arguments
|
||||
could later change them to `["-c", "<arbitrary program>"]` without the binary
|
||||
changing at all. Arguments are therefore signature-bound, exactly as they are
|
||||
for MCP servers.
|
||||
- An **OpenAI-compatible HTTP** lane has no binary, so it discloses the
|
||||
**destination host** and the config key that names it. Disclosing `curl` would
|
||||
be technically true and practically meaningless; the destination is the
|
||||
disclosure that matters. A `localhost` destination is still disclosed, and is
|
||||
distinguished from a remote one — a lane pointed at a local port is an egress
|
||||
channel too, and the port may not be what the user assumes.
|
||||
|
||||
Both forms additionally name the **egress payload classes**, rather than an
|
||||
unhelpful "sends data to the tool".
|
||||
|
||||
**Stated honestly:** consent-at-install is a weaker gate for a *standing* egress
|
||||
channel than it is for a hook. A user consents once; the lane thereafter receives
|
||||
every plan on every review run. Disclosure makes the channel visible, pinned and
|
||||
revocable — it does not make it safe. A per-run egress prompt was considered and
|
||||
rejected as consent fatigue that trains users to approve blindly.
|
||||
|
||||
One consequence is worth naming because it does not follow the pattern of the
|
||||
other three classes. A lane's destination host lives in `.planning/config.json`,
|
||||
which is user- and CI-editable at any time with no re-install and no integrity
|
||||
check — unlike every other consent-bound value, all of which come from the
|
||||
SHA-pinned manifest. Consent therefore binds the **resolved host**, not merely
|
||||
the config key, so that a later edit redirecting a consented lane to a different
|
||||
destination is detectable rather than silent.
|
||||
|
||||
SLSA provenance (the `provenance` field in `capability.json`) provides a
|
||||
machine-checkable link from a capability bundle back to a specific commit in a
|
||||
|
||||
Reference in New Issue
Block a user