fix(#5037): refuse or write through a symlinked settings.json

.changeset/proud-cats-click.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 5300
+---
+**Installs, updates and uninstalls no longer replace a symlinked `settings.json` with a plain file** — without `GSD_ALLOW_SYMLINKED_DEST` they stop with a message naming it, and with it the write goes through to the link's target. (#5037)

docs/CONFIGURATION.md

@@ -2286,7 +2286,7 @@ Use `provider: "generic"` (or `"custom"`) for OpenRouter, LiteLLM, local gateway
 | `GSD_PROJECT` | Override project root for multi-project workspace support (v1.32) |
 | `GSD_SKIP_SCHEMA_CHECK` | Skip schema drift detection during execute-phase (v1.31) |
 | `GSD_EXIT_CONTRACT` | Select the exit-code projection: `v1` (default) or `v2`. See [Exit-code contract](#exit-code-contract-gsd_exit_contract) below. |
-| `GSD_ALLOW_SYMLINKED_DEST` | Set to `1` (or `true`) to permit install/update when `CLAUDE_CONFIG_DIR` (or any artifact-kind child like `skills/`, `hooks/`) is an **intentional, user-owned symlink** pointing outside the install root. v1.7.x write-confinement (ADR-1239 Phase B) refuses such layouts by default to prevent untrusted `destSubpath` traversal. Opt in only if you manage configHome via symlinked external dirs, multi-account config layouts (`~/.claude-personal`, `~/.claude-team`), or dotfiles-managed configHome (nix-darwin, etc.). Two refusals remain load-bearing even with opt-in: path-traversal in `destSubpath` (`../../etc`-style), and a symlink whose resolved target equals the install root itself (would let the prune pass wipe it). |
+| `GSD_ALLOW_SYMLINKED_DEST` | Set to `1` (or `true`) to permit install/update when `CLAUDE_CONFIG_DIR` (or any artifact-kind child like `skills/`, `hooks/`) is an **intentional, user-owned symlink** pointing outside the install root. v1.7.x write-confinement (ADR-1239 Phase B) refuses such layouts by default to prevent untrusted `destSubpath` traversal. Opt in only if you manage configHome via symlinked external dirs, multi-account config layouts (`~/.claude-personal`, `~/.claude-team`), or dotfiles-managed configHome (nix-darwin, etc.). A symlinked settings file (for example a dotfiles-managed `settings.json`) follows the same rule: without the opt-in the install refuses to write it, and with the opt-in GSD writes to the link's target and leaves the link in place. A settings link whose target is missing is refused either way rather than replaced. Two refusals remain load-bearing even with opt-in: path-traversal in `destSubpath` (`../../etc`-style), and a symlink whose resolved target equals the install root itself (would let the prune pass wipe it). |
 | `NODE_DISABLE_COMPILE_CACHE` | Set (to any value) to turn off Node's compile cache, which `gsd-tools` and the hooks that load `gsd-core/bin/lib` enable to start faster. The cache lives in `node-compile-cache` under the OS temp directory, or in `NODE_COMPILE_CACHE` if you set it. Node reads both variables itself. |
 | `WSL_DISTRO_NAME` | Detected by installer for WSL path handling |
 

src/install-engine.cts

@@ -471,6 +471,65 @@ function hasExistingSymlinkBetween(
   return false;
 }
 
+/**
+ * Whether a symlink's target is genuinely absent. Only ENOENT and ENOTDIR mean
+ * that: a link loop (ELOOP) or an unreadable directory (EACCES) also fail to
+ * resolve, but saying "the target does not exist" there would be wrong.
+ */
+function linkTargetIsMissing(linkPath: string): boolean {
+  try {
+    installFs().realpathSync(linkPath);
+    return false;
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException).code;
+    return code === 'ENOENT' || code === 'ENOTDIR';
+  }
+}
+
+/**
+ * The path a temp-file-and-rename save (`atomicWriteFileSync`,
+ * `platformWriteSync`) should be handed for `targetPath`, a file under
+ * `installRoot` that may be a user-owned symlink (#5037).
+ *
+ * A rename onto a symlink replaces the link with a plain file, so the user's
+ * real file silently stops receiving changes. A symlinked `targetPath` is
+ * therefore refused unless `GSD_ALLOW_SYMLINKED_DEST` is set, and with the
+ * opt-in the link's real target is returned, so the rename lands there and the
+ * link stays in place. A plain or missing file is returned unchanged. A
+ * dangling link is refused either way (`tryLstat` sees it where `existsSync`
+ * would not), the same as `hasExistingSymlinkBetween` treats one.
+ *
+ * `label` starts the refusal message and names the writer, e.g.
+ * `writeSettings: settings file`. When the opt-in is already set, telling the
+ * user to re-run with it would send them in a circle, so those refusals say
+ * what is actually wrong instead: the link's target is missing, or it resolves
+ * to the install root itself (which `hasExistingSymlinkBetween` always refuses).
+ */
+function resolveSymlinkedLeafWritePath(installRoot: string, targetPath: string, label: string): string {
+  const stat = tryLstat(targetPath);
+  if (!stat || !stat.isSymbolicLink()) return targetPath;
+  const optIn = isSymlinkedDestOptIn();
+  if (hasExistingSymlinkBetween(installRoot, targetPath, { allowOptInFollow: optIn })) {
+    if (optIn && linkTargetIsMissing(targetPath)) {
+      throw new Error(
+        `${label} "${targetPath}" is a symlink whose target does not exist, so it was not written. ` +
+          'Point the link at an existing file or remove it, then re-run.',
+      );
+    }
+    if (optIn) {
+      throw new Error(
+        `${label} "${targetPath}" is a symlink GSD will not follow even with GSD_ALLOW_SYMLINKED_DEST set, ` +
+          `because it resolves to the install root "${installRoot}" itself or cannot be resolved, so it was not written.`,
+      );
+    }
+    throw new Error(
+      `${label} "${targetPath}" is a symlink the install root "${installRoot}" does not trust, so it was not written. ` +
+        'If this is an intentional user-owned symlink layout, re-run with GSD_ALLOW_SYMLINKED_DEST=1.',
+    );
+  }
+  return installFs().realpathSync(targetPath);
+}
+
 // ---------------------------------------------------------------------------
 // User-artifact staging root
 // ---------------------------------------------------------------------------
@@ -2210,6 +2269,7 @@ export = {
   _copyStaged,
   hasExistingSymlinkBetween,
   isSymlinkedDestOptIn,
+  resolveSymlinkedLeafWritePath,
   _resolveUserArtifactStagingRoot,
   _tryResolveUserArtifactStagingRoot,
   migrateLegacyDevPreferencesToSkill,

src/runtime-hooks-surface.cts

@@ -13,6 +13,7 @@
  *   Codex hooks.json: ensureCodexHooksJsonSessionStart, ensureCodexHooksJsonEvent,
  *            reconcileCodexHooksJsonEvent, reconcileCodexHooksJsonSessionStart,
  *            removeCodexHooksJsonEvent, removeCodexHooksJsonSessionStart,
+ *            codexHooksJsonHasManagedHooks,
  *            buildCodexHookWindowsShimIR, buildCodexHookBlock, rewriteLegacyCodexHookBlock
  *   Shared:  buildHookCommand, rewriteLegacyManagedNodeHookCommands
  *
@@ -949,14 +950,14 @@ interface ReconcileResult {
 }
 
 /**
- * Lazily require install-engine.cjs's `hasExistingSymlinkBetween` /
- * `isSymlinkedDestOptIn` — mirrors user-artifact-staging.cts's
- * `_installEngineSymlinkGuard` (same call-time-require rationale: avoid a
- * static circular require between install-engine.cts and this module).
+ * Lazily require install-engine.cjs's `resolveSymlinkedLeafWritePath`, which
+ * applies the `hasExistingSymlinkBetween` / `isSymlinkedDestOptIn` policy to a
+ * single file. Mirrors user-artifact-staging.cts's `_installEngineSymlinkGuard`
+ * (same call-time-require rationale: avoid a static circular require between
+ * install-engine.cts and this module).
  */
 interface InstallEngineSymlinkGuard {
-  hasExistingSymlinkBetween: (root: string, fullPath: string, options?: { allowOptInFollow?: boolean }) => boolean;
-  isSymlinkedDestOptIn: () => boolean;
+  resolveSymlinkedLeafWritePath: (installRoot: string, targetPath: string, label: string) => string;
 }
 
 function _installEngineSymlinkGuard(): InstallEngineSymlinkGuard {
@@ -965,47 +966,41 @@ function _installEngineSymlinkGuard(): InstallEngineSymlinkGuard {
   return mod;
 }
 
+/**
+ * Whether one hook object in a Codex hooks.json event entry is GSD's. The
+ * removal in reconcileCodexHooksJsonEvent and the read-only
+ * codexHooksJsonHasManagedHooks both ask this, so they agree on what a
+ * removal would delete.
+ */
+function isManagedCodexHooksJsonHook(hook: unknown, targetDir: string): boolean {
+  const cmd = hook && typeof hook === 'object' ? (hook as Record<string, unknown>)['command'] : null;
+  return isManagedHookCommand(cmd as string | null | undefined, {
+    surface: 'codex-hooks-json',
+    includeLegacyAliases: true,
+    configDir: targetDir,
+  });
+}
+
 function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts: ReconcileCodexOpts = {}): ReconcileResult {
   const hooksJsonPath = path.join(targetDir, 'hooks.json');
   const managedCommand = typeof opts.managedCommand === 'string' ? opts.managedCommand : null;
   const commandWindows = typeof opts.commandWindows === 'string' ? opts.commandWindows : null;
   const matcher = typeof opts.matcher === 'string' ? opts.matcher : undefined;
   const timeout = typeof opts.timeout === 'number' ? opts.timeout : undefined;
   // #2586 Major 2: every Codex hooks.json writer funnels through this one
-  // function, and atomicWriteFileSync's final step is a rename(2) onto
-  // `hooksJsonPath` — which, when that path is a symlink, REPLACES the
-  // symlink with a plain file rather than writing through it. Refuse (with
-  // the same GSD_ALLOW_SYMLINKED_DEST opt-in every other install call site
-  // honors) before reading or writing, so a symlinked hooks.json is neither
-  // silently destroyed nor left the caller no escape hatch.
-  const symlinkGuard = _installEngineSymlinkGuard();
-  // The path this function actually reads/writes. Defaults to the nominal
-  // hooks.json path; reassigned below to the symlink's real target when the
-  // opt-in is active, so the write lands on the file the user's symlink
-  // points at instead of clobbering the symlink itself (see note below).
-  let effectiveHooksJsonPath = hooksJsonPath;
-  if (fs.existsSync(hooksJsonPath) && fs.lstatSync(hooksJsonPath).isSymbolicLink()) {
-    if (
-      symlinkGuard.hasExistingSymlinkBetween(targetDir, hooksJsonPath, {
-        allowOptInFollow: symlinkGuard.isSymlinkedDestOptIn(),
-      })
-    ) {
-      throw new Error(
-        `hooks.json at "${hooksJsonPath}" contains a symlink the install root "${targetDir}" does not trust — ` +
-          'refusing to read or write it. If this is an intentional user-owned symlink layout, re-run with ' +
-          'GSD_ALLOW_SYMLINKED_DEST=1.',
-      );
-    }
-    // hasExistingSymlinkBetween returned false only because the opt-in is
-    // active (a symlinked leaf always trips it otherwise) — so this IS a
-    // symlink and we are cleared to follow it. atomicWriteFileSync's final
-    // step is a rename(2) onto its target, which REPLACES an existing
-    // symlink at that path rather than writing through it; resolving to the
-    // real path here makes the read AND the write operate on the symlink's
-    // target, leaving the symlink itself untouched, matching what "follow"
-    // is supposed to mean.
-    effectiveHooksJsonPath = fs.realpathSync(hooksJsonPath);
-  }
+  // function, and atomicWriteFileSync's final step is a rename(2) onto its
+  // target, which replaces a symlink at that path with a plain file instead
+  // of writing through it. install-engine's resolveSymlinkedLeafWritePath,
+  // shared with writeSettings (#5037), refuses a symlinked hooks.json unless
+  // GSD_ALLOW_SYMLINKED_DEST is set, and with the opt-in returns the link's
+  // real target, so the read AND the write below operate on the file the
+  // user's symlink points at and the link itself stays in place. A dangling
+  // hooks.json link is refused rather than replaced, the same as writeSettings.
+  const effectiveHooksJsonPath = _installEngineSymlinkGuard().resolveSymlinkedLeafWritePath(
+    targetDir,
+    hooksJsonPath,
+    'hooks.json at',
+  );
   let parsed: Record<string, unknown> = {};
   let currentContent: string | null = null;
   if (fs.existsSync(effectiveHooksJsonPath)) {
@@ -1060,12 +1055,7 @@ function reconcileCodexHooksJsonEvent(targetDir: string, eventName: string, opts
       continue;
     }
     const keptHooks = originalHooks.filter((hook) => {
-      const cmd = hook && typeof hook === 'object' ? (hook as Record<string, unknown>)['command'] : null;
-      const managed = isManagedHookCommand(cmd as string | null | undefined, {
-        surface: 'codex-hooks-json',
-        includeLegacyAliases: true,
-        configDir: targetDir,
-      });
+      const managed = isManagedCodexHooksJsonHook(hook, targetDir);
       if (managed) removedLegacy = true;
       return !managed;
     });
@@ -1302,6 +1292,49 @@ function removeCodexHooksJsonSessionStart(targetDir: string): ReconcileResult {
   return reconcileCodexHooksJsonSessionStart(targetDir, { managedCommand: null });
 }
 
+/**
+ * Read-only: whether removing `eventNames` from `targetDir`'s hooks.json would
+ * delete a GSD hook. The removal writes hooks.json through the symlink policy
+ * (#5037), which refuses a symlinked file without GSD_ALLOW_SYMLINKED_DEST and
+ * a dangling one either way, so uninstall asks this first and leaves alone a
+ * hooks.json it has nothing to remove from. A dangling link reads as absent.
+ * Entries are found where the removal finds them, under the nested `hooks`
+ * table or as a legacy top-level event array. A file that cannot be read or
+ * parsed counts as holding GSD hooks, so the removal still runs and reports
+ * the parse error as before.
+ */
+function codexHooksJsonHasManagedHooks(targetDir: string, eventNames: readonly string[]): boolean {
+  const hooksJsonPath = path.join(targetDir, 'hooks.json');
+  if (!fs.existsSync(hooksJsonPath)) return false;
+  let parsed: unknown;
+  try {
+    const raw = fs.readFileSync(hooksJsonPath, 'utf8');
+    if (!raw.trim()) return false;
+    parsed = JSON.parse(raw);
+  } catch {
+    return true;
+  }
+  if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return false;
+  const root = parsed as Record<string, unknown>;
+  const nested = root['hooks'] && typeof root['hooks'] === 'object' && !Array.isArray(root['hooks'])
+    ? (root['hooks'] as Record<string, unknown>)
+    : {};
+  for (const eventName of eventNames) {
+    for (const table of [root, nested]) {
+      const entries = table[eventName];
+      if (!Array.isArray(entries)) continue;
+      for (const entry of entries) {
+        if (!entry || typeof entry !== 'object' || Array.isArray(entry)) continue;
+        const hooks = (entry as Record<string, unknown>)['hooks'];
+        if (Array.isArray(hooks) && hooks.some((hook) => isManagedCodexHooksJsonHook(hook, targetDir))) {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 // ---------------------------------------------------------------------------
 // #2586: cleanupOrphanedCodexContextMonitorScript
 // ---------------------------------------------------------------------------
@@ -3299,6 +3332,7 @@ export = {
   ensureCodexHooksJsonEvent,
   removeCodexHooksJsonEvent,
   removeCodexHooksJsonSessionStart,
+  codexHooksJsonHasManagedHooks,
   buildCodexHookWindowsShimIR,
   cleanupOrphanedCodexContextMonitorScript,
   isGsdOwnedCodexContextMonitorScript,