From bfb338557d67c5ac2f2f41dc3f77bddcb232ad74 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Thu, 18 Jun 2026 15:22:31 +0100 Subject: [PATCH 1/5] fix(safe-outputs): trust cross-repo checkout dirs from manifest configure_git_credentials.sh now reads the checkout manifest and adds each cross-repository checkout subdirectory as a git safe.directory. Without this, safe-outputs handlers that run git inside those subdirectories (resolved via repoCwd) fail with "dubious ownership", surfacing as "Failed to pin branch". Fixes #40079 --- .../js/configure_git_credentials.test.cjs | 142 ++++++++++++++++++ actions/setup/sh/configure_git_credentials.sh | 49 ++++++ 2 files changed, 191 insertions(+) create mode 100644 actions/setup/js/configure_git_credentials.test.cjs diff --git a/actions/setup/js/configure_git_credentials.test.cjs b/actions/setup/js/configure_git_credentials.test.cjs new file mode 100644 index 00000000000..7d5b8c1d86a --- /dev/null +++ b/actions/setup/js/configure_git_credentials.test.cjs @@ -0,0 +1,142 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { spawnSync } from "child_process"; + +// Path to the shell script under test. +const SCRIPT_PATH = path.join(__dirname, "..", "sh", "configure_git_credentials.sh"); + +function createTempDir(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function removeDir(dir) { + if (dir && fs.existsSync(dir)) { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +/** + * Run configure_git_credentials.sh in an isolated HOME so that the global git + * config it writes does not affect the developer's real ~/.gitconfig. + */ +function runScript(env) { + return spawnSync("sh", [SCRIPT_PATH], { + encoding: "utf8", + env: { ...env }, + }); +} + +function readSafeDirectories(home) { + const result = spawnSync("git", ["config", "--global", "--get-all", "safe.directory"], { + encoding: "utf8", + env: { ...process.env, HOME: home }, + }); + if (result.status !== 0) { + return []; + } + return result.stdout.split("\n").filter(Boolean); +} + +describe("configure_git_credentials.sh checkout manifest trust", () => { + const tempDirs = []; + + function tempDir(prefix) { + const dir = createTempDir(prefix); + tempDirs.push(dir); + return dir; + } + + afterEach(() => { + while (tempDirs.length > 0) { + removeDir(tempDirs.pop()); + } + }); + + function setup(manifest) { + const root = tempDir("cfg-git-creds-"); + const home = path.join(root, "home"); + const workspace = path.join(root, "ws"); + const runnerTemp = path.join(root, "runner"); + const safeOutputs = path.join(runnerTemp, "gh-aw", "safeoutputs"); + fs.mkdirSync(home, { recursive: true }); + fs.mkdirSync(workspace, { recursive: true }); + fs.mkdirSync(safeOutputs, { recursive: true }); + if (manifest !== undefined) { + fs.writeFileSync(path.join(safeOutputs, "checkout-manifest.json"), JSON.stringify(manifest), "utf8"); + } + return { home, workspace, runnerTemp }; + } + + it("trusts cross-repo checkout subdirectories listed in the manifest", () => { + const { home, workspace, runnerTemp } = setup({ + "owner/repo": { repository: "owner/repo", path: "github", default_branch: "main" }, + "owner/tools": { repository: "owner/tools", path: "vendor/tools", default_branch: "main" }, + }); + + const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, RUNNER_TEMP: runnerTemp }); + expect(result.status).toBe(0); + + const entries = readSafeDirectories(home); + expect(entries).toContain(workspace); + expect(entries).toContain(path.join(workspace, "github")); + expect(entries).toContain(path.join(workspace, "vendor", "tools")); + }); + + it("skips manifest entries with an empty path", () => { + const { home, workspace, runnerTemp } = setup({ + "owner/repo": { repository: "owner/repo", path: "", default_branch: "main" }, + }); + + const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, RUNNER_TEMP: runnerTemp }); + expect(result.status).toBe(0); + + const entries = readSafeDirectories(home); + // Only the workspace itself is trusted; the empty-path entry adds nothing extra. + expect(entries).toEqual([workspace]); + }); + + it("rejects manifest paths that escape the workspace (path traversal)", () => { + const { home, workspace, runnerTemp } = setup({ + "evil/repo": { repository: "evil/repo", path: "../../escape", default_branch: "main" }, + }); + + const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, RUNNER_TEMP: runnerTemp }); + expect(result.status).toBe(0); + + const entries = readSafeDirectories(home); + expect(entries).toEqual([workspace]); + expect(entries.some(e => e.includes("escape"))).toBe(false); + }); + + it("honors GH_AW_CHECKOUT_MANIFEST override", () => { + const root = tempDir("cfg-git-creds-override-"); + const home = path.join(root, "home"); + const workspace = path.join(root, "ws"); + fs.mkdirSync(home, { recursive: true }); + fs.mkdirSync(workspace, { recursive: true }); + const manifestPath = path.join(root, "custom-manifest.json"); + fs.writeFileSync( + manifestPath, + JSON.stringify({ "owner/repo": { repository: "owner/repo", path: "sub", default_branch: "main" } }), + "utf8" + ); + + const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, GH_AW_CHECKOUT_MANIFEST: manifestPath }); + expect(result.status).toBe(0); + + const entries = readSafeDirectories(home); + expect(entries).toContain(path.join(workspace, "sub")); + }); + + it("succeeds when no manifest is present", () => { + const { home, workspace, runnerTemp } = setup(undefined); + + const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, RUNNER_TEMP: runnerTemp }); + expect(result.status).toBe(0); + + const entries = readSafeDirectories(home); + expect(entries).toEqual([workspace]); + }); +}); diff --git a/actions/setup/sh/configure_git_credentials.sh b/actions/setup/sh/configure_git_credentials.sh index d2bde8ecae6..cf47bc04101 100755 --- a/actions/setup/sh/configure_git_credentials.sh +++ b/actions/setup/sh/configure_git_credentials.sh @@ -8,9 +8,17 @@ # URL for authentication when credentials are provided; silently skips auth when any required # credential variable (GITHUB_REPOSITORY, GITHUB_SERVER_URL, GITHUB_TOKEN) is absent. # +# When a checkout manifest is present, every cross-repository checkout subdirectory it +# records is also trusted as a safe.directory so that safe-outputs handlers can run git +# inside those subdirectories without hitting "dubious ownership" errors. +# # Required environment variables: # GITHUB_WORKSPACE - Workspace directory path (for safe.directory) # +# Optional environment variables: +# RUNNER_TEMP - Runner temp dir; used to locate the checkout manifest +# GH_AW_CHECKOUT_MANIFEST - Explicit path to the checkout manifest (overrides default) +# # Optional environment variables for remote authentication: # GITHUB_REPOSITORY - Repository slug (e.g., "org/repo") # GITHUB_SERVER_URL - GitHub server URL (with or without https:// prefix) @@ -33,6 +41,47 @@ if [ -n "${GITHUB_WORKSPACE:-}" ]; then git config --global --add safe.directory "${GITHUB_WORKSPACE}" fi +# Trust cross-repository checkout directories recorded in the checkout manifest. +# Cross-repo checkouts live in subdirectories of the workspace (e.g. +# "${GITHUB_WORKSPACE}/github"), each a separate git repository whose top-level is +# not GITHUB_WORKSPACE. The safe-outputs handlers run git inside these +# subdirectories, so without trusting them git aborts with "dubious ownership" +# (surfacing as errors such as "Failed to pin branch"). +MANIFEST_PATH="${GH_AW_CHECKOUT_MANIFEST:-}" +if [ -z "${MANIFEST_PATH}" ] && [ -n "${RUNNER_TEMP:-}" ]; then + MANIFEST_PATH="${RUNNER_TEMP}/gh-aw/safeoutputs/checkout-manifest.json" +fi +if [ -n "${GITHUB_WORKSPACE:-}" ] && [ -n "${MANIFEST_PATH}" ] && [ -f "${MANIFEST_PATH}" ] && command -v node >/dev/null 2>&1; then + GH_AW_MANIFEST_PATH="${MANIFEST_PATH}" node -e ' + const fs = require("fs"); + const path = require("path"); + const ws = process.env.GITHUB_WORKSPACE || ""; + try { + const manifest = JSON.parse(fs.readFileSync(process.env.GH_AW_MANIFEST_PATH, "utf8")); + if (manifest && typeof manifest === "object") { + const seen = new Set(); + for (const entry of Object.values(manifest)) { + if (!entry || typeof entry !== "object") continue; + const p = typeof entry.path === "string" ? entry.path : ""; + if (!p) continue; + // Only trust paths that resolve to a location inside the workspace, + // guarding against path traversal in a malformed/hostile manifest. + const resolved = path.resolve(ws, p); + const rel = path.relative(ws, resolved); + if (rel === "" || rel.startsWith("..") || path.isAbsolute(rel)) continue; + if (seen.has(resolved)) continue; + seen.add(resolved); + process.stdout.write(resolved + "\n"); + } + } + } catch (_e) { + /* ignore missing or malformed manifest */ + } + ' 2>/dev/null | while IFS= read -r dir; do + [ -n "${dir}" ] && git config --global --add safe.directory "${dir}" + done +fi + # Configure remote URL authentication when all required credentials are present. # Silently skips when any variable is absent (e.g., inside the safeoutputs container # where GITHUB_SERVER_URL is intentionally not exposed). From a28abd107d18a871f18fa96588e8f394719fe30b Mon Sep 17 00:00:00 2001 From: Don Syme Date: Thu, 18 Jun 2026 15:24:15 +0100 Subject: [PATCH 2/5] style: apply prettier formatting to test --- actions/setup/js/configure_git_credentials.test.cjs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/actions/setup/js/configure_git_credentials.test.cjs b/actions/setup/js/configure_git_credentials.test.cjs index 7d5b8c1d86a..d461764f1bc 100644 --- a/actions/setup/js/configure_git_credentials.test.cjs +++ b/actions/setup/js/configure_git_credentials.test.cjs @@ -117,11 +117,7 @@ describe("configure_git_credentials.sh checkout manifest trust", () => { fs.mkdirSync(home, { recursive: true }); fs.mkdirSync(workspace, { recursive: true }); const manifestPath = path.join(root, "custom-manifest.json"); - fs.writeFileSync( - manifestPath, - JSON.stringify({ "owner/repo": { repository: "owner/repo", path: "sub", default_branch: "main" } }), - "utf8" - ); + fs.writeFileSync(manifestPath, JSON.stringify({ "owner/repo": { repository: "owner/repo", path: "sub", default_branch: "main" } }), "utf8"); const result = runScript({ HOME: home, GITHUB_WORKSPACE: workspace, GH_AW_CHECKOUT_MANIFEST: manifestPath }); expect(result.status).toBe(0); From 4dc109e6abbb264dc1c97021be2382623c9dd97f Mon Sep 17 00:00:00 2001 From: Don Syme Date: Thu, 18 Jun 2026 15:33:52 +0100 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- actions/setup/js/configure_git_credentials.test.cjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actions/setup/js/configure_git_credentials.test.cjs b/actions/setup/js/configure_git_credentials.test.cjs index d461764f1bc..0c3f4b7a791 100644 --- a/actions/setup/js/configure_git_credentials.test.cjs +++ b/actions/setup/js/configure_git_credentials.test.cjs @@ -24,7 +24,7 @@ function removeDir(dir) { function runScript(env) { return spawnSync("sh", [SCRIPT_PATH], { encoding: "utf8", - env: { ...env }, + env: { ...process.env, ...env }, }); } From 3e685ab8bf45b288241dc9b6933558e5686150b7 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Thu, 18 Jun 2026 15:34:06 +0100 Subject: [PATCH 4/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- actions/setup/sh/configure_git_credentials.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/actions/setup/sh/configure_git_credentials.sh b/actions/setup/sh/configure_git_credentials.sh index cf47bc04101..b8b608adeae 100755 --- a/actions/setup/sh/configure_git_credentials.sh +++ b/actions/setup/sh/configure_git_credentials.sh @@ -64,15 +64,16 @@ if [ -n "${GITHUB_WORKSPACE:-}" ] && [ -n "${MANIFEST_PATH}" ] && [ -f "${MANIFE if (!entry || typeof entry !== "object") continue; const p = typeof entry.path === "string" ? entry.path : ""; if (!p) continue; + if (/[\r\n\0]/.test(p)) continue; // Only trust paths that resolve to a location inside the workspace, // guarding against path traversal in a malformed/hostile manifest. const resolved = path.resolve(ws, p); + if (/[\r\n\0]/.test(resolved)) continue; const rel = path.relative(ws, resolved); if (rel === "" || rel.startsWith("..") || path.isAbsolute(rel)) continue; if (seen.has(resolved)) continue; seen.add(resolved); process.stdout.write(resolved + "\n"); - } } } catch (_e) { /* ignore missing or malformed manifest */ From 3b8eb9e4c8a78be8632e2a43220bb76d2e9f7614 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Thu, 18 Jun 2026 15:40:51 +0100 Subject: [PATCH 5/5] fix: restore for-loop closing brace in embedded manifest parser The autofix suggestion that added \r\n\0 path guards accidentally removed the for-loop's closing brace, breaking the embedded node script so it threw and trusted no cross-repo directories. This caused the configure_git_credentials JS tests to fail in CI. --- actions/setup/sh/configure_git_credentials.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/actions/setup/sh/configure_git_credentials.sh b/actions/setup/sh/configure_git_credentials.sh index b8b608adeae..a2bb4075fd9 100755 --- a/actions/setup/sh/configure_git_credentials.sh +++ b/actions/setup/sh/configure_git_credentials.sh @@ -74,6 +74,7 @@ if [ -n "${GITHUB_WORKSPACE:-}" ] && [ -n "${MANIFEST_PATH}" ] && [ -f "${MANIFE if (seen.has(resolved)) continue; seen.add(resolved); process.stdout.write(resolved + "\n"); + } } } catch (_e) { /* ignore missing or malformed manifest */