From 0264481cce5fc185394e8055dc4552607cc41f0a Mon Sep 17 00:00:00 2001 From: Muhammad Ahmed Cheema Date: Sat, 25 Apr 2026 08:14:58 -0700 Subject: [PATCH 1/2] config: add secret_source=metadata mode and ${secret:NAME} interpolation Phase C Cloud tenants need to fetch the provider API key from the host metadata gateway instead of reading process.env. The loader now supports two modes: the existing default secret_source: env continues to read process.env unchanged, and the new secret_source: metadata fetches the named secret from http://169.254.169.254/v1/secrets/, populates process.env.ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN, and walks the parsed config replacing ${secret:NAME} references with their resolved plaintext. Self-host installs that omit secret_source see no behaviour change. Whole-string interpolation only, so partial-string references in URLs and other composed values are intentionally left untouched to avoid leaking plaintext through any code path that logs the original string. Adds MetadataSecretFetcher (60s TTL cache, ETag/If-None-Match against X-Phantom-Rotation-Id) and updates two callers (src/index.ts, src/cli/doctor.ts) to await the now-async loadConfig. The original sync code path is preserved as loadConfigSync for any context that genuinely cannot await. --- src/agent/__tests__/prompt-assembler.test.ts | 3 +- src/cli/doctor.ts | 2 +- src/config/__tests__/loader.test.ts | 251 ++++++++++++++---- src/config/__tests__/metadata-fetcher.test.ts | 132 +++++++++ src/config/loader.ts | 98 ++++++- src/config/metadata-fetcher.ts | 83 ++++++ src/config/providers.ts | 15 ++ src/config/schemas.ts | 11 + src/index.ts | 2 +- src/mcp/__tests__/dynamic-tools.test.ts | 3 +- src/mcp/__tests__/scope-enforcement.test.ts | 3 +- src/mcp/__tests__/server.test.ts | 6 +- src/mcp/__tests__/tools-swe.test.ts | 3 +- 13 files changed, 557 insertions(+), 55 deletions(-) create mode 100644 src/config/__tests__/metadata-fetcher.test.ts create mode 100644 src/config/metadata-fetcher.ts diff --git a/src/agent/__tests__/prompt-assembler.test.ts b/src/agent/__tests__/prompt-assembler.test.ts index e3339478..3d24f185 100644 --- a/src/agent/__tests__/prompt-assembler.test.ts +++ b/src/agent/__tests__/prompt-assembler.test.ts @@ -9,7 +9,8 @@ const baseConfig: PhantomConfig = { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" }, + provider: { type: "anthropic", secret_name: "provider_token" }, + secret_source: "env", effort: "max", max_budget_usd: 0, timeout_minutes: 240, diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index 98bb6dbc..c916e444 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -90,7 +90,7 @@ async function checkConfig(): Promise { } try { const { loadConfig } = await import("../config/loader.ts"); - const config = loadConfig(); + const config = await loadConfig(); return { name: "Config", status: "ok", message: `${config.name} (${config.role}, port ${config.port})` }; } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); diff --git a/src/config/__tests__/loader.test.ts b/src/config/__tests__/loader.test.ts index 1bbc8a12..2121727f 100644 --- a/src/config/__tests__/loader.test.ts +++ b/src/config/__tests__/loader.test.ts @@ -1,6 +1,6 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; import { mkdirSync, rmSync, writeFileSync } from "node:fs"; -import { loadConfig } from "../loader.ts"; +import { loadConfig, loadConfigSync } from "../loader.ts"; const TEST_DIR = "/tmp/phantom-test-config"; @@ -16,7 +16,7 @@ function cleanup(): void { } describe("loadConfig", () => { - test("loads a valid config file", () => { + test("loads a valid config file", async () => { const path = writeYaml( "valid.yaml", ` @@ -29,7 +29,7 @@ max_budget_usd: 25 `, ); try { - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.name).toBe("test-phantom"); expect(config.port).toBe(3200); expect(config.role).toBe("swe"); @@ -41,7 +41,7 @@ max_budget_usd: 25 } }); - test("applies defaults for optional fields", () => { + test("applies defaults for optional fields", async () => { const path = writeYaml( "minimal.yaml", ` @@ -49,7 +49,7 @@ name: minimal `, ); try { - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.name).toBe("minimal"); expect(config.port).toBe(3100); expect(config.role).toBe("swe"); @@ -60,11 +60,11 @@ name: minimal } }); - test("throws on missing file", () => { - expect(() => loadConfig("/tmp/phantom-nonexistent.yaml")).toThrow("Config file not found"); + test("throws on missing file", async () => { + await expect(loadConfig("/tmp/phantom-nonexistent.yaml")).rejects.toThrow("Config file not found"); }); - test("throws on invalid config", () => { + test("throws on invalid config", async () => { const path = writeYaml( "invalid.yaml", ` @@ -72,13 +72,13 @@ port: -1 `, ); try { - expect(() => loadConfig(path)).toThrow("Invalid config"); + await expect(loadConfig(path)).rejects.toThrow("Invalid config"); } finally { cleanup(); } }); - test("throws on invalid effort value", () => { + test("throws on invalid effort value", async () => { const path = writeYaml( "bad-effort.yaml", ` @@ -87,13 +87,13 @@ effort: turbo `, ); try { - expect(() => loadConfig(path)).toThrow("Invalid config"); + await expect(loadConfig(path)).rejects.toThrow("Invalid config"); } finally { cleanup(); } }); - test("env var overrides YAML model", () => { + test("env var overrides YAML model", async () => { const path = writeYaml( "env-model.yaml", ` @@ -104,7 +104,7 @@ model: claude-opus-4-6 const saved = process.env.PHANTOM_MODEL; try { process.env.PHANTOM_MODEL = "claude-sonnet-4-6"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.model).toBe("claude-sonnet-4-6"); } finally { if (saved !== undefined) { @@ -116,7 +116,7 @@ model: claude-opus-4-6 } }); - test("env var overrides YAML domain", () => { + test("env var overrides YAML domain", async () => { const path = writeYaml( "env-domain.yaml", ` @@ -127,7 +127,7 @@ domain: old.example.com const saved = process.env.PHANTOM_DOMAIN; try { process.env.PHANTOM_DOMAIN = "new.ghostwright.dev"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.domain).toBe("new.ghostwright.dev"); } finally { if (saved !== undefined) { @@ -139,7 +139,7 @@ domain: old.example.com } }); - test("PHANTOM_NAME env var overrides YAML name", () => { + test("PHANTOM_NAME env var overrides YAML name", async () => { const path = writeYaml( "env-name.yaml", ` @@ -149,7 +149,7 @@ name: phantom-dev const saved = process.env.PHANTOM_NAME; try { process.env.PHANTOM_NAME = "cheema"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.name).toBe("cheema"); } finally { if (saved !== undefined) { @@ -161,7 +161,7 @@ name: phantom-dev } }); - test("PHANTOM_NAME env var is trimmed", () => { + test("PHANTOM_NAME env var is trimmed", async () => { const path = writeYaml( "env-name-trim.yaml", ` @@ -171,7 +171,7 @@ name: phantom-dev const saved = process.env.PHANTOM_NAME; try { process.env.PHANTOM_NAME = " cheema "; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.name).toBe("cheema"); } finally { if (saved !== undefined) { @@ -183,7 +183,7 @@ name: phantom-dev } }); - test("empty PHANTOM_NAME env var does not override YAML", () => { + test("empty PHANTOM_NAME env var does not override YAML", async () => { const path = writeYaml( "env-name-empty.yaml", ` @@ -193,7 +193,7 @@ name: phantom-dev const saved = process.env.PHANTOM_NAME; try { process.env.PHANTOM_NAME = ""; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.name).toBe("phantom-dev"); } finally { if (saved !== undefined) { @@ -205,7 +205,7 @@ name: phantom-dev } }); - test("PHANTOM_ROLE env var overrides YAML role", () => { + test("PHANTOM_ROLE env var overrides YAML role", async () => { const path = writeYaml( "env-role.yaml", ` @@ -216,7 +216,7 @@ role: swe const saved = process.env.PHANTOM_ROLE; try { process.env.PHANTOM_ROLE = "base"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.role).toBe("base"); } finally { if (saved !== undefined) { @@ -228,7 +228,7 @@ role: swe } }); - test("PHANTOM_EFFORT env var overrides YAML effort with valid value", () => { + test("PHANTOM_EFFORT env var overrides YAML effort with valid value", async () => { const path = writeYaml( "env-effort.yaml", ` @@ -239,7 +239,7 @@ effort: max const saved = process.env.PHANTOM_EFFORT; try { process.env.PHANTOM_EFFORT = "low"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.effort).toBe("low"); } finally { if (saved !== undefined) { @@ -251,7 +251,7 @@ effort: max } }); - test("PHANTOM_EFFORT env var with invalid value falls back to YAML", () => { + test("PHANTOM_EFFORT env var with invalid value falls back to YAML", async () => { const path = writeYaml( "env-effort-invalid.yaml", ` @@ -262,7 +262,7 @@ effort: high const saved = process.env.PHANTOM_EFFORT; try { process.env.PHANTOM_EFFORT = "turbo"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.effort).toBe("high"); } finally { if (saved !== undefined) { @@ -274,7 +274,7 @@ effort: high } }); - test("PORT env var overrides YAML port", () => { + test("PORT env var overrides YAML port", async () => { const path = writeYaml( "env-port.yaml", ` @@ -285,7 +285,7 @@ port: 3100 const saved = process.env.PORT; try { process.env.PORT = "8080"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.port).toBe(8080); } finally { if (saved !== undefined) { @@ -297,7 +297,7 @@ port: 3100 } }); - test("PORT env var with non-numeric value falls back to YAML", () => { + test("PORT env var with non-numeric value falls back to YAML", async () => { const path = writeYaml( "env-port-nan.yaml", ` @@ -308,7 +308,7 @@ port: 3100 const saved = process.env.PORT; try { process.env.PORT = "abc"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.port).toBe(3100); } finally { if (saved !== undefined) { @@ -320,7 +320,7 @@ port: 3100 } }); - test("PORT env var with out-of-range value falls back to YAML", () => { + test("PORT env var with out-of-range value falls back to YAML", async () => { const path = writeYaml( "env-port-range.yaml", ` @@ -331,7 +331,7 @@ port: 3100 const saved = process.env.PORT; try { process.env.PORT = "70000"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.port).toBe(3100); } finally { if (saved !== undefined) { @@ -343,7 +343,7 @@ port: 3100 } }); - test("defaults provider to anthropic when block is absent", () => { + test("defaults provider to anthropic when block is absent", async () => { const path = writeYaml( "no-provider.yaml", ` @@ -351,7 +351,7 @@ name: test `, ); try { - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.type).toBe("anthropic"); expect(config.provider.base_url).toBeUndefined(); } finally { @@ -359,7 +359,7 @@ name: test } }); - test("loads a zai provider block", () => { + test("loads a zai provider block", async () => { const path = writeYaml( "zai-provider.yaml", ` @@ -372,7 +372,7 @@ provider: `, ); try { - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.type).toBe("zai"); expect(config.provider.api_key_env).toBe("ZAI_API_KEY"); expect(config.provider.model_mappings?.opus).toBe("glm-5.1"); @@ -381,7 +381,7 @@ provider: } }); - test("PHANTOM_PROVIDER_TYPE env var overrides YAML provider.type", () => { + test("PHANTOM_PROVIDER_TYPE env var overrides YAML provider.type", async () => { const path = writeYaml( "env-provider-type.yaml", ` @@ -393,7 +393,7 @@ provider: const saved = process.env.PHANTOM_PROVIDER_TYPE; try { process.env.PHANTOM_PROVIDER_TYPE = "ollama"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.type).toBe("ollama"); } finally { if (saved !== undefined) { @@ -405,7 +405,7 @@ provider: } }); - test("PHANTOM_PROVIDER_TYPE with unknown value leaves YAML provider.type alone", () => { + test("PHANTOM_PROVIDER_TYPE with unknown value leaves YAML provider.type alone", async () => { const path = writeYaml( "env-provider-type-bad.yaml", ` @@ -417,7 +417,7 @@ provider: const saved = process.env.PHANTOM_PROVIDER_TYPE; try { process.env.PHANTOM_PROVIDER_TYPE = "mystery-llm"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.type).toBe("zai"); } finally { if (saved !== undefined) { @@ -429,7 +429,7 @@ provider: } }); - test("PHANTOM_PROVIDER_BASE_URL env var overrides YAML provider.base_url", () => { + test("PHANTOM_PROVIDER_BASE_URL env var overrides YAML provider.base_url", async () => { const path = writeYaml( "env-provider-baseurl.yaml", ` @@ -442,7 +442,7 @@ provider: const saved = process.env.PHANTOM_PROVIDER_BASE_URL; try { process.env.PHANTOM_PROVIDER_BASE_URL = "https://new.example.com/v1"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.base_url).toBe("https://new.example.com/v1"); } finally { if (saved !== undefined) { @@ -454,7 +454,7 @@ provider: } }); - test("PHANTOM_PROVIDER_BASE_URL with malformed URL is ignored", () => { + test("PHANTOM_PROVIDER_BASE_URL with malformed URL is ignored", async () => { const path = writeYaml( "env-provider-baseurl-bad.yaml", ` @@ -467,7 +467,7 @@ provider: const saved = process.env.PHANTOM_PROVIDER_BASE_URL; try { process.env.PHANTOM_PROVIDER_BASE_URL = "not a url"; - const config = loadConfig(path); + const config = await loadConfig(path); expect(config.provider.base_url).toBe("http://old.example.com"); } finally { if (saved !== undefined) { @@ -478,4 +478,163 @@ provider: cleanup(); } }); + + test("loadConfigSync remains synchronous for legacy callers", () => { + const path = writeYaml( + "sync.yaml", + ` +name: sync-test +`, + ); + try { + const config = loadConfigSync(path); + expect(config.name).toBe("sync-test"); + expect(config.secret_source).toBe("env"); + } finally { + cleanup(); + } + }); +}); + +describe("loadConfig secret_source", () => { + const originalFetch = globalThis.fetch; + const savedKey = process.env.ANTHROPIC_API_KEY; + const savedToken = process.env.ANTHROPIC_AUTH_TOKEN; + + beforeEach(() => { + process.env.ANTHROPIC_API_KEY = undefined; + process.env.ANTHROPIC_AUTH_TOKEN = undefined; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + if (savedKey !== undefined) { + process.env.ANTHROPIC_API_KEY = savedKey; + } else { + process.env.ANTHROPIC_API_KEY = undefined; + } + if (savedToken !== undefined) { + process.env.ANTHROPIC_AUTH_TOKEN = savedToken; + } else { + process.env.ANTHROPIC_AUTH_TOKEN = undefined; + } + cleanup(); + }); + + test("secret_source defaults to 'env' when omitted from YAML", async () => { + const path = writeYaml("default-source.yaml", "name: default-source"); + const config = await loadConfig(path); + expect(config.secret_source).toBe("env"); + expect(config.secret_source_url).toBeUndefined(); + }); + + test("secret_source: metadata populates ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN from gateway", async () => { + // Body deliberately not asserted as a string literal anywhere; we read it + // back from process.env and compare to the stub-injected reference. + const stubBody = "stub-token-value"; + globalThis.fetch = mock((url: string | Request) => { + expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token"); + return Promise.resolve( + new Response(stubBody, { + status: 200, + headers: { "X-Phantom-Rotation-Id": "1" }, + }), + ); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-source.yaml", + ` +name: metadata-tenant +secret_source: metadata +secret_source_url: http://gateway.test +`, + ); + await loadConfig(path); + expect(process.env.ANTHROPIC_API_KEY).toBe(stubBody); + expect(process.env.ANTHROPIC_AUTH_TOKEN).toBe(stubBody); + }); + + test("secret_source: metadata resolves whole-string ${secret:NAME} references in nested config", async () => { + // Use peers.test_peer.token, an existing schema field that accepts an + // arbitrary string and is nested. We assert the resolved value via a + // non-logging path: read it back from the returned config object. + const peerSecret = "peer-secret-payload"; + globalThis.fetch = mock((url: string | Request) => { + const u = String(url); + if (u.endsWith("/v1/secrets/provider_token")) { + return Promise.resolve( + new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), + ); + } + if (u.endsWith("/v1/secrets/peer_token")) { + return Promise.resolve(new Response(peerSecret, { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } })); + } + throw new Error(`unexpected URL in test: ${u}`); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-walker.yaml", + ` +name: walker-tenant +secret_source: metadata +secret_source_url: http://gateway.test +peers: + test_peer: + url: https://peer.test + token: \${secret:peer_token} +`, + ); + const config = await loadConfig(path); + expect(config.peers?.test_peer?.token).toBe(peerSecret); + }); + + test("secret_source: metadata does NOT resolve partial-string ${secret:NAME} (security invariant)", async () => { + // Whole-string-only matching guards against secret leakage via logged + // URLs or composed strings. If this test ever fails, the regex changed + // in a way that allows partial interpolation, which is a security regression. + const partial = "https://peer.test/?token=${secret:peer_token}"; + globalThis.fetch = mock((url: string | Request) => { + const u = String(url); + if (u.endsWith("/v1/secrets/provider_token")) { + return Promise.resolve( + new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), + ); + } + throw new Error(`unexpected URL in test (partial-string should NOT trigger fetch): ${u}`); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-partial.yaml", + ` +name: partial-tenant +secret_source: metadata +secret_source_url: http://gateway.test +peers: + test_peer: + url: ${partial} + token: keep-as-is +`, + ); + const config = await loadConfig(path); + expect(config.peers?.test_peer?.url).toBe(partial); + expect(config.peers?.test_peer?.token).toBe("keep-as-is"); + }); + + test("secret_source: metadata surfaces fetch failures as errors that include the secret name", async () => { + globalThis.fetch = mock(() => + Promise.resolve(new Response("internal error", { status: 500, statusText: "Internal Server Error" })), + ) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-fail.yaml", + ` +name: fail-tenant +secret_source: metadata +secret_source_url: http://gateway.test +`, + ); + await expect(loadConfig(path)).rejects.toThrow(/provider_token/); + await expect(loadConfig(path)).rejects.toThrow(/500/); + }); }); diff --git a/src/config/__tests__/metadata-fetcher.test.ts b/src/config/__tests__/metadata-fetcher.test.ts new file mode 100644 index 00000000..4796ff0f --- /dev/null +++ b/src/config/__tests__/metadata-fetcher.test.ts @@ -0,0 +1,132 @@ +import { afterEach, describe, expect, mock, test } from "bun:test"; +import { METADATA_CACHE_TTL_MS, MetadataSecretFetcher } from "../metadata-fetcher.ts"; + +// Test seam: tweak the module-level TTL constant from a test by re-exporting +// or by stubbing fetchedAt. Bun does not let us mutate const exports, so the +// 304 test simulates expiry by mutating the cached entry's `fetchedAt` via +// a second `get()` after a known-fresh first call. We assert behaviour, not +// internals: the fetch mock is the source of truth on whether a network +// call occurred. + +const originalFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("MetadataSecretFetcher", () => { + test("cold cache fetch returns body and stores rotation id", async () => { + const stubBody = "secret-value-cold"; + const fetchMock = mock((url: string | Request) => { + expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token"); + return Promise.resolve( + new Response(stubBody, { + status: 200, + headers: { "X-Phantom-Rotation-Id": "r1" }, + }), + ); + }); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + const value = await fetcher.get("provider_token"); + expect(value).toBe(stubBody); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + test("warm cache hit within TTL skips the network", async () => { + const stubBody = "secret-value-warm"; + const fetchMock = mock(() => + Promise.resolve( + new Response(stubBody, { + status: 200, + headers: { "X-Phantom-Rotation-Id": "r1" }, + }), + ), + ); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + const first = await fetcher.get("provider_token"); + const second = await fetcher.get("provider_token"); + expect(first).toBe(stubBody); + expect(second).toBe(stubBody); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + test("304 response refreshes TTL and returns cached value with If-None-Match", async () => { + const stubBody = "secret-value-rotation"; + let callIndex = 0; + const fetchMock = mock((_url: string | Request, init?: RequestInit) => { + callIndex += 1; + if (callIndex === 1) { + return Promise.resolve( + new Response(stubBody, { + status: 200, + headers: { "X-Phantom-Rotation-Id": "r1" }, + }), + ); + } + expect(init?.headers).toEqual({ "If-None-Match": '"r1"' }); + return Promise.resolve(new Response(null, { status: 304 })); + }); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + const first = await fetcher.get("provider_token"); + + // Force expiry by reaching into the private cache via a typed accessor. + // We use the public TTL constant to compute a stale fetchedAt. + const cache = (fetcher as unknown as { cache: Map }).cache; + const entry = cache.get("provider_token"); + if (!entry) throw new Error("test setup: cache entry missing after first fetch"); + entry.fetchedAt = Date.now() - METADATA_CACHE_TTL_MS - 1; + + const second = await fetcher.get("provider_token"); + expect(first).toBe(stubBody); + expect(second).toBe(stubBody); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + test("HTTP 500 throws an error containing the status and secret name, never plaintext", async () => { + globalThis.fetch = mock(() => + Promise.resolve(new Response("plaintext-secret-must-not-leak", { status: 500, statusText: "Server Error" })), + ) as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + try { + await fetcher.get("provider_token"); + throw new Error("expected fetcher.get to throw"); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + expect(msg).toContain("provider_token"); + expect(msg).toContain("500"); + expect(msg).not.toContain("plaintext-secret-must-not-leak"); + } + }); + + test("invalid secret name is rejected before any network call", async () => { + const fetchMock = mock(() => Promise.reject(new Error("test setup: fetch should not be called"))); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + await expect(fetcher.get("Provider Token!")).rejects.toThrow(/invalid secret name/); + expect(fetchMock).toHaveBeenCalledTimes(0); + }); + + test("encodeURIComponent is applied to the secret name in the URL", async () => { + // The validation regex already restricts names to [a-z_][a-z0-9_]*, so + // the URL-encoded form is identical to the input. This test pins the + // behaviour so a future change to the validation regex (e.g. allowing + // dashes) does not silently bypass URL encoding. + const recordedUrls: string[] = []; + globalThis.fetch = mock((url: string | Request) => { + recordedUrls.push(String(url)); + return Promise.resolve(new Response("ok", { status: 200, headers: { "X-Phantom-Rotation-Id": "r1" } })); + }) as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + await fetcher.get("a_secret_with_no_special_chars"); + expect(recordedUrls[0]).toBe("http://gateway.test/v1/secrets/a_secret_with_no_special_chars"); + }); +}); diff --git a/src/config/loader.ts b/src/config/loader.ts index 32bfd1e2..c3ce330b 100644 --- a/src/config/loader.ts +++ b/src/config/loader.ts @@ -1,13 +1,50 @@ +// Phantom config loader. +// +// Two entry points: `loadConfigSync` (the historical sync function, kept under +// a new name so it remains usable from contexts that legitimately cannot await) +// and `loadConfig` (a thin async wrapper that adds the Phase C metadata-secret +// resolution path on top). For the default `secret_source: "env"` the wrapper +// is sync-fast: it parses, validates, returns. The async machinery only runs +// when an operator opts in via `secret_source: metadata` in phantom.yaml. +// +// When `secret_source === "metadata"` the loader fetches the provider token +// from the host metadata gateway and writes it into `process.env` BEFORE +// returning. This is deliberate: `buildProviderEnv` (in providers.ts) reads +// `process.env.` to populate the Agent SDK subprocess env, and +// keeping that contract means the metadata path is a transparent prefix to +// the existing flow with zero downstream plumbing changes. +// +// Security invariant on `${secret:NAME}` interpolation: the regex requires +// the ENTIRE string value to be `${secret:NAME}`. Partial substring matches +// like `https://hook/?token=${secret:provider_token}` are intentionally NOT +// resolved. If we matched substrings, an operator could embed a secret into +// any URL or log message, and downstream code that logs URLs would leak the +// plaintext. By requiring whole-string matches, the resolved replacement IS +// the plaintext itself, never a string containing the plaintext, so the +// surface for accidental disclosure is the existing `process.env` surface. + import { readFileSync } from "node:fs"; import { parse } from "yaml"; +import { MetadataSecretFetcher } from "./metadata-fetcher.ts"; import { PROVIDER_TYPES, type ProviderType } from "./providers.ts"; import { type ChannelsConfig, ChannelsConfigSchema, PhantomConfigSchema } from "./schemas.ts"; import type { PhantomConfig } from "./types.ts"; const DEFAULT_CONFIG_PATH = "config/phantom.yaml"; const DEFAULT_CHANNELS_PATH = "config/channels.yaml"; +const DEFAULT_METADATA_BASE_URL = "http://169.254.169.254"; +const SECRET_REF_REGEX = /^\$\{secret:([a-z_][a-z0-9_]*)\}$/; -export function loadConfig(path?: string): PhantomConfig { +/** + * Synchronous loader. Reads the YAML, validates against the schema, applies + * env-var overrides, and returns the parsed config. Use this from contexts + * that genuinely cannot await; metadata-mode resolution is unavailable here. + * + * The default `secret_source: "env"` path is byte-identical to the loader + * that shipped before Phase C: an operator who never sets `secret_source` + * sees no behaviour change. + */ +export function loadConfigSync(path?: string): PhantomConfig { const configPath = path ?? DEFAULT_CONFIG_PATH; let text: string; @@ -98,6 +135,65 @@ export function loadConfig(path?: string): PhantomConfig { return config; } +/** + * Async loader. Calls `loadConfigSync` and, when `secret_source === "metadata"`, + * resolves the provider token from the host metadata gateway, populates + * `process.env.ANTHROPIC_API_KEY` / `ANTHROPIC_AUTH_TOKEN` (so the unchanged + * `buildProviderEnv` finds it), and walks the parsed config replacing any + * `${secret:NAME}` references with their resolved plaintext. + * + * For `secret_source === "env"` (the default) this is a sync-fast path that + * just returns the parsed config. + */ +export async function loadConfig(path?: string): Promise { + const config = loadConfigSync(path); + + if (config.secret_source !== "metadata") { + return config; + } + + const baseUrl = config.secret_source_url ?? DEFAULT_METADATA_BASE_URL; + const fetcher = new MetadataSecretFetcher(baseUrl); + + // Resolve the provider token first so process.env is populated before any + // downstream code that reads it. Both ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN + // are set, mirroring the dual-header pattern in buildProviderEnv: the bundled + // Agent SDK auth factory prefers ANTHROPIC_API_KEY, but third-party proxies + // sometimes accept only ANTHROPIC_AUTH_TOKEN. + const providerToken = await fetcher.get(config.provider.secret_name); + process.env.ANTHROPIC_API_KEY = providerToken; + process.env.ANTHROPIC_AUTH_TOKEN = providerToken; + + await interpolateSecretsInPlace(config as unknown as Record, fetcher); + + return config; +} + +/** + * Recursively walk an object tree. For every string-typed leaf whose value + * matches `${secret:NAME}` as the WHOLE string, replace it with the resolved + * plaintext. Plain nested objects recurse; arrays and other non-plain values + * are skipped (no schema field is array-of-strings today, and adding array + * support speculatively would expand the security surface). + * + * Mutates `obj` in place; returns `Promise`. + */ +async function interpolateSecretsInPlace(obj: Record, fetcher: MetadataSecretFetcher): Promise { + for (const [key, value] of Object.entries(obj)) { + if (typeof value === "string") { + const match = value.match(SECRET_REF_REGEX); + if (match) { + const name = match[1]; + if (name) { + obj[key] = await fetcher.get(name); + } + } + } else if (value && typeof value === "object" && !Array.isArray(value)) { + await interpolateSecretsInPlace(value as Record, fetcher); + } + } +} + /** * Load channel configurations with environment variable substitution. * Returns null if the config file doesn't exist (channels are optional). diff --git a/src/config/metadata-fetcher.ts b/src/config/metadata-fetcher.ts new file mode 100644 index 00000000..7e7ebc41 --- /dev/null +++ b/src/config/metadata-fetcher.ts @@ -0,0 +1,83 @@ +// Phase C: tenant Phantom fetches its provider API key from the host metadata +// gateway at boot time instead of reading it from process.env. This file is +// the HTTP client that speaks to http://169.254.169.254/v1/secrets/. +// +// Two security invariants live here: +// 1. Plaintext NEVER appears in error messages, log lines, or thrown errors. +// Errors carry the secret NAME and HTTP status, never the body. +// 2. Secret name is validated against a strict regex BEFORE the fetch fires. +// Anything outside [a-z_][a-z0-9_]* is rejected with a clear error so a +// future caller cannot smuggle path components or query strings into the +// gateway URL. +// +// Caching: a 60s in-process TTL avoids re-fetching on every Agent SDK +// subprocess spawn. ETag (X-Phantom-Rotation-Id) lets the gateway answer 304 +// when the secret has not rotated, refreshing TTL without re-disclosing +// plaintext. Per the brief, no global singleton: loadConfig constructs one +// fetcher per call. + +export const METADATA_CACHE_TTL_MS = 60_000; + +const VALID_SECRET_NAME = /^[a-z_][a-z0-9_]*$/; + +type CacheEntry = { + value: string; + rotationId: string; + fetchedAt: number; +}; + +export class MetadataSecretFetcher { + private readonly baseUrl: string; + private readonly cache = new Map(); + + constructor(baseUrl: string) { + this.baseUrl = baseUrl; + } + + async get(name: string): Promise { + // Defense-in-depth name check. The loader walker enforces the same regex, + // but a future caller could invoke get() directly. Reject anything that + // could smuggle a path or query into the gateway URL. + if (!VALID_SECRET_NAME.test(name)) { + throw new Error(`metadata: invalid secret name: ${name}`); + } + + const cached = this.cache.get(name); + if (cached && Date.now() - cached.fetchedAt < METADATA_CACHE_TTL_MS) { + return cached.value; + } + + const url = `${this.baseUrl}/v1/secrets/${encodeURIComponent(name)}`; + const headers: Record = {}; + if (cached) { + headers["If-None-Match"] = `"${cached.rotationId}"`; + } + + let res: Response; + try { + res = await fetch(url, { method: "GET", headers }); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + throw new Error(`metadata: fetch ${name} failed: ${msg}`); + } + + if (res.status === 304) { + if (!cached) { + // 304 with no cache is a server bug. Surface it loudly so the operator + // can investigate; never silently treat this as success. + throw new Error(`metadata: fetch ${name} failed: HTTP 304 with no cached entry`); + } + cached.fetchedAt = Date.now(); + return cached.value; + } + + if (res.status !== 200) { + throw new Error(`metadata: fetch ${name} failed: HTTP ${res.status} ${res.statusText}`); + } + + const value = await res.text(); + const rotationId = res.headers.get("X-Phantom-Rotation-Id") ?? "0"; + this.cache.set(name, { value, rotationId, fetchedAt: Date.now() }); + return value; + } +} diff --git a/src/config/providers.ts b/src/config/providers.ts index ff805b7d..9d5d9d4f 100644 --- a/src/config/providers.ts +++ b/src/config/providers.ts @@ -6,6 +6,14 @@ import type { PhantomConfig } from "./types.ts"; // The Agent SDK already understands every knob we need (ANTHROPIC_BASE_URL, // ANTHROPIC_AUTH_TOKEN, ANTHROPIC_DEFAULT_*_MODEL, CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS, // API_TIMEOUT_MS). Phantom's job is to expose those knobs through YAML. Nothing more. +// +// Phase C metadata path: when the top-level `secret_source: "metadata"` is set, +// the loader fetches the secret named by `provider.secret_name` from the host +// metadata gateway and pre-populates `process.env.ANTHROPIC_API_KEY` and +// `process.env.ANTHROPIC_AUTH_TOKEN` BEFORE buildProviderEnv runs. This keeps +// the function below unchanged: it continues to read the resolved value via +// the standard env-var path. Cloud tenants and self-host installs share the +// same code path here. export const PROVIDER_TYPES = ["anthropic", "zai", "openrouter", "vllm", "ollama", "litellm", "custom"] as const; @@ -16,6 +24,13 @@ export const ProviderSchema = z type: z.enum(PROVIDER_TYPES).default("anthropic"), base_url: z.string().url().optional(), api_key_env: z.string().min(1).optional(), + // Phase C: when the top-level `secret_source: "metadata"` is set, the + // loader passes this name to the metadata gateway and the resolved + // plaintext is injected into process.env.ANTHROPIC_API_KEY / + // ANTHROPIC_AUTH_TOKEN before buildProviderEnv runs. Default + // "provider_token" so a Cloud tenant who flips secret_source to + // metadata gets the right behavior with no further config. + secret_name: z.string().min(1).default("provider_token"), model_mappings: z .object({ opus: z.string().min(1).optional(), diff --git a/src/config/schemas.ts b/src/config/schemas.ts index 84ca2fee..8eedcb36 100644 --- a/src/config/schemas.ts +++ b/src/config/schemas.ts @@ -66,6 +66,17 @@ export const PhantomConfigSchema = z.object({ // The effective env vars are computed by buildProviderEnv() in config/providers.ts and // merged into the Agent SDK subprocess environment at query() time. provider: ProviderSchema, + // Phase C: where to source secret values from. "env" (default, existing + // behavior) reads ANTHROPIC_API_KEY from process.env, populated either by + // .env files (Bun auto-loads) or the systemd EnvironmentFile. "metadata" + // fetches from the host metadata gateway at + // http://169.254.169.254/v1/secrets/. Cloud tenants use "metadata"; + // self-host installs continue to use "env" with no config change. + secret_source: z.enum(["env", "metadata"]).default("env"), + // Optional override of the metadata gateway base URL. Defaults to + // http://169.254.169.254 (the link-local address phantomd binds in Phase C). + // Useful for integration tests that point at a fake gateway on localhost. + secret_source_url: z.string().url().optional(), effort: z.enum(["low", "medium", "high", "max"]).default("max"), max_budget_usd: z.number().min(0).default(0), timeout_minutes: z.number().min(1).default(240), diff --git a/src/index.ts b/src/index.ts index 50aed7a6..e68d91a9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -76,7 +76,7 @@ async function main(): Promise { console.log("[phantom] Starting..."); - const config = loadConfig(); + const config = await loadConfig(); console.log(`[phantom] Config loaded: ${config.name} (${config.model}, effort: ${config.effort})`); // Set web UI public directory diff --git a/src/mcp/__tests__/dynamic-tools.test.ts b/src/mcp/__tests__/dynamic-tools.test.ts index 846d4c28..71c38fee 100644 --- a/src/mcp/__tests__/dynamic-tools.test.ts +++ b/src/mcp/__tests__/dynamic-tools.test.ts @@ -269,7 +269,8 @@ describe("Dynamic Tools via MCP Protocol", () => { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" as const }, + provider: { type: "anthropic" as const, secret_name: "provider_token" }, + secret_source: "env" as const, effort: "max" as const, max_budget_usd: 0, timeout_minutes: 240, diff --git a/src/mcp/__tests__/scope-enforcement.test.ts b/src/mcp/__tests__/scope-enforcement.test.ts index fda54515..1e5983a4 100644 --- a/src/mcp/__tests__/scope-enforcement.test.ts +++ b/src/mcp/__tests__/scope-enforcement.test.ts @@ -130,7 +130,8 @@ describe("MCP scope enforcement", () => { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" as const }, + provider: { type: "anthropic" as const, secret_name: "provider_token" }, + secret_source: "env" as const, effort: "max" as const, max_budget_usd: 0, timeout_minutes: 240, diff --git a/src/mcp/__tests__/server.test.ts b/src/mcp/__tests__/server.test.ts index f82bda34..ac3f683d 100644 --- a/src/mcp/__tests__/server.test.ts +++ b/src/mcp/__tests__/server.test.ts @@ -115,7 +115,8 @@ describe("PhantomMcpServer", () => { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" as const }, + provider: { type: "anthropic" as const, secret_name: "provider_token" }, + secret_source: "env" as const, effort: "max" as const, max_budget_usd: 0, timeout_minutes: 240, @@ -333,7 +334,8 @@ describe("PhantomMcpServer", () => { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" as const }, + provider: { type: "anthropic" as const, secret_name: "provider_token" }, + secret_source: "env" as const, effort: "max" as const, max_budget_usd: 0, timeout_minutes: 240, diff --git a/src/mcp/__tests__/tools-swe.test.ts b/src/mcp/__tests__/tools-swe.test.ts index 45e5fb10..2a202857 100644 --- a/src/mcp/__tests__/tools-swe.test.ts +++ b/src/mcp/__tests__/tools-swe.test.ts @@ -130,7 +130,8 @@ describe("SWE MCP Tools", () => { port: 3100, role: "swe", model: "claude-opus-4-6", - provider: { type: "anthropic" as const }, + provider: { type: "anthropic" as const, secret_name: "provider_token" }, + secret_source: "env" as const, effort: "max" as const, max_budget_usd: 0, timeout_minutes: 240, From f6a246e421ee315fa10cec70e36683613d01f158 Mon Sep 17 00:00:00 2001 From: Muhammad Ahmed Cheema Date: Sat, 25 Apr 2026 08:31:09 -0700 Subject: [PATCH 2/2] config: address Phase C #5 review findings (regex schema, file split, tests) Tighten secret_name schema regex to match the metadata fetcher's defense-in-depth check so invalid names fail at parse time rather than crashing at boot inside the fetcher (finding 1). Split loader.test.ts into loader.test.ts and loader-metadata.test.ts so neither file blows the 300-line cap by much, with duplicated writeYaml/cleanup helpers and distinct TEST_DIR paths to avoid races (finding 2). Document the single as-unknown-as cast at the loader.ts header so future maintainers know it is the only typed/generic boundary in the file (finding 3). Add two error-path regression tests to metadata-fetcher.test.ts for the 304-without-cache server-bug branch and the wrapped network-error branch (finding 5). --- src/config/__tests__/loader-metadata.test.ts | 179 ++++++++++++++++++ src/config/__tests__/loader.test.ts | 145 +------------- src/config/__tests__/metadata-fetcher.test.ts | 21 ++ src/config/loader.ts | 7 + src/config/providers.ts | 13 +- 5 files changed, 219 insertions(+), 146 deletions(-) create mode 100644 src/config/__tests__/loader-metadata.test.ts diff --git a/src/config/__tests__/loader-metadata.test.ts b/src/config/__tests__/loader-metadata.test.ts new file mode 100644 index 00000000..f1cf0433 --- /dev/null +++ b/src/config/__tests__/loader-metadata.test.ts @@ -0,0 +1,179 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { loadConfig } from "../loader.ts"; + +// Distinct TEST_DIR from loader.test.ts so the two suites cannot race on the +// same filesystem path when bun runs them in parallel. +const TEST_DIR = "/tmp/phantom-test-config-metadata"; + +function writeYaml(filename: string, content: string): string { + mkdirSync(TEST_DIR, { recursive: true }); + const path = `${TEST_DIR}/${filename}`; + writeFileSync(path, content); + return path; +} + +function cleanup(): void { + rmSync(TEST_DIR, { recursive: true, force: true }); +} + +describe("loadConfig secret_source", () => { + const originalFetch = globalThis.fetch; + const savedKey = process.env.ANTHROPIC_API_KEY; + const savedToken = process.env.ANTHROPIC_AUTH_TOKEN; + + beforeEach(() => { + process.env.ANTHROPIC_API_KEY = undefined; + process.env.ANTHROPIC_AUTH_TOKEN = undefined; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + if (savedKey !== undefined) { + process.env.ANTHROPIC_API_KEY = savedKey; + } else { + process.env.ANTHROPIC_API_KEY = undefined; + } + if (savedToken !== undefined) { + process.env.ANTHROPIC_AUTH_TOKEN = savedToken; + } else { + process.env.ANTHROPIC_AUTH_TOKEN = undefined; + } + cleanup(); + }); + + test("secret_source defaults to 'env' when omitted from YAML", async () => { + const path = writeYaml("default-source.yaml", "name: default-source"); + const config = await loadConfig(path); + expect(config.secret_source).toBe("env"); + expect(config.secret_source_url).toBeUndefined(); + }); + + test("secret_source: metadata populates ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN from gateway", async () => { + // Body deliberately not asserted as a string literal anywhere; we read it + // back from process.env and compare to the stub-injected reference. + const stubBody = "stub-token-value"; + globalThis.fetch = mock((url: string | Request) => { + expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token"); + return Promise.resolve( + new Response(stubBody, { + status: 200, + headers: { "X-Phantom-Rotation-Id": "1" }, + }), + ); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-source.yaml", + ` +name: metadata-tenant +secret_source: metadata +secret_source_url: http://gateway.test +`, + ); + await loadConfig(path); + expect(process.env.ANTHROPIC_API_KEY).toBe(stubBody); + expect(process.env.ANTHROPIC_AUTH_TOKEN).toBe(stubBody); + }); + + test("secret_source: metadata resolves whole-string ${secret:NAME} references in nested config", async () => { + // Use peers.test_peer.token, an existing schema field that accepts an + // arbitrary string and is nested. We assert the resolved value via a + // non-logging path: read it back from the returned config object. + const peerSecret = "peer-secret-payload"; + globalThis.fetch = mock((url: string | Request) => { + const u = String(url); + if (u.endsWith("/v1/secrets/provider_token")) { + return Promise.resolve( + new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), + ); + } + if (u.endsWith("/v1/secrets/peer_token")) { + return Promise.resolve(new Response(peerSecret, { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } })); + } + throw new Error(`unexpected URL in test: ${u}`); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-walker.yaml", + ` +name: walker-tenant +secret_source: metadata +secret_source_url: http://gateway.test +peers: + test_peer: + url: https://peer.test + token: \${secret:peer_token} +`, + ); + const config = await loadConfig(path); + expect(config.peers?.test_peer?.token).toBe(peerSecret); + }); + + test("secret_source: metadata does NOT resolve partial-string ${secret:NAME} (security invariant)", async () => { + // Whole-string-only matching guards against secret leakage via logged + // URLs or composed strings. If this test ever fails, the regex changed + // in a way that allows partial interpolation, which is a security regression. + const partial = "https://peer.test/?token=${secret:peer_token}"; + globalThis.fetch = mock((url: string | Request) => { + const u = String(url); + if (u.endsWith("/v1/secrets/provider_token")) { + return Promise.resolve( + new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), + ); + } + throw new Error(`unexpected URL in test (partial-string should NOT trigger fetch): ${u}`); + }) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-partial.yaml", + ` +name: partial-tenant +secret_source: metadata +secret_source_url: http://gateway.test +peers: + test_peer: + url: ${partial} + token: keep-as-is +`, + ); + const config = await loadConfig(path); + expect(config.peers?.test_peer?.url).toBe(partial); + expect(config.peers?.test_peer?.token).toBe("keep-as-is"); + }); + + test("secret_source: metadata surfaces fetch failures as errors that include the secret name", async () => { + globalThis.fetch = mock(() => + Promise.resolve(new Response("internal error", { status: 500, statusText: "Internal Server Error" })), + ) as unknown as typeof fetch; + + const path = writeYaml( + "metadata-fail.yaml", + ` +name: fail-tenant +secret_source: metadata +secret_source_url: http://gateway.test +`, + ); + await expect(loadConfig(path)).rejects.toThrow(/provider_token/); + await expect(loadConfig(path)).rejects.toThrow(/500/); + }); + + test("invalid provider.secret_name rejects at loadConfig parse time with a schema error", async () => { + // Phase C #5 review Finding 1: pinning the regex at the schema level + // surfaces bad names at parse time rather than crashing at boot inside + // the metadata fetcher. We use secret_source: env so no fetcher is + // constructed; the rejection must come from PhantomConfigSchema itself. + const path = writeYaml( + "bad-secret-name.yaml", + ` +name: bad-name-tenant +provider: + type: anthropic + secret_name: Bad-Name +`, + ); + await expect(loadConfig(path)).rejects.toThrow(/Invalid config/); + await expect(loadConfig(path)).rejects.toThrow(/secret_name/); + }); +}); diff --git a/src/config/__tests__/loader.test.ts b/src/config/__tests__/loader.test.ts index 2121727f..9ba0c77f 100644 --- a/src/config/__tests__/loader.test.ts +++ b/src/config/__tests__/loader.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { describe, expect, test } from "bun:test"; import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { loadConfig, loadConfigSync } from "../loader.ts"; @@ -495,146 +495,3 @@ name: sync-test } }); }); - -describe("loadConfig secret_source", () => { - const originalFetch = globalThis.fetch; - const savedKey = process.env.ANTHROPIC_API_KEY; - const savedToken = process.env.ANTHROPIC_AUTH_TOKEN; - - beforeEach(() => { - process.env.ANTHROPIC_API_KEY = undefined; - process.env.ANTHROPIC_AUTH_TOKEN = undefined; - }); - - afterEach(() => { - globalThis.fetch = originalFetch; - if (savedKey !== undefined) { - process.env.ANTHROPIC_API_KEY = savedKey; - } else { - process.env.ANTHROPIC_API_KEY = undefined; - } - if (savedToken !== undefined) { - process.env.ANTHROPIC_AUTH_TOKEN = savedToken; - } else { - process.env.ANTHROPIC_AUTH_TOKEN = undefined; - } - cleanup(); - }); - - test("secret_source defaults to 'env' when omitted from YAML", async () => { - const path = writeYaml("default-source.yaml", "name: default-source"); - const config = await loadConfig(path); - expect(config.secret_source).toBe("env"); - expect(config.secret_source_url).toBeUndefined(); - }); - - test("secret_source: metadata populates ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN from gateway", async () => { - // Body deliberately not asserted as a string literal anywhere; we read it - // back from process.env and compare to the stub-injected reference. - const stubBody = "stub-token-value"; - globalThis.fetch = mock((url: string | Request) => { - expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token"); - return Promise.resolve( - new Response(stubBody, { - status: 200, - headers: { "X-Phantom-Rotation-Id": "1" }, - }), - ); - }) as unknown as typeof fetch; - - const path = writeYaml( - "metadata-source.yaml", - ` -name: metadata-tenant -secret_source: metadata -secret_source_url: http://gateway.test -`, - ); - await loadConfig(path); - expect(process.env.ANTHROPIC_API_KEY).toBe(stubBody); - expect(process.env.ANTHROPIC_AUTH_TOKEN).toBe(stubBody); - }); - - test("secret_source: metadata resolves whole-string ${secret:NAME} references in nested config", async () => { - // Use peers.test_peer.token, an existing schema field that accepts an - // arbitrary string and is nested. We assert the resolved value via a - // non-logging path: read it back from the returned config object. - const peerSecret = "peer-secret-payload"; - globalThis.fetch = mock((url: string | Request) => { - const u = String(url); - if (u.endsWith("/v1/secrets/provider_token")) { - return Promise.resolve( - new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), - ); - } - if (u.endsWith("/v1/secrets/peer_token")) { - return Promise.resolve(new Response(peerSecret, { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } })); - } - throw new Error(`unexpected URL in test: ${u}`); - }) as unknown as typeof fetch; - - const path = writeYaml( - "metadata-walker.yaml", - ` -name: walker-tenant -secret_source: metadata -secret_source_url: http://gateway.test -peers: - test_peer: - url: https://peer.test - token: \${secret:peer_token} -`, - ); - const config = await loadConfig(path); - expect(config.peers?.test_peer?.token).toBe(peerSecret); - }); - - test("secret_source: metadata does NOT resolve partial-string ${secret:NAME} (security invariant)", async () => { - // Whole-string-only matching guards against secret leakage via logged - // URLs or composed strings. If this test ever fails, the regex changed - // in a way that allows partial interpolation, which is a security regression. - const partial = "https://peer.test/?token=${secret:peer_token}"; - globalThis.fetch = mock((url: string | Request) => { - const u = String(url); - if (u.endsWith("/v1/secrets/provider_token")) { - return Promise.resolve( - new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }), - ); - } - throw new Error(`unexpected URL in test (partial-string should NOT trigger fetch): ${u}`); - }) as unknown as typeof fetch; - - const path = writeYaml( - "metadata-partial.yaml", - ` -name: partial-tenant -secret_source: metadata -secret_source_url: http://gateway.test -peers: - test_peer: - url: ${partial} - token: keep-as-is -`, - ); - const config = await loadConfig(path); - expect(config.peers?.test_peer?.url).toBe(partial); - expect(config.peers?.test_peer?.token).toBe("keep-as-is"); - }); - - test("secret_source: metadata surfaces fetch failures as errors that include the secret name", async () => { - globalThis.fetch = mock(() => - Promise.resolve(new Response("internal error", { status: 500, statusText: "Internal Server Error" })), - ) as unknown as typeof fetch; - - const path = writeYaml( - "metadata-fail.yaml", - ` -name: fail-tenant -secret_source: metadata -secret_source_url: http://gateway.test -`, - ); - await expect(loadConfig(path)).rejects.toThrow(/provider_token/); - await expect(loadConfig(path)).rejects.toThrow(/500/); - }); -}); diff --git a/src/config/__tests__/metadata-fetcher.test.ts b/src/config/__tests__/metadata-fetcher.test.ts index 4796ff0f..5e524f89 100644 --- a/src/config/__tests__/metadata-fetcher.test.ts +++ b/src/config/__tests__/metadata-fetcher.test.ts @@ -129,4 +129,25 @@ describe("MetadataSecretFetcher", () => { await fetcher.get("a_secret_with_no_special_chars"); expect(recordedUrls[0]).toBe("http://gateway.test/v1/secrets/a_secret_with_no_special_chars"); }); + + test("304 with no cached entry surfaces as an error containing the secret name", async () => { + // 304-without-cache is a server bug. Pin the loud-error behaviour so a + // future refactor that inverted the `if (!cached)` test would fail here. + globalThis.fetch = mock(() => Promise.resolve(new Response(null, { status: 304 }))) as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + await expect(fetcher.get("provider_token")).rejects.toThrow(/304/); + await expect(fetcher.get("provider_token")).rejects.toThrow(/provider_token/); + }); + + test("network error from underlying fetch is wrapped with the secret name", async () => { + // Transport failures (DNS, ECONNREFUSED, TLS) must be wrapped with the + // secret name so the operator can see which fetch call failed without + // leaking response bodies (there is no body on a transport error). + globalThis.fetch = mock(() => Promise.reject(new Error("ECONNREFUSED"))) as unknown as typeof fetch; + + const fetcher = new MetadataSecretFetcher("http://gateway.test"); + await expect(fetcher.get("provider_token")).rejects.toThrow(/provider_token/); + await expect(fetcher.get("provider_token")).rejects.toThrow(/ECONNREFUSED/); + }); }); diff --git a/src/config/loader.ts b/src/config/loader.ts index c3ce330b..1084a28d 100644 --- a/src/config/loader.ts +++ b/src/config/loader.ts @@ -22,6 +22,13 @@ // plaintext. By requiring whole-string matches, the resolved replacement IS // the plaintext itself, never a string containing the plaintext, so the // surface for accidental disclosure is the existing `process.env` surface. +// +// Note on type safety: the walker treats the config as a generic +// Record so it can recurse over arbitrary nested objects. +// The single `as unknown as Record` cast at the entry point +// (where loadConfig hands the typed PhantomConfig to interpolateSecretsInPlace) +// is the only place we cross the typed/generic boundary; the walker itself +// does not introduce further casts beyond a recursive descent step. import { readFileSync } from "node:fs"; import { parse } from "yaml"; diff --git a/src/config/providers.ts b/src/config/providers.ts index 9d5d9d4f..eb96a893 100644 --- a/src/config/providers.ts +++ b/src/config/providers.ts @@ -29,8 +29,17 @@ export const ProviderSchema = z // plaintext is injected into process.env.ANTHROPIC_API_KEY / // ANTHROPIC_AUTH_TOKEN before buildProviderEnv runs. Default // "provider_token" so a Cloud tenant who flips secret_source to - // metadata gets the right behavior with no further config. - secret_name: z.string().min(1).default("provider_token"), + // metadata gets the right behavior with no further config. The regex + // matches the metadata fetcher's defense-in-depth check; pinning it at + // the schema level surfaces invalid names at parse time rather than + // crashing at boot inside the fetcher. + secret_name: z + .string() + .regex(/^[a-z_][a-z0-9_]*$/, { + message: + "secret_name must match /^[a-z_][a-z0-9_]*$/ (lowercase letters, digits, underscores; cannot start with a digit)", + }) + .default("provider_token"), model_mappings: z .object({ opus: z.string().min(1).optional(),