Skip to content

Commit f6a246e

Browse files
mcheemaamcheemaa
committed
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).
1 parent 0264481 commit f6a246e

5 files changed

Lines changed: 219 additions & 146 deletions

File tree

‎src/config/__tests__/loader-metadata.test.ts‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
2+
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
3+
import { loadConfig } from "../loader.ts";
4+
5+
// Distinct TEST_DIR from loader.test.ts so the two suites cannot race on the
6+
// same filesystem path when bun runs them in parallel.
7+
const TEST_DIR = "/tmp/phantom-test-config-metadata";
8+
9+
function writeYaml(filename: string, content: string): string {
10+
mkdirSync(TEST_DIR, { recursive: true });
11+
const path = `${TEST_DIR}/${filename}`;
12+
writeFileSync(path, content);
13+
return path;
14+
}
15+
16+
function cleanup(): void {
17+
rmSync(TEST_DIR, { recursive: true, force: true });
18+
}
19+
20+
describe("loadConfig secret_source", () => {
21+
const originalFetch = globalThis.fetch;
22+
const savedKey = process.env.ANTHROPIC_API_KEY;
23+
const savedToken = process.env.ANTHROPIC_AUTH_TOKEN;
24+
25+
beforeEach(() => {
26+
process.env.ANTHROPIC_API_KEY = undefined;
27+
process.env.ANTHROPIC_AUTH_TOKEN = undefined;
28+
});
29+
30+
afterEach(() => {
31+
globalThis.fetch = originalFetch;
32+
if (savedKey !== undefined) {
33+
process.env.ANTHROPIC_API_KEY = savedKey;
34+
} else {
35+
process.env.ANTHROPIC_API_KEY = undefined;
36+
}
37+
if (savedToken !== undefined) {
38+
process.env.ANTHROPIC_AUTH_TOKEN = savedToken;
39+
} else {
40+
process.env.ANTHROPIC_AUTH_TOKEN = undefined;
41+
}
42+
cleanup();
43+
});
44+
45+
test("secret_source defaults to 'env' when omitted from YAML", async () => {
46+
const path = writeYaml("default-source.yaml", "name: default-source");
47+
const config = await loadConfig(path);
48+
expect(config.secret_source).toBe("env");
49+
expect(config.secret_source_url).toBeUndefined();
50+
});
51+
52+
test("secret_source: metadata populates ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN from gateway", async () => {
53+
// Body deliberately not asserted as a string literal anywhere; we read it
54+
// back from process.env and compare to the stub-injected reference.
55+
const stubBody = "stub-token-value";
56+
globalThis.fetch = mock((url: string | Request) => {
57+
expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token");
58+
return Promise.resolve(
59+
new Response(stubBody, {
60+
status: 200,
61+
headers: { "X-Phantom-Rotation-Id": "1" },
62+
}),
63+
);
64+
}) as unknown as typeof fetch;
65+
66+
const path = writeYaml(
67+
"metadata-source.yaml",
68+
`
69+
name: metadata-tenant
70+
secret_source: metadata
71+
secret_source_url: http://gateway.test
72+
`,
73+
);
74+
await loadConfig(path);
75+
expect(process.env.ANTHROPIC_API_KEY).toBe(stubBody);
76+
expect(process.env.ANTHROPIC_AUTH_TOKEN).toBe(stubBody);
77+
});
78+
79+
test("secret_source: metadata resolves whole-string ${secret:NAME} references in nested config", async () => {
80+
// Use peers.test_peer.token, an existing schema field that accepts an
81+
// arbitrary string and is nested. We assert the resolved value via a
82+
// non-logging path: read it back from the returned config object.
83+
const peerSecret = "peer-secret-payload";
84+
globalThis.fetch = mock((url: string | Request) => {
85+
const u = String(url);
86+
if (u.endsWith("/v1/secrets/provider_token")) {
87+
return Promise.resolve(
88+
new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }),
89+
);
90+
}
91+
if (u.endsWith("/v1/secrets/peer_token")) {
92+
return Promise.resolve(new Response(peerSecret, { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }));
93+
}
94+
throw new Error(`unexpected URL in test: ${u}`);
95+
}) as unknown as typeof fetch;
96+
97+
const path = writeYaml(
98+
"metadata-walker.yaml",
99+
`
100+
name: walker-tenant
101+
secret_source: metadata
102+
secret_source_url: http://gateway.test
103+
peers:
104+
test_peer:
105+
url: https://peer.test
106+
token: \${secret:peer_token}
107+
`,
108+
);
109+
const config = await loadConfig(path);
110+
expect(config.peers?.test_peer?.token).toBe(peerSecret);
111+
});
112+
113+
test("secret_source: metadata does NOT resolve partial-string ${secret:NAME} (security invariant)", async () => {
114+
// Whole-string-only matching guards against secret leakage via logged
115+
// URLs or composed strings. If this test ever fails, the regex changed
116+
// in a way that allows partial interpolation, which is a security regression.
117+
const partial = "https://peer.test/?token=${secret:peer_token}";
118+
globalThis.fetch = mock((url: string | Request) => {
119+
const u = String(url);
120+
if (u.endsWith("/v1/secrets/provider_token")) {
121+
return Promise.resolve(
122+
new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }),
123+
);
124+
}
125+
throw new Error(`unexpected URL in test (partial-string should NOT trigger fetch): ${u}`);
126+
}) as unknown as typeof fetch;
127+
128+
const path = writeYaml(
129+
"metadata-partial.yaml",
130+
`
131+
name: partial-tenant
132+
secret_source: metadata
133+
secret_source_url: http://gateway.test
134+
peers:
135+
test_peer:
136+
url: ${partial}
137+
token: keep-as-is
138+
`,
139+
);
140+
const config = await loadConfig(path);
141+
expect(config.peers?.test_peer?.url).toBe(partial);
142+
expect(config.peers?.test_peer?.token).toBe("keep-as-is");
143+
});
144+
145+
test("secret_source: metadata surfaces fetch failures as errors that include the secret name", async () => {
146+
globalThis.fetch = mock(() =>
147+
Promise.resolve(new Response("internal error", { status: 500, statusText: "Internal Server Error" })),
148+
) as unknown as typeof fetch;
149+
150+
const path = writeYaml(
151+
"metadata-fail.yaml",
152+
`
153+
name: fail-tenant
154+
secret_source: metadata
155+
secret_source_url: http://gateway.test
156+
`,
157+
);
158+
await expect(loadConfig(path)).rejects.toThrow(/provider_token/);
159+
await expect(loadConfig(path)).rejects.toThrow(/500/);
160+
});
161+
162+
test("invalid provider.secret_name rejects at loadConfig parse time with a schema error", async () => {
163+
// Phase C #5 review Finding 1: pinning the regex at the schema level
164+
// surfaces bad names at parse time rather than crashing at boot inside
165+
// the metadata fetcher. We use secret_source: env so no fetcher is
166+
// constructed; the rejection must come from PhantomConfigSchema itself.
167+
const path = writeYaml(
168+
"bad-secret-name.yaml",
169+
`
170+
name: bad-name-tenant
171+
provider:
172+
type: anthropic
173+
secret_name: Bad-Name
174+
`,
175+
);
176+
await expect(loadConfig(path)).rejects.toThrow(/Invalid config/);
177+
await expect(loadConfig(path)).rejects.toThrow(/secret_name/);
178+
});
179+
});

