fix(cli): Windows cert check/uninstall key off the real CA identity, not a hardcoded legacy host (#7275) (#7557)

* fix(cli): Windows cert check/uninstall key off the real CA identity, not a hardcoded legacy host (#7275)

checkCertInstalledWindows()/uninstallCertWindows() queried the Windows Root
store by the literal legacy hostname daily-cloudcode-pa.googleapis.com
regardless of the certPath passed in (the check's param was even
underscore-prefixed/unused). It only worked because that hostname happens to
be the generated CA's own commonName today (generate.ts derives it from
ANTIGRAVITY_TARGET.hosts[0]) -- a coincidence, not a derivation, with no
shared symbol coupling the two. installCertWindows() was already correct.

Both functions now derive a SHA-1 thumbprint straight from the certPath file
via the new exported certutilThumbprint() helper (reusing the existing
getCertFingerprint() logic checkCertInstalledMac() already keys off), so the
Windows store lookup/delete always matches the real generated CA regardless
of any future rename/reorder of ANTIGRAVITY_TARGET.hosts in generate.ts. Same
anti-pattern class as #6338 (DNS side).

* test(cli): assert certutil argv precisely instead of scanning for the legacy host (#7275)

The two negative substring checks tripped CodeQL's
js/incomplete-url-substring-sanitization (high) by looking like URL
sanitization while actually asserting a certutil argv. Replaced with
assertions on the exact argv / extracted certId, which are strictly
stronger: pinning the value proves no other identity can be passed.
Both still fail against the unfixed install.ts (5/5 RED) and pass with
the fix (5/5 GREEN).

changelog.d/fixes/7275-windows-cert-check-uninstall-hardcoded-legacy-host.md

@@ -0,0 +1 @@
+- fix(cli): Windows MITM root-CA check/uninstall keyed off the hardcoded legacy hostname `daily-cloudcode-pa.googleapis.com` instead of the actual generated CA's identity — they now derive a SHA-1 thumbprint from the real `certPath` file (same pattern `#6338` used for the DNS side of this anti-pattern) (#7275)

src/mitm/cert/install.ts

@@ -161,9 +161,25 @@ async function checkCertInstalledLinux(certPath: string): Promise<boolean> {
   }
 }
 
-async function checkCertInstalledWindows(_certPath: string): Promise<boolean> {
+/**
+ * Windows `certutil -store <storename> <certId>` accepts a serial number, a
+ * SHA-1 thumbprint, or a substring of the subject/friendly name as `certId`.
+ * Older code passed the literal legacy hostname `daily-cloudcode-pa.googleapis.com`
+ * here — it only "worked" because that happens to be the CA's own commonName
+ * today (`generate.ts` derives it from `ANTIGRAVITY_TARGET.hosts[0]`), a
+ * coincidence with no shared symbol coupling the two (#7275). Deriving the
+ * thumbprint from the actual `certPath` file — the same identity
+ * {@link checkCertInstalledMac} already keys off via {@link getCertFingerprint}
+ * — makes the Windows store lookup match the real generated CA regardless of
+ * any future rename/reorder in `generate.ts`.
+ */
+export function certutilThumbprint(certPath: string): string {
+  return getCertFingerprint(certPath).replace(/:/g, "");
+}
+
+async function checkCertInstalledWindows(certPath: string): Promise<boolean> {
   try {
-    await execFileText("certutil", ["-store", "Root", "daily-cloudcode-pa.googleapis.com"]);
+    await execFileText("certutil", ["-store", "Root", certutilThumbprint(certPath)]);
     return true;
   } catch {
     return false;
@@ -381,7 +397,7 @@ export async function uninstallCert(sudoPassword: string, certPath: string): Pro
   }
 
   if (IS_WIN) {
-    await uninstallCertWindows();
+    await uninstallCertWindows(certPath);
   } else if (IS_MAC) {
     await uninstallCertMac(sudoPassword, certPath);
   } else {
@@ -431,10 +447,20 @@ async function uninstallCertLinux(sudoPassword: string, certPath: string): Promi
   }
 }
 
-async function uninstallCertWindows(): Promise<void> {
-  await runElevatedPowerShell(`
-    $proc = Start-Process certutil -ArgumentList @('-delstore','Root','daily-cloudcode-pa.googleapis.com') -Verb RunAs -Wait -PassThru;
+/**
+ * Pure builder for the elevated `certutil -delstore` script, extracted so the
+ * regression test can assert the argv it embeds without spawning a real
+ * `powershell`/UAC prompt (mirrors {@link buildCertManualGuide} /
+ * {@link buildElevatedScriptWrapper}, already tested the same way).
+ */
+export function buildWindowsDelstoreScript(thumbprint: string): string {
+  return `
+    $proc = Start-Process certutil -ArgumentList @('-delstore','Root',${quotePowerShell(thumbprint)}) -Verb RunAs -Wait -PassThru;
     if ($proc.ExitCode -ne 0) { throw "certutil exited with code $($proc.ExitCode)" }
-  `);
+  `;
+}
+
+async function uninstallCertWindows(certPath: string): Promise<void> {
+  await runElevatedPowerShell(buildWindowsDelstoreScript(certutilThumbprint(certPath)));
   console.log("✅ Uninstalled certificate from Windows Root store");
 }

tests/unit/windows-cert-identity-7275.test.ts

@@ -0,0 +1,154 @@
+/**
+ * Regression test for #7275 — Windows cert check/uninstall used a hardcoded
+ * legacy hostname (`daily-cloudcode-pa.googleapis.com`) instead of the real
+ * generated CA's identity.
+ *
+ * Root cause: `checkCertInstalledWindows()`/`uninstallCertWindows()` queried
+ * the Windows Root store by the literal legacy hostname regardless of the
+ * `certPath` passed in — it only "worked" because that hostname happens to be
+ * the CA's own `commonName` today (`generate.ts` derives it from
+ * `ANTIGRAVITY_TARGET.hosts[0]`), a coincidence with no shared symbol coupling
+ * the two. `installCertWindows()` was already correct (keys off `certPath`).
+ *
+ * Fix: both functions now derive a SHA-1 thumbprint straight from the
+ * `certPath` file (via the exported `certutilThumbprint()`, reusing the same
+ * `getCertFingerprint()` logic {@link checkCertInstalledMac} already uses) —
+ * the same identity `installCertWindows()` installs, independent of
+ * `ANTIGRAVITY_TARGET.hosts` ordering/content.
+ *
+ * Methodology: a real `certutil` stub on PATH captures argv (no
+ * `child_process` mocking), with `process.platform` forced to `win32` before
+ * the module is imported (`IS_WIN` is a load-time const) and a fake — but
+ * real-file — cert so `getCertFingerprint()` runs unmodified.
+ */
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import crypto from "node:crypto";
+
+const LEGACY_HARDCODED_HOST = "daily-cloudcode-pa.googleapis.com";
+
+const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, "platform")!;
+const originalPath = process.env.PATH;
+
+const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-7275-"));
+const binDir = path.join(tmpRoot, "bin");
+fs.mkdirSync(binDir, { recursive: true });
+const captureFile = path.join(tmpRoot, "certutil-argv.log");
+fs.writeFileSync(captureFile, "");
+
+// A real executable on PATH — not a child_process mock — so
+// checkCertInstalledWindows() exercises the genuine execFile() code path.
+const certutilStubPath = path.join(binDir, "certutil");
+fs.writeFileSync(
+  certutilStubPath,
+  `#!/usr/bin/env node
+const fs = require("fs");
+fs.appendFileSync(${JSON.stringify(captureFile)}, process.argv.slice(2).join(" ") + "\\n");
+process.exit(0);
+`,
+  { mode: 0o755 }
+);
+
+Object.defineProperty(process, "platform", { value: "win32", configurable: true });
+process.env.PATH = `${binDir}${path.delimiter}${originalPath}`;
+
+// Imported AFTER forcing win32: IS_WIN inside install.ts is a load-time const.
+const { checkCertInstalled, certutilThumbprint, buildWindowsDelstoreScript } = await import(
+  "../../src/mitm/cert/install.ts"
+);
+
+test.after(() => {
+  Object.defineProperty(process, "platform", originalPlatformDescriptor);
+  process.env.PATH = originalPath;
+  fs.rmSync(tmpRoot, { recursive: true, force: true });
+});
+
+function fakeCertFile(seed: string): string {
+  const der = crypto.createHash("sha256").update(seed).digest();
+  const pem =
+    "-----BEGIN CERTIFICATE-----\n" +
+    der.toString("base64").match(/.{1,64}/g)!.join("\n") +
+    "\n-----END CERTIFICATE-----\n";
+  const certPath = path.join(tmpRoot, `${seed}.crt`);
+  fs.writeFileSync(certPath, pem);
+  return certPath;
+}
+
+test("checkCertInstalledWindows() keys off the real cert's thumbprint, not the legacy hardcoded host", async () => {
+  const certPath = fakeCertFile("probe-a");
+  const expectedThumbprint = certutilThumbprint(certPath);
+
+  const isInstalled = await checkCertInstalled(certPath);
+  assert.equal(isInstalled, true, "the certutil stub always exits 0 → should report installed");
+
+  const capturedArgv = fs.readFileSync(captureFile, "utf8").trim().split("\n").at(-1)!;
+  // Exact-equality is strictly stronger than a negative substring check: an argv
+  // that IS `-store Root <thumbprint>` cannot also carry the legacy hostname.
+  assert.equal(
+    capturedArgv,
+    `-store Root ${expectedThumbprint}`,
+    "certutil must be queried with the real cert's thumbprint, not the legacy hostname"
+  );
+});
+
+test("checkCertInstalledWindows() tracks certPath — a different cert yields a different query", async () => {
+  fs.writeFileSync(captureFile, "");
+  const certPathB = fakeCertFile("probe-b");
+  const thumbprintA = certutilThumbprint(fakeCertFile("probe-a"));
+  const thumbprintB = certutilThumbprint(certPathB);
+  assert.notEqual(thumbprintA, thumbprintB, "sanity: distinct cert content → distinct thumbprint");
+
+  await checkCertInstalled(certPathB);
+  const capturedArgv = fs.readFileSync(captureFile, "utf8").trim().split("\n").at(-1)!;
+  assert.equal(capturedArgv, `-store Root ${thumbprintB}`);
+});
+
+test("buildWindowsDelstoreScript() embeds the cert's own thumbprint, not the legacy hardcoded host", () => {
+  const certPath = fakeCertFile("probe-c");
+  const thumbprint = certutilThumbprint(certPath);
+  const script = buildWindowsDelstoreScript(thumbprint);
+
+  // Assert on the certId argument certutil actually receives, rather than on a
+  // negative substring scan of the whole script: pinning the extracted value to
+  // the real thumbprint proves no other identity (legacy hostname included) can
+  // be the one passed to -delstore.
+  const certIdArg = script.match(/'-delstore'\s*,\s*'Root'\s*,\s*'([^']+)'/)?.[1];
+  assert.equal(
+    certIdArg,
+    thumbprint,
+    "delstore must target the cert's own thumbprint as its certId argument"
+  );
+});
+
+test("check and uninstall derive identity from the SAME source (certutilThumbprint(certPath)) — immune to ANTIGRAVITY_TARGET.hosts reordering", async () => {
+  fs.writeFileSync(captureFile, "");
+  const certPath = fakeCertFile("probe-d");
+
+  await checkCertInstalled(certPath);
+  const checkArgv = fs.readFileSync(captureFile, "utf8").trim().split("\n").at(-1)!;
+  const checkThumbprint = checkArgv.replace("-store Root ", "");
+
+  const delstoreScript = buildWindowsDelstoreScript(certutilThumbprint(certPath));
+
+  assert.ok(
+    delstoreScript.includes(checkThumbprint),
+    "uninstall must target the exact same identity the check used — " +
+      "no coincidence-based coupling through a hardcoded hostname"
+  );
+});
+
+test("source no longer hardcodes the legacy host for the Windows check/uninstall paths", () => {
+  const source = fs.readFileSync(
+    new URL("../../src/mitm/cert/install.ts", import.meta.url),
+    "utf8"
+  );
+  // Only allowed inside the doc-comment explaining the historical bug — never
+  // as a live argv literal passed to certutil.
+  assert.ok(
+    !source.includes(`"${LEGACY_HARDCODED_HOST}"`),
+    "the legacy hostname must not appear as a string literal anywhere in install.ts"
+  );
+});