* fix(#2393): add GSD_ALLOW_SYMLINKED_DEST opt-in for intentional user-owned symlink layouts Bug: v1.7.0's destSubpath write-confinement (ADR-1239 Phase B) refused install/update whenever CLAUDE_CONFIG_DIR (or an artifact-kind child like skills/, hooks/) was a pre-existing symlink, with no opt-out. Three legitimate user-owned layouts were blocked: - (lars-hh) CLAUDE_CONFIG_DIR=~/.claude-personal with skills/hooks symlinked to a user-owned external dir - (Mamiki) ~/.claude/skills is a Windows Junction to a shared skills dir - (Azd325) ~/.claude itself is a symlink to a dotfiles repo (the early root-is-symlink return refused before the component loop ran) Fix: add GSD_ALLOW_SYMLINKED_DEST env var (accepts '1' or 'true'). When set, hasExistingSymlinkBetween follows symlinks instead of refusing them. Cross-platform: fs.lstatSync().isSymbolicLink() returns true for both POSIX symlinks and NTFS junctions (Node ≥ 16), so Mamiki's Junction case is handled by the same code path. Threat model preserved (these still refuse EVEN WITH opt-in): (a) path-traversal in the destSubpath string itself ('../../etc'-style) — ADR-1239 Phase B threat (a), untrusted destSubpath protection (b) a symlink whose resolved real path equals the install root itself — would let _removeGsdEntries wipe the root; #1704 threat (b) (c) broken symlinks (realpathSync throws) — fail-closed What opt-in RELAXES specifically: the 'pre-existing symlink pointing outside configHome' refusal — #1704 threat (c). The user has explicitly asserted they own and trust the symlink target. Error messages at all 4 call sites (installRuntimeArtifacts, _copyStaged, migrateLegacyDevPreferencesToSkill, installOpencodeFamilySkills) updated to (1) name the env var opt-in, (2) be accurate when the root itself is a symlink (Azd325's complaint that the old message accused destDir of 'containing' a symlink when the root was the actual symlink). Docs: docs/CONFIGURATION.md Environment Variables table updated. Regression tests in tests/install-write-confinement.test.cjs cover: - child-symlink layout (lars-hh / Mamiki): default refuses, opt-in allows - root-is-symlink layout (Azd325): default refuses, opt-in follows - path-traversal '../../etc' refused EVEN WITH opt-in (threat a preserved) - resolved-target-equals-install-root refused EVEN WITH opt-in (threat b) - broken symlink refused EVEN WITH opt-in (fail-closed) * test(#2393): import beforeEach/afterEach in install-write-confinement suite The original file imported only { describe, test } from node:test. The new #2393 opt-in describe block uses beforeEach/afterEach to manage the GSD_ALLOW_SYMLINKED_DEST env var lifecycle — add them to the import. * test(#2393): correct broken-symlink test — existsSync follows link → loop terminates early Initial test expected broken symlinks to be refused even with opt-in. That was wrong: fs.existsSync follows symlinks, so a broken symlink returns false from existsSync and the component loop terminates before the symlink check fires. Both default and opt-in paths share this behavior; the fix preserves it. Updates the test to pin the actual current behavior so a future refactor (e.g. switching to lstatSync for existence) is a deliberate behavior change. * fix(#2393): realpath the install root — guard against macOS /var ↔ /private/var Code review (security subagent) flagged a HIGH-severity hole in the threat-(b) preservation: realTarget (from fs.realpathSync) is fully symlink-resolved, resolvedRoot (from path.resolve) is lexical-only. On macOS /var is a symlink to /private/var, so resolvedRoot='/var/foo/.claude' but realConfigHome is '/private/var/foo/.claude'. A symlink whose realtarget matches the install root by real path would compare unequal to the lexical resolvedRoot — defeating the wipe-protection guard exactly in the reporter's case (Azd325, nix-darwin: ~/.claude is itself a symlink). Fix: compute realRoot once via fs.realpathSync(resolvedRoot) at function entry (with fail-closed fallback to lexical form on realpath failure — broken/missing root, permission denied, exotic FS). Threat (a) path-traversal check above still confines regardless. Compare against BOTH lexical and real forms in both the root-symlink and component-symlink branches. Also adds the reviewer's transitivity-trust clarification comment: once a symlink is followed under opt-in, the walk continues from the resolved real path WITHOUT re-checking further segments stay inside a confining boundary. This is documented opt-in semantics — one opt-in trusts the whole reachable tree — and the comment makes the design choice explicit so a future maintainer doesn't add a 'follow one symlink only' expectation. Regression test added for the macOS /var normalization case (spelled configHome via os.tmpdir() lexically while pointing the test symlink through its realpath). Test skips on non-darwin platforms and when os.tmpdir() has no symlink component. * fix(#2393): root-symlink branch — do not apply threat-(b) check to root itself Initial fix applied the wipe-threat-(b) check to the root-symlink branch unconditionally. That was wrong: when root itself is a symlink (Azd325's nix-darwin case), its realpath IS realRoot by construction — so the check always fires, defeating the opt-in for exactly the case it was meant to enable. The wipe threat (b) does NOT apply to root being a symlink: destDir is a CHILD of root, and resolving root just gives root's target. There is no circular back-reference to root from a path that descends from a resolved root. So the root-symlink branch should just follow the symlink under opt-in and continue the walk, no threat-(b) check. Threat (b) only fires in the COMPONENT loop, where a child symlink can resolve back to the install root. That branch keeps the (b) check using BOTH lexical and real forms of root (the macOS /var ↔ /private/var fix from the prior commit). * fix(#2393): apply opt-in at the 5 bin/install.js call sites + add env-var/transitive tests Code review (correctness subagent) flagged a Critical coverage gap: the initial fix updated only the 4 src/install-engine.cts call sites. Five more call sites in bin/install.js still used the 2-arg signature, so the opt-in env var was silently ignored on: - installCodexConfig (config.toml + agents/ dir + per-agent .toml paths) — Codex only - copyWithPathReplacement (the generic emit path: workflows, commands, staging) — ALL runtimes - resolveInstallRelativePath (path resolver used in various places) Result: a user setting GSD_ALLOW_SYMLINKED_DEST=1 would see SOME refusals disappear (engine path) and OTHERS remain (bin/install.js paths) — a partially-applied install and a confusing UX, directly contradicting the PR's headline claim. Fix: - Export isSymlinkedDestOptIn from src/install-engine.cts alongside hasExistingSymlinkBetween - Import it in bin/install.js - Update all 5 bin/install.js call sites to pass { allowOptInFollow } - Update all 3 bin/install.js error messages to name the env var, matching the engine's phrasing Also addresses reviewer's Medium test-adequacy findings: - isSymlinkedDestOptIn env-var parsing now tested directly (accepts only documented '1' / 'true'; rejects 'TRUE', 'yes', 'on', '0', 'false', empty, unset) - transitive symlink chain (configHome/outer → outside1 → outside2) test pins the documented 'transitive and unbounded' opt-in semantics so a future contributor can't accidentally narrow it * chore(changeset): backfill pr:2445 in .changeset/eager-wasps-swim.md
This commit is contained in:
Tom Boucher
@@ -10,7 +10,7 @@
|
||||
* - _copyStaged: escape rejection, symlink escape (regression preserved)
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
@@ -2656,3 +2656,311 @@ describe('bug #3442: shim/wrapper serialized-command drift guard', () => {
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
// ─── #2393: GSD_ALLOW_SYMLINKED_DEST opt-in for intentional symlinked-dest layouts ────
|
||||
//
|
||||
// Three reporter layouts, all refused by the pre-#2393 guard with no opt-out:
|
||||
// (lars-hh) CLAUDE_CONFIG_DIR=~/.claude-personal with skills/hooks symlinked to
|
||||
// a user-owned external dir
|
||||
// (Mamiki) ~/.claude/skills is a Windows Junction to D:\claude-shared-resources\skills
|
||||
// (Azd325) ~/.claude itself is a symlink to a dotfiles repo (root-is-symlink)
|
||||
//
|
||||
// Fix: GSD_ALLOW_SYMLINKED_DEST=1 follows symlinks instead of refusing them,
|
||||
// while preserving the load-bearing refusals from #1704 / ADR-1239 Phase B:
|
||||
// (a) path-traversal in the destSubpath string itself ('../../etc')
|
||||
// (b) a resolved symlink target equal to the install root (would let _removeGsdEntries
|
||||
// wipe the root — the config-root-wipe threat)
|
||||
|
||||
describe('#2393: GSD_ALLOW_SYMLINKED_DEST opt-in for intentional symlinked-dest layouts', () => {
|
||||
const { hasExistingSymlinkBetween } = require('../gsd-core/bin/lib/install-engine.cjs');
|
||||
|
||||
beforeEach(() => {
|
||||
delete process.env.GSD_ALLOW_SYMLINKED_DEST;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
delete process.env.GSD_ALLOW_SYMLINKED_DEST;
|
||||
});
|
||||
|
||||
// Reporter case (lars-hh / Mamiki): a child component of configHome is a symlink
|
||||
// to a user-owned dir outside configHome. Default refuses; opt-in follows.
|
||||
test('child-symlink layout: default refuses, GSD_ALLOW_SYMLINKED_DEST=1 allows', (t) => {
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-cfg-'));
|
||||
const outsideTarget = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-out-'));
|
||||
try {
|
||||
const linkPath = path.join(configHome, 'skills');
|
||||
try {
|
||||
fs.symlinkSync(outsideTarget, linkPath);
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
const destDir = path.join(linkPath, 'gsd-foo');
|
||||
|
||||
// Default: refuse (existing pre-#2393 behavior unchanged).
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir),
|
||||
true,
|
||||
'default must refuse symlinked destDir (pre-#2393 behavior)',
|
||||
);
|
||||
|
||||
// Opt-in: allow (user asserted they trust the target).
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir, { allowOptInFollow: true }),
|
||||
false,
|
||||
'GSD_ALLOW_SYMLINKED_DEST=1 must allow intentional user-owned child symlink',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(path.join(configHome, 'skills')); } catch { /* already gone */ }
|
||||
cleanup(configHome);
|
||||
cleanup(outsideTarget);
|
||||
}
|
||||
});
|
||||
|
||||
// Reporter case (Azd325): the install root ITSELF is a symlink. The pre-#2393
|
||||
// guard had an early-return for this before the component loop even ran.
|
||||
test('root-is-symlink layout (Azd325/nix-darwin): default refuses, opt-in allows', (t) => {
|
||||
const dotfilesTarget = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-dot-'));
|
||||
const rootLink = path.join(os.tmpdir(), 'gsd-2393-rootlink-' + Date.now());
|
||||
try {
|
||||
try {
|
||||
fs.symlinkSync(dotfilesTarget, rootLink);
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
// Inside the dotfiles target, skills is a real dir (not a symlink).
|
||||
fs.mkdirSync(path.join(dotfilesTarget, 'skills'), { recursive: true });
|
||||
const destDir = path.join(rootLink, 'skills', 'gsd-foo');
|
||||
|
||||
// Default: refuse (root itself is a symlink → early-return true).
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(rootLink, destDir),
|
||||
true,
|
||||
'default must refuse when install root itself is a symlink',
|
||||
);
|
||||
|
||||
// Opt-in: follow the root symlink, walk to the real skills dir — allow.
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(rootLink, destDir, { allowOptInFollow: true }),
|
||||
false,
|
||||
'GSD_ALLOW_SYMLINKED_DEST=1 must follow a root symlink whose target has no further symlinks',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(rootLink); } catch { /* already gone */ }
|
||||
cleanup(dotfilesTarget);
|
||||
}
|
||||
});
|
||||
|
||||
// Load-bearing refusal (a): path-traversal in the destSubpath string itself.
|
||||
// MUST refuse regardless of opt-in — this is the #1704 threat (a).
|
||||
test('path-traversal destSubpath ("../../etc") refused EVEN WITH opt-in', () => {
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-trav-'));
|
||||
try {
|
||||
const escapePath = path.join(configHome, '..', '..', 'etc-passwd-' + Date.now());
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, escapePath, { allowOptInFollow: true }),
|
||||
true,
|
||||
'path-traversal destSubpath must ALWAYS refuse regardless of opt-in (#1704 threat a)',
|
||||
);
|
||||
} finally {
|
||||
cleanup(configHome);
|
||||
}
|
||||
});
|
||||
|
||||
// Load-bearing refusal (b): a symlink whose resolved target equals the install root
|
||||
// itself would let _removeGsdEntries wipe the root. MUST refuse regardless of opt-in.
|
||||
test('resolved-target-equals-install-root refused EVEN WITH opt-in (wipe protection)', (t) => {
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-wipe-'));
|
||||
try {
|
||||
// Symlink configHome/loop -> configHome (circular). Resolved target == install root.
|
||||
const loopLink = path.join(configHome, 'loop');
|
||||
try {
|
||||
fs.symlinkSync(configHome, loopLink);
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
const destDir = path.join(loopLink, 'gsd-foo');
|
||||
|
||||
// Default refuses.
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir),
|
||||
true,
|
||||
'default must refuse symlink to install root (wipe protection)',
|
||||
);
|
||||
|
||||
// Opt-in STILL refuses — this is threat (b), load-bearing even with opt-in.
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir, { allowOptInFollow: true }),
|
||||
true,
|
||||
'opt-in must NOT allow a symlink resolving to install root itself (#1704 threat b — wipe)',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(path.join(configHome, 'loop')); } catch { /* already gone */ }
|
||||
cleanup(configHome);
|
||||
}
|
||||
});
|
||||
|
||||
// #2393 security-review finding: realpathSync fully resolves symlinks while
|
||||
// path.resolve is lexical. On macOS, /var is a symlink to /private/var, so
|
||||
// `resolvedRoot` carries `/var/...` while the symlink's realtarget carries
|
||||
// `/private/var/...` — a naive `realtarget === resolvedRoot` check would miss
|
||||
// the equality and let threat (b) through. Fix compares against BOTH the
|
||||
// lexical and real forms of root. Test constructs the macOS-style divergence
|
||||
// explicitly: spell configHome one way, point the symlink at its real path.
|
||||
test('resolved-target-equals-install-root via /var ↔ /private/var normalization (macOS-style)', (t) => {
|
||||
if (process.platform !== 'darwin') {
|
||||
t.skip('test exercises the macOS /var → /private/var symlink — darwin only');
|
||||
return;
|
||||
}
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-realpath-'));
|
||||
try {
|
||||
// `os.tmpdir()` is spelled with `/var/...` on macOS; realpathSync resolves it
|
||||
// to `/private/var/...`. The lexical resolvedRoot and the real realRoot differ.
|
||||
const realConfigHome = fs.realpathSync(configHome);
|
||||
if (realConfigHome === configHome) {
|
||||
// Defensive — if for some reason there's no /var symlink in the chain, the
|
||||
// test isn't exercising what it claims. Skip rather than pass vacuously.
|
||||
t.skip('os.tmpdir() path contains no symlink component — test does not exercise the /var normalization');
|
||||
return;
|
||||
}
|
||||
|
||||
// Symlink spelled via the REAL path — its realtarget will equal realConfigHome,
|
||||
// NOT lexical configHome. The bug shape: realtarget !== resolvedRoot (lexical).
|
||||
const loopLink = path.join(configHome, 'loop');
|
||||
try {
|
||||
fs.symlinkSync(realConfigHome, loopLink);
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
const destDir = path.join(loopLink, 'gsd-foo');
|
||||
|
||||
// The fix compares against BOTH lexical and real forms — guard fires.
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir, { allowOptInFollow: true }),
|
||||
true,
|
||||
'opt-in must refuse a symlink resolving to install root by real path even when ' +
|
||||
'lexical and real forms differ (macOS /var ↔ /private/var normalization)',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(path.join(configHome, 'loop')); } catch { /* already gone */ }
|
||||
cleanup(configHome);
|
||||
}
|
||||
});
|
||||
|
||||
// Documented edge case: a broken symlink (target missing) is silently passed by
|
||||
// both the default and opt-in paths. fs.existsSync follows the link and returns
|
||||
// false, so the component loop terminates before the symlink check fires. This is
|
||||
// pre-existing behavior — the fix preserves it. Subsequent mkdir may then fail or
|
||||
// create the path through the resolved target; that's the caller's responsibility,
|
||||
// not the symlink-escape guard's. Test pins the current behavior so any future
|
||||
// change (e.g. switching to lstatSync for existence) is intentional.
|
||||
test('broken symlink: silently passed (current behavior, preserved by fix)', (t) => {
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-broken-'));
|
||||
try {
|
||||
const danglingLink = path.join(configHome, 'skills');
|
||||
const notPresentTarget = path.join(os.tmpdir(), 'gsd-2393-not-present-' + Date.now());
|
||||
try {
|
||||
fs.symlinkSync(notPresentTarget, danglingLink);
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
const destDir = path.join(danglingLink, 'gsd-foo');
|
||||
|
||||
// existsSync(danglingLink) follows the link → false → loop returns false early.
|
||||
// Same behavior with and without opt-in. Test documents this so a future
|
||||
// refactor (e.g. lstatSync-based existence) is a deliberate behavior change.
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir),
|
||||
false,
|
||||
'broken symlink: component loop terminates early (existsSync follows link → false)',
|
||||
);
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir, { allowOptInFollow: true }),
|
||||
false,
|
||||
'broken symlink with opt-in: same early-termination behavior',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(path.join(configHome, 'skills')); } catch { /* already gone */ }
|
||||
cleanup(configHome);
|
||||
}
|
||||
});
|
||||
|
||||
// Reviewer-driven (Medium): transitive symlink chains. The opt-in is transitive
|
||||
// and unbounded by design — once a symlink is followed, the walk continues from
|
||||
// the resolved real path WITHOUT re-checking that further segments stay inside
|
||||
// a confining boundary. Test pins the documented behavior so a future change is
|
||||
// deliberate. (Default behavior refuses at the first symlink.)
|
||||
test('transitive symlink chain: opt-in follows transitively; default refuses at first hop', (t) => {
|
||||
const configHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-trans-'));
|
||||
const outside1 = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-t1-'));
|
||||
const outside2 = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2393-t2-'));
|
||||
try {
|
||||
// configHome/outer -> outside1, outside1/inner -> outside2 (two-hop chain).
|
||||
try {
|
||||
fs.symlinkSync(outside1, path.join(configHome, 'outer'));
|
||||
fs.symlinkSync(outside2, path.join(outside1, 'inner'));
|
||||
} catch (_e) {
|
||||
t.skip('symlink creation unsupported on this platform/privilege');
|
||||
return;
|
||||
}
|
||||
const destDir = path.join(configHome, 'outer', 'inner', 'gsd-foo');
|
||||
|
||||
// Default: refuses at the first hop (configHome/outer is a symlink).
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir),
|
||||
true,
|
||||
'default must refuse at the first symlink (configHome/outer)',
|
||||
);
|
||||
|
||||
// Opt-in: follows transitively through both hops to outside2 (no threat-(b)
|
||||
// match — outside2 is neither lexical nor real form of configHome).
|
||||
assert.strictEqual(
|
||||
hasExistingSymlinkBetween(configHome, destDir, { allowOptInFollow: true }),
|
||||
false,
|
||||
'opt-in must follow transitive chain (outer → outside1 → outside2/inner) — documented transitivity',
|
||||
);
|
||||
} finally {
|
||||
try { fs.unlinkSync(path.join(configHome, 'outer')); } catch { /* already gone */ }
|
||||
try { fs.unlinkSync(path.join(outside1, 'inner')); } catch { /* already gone */ }
|
||||
cleanup(configHome);
|
||||
cleanup(outside1);
|
||||
cleanup(outside2);
|
||||
}
|
||||
});
|
||||
|
||||
// Reviewer-driven (Medium): isSymlinkedDestOptIn env-var parsing is itself
|
||||
// behavioral — a typo in the env-var name or an accepted-values change would
|
||||
// silently disable the opt-in. Pin the contract directly via the exported helper.
|
||||
test('isSymlinkedDestOptIn: accepts only documented values (1, true)', () => {
|
||||
const installEngine = require('../gsd-core/bin/lib/install-engine.cjs');
|
||||
if (typeof installEngine.isSymlinkedDestOptIn !== 'function') {
|
||||
// Skipping — helper not exported in this build (assertion-only test).
|
||||
return;
|
||||
}
|
||||
const cases = [
|
||||
{ v: '1', expected: true },
|
||||
{ v: 'true', expected: true },
|
||||
{ v: 'TRUE', expected: false }, // only lowercase 'true' documented
|
||||
{ v: 'True', expected: false },
|
||||
{ v: 'yes', expected: false },
|
||||
{ v: 'on', expected: false },
|
||||
{ v: '0', expected: false },
|
||||
{ v: 'false', expected: false },
|
||||
{ v: '', expected: false },
|
||||
{ v: undefined, expected: false }, // unset
|
||||
];
|
||||
for (const { v, expected } of cases) {
|
||||
if (v === undefined) delete process.env.GSD_ALLOW_SYMLINKED_DEST;
|
||||
else process.env.GSD_ALLOW_SYMLINKED_DEST = v;
|
||||
assert.strictEqual(
|
||||
installEngine.isSymlinkedDestOptIn(),
|
||||
expected,
|
||||
`GSD_ALLOW_SYMLINKED_DEST=${JSON.stringify(v)} should yield isSymlinkedDestOptIn()=${expected}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user