‎src/config/__tests__/loader.test.ts‎

Lines changed: 1 addition & 144 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
1+
import { describe, expect, test } from "bun:test";
22
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
33
import { loadConfig, loadConfigSync } from "../loader.ts";
44

@@ -495,146 +495,3 @@ name: sync-test
495495
}
496496
});
497497
});
498-
499-
describe("loadConfig secret_source", () => {
500-
const originalFetch = globalThis.fetch;
501-
const savedKey = process.env.ANTHROPIC_API_KEY;
502-
const savedToken = process.env.ANTHROPIC_AUTH_TOKEN;
503-
504-
beforeEach(() => {
505-
process.env.ANTHROPIC_API_KEY = undefined;
506-
process.env.ANTHROPIC_AUTH_TOKEN = undefined;
507-
});
508-
509-
afterEach(() => {
510-
globalThis.fetch = originalFetch;
511-
if (savedKey !== undefined) {
512-
process.env.ANTHROPIC_API_KEY = savedKey;
513-
} else {
514-
process.env.ANTHROPIC_API_KEY = undefined;
515-
}
516-
if (savedToken !== undefined) {
517-
process.env.ANTHROPIC_AUTH_TOKEN = savedToken;
518-
} else {
519-
process.env.ANTHROPIC_AUTH_TOKEN = undefined;
520-
}
521-
cleanup();
522-
});
523-
524-
test("secret_source defaults to 'env' when omitted from YAML", async () => {
525-
const path = writeYaml("default-source.yaml", "name: default-source");
526-
const config = await loadConfig(path);
527-
expect(config.secret_source).toBe("env");
528-
expect(config.secret_source_url).toBeUndefined();
529-
});
530-
531-
test("secret_source: metadata populates ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN from gateway", async () => {
532-
// Body deliberately not asserted as a string literal anywhere; we read it
533-
// back from process.env and compare to the stub-injected reference.
534-
const stubBody = "stub-token-value";
535-
globalThis.fetch = mock((url: string | Request) => {
536-
expect(String(url)).toBe("http://gateway.test/v1/secrets/provider_token");
537-
return Promise.resolve(
538-
new Response(stubBody, {
539-
status: 200,
540-
headers: { "X-Phantom-Rotation-Id": "1" },
541-
}),
542-
);
543-
}) as unknown as typeof fetch;
544-
545-
const path = writeYaml(
546-
"metadata-source.yaml",
547-
`
548-
name: metadata-tenant
549-
secret_source: metadata
550-
secret_source_url: http://gateway.test
551-
`,
552-
);
553-
await loadConfig(path);
554-
expect(process.env.ANTHROPIC_API_KEY).toBe(stubBody);
555-
expect(process.env.ANTHROPIC_AUTH_TOKEN).toBe(stubBody);
556-
});
557-
558-
test("secret_source: metadata resolves whole-string ${secret:NAME} references in nested config", async () => {
559-
// Use peers.test_peer.token, an existing schema field that accepts an
560-
// arbitrary string and is nested. We assert the resolved value via a
561-
// non-logging path: read it back from the returned config object.
562-
const peerSecret = "peer-secret-payload";
563-
globalThis.fetch = mock((url: string | Request) => {
564-
const u = String(url);
565-
if (u.endsWith("/v1/secrets/provider_token")) {
566-
return Promise.resolve(
567-
new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }),
568-
);
569-
}
570-
if (u.endsWith("/v1/secrets/peer_token")) {
571-
return Promise.resolve(new Response(peerSecret, { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }));
572-
}
573-
throw new Error(`unexpected URL in test: ${u}`);
574-
}) as unknown as typeof fetch;
575-
576-
const path = writeYaml(
577-
"metadata-walker.yaml",
578-
`
579-
name: walker-tenant
580-
secret_source: metadata
581-
secret_source_url: http://gateway.test
582-
peers:
583-
test_peer:
584-
url: https://peer.test
585-
token: \${secret:peer_token}
586-
`,
587-
);
588-
const config = await loadConfig(path);
589-
expect(config.peers?.test_peer?.token).toBe(peerSecret);
590-
});
591-
592-
test("secret_source: metadata does NOT resolve partial-string ${secret:NAME} (security invariant)", async () => {
593-
// Whole-string-only matching guards against secret leakage via logged
594-
// URLs or composed strings. If this test ever fails, the regex changed
595-
// in a way that allows partial interpolation, which is a security regression.
596-
const partial = "https://peer.test/?token=${secret:peer_token}";
597-
globalThis.fetch = mock((url: string | Request) => {
598-
const u = String(url);
599-
if (u.endsWith("/v1/secrets/provider_token")) {
600-
return Promise.resolve(
601-
new Response("provider-token-value", { status: 200, headers: { "X-Phantom-Rotation-Id": "1" } }),
602-
);
603-
}
604-
throw new Error(`unexpected URL in test (partial-string should NOT trigger fetch): ${u}`);
605-
}) as unknown as typeof fetch;
606-
607-
const path = writeYaml(
608-
"metadata-partial.yaml",
609-
`
610-
name: partial-tenant
611-
secret_source: metadata
612-
secret_source_url: http://gateway.test
613-
peers:
614-
test_peer:
615-
url: ${partial}
616-
token: keep-as-is
617-
`,
618-
);
619-
const config = await loadConfig(path);
620-
expect(config.peers?.test_peer?.url).toBe(partial);
621-
expect(config.peers?.test_peer?.token).toBe("keep-as-is");
622-
});
623-
624-
test("secret_source: metadata surfaces fetch failures as errors that include the secret name", async () => {
625-
globalThis.fetch = mock(() =>
626-
Promise.resolve(new Response("internal error", { status: 500, statusText: "Internal Server Error" })),
627-
) as unknown as typeof fetch;
628-
629-
const path = writeYaml(
630-
"metadata-fail.yaml",
631-
`
632-
name: fail-tenant
633-
secret_source: metadata
634-
secret_source_url: http://gateway.test
635-
`,
636-
);
637-
await expect(loadConfig(path)).rejects.toThrow(/provider_token/);
638-
await expect(loadConfig(path)).rejects.toThrow(/500/);
639-
});
640-
});

