fix(#4636,#4653): close the symlink hole, revert a wrong collapse, fix six review findings

The RED checkpoint and two orthogonal reviews found eight defects. All fixed here.

THE COLLAPSE THAT WAS WRONG — installer-migrations. Routing ensureInsideConfig's
containment decision through the realpath-based canonical predicate broke four
tests, and the failure message says it plainly: "migration path escapes
configDir: extensions/gsd.cjs". That module's entire contract is that a
symlinked managed path is snapshotted, restored and backed up AS A LINK and
never dereferenced. The canonical predicate dereferences, then rejects the
result for escaping configDir — so it destroys exactly the thing the module
exists to preserve. Reverted to lexical, with the ruling recorded above the
function so it is not collapsed a third time. normalizeRelPath is the real
pre-gate there; it throws on absolute paths and '..' before this check runs.

That makes THREE deliberately-retained implementations, not two, and they share
one shape worth naming: a realpath-based predicate is the wrong tool wherever a
symlink must be PRESERVED rather than resolved. CONTEXT.md and
docs/explanation/security-model.md are corrected — both previously described
ensureInsideConfig as collapsed.

THE MISSED CONSUMER. tests/security-prompt-injection.security.test.cjs
destructures validatePath from the compiled lib; un-exporting it turned five
tests into TypeError. It appeared in my own earlier search output and I did not
follow it up. Translated under the same rule as the rest: assertions on the
rejection REASON go through assertWithinRoot, boolean-only through
tryWithinRoot.

VALIDATE-ONE-PATH-USE-ANOTHER, FOUND TWICE MORE. This is the fourth and fifth
occurrence in this epic of the exact defect it exists to prevent.
  - scripts/check-glossary-refs.cjs decided containment on `token` and then
    stat'd a separately re-joined path.join(ROOT, token). The ContainedPath is
    now carried through to the probe, so the validated value is the probed one.
  - src/init.cts computed skillPathContained and DISCARDED it, re-joining from
    the raw input for the existsSync and read. The branded type exists to make
    that a type error and here it was inert.

AND THE OVER-CORRECTION OF THAT FIX, caught before it shipped. The first attempt
also substituted the validated value into the EMITTED `ref` for a global skill.
That value is a display token, not a path anything reads through — the only fs
access in that branch runs on the lexical path beforehand — so substituting it
changed emitted output two ways: it is realpath-resolved, so a symlinked global
skills directory would have emitted its resolved target instead of the user's
own path, and it came from path.join, so Windows would have emitted a backslash
where the template has a literal '/'. Restored, with the distinction recorded:
the containment check there is a GATE, not a path producer.

A TEST THAT COULD NOT FAIL. The first symlink regression planted its symlink
from inside a hooked fs.readdirSync and never asserted the planting happened —
if the hook did not fire, the "nothing was written outside" assertion passed
trivially, green against vulnerable code. It now asserts the plant, matching its
sibling. The other two were re-checked: one already asserted its equivalent, the
other plants synchronously and cannot silently no-op.

THE SYMLINK FIX ITSELF, now that the tests are proven red on the matrix.
isPathConfined is lexical by design and structurally cannot see a symlink; three
callers relied on it with no defense of their own. install-engine.cts:1608 and
install-profiles.cts:880 refuse to mkdir/write through a link — mkdirSync with
recursive:true does NOT throw on an existing symlink-to-directory, so a planted
link redirected the SKILL.md write outside the install root.
install-profiles.cts:755 refuses to read through one — statSync FOLLOWS links,
so an outside file's contents were returned and installed as a skill body. Each
mirrors the guard retired-artifact-cleanup.cts:77 already uses.

Severity stated accurately rather than dramatically: only the read at :755 needs
no race. _removeGsdEntries sweeps a pre-planted link at :1608 before the write
loop, and :880's stageDir is a fresh mkdtemp, so both of those require winning a
window. They are fixed as defense-in-depth, not as live exploits.

ALSO: the Changed changeset claimed "every command's observable behavior [is]
unchanged". Three rejection messages are reworded. It now says so, and says that
none of them reveals a host path it previously hid. A stale comment in
verify.cts still named validatePath; an init.cts warning hardcoded "resolves
outside the project directory" for a check that also rejects absolute paths, NUL
bytes and empty strings; and the rationale deleted with check-glossary-refs'
retired helper is restored, noting honestly that a rejected token is now
realpath-resolved before rejection rather than rejected by string comparison.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
sim
2026-09-12 13:59:52 -04:00
parent 9953d02184
commit 6f0e5ccf85
12 changed files with 116 additions and 42 deletions

View File

@@ -162,17 +162,31 @@ module is the central security utility. It provides:
- Shell argument validation: arguments passed to subshell commands are
validated before use
Two containment checks elsewhere in the tree are deliberately NOT routed through
Three containment checks elsewhere in the tree are deliberately NOT routed through
this predicate, because each is narrower or stricter rather than a second opinion.
The common thread is that a realpath-based predicate is the wrong tool wherever a
symlink must be *preserved* rather than resolved.
The backup-restore gate in `gsd-core/bin/gsd-tools.cjs` rejects symlinks outright:
the canonical predicate accepts a link whose target resolves inside the root, but
for a restore that is still wrong, because writing through the link overwrites
whatever it points at instead of materializing a regular file at the backed-up
path. And `isPathConfined` in `src/external-descriptor-trust.cts` is lexical by
path. `isPathConfined` in `src/external-descriptor-trust.cts` is lexical by
design, because two install callers must validate a destination *before* the
`mkdirSync` that creates it, where `realpath` cannot resolve. A lexical check
cannot see a symlink, so callers that rely on it for a write-confinement
guarantee must pair it with their own symlink refusal.
`mkdirSync` that creates it, where `realpath` cannot resolve. And
`ensureInsideConfig` in `src/installer-migrations.cts` is lexical because that
module's contract is that a symlinked managed path is snapshotted, restored and
backed up *as a link* and never dereferenced — resolving it would dereference
precisely the links the module exists to preserve, and then reject them for
escaping the config directory.
A lexical check cannot see a symlink, so callers that rely on one for a
write-confinement guarantee must pair it with their own symlink refusal. Three
install call sites did not, and now do: a link planted at a capability skill's
destination made `mkdirSync` succeed silently and redirected the write outside
the install root, and a link planted at a capability's own `SKILL.md` was
followed by `statSync`, so an outside file's contents were installed as a skill
body.
**Runtime hook: `gsd-prompt-guard.js`.** This hook fires on every Write or
Edit call that targets `.planning/` files. It scans the content being written