From 607cad118a4f8ae95a940b9e56381edf96344d92 Mon Sep 17 00:00:00 2001
From: Mark Larah <mark@larah.me>
Date: Tue, 19 May 2026 17:07:06 -0700
Subject: [PATCH] Harden /merge workflow against race conditions and path
 traversal

- Remove --auto flag from gh pr merge to prevent race condition where
  PR contents could change between authorization check and merge
- Add path normalization check to reject traversal attempts
- Add explicit check that all files are within the authorized GAP dir
- Check PR mergeable state before attempting merge
- Filter out comments from users with no repo association to reduce
  Actions minute burn from spam
- Switch from ad-hoc js-yaml install to npm ci with lockfile (uses the
  yaml package already in devDependencies)
- Convert to ESM to match the rest of the project
- Parallelize API calls for faster execution
- Add per_page: 100 to listFiles to handle larger PRs
- Use async readFile instead of blocking readFileSync

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---
 .github/workflows/merge.yml | 18 +++++++---
 scripts/verify-merge.js     | 70 ++++++++++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 20 deletions(-)

diff --git a/.github/workflows/merge.yml b/.github/workflows/merge.yml
index 18188da..d320075 100644
--- a/.github/workflows/merge.yml
+++ b/.github/workflows/merge.yml
@@ -13,16 +13,26 @@ jobs:
     runs-on: ubuntu-latest
     if: >
       github.event.issue.pull_request &&
-      github.event.comment.body == '/merge'
+      github.event.comment.body == '/merge' &&
+      github.event.comment.author_association != 'NONE'
     steps:
       - uses: actions/checkout@v6
-      - run: npm install js-yaml
+
+      - name: Setup Node.js
+        uses: actions/setup-node@v6
+        with:
+          node-version: "24"
+          cache: "npm"
+
+      - run: npm ci
 
       - name: Verify actor is authorized
         uses: actions/github-script@v9
         with:
           script: |
-            const verifyMerge = require('./scripts/verify-merge.js');
+            const { default: verifyMerge } = await import(
+              `${process.env.GITHUB_WORKSPACE}/scripts/verify-merge.js`
+            );
             await verifyMerge({ github, context });
 
       - name: Merge PR
@@ -30,4 +40,4 @@ jobs:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           PR_NUMBER: ${{ github.event.issue.number }}
         run: |
-          gh pr merge "$PR_NUMBER" --squash --auto
+          gh pr merge "$PR_NUMBER" --squash
diff --git a/scripts/verify-merge.js b/scripts/verify-merge.js
index 5ad8e3e..a46d75a 100644
--- a/scripts/verify-merge.js
+++ b/scripts/verify-merge.js
@@ -1,32 +1,70 @@
-const fs = require("fs");
-const yaml = require("js-yaml");
+import { readFile } from "fs/promises";
+import path from "path";
+import { parse as parseYaml } from "yaml";
 
-module.exports = async ({ github, context }) => {
+export default async ({ github, context }) => {
   const actor = context.payload.comment.user.login;
   const prNumber = context.issue.number;
 
-  const { data: files } = await github.rest.pulls.listFiles({
-    ...context.repo,
-    pull_number: prNumber,
-  });
+  const [{ data: pr }, { data: files }] = await Promise.all([
+    github.rest.pulls.get({
+      ...context.repo,
+      pull_number: prNumber,
+    }),
+    github.rest.pulls.listFiles({
+      ...context.repo,
+      pull_number: prNumber,
+      per_page: 100,
+    }),
+  ]);
+
+  if (pr.mergeable === false) {
+    throw new Error(
+      "PR is not in a mergeable state. Resolve conflicts and try again.",
+    );
+  }
+
+  const gapDirSet = new Set();
 
-  const gapDirs = files
-    .map((f) => f.filename)
-    .map((p) => p.split("/").slice(0, 2).join("/"));
+  for (const f of files) {
+    const normalized = path.normalize(f.filename);
+    if (normalized !== f.filename || normalized.startsWith("..")) {
+      throw new Error(
+        `File path "${f.filename}" contains path traversal or is not normalized.`,
+      );
+    }
+    gapDirSet.add(f.filename.split("/").slice(0, 2).join("/"));
+  }
 
-  const gapsChanged = [...new Set(gapDirs)];
+  const gapsChanged = [...gapDirSet];
 
   if (gapsChanged.length !== 1 || !gapsChanged[0].match(/^gaps\/GAP-\d+$/)) {
-    throw new Error("You can only run /merge for PRs that touch exactly one GAP directory and nothing else.");
+    throw new Error(
+      "You can only run /merge for PRs that touch exactly one GAP directory and nothing else.",
+    );
+  }
+
+  const gapDir = gapsChanged[0];
+
+  for (const f of files) {
+    if (!f.filename.startsWith(`${gapDir}/`)) {
+      throw new Error(
+        `File "${f.filename}" is outside the expected GAP directory (${gapDir}).`,
+      );
+    }
   }
 
-  const metadata = yaml.load(fs.readFileSync(`${gapsChanged[0]}/metadata.yml`, "utf8"));
+  const metadata = parseYaml(
+    await readFile(`${gapDir}/metadata.yml`, "utf8"),
+  );
   const authorizedMergers = new Set([
-    ...metadata.authors.map(author => author.githubUsername.replace(/^@/, '')),
-    metadata.sponsor.replace(/^@/, ''),
+    ...metadata.authors.map((author) =>
+      author.githubUsername.replace(/^@/, ""),
+    ),
+    metadata.sponsor.replace(/^@/, ""),
   ]);
 
   if (!authorizedMergers.has(actor)) {
-    throw new Error(`${actor} is not authorized to merge ${gapsChanged[0]}.`);
+    throw new Error(`${actor} is not authorized to merge ${gapDir}.`);
   }
 };