cherry-pick(security): harden project-controlled surfaces (PR #4755 partial)

Cherry-pick of gsd-build/gsd-2 65ca5aa2e — applies the security hardening
hunks that conflicted minimally:

- mcp-server/env-writer: validate writes against a strict allowlist
- web/api/files: enforce path containment via web/lib/secure-path
- vscode-extension: read binaryPath/autoStart only from trusted
  global/default scopes (resolveTrustedSfStartupConfig), avoiding
  workspace-controlled override (renamed Gsd → Sf for sf naming)
- New regression tests: mcp-client-security, vscode-startup-security,
  web-files-symlink

Skipped hunks (drifted): mcp-server/server.ts, mcp-client/index.ts,
mcp-server/README.md.

Co-Authored-By: Jeremy <jeremy@fluxlabs.net>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

packages/mcp-server/src/env-writer.test.ts

@@ -3,7 +3,7 @@
 
 import { describe, it, afterEach } from 'node:test';
 import assert from 'node:assert/strict';
-import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync } from 'node:fs';
+import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync, realpathSync, symlinkSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 
@@ -14,6 +14,7 @@ import {
   applySecrets,
   isSafeEnvVarKey,
   isSupportedDeploymentEnvironment,
+  resolveProjectEnvFilePath,
   shellEscapeSingle,
 } from './env-writer.js';
 
@@ -181,6 +182,83 @@ describe('writeEnvKey', () => {
       rmSync(tmp, { recursive: true, force: true });
     }
   });
