From 4c231e400d33a79f648af071078e9524a2e11a2a Mon Sep 17 00:00:00 2001 From: Anupam Mediratta Date: Mon, 21 Sep 2026 08:17:45 +0530 Subject: [PATCH] fix(action): block symlink escape past GITHUB_WORKSPACE runConfdiff() reads the new side of a changed file straight off the working tree, which follows symlinks. A PR that retargets an already-tracked symlink to point outside the checkout (e.g. a repo that legitimately symlinks shared config) could have its target's contents read and posted in the sticky PR comment. Canonicalize each changed file's path with realpathSync and skip (with a visible warning, same pattern as parse errors) any file that resolves outside GITHUB_WORKSPACE. This replaces the CLI-side "..".-segment check proposed in PR #4, per discussion there: the CLI must keep supporting legitimate relative paths, git never tracks ".."-containing paths so that check was dead code on the Action's actual input, and the real residual vector is this Action-side symlink read. Refs: https://github.com/esperanza-volkov/confdiff/pull/4#issuecomment-5754281257 https://github.com/esperanza-volkov/confdiff/pull/4#issuecomment-5754463539 Co-Authored-By: Claude Sonnet 5 --- action/index.mjs | 17 +++++++- test/action-symlink.test.ts | 83 +++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 test/action-symlink.test.ts diff --git a/action/index.mjs b/action/index.mjs index 3e58d75..2f98f9d 100644 --- a/action/index.mjs +++ b/action/index.mjs @@ -8,9 +8,9 @@ // and uses global fetch for the GitHub API. import { execFileSync } from "node:child_process"; -import { mkdtempSync, writeFileSync, readFileSync, appendFileSync, existsSync } from "node:fs"; +import { mkdtempSync, writeFileSync, readFileSync, appendFileSync, existsSync, realpathSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join, dirname, extname, basename } from "node:path"; +import { join, dirname, extname, basename, sep } from "node:path"; import { fileURLToPath } from "node:url"; const HERE = dirname(fileURLToPath(import.meta.url)); @@ -139,6 +139,7 @@ async function main() { const { files, warn } = changedFiles(base, pathspecs); const tmp = mkdtempSync(join(tmpdir(), "confdiff-")); + const workspace = realpathSync(env("GITHUB_WORKSPACE", process.cwd())); const sections = []; let anyDiff = false; let errors = 0; @@ -146,6 +147,18 @@ async function main() { for (const f of files) { const show = trySh("git", ["show", `${base}:${f}`]); if (!show.ok) continue; // not present at base (added/renamed) — skip + + // `f` is read straight off the working tree below, which follows symlinks. + // A tracked symlink retargeted (in the PR) to point outside the checkout + // would let its target's contents get read and posted in the PR comment. + let resolved; + try { resolved = realpathSync(f); } catch { resolved = null; } + if (resolved === null || (resolved !== workspace && !resolved.startsWith(workspace + sep))) { + errors++; + sections.push(`#### \`${f}\`\n\n> ⚠️ skipped: resolves outside the workspace (possible symlink escape)`); + continue; + } + const oldPath = join(tmp, "old_" + basename(f)); writeFileSync(oldPath, show.out); const { status, out } = runConfdiff(oldPath, f, stripped); diff --git a/test/action-symlink.test.ts b/test/action-symlink.test.ts new file mode 100644 index 0000000..eb66d45 --- /dev/null +++ b/test/action-symlink.test.ts @@ -0,0 +1,83 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, symlinkSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const here = dirname(fileURLToPath(import.meta.url)); +const ACTION = join(here, "..", "action", "index.mjs"); + +function git(cwd: string, args: string[]) { + const r = spawnSync("git", args, { cwd, encoding: "utf8" }); + if (r.status !== 0) throw new Error(`git ${args.join(" ")} failed: ${r.stderr}`); + return r.stdout.trim(); +} + +function initRepo() { + const dir = mkdtempSync(join(tmpdir(), "confdiff-action-")); + git(dir, ["init", "-q"]); + git(dir, ["config", "user.email", "test@test.com"]); + git(dir, ["config", "user.name", "test"]); + return dir; +} + +function runAction(repo: string, base: string) { + return spawnSync(process.execPath, [ACTION], { + cwd: repo, + encoding: "utf8", + env: { + ...process.env, + GITHUB_WORKSPACE: repo, + INPUT_BASE: base, + INPUT_COMMENT: "false", + INPUT_REDACT: "false", + INPUT_FAIL_ON_DIFF: "false", + INPUT_PATHS: "", + INPUT_ARGS: "", + GITHUB_EVENT_PATH: "", + }, + }); +} + +test("action: symlink retargeted outside the workspace is skipped, not read", () => { + const repo = initRepo(); + const secretDir = mkdtempSync(join(tmpdir(), "confdiff-secret-")); + writeFileSync(join(secretDir, "secret.json"), '{"TOP_SECRET_MARKER": true}\n'); + + writeFileSync(join(repo, "shared.json"), '{"a": 1}\n'); + symlinkSync("shared.json", join(repo, "config.json")); + git(repo, ["add", "shared.json", "config.json"]); + git(repo, ["commit", "-q", "-m", "base"]); + const base = git(repo, ["rev-parse", "HEAD"]); + + // Retarget the tracked symlink to point outside the workspace. + spawnSync("rm", [join(repo, "config.json")]); + symlinkSync(join(secretDir, "secret.json"), join(repo, "config.json")); + git(repo, ["add", "config.json"]); + git(repo, ["commit", "-q", "-m", "head"]); + + const r = runAction(repo, base); + assert.equal(r.status, 0, r.stderr); + assert.doesNotMatch(r.stdout, /TOP_SECRET_MARKER/); + assert.match(r.stdout, /resolves outside the workspace/); +}); + +test("action: a normal in-workspace config change still produces a semantic diff", () => { + const repo = initRepo(); + writeFileSync(join(repo, "config.json"), '{"a": 1}\n'); + git(repo, ["add", "config.json"]); + git(repo, ["commit", "-q", "-m", "base"]); + const base = git(repo, ["rev-parse", "HEAD"]); + + writeFileSync(join(repo, "config.json"), '{"a": 2}\n'); + git(repo, ["add", "config.json"]); + git(repo, ["commit", "-q", "-m", "head"]); + + const r = runAction(repo, base); + assert.equal(r.status, 0, r.stderr); + assert.doesNotMatch(r.stdout, /resolves outside the workspace/); + assert.match(r.stdout, /config\.json/); + assert.match(r.stdout, /1 => 2/); +});