fix(studio): verbatim-symlink install executes bytes outside the substrate root — add a realpath containment walk - #506
Merged
KnockOutEZ merged 3 commits intoAug 27, 2026
Conversation
…cks accept The install copies symlinks verbatim, which carries a source-authored ABSOLUTE link across intact, and every containment check was a string check on a path that never touched the filesystem. Five arms reproduce it: an absolute link at the executable, an escaping link elsewhere in the tree, a relative link that climbs out, a record whose directory is a link out of the root, and a record whose executable resolves outside. All five are red against today's module. The positive framework arms stay as they were, plus an anti-false-decline arm for the /var -> /private/var prefix the platform actually uses.
…strings verbatimSymlinks copies a link's target string unchanged. That is what keeps a bundle's relative framework links working, and equally what carries a source-authored ABSOLUTE link across intact — while every containment check was a string check that never touched the disk: the manifest string 'bin/run' has no '..' and no leading separator, isInside compared resolve() output, and the acquire probe used existsSync, which FOLLOWS the link. defaultLaunch then spawned the joined path and the OS ran bytes from outside the substrate root. - acquire walks the installed tree and refuses any link that leaves it, judged by where it RESOLVES rather than how it is spelt, before the executable probe that cannot see past it. Symlinked directories are judged, not descended. - isInside realpaths both sides, so a symlinked directory inside the root no longer satisfies containment and the /var -> /private/var prefix still does. - readSubstrateRecord resolves the spawn target itself, so a link swapped in after acquisition reads as absent rather than as a launchable component. The docstring claim that no shape of record.json could name a program elsewhere on the machine was false while it compared strings; it is restated as what makes it true now.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Mutating the absolute-target refusal out of the walk left the file green: for a link that RESOLVES, the absolute rule and the resolve rule reach the same verdict and either alone suffices — two mechanisms keyed on one decision. They part on a DANGLING absolute link. It resolves to nothing, so the walk falls back to judging it lexically, and a target spelt inside today's substrate root passes — leaving an absolute path baked into the installed tree that the substrate follows the moment anything appears there, and that points elsewhere the moment the data dir moves. The new arm plants exactly that and reds without the rule. Four mutants, each killed by a named test: realpath only one side of isInside (11 reds, including the anti-false-decline arm), drop the absolute refusal (1), remove the walk (4), remove the spawn-target resolve in readSubstrateRecord (1). All reverted by reverse-edit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes KnockOutEZ/wigolo-studio-run#173. PX0 exit-2, SEC-2/K2 MED — blocking, because it falsifies a shipped security claim.
The defect
install()copies withverbatimSymlinks, which is required (883d94a0: it is what keeps a bundle's relative framework links working). It equally carries a source-authored absolute symlink across intact — and every containment check was a string check on a path that never touched the filesystem:bin/run— no.., not absolute, so the manifest rule passes;isInsidecomparedresolve()output, so the record rule passes;existsSync, which follows the link, so verification passes.defaultLaunchthen spawnsjoin(path, executable), the OS follows the link, and the bytes that run live somewhere else on the machine.Base-vs-tip differential
Driving the real
acquireSubstrate/readSubstrateRecord/defaultLaunchfromdist/, against a bundle whosebin/runisln -s <elsewhere>/payload.sh:OLD (
7ed4bc61)NEW
The
883d94a0behaviour is untouched — a framework bundle reachable only through two relative links still installs, still reads back, and still launches after the install source is deleted:What changed
acquireSubstratewalks the installed tree and refuses any link that leaves it, before the executable probe that cannot see past it. Judged by where a link resolves, not how it is spelt — an absolute target is refused outright, a relative one is refused when it resolves out. Symlinked directories are judged as links, never descended into, so a link cycle cannot walk forever.isInsiderealpaths both sides. A symlinked directory inside~/.wigolo/substrate/no longer satisfies containment (the exit review's LOW SPA-shell heuristic produces false positives on short static HTML pages #3), and the macOS/var→/private/varprefix still does — resolving one side only would decline every legitimate record on the platform the desktop component targets.readSubstrateRecordresolves the spawn target itself, so a link swapped in after acquisition reads as absent rather than as a launchable component.The two false claims
substrate-acquire.ts:133-139claimed no shape ofrecord.jsoncould name a program elsewhere on the machine. That was false while it compared strings — the escape does not need arecord.jsonedit at all. Restated as what makes it true now: the filesystem answers containment, not the text.substrate-acquire.test.ts:342-344asserted "no link in the installed tree may address anything outside the directory it was installed into" against a fixture whose links are relative by construction. Restated as what it actually establishes (the copy did not rewrite them), with the containment claim moved to the negative arms that hold it up.Tests
Five new arms, all red against
7ed4bc61and green here: an absolute link at the executable; an escaping link elsewhere in the tree while the executable is a genuine file (a walk, not a second probe on the one named path); a relative link that climbs out (so a fix that merely banned the leading separator would not pass); a record whose directory is a link out of the root; a record whose executable resolves outside. Plus an anti-false-decline arm for the linked-prefix case.Suites:
npm testgreen,npx tsc --noEmitclean,npm run gate:studioexit 0.