‎src/config/__tests__/metadata-fetcher.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,4 +129,25 @@ describe("MetadataSecretFetcher", () => {
129129
await fetcher.get("a_secret_with_no_special_chars");
130130
expect(recordedUrls[0]).toBe("http://gateway.test/v1/secrets/a_secret_with_no_special_chars");
131131
});
132+
133+
test("304 with no cached entry surfaces as an error containing the secret name", async () => {
134+
// 304-without-cache is a server bug. Pin the loud-error behaviour so a
135+
// future refactor that inverted the `if (!cached)` test would fail here.
136+
globalThis.fetch = mock(() => Promise.resolve(new Response(null, { status: 304 }))) as unknown as typeof fetch;
137+
138+
const fetcher = new MetadataSecretFetcher("http://gateway.test");
139+
await expect(fetcher.get("provider_token")).rejects.toThrow(/304/);
140+
await expect(fetcher.get("provider_token")).rejects.toThrow(/provider_token/);
141+
});
142+
143+
test("network error from underlying fetch is wrapped with the secret name", async () => {
144+
// Transport failures (DNS, ECONNREFUSED, TLS) must be wrapped with the
145+
// secret name so the operator can see which fetch call failed without
146+
// leaking response bodies (there is no body on a transport error).
147+
globalThis.fetch = mock(() => Promise.reject(new Error("ECONNREFUSED"))) as unknown as typeof fetch;
148+
149+
const fetcher = new MetadataSecretFetcher("http://gateway.test");
150+
await expect(fetcher.get("provider_token")).rejects.toThrow(/provider_token/);
151+
await expect(fetcher.get("provider_token")).rejects.toThrow(/ECONNREFUSED/);
152+
});
132153
});

‎src/config/loader.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@
2222
// plaintext. By requiring whole-string matches, the resolved replacement IS
2323
// the plaintext itself, never a string containing the plaintext, so the
2424
// surface for accidental disclosure is the existing `process.env` surface.
25+
//
26+
// Note on type safety: the walker treats the config as a generic
27+
// Record<string, unknown> so it can recurse over arbitrary nested objects.
28+
// The single `as unknown as Record<string, unknown>` cast at the entry point
29+
// (where loadConfig hands the typed PhantomConfig to interpolateSecretsInPlace)
30+
// is the only place we cross the typed/generic boundary; the walker itself
31+
// does not introduce further casts beyond a recursive descent step.
2532

2633
import { readFileSync } from "node:fs";
2734
import { parse } from "yaml";

‎src/config/providers.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,17 @@ export const ProviderSchema = z
2929
// plaintext is injected into process.env.ANTHROPIC_API_KEY /
3030
// ANTHROPIC_AUTH_TOKEN before buildProviderEnv runs. Default
3131
// "provider_token" so a Cloud tenant who flips secret_source to
32-
// metadata gets the right behavior with no further config.
33-
secret_name: z.string().min(1).default("provider_token"),
32+
// metadata gets the right behavior with no further config. The regex
33+
// matches the metadata fetcher's defense-in-depth check; pinning it at
34+
// the schema level surfaces invalid names at parse time rather than
35+
// crashing at boot inside the fetcher.
36+
secret_name: z
37+
.string()
38+
.regex(/^[a-z_][a-z0-9_]*$/, {
39+
message:
40+
"secret_name must match /^[a-z_][a-z0-9_]*$/ (lowercase letters, digits, underscores; cannot start with a digit)",
41+
})
42+
.default("provider_token"),
3443
model_mappings: z
3544
.object({
3645
opus: z.string().min(1).optional(),

0 commit comments

Comments
 (0)