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 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/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.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