fix(studio): substrate containment defeated by a symlinked root and a non-version path (PX0 exit SEC-1+SEC-2)

[KnockOutEZ]

Closes wigolo-studio-run#197. PX0 exit-review SECURITY, two confirmed MED findings, both in src/studio/substrate-acquire.ts.

Both defects falsified the decision record at readSubstrateRecord's docstring — the one that backs the prompt-less substrate spawn on the fetch path (S9's amended-D4: "the filesystem, not the text, answers it"). The docstring's premise is that acquireSubstrate writes path as substrateRoot()/<version> and nothing else, so a record naming anywhere else was not written by the acquirer. Two shapes made that premise false while every containment check passed.

SEC-1 — a symlinked substrate root defeats containment

isInside(candidate, root) realpath-resolves BOTH sides then prefix-compares. Resolving both sides is correct — on macOS the data dir sits under /var -> /private/var, and resolving one side only would decline every legitimate record there — and it is structurally blind to the root itself being a link. Where <dataDir>/substrate is a symlink, both sides resolve into the link's target, every comparison agrees, and the spawn target lives entirely outside the data dir — so it is not what this product installed, and rm -rf ~/.wigolo does not remove it either.

lstat is exactly the missing distinction: it stats the entry rather than what the entry points at, so a linked ancestor stays invisible (the macOS shape keeps working) and the root's own link-ness does not. A junction answers isSymbolicLink() true, so the rule holds on Windows as well. Refused in readSubstrateRecord and, for the same reason, in acquireSubstrate — a linked root does not stop install(), so acquisition would otherwise report acquired for a component copied outside the data dir and then read back as absent on every later run.

SEC-2 — a record could name any path under the root, not root/<version>

path was only required to be somewhere inside the root. It must now resolve to realpath(join(root, version)), with version required to be a single directory name — otherwise join(root, '.') is the root itself and a payload dropped beside record.json reads back as an installed component, which is the same "not written by the acquirer" class one level up.

Repro-first, and the arms have teeth

Tip-red arms for both shapes were committed before the fix (8454f009 → 96d1841d), with controls that each escape is real: for SEC-1 that the existing containment comparison passes on it and the spawn target resolves into the attacker's directory; for SEC-2 that the path genuinely is inside the root.

The first SEC-2 arm was green against a build with the equality deleted — with no real <root>/<version> on disk the refusal came from the expected location being absent, the weaker half of the rule. It now plants a legitimate install alongside the decoy, so both sides resolve and only the inequality refuses. Mutation table, each guard deleted in turn and reverse-edited back:

mutation	result
baseline	71 passed (71)
readSubstrateRecord drops isLinkEntry(root)	1 failed | 70 passed
readSubstrateRecord drops isSingleDirectoryName(version)	1 failed | 70 passed
readSubstrateRecord drops the root/<version> equality	1 failed | 70 passed
acquireSubstrate drops isLinkEntry(root)	1 failed | 70 passed
readSubstrateRecord drops isInside(exec, root)	2 failed | 69 passed

if (!isInside(raw.path, root)) is deleted, not kept as defence in depth: the equality strictly dominates it, and where <root>/<version> is itself a link out of the root the equality holds — both sides are the same spelling — so containment never decided that case either; the executable check does. Deleting the line changed no test. A guard no arm can kill is not a layer, it is a line a future reader will trust. (A-197-2.)

Fold: cov-finding-1, the nine skipIf(win32) containment arms

Their rationale — "creating a symlink there needs elevation" — is contradicted by the same file, which plants an unskipped junction. But un-skipping was not the correction. The accurate rule is about link type: a junction needs no elevation but is a directory link Node normalises to an absolute target, and the arms that drive acquireSubstrate have their plant re-created by cpSync(..., verbatimSymlinks: true), which calls symlinkSync(target, dest) with no type argument — so a junction does not survive the copy.

• The two record-level arms make their own plants and need no copy: they now use junctions and run unconditionally on all three shipped OSes (the executable arm links bin/ rather than bin/run to make that possible). Both new SEC arms are junction-based and run on win32 too.
• The seven copy-driven arms are gated on a measured link-capability probe rather than on the platform, so a Windows runner with Developer Mode runs them. Blindly un-skipping those would have been worse than the skip: four expect outcome: 'failed', which is also what a cpSync dying of EPERM produces — four arms passing while testing nothing. A probe arm keeps a silently-false capability answer from vacating the family.

Demo

The exit session's two repro shapes, re-run against the built dist/ before and after. Full JSON in the closing comment.

arm	BEFORE (tip 019f160c)	AFTER
SEC-1 A — symlinked root, spawn target resolving into the attacker dir	record RETURNED	null
SEC-1 B (control) — real root, path outside	null	null
SEC-1 C (control) — real root, canonical root/<version>	returned	returned
SEC-2 — nested-but-contained path, legit install also present	record RETURNED	null
SEC-2b — version: "."	record RETURNED	null

Verification

npm run build 0 · npx tsc --noEmit 0 · npm run typecheck:studio 0 · npm run gate:studio 0 (tests/ type-check debt holds at baseline 363) · full root suite green.

CI deferred — quota (CEO 2026-08-28): merged on local green.

Territory

Lane core:studio-core owns src/studio/** and tests/unit/studio/** — three of the four files. lane-extra: declared for tests/unit/cli/studio.test.ts: its plantRecord fixture planted <root>/installed, a path the acquirer cannot write, so SEC-2 necessarily reds it. Corrected to <root>/<version>, which makes the fixture more faithful, not less. The same correction was needed in tests/unit/studio/auto-launch.test.ts (in lane).

Non-goals held

No change to launch/spawn wiring beyond what containment required — the deps.launchable guard gap, the catch-block rmSync and the env passthrough stay POLISH issues. No move of src/studio/** to the private repo.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9cd4679f-6dbd-4369-abfd-956f9eae9f48

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}