+
+  it('does not follow symlinked env files when writing', async () => {
+    const tmp = makeTempDir('write');
+    const outside = makeTempDir('write-outside');
+    try {
+      const outsideEnv = join(outside, '.env');
+      writeFileSync(outsideEnv, 'SECRET=outside\n');
+      symlinkSync(outsideEnv, join(tmp, '.env'));
+
+      await assert.rejects(
+        () => writeEnvKey(join(tmp, '.env'), 'SECRET', 'inside'),
+        /ELOOP|symbolic link|symlink/i,
+      );
+      assert.equal(readFileSync(outsideEnv, 'utf8'), 'SECRET=outside\n');
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+      rmSync(outside, { recursive: true, force: true });
+    }
+  });
+});
+
+// ---------------------------------------------------------------------------
+// resolveProjectEnvFilePath
+// ---------------------------------------------------------------------------
+
+describe('resolveProjectEnvFilePath', () => {
+  it('allows .env under the project root', () => {
+    const tmp = makeTempDir('env-path');
+    try {
+      assert.equal(resolveProjectEnvFilePath(tmp, '.env'), join(realpathSync.native(tmp), '.env'));
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+
+  it('rejects envFilePath outside the project root', () => {
+    const tmp = makeTempDir('env-path');
+    try {
+      assert.throws(
+        () => resolveProjectEnvFilePath(tmp, '../outside.env'),
+        /inside the project directory/,
+      );
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+
+  it('rejects symlinked parent directories that escape the project root', () => {
+    const tmp = makeTempDir('env-path');
+    const outside = makeTempDir('env-path-outside');
+    try {
+      symlinkSync(outside, join(tmp, 'linked-outside'), 'dir');
+      assert.throws(
+        () => resolveProjectEnvFilePath(tmp, 'linked-outside/.env'),
+        /inside the project directory/,
+      );
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+      rmSync(outside, { recursive: true, force: true });
+    }
+  });
+
+  it('rejects existing env files that are symlinks outside the project root', () => {
+    const tmp = makeTempDir('env-path');
+    const outside = makeTempDir('env-path-outside');
+    try {
+      writeFileSync(join(outside, '.env'), 'SECRET=outside\n');
+      symlinkSync(join(outside, '.env'), join(tmp, '.env'));
+      assert.throws(
+        () => resolveProjectEnvFilePath(tmp, '.env'),
+        /inside the project directory/,
+      );
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+      rmSync(outside, { recursive: true, force: true });
+    }
+  });
 });
 
 // ---------------------------------------------------------------------------

packages/mcp-server/src/env-writer.ts

@@ -5,9 +5,9 @@
 // destinations, and checking existing keys. Used by secure_env_collect
 // MCP tool. No TUI dependencies — pure filesystem + process.env operations.
 
-import { readFile, writeFile } from "node:fs/promises";
-import { existsSync, statSync } from "node:fs";
-import { resolve } from "node:path";
+import { open, readFile, rename, rm } from "node:fs/promises";
+import { constants, existsSync, lstatSync, realpathSync, statSync } from "node:fs";
+import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path";
 
 // ---------------------------------------------------------------------------
 // checkExistingEnvKeys
@@ -71,10 +71,19 @@ export async function writeEnvKey(filePath: string, key: string, value: string):
   if (typeof value !== "string") {
     throw new TypeError(`writeEnvKey expects a string value for key "${key}", got ${typeof value}`);
   }
+  assertWritableEnvFileTarget(filePath);
   let content = "";
   try {
-    content = await readFile(filePath, "utf8");
-  } catch {
+    const handle = await open(filePath, constants.O_RDONLY | constants.O_NOFOLLOW);
+    try {
+      content = await handle.readFile("utf8");
+    } finally {
+      await handle.close();
+    }
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code !== "ENOENT") {
+      throw err;
+    }
     content = "";
   }
   const escaped = value.replace(/\\/g, "\\\\").replace(/\n/g, "\\n").replace(/\r/g, "");
@@ -86,7 +95,42 @@ export async function writeEnvKey(filePath: string, key: string, value: string):
     if (content.length > 0 && !content.endsWith("\n")) content += "\n";
     content += `${line}\n`;
   }
-  await writeFile(filePath, content, "utf8");
+  const tempPath = join(
+    dirname(filePath),
+    `.${basename(filePath)}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp`,
+  );
+  let handle: Awaited<ReturnType<typeof open>> | undefined;
+  try {
+    handle = await open(tempPath, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL, 0o600);
+    await handle.writeFile(content, "utf8");
+    await handle.close();
+    handle = undefined;
+    assertWritableEnvFileTarget(filePath);
+    await rename(tempPath, filePath);
+  } catch (err) {
+    if (handle) {
+      try {
+        await handle.close();
+      } catch {
+        // Best-effort cleanup.
+      }
+    }
+    await rm(tempPath, { force: true }).catch(() => undefined);
+    throw err;
+  }
+}
+
+function assertWritableEnvFileTarget(filePath: string): void {
+  try {
+    if (lstatSync(filePath).isSymbolicLink()) {
+      throw new Error("Refusing to write symlinked env file");
+    }
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+      return;
+    }
+    throw err;
+  }
 }
 
 // ---------------------------------------------------------------------------
@@ -101,6 +145,32 @@ export function isSupportedDeploymentEnvironment(env: string): boolean {
   return env === "development" || env === "preview" || env === "production";
 }
 
+function isWithinProjectRoot(projectRoot: string, candidatePath: string): boolean {
+  const rel = relative(projectRoot, candidatePath);
+  return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel));
+}
+
+export function resolveProjectEnvFilePath(projectDir: string, envFilePath = ".env"): string {
+  const projectRoot = realpathSync.native(resolve(projectDir));
+  const candidate = resolve(projectRoot, envFilePath);
+  if (!isWithinProjectRoot(projectRoot, candidate)) {
+    throw new Error("envFilePath must resolve inside the project directory");
+  }
+  if (existsSync(candidate)) {
+    const targetRealPath = realpathSync.native(candidate);
+    if (isWithinProjectRoot(projectRoot, targetRealPath)) {
+      return candidate;
+    }
+    throw new Error("envFilePath must resolve inside the project directory");
+  }
+  const candidateParent = dirname(candidate);
+  const parentRealPath = realpathSync.native(candidateParent);
+  if (isWithinProjectRoot(projectRoot, parentRealPath)) {
+    return candidate;
+  }
+  throw new Error("envFilePath must resolve inside the project directory");
+}
+
 // ---------------------------------------------------------------------------
 // Shell helpers (for vercel/convex CLI)
 // ---------------------------------------------------------------------------

src/resources/extensions/sf/sf-db.ts

@@ -1960,23 +1960,6 @@ function parseTaskArrayColumn(raw: unknown): string[] {
   }
 }
 
-function parseTaskArrayColumn(raw: unknown): string[] {
-  if (typeof raw !== "string" || raw.trim() === "") return [];
-
-  try {
-    const parsed = JSON.parse(raw);
-    if (Array.isArray(parsed)) return parsed.map((value) => String(value));
-    if (parsed === null || parsed === undefined || parsed === "") return [];
-    return [String(parsed)];
-  } catch {
-    // Older/corrupt rows may contain comma-separated strings instead of JSON.
-    return raw
-      .split(",")
-      .map((value) => value.trim())
-      .filter(Boolean);
-  }
-}
-
 function rowToTask(row: Record<string, unknown>): TaskRow {
   const parseTaskArray = (value: unknown): string[] => {
     if (Array.isArray(value)) {

src/resources/extensions/sf/tests/mcp-client-security.test.ts

@@ -0,0 +1,76 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+
+import {
+  _buildMcpChildEnvForTest,
+  _buildMcpTrustConfirmOptionsForTest,
+} from "../../mcp-client/index.ts";
+
+test("MCP stdio child env only includes safe inherited keys plus explicit config env", () => {
+  const previousSecret = process.env.SECRET_MCP_TEST_TOKEN;
+  const previousPath = process.env.PATH;
+  try {
+    process.env.SECRET_MCP_TEST_TOKEN = "should-not-leak";
+    process.env.PATH = "/usr/bin";
+
+    const env = _buildMcpChildEnvForTest({
+      EXPLICIT_TOKEN: "${SECRET_MCP_TEST_TOKEN}",
+      PLAIN_VALUE: "ok",
+    });
+
+    assert.equal(env.PATH, "/usr/bin");
+    assert.equal(env.SECRET_MCP_TEST_TOKEN, undefined);
+    assert.equal(env.EXPLICIT_TOKEN, "should-not-leak");
+    assert.equal(env.PLAIN_VALUE, "ok");
+  } finally {
+    if (previousSecret === undefined) delete process.env.SECRET_MCP_TEST_TOKEN;
+    else process.env.SECRET_MCP_TEST_TOKEN = previousSecret;
+    if (previousPath === undefined) delete process.env.PATH;
+    else process.env.PATH = previousPath;
+  }
+});
+
+test("MCP stdio trust confirmation is abort-aware", () => {
+  const controller = new AbortController();
+  const options = _buildMcpTrustConfirmOptionsForTest(controller.signal);
+
+  assert.equal(options.timeout, 120_000);
+  assert.equal(options.signal, controller.signal);
+});
+
+test("MCP client uses a single in-flight connection per canonical server", () => {
+  const source = readFileSync(new URL("../../mcp-client/index.ts", import.meta.url), "utf8");
+
+  assert.match(source, /const pendingConnections = new Map<string, Promise<Client>>\(\)/);
+  assert.match(source, /const pending = pendingConnections\.get\(config\.name\)/);
+  assert.match(source, /pendingConnections\.set\(config\.name, connectionPromise\)/);
+  assert.match(source, /pendingConnections\.delete\(config\.name\)/);
+  assert.match(source, /env: config\.env \?\? \{\}/);
+});
+
+test("MCP stdio trust is persisted only after a successful connection", () => {
+  const source = readFileSync(new URL("../../mcp-client/index.ts", import.meta.url), "utf8");
+  const connectIndex = source.indexOf("await client.connect(transport");
+  const trustIndex = source.indexOf("trustedStdioServers.add(approvedTrustKey)");
+
+  assert.ok(connectIndex > -1, "connectServer should await client.connect");
+  assert.ok(trustIndex > connectIndex, "trust should be recorded after client.connect succeeds");
+  assert.doesNotMatch(source, /assertTrustedStdioServer[\s\S]*trustedStdioServers\.add\(trustKey\)/);
+});
+
+test("MCP client closes transports after failed connection attempts", () => {
+  const source = readFileSync(new URL("../../mcp-client/index.ts", import.meta.url), "utf8");
+
+  assert.match(source, /catch \(err\) \{[\s\S]*await transport\.close\(\)/);
+  assert.match(source, /catch \(err\) \{[\s\S]*await client\.close\(\)/);
+  assert.match(source, /catch \(err\) \{[\s\S]*throw err/);
+});
+
+test("MCP client clears process-local trust and closes transports on session cleanup", () => {
+  const source = readFileSync(new URL("../../mcp-client/index.ts", import.meta.url), "utf8");
+
+  assert.match(source, /async function closeAll\(\)[\s\S]*await conn\.transport\.close\(\)/);
+  assert.match(source, /async function closeAll\(\)[\s\S]*pendingConnections\.clear\(\)/);
+  assert.match(source, /async function closeAll\(\)[\s\S]*trustedStdioServers\.clear\(\)/);
+});

src/tests/vscode-startup-security.test.ts

@@ -0,0 +1,15 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+import { join } from "node:path";
+
+const extensionSource = readFileSync(join(process.cwd(), "vscode-extension", "src", "extension.ts"), "utf-8");
+
+test("VS Code startup uses inspected global/default config for binary path and auto-start", () => {
+  assert.match(extensionSource, /inspect<T>\(key\)/);
+  assert.match(extensionSource, /globalValue \?\? inspected\?\.defaultValue/);
+  assert.match(extensionSource, /binaryPath:\s*getTrustedConfigurationValue\("sf",\s*"binaryPath",\s*"sf"\)/);
+  assert.match(extensionSource, /autoStart:\s*getTrustedConfigurationValue\("sf",\s*"autoStart",\s*false\)/);
+  assert.doesNotMatch(extensionSource, /config\.get<string>\("binaryPath"/);
+  assert.doesNotMatch(extensionSource, /config\.get<boolean>\("autoStart"/);
+});

src/tests/web-files-symlink.test.ts

@@ -0,0 +1,45 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import { mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+
+import { resolveSecurePath } from "../../web/lib/secure-path.ts";
+
+test("web file API resolves normal project files under the canonical root", () => {
+  const root = mkdtempSync(join(tmpdir(), "gsd-web-root-"));
+  try {
+    writeFileSync(join(root, "inside.txt"), "inside");
+
+    assert.equal(resolveSecurePath("inside.txt", root), join(realpathSync.native(root), "inside.txt"));
+  } finally {
+    rmSync(root, { recursive: true, force: true });
+  }
+});
+
+test("web file API rejects symlinks that resolve outside the project root", () => {
+  const root = mkdtempSync(join(tmpdir(), "gsd-web-root-"));
+  const outside = mkdtempSync(join(tmpdir(), "gsd-web-outside-"));
+  try {
+    writeFileSync(join(outside, "secret.txt"), "secret");
+    symlinkSync(join(outside, "secret.txt"), join(root, "linked-secret.txt"));
+
+    assert.equal(resolveSecurePath("linked-secret.txt", root), null);
+  } finally {
+    rmSync(root, { recursive: true, force: true });
+    rmSync(outside, { recursive: true, force: true });
+  }
+});
+
+test("web file API rejects writes through symlinked parent directories outside root", () => {
+  const root = mkdtempSync(join(tmpdir(), "gsd-web-root-"));
+  const outside = mkdtempSync(join(tmpdir(), "gsd-web-outside-"));
+  try {
+    symlinkSync(outside, join(root, "linked-outside"), "dir");
+
+    assert.equal(resolveSecurePath("linked-outside/new.txt", root), null);
+  } finally {
+    rmSync(root, { recursive: true, force: true });
+    rmSync(outside, { recursive: true, force: true });
+  }
+});

vscode-extension/src/extension.ts

@@ -28,6 +28,19 @@ let lineDecorations: GsdLineDecorationManager | undefined;
 let gitIntegration: GsdGitIntegration | undefined;
 let permissionManager: GsdPermissionManager | undefined;
 
+function getTrustedConfigurationValue<T>(section: string, key: string, fallback: T): T {
+	const config = vscode.workspace.getConfiguration(section);
+	const inspected = config.inspect<T>(key);
+	return inspected?.globalValue ?? inspected?.defaultValue ?? fallback;
+}
+
+export function resolveTrustedSfStartupConfig(): { binaryPath: string; autoStart: boolean } {
+	return {
+		binaryPath: getTrustedConfigurationValue("sf", "binaryPath", "sf"),
+		autoStart: getTrustedConfigurationValue("sf", "autoStart", false),
+	};
+}
+
 function requireConnected(): boolean {
 	if (!client?.isConnected) {
 		vscode.window.showWarningMessage("SF agent is not running.");
@@ -42,8 +55,9 @@ function handleError(err: unknown, context: string): void {
 }
 
 export function activate(context: vscode.ExtensionContext): void {
+	const startupConfig = resolveTrustedSfStartupConfig();
 	const config = vscode.workspace.getConfiguration("sf");
-	const binaryPath = config.get<string>("binaryPath", "sf");
+	const binaryPath = startupConfig.binaryPath;
 	const cwd = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? process.cwd();
 
 	client = new SfClient(binaryPath, cwd);
@@ -960,7 +974,7 @@ export function activate(context: vscode.ExtensionContext): void {
 
 	// -- Auto-start ---------------------------------------------------------
 
-	if (config.get<boolean>("autoStart", false)) {
+	if (startupConfig.autoStart) {
 		vscode.commands.executeCommand("sf.start");
 	}
 }

web/app/api/files/route.ts

@@ -1,7 +1,8 @@
 import { existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs";
-import { join, resolve, relative, dirname, basename } from "node:path";
+import { join, dirname } from "node:path";
 
 import { requireProjectCwd } from "../../../../src/web/bridge-service.ts";
+import { resolveSecurePath } from "../../../lib/secure-path.ts";
 
 export const runtime = "nodejs";
 export const dynamic = "force-dynamic";
@@ -43,27 +44,6 @@ function getRootForMode(mode: RootMode, projectCwd: string): string {
   return mode === "project" ? projectCwd : getGsdRoot(projectCwd);
 }
 
-/**
- * Validate and resolve a requested path against the given root directory.
- * Returns the resolved absolute path or null if the path is invalid.
- */
-function resolveSecurePath(requestedPath: string, root: string): string | null {
-  if (requestedPath.startsWith("/") || requestedPath.startsWith("\\")) {
-    return null;
-  }
-  if (requestedPath.includes("..")) {
-    return null;
-  }
-
-  const resolved = resolve(root, requestedPath);
-  const rel = relative(root, resolved);
-  if (rel.startsWith("..") || resolve(root, rel) !== resolved) {
-    return null;
-  }
-
-  return resolved;
-}
-
 function buildTree(dirPath: string, skipDirs?: Set<string>, depth = 0, maxDepth = Infinity): FileNode[] {
   if (!existsSync(dirPath)) return [];
   if (depth >= maxDepth) return [];

web/lib/secure-path.ts

@@ -0,0 +1,48 @@
+import { existsSync, realpathSync } from "node:fs";
+import { dirname, isAbsolute, relative, resolve } from "node:path";
+
+function isWithinRoot(rootRealPath: string, candidateRealPath: string): boolean {
+  const rel = relative(rootRealPath, candidateRealPath);
+  return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel));
+}
+
+/**
+ * Validate and resolve a requested path against the given root directory.
+ * Returns the resolved absolute path or null if the path is invalid.
+ */
+export function resolveSecurePath(requestedPath: string, root: string, options: { mustExist?: boolean } = {}): string | null {
+  if (requestedPath.startsWith("/") || requestedPath.startsWith("\\")) {
+    return null;
+  }
+  if (requestedPath.includes("..")) {
+    return null;
+  }
+
+  let rootRealPath: string;
+  try {
+    rootRealPath = realpathSync.native(root);
+  } catch {
+    return null;
+  }
+
+  const resolved = resolve(rootRealPath, requestedPath);
+  const rel = relative(rootRealPath, resolved);
+  if (rel.startsWith("..") || isAbsolute(rel)) {
+    return null;
+  }
+
+  try {
+    if (existsSync(resolved)) {
+      const targetRealPath = realpathSync.native(resolved);
+      if (!isWithinRoot(rootRealPath, targetRealPath)) return null;
+    } else {
+      if (options.mustExist) return null;
+      const parentRealPath = realpathSync.native(dirname(resolved));
+      if (!isWithinRoot(rootRealPath, parentRealPath)) return null;
+    }
+  } catch {
+    return null;
+  }
+
+  return resolved;
+}