From 8454f009444a207380d9375a589276884284a14b Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Sun, 30 Aug 2026 00:36:48 +0600 Subject: [PATCH 1/4] test(studio): pin the symlinked root and the non-version path as containment escapes Both shapes reach the record that backs the prompt-less substrate spawn, and both pass every containment check the module has: the symlinked root because each side resolves through the link, the nested path because it really is inside the root. Red at tip, with controls that the escapes are real and that a linked ANCESTOR of the root still reads. --- tests/unit/studio/substrate-acquire.test.ts | 131 ++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/tests/unit/studio/substrate-acquire.test.ts b/tests/unit/studio/substrate-acquire.test.ts index c936af0c..d2d97511 100644 --- a/tests/unit/studio/substrate-acquire.test.ts +++ b/tests/unit/studio/substrate-acquire.test.ts @@ -697,6 +697,137 @@ describe('containment is answered by the filesystem, not by string comparison', }); }); +/** + * CONTAINMENT IS RELATIVE TO A ROOT, AND THE ROOT IS ITSELF A PATH ON DISK. + * + * Everything above answers "is this inside the root" by resolving both sides. That is the right + * question and it has a blind spot the size of the feature: NOTHING required the root to be a real + * directory. `substrateRoot()` is `/substrate`, and if that entry is a LINK to somewhere + * else, both sides resolve into the link's target, the prefix comparison agrees, and every + * containment check passes for a spawn target that lives entirely outside the data dir — and that + * therefore also survives the `rm -rf ~/.wigolo` the release checklist prescribes. + * + * The second shape is narrower and independent: `readSubstrateRecord` asked only whether `path` was + * SOMEWHERE under the root. The docstring above it states the actual rule — the acquirer writes + * `path` as `substrateRoot()/` and nothing else, which is the entire reason a record can + * be treated as this product's own installation rather than as a launch instruction from whoever + * last edited the file. A nested path under the root satisfies containment and could not have been + * written by the acquirer, so reading it as PRESENT is the docstring's premise being false in the + * one place the no-consent spawn depends on it. + * + * Both were reproduced against the built `dist/` at core `019f160c` with control arms. They are + * independent: pinning `path` to `join(root, version)` does not close the symlinked root, and + * refusing a symlinked root does not close the nested path. + * + * ⚠ THESE ARMS RUN ON EVERY PLATFORM. The plants are DIRECTORY links, made with `'junction'` — + * ignored on POSIX, and on Windows the one directory link that needs no elevation. Nothing here + * goes through `cpSync`, so there is no second link for the copy to re-create. + */ +describe('the substrate root is a real directory, and a record names exactly root/', () => { + let attacker: string; + + beforeEach(() => { + attacker = realpathSync(mkdtempSync(join(tmpdir(), 'wigolo-attacker-'))); + }); + + afterEach(() => { + rmSync(attacker, { recursive: true, force: true }); + }); + + /** The canonical shape, planted at the root this call is asked about. */ + function plantCanonical(root: string, version = '1.2.3'): void { + const dir = join(root, version); + mkdirSync(join(dir, 'bin'), { recursive: true }); + writeFileSync(join(dir, 'bin', 'run'), '#!/bin/sh\n'); + writeFileSync(join(root, SUBSTRATE_RECORD), JSON.stringify({ version, path: dir, executable: 'bin/run' })); + } + + it('ACCEPTS the canonical shape — a real root holding exactly root/', () => { + // ANTI-VACUITY, local to this block. Every arm below is a refusal, and a `readSubstrateRecord` + // hardwired to `return null` would satisfy all of them while having removed the feature. + mkdirSync(substrateRoot(dataDir), { recursive: true }); + plantCanonical(substrateRoot(dataDir)); + expect(readSubstrateRecord(dataDir)?.version).toBe('1.2.3'); + }); + + it('SEC-1: reads as absent when the substrate root is itself a link to somewhere else', () => { + const root = substrateRoot(dataDir); + // The root is never created — it IS the link. Everything the "acquirer" then writes lands in + // the attacker's directory while being spelt as `/substrate/1.2.3`. + symlinkSync(attacker, root, 'junction'); + plantCanonical(root); + + // CONTROL — THE ESCAPE IS REAL AND EVERY EXISTING CHECK PASSES ON IT. + const dir = join(root, '1.2.3'); + // (a) the record's own containment rule agrees, because both sides resolve through the link; + expect(realpathSync(dir).startsWith(realpathSync(root) + sep)).toBe(true); + // (b) the executable `defaultLaunch` would spawn is on disk; + expect(existsSync(join(dir, 'bin', 'run'))).toBe(true); + // (c) and the bytes it would run live OUTSIDE the data dir entirely — so they are not what + // this product installed, and `rm -rf ~/.wigolo` does not remove them. + expect(realpathSync(dir).startsWith(realpathSync(dataDir) + sep)).toBe(false); + expect(realpathSync(join(dir, 'bin', 'run')).startsWith(attacker + sep)).toBe(true); + + expect(readSubstrateRecord(dataDir)).toBeNull(); + }); + + it('SEC-1: the refusal is about the ROOT, not about any link on the way to it', () => { + // ANTI-OVERREACH, and the arm that fails a fix written as "refuse if anything in the path is a + // link". On macOS the data dir routinely sits under `/var -> /private/var`, so a rule that + // walked the ancestors would decline every legitimate record on the platform the desktop + // component targets. Only the root ENTRY ITSELF is required to be real. + const realBase = realpathSync(mkdtempSync(join(tmpdir(), 'wigolo-linked-data-'))); + const linkedBase = join(dirname(realBase), `${basename(realBase)}-via-link`); + symlinkSync(realBase, linkedBase, 'junction'); + try { + expect(realpathSync(linkedBase)).toBe(realBase); + mkdirSync(substrateRoot(linkedBase), { recursive: true }); + // The root is a real directory REACHED THROUGH a link, which is the shape that must keep + // working. + expect(lstatSync(substrateRoot(linkedBase)).isSymbolicLink()).toBe(false); + plantCanonical(substrateRoot(linkedBase)); + expect(readSubstrateRecord(linkedBase)?.version).toBe('1.2.3'); + } finally { + rmSync(linkedBase, { recursive: true, force: true }); + rmSync(realBase, { recursive: true, force: true }); + } + }); + + it('SEC-2: reads as absent when the path is nested under the root but is not root/', () => { + const root = substrateRoot(dataDir); + const nested = join(root, 'tmp', 'deep'); + mkdirSync(nested, { recursive: true }); + writeFileSync(join(nested, 'helper'), '#!/bin/sh\n'); + writeFileSync(join(root, SUBSTRATE_RECORD), JSON.stringify({ version: '1.2.3', path: nested, executable: 'helper' })); + + // CONTROL: containment is genuinely satisfied — this really is inside the root, so the arm is + // not the pre-existing "path escapes the root" rule wearing a new name. What it is not is the + // one path the acquirer writes. + expect(realpathSync(nested).startsWith(realpathSync(root) + sep)).toBe(true); + expect(existsSync(join(nested, 'helper'))).toBe(true); + + expect(readSubstrateRecord(dataDir)).toBeNull(); + }); + + it('SEC-2: reads as absent when the version is not one directory name', () => { + // THE ARM THAT MAKES `join(root, version)` A RULE RATHER THAN A COINCIDENCE. Pinning `path` to + // `join(root, raw.version)` is only worth anything while `version` is a directory NAME: with + // `version: '.'` the join is the root itself, the equality holds, containment holds, and a + // payload dropped beside `record.json` reads back as an installed component. That is the same + // "not written by the acquirer" class, one level up, and it survives the equality fix alone. + const root = substrateRoot(dataDir); + mkdirSync(join(root, 'bin'), { recursive: true }); + writeFileSync(join(root, 'bin', 'run'), '#!/bin/sh\n'); + writeFileSync(join(root, SUBSTRATE_RECORD), JSON.stringify({ version: '.', path: root, executable: 'bin/run' })); + + // CONTROL: the equality this fix is built on is SATISFIED by this shape. + expect(realpathSync(root)).toBe(realpathSync(join(root, '.'))); + expect(existsSync(join(root, 'bin', 'run'))).toBe(true); + + expect(readSubstrateRecord(dataDir)).toBeNull(); + }); +}); + describe('source resolution', () => { it('reads a manifest from a substrate directory', () => { expect(readSubstrateManifest(sourceDir)).toEqual({ version: '1.2.3', executable: 'bin/run' }); From 96d1841d698170cbb2f80e11bd679c30dc1a4968 Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Sun, 30 Aug 2026 00:39:18 +0600 Subject: [PATCH 2/4] fix(studio): refuse a linked substrate root and a record path that is not root/ The record backs a prompt-less spawn, and its docstring justified that by saying the acquirer writes path as substrateRoot()/ and nothing else. Two shapes made that false. A root that is itself a link resolved along with the candidate, so every containment comparison agreed for a tree outside the data dir; readSubstrateRecord and acquireSubstrate now both refuse it via lstat, which is blind to a linked ancestor and so keeps the macOS /var -> /private/var shape working. And path was only required to be somewhere under the root; it must now resolve to join(root, version), with version required to be one directory name so the join cannot be '.' or a traversal. Two fixtures planted a path the acquirer cannot produce and are corrected. --- src/studio/substrate-acquire.ts | 76 +++++++++++++++++++++++++-- tests/unit/cli/studio.test.ts | 12 +++-- tests/unit/studio/auto-launch.test.ts | 9 +++- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/src/studio/substrate-acquire.ts b/src/studio/substrate-acquire.ts index 4f7730cf..04e8f853 100644 --- a/src/studio/substrate-acquire.ts +++ b/src/studio/substrate-acquire.ts @@ -1,4 +1,4 @@ -import { cpSync, existsSync, mkdirSync, readdirSync, readFileSync, readlinkSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; +import { cpSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, readlinkSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, isAbsolute, join, relative, resolve as resolvePath, sep } from 'node:path'; import { getConfig } from '../config.js'; import { createLogger } from '../logger.js'; @@ -114,6 +114,33 @@ function isInside(candidate: string, root: string): boolean { return c === r || c.startsWith(r.endsWith(sep) ? r : r + sep); } +/** + * Is this entry ITSELF a link, judged without following it? + * + * ⚠ THE ROOT IS A PATH TOO, AND {@link isInside} CANNOT SEE IT. Containment is stated relative to + * the substrate root, and `isInside` resolves BOTH sides — which is correct for a root reached + * THROUGH a link (macOS puts the data dir under `/var -> /private/var`, and resolving one side + * only would decline every real record there) and blind to the root BEING one. Where + * `/substrate` is a link to somewhere else, both sides resolve into the target, the + * prefix comparison agrees, and every check passes for a tree that is not in the data dir at all — + * so it is not what this product installed, and `rm -rf ~/.wigolo` does not remove it either. + * + * `lstat` is the whole distinction: it stats the entry rather than what the entry points at, so an + * ancestor link is invisible to it (which is what keeps the legitimate macOS shape working) and + * the root's own link-ness is not. A junction answers `true` here as well, which is what makes the + * rule hold on Windows rather than only on the platforms with POSIX symlinks. + * + * A root that cannot be stat'd is not a link — it is absent, and every caller here is already on a + * path that reads an absent substrate as absent. + */ +function isLinkEntry(p: string): boolean { + try { + return lstatSync(p).isSymbolicLink(); + } catch { + return false; + } +} + /** The real location of `p`, or null when it does not resolve to anything on disk. */ function realpathIfPossible(p: string): string | null { try { @@ -164,8 +191,23 @@ function isSingleDirectoryName(name: string): boolean { * elsewhere on the machine", and that was FALSE for as long as it compared strings: neither the * record's `path` nor its `executable` has to be a link ITSELF for the joined path to resolve * outside — any link along the way does it, and `existsSync` follows links so the probe agreed. - * So the executable is RESOLVED and required to land inside the substrate root. The claim is true - * of this version because the filesystem, not the text, answers it. + * So the executable is RESOLVED and required to land inside the substrate root. + * + * ⚠ AND THE PARAGRAPH ABOVE WAS ITSELF TOO STRONG UNTIL PX0's EXIT REVIEW (SEC-1, SEC-2). It + * asserted a property of the CODE that only two of the three necessary rules held up, and the + * no-consent spawn rests on the whole claim being true as written. The two that were missing: + * + * SEC-1 — "inside the substrate root" is only a containment statement while the root is a real + * directory. `isInside` resolves both sides, so a root that is a LINK resolves along with the + * candidate and every comparison agrees for a tree living entirely outside the data dir. The + * root's own link-ness is now refused first ({@link isLinkEntry}), before anything is compared. + * + * SEC-2 — "anywhere under the root" is weaker than the rule this docstring states. The acquirer + * writes `join(root, version)` and nothing else, so a nested-but-contained path could not have + * come from it; `path` is now required to RESOLVE to that one location, and `version` is + * required to be a single directory name so the join is a name rather than a traversal. + * + * The claim is true of this version because the filesystem, not the text, answers all three. */ export function readSubstrateRecord(dataDir?: string): SubstrateRecord | null { try { @@ -184,6 +226,20 @@ export function readSubstrateRecord(dataDir?: string): SubstrateRecord | null { if (!raw.version || !raw.executable || !raw.path) return null; if (!staysInsideItsDirectory(raw.executable)) return null; const root = substrateRoot(dataDir); + // SEC-1 — BEFORE ANY COMPARISON, because the comparison is the thing this defeats. See + // {@link isLinkEntry}. + if (isLinkEntry(root)) return null; + // SEC-2 — the acquirer writes ONE path, so that is the one path a record may name. + // `version` first: `join(root, '.')` is the root and `join(root, '../x')` leaves it, so + // without this the equality below would hold for locations the acquirer cannot produce. + if (!isSingleDirectoryName(raw.version)) return null; + const expected = realpathIfPossible(join(root, raw.version)); + const named = realpathIfPossible(raw.path); + if (expected === null || named === null || named !== expected) return null; + // NOT SUBSUMED BY THE EQUALITY ABOVE. Both sides of it are the same spelling, so it holds + // even when `/` is itself a link out of the root — the shape the arms below + // plant. Containment is what refuses that, and it is the only rule that resolves the + // DIRECTORY rather than comparing two names for it. if (!isInside(raw.path, root)) return null; const exec = join(raw.path, raw.executable); if (!existsSync(exec)) return null; @@ -405,6 +461,20 @@ export async function acquireSubstrate(deps: AcquireSubstrateDeps = {}): Promise } const root = substrateRoot(dataDir); + // THE SAME ROOT RULE AS THE READER'S, ON THE WRITE SIDE — because the two answering differently + // is its own defect. A linked root does not stop `install()`: `mkdirSync(root, {recursive:true})` + // succeeds on an existing link, the bytes land in the link's target, and `isInside(destDir, root)` + // agrees because both sides resolve there. The acquisition would report `acquired` for a record + // `readSubstrateRecord` now refuses on every subsequent run — the permanent disagreement the + // version guard below exists to prevent — while having copied a component into a directory + // outside the data dir. `isLinkEntry` cannot throw, so this stays a guard outside the try. + if (isLinkEntry(root)) { + return { + outcome: 'failed', + detail: 'the directory the desktop component installs into is a link to somewhere else', + error: `substrate root is a symlink: ${root}`, + }; + } // EVERY STATEMENT THAT CAN THROW IS INSIDE THE TRY, INCLUDING THE GUARDS. `destDir` used to be // computed one line above the `try`, which made NEVER THROWS true only for the argument shapes // the guards happened to anticipate; moving the join in fixed that, and then left the guards diff --git a/tests/unit/cli/studio.test.ts b/tests/unit/cli/studio.test.ts index 630309b3..ab7bedc8 100644 --- a/tests/unit/cli/studio.test.ts +++ b/tests/unit/cli/studio.test.ts @@ -1592,13 +1592,15 @@ function fakeSpawn(calls: SpawnCall[], unrefs?: { count: number }): (c: string, /** * Plant a valid acquisition record — valid means the executable it names is really on disk AND - * sits under `substrateRoot()`, which is the only place `acquireSubstrate` ever installs to. This - * used to plant at `/installed`, a location the acquirer never writes; the containment - * rule in `readSubstrateRecord` reads such a record as absent, so the fixture would have been - * pinning `runStudio` against a record the product cannot produce. + * sits at `substrateRoot()/`, which is the only place `acquireSubstrate` ever installs + * to. This used to plant at `/installed`, a location the acquirer never writes; the + * containment rule in `readSubstrateRecord` reads such a record as absent, so the fixture would + * have been pinning `runStudio` against a record the product cannot produce. `/installed` + * was the same mistake one level in — contained, but still not a path the acquirer writes — and + * PX0's SEC-2 made the reader say so. */ function plantRecord(dataDir: string, executable = 'wigolo-studio'): string { - const substrateDir = join(dataDir, 'substrate', 'installed'); + const substrateDir = join(dataDir, 'substrate', '0.1.0'); mkdirSync(substrateDir, { recursive: true }); writeFileSync(join(substrateDir, executable), '#!/bin/sh\nexit 0\n', { mode: 0o755 }); mkdirSync(join(dataDir, 'substrate'), { recursive: true }); diff --git a/tests/unit/studio/auto-launch.test.ts b/tests/unit/studio/auto-launch.test.ts index faf3e177..0b09b5db 100644 --- a/tests/unit/studio/auto-launch.test.ts +++ b/tests/unit/studio/auto-launch.test.ts @@ -38,7 +38,10 @@ function publishHandle(): void { */ function plantSubstrateRecord(dataDir: string): string { const root = join(dataDir, 'substrate'); - const componentDir = join(root, 'component'); + // `/` AND NOTHING ELSE — the one location `acquireSubstrate` writes, and since + // PX0's SEC-2 the only one `readSubstrateRecord` reads. A fixture planted at `/component` + // pinned the launcher against a record the product cannot produce. + const componentDir = join(root, '0.0.1'); mkdirSync(componentDir, { recursive: true }); const executable = join(componentDir, 'studio-app'); writeFileSync(executable, '#!/bin/sh\nexit 0\n'); @@ -196,7 +199,9 @@ describe('studioLaunchable — the recorded distribution ceiling', () => { ); } - const componentDir = join(root, 'component'); + // `/` — see {@link plantSubstrateRecord}. Any other location under the root + // reads as absent since PX0's SEC-2, so this helper would plant a record nothing accepts. + const componentDir = join(root, '0.0.1'); mkdirSync(componentDir, { recursive: true }); writeFileSync(join(componentDir, 'studio-app'), '#!/bin/sh\nexit 0\n'); writeFileSync( From f28a050763bad6cc1ebd59c520c4ad4ceaa806b4 Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Sun, 30 Aug 2026 00:41:49 +0600 Subject: [PATCH 3/4] test(studio): make the root/ equality the thing that kills the SEC-2 arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nested-path arm was green against a build with the equality deleted: with no real / on disk the refusal came from the expected location being absent, which is the weaker half of the rule. It now plants a legitimate install alongside the decoy, so both sides resolve and only the inequality refuses; the absent-location case keeps its own arm. Dropping isInside(raw.path, root) likewise changed no test — the equality dominates it and the executable check is what refuses a linked / — so it is removed rather than left to be trusted. --- src/studio/substrate-acquire.ts | 17 +++++---- tests/unit/studio/substrate-acquire.test.ts | 40 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/studio/substrate-acquire.ts b/src/studio/substrate-acquire.ts index 04e8f853..8e334603 100644 --- a/src/studio/substrate-acquire.ts +++ b/src/studio/substrate-acquire.ts @@ -235,12 +235,17 @@ export function readSubstrateRecord(dataDir?: string): SubstrateRecord | null { if (!isSingleDirectoryName(raw.version)) return null; const expected = realpathIfPossible(join(root, raw.version)); const named = realpathIfPossible(raw.path); - if (expected === null || named === null || named !== expected) return null; - // NOT SUBSUMED BY THE EQUALITY ABOVE. Both sides of it are the same spelling, so it holds - // even when `/` is itself a link out of the root — the shape the arms below - // plant. Containment is what refuses that, and it is the only rule that resolves the - // DIRECTORY rather than comparing two names for it. - if (!isInside(raw.path, root)) return null; + // `expected === null` is the both-unresolved case, which the inequality alone would let past + // (`null !== null` is false) and which the executable probe below would then refuse anyway. + // It is stated here because this is where the rule is decided; it is not carrying the rule. + if (expected === null || named !== expected) return null; + // ⚠ `isInside(raw.path, root)` USED TO SIT HERE AND IS GONE, deliberately. The equality above + // strictly dominates it: any `path` outside the root either fails to equal + // `realpath(join(root, version))` or fails to resolve at all, and where `/` is + // ITSELF a link out of the root the equality holds (both sides are the same spelling) — so + // containment never decided that case either. What decides it is the executable check below, + // which resolves the path the OS will actually run. Measured 2026-08-29: deleting the line + // changed no test, which is the whole reason it is not still here being trusted. const exec = join(raw.path, raw.executable); if (!existsSync(exec)) return null; // The spawn target itself, resolved. `isInside` returns false for a path that does not diff --git a/tests/unit/studio/substrate-acquire.test.ts b/tests/unit/studio/substrate-acquire.test.ts index d2d97511..d841e63f 100644 --- a/tests/unit/studio/substrate-acquire.test.ts +++ b/tests/unit/studio/substrate-acquire.test.ts @@ -795,6 +795,14 @@ describe('the substrate root is a real directory, and a record names exactly roo it('SEC-2: reads as absent when the path is nested under the root but is not root/', () => { const root = substrateRoot(dataDir); + // ⚠ THE LEGITIMATE INSTALL IS PLANTED TOO, AND THAT IS WHAT MAKES THIS ARM SHARP. Without it + // `join(root, version)` resolves to nothing and the refusal comes from "the acquirer's path is + // not on disk" — a real rule, but the WEAK half: it leaves the arm green against a fix that + // dropped the equality entirely. Measured, 2026-08-29: that mutant survived. With a real + // `/1.2.3` present, both sides resolve and only `named !== expected` refuses this record. + // It is also the realistic shape — an installed component, and a record edited beside it to + // name a payload dropped somewhere else under the same root. + plantCanonical(root); const nested = join(root, 'tmp', 'deep'); mkdirSync(nested, { recursive: true }); writeFileSync(join(nested, 'helper'), '#!/bin/sh\n'); @@ -805,7 +813,23 @@ describe('the substrate root is a real directory, and a record names exactly roo // one path the acquirer writes. expect(realpathSync(nested).startsWith(realpathSync(root) + sep)).toBe(true); expect(existsSync(join(nested, 'helper'))).toBe(true); + // CONTROL: and the path the acquirer WOULD have written is on disk, so the refusal cannot be + // the "expected location is absent" arm. + expect(existsSync(join(root, '1.2.3', 'bin', 'run'))).toBe(true); + + expect(readSubstrateRecord(dataDir)).toBeNull(); + }); + it('SEC-2: reads as absent when root/ is absent, even though the path it names is real', () => { + // The other half of the same rule, kept as its own arm now that the one above deliberately + // plants a real `/`: a record naming a location that exists, filed under a + // version that was never installed, is still not something the acquirer wrote. + const root = substrateRoot(dataDir); + const nested = join(root, 'tmp', 'deep'); + mkdirSync(nested, { recursive: true }); + writeFileSync(join(nested, 'helper'), '#!/bin/sh\n'); + writeFileSync(join(root, SUBSTRATE_RECORD), JSON.stringify({ version: '9.9.9', path: nested, executable: 'helper' })); + expect(existsSync(join(root, '9.9.9'))).toBe(false); expect(readSubstrateRecord(dataDir)).toBeNull(); }); @@ -826,6 +850,22 @@ describe('the substrate root is a real directory, and a record names exactly roo expect(readSubstrateRecord(dataDir)).toBeNull(); }); + + it('SEC-1: does not INSTALL into a linked root either, rather than acquiring what it then refuses', async () => { + // THE WRITE SIDE OF THE SAME RULE, and not the read arm wearing a second hat. `mkdirSync` + // succeeds on an existing link and `isInside(destDir, root)` agrees because both sides resolve + // through it, so acquisition would report `acquired`, copy the component into a directory + // OUTSIDE the data dir, and then read back as absent on every subsequent run — the permanent + // acquire/read disagreement this module's version guard exists to prevent. + const root = substrateRoot(dataDir); + symlinkSync(attacker, root, 'junction'); + const r = await acquireSubstrate({ dataDir, source: localPathSource(sourceDir) }); + expect(r.outcome).toBe('failed'); + expect(r.error).toMatch(/symlink/); + // And it refused BEFORE copying anything into the attacker's directory. + expect(existsSync(join(attacker, '1.2.3'))).toBe(false); + expect(readSubstrateRecord(dataDir)).toBeNull(); + }); }); describe('source resolution', () => { From d80863d7a5050110229bb3c7575e069d30ebf61c Mon Sep 17 00:00:00 2001 From: KnockOutEZ Date: Sun, 30 Aug 2026 00:43:57 +0600 Subject: [PATCH 4/4] test(studio): gate the containment arms on a measured link capability, not on the platform The nine skipIf(win32) arms cited 'creating a symlink needs elevation', which this same file contradicts by planting an unskipped junction. The accurate rule is about link TYPE: a junction needs no elevation but is a directory link that Node normalises to an absolute target, and the arms driving acquireSubstrate have their plant re-created by cpSync with no type hint, so a junction does not survive the copy. The two record-level arms need no copy, so they now plant junctions and run unconditionally on all three shipped OSes; the executable arm links bin/ rather than bin/run to make that possible. The seven copy-driven arms ask the machine whether it can make ordinary links instead of assuming Windows cannot, so they run on a Windows runner with Developer Mode. Un-skipping them blindly would have been worse than the skip: four expect outcome 'failed', which is also what a cpSync EPERM produces. A probe arm keeps a silently-false capability answer from vacating the family. --- tests/unit/studio/substrate-acquire.test.ts | 109 ++++++++++++++++---- 1 file changed, 89 insertions(+), 20 deletions(-) diff --git a/tests/unit/studio/substrate-acquire.test.ts b/tests/unit/studio/substrate-acquire.test.ts index d841e63f..ab0b96a3 100644 --- a/tests/unit/studio/substrate-acquire.test.ts +++ b/tests/unit/studio/substrate-acquire.test.ts @@ -25,6 +25,48 @@ import { * somewhere else entirely. */ +/** + * CAN THIS MACHINE PLANT THE LINKS THESE FIXTURES NEED? MEASURED, NOT GUESSED. + * + * The containment family used to be gated on `process.platform === 'win32'` with the rationale + * "creating a symlink there needs elevation" — a rationale this same file contradicts, since the + * linked-prefix arm plants a `'junction'` unskipped and the studio guard plants one on win32 too. + * The accurate statement is narrower and is about link TYPE, not platform: + * + * - A junction needs no elevation, and is a DIRECTORY link. Every arm whose plant this file + * makes itself can use one, so those arms run on all three shipped OSes unconditionally. + * - The arms below that drive `acquireSubstrate` cannot. `install()` copies with + * `cpSync(..., verbatimSymlinks: true)`, which re-creates each link as `symlinkSync(target, + * dest)` with NO type argument — Node never chooses a junction there, so a junction planted in + * the SOURCE comes out the other side as an ordinary link. Whether that succeeds is a property + * of the machine (Developer Mode, or an elevated token — GitHub's Windows runners generally + * have one), not of this repo. + * + * So the gate asks the machine instead of assuming the answer. Where Windows CAN make links, the + * arms RUN there rather than being skipped on a guess; where it genuinely cannot, they skip for a + * measured reason. Skipping on a guess is worse than either: four of these arms expect + * `outcome: 'failed'`, and a `cpSync` that dies of EPERM produces exactly that — so un-skipping + * them blindly would have bought four arms that pass without testing anything. + */ +let linkCapability: boolean | null = null; +function canPlantSymlinks(): boolean { + if (linkCapability !== null) return linkCapability; + const probe = mkdtempSync(join(tmpdir(), 'wigolo-link-probe-')); + try { + mkdirSync(join(probe, 'target')); + writeFileSync(join(probe, 'target', 'file'), 'x'); + symlinkSync(join(probe, 'target'), join(probe, 'dir-link')); + symlinkSync(join(probe, 'target', 'file'), join(probe, 'file-link')); + linkCapability = + lstatSync(join(probe, 'dir-link')).isSymbolicLink() && lstatSync(join(probe, 'file-link')).isSymbolicLink(); + } catch { + linkCapability = false; + } finally { + rmSync(probe, { recursive: true, force: true }); + } + return linkCapability; +} + let dataDir: string; let sourceDir: string; @@ -55,6 +97,18 @@ afterEach(() => { delete process.env[SUBSTRATE_PATH_ENV]; }); +describe('the link-capability gate answers about the machine, not about a guess', () => { + it('is TRUE on every platform with POSIX symlinks', () => { + // THE OUTSIDE SIGNAL. A probe that answered `false` everywhere — a typo in the plant, a + // `tmpdir()` that stopped being writable — would skip the whole containment family and report + // green, which is the exact failure mode replacing `skipIf(win32)` exists to remove. On win32 + // the answer is a property of the runner (Developer Mode, or an elevated token) rather than of + // this repo, so there is nothing here to assert; the junction-based arms run there regardless. + if (process.platform !== 'win32') expect(canPlantSymlinks()).toBe(true); + expect(typeof canPlantSymlinks()).toBe('boolean'); + }); +}); + describe('acquireSubstrate — install, verify, record (D-S10-3)', () => { it('installs the component and records it', async () => { const r = await acquireSubstrate({ dataDir, source: localPathSource(sourceDir) }); @@ -367,8 +421,10 @@ describe('the version a record is filed under must be one directory name', () => * The acquire-time VERIFY cannot catch this: the top-level executable is a real file, so the probe * passes while every framework link underneath it points somewhere else. * - * Windows is skipped because creating a symlink there needs elevation, and this corruption class - * is a POSIX-symlinked bundle shape. + * These arms are gated on {@link canPlantSymlinks}, not on the platform. The fixture's links are + * RELATIVE by construction and the assertions read their target strings back verbatim, so a + * junction — which Node normalises to an absolute target — would be asserting something else. + * Where a machine can make ordinary links, including a Windows one with Developer Mode, these run. */ describe('the install copies symlinks verbatim rather than resolving them', () => { /** @@ -400,7 +456,7 @@ describe('the install copies symlinks verbatim rather than resolving them', () = rmSync(frameworkDir, { recursive: true, force: true }); }); - it.skipIf(process.platform === 'win32')('leaves the installed links relative instead of pointing them back at the source', async () => { + it.skipIf(!canPlantSymlinks())('leaves the installed links relative instead of pointing them back at the source', async () => { const r = await acquireSubstrate({ dataDir, source: localPathSource(frameworkDir) }); expect(r.outcome).toBe('acquired'); const installed = join(substrateRoot(dataDir), '7.7.7', 'Frameworks', 'E.framework'); @@ -418,7 +474,7 @@ describe('the install copies symlinks verbatim rather than resolving them', () = expect(spawnTarget.startsWith(realpathSync(substrateRoot(dataDir)) + sep)).toBe(true); }); - it.skipIf(process.platform === 'win32')('still resolves once the install source is deleted', async () => { + it.skipIf(!canPlantSymlinks())('still resolves once the install source is deleted', async () => { // ANTI-VACUITY, and the arm that a rewritten-link copy cannot pass. An absolute link into the // source satisfies every existence check above for as long as the source survives — the // corruption only becomes visible when the thing it secretly depends on goes away. The install @@ -455,8 +511,11 @@ describe('the install copies symlinks verbatim rather than resolving them', () = * "where does this RESOLVE", not "does this start with a slash", and an arm that only plants * absolute links would stay green against a fix that merely banned the leading separator. * - * Windows is skipped for the same reason the arms above are: creating a symlink there needs - * elevation. + * Gated on {@link canPlantSymlinks} for the same reason as the arms above: the plant has to + * survive `cpSync`, which re-creates it without a type hint, so a junction does not help. Note + * what un-skipping these blindly would have bought — they expect `outcome: 'failed'`, and a + * `cpSync` that dies of EPERM produces exactly that, so on a machine that cannot make links they + * would pass while testing nothing. */ describe('the install refuses a tree whose links leave it', () => { let outside: string; @@ -486,7 +545,7 @@ describe('the install refuses a tree whose links leave it', () => { return dir; } - it.skipIf(process.platform === 'win32')('refuses a bundle whose executable is an absolute link out of the tree', async () => { + it.skipIf(!canPlantSymlinks())('refuses a bundle whose executable is an absolute link out of the tree', async () => { const src = makeLinkedSourceDir('3.3.3', payload); try { // CONTROL: the escape is real, and every string-only check passes on it. The manifest @@ -503,7 +562,7 @@ describe('the install refuses a tree whose links leave it', () => { } }); - it.skipIf(process.platform === 'win32')('refuses an escaping link even when the executable itself is a real file', async () => { + it.skipIf(!canPlantSymlinks())('refuses an escaping link even when the executable itself is a real file', async () => { // The acquire-time probe is satisfied here — `bin/run` is genuine bytes — so this arm is what // distinguishes a containment WALK from a second existence check on the one named path. A // bundle's dynamic libraries and resources are reached through links the manifest never names. @@ -518,7 +577,7 @@ describe('the install refuses a tree whose links leave it', () => { } }); - it.skipIf(process.platform === 'win32')('refuses a DANGLING absolute link even when it is spelt inside the root', async () => { + it.skipIf(!canPlantSymlinks())('refuses a DANGLING absolute link even when it is spelt inside the root', async () => { // THE ARM THAT MAKES THE ABSOLUTE RULE ITS OWN MECHANISM. For a link that resolves, the // absolute rule and the resolve rule agree and either alone would do. They part exactly here: // this link resolves to nothing, so a walk that judged only by resolution would fall back to @@ -540,7 +599,7 @@ describe('the install refuses a tree whose links leave it', () => { } }); - it.skipIf(process.platform === 'win32')('refuses a RELATIVE link that climbs out of the installed tree', async () => { + it.skipIf(!canPlantSymlinks())('refuses a RELATIVE link that climbs out of the installed tree', async () => { // Not the reported vector — a relative link re-anchors at the destination and usually dangles. // It is here because the rule is "where does this resolve", and a fix that only banned a // leading separator would leave this one green while the hole stayed open. @@ -576,12 +635,13 @@ describe('the install refuses a tree whose links leave it', () => { * on the warmup path, unattended and with no timeout of its own, so the failure is a warmup that * never returns rather than a component that fails to install. * - * Windows is skipped for the same reason as the arms above: creating a symlink there needs - * elevation. The per-test timeout is deliberate — if the rule is ever lost, this arm must report + * Gated on {@link canPlantSymlinks} for the same reason as the arms above: `self -> .` is relative + * by construction, it has to survive `cpSync`, and the arm reads its target back verbatim — none + * of which a junction does. The per-test timeout is deliberate — if the rule is ever lost, this arm must report * as a failing test rather than as a runner that stopped making progress. */ describe('the walk terminates on a link cycle that is contained', () => { - it.skipIf(process.platform === 'win32')( + it.skipIf(!canPlantSymlinks())( 'installs a tree whose directory link points at its own parent', async () => { const src = mkdtempSync(join(tmpdir(), 'wigolo-substrate-cycle-')); @@ -625,13 +685,18 @@ describe('containment is answered by the filesystem, not by string comparison', rmSync(outside, { recursive: true, force: true }); }); - it.skipIf(process.platform === 'win32')('reads as absent when the directory the record names is a link out of the root', () => { + // ⚠ THESE TWO RUN EVERYWHERE. The plants are DIRECTORY links made with `'junction'` — ignored on + // POSIX, and on Windows the one directory link that needs no elevation — and nothing here goes + // through `cpSync`, so there is no second link for the copy to re-create with the wrong type. + // That is the difference between them and the `canPlantSymlinks()` family above. + it('reads as absent when the directory the record names is a link out of the root', () => { // The record's `path` string is exactly what the acquirer writes — `/1.2.3` — so string - // containment is satisfied, and the executable is on disk because `existsSync` follows links. - // Only resolving the directory shows it is not in the root at all. + // containment is satisfied, `join(root, version)` and `path` are the same spelling so the + // acquirer-path equality is satisfied too, and the executable is on disk because `existsSync` + // follows links. Only resolving what gets SPAWNED shows it is not in the root at all. mkdirSync(substrateRoot(dataDir), { recursive: true }); const dir = join(substrateRoot(dataDir), '1.2.3'); - symlinkSync(outside, dir); + symlinkSync(outside, dir, 'junction'); writeFileSync( join(substrateRoot(dataDir), 'record.json'), JSON.stringify({ version: '1.2.3', path: dir, executable: 'bin/run' }), @@ -640,12 +705,16 @@ describe('containment is answered by the filesystem, not by string comparison', expect(readSubstrateRecord(dataDir)).toBeNull(); }); - it.skipIf(process.platform === 'win32')('reads as absent when the executable it names resolves outside the root', () => { + it('reads as absent when the executable it names resolves outside the root', () => { // The last line of defence, for a tree that was not installed by this process — a link swapped // in after acquisition, or a record hand-edited beside one. + // + // The link is on `bin/` rather than on `bin/run` so it can be a junction and the arm can run on + // win32. It is the same defect either way: the record names `bin/run`, that path exists, and + // the bytes the OS would execute live outside the substrate root. const dir = join(substrateRoot(dataDir), '1.2.3'); - mkdirSync(join(dir, 'bin'), { recursive: true }); - symlinkSync(join(outside, 'bin', 'run'), join(dir, 'bin', 'run')); + mkdirSync(dir, { recursive: true }); + symlinkSync(join(outside, 'bin'), join(dir, 'bin'), 'junction'); writeFileSync( join(substrateRoot(dataDir), 'record.json'), JSON.stringify({ version: '1.2.3', path: dir, executable: 'bin/run' }),