From 66ff949c11c651eb289b04352002c2dafbd87fe7 Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Tue, 28 Apr 2026 05:30:34 +0200 Subject: [PATCH] cherry-pick(security): harden project-controlled surfaces (PR #4755 partial) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-Authored-By: Claude Sonnet 4.6 --- packages/mcp-server/src/env-writer.test.ts | 80 +++++++++++++++++- packages/mcp-server/src/env-writer.ts | 82 +++++++++++++++++-- src/resources/extensions/sf/sf-db.ts | 17 ---- .../sf/tests/mcp-client-security.test.ts | 76 +++++++++++++++++ src/tests/vscode-startup-security.test.ts | 15 ++++ src/tests/web-files-symlink.test.ts | 45 ++++++++++ vscode-extension/src/extension.ts | 18 +++- web/app/api/files/route.ts | 24 +----- web/lib/secure-path.ts | 48 +++++++++++ 9 files changed, 357 insertions(+), 48 deletions(-) create mode 100644 src/resources/extensions/sf/tests/mcp-client-security.test.ts create mode 100644 src/tests/vscode-startup-security.test.ts create mode 100644 src/tests/web-files-symlink.test.ts create mode 100644 web/lib/secure-path.ts diff --git a/packages/mcp-server/src/env-writer.test.ts b/packages/mcp-server/src/env-writer.test.ts index deee26f13..8988725f8 100644 --- a/packages/mcp-server/src/env-writer.test.ts +++ b/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 }); + } + }); }); // --------------------------------------------------------------------------- diff --git a/packages/mcp-server/src/env-writer.ts b/packages/mcp-server/src/env-writer.ts index bf8296209..187c08113 100644 --- a/packages/mcp-server/src/env-writer.ts +++ b/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> | 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) // --------------------------------------------------------------------------- diff --git a/src/resources/extensions/sf/sf-db.ts b/src/resources/extensions/sf/sf-db.ts index c28ab9154..8c42ca7e7 100644 --- a/src/resources/extensions/sf/sf-db.ts +++ b/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): TaskRow { const parseTaskArray = (value: unknown): string[] => { if (Array.isArray(value)) { diff --git a/src/resources/extensions/sf/tests/mcp-client-security.test.ts b/src/resources/extensions/sf/tests/mcp-client-security.test.ts new file mode 100644 index 000000000..dcc741c95 --- /dev/null +++ b/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>\(\)/); + 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\(\)/); +}); diff --git a/src/tests/vscode-startup-security.test.ts b/src/tests/vscode-startup-security.test.ts new file mode 100644 index 000000000..ff6c5668a --- /dev/null +++ b/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\(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\("binaryPath"/); + assert.doesNotMatch(extensionSource, /config\.get\("autoStart"/); +}); diff --git a/src/tests/web-files-symlink.test.ts b/src/tests/web-files-symlink.test.ts new file mode 100644 index 000000000..e9bec3ef7 --- /dev/null +++ b/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 }); + } +}); diff --git a/vscode-extension/src/extension.ts b/vscode-extension/src/extension.ts index cce3c7cb9..4c74c0251 100644 --- a/vscode-extension/src/extension.ts +++ b/vscode-extension/src/extension.ts @@ -28,6 +28,19 @@ let lineDecorations: GsdLineDecorationManager | undefined; let gitIntegration: GsdGitIntegration | undefined; let permissionManager: GsdPermissionManager | undefined; +function getTrustedConfigurationValue(section: string, key: string, fallback: T): T { + const config = vscode.workspace.getConfiguration(section); + const inspected = config.inspect(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("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("autoStart", false)) { + if (startupConfig.autoStart) { vscode.commands.executeCommand("sf.start"); } } diff --git a/web/app/api/files/route.ts b/web/app/api/files/route.ts index 91e1b7826..a0bab9fa6 100644 --- a/web/app/api/files/route.ts +++ b/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, depth = 0, maxDepth = Infinity): FileNode[] { if (!existsSync(dirPath)) return []; if (depth >= maxDepth) return []; diff --git a/web/lib/secure-path.ts b/web/lib/secure-path.ts new file mode 100644 index 000000000..b809ca46a --- /dev/null +++ b/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; +}