From 8b26ec7803fcd0856eea39b630d6ba0c696a8ecc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 30 Mar 2026 16:32:40 -0400 Subject: [PATCH] fix: use loose equality for null checks in secure_env_collect (#2997) (#3231) When ctx.ui.custom() returns undefined instead of null, strict equality checks (=== null / !== null) let the undefined value pass through to writeEnvKey, which crashes on .replace(). Switch to loose equality (== null / != null) in both the manifest orchestrator and extension execute paths so both null and undefined are treated as "skipped". Also add a type guard in writeEnvKey for defense in depth. Co-authored-by: Claude Opus 4.6 --- .../extensions/get-secrets-from-user.ts | 13 +- .../gsd/tests/secure-env-collect.test.ts | 134 ++++++++++++++++++ 2 files changed, 142 insertions(+), 5 deletions(-) diff --git a/src/resources/extensions/get-secrets-from-user.ts b/src/resources/extensions/get-secrets-from-user.ts index 300852305..7fe418f59 100644 --- a/src/resources/extensions/get-secrets-from-user.ts +++ b/src/resources/extensions/get-secrets-from-user.ts @@ -54,6 +54,9 @@ function hydrateProcessEnv(key: string, value: string): void { } async function writeEnvKey(filePath: string, key: string, value: string): Promise { + if (typeof value !== "string") { + throw new TypeError(`writeEnvKey expects a string value for key "${key}", got ${typeof value}`); + } let content = ""; try { content = await readFile(filePath, "utf8"); @@ -419,7 +422,7 @@ export async function collectSecretsFromManifest( for (const { key, value } of collected) { const entry = manifest.entries.find((e) => e.key === key); if (entry) { - entry.status = value !== null ? "collected" : "skipped"; + entry.status = value != null ? "collected" : "skipped"; } } @@ -427,14 +430,14 @@ export async function collectSecretsFromManifest( await writeFile(manifestPath, formatSecretsManifest(manifest), "utf8"); // (j) Apply collected values to destination - const provided = collected.filter((c) => c.value !== null) as Array<{ key: string; value: string }>; + const provided = collected.filter((c) => c.value != null) as Array<{ key: string; value: string }>; const { applied } = await applySecrets(provided, destination, { envFilePath: resolve(ctx.cwd, ".env"), }); const skipped = [ ...alreadySkipped, - ...collected.filter((c) => c.value === null).map((c) => c.key), + ...collected.filter((c) => c.value == null).map((c) => c.key), ]; return { applied, skipped, existingSkipped }; @@ -505,8 +508,8 @@ export default function secureEnv(pi: ExtensionAPI) { collected.push({ key: item.key, value }); } - const provided = collected.filter((c) => c.value !== null) as Array<{ key: string; value: string }>; - const skipped = collected.filter((c) => c.value === null).map((c) => c.key); + const provided = collected.filter((c) => c.value != null) as Array<{ key: string; value: string }>; + const skipped = collected.filter((c) => c.value == null).map((c) => c.key); // Apply to destination via shared helper const { applied, errors } = await applySecrets(provided, destination, { diff --git a/src/resources/extensions/gsd/tests/secure-env-collect.test.ts b/src/resources/extensions/gsd/tests/secure-env-collect.test.ts index bd6096674..18acf7dd4 100644 --- a/src/resources/extensions/gsd/tests/secure-env-collect.test.ts +++ b/src/resources/extensions/gsd/tests/secure-env-collect.test.ts @@ -183,3 +183,137 @@ test("secure_env_collect: detectDestination — convex file (not dir) does not t rmSync(tmp, { recursive: true, force: true }); } }); + +// ─── Bug #2997: undefined vs null handling ────────────────────────────────── + +/** + * When ctx.ui.custom() returns undefined (e.g. noOpUIContext, component + * disposal, abort), the strict null checks (=== null / !== null) let + * undefined slip through as a "provided" value, crashing writeEnvKey + * which calls .replace() on it. + * + * These tests verify the fix: loose equality (== null / != null) so that + * both null AND undefined are treated as "skipped". + */ + +// Helper to dynamically load the orchestrator +async function loadOrchestrator(): Promise<{ + collectSecretsFromManifest: Function; +}> { + const mod = await import("../../get-secrets-from-user.ts"); + return { collectSecretsFromManifest: mod.collectSecretsFromManifest }; +} + +// Helper to dynamically load files.ts functions +async function loadFilesExports(): Promise<{ + formatSecretsManifest: (m: any) => string; +}> { + const mod = await import("../files.ts"); + return { formatSecretsManifest: mod.formatSecretsManifest }; +} + +function makeManifest(entries: Array<{ key: string; status?: string; formatHint?: string; guidance?: string[] }>): any { + return { + milestone: "M001", + generatedAt: "2026-03-12T00:00:00Z", + entries: entries.map((e) => ({ + key: e.key, + service: "TestService", + dashboardUrl: "", + guidance: e.guidance ?? [], + formatHint: e.formatHint ?? "", + status: e.status ?? "pending", + destination: "dotenv", + })), + }; +} + +async function writeManifestFile(dir: string, manifest: any): Promise { + const { formatSecretsManifest } = await loadFilesExports(); + const milestoneDir = join(dir, ".gsd", "milestones", "M001"); + mkdirSync(milestoneDir, { recursive: true }); + const filePath = join(milestoneDir, "M001-SECRETS.md"); + writeFileSync(filePath, formatSecretsManifest(manifest)); + return filePath; +} + +test("secure_env_collect #2997: undefined from ctx.ui.custom() is treated as skipped, not provided", async (t) => { + const { collectSecretsFromManifest } = await loadOrchestrator(); + + const tmp = makeTempDir("sec-undefined-test"); + t.after(() => { + rmSync(tmp, { recursive: true, force: true }); + }); + + const manifest = makeManifest([ + { key: "SECRET_THAT_RETURNS_UNDEFINED", status: "pending" }, + ]); + await writeManifestFile(tmp, manifest); + + let callIndex = 0; + const mockCtx = { + cwd: tmp, + hasUI: true, + ui: { + // First call is summary screen, second is collect — return undefined + // to simulate noOpUIContext or component disposal + custom: async (_factory: any) => { + callIndex++; + if (callIndex <= 1) return null; // summary screen dismiss + return undefined; // BUG TRIGGER: should be treated as skipped + }, + }, + }; + + // Before the fix, this crashes with: + // "Cannot read properties of undefined (reading 'replace')" + const result = await collectSecretsFromManifest(tmp, "M001", mockCtx as any); + + // The undefined-returning key must appear in skipped, not in applied + assert.ok( + result.skipped.includes("SECRET_THAT_RETURNS_UNDEFINED"), + "Key returning undefined should be in skipped list", + ); + assert.ok( + !result.applied.includes("SECRET_THAT_RETURNS_UNDEFINED"), + "Key returning undefined must NOT be in applied list", + ); +}); + +test("secure_env_collect #2997: null from ctx.ui.custom() is still treated as skipped (regression guard)", async (t) => { + const { collectSecretsFromManifest } = await loadOrchestrator(); + + const tmp = makeTempDir("sec-null-test"); + t.after(() => { + rmSync(tmp, { recursive: true, force: true }); + }); + + const manifest = makeManifest([ + { key: "SECRET_THAT_RETURNS_NULL", status: "pending" }, + ]); + await writeManifestFile(tmp, manifest); + + let callIndex = 0; + const mockCtx = { + cwd: tmp, + hasUI: true, + ui: { + custom: async (_factory: any) => { + callIndex++; + if (callIndex <= 1) return null; // summary screen dismiss + return null; // explicit null skip + }, + }, + }; + + const result = await collectSecretsFromManifest(tmp, "M001", mockCtx as any); + + assert.ok( + result.skipped.includes("SECRET_THAT_RETURNS_NULL"), + "Key returning null should be in skipped list", + ); + assert.ok( + !result.applied.includes("SECRET_THAT_RETURNS_NULL"), + "Key returning null must NOT be in applied list", + ); +});