Harden /merge workflow security#38
Closed
magicmark wants to merge 1 commit into
Closed
Conversation
- 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>
magicmark
force-pushed
the
fix/merge-security-hardening
branch
from
44edaaa to
607cad1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security hardening for the
/mergeworkflow introduced in #32.Changes
Remove
--autoflag —gh pr merge --squash --autoenables auto-merge, which means the actual merge happens later (when checks pass). Between the/mergeauthorization check and the eventual merge, a PR author could force-push new commits that touch files outside their GAP directory (e.g..github/workflows/). By merging immediately (without--auto), the authorization check and merge are atomic within the same workflow run.Path traversal defense — Validate that all PR file paths are normalized (no
..components) and explicitly verify every file lives under the single authorized GAP directory. Git itself prevents..in tree paths, so this is defense-in-depth rather than an active exploit, but it guards against any future API behavior changes.Mergeable state check — Verify
pr.mergeablebefore attempting merge to fail fast with a clear error message.Filter out
NONEassociation commenters — Skip workflow runs for commenters with no relationship to the repo. The authorization check would reject them anyway, but this avoids burning Actions minutes on spam.Use
npm ciwith lockfile instead of ad-hocnpm install js-yaml— The original workflow installedjs-yamlwithout version pinning at runtime, which is a supply-chain risk. The repo already has theyamlpackage indevDependencieswith a lockfile, so we use that instead.Convert to ESM — The project is
"type": "module"and all other scripts use ESM. Alignsverify-merge.jswith the rest of the codebase.Security model after this change
issue_commenttriggers run the workflow from the default branch — an attacker cannot modify the verification logic via their PRmetadata.ymlonmain(the checkout is the default branch)gh pr mergesucceeds)Test plan
/mergeon a PR touching a single GAP dir as an authorized author → merges/mergeas an unauthorized user → rejects with error/mergeon a PR touching multiple GAP dirs → rejects/mergeon a PR touching files outsidegaps/→ rejects🤖 Generated with Claude Code