fix: seal P1 acceptance startup and raw Git state

2026-08-10 00:02:29 +02:00
parent 87ee55a35f
commit d14a99abe7
3 changed files with 132 additions and 22 deletions
@@ -400,7 +400,7 @@ export async function resolveProductionExecutables({ repositoryRoot } = {}) {

 let fallbackGitHooksPath;
 function ownedFallbackGitHooksPath() {
-  if (!fallbackGitHooksPath) fallbackGitHooksPath = mkdtempSync(join(tmpdir(), `p1-git-hooks-${process.pid}-`));
+  if (!fallbackGitHooksPath) fallbackGitHooksPath = realpathSync(mkdtempSync(join(tmpdir(), `p1-git-hooks-${process.pid}-`)));
  return fallbackGitHooksPath;
 }
 function gitSafeConfig(hooksPath) {
@@ -533,6 +533,80 @@ function assertSafeGitRepositoryState(argv, cwd, runRoot, fixedHooksPath) {
    validateGitDirectoryState(realpathSync(target), runRoot);
  }
 }
+function rawGitCommonDirectory(gitDirectory) {
+  const path = join(gitDirectory, "commondir");
+  if (!existsSync(path)) return gitDirectory;
+  const entry = lstatSync(path);
+  const value = entry.isFile() && !entry.isSymbolicLink() ? readFileSync(path, "utf8") : "";
+  if (!/^[^\0\n\r]+\n?$/.test(value)) throw new Error("unsafe Git repository state: common directory is unsafe");
+  const common = resolve(gitDirectory, value.trimEnd());
+  const commonEntry = lstatSync(common);
+  if (!commonEntry.isDirectory() || commonEntry.isSymbolicLink() || realpathSync(common) !== common) {
+    throw new Error("unsafe Git repository state: common directory is unsafe");
+  }
+  return common;
+}
+function rawGitConfigEntries(path, required = false) {
+  if (!existsSync(path)) {
+    if (required) throw new Error("unsafe Git repository state: local config is unavailable");
+    return [];
+  }
+  const entry = lstatSync(path);
+  if (!entry.isFile() || entry.isSymbolicLink()) throw new Error("unsafe Git repository state: local config is unsafe");
+  return parseLocalGitConfig(readFileSync(path, "utf8"));
+}
+function unsafeRawGitConfigKey(key) {
+  const normalized = key.toLowerCase();
+  return /^filter\..+\.(?:clean|smudge|process|required)$/.test(normalized)
+    || /^remote\..+\.(?:uploadpack|receivepack)$/.test(normalized)
+    || /^(?:core\.(?:hookspath|attributesfile)|diff\.external|interactive\.difffilter)$/.test(normalized)
+    || /^include(?:if\..+)?\.path$/.test(normalized);
+}
+function validateRawGitDirectoryState(gitDirectory) {
+  const common = rawGitCommonDirectory(gitDirectory);
+  const entries = [
+    ...rawGitConfigEntries(join(common, "config"), true),
+    ...rawGitConfigEntries(join(gitDirectory, "config.worktree")),
+  ];
+  if (entries.some(([key]) => unsafeRawGitConfigKey(key))) {
+    throw new Error("unsafe Git repository state: executable local config is present");
+  }
+  for (const hooks of new Set([join(common, "hooks"), join(gitDirectory, "hooks")])) {
+    if (!existsSync(hooks)) continue;
+    const hooksEntry = lstatSync(hooks);
+    if (!hooksEntry.isDirectory() || hooksEntry.isSymbolicLink()) throw new Error("unsafe Git repository state: repository hooks are unsafe");
+    for (const entry of readdirSync(hooks, { withFileTypes: true })) {
+      if (entry.isSymbolicLink() || !entry.isFile() || !entry.name.endsWith(".sample")) {
+        throw new Error("unsafe Git repository state: repository hook is present");
+      }
+    }
+  }
+  return entries;
+}
+function rawRepositoryGitDirectory(argv, cwd) {
+  if (argv[0] === "--git-dir") return repositoryGitDirectory(argv, cwd);
+  let current = argv[0] === "-C" ? argv[1] : cwd;
+  if (!current) return undefined;
+  current = realpathSync(current);
+  while (true) {
+    if (existsSync(join(current, ".git"))) return repositoryGitDirectory(["-C", current]);
+    const parent = dirname(current);
+    if (parent === current) return undefined;
+    current = parent;
+  }
+}
+function assertSafeRawGitRepositoryState(argv, cwd, fixedHooksPath) {
+  assertEmptyHooksDirectory(fixedHooksPath);
+  const gitDirectory = rawRepositoryGitDirectory(argv, cwd);
+  const entries = gitDirectory ? validateRawGitDirectoryState(gitDirectory) : [];
+  const target = exactRemoteTarget(argv, entries);
+  if (target !== undefined) {
+    if (!ownedGitPath(target) || !existsSync(join(target, "config"))) {
+      throw new Error("unsafe Git repository state: remote target is not exact and owned");
+    }
+    validateRawGitDirectoryState(realpathSync(target));
+  }
+}

 const FIXTURE_GIT_CONFIG = new Map([
  ["user.name", new Set(["P1 Fixture Curator", "P1 Context Curator"])],
@@ -847,16 +921,24 @@ export async function runCommand(options) {
  const allowedTht = activeExecutablePolicy?.thtPath;
  if (canonical !== allowedGit && canonical !== allowedTht) policyError("command executable is not allowlisted", details());
  if (!Array.isArray(argv) || argv.some((value) => typeof value !== "string")) policyError("command argv must be a string array", details());
+  const guardedByProduction = productionSurfaceOwner !== undefined;
+  let rawGitHooksPath; let rawGitArgv;
  try {
-    if (canonical === allowedGit) validateGitInvocation(argv, { runRoot: activeExecutablePolicy?.runRoot });
+    if (canonical === allowedGit) {
+      validateGitInvocation(argv, { runRoot: activeExecutablePolicy?.runRoot });
+      if (!guardedByProduction) {
+        rawGitHooksPath = ownedFallbackGitHooksPath();
+        rawGitArgv = argv[0] === "-c" ? argv.slice(2) : argv;
+        assertSafeRawGitRepositoryState(rawGitArgv, cwd, rawGitHooksPath);
+      }
+    }
    if (canonical === allowedTht) validateThtInvocation(argv, { thtPath: allowedTht, runRoot: activeExecutablePolicy.runRoot, cwd });
  } catch (error) { policyError(error.message, details()); }
  if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1 || timeoutMs > 300_000 || !Number.isSafeInteger(maxOutputBytes) || maxOutputBytes < 1 || maxOutputBytes > MAX_OUTPUT) {
    policyError("command bounds are invalid", details());
  }
  return await new Promise((resolvePromise, reject) => {
-    const guardedByProduction = productionSurfaceOwner !== undefined;
-    const childArgv = canonical === allowedGit && !guardedByProduction ? hardenedGitArgv(argv, ownedFallbackGitHooksPath()) : argv;
+    const childArgv = canonical === allowedGit && !guardedByProduction ? hardenedGitArgv(rawGitArgv, rawGitHooksPath) : argv;
    const childEnv = canonical === allowedGit && !guardedByProduction ? { ...(env ?? {}), ...baseSafeGitEnvironment(canonical) } : env;
    const child = mutableChildProcess.execFile(canonical, childArgv, { cwd, env: childEnv, timeout: timeoutMs, maxBuffer: maxOutputBytes, encoding: "utf8", shell: false }, (error, stdout, stderr) => {
      const code = error && typeof error.code === "number" ? error.code : error ? 1 : 0;
@@ -293,6 +293,30 @@ test("command helper accepts only executable plus separate argv", async () => {
  assert.equal(result.code, 0);
 });

+test("raw runCommand rejects a configured clean filter before exact Git add", async () => {
+  const repositoryRoot = await fakeRepository();
+  const content = join(repositoryRoot, "workspace-content");
+  const helper = join(repositoryRoot, "clean-helper");
+  const marker = join(repositoryRoot, "clean-helper-ran");
+  await execFileAsync("/usr/bin/git", ["init", "--initial-branch=main"], { cwd: repositoryRoot });
+  await execFileAsync("/usr/bin/git", ["config", "user.name", "P1 Fixture Curator"], { cwd: repositoryRoot });
+  await execFileAsync("/usr/bin/git", ["config", "user.email", "p1-curator@example.invalid"], { cwd: repositoryRoot });
+  await mkdir(content);
+  await writeFile(join(content, "guide.md"), "content\n");
+  await writeFile(join(repositoryRoot, ".gitattributes"), "workspace-content/** filter=bad\n");
+  await writeFile(helper, `#!/bin/sh\nprintf ran > '${marker}'\ncat\n`, { mode: 0o700 });
+  await execFileAsync("/usr/bin/git", ["config", "filter.bad.clean", `'${helper}'`], { cwd: repositoryRoot });
+
+  const { gitPath } = await resolveProductionExecutables({
+    repositoryRoot: await realpath(join(dirname(fileURLToPath(import.meta.url)), "..", "..")),
+  });
+  await assert.rejects(
+    runCommand({ executable: gitPath, argv: ["add", "workspace-content"], cwd: repositoryRoot, env: process.env }),
+    /unsafe Git repository state/,
+  );
+  await assert.rejects(lstat(marker));
+});
+

 test("safe environment rejects ambient THT and keeps only strict process allowlist plus fixture values", () => {
  const safe = buildSafeEnvironment({
@@ -350,7 +374,7 @@ test("announce callback observes PASS and manual pending before non-keep cleanup
 test("public wrapper replaces ambient environment before invoking the runner", async () => {
  const wrapper = await readFile(join(dirname(fileURLToPath(import.meta.url)), "..", "..", "scripts", "p1-acceptance.sh"), "utf8");
  assert.match(wrapper, /safe_env=\(\/usr\/bin\/env -i/);
-  assert.match(wrapper, /P1_ACCEPTANCE_FAIL_AT/);
+  assert.doesNotMatch(wrapper, /P1_ACCEPTANCE_FAIL_AT|LANG|LC_ALL|TZ/);
  assert.doesNotMatch(wrapper, /export THT_BIN/);
 });

@@ -842,15 +866,26 @@ test("secret scan fails closed on a recoverable symlink outside fixture-secrets"
  await assert.rejects(scanSecrets({ runRoot: run.root, forbiddenValues: [canary], expectedGitRepositories: [] }), /symlink outside fixture-secrets/);
 });

-test("direct public wrapper execution cannot source ambient BASH_ENV or ENV", async () => {
+test("direct public wrapper clears startup files and exported functions before Bash starts", async () => {
  const root = await fakeRepository();
-  const startup = join(root, "startup");
-  const marker = join(root, "ambient-shell-ran");
-  await writeFile(startup, `printf sourced > '${marker}'\n`);
+  const bashStartup = join(root, "bash-startup");
+  const envStartup = join(root, "env-startup");
+  const bashMarker = join(root, "bash-env-ran");
+  const envMarker = join(root, "env-ran");
+  const functionMarker = join(root, "exported-function-ran");
+  await writeFile(bashStartup, `printf sourced > '${bashMarker}'\n`);
+  await writeFile(envStartup, `printf sourced > '${envMarker}'\n`);
  const wrapper = join(dirname(fileURLToPath(import.meta.url)), "..", "..", "scripts", "p1-acceptance.sh");
-  await assert.rejects(execFileAsync(wrapper, ["invalid"], { env: { ...process.env, BASH_ENV: startup, ENV: startup } }));
-  await assert.rejects(lstat(marker));
-  assert.match(await readFile(wrapper, "utf8"), /^#!\/usr\/bin\/env -S -u BASH_ENV -u ENV \/bin\/bash\n/);
+  await assert.rejects(execFileAsync(wrapper, ["invalid"], {
+    env: {
+      ...process.env,
+      BASH_ENV: bashStartup,
+      ENV: envStartup,
+      "BASH_FUNC_cd%%": `() { printf function > '${functionMarker}'; builtin cd "$@"; }`,
+    },
+  }));
+  for (const marker of [bashMarker, envMarker, functionMarker]) await assert.rejects(lstat(marker));
+  assert.match(await readFile(wrapper, "utf8"), /^#!\/usr\/bin\/env -S -i PATH=\/usr\/bin:\/bin \/bin\/bash\n/);
 });

 test("final listener ownership state is a declared hash-bound report artifact", async () => {
@@ -1,4 +1,4 @@
-#!/usr/bin/env -S -u BASH_ENV -u ENV /bin/bash
+#!/usr/bin/env -S -i PATH=/usr/bin:/bin /bin/bash
 set -euo pipefail
 script_path=${BASH_SOURCE[0]}
 script_dir=${script_path%/*}
@@ -51,19 +51,12 @@ wrapper_root=$(/usr/bin/mktemp -d /tmp/thoth-p1-wrapper.XXXXXXXX)
 trap '/bin/rm -rf -- "$wrapper_root"' EXIT HUP INT TERM
 /bin/mkdir -m 700 "$wrapper_root/home" "$wrapper_root/tmp"
 owned_path="${node_path%/*}:/usr/bin:/bin"
-build_env=(/usr/bin/env -i "PATH=$owned_path" "HOME=$wrapper_root/home" "TMPDIR=$wrapper_root/tmp" "LANG=${LANG:-C}")
-for name in LC_ALL TZ; do
-  [[ -n "${!name:-}" ]] && build_env+=("$name=${!name}")
-done
+build_env=(/usr/bin/env -i "PATH=$owned_path" "HOME=$wrapper_root/home" "TMPDIR=$wrapper_root/tmp")
 /bin/rm -rf -- "$repo_root/backend/dist"
 "${build_env[@]}" "$node_path" "$npm_path" --prefix "$repo_root/backend" run build

-safe_env=(/usr/bin/env -i "PATH=$owned_path" "HOME=$wrapper_root/home" "TMPDIR=$wrapper_root/tmp" "LANG=${LANG:-C}"
+safe_env=(/usr/bin/env -i "PATH=$owned_path" "HOME=$wrapper_root/home" "TMPDIR=$wrapper_root/tmp"
  "THT_BIN=$repo_root/harness/.venv/bin/tht" "P1_ACCEPTANCE_NODE_PATH=$node_path" "P1_ACCEPTANCE_NPM_PATH=$npm_path")
-for name in LC_ALL TZ; do
-  [[ -n "${!name:-}" ]] && safe_env+=("$name=${!name}")
-done
-[[ -n "${P1_ACCEPTANCE_FAIL_AT:-}" ]] && safe_env+=("P1_ACCEPTANCE_FAIL_AT=$P1_ACCEPTANCE_FAIL_AT")
 set +e
 "${safe_env[@]}" "$node_path" "$repo_root/backend/scripts/p1-acceptance.mjs" "$@"
 status=$?