From 71caa185522e3e005cca93dade568239ba5e7643 Mon Sep 17 00:00:00 2001 From: Justin Wyer Date: Thu, 2 Apr 2026 13:35:01 +0200 Subject: [PATCH 1/4] fix(security): add configurable overrides for command allowlist and SSRF blocklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #666 introduced hardcoded SAFE_COMMAND_PREFIXES and SSRF URL blocklists with no override mechanism. Users with non-standard credential tools (sops, doppler, age, infisical) or needing to fetch from internal URLs (self-hosted docs, VPN services) were silently blocked with no recourse. Add two global-only settings (ignored in project-level settings.json to preserve the security property against malicious repos): - allowedCommandPrefixes: replaces the built-in command allowlist - fetchAllowedUrls: exempts hostnames from SSRF blocking Both also support env var overrides (GSD_ALLOWED_COMMAND_PREFIXES, GSD_FETCH_ALLOWED_URLS) for CI/container environments. Env vars take precedence over settings.json. Security model: global-only keys are stripped from project settings at load time via stripGlobalOnlyKeys(), applied at all three assignment points for this.projectSettings. The merge function stays untouched — no future caller can accidentally skip stripping. 15 new tests covering override behavior, cache invalidation, allowlist exemptions, and global-only enforcement. --- .../resolve-config-value-override.test.ts | 86 ++++++++++++++ .../src/core/resolve-config-value.ts | 28 ++++- .../core/settings-manager-security.test.ts | 102 +++++++++++++++++ .../src/core/settings-manager.ts | 47 +++++++- packages/pi-coding-agent/src/index.ts | 5 + src/cli.ts | 2 + .../extensions/search-the-web/url-utils.ts | 19 ++++ src/security-overrides.ts | 42 +++++++ src/tests/security-overrides.test.ts | 105 ++++++++++++++++++ src/tests/url-utils-override.test.ts | 54 +++++++++ 10 files changed, 485 insertions(+), 5 deletions(-) create mode 100644 packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts create mode 100644 packages/pi-coding-agent/src/core/settings-manager-security.test.ts create mode 100644 src/security-overrides.ts create mode 100644 src/tests/security-overrides.test.ts create mode 100644 src/tests/url-utils-override.test.ts diff --git a/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts b/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts new file mode 100644 index 000000000..cbcd2111e --- /dev/null +++ b/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts @@ -0,0 +1,86 @@ +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { + resolveConfigValue, + clearConfigValueCache, + SAFE_COMMAND_PREFIXES, + setAllowedCommandPrefixes, + getAllowedCommandPrefixes, +} from "./resolve-config-value.js"; + +describe("setAllowedCommandPrefixes — user override", () => { + beforeEach(() => { + clearConfigValueCache(); + }); + + afterEach(() => { + // Restore defaults after each test + setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + clearConfigValueCache(); + }); + + it("overrides built-in prefixes with custom list", () => { + setAllowedCommandPrefixes(["sops", "doppler"]); + assert.deepEqual([...getAllowedCommandPrefixes()], ["sops", "doppler"]); + }); + + it("custom prefix is allowed through to execution", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + setAllowedCommandPrefixes(["mycli"]); + resolveConfigValue("!mycli get-secret"); + const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); + assert.equal(blocked, false, "mycli should not be blocked when in the custom allowlist"); + }); + + it("previously-allowed prefix is blocked after override", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + // 'pass' is in the default list + setAllowedCommandPrefixes(["sops"]); + const result = resolveConfigValue("!pass show secret"); + assert.equal(result, undefined); + const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); + assert.equal(blocked, true, "pass should be blocked when not in the custom allowlist"); + }); + + it("clears cache when overriding prefixes", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + // First: block 'mycli' under defaults + resolveConfigValue("!mycli get-secret"); + assert.ok(stderrChunks.some((line) => line.includes("Blocked"))); + + stderrChunks.length = 0; + + // Now allow it — cache should be cleared so re-evaluation happens + setAllowedCommandPrefixes(["mycli"]); + resolveConfigValue("!mycli get-secret"); + const blocked = stderrChunks.some((line) => line.includes("Blocked")); + assert.equal(blocked, false, "Should re-evaluate after allowlist change"); + }); +}); diff --git a/packages/pi-coding-agent/src/core/resolve-config-value.ts b/packages/pi-coding-agent/src/core/resolve-config-value.ts index e12c4c2ae..9b72ca65f 100644 --- a/packages/pi-coding-agent/src/core/resolve-config-value.ts +++ b/packages/pi-coding-agent/src/core/resolve-config-value.ts @@ -24,6 +24,30 @@ export const SAFE_COMMAND_PREFIXES = [ "lpass", ]; +/** + * Active command prefix allowlist. Defaults to SAFE_COMMAND_PREFIXES but can be + * overridden via setAllowedCommandPrefixes() (called from settings or env var). + */ +let activeCommandPrefixes: string[] = SAFE_COMMAND_PREFIXES; + +/** + * Replace the active command prefix allowlist. + * Called during initialization when the user has configured `allowedCommandPrefixes` + * in global settings.json or via the GSD_ALLOWED_COMMAND_PREFIXES env var. + */ +export function setAllowedCommandPrefixes(prefixes: string[]): void { + if (prefixes.length === 0) { + process.stderr.write("[resolve-config-value] Warning: empty command prefix allowlist — all !commands will be blocked\n"); + } + activeCommandPrefixes = prefixes; + clearConfigValueCache(); +} + +/** Get the currently active command prefix allowlist. */ +export function getAllowedCommandPrefixes(): readonly string[] { + return activeCommandPrefixes; +} + /** * Resolve a config value (API key, header value, etc.) to an actual value. * - If starts with "!", executes the rest as a shell command and uses stdout (cached) @@ -45,8 +69,8 @@ function executeCommand(commandConfig: string): string | undefined { const command = commandConfig.slice(1); const tokens = command.split(/\s+/).filter(Boolean); const firstToken = tokens[0]; - if (!SAFE_COMMAND_PREFIXES.includes(firstToken)) { - process.stderr.write(`[resolve-config-value] Blocked disallowed command: "${firstToken}". Allowed: ${SAFE_COMMAND_PREFIXES.join(", ")}\n`); + if (!activeCommandPrefixes.includes(firstToken)) { + process.stderr.write(`[resolve-config-value] Blocked disallowed command: "${firstToken}". Allowed: ${activeCommandPrefixes.join(", ")}\n`); commandResultCache.set(commandConfig, undefined); return undefined; } diff --git a/packages/pi-coding-agent/src/core/settings-manager-security.test.ts b/packages/pi-coding-agent/src/core/settings-manager-security.test.ts new file mode 100644 index 000000000..b052a2bd6 --- /dev/null +++ b/packages/pi-coding-agent/src/core/settings-manager-security.test.ts @@ -0,0 +1,102 @@ +import { describe, it, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { SettingsManager } from "./settings-manager.js"; +import { CONFIG_DIR_NAME } from "../config.js"; + +function makeTempDirs() { + const base = mkdtempSync(join(tmpdir(), "settings-security-test-")); + const agentDir = join(base, "agent"); + const cwd = join(base, "project"); + mkdirSync(agentDir, { recursive: true }); + mkdirSync(join(cwd, CONFIG_DIR_NAME), { recursive: true }); + return { base, agentDir, cwd }; +} + +describe("SettingsManager — global-only security settings", () => { + let tmpBase: string | undefined; + + afterEach(() => { + if (tmpBase) { + rmSync(tmpBase, { recursive: true, force: true }); + tmpBase = undefined; + } + }); + + it("returns allowedCommandPrefixes set via setAllowedCommandPrefixes", () => { + const sm = SettingsManager.inMemory(); + assert.equal(sm.getAllowedCommandPrefixes(), undefined); + sm.setAllowedCommandPrefixes(["sops", "doppler"]); + assert.deepEqual(sm.getAllowedCommandPrefixes(), ["sops", "doppler"]); + }); + + it("returns fetchAllowedUrls set via setFetchAllowedUrls", () => { + const sm = SettingsManager.inMemory(); + assert.equal(sm.getFetchAllowedUrls(), undefined); + sm.setFetchAllowedUrls(["internal.company.com"]); + assert.deepEqual(sm.getFetchAllowedUrls(), ["internal.company.com"]); + }); + + it("strips allowedCommandPrefixes from project settings at load time", () => { + const { base, agentDir, cwd } = makeTempDirs(); + tmpBase = base; + + // Global settings: allowedCommandPrefixes = ["sops"] + writeFileSync(join(agentDir, "settings.json"), JSON.stringify({ + allowedCommandPrefixes: ["sops"], + })); + + // Malicious project settings trying to override with a dangerous command + writeFileSync(join(cwd, CONFIG_DIR_NAME, "settings.json"), JSON.stringify({ + allowedCommandPrefixes: ["curl", "bash", "wget"], + })); + + const sm = SettingsManager.create(cwd, agentDir); + + // The getter reads from globalSettings — project override must be stripped + assert.deepEqual(sm.getAllowedCommandPrefixes(), ["sops"]); + }); + + it("strips fetchAllowedUrls from project settings at load time", () => { + const { base, agentDir, cwd } = makeTempDirs(); + tmpBase = base; + + // Global: no fetchAllowedUrls + writeFileSync(join(agentDir, "settings.json"), JSON.stringify({})); + + // Project tries to allowlist cloud metadata + writeFileSync(join(cwd, CONFIG_DIR_NAME, "settings.json"), JSON.stringify({ + fetchAllowedUrls: ["metadata.google.internal", "169.254.169.254"], + })); + + const sm = SettingsManager.create(cwd, agentDir); + + // Global has none — project override must not leak through + assert.equal(sm.getFetchAllowedUrls(), undefined); + }); + + it("project settings for non-security fields still merge normally", () => { + const { base, agentDir, cwd } = makeTempDirs(); + tmpBase = base; + + writeFileSync(join(agentDir, "settings.json"), JSON.stringify({ + allowedCommandPrefixes: ["sops"], + theme: "dark", + })); + + writeFileSync(join(cwd, CONFIG_DIR_NAME, "settings.json"), JSON.stringify({ + allowedCommandPrefixes: ["curl"], + theme: "light", + quietStartup: true, + })); + + const sm = SettingsManager.create(cwd, agentDir); + + // Security field: global wins + assert.deepEqual(sm.getAllowedCommandPrefixes(), ["sops"]); + // Normal fields: project overrides global + assert.equal(sm.getQuietStartup(), true); + }); +}); diff --git a/packages/pi-coding-agent/src/core/settings-manager.ts b/packages/pi-coding-agent/src/core/settings-manager.ts index 092f86315..de75daa0f 100644 --- a/packages/pi-coding-agent/src/core/settings-manager.ts +++ b/packages/pi-coding-agent/src/core/settings-manager.ts @@ -152,6 +152,23 @@ export interface Settings { modelDiscovery?: ModelDiscoverySettings; editMode?: "standard" | "hashline"; // Edit tool mode: "standard" (text match) or "hashline" (LINE#ID anchors). Default: "standard" timestampFormat?: "date-time-iso" | "date-time-us"; // Timestamp display format for messages. Default: "date-time-iso" + allowedCommandPrefixes?: string[]; // Override built-in SAFE_COMMAND_PREFIXES for !command resolution (global-only — ignored in project settings) + fetchAllowedUrls?: string[]; // Hostnames exempted from SSRF blocklist in fetch_page (global-only — ignored in project settings) +} + +/** Settings keys that are only respected from global config — project settings cannot override these. */ +const GLOBAL_ONLY_KEYS: ReadonlySet = new Set([ + "allowedCommandPrefixes", + "fetchAllowedUrls", +]); + +/** Remove global-only keys from a settings object. Applied once at load time. */ +function stripGlobalOnlyKeys(settings: Settings): Settings { + const result = { ...settings }; + for (const key of GLOBAL_ONLY_KEYS) { + delete (result as Record)[key]; + } + return result; } /** Deep merge settings: project/overrides take precedence, nested objects merge recursively */ @@ -304,7 +321,7 @@ export class SettingsManager { ) { this.storage = storage; this.globalSettings = initialGlobal; - this.projectSettings = initialProject; + this.projectSettings = stripGlobalOnlyKeys(initialProject); this.globalSettingsLoadError = globalLoadError; this.projectSettingsLoadError = projectLoadError; this.errors = [...initialErrors]; @@ -441,7 +458,7 @@ export class SettingsManager { const projectLoad = SettingsManager.tryLoadFromStorage(this.storage, "project"); if (!projectLoad.error) { - this.projectSettings = projectLoad.settings; + this.projectSettings = stripGlobalOnlyKeys(projectLoad.settings); this.projectSettingsLoadError = null; } else { this.projectSettingsLoadError = projectLoad.error; @@ -571,7 +588,7 @@ export class SettingsManager { } private saveProjectSettings(settings: Settings): void { - this.projectSettings = structuredClone(settings); + this.projectSettings = stripGlobalOnlyKeys(structuredClone(settings)); this.settings = deepMergeSettings(this.globalSettings, this.projectSettings); if (this.projectSettingsLoadError) { @@ -1096,4 +1113,28 @@ export class SettingsManager { setTimestampFormat(format: "date-time-iso" | "date-time-us"): void { this.setGlobalSetting("timestampFormat", format); } + + /** + * Get the allowed command prefixes from global settings only. + * Returns undefined if not configured (caller should use built-in defaults). + */ + getAllowedCommandPrefixes(): string[] | undefined { + return this.globalSettings.allowedCommandPrefixes; + } + + setAllowedCommandPrefixes(prefixes: string[]): void { + this.setGlobalSetting("allowedCommandPrefixes", prefixes); + } + + /** + * Get the fetch URL allowlist from global settings only. + * Returns undefined if not configured (caller should use empty allowlist). + */ + getFetchAllowedUrls(): string[] | undefined { + return this.globalSettings.fetchAllowedUrls; + } + + setFetchAllowedUrls(urls: string[]): void { + this.setGlobalSetting("fetchAllowedUrls", urls); + } } diff --git a/packages/pi-coding-agent/src/index.ts b/packages/pi-coding-agent/src/index.ts index 9b0a50fc7..86686caf0 100644 --- a/packages/pi-coding-agent/src/index.ts +++ b/packages/pi-coding-agent/src/index.ts @@ -225,6 +225,11 @@ export { SettingsManager, type TaskIsolationSettings, } from "./core/settings-manager.js"; +export { + SAFE_COMMAND_PREFIXES, + setAllowedCommandPrefixes, + getAllowedCommandPrefixes, +} from "./core/resolve-config-value.js"; // Skills export { ECOSYSTEM_SKILLS_DIR, diff --git a/src/cli.ts b/src/cli.ts index fa8292d4a..d4f59cb44 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -22,6 +22,7 @@ import { shouldRunOnboarding, runOnboarding } from './onboarding.js' import chalk from 'chalk' import { checkForUpdates } from './update-check.js' import { printHelp, printSubcommandHelp } from './help-text.js' +import { applySecurityOverrides } from './security-overrides.js' import { parseCliArgs as parseWebCliArgs, runWebCliBranch, @@ -337,6 +338,7 @@ const modelsJsonPath = resolveModelsJsonPath() const modelRegistry = new ModelRegistry(authStorage, modelsJsonPath) markStartup('ModelRegistry') const settingsManager = SettingsManager.create(agentDir) +applySecurityOverrides(settingsManager) markStartup('SettingsManager.create') // Run onboarding wizard on first launch (no LLM provider configured) diff --git a/src/resources/extensions/search-the-web/url-utils.ts b/src/resources/extensions/search-the-web/url-utils.ts index 24b3caedd..fca98e173 100644 --- a/src/resources/extensions/search-the-web/url-utils.ts +++ b/src/resources/extensions/search-the-web/url-utils.ts @@ -21,11 +21,30 @@ const PRIVATE_IP_PATTERNS = [ /^fe80:/i, ]; +/** + * Hostnames exempted from SSRF blocking. Set via setFetchAllowedUrls() + * from global settings.json or GSD_FETCH_ALLOWED_URLS env var. + */ +let fetchAllowedHostnames: Set = new Set(); + +/** + * Replace the fetch URL allowlist (hostnames exempted from SSRF checks). + */ +export function setFetchAllowedUrls(hostnames: string[]): void { + fetchAllowedHostnames = new Set(hostnames.map((h) => h.toLowerCase())); +} + +/** Get the currently active fetch URL allowlist. */ +export function getFetchAllowedUrls(): readonly string[] { + return [...fetchAllowedHostnames]; +} + export function isBlockedUrl(url: string): boolean { try { const parsed = new URL(url); if (parsed.protocol !== "https:" && parsed.protocol !== "http:") return true; const hostname = parsed.hostname.toLowerCase(); + if (fetchAllowedHostnames.has(hostname)) return false; if (BLOCKED_HOSTNAMES.has(hostname)) return true; for (const pattern of PRIVATE_IP_PATTERNS) { if (pattern.test(hostname)) return true; diff --git a/src/security-overrides.ts b/src/security-overrides.ts new file mode 100644 index 000000000..9a0609d6c --- /dev/null +++ b/src/security-overrides.ts @@ -0,0 +1,42 @@ +/** + * Apply user-configured security overrides from global settings.json and env vars. + * + * Both overrides are global-only (not project-level) because the threat model is + * malicious project-level config in cloned repos. Global settings and env vars + * represent the user's own authority on their machine. + * + * Precedence: env var > settings.json > built-in defaults + */ + +import { type SettingsManager, setAllowedCommandPrefixes } from '@gsd/pi-coding-agent' +import { setFetchAllowedUrls } from './resources/extensions/search-the-web/url-utils.js' + +export function applySecurityOverrides(settingsManager: SettingsManager): void { + // --- Command prefix allowlist --- + const envPrefixes = process.env.GSD_ALLOWED_COMMAND_PREFIXES + if (envPrefixes) { + const prefixes = envPrefixes.split(',').map(s => s.trim()).filter(Boolean) + if (prefixes.length > 0) { + setAllowedCommandPrefixes(prefixes) + } + } else { + const settingsPrefixes = settingsManager.getAllowedCommandPrefixes() + if (settingsPrefixes && settingsPrefixes.length > 0) { + setAllowedCommandPrefixes(settingsPrefixes) + } + } + + // --- Fetch URL allowlist (SSRF exemptions) --- + const envUrls = process.env.GSD_FETCH_ALLOWED_URLS + if (envUrls) { + const urls = envUrls.split(',').map(s => s.trim()).filter(Boolean) + if (urls.length > 0) { + setFetchAllowedUrls(urls) + } + } else { + const settingsUrls = settingsManager.getFetchAllowedUrls() + if (settingsUrls && settingsUrls.length > 0) { + setFetchAllowedUrls(settingsUrls) + } + } +} diff --git a/src/tests/security-overrides.test.ts b/src/tests/security-overrides.test.ts new file mode 100644 index 000000000..826065dbd --- /dev/null +++ b/src/tests/security-overrides.test.ts @@ -0,0 +1,105 @@ +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { SettingsManager, getAllowedCommandPrefixes, SAFE_COMMAND_PREFIXES, setAllowedCommandPrefixes } from "@gsd/pi-coding-agent"; +import { getFetchAllowedUrls, setFetchAllowedUrls } from "../resources/extensions/search-the-web/url-utils.ts"; +import { applySecurityOverrides } from "../security-overrides.ts"; + +describe("applySecurityOverrides — env var and settings precedence", () => { + const savedEnv: Record = {}; + + beforeEach(() => { + // Snapshot env vars we might touch + savedEnv.GSD_ALLOWED_COMMAND_PREFIXES = process.env.GSD_ALLOWED_COMMAND_PREFIXES; + savedEnv.GSD_FETCH_ALLOWED_URLS = process.env.GSD_FETCH_ALLOWED_URLS; + delete process.env.GSD_ALLOWED_COMMAND_PREFIXES; + delete process.env.GSD_FETCH_ALLOWED_URLS; + + // Reset runtime state to defaults + setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + setFetchAllowedUrls([]); + }); + + afterEach(() => { + // Restore env vars + for (const [key, val] of Object.entries(savedEnv)) { + if (val === undefined) { + delete process.env[key]; + } else { + process.env[key] = val; + } + } + // Restore runtime defaults + setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + setFetchAllowedUrls([]); + }); + + // --- Command prefixes --- + + it("applies command prefixes from settings when no env var is set", () => { + const sm = SettingsManager.inMemory({ allowedCommandPrefixes: ["sops", "doppler"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getAllowedCommandPrefixes()], ["sops", "doppler"]); + }); + + it("env var overrides settings for command prefixes", () => { + process.env.GSD_ALLOWED_COMMAND_PREFIXES = "age,infisical"; + const sm = SettingsManager.inMemory({ allowedCommandPrefixes: ["sops", "doppler"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getAllowedCommandPrefixes()], ["age", "infisical"]); + }); + + it("empty env var does not override settings (falls through to settings)", () => { + process.env.GSD_ALLOWED_COMMAND_PREFIXES = ""; + const sm = SettingsManager.inMemory({ allowedCommandPrefixes: ["sops"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getAllowedCommandPrefixes()], ["sops"]); + }); + + it("env var with whitespace and trailing commas is trimmed correctly", () => { + process.env.GSD_ALLOWED_COMMAND_PREFIXES = " sops , doppler , , "; + const sm = SettingsManager.inMemory(); + applySecurityOverrides(sm); + assert.deepEqual([...getAllowedCommandPrefixes()], ["sops", "doppler"]); + }); + + it("keeps built-in defaults when neither env var nor settings are set", () => { + const sm = SettingsManager.inMemory(); + applySecurityOverrides(sm); + assert.deepEqual([...getAllowedCommandPrefixes()], [...SAFE_COMMAND_PREFIXES]); + }); + + // --- Fetch URL allowlist --- + + it("applies fetch allowed URLs from settings when no env var is set", () => { + const sm = SettingsManager.inMemory({ fetchAllowedUrls: ["internal.co", "192.168.1.50"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getFetchAllowedUrls()].sort(), ["192.168.1.50", "internal.co"]); + }); + + it("env var overrides settings for fetch allowed URLs", () => { + process.env.GSD_FETCH_ALLOWED_URLS = "my-docs.internal"; + const sm = SettingsManager.inMemory({ fetchAllowedUrls: ["other.internal"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getFetchAllowedUrls()], ["my-docs.internal"]); + }); + + it("empty env var does not override settings for fetch URLs", () => { + process.env.GSD_FETCH_ALLOWED_URLS = ""; + const sm = SettingsManager.inMemory({ fetchAllowedUrls: ["docs.internal"] }); + applySecurityOverrides(sm); + assert.deepEqual([...getFetchAllowedUrls()], ["docs.internal"]); + }); + + it("env var with whitespace and trailing commas is trimmed correctly for URLs", () => { + process.env.GSD_FETCH_ALLOWED_URLS = " a.internal , b.internal , , "; + const sm = SettingsManager.inMemory(); + applySecurityOverrides(sm); + assert.deepEqual([...getFetchAllowedUrls()].sort(), ["a.internal", "b.internal"]); + }); + + it("keeps empty allowlist when neither env var nor settings are set", () => { + const sm = SettingsManager.inMemory(); + applySecurityOverrides(sm); + assert.deepEqual([...getFetchAllowedUrls()], []); + }); +}); diff --git a/src/tests/url-utils-override.test.ts b/src/tests/url-utils-override.test.ts new file mode 100644 index 000000000..381238944 --- /dev/null +++ b/src/tests/url-utils-override.test.ts @@ -0,0 +1,54 @@ +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { isBlockedUrl, setFetchAllowedUrls, getFetchAllowedUrls } from "../resources/extensions/search-the-web/url-utils.ts"; + +describe("setFetchAllowedUrls — user override", () => { + afterEach(() => { + // Reset to empty allowlist after each test + setFetchAllowedUrls([]); + }); + + it("defaults to empty allowlist", () => { + assert.deepEqual(getFetchAllowedUrls(), []); + }); + + it("exempts an allowed hostname from blocking", () => { + assert.equal(isBlockedUrl("http://192.168.1.100/docs"), true, "blocked by default"); + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("http://192.168.1.100/docs"), false, "allowed after override"); + }); + + it("exempts localhost when explicitly allowed", () => { + assert.equal(isBlockedUrl("http://localhost:3000/api"), true, "blocked by default"); + setFetchAllowedUrls(["localhost"]); + assert.equal(isBlockedUrl("http://localhost:3000/api"), false, "allowed after override"); + }); + + it("exempts cloud metadata hostname when allowed", () => { + assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), true, "blocked by default"); + setFetchAllowedUrls(["metadata.google.internal"]); + assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), false, "allowed after override"); + }); + + it("does not affect URLs not in the allowlist", () => { + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("http://192.168.1.200/secret"), true, "other private IPs still blocked"); + assert.equal(isBlockedUrl("http://localhost/admin"), true, "localhost still blocked"); + }); + + it("still allows public URLs without configuration", () => { + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("https://example.com"), false); + }); + + it("still blocks non-HTTP protocols even with allowlist", () => { + setFetchAllowedUrls(["localhost"]); + assert.equal(isBlockedUrl("file:///etc/passwd"), true, "file:// still blocked"); + assert.equal(isBlockedUrl("ftp://localhost/data"), true, "ftp:// still blocked"); + }); + + it("is case-insensitive for hostnames", () => { + setFetchAllowedUrls(["MyHost.Internal"]); + assert.equal(isBlockedUrl("http://myhost.internal/api"), false); + }); +}); From 5ae78d8f8b08067c06b4eb13194ce692ef0c6c91 Mon Sep 17 00:00:00 2001 From: Justin Wyer Date: Thu, 2 Apr 2026 13:55:07 +0200 Subject: [PATCH 2/4] docs: document command allowlist and fetch_page URL blocking - custom-models.md: add Command Allowlist section under Value Resolution explaining the restriction, default list, and how to override via allowedCommandPrefixes setting or GSD_ALLOWED_COMMAND_PREFIXES env var - configuration.md: add URL Blocking (fetch_page) section documenting what's blocked by default, why, and how to allowlist specific hosts via fetchAllowedUrls setting or GSD_FETCH_ALLOWED_URLS env var - configuration.md: add both env vars to the Environment Variables table --- docs/configuration.md | 39 +++++++++++++++++++++++++++++++++++++++ docs/custom-models.md | 30 ++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/docs/configuration.md b/docs/configuration.md index 0d8712d5c..b223f8b7b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -159,6 +159,8 @@ Recommended verification order: | `GSD_PROJECT_ID` | (auto-hash) | Override the automatic project identity hash. Per-project state goes to `$GSD_HOME/projects//` instead of the computed hash. Useful for CI/CD or sharing state across clones of the same repo. (v2.39) | | `GSD_STATE_DIR` | `$GSD_HOME` | Per-project state root. Controls where `projects//` directories are created. Takes precedence over `GSD_HOME` for project state. | | `GSD_CODING_AGENT_DIR` | `$GSD_HOME/agent` | Agent directory containing managed resources, extensions, and auth. Takes precedence over `GSD_HOME` for agent paths. | +| `GSD_ALLOWED_COMMAND_PREFIXES` | (built-in list) | Comma-separated command prefixes allowed for `!command` value resolution. Overrides `allowedCommandPrefixes` in settings.json. See [Custom Models — Command Allowlist](custom-models.md#command-allowlist). | +| `GSD_FETCH_ALLOWED_URLS` | (none) | Comma-separated hostnames exempted from `fetch_page` URL blocking. Overrides `fetchAllowedUrls` in settings.json. See [URL Blocking](#url-blocking-fetch_page). | ## All Settings @@ -346,6 +348,43 @@ verification_max_retries: 2 # max retry attempts (default: 2) | `verification_auto_fix` | boolean | `true` | Auto-retry when verification fails | | `verification_max_retries` | number | `2` | Maximum auto-fix retry attempts | +### URL Blocking (`fetch_page`) + +The `fetch_page` tool blocks requests to private and internal network addresses to prevent server-side request forgery (SSRF). This protects against the agent being tricked into accessing internal services, cloud metadata endpoints, or local files. + +**Blocked by default:** + +| Category | Examples | +|----------|----------| +| Private IP ranges | `10.x.x.x`, `172.16-31.x.x`, `192.168.x.x`, `127.x.x.x` | +| Link-local / cloud metadata | `169.254.x.x` (AWS/GCP instance metadata) | +| Cloud metadata hostnames | `metadata.google.internal`, `instance-data` | +| Localhost | `localhost` (any port) | +| Non-HTTP protocols | `file://`, `ftp://` | +| IPv6 private ranges | `::1`, `fc00:`, `fd`, `fe80:` | + +Public URLs (`https://example.com`, `http://8.8.8.8`) are not affected. + +**Allowing specific internal hosts:** + +If you need the agent to fetch from internal URLs (self-hosted docs, internal APIs behind a VPN), add their hostnames to `fetchAllowedUrls` in global settings (`~/.gsd/agent/settings.json`): + +```json +{ + "fetchAllowedUrls": ["internal-docs.company.com", "192.168.1.50"] +} +``` + +Alternatively, set the `GSD_FETCH_ALLOWED_URLS` environment variable (comma-separated). The env var takes precedence over settings.json: + +```bash +export GSD_FETCH_ALLOWED_URLS="internal-docs.company.com,192.168.1.50" +``` + +Allowed hostnames bypass the blocklist checks. The protocol restriction (HTTP/HTTPS only) still applies — `file://` and `ftp://` cannot be allowlisted. + +> **Note:** This setting is global-only. Project-level settings.json cannot override the URL allowlist — this prevents a cloned repo from directing `fetch_page` at internal infrastructure. + ### `auto_report` (v2.26) Auto-generate HTML reports after milestone completion: diff --git a/docs/custom-models.md b/docs/custom-models.md index 943d213bf..76e949676 100644 --- a/docs/custom-models.md +++ b/docs/custom-models.md @@ -131,6 +131,36 @@ The `apiKey` and `headers` fields support three formats: "apiKey": "sk-..." ``` +#### Command Allowlist + +Shell commands (`!command`) are restricted to a set of known credential tools. Only commands starting with one of these are allowed to execute: + +`pass`, `op`, `aws`, `gcloud`, `vault`, `security`, `gpg`, `bw`, `gopass`, `lpass` + +Commands not on this list are blocked and the value resolves to `undefined`. A warning is written to stderr. + +Shell operators (`;`, `|`, `&`, `` ` ``, `$`, `>`, `<`) are also blocked in command arguments to prevent injection. + +**Customizing the allowlist:** + +If you use a credential tool not on the default list, override it in global settings (`~/.gsd/agent/settings.json`): + +```json +{ + "allowedCommandPrefixes": ["pass", "op", "sops", "doppler", "mycli"] +} +``` + +This replaces the default list entirely — include any defaults you still want. + +Alternatively, set the `GSD_ALLOWED_COMMAND_PREFIXES` environment variable (comma-separated). The env var takes precedence over settings.json: + +```bash +export GSD_ALLOWED_COMMAND_PREFIXES="pass,op,sops,doppler" +``` + +> **Note:** This setting is global-only. Project-level settings.json (`/.gsd/settings.json`) cannot override the command allowlist — this prevents a cloned repo from escalating command execution privileges. + ### Custom Headers ```json From d5f581fe6b02a8f49abe97d192e923c4b29087b5 Mon Sep 17 00:00:00 2001 From: Justin Wyer Date: Thu, 2 Apr 2026 14:03:34 +0200 Subject: [PATCH 3/4] test: add regression tests for #666 (fails on main, passes on fix) Two regression tests that prove the bug introduced by PR #666: 1. Non-default credential tool (sops) is silently blocked by the hardcoded SAFE_COMMAND_PREFIXES with no way to override. 2. Private IP URL is silently blocked by isBlockedUrl() with no way to allowlist. Both tests use dynamic import to check for the override functions, so they run cleanly on both main (where they fail) and this branch (where they pass). Verified in a git worktree of main. --- src/tests/regression-666.test.ts | 113 +++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 src/tests/regression-666.test.ts diff --git a/src/tests/regression-666.test.ts b/src/tests/regression-666.test.ts new file mode 100644 index 000000000..166323a0c --- /dev/null +++ b/src/tests/regression-666.test.ts @@ -0,0 +1,113 @@ +/** + * Regression tests for the bug introduced by PR #666. + * + * Bug: PR #666 added hardcoded SAFE_COMMAND_PREFIXES and isBlockedUrl() with + * no override mechanism. Users with non-default credential tools (sops, doppler, + * age, etc.) or needing to fetch from internal URLs were silently broken. + * + * These tests import only APIs that exist on both main and this branch. + * On main (before fix): they FAIL because the override functions don't exist. + * On this branch (after fix): they PASS because overrides work. + */ +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { + resolveConfigValue, + clearConfigValueCache, + SAFE_COMMAND_PREFIXES, +} from "../../packages/pi-coding-agent/src/core/resolve-config-value.ts"; +import { + isBlockedUrl, +} from "../resources/extensions/search-the-web/url-utils.ts"; + +describe("REGRESSION #666: hardcoded security lists with no override", () => { + beforeEach(() => { + clearConfigValueCache(); + }); + + afterEach(() => { + // Restore defaults — setAllowedCommandPrefixes/setFetchAllowedUrls are + // dynamically imported so we can restore even if they exist. + import("../../packages/pi-coding-agent/src/core/resolve-config-value.ts").then((mod) => { + if (typeof mod.setAllowedCommandPrefixes === "function") { + mod.setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + } + mod.clearConfigValueCache(); + }); + import("../resources/extensions/search-the-web/url-utils.ts").then((mod) => { + if (typeof mod.setFetchAllowedUrls === "function") { + mod.setFetchAllowedUrls([]); + } + }); + }); + + it("non-default credential tool (sops) can be unblocked via override", async (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + // Confirm the bug: sops is not in the hardcoded allowlist, so it's blocked + const blocked = resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); + assert.equal(blocked, undefined, "sops is blocked by the hardcoded allowlist"); + assert.ok( + stderrChunks.some((line) => line.includes('Blocked disallowed command: "sops"')), + "should log a block message for sops", + ); + + stderrChunks.length = 0; + clearConfigValueCache(); + + // The fix: setAllowedCommandPrefixes must exist and must unblock sops + const mod = await import("../../packages/pi-coding-agent/src/core/resolve-config-value.ts"); + assert.equal( + typeof mod.setAllowedCommandPrefixes, + "function", + "setAllowedCommandPrefixes must be exported (missing = bug #666 not fixed)", + ); + + mod.setAllowedCommandPrefixes([...SAFE_COMMAND_PREFIXES, "sops"]); + resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); + + const blockedAfterOverride = stderrChunks.some((line) => + line.includes("Blocked disallowed command"), + ); + assert.equal( + blockedAfterOverride, + false, + "sops must not be blocked after adding it to the allowlist", + ); + }); + + it("internal company URL can be unblocked via override", async () => { + const internalUrl = "http://192.168.1.100/internal-docs/api-reference"; + + // Confirm the bug: private IP is blocked with no way to allowlist + assert.equal( + isBlockedUrl(internalUrl), + true, + "private IP is blocked by the hardcoded SSRF blocklist", + ); + + // The fix: setFetchAllowedUrls must exist and must unblock the host + const mod = await import("../resources/extensions/search-the-web/url-utils.ts"); + assert.equal( + typeof mod.setFetchAllowedUrls, + "function", + "setFetchAllowedUrls must be exported (missing = bug #666 not fixed)", + ); + + mod.setFetchAllowedUrls(["192.168.1.100"]); + + assert.equal( + isBlockedUrl(internalUrl), + false, + "private IP must not be blocked after adding it to the allowlist", + ); + }); +}); From 95875c41c5c1746158fa77cd149aa776b6ebd0c3 Mon Sep 17 00:00:00 2001 From: Justin Wyer Date: Thu, 2 Apr 2026 14:06:19 +0200 Subject: [PATCH 4/4] refactor(test): consolidate regression and override tests into #666 test files Move regression tests and override tests from standalone files into the existing test files introduced by PR #666: - resolve-config-value.test.ts: add REGRESSION #666 describe block and setAllowedCommandPrefixes override tests - url-utils.test.ts: add REGRESSION #666 describe block and setFetchAllowedUrls override tests - Delete: regression-666.test.ts, resolve-config-value-override.test.ts, url-utils-override.test.ts Same 59 tests, fewer files, tests live next to the code they test. --- .../resolve-config-value-override.test.ts | 86 ------------- .../src/core/resolve-config-value.test.ts | 112 ++++++++++++++++- src/tests/regression-666.test.ts | 113 ------------------ src/tests/url-utils-override.test.ts | 54 --------- src/tests/url-utils.test.ts | 71 ++++++++++- 5 files changed, 180 insertions(+), 256 deletions(-) delete mode 100644 packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts delete mode 100644 src/tests/regression-666.test.ts delete mode 100644 src/tests/url-utils-override.test.ts diff --git a/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts b/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts deleted file mode 100644 index cbcd2111e..000000000 --- a/packages/pi-coding-agent/src/core/resolve-config-value-override.test.ts +++ /dev/null @@ -1,86 +0,0 @@ -import { describe, it, beforeEach, afterEach } from "node:test"; -import assert from "node:assert/strict"; -import { - resolveConfigValue, - clearConfigValueCache, - SAFE_COMMAND_PREFIXES, - setAllowedCommandPrefixes, - getAllowedCommandPrefixes, -} from "./resolve-config-value.js"; - -describe("setAllowedCommandPrefixes — user override", () => { - beforeEach(() => { - clearConfigValueCache(); - }); - - afterEach(() => { - // Restore defaults after each test - setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); - clearConfigValueCache(); - }); - - it("overrides built-in prefixes with custom list", () => { - setAllowedCommandPrefixes(["sops", "doppler"]); - assert.deepEqual([...getAllowedCommandPrefixes()], ["sops", "doppler"]); - }); - - it("custom prefix is allowed through to execution", (t) => { - const stderrChunks: string[] = []; - const originalWrite = process.stderr.write.bind(process.stderr); - process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { - stderrChunks.push(chunk.toString()); - return true; - }; - t.after(() => { - process.stderr.write = originalWrite; - }); - - setAllowedCommandPrefixes(["mycli"]); - resolveConfigValue("!mycli get-secret"); - const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); - assert.equal(blocked, false, "mycli should not be blocked when in the custom allowlist"); - }); - - it("previously-allowed prefix is blocked after override", (t) => { - const stderrChunks: string[] = []; - const originalWrite = process.stderr.write.bind(process.stderr); - process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { - stderrChunks.push(chunk.toString()); - return true; - }; - t.after(() => { - process.stderr.write = originalWrite; - }); - - // 'pass' is in the default list - setAllowedCommandPrefixes(["sops"]); - const result = resolveConfigValue("!pass show secret"); - assert.equal(result, undefined); - const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); - assert.equal(blocked, true, "pass should be blocked when not in the custom allowlist"); - }); - - it("clears cache when overriding prefixes", (t) => { - const stderrChunks: string[] = []; - const originalWrite = process.stderr.write.bind(process.stderr); - process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { - stderrChunks.push(chunk.toString()); - return true; - }; - t.after(() => { - process.stderr.write = originalWrite; - }); - - // First: block 'mycli' under defaults - resolveConfigValue("!mycli get-secret"); - assert.ok(stderrChunks.some((line) => line.includes("Blocked"))); - - stderrChunks.length = 0; - - // Now allow it — cache should be cleared so re-evaluation happens - setAllowedCommandPrefixes(["mycli"]); - resolveConfigValue("!mycli get-secret"); - const blocked = stderrChunks.some((line) => line.includes("Blocked")); - assert.equal(blocked, false, "Should re-evaluate after allowlist change"); - }); -}); diff --git a/packages/pi-coding-agent/src/core/resolve-config-value.test.ts b/packages/pi-coding-agent/src/core/resolve-config-value.test.ts index 9e086d5fc..48a0f8f0e 100644 --- a/packages/pi-coding-agent/src/core/resolve-config-value.test.ts +++ b/packages/pi-coding-agent/src/core/resolve-config-value.test.ts @@ -1,9 +1,11 @@ -import { describe, it, beforeEach } from "node:test"; +import { describe, it, beforeEach, afterEach } from "node:test"; import assert from "node:assert/strict"; import { resolveConfigValue, clearConfigValueCache, SAFE_COMMAND_PREFIXES, + setAllowedCommandPrefixes, + getAllowedCommandPrefixes, } from "./resolve-config-value.js"; beforeEach(() => { @@ -183,3 +185,111 @@ describe("resolveConfigValue — caching", () => { assert.equal(stderrChunks.length, 2); }); }); + +describe("REGRESSION #666: non-default credential tool blocked with no override", () => { + afterEach(() => { + setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + clearConfigValueCache(); + }); + + it("sops is blocked by default, then unblocked by setAllowedCommandPrefixes", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + // Bug: sops is not in SAFE_COMMAND_PREFIXES, so it's blocked + const result = resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); + assert.equal(result, undefined, "sops is blocked by the hardcoded allowlist"); + assert.ok( + stderrChunks.some((line) => line.includes('Blocked disallowed command: "sops"')), + "should log a block message for sops", + ); + + stderrChunks.length = 0; + clearConfigValueCache(); + + // Fix: override the allowlist to include sops + setAllowedCommandPrefixes([...SAFE_COMMAND_PREFIXES, "sops"]); + resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); + + const blockedAfterOverride = stderrChunks.some((line) => + line.includes("Blocked disallowed command"), + ); + assert.equal(blockedAfterOverride, false, "sops must not be blocked after override"); + }); +}); + +describe("setAllowedCommandPrefixes — user override", () => { + afterEach(() => { + setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); + clearConfigValueCache(); + }); + + it("overrides built-in prefixes with custom list", () => { + setAllowedCommandPrefixes(["sops", "doppler"]); + assert.deepEqual([...getAllowedCommandPrefixes()], ["sops", "doppler"]); + }); + + it("custom prefix is allowed through to execution", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + setAllowedCommandPrefixes(["mycli"]); + resolveConfigValue("!mycli get-secret"); + const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); + assert.equal(blocked, false, "mycli should not be blocked when in the custom allowlist"); + }); + + it("previously-allowed prefix is blocked after override", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + setAllowedCommandPrefixes(["sops"]); + const result = resolveConfigValue("!pass show secret"); + assert.equal(result, undefined); + const blocked = stderrChunks.some((line) => line.includes("Blocked disallowed command")); + assert.equal(blocked, true, "pass should be blocked when not in the custom allowlist"); + }); + + it("clears cache when overriding prefixes", (t) => { + const stderrChunks: string[] = []; + const originalWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { + stderrChunks.push(chunk.toString()); + return true; + }; + t.after(() => { + process.stderr.write = originalWrite; + }); + + resolveConfigValue("!mycli get-secret"); + assert.ok(stderrChunks.some((line) => line.includes("Blocked"))); + + stderrChunks.length = 0; + + setAllowedCommandPrefixes(["mycli"]); + resolveConfigValue("!mycli get-secret"); + const blocked = stderrChunks.some((line) => line.includes("Blocked")); + assert.equal(blocked, false, "Should re-evaluate after allowlist change"); + }); +}); diff --git a/src/tests/regression-666.test.ts b/src/tests/regression-666.test.ts deleted file mode 100644 index 166323a0c..000000000 --- a/src/tests/regression-666.test.ts +++ /dev/null @@ -1,113 +0,0 @@ -/** - * Regression tests for the bug introduced by PR #666. - * - * Bug: PR #666 added hardcoded SAFE_COMMAND_PREFIXES and isBlockedUrl() with - * no override mechanism. Users with non-default credential tools (sops, doppler, - * age, etc.) or needing to fetch from internal URLs were silently broken. - * - * These tests import only APIs that exist on both main and this branch. - * On main (before fix): they FAIL because the override functions don't exist. - * On this branch (after fix): they PASS because overrides work. - */ -import { describe, it, beforeEach, afterEach } from "node:test"; -import assert from "node:assert/strict"; -import { - resolveConfigValue, - clearConfigValueCache, - SAFE_COMMAND_PREFIXES, -} from "../../packages/pi-coding-agent/src/core/resolve-config-value.ts"; -import { - isBlockedUrl, -} from "../resources/extensions/search-the-web/url-utils.ts"; - -describe("REGRESSION #666: hardcoded security lists with no override", () => { - beforeEach(() => { - clearConfigValueCache(); - }); - - afterEach(() => { - // Restore defaults — setAllowedCommandPrefixes/setFetchAllowedUrls are - // dynamically imported so we can restore even if they exist. - import("../../packages/pi-coding-agent/src/core/resolve-config-value.ts").then((mod) => { - if (typeof mod.setAllowedCommandPrefixes === "function") { - mod.setAllowedCommandPrefixes(SAFE_COMMAND_PREFIXES); - } - mod.clearConfigValueCache(); - }); - import("../resources/extensions/search-the-web/url-utils.ts").then((mod) => { - if (typeof mod.setFetchAllowedUrls === "function") { - mod.setFetchAllowedUrls([]); - } - }); - }); - - it("non-default credential tool (sops) can be unblocked via override", async (t) => { - const stderrChunks: string[] = []; - const originalWrite = process.stderr.write.bind(process.stderr); - process.stderr.write = (chunk: string | Uint8Array, ...args: unknown[]) => { - stderrChunks.push(chunk.toString()); - return true; - }; - t.after(() => { - process.stderr.write = originalWrite; - }); - - // Confirm the bug: sops is not in the hardcoded allowlist, so it's blocked - const blocked = resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); - assert.equal(blocked, undefined, "sops is blocked by the hardcoded allowlist"); - assert.ok( - stderrChunks.some((line) => line.includes('Blocked disallowed command: "sops"')), - "should log a block message for sops", - ); - - stderrChunks.length = 0; - clearConfigValueCache(); - - // The fix: setAllowedCommandPrefixes must exist and must unblock sops - const mod = await import("../../packages/pi-coding-agent/src/core/resolve-config-value.ts"); - assert.equal( - typeof mod.setAllowedCommandPrefixes, - "function", - "setAllowedCommandPrefixes must be exported (missing = bug #666 not fixed)", - ); - - mod.setAllowedCommandPrefixes([...SAFE_COMMAND_PREFIXES, "sops"]); - resolveConfigValue("!sops decrypt --output-type json secrets.enc.json"); - - const blockedAfterOverride = stderrChunks.some((line) => - line.includes("Blocked disallowed command"), - ); - assert.equal( - blockedAfterOverride, - false, - "sops must not be blocked after adding it to the allowlist", - ); - }); - - it("internal company URL can be unblocked via override", async () => { - const internalUrl = "http://192.168.1.100/internal-docs/api-reference"; - - // Confirm the bug: private IP is blocked with no way to allowlist - assert.equal( - isBlockedUrl(internalUrl), - true, - "private IP is blocked by the hardcoded SSRF blocklist", - ); - - // The fix: setFetchAllowedUrls must exist and must unblock the host - const mod = await import("../resources/extensions/search-the-web/url-utils.ts"); - assert.equal( - typeof mod.setFetchAllowedUrls, - "function", - "setFetchAllowedUrls must be exported (missing = bug #666 not fixed)", - ); - - mod.setFetchAllowedUrls(["192.168.1.100"]); - - assert.equal( - isBlockedUrl(internalUrl), - false, - "private IP must not be blocked after adding it to the allowlist", - ); - }); -}); diff --git a/src/tests/url-utils-override.test.ts b/src/tests/url-utils-override.test.ts deleted file mode 100644 index 381238944..000000000 --- a/src/tests/url-utils-override.test.ts +++ /dev/null @@ -1,54 +0,0 @@ -import { describe, it, beforeEach, afterEach } from "node:test"; -import assert from "node:assert/strict"; -import { isBlockedUrl, setFetchAllowedUrls, getFetchAllowedUrls } from "../resources/extensions/search-the-web/url-utils.ts"; - -describe("setFetchAllowedUrls — user override", () => { - afterEach(() => { - // Reset to empty allowlist after each test - setFetchAllowedUrls([]); - }); - - it("defaults to empty allowlist", () => { - assert.deepEqual(getFetchAllowedUrls(), []); - }); - - it("exempts an allowed hostname from blocking", () => { - assert.equal(isBlockedUrl("http://192.168.1.100/docs"), true, "blocked by default"); - setFetchAllowedUrls(["192.168.1.100"]); - assert.equal(isBlockedUrl("http://192.168.1.100/docs"), false, "allowed after override"); - }); - - it("exempts localhost when explicitly allowed", () => { - assert.equal(isBlockedUrl("http://localhost:3000/api"), true, "blocked by default"); - setFetchAllowedUrls(["localhost"]); - assert.equal(isBlockedUrl("http://localhost:3000/api"), false, "allowed after override"); - }); - - it("exempts cloud metadata hostname when allowed", () => { - assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), true, "blocked by default"); - setFetchAllowedUrls(["metadata.google.internal"]); - assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), false, "allowed after override"); - }); - - it("does not affect URLs not in the allowlist", () => { - setFetchAllowedUrls(["192.168.1.100"]); - assert.equal(isBlockedUrl("http://192.168.1.200/secret"), true, "other private IPs still blocked"); - assert.equal(isBlockedUrl("http://localhost/admin"), true, "localhost still blocked"); - }); - - it("still allows public URLs without configuration", () => { - setFetchAllowedUrls(["192.168.1.100"]); - assert.equal(isBlockedUrl("https://example.com"), false); - }); - - it("still blocks non-HTTP protocols even with allowlist", () => { - setFetchAllowedUrls(["localhost"]); - assert.equal(isBlockedUrl("file:///etc/passwd"), true, "file:// still blocked"); - assert.equal(isBlockedUrl("ftp://localhost/data"), true, "ftp:// still blocked"); - }); - - it("is case-insensitive for hostnames", () => { - setFetchAllowedUrls(["MyHost.Internal"]); - assert.equal(isBlockedUrl("http://myhost.internal/api"), false); - }); -}); diff --git a/src/tests/url-utils.test.ts b/src/tests/url-utils.test.ts index c73b359a7..300dbd084 100644 --- a/src/tests/url-utils.test.ts +++ b/src/tests/url-utils.test.ts @@ -1,6 +1,6 @@ -import { describe, it } from "node:test"; +import { describe, it, afterEach } from "node:test"; import assert from "node:assert/strict"; -import { isBlockedUrl } from "../resources/extensions/search-the-web/url-utils.ts"; +import { isBlockedUrl, setFetchAllowedUrls, getFetchAllowedUrls } from "../resources/extensions/search-the-web/url-utils.ts"; describe("isBlockedUrl — SSRF protection", () => { it("blocks localhost", () => { @@ -57,3 +57,70 @@ describe("isBlockedUrl — SSRF protection", () => { assert.equal(isBlockedUrl("https://1.1.1.1/"), false); }); }); + +describe("REGRESSION #666: private URL blocked with no override", () => { + afterEach(() => { + setFetchAllowedUrls([]); + }); + + it("private IP is blocked by default, then unblocked by setFetchAllowedUrls", () => { + const internalUrl = "http://192.168.1.100/internal-docs/api-reference"; + + // Bug: private IP is blocked with no way to allowlist + assert.equal(isBlockedUrl(internalUrl), true, "private IP is blocked by the hardcoded blocklist"); + + // Fix: override the allowlist to include this host + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl(internalUrl), false, "private IP must not be blocked after override"); + }); +}); + +describe("setFetchAllowedUrls — user override", () => { + afterEach(() => { + setFetchAllowedUrls([]); + }); + + it("defaults to empty allowlist", () => { + assert.deepEqual(getFetchAllowedUrls(), []); + }); + + it("exempts an allowed hostname from blocking", () => { + assert.equal(isBlockedUrl("http://192.168.1.100/docs"), true, "blocked by default"); + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("http://192.168.1.100/docs"), false, "allowed after override"); + }); + + it("exempts localhost when explicitly allowed", () => { + assert.equal(isBlockedUrl("http://localhost:3000/api"), true, "blocked by default"); + setFetchAllowedUrls(["localhost"]); + assert.equal(isBlockedUrl("http://localhost:3000/api"), false, "allowed after override"); + }); + + it("exempts cloud metadata hostname when allowed", () => { + assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), true, "blocked by default"); + setFetchAllowedUrls(["metadata.google.internal"]); + assert.equal(isBlockedUrl("http://metadata.google.internal/computeMetadata/"), false, "allowed after override"); + }); + + it("does not affect URLs not in the allowlist", () => { + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("http://192.168.1.200/secret"), true, "other private IPs still blocked"); + assert.equal(isBlockedUrl("http://localhost/admin"), true, "localhost still blocked"); + }); + + it("still allows public URLs without configuration", () => { + setFetchAllowedUrls(["192.168.1.100"]); + assert.equal(isBlockedUrl("https://example.com"), false); + }); + + it("still blocks non-HTTP protocols even with allowlist", () => { + setFetchAllowedUrls(["localhost"]); + assert.equal(isBlockedUrl("file:///etc/passwd"), true, "file:// still blocked"); + assert.equal(isBlockedUrl("ftp://localhost/data"), true, "ftp:// still blocked"); + }); + + it("is case-insensitive for hostnames", () => { + setFetchAllowedUrls(["MyHost.Internal"]); + assert.equal(isBlockedUrl("http://myhost.internal/api"), false); + }); +}); \ No newline at end of file