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