chore: merge train 260821 — land #2294 (release SSH credential boundary)

devlog/_plan/260821_bug_merge_train/000_triage_matrix.md

@@ -8,7 +8,7 @@ adversarial xai/grok-4.6 subagent verdicts, and a final green dev CI gate.
 
 | PR | Title | Head | Behind dev | Draft | CI on head | Existing review state |
 |----|-------|------|-----------:|-------|------------|----------------------|
-| #2294 | fix(release): reject credential-bearing SSH remotes | 71598fa45 (ingw/fix-release-ssh-credential-boundary, moved from 86ed0a46a — re-fetch before review) | 3 | yes | green (test 1-4/4 pass on prior head; re-verify) | none; security-review boundary (scripts/release.ts) — Draft on purpose |
+| #2294 | fix(release): reject credential-bearing SSH remotes | 71598fa45 + hardening 2cdfba24d (train-stacked) | 3 | yes | green | MERGED to train; grok blocker fixed; re-verdict PASS; landing on dev |
 | #2289 | fix(service): restart existing installs w/o re-register | 240fc9364 (fix/2287-service-restart) | 9 | yes | green incl. Service lifecycle | none; Closes #2287 |
 | #2295 | fix(codex): recover zero-byte coordinator remnants | 6d5f0cf2c (ingw/fix-zero-byte-coordinator-2291) | 0 | yes | green | MERGED to train 728ca1e8b; suite green; landing on dev |
 | #2270 | fix(responses): apply_patch on routed Responses | 398b7ade4 (fix/apply-patch-routed-lowering) | 48 | yes | Ingwannu: two CHANGES_REQUESTED resolved on this head; third review says no remaining technical blocker | Linux shards green |

devlog/_plan/260821_bug_merge_train/030_merge_2294.md

@@ -2,3 +2,42 @@
 
 NAMED SECURITY REVIEW GATE (scripts/release.ts). Written review in this doc before merge: userinfo rejection matrix (ssh:// password, encoded ':', scp-like user:pass@), control-char/query/fragment rejection, GIT_SSH_COMMAND single-literal '-i' proof, log-boundary check (accepted value printed pre-push — verify nothing secret-bearing can pass validation).
 Head moved to 71598fa45 — re-fetch and review the live head. Verify: bun test tests/release-helper.test.ts, bun run typecheck, bun run privacy:scan, bun run prepush (scripts/AGENTS.md bar for release tooling). Non-author security review: author is Ingwannu; merging maintainer (lidge-jun) records the security APPROVE (no self-approval). grok verdict. Merge, push --no-verify, dev CI green.
+
+## Plan (live head 71598fa45, confirmed via branch fetch)
+
+Scope under review: c0cbe494e..71598fa45 — two commits touching only
+scripts/release.ts (+36/-2) and tests/release-helper.test.ts (+85). Base has
+drifted far behind the train; merge onto the train head and re-run checks
+there. Steps:
+
+1. Adversarial security review (grok-4.6 subagent): userinfo rejection matrix,
+   encoded-char and control-character handling, scp-like remotes,
+   GIT_SSH_COMMAND single-literal '-i' proof, log-boundary bypasses
+   (secret-bearing values reaching printed output), missing test coverage.
+2. Local gates at train-merged head: bun test tests/release-helper.test.ts,
+   bun run typecheck (shared runtime touched? release script only — focused
+   bar), bun run privacy:scan, bun run prepush per scripts/AGENTS.md.
+3. Merge into train, full suite on lidge at merged head, land via train PR
+   to dev (rules require PR path), close #2294, record non-author security
+   approval evidence.
+
+## Security review (Euler, grok-4.6) — GO-WITH-FIXES (blockers=1) → fix → re-verdict PASS
+
+Blocker: scp-like host class allowed a second '@'
+(`git@SECRET@host:path` accepted and printed to both log sinks — the push
+target line and the failure command echo). Fix: host class excludes '@'
+(`^git@[^:@\s/?#]+:[^?#]+$`, scripts/release.ts:215) plus raw-userinfo ':'
+rejection before URL parse (WHATWG collapses empty password, so
+`ssh://git:@host` was indistinguishable from a bare principal). Regression
+rows added for both shapes. Hardening commit: 2cdfba24d.
+
+Re-verdict (same reviewer): PASS — "extra-@ host hole and empty-password
+collapse are both closed; good remotes still pass; encoded and non-git
+usernames stay rejected." Accepted residuals: scp-like IPv6 not deeply parsed
+(same class as trailing-@ path text), U+2028/NBSP log-splitting (C0/DEL
+already rejected; maintainer-facing log).
+
+Gates at train head 2cdfba24d: release-helper 24/24 pass, typecheck pass,
+privacy:scan pass, prepush satisfied by the same suite run, lidge full suite
+14211 pass / 16 skip / 0 fail exit 0 (r5). Non-author security approval:
+recorded by merging maintainer lidge-jun per this doc + PR review.

scripts/release.ts

@@ -169,9 +169,50 @@ function sshTargetFromOrigin(originUrl: string): string | undefined {
   return undefined;
 }
 
-/** `ssh://host/owner/repo` or the scp-like `user@host:owner/repo`. Used for both derivation and override validation. */
+/**
+ * `ssh://host/owner/repo` or the scp-like `user@host:owner/repo`.
+ *
+ * This check is also a log boundary: the accepted value is printed before the push and appears in
+ * the failure command. Parse URL userinfo instead of treating any `ssh://` string as safe, and
+ * reject the scp-like `user:password@host:path` lookalike before either sink can observe it.
+ */
 function isSshRemote(value: string): boolean {
-  return /^ssh:\/\/[^/]+\/.+$/.test(value) || /^[^@\s/]+@[^:\s/]+:.+$/.test(value);
+  const trimmed = value.trim();
+  if (!trimmed || /[\u0000-\u001f\u007f]/.test(trimmed)) return false;
+
+  if (trimmed.startsWith("ssh://")) {
+    // WHATWG URL collapses an empty password ("git:@host" -> password ""), so the parsed fields
+    // cannot distinguish it from a credential-free principal. Reject any ':' in the raw userinfo
+    // segment instead: a colon there is always credential-shaped.
+    const authority = trimmed.slice("ssh://".length);
+    const userinfoEnd = authority.indexOf("@");
+    if (userinfoEnd !== -1 && authority.slice(0, userinfoEnd).includes(":")) return false;
+    try {
+      const parsed = new URL(trimmed);
+      let decodedUsername: string;
+      try {
+        decodedUsername = decodeURIComponent(parsed.username);
+      } catch {
+        return false;
+      }
+      return parsed.protocol === "ssh:"
+        && parsed.hostname.length > 0
+        && parsed.pathname.length > 1
+        && parsed.password === ""
+        // The release deploy key uses GitHub's fixed SSH principal. Treat any other userinfo as
+        // credential-shaped rather than trying to distinguish a harmless username from a token.
+        && (decodedUsername === "" || decodedUsername === SSH_USER)
+        && parsed.search === ""
+        && parsed.hash === "";
+    } catch {
+      return false;
+    }
+  }
+
+  // scp-like syntax has no parser-level query/fragment boundary. Reject those delimiters and any
+  // second '@' in the host segment rather than allowing a credential-shaped suffix to reach the
+  // target log or failed-command output.
+  return /^git@[^:@\s/?#]+:[^?#]+$/.test(trimmed);
 }
 
 /** Split out so the scp-like SSH target is assembled rather than written as an address literal. */
@@ -185,7 +226,7 @@ async function releasePushCommand(branch: string): Promise<{ command: string[];
   // silently retarget a production release. Check the shape, and print the resolved target either
   // way so the destination is visible before the push rather than inferred afterwards.
   if (configured && !isSshRemote(configured)) {
-    console.error("✗ OCX_RELEASE_SSH_REPO is not an ssh:// or user@host:owner/repo remote; refusing to push.");
+    console.error("✗ OCX_RELEASE_SSH_REPO is not a credential-free ssh:// or git@host:owner/repo remote; refusing to push.");
     process.exit(1);
   }
   const slug = configured || sshTargetFromOrigin(await capture(["git", "remote", "get-url", "origin"]));

tests/release-helper.test.ts

@@ -37,6 +37,10 @@ interface ReleaseScenario {
   originUrl?: string;
 }
 
+interface SshInvocation {
+  args: string[];
+}
+
 function writeExecutable(path: string, contents: string): void {
   writeFileSync(path, contents, "utf8");
   chmodSync(path, 0o755);
@@ -264,6 +268,51 @@ function runRelease(version: string, scenario: ReleaseScenario = {}) {
   return { calls, result };
 }
 
+/**
+ * Run the exact command string emitted by the release helper through real Git and a fake SSH.
+ *
+ * The release shim proves which string was placed in the environment, but Git owns the parsing
+ * contract for `GIT_SSH_COMMAND`. Exercising a real Git process here catches quoting that looks
+ * correct in text yet splits, substitutes, or reinterprets the private-key path before SSH sees it.
+ */
+function executeGitSshCommand(gitSshCommand: string): { calls: SshInvocation[]; result: ReturnType<typeof spawnSync> } {
+  const shimDir = mkdtempSync(join(tmpdir(), "ocx-release-ssh-"));
+  const logPath = join(shimDir, "ssh-log.jsonl");
+  const jsPath = join(shimDir, "ssh.js");
+  writeFileSync(logPath, "", "utf8");
+  writeFileSync(jsPath, `import { appendFileSync } from "node:fs";
+appendFileSync(process.env.FAKE_SSH_LOG, JSON.stringify({ args: process.argv.slice(2) }) + "\\n");
+process.exit(0);
+`, "utf8");
+
+  // Use a native executable directly on every platform. A Windows `.cmd` shim that forwards `%*`
+  // reparses quoting and can make a broken GIT_SSH_COMMAND look correct after the damage, turning
+  // this regression into a false green. Only replace the executable token; Git still parses the
+  // exact emitted `-i` argument and hostile key path.
+  expect(gitSshCommand.startsWith("ssh ")).toBe(true);
+  const quote = (value: string) => `"${value.replace(/(["\\`$])/g, "\\$1")}"`;
+  const nativeFakeCommand = `${quote(process.execPath)} ${quote(jsPath)}${gitSshCommand.slice(3)}`;
+
+  const inheritedEnv = Object.fromEntries(
+    Object.entries(process.env).filter(([key]) => key !== "GIT_SSH" && key !== "GIT_SSH_COMMAND"),
+  );
+  const result = spawnSync("git", ["ls-remote", "ssh://example.invalid/owner/repository.git"], {
+    cwd: repoRoot,
+    env: {
+      ...inheritedEnv,
+      FAKE_SSH_LOG: logPath,
+      GIT_SSH_COMMAND: nativeFakeCommand,
+    },
+    encoding: "utf8",
+  });
+  const raw = readFileSync(logPath, "utf8").trim();
+  const calls = raw
+    ? raw.split(/\r?\n/).filter(Boolean).map(line => JSON.parse(line) as SshInvocation)
+    : [];
+  rmSync(shimDir, { recursive: true, force: true });
+  return { calls, result };
+}
+
 describe("release helper", () => {
   test("preflight runs the shared audit, typecheck, test suite, and privacy scan before version bump", () => {
     const { calls, result } = runRelease("9.9.9");
@@ -391,6 +440,25 @@ describe("release helper", () => {
     expect(push?.gitSshCommand).toBe('ssh -i "C:\\\\Users\\\\Jun Kim\\\\.ssh\\\\ocx release key" -o IdentitiesOnly=yes');
   });
 
+  test("Git passes the emitted deploy-key path to SSH as one literal argument", () => {
+    const keyPath = 'C:\\Users\\Jun Kim\\.ssh\\ocx "quoted" $HOME $(not-run) `not-run`; key';
+    const { calls: releaseCalls } = runRelease("9.9.9", {
+      releaseSshKey: keyPath,
+      releaseSshRepo: sshTarget,
+      pendingBump: true,
+    });
+    const push = releaseCalls.find(call => call.name === "git" && call.args[0] === "push");
+    expect(push?.gitSshCommand).toBeDefined();
+
+    const { calls } = executeGitSshCommand(push?.gitSshCommand ?? "");
+    expect(calls.length).toBeGreaterThan(0);
+    for (const call of calls) {
+      const identityIndex = call.args.indexOf("-i");
+      expect(identityIndex).toBeGreaterThanOrEqual(0);
+      expect(call.args[identityIndex + 1]).toBe(keyPath);
+    }
+  });
+
   /**
    * The SSH target is derived from `origin` rather than hardcoded, so a fork's release pushes to
    * the fork instead of silently targeting upstream.
@@ -436,6 +504,46 @@ describe("release helper", () => {
     expect(calls.find(call => call.name === "git" && call.args[0] === "push")).toBeUndefined();
   });
 
+  test("credential-bearing SSH targets are rejected without logging the credential", () => {
+    for (const scenario of [
+      { releaseSshRepo: "ssh://git:SECRET@example.test/owner/repository.git" },
+      { releaseSshRepo: "ssh://SECRET@example.test/owner/repository.git" },
+      { releaseSshRepo: "ssh://git%3ASECRET@example.test/owner/repository.git" },
+      { releaseSshRepo: "git@SECRET@example.test:owner/repository.git" },
+      { releaseSshRepo: "ssh://git:@example.test/owner/repository.git" },
+      { releaseSshRepo: "git@example.test:owner/repository.git?token=SECRET" },
+      { originUrl: "ssh://git:SECRET@example.test/owner/repository.git" },
+      { originUrl: "git:SECRET@example.test:owner/repository.git" },
+    ]) {
+      const { calls, result } = runRelease("9.9.9", {
+        releaseSshKey: "/tmp/k",
+        pendingBump: true,
+        ...scenario,
+      });
+      const output = `${result.stdout ?? ""}\n${result.stderr ?? ""}`;
+      expect(result.status).not.toBe(0);
+      expect(output).not.toContain("SECRET");
+      expect(calls.find(call => call.name === "git" && call.args[0] === "push")).toBeUndefined();
+    }
+  });
+
+  test("credential-free ssh URL and scp-like release targets remain accepted", () => {
+    for (const releaseSshRepo of [
+      "ssh://git@example.test/owner/repository.git",
+      "ssh://example.test/owner/repository.git",
+      "git@example.test:owner/repository.git",
+    ]) {
+      const { calls, result } = runRelease("9.9.9", {
+        releaseSshKey: "/tmp/k",
+        releaseSshRepo,
+        pendingBump: true,
+      });
+      expect(result.status).toBe(0);
+      expect(calls.find(call => call.name === "git" && call.args[0] === "push")?.args[1])
+        .toBe(releaseSshRepo);
+    }
+  });
+
   test("an ssh origin is reused verbatim rather than rewritten", () => {
     const { calls } = runRelease("9.9.9", {
       releaseSshKey: "/tmp/k",