Harden changelog workflows against concurrency DoS, TOCTOU, and injection#41
Harden changelog workflows against concurrency DoS, TOCTOU, and injection#41
Conversation
…tion The recent security hardening of docs-deploy.yml (866ff9a, d6e9751) fixed several classes of vulnerability. The changelog workflows had the same exposure — this commit applies equivalent mitigations: - Append workflow_run.id and head_repository.full_name to the changelog-submit concurrency group to prevent fork authors from canceling legitimate runs via branch-name collision - Set top-level permissions to {} on both reusable workflows so any future jobs default to no permissions - Add repository.fork == false guard to changelog-submit to prevent execution when the hosting repo is itself a fork - Checkout by SHA instead of branch name in the submit action to close the TOCTOU race window between PR data fetch and checkout - Validate HEAD_REF and BASE_REF against an allowlist regex before use in git commands - Move ${{ github.repository_owner }} and ${{ github.event.repository.name }} from direct interpolation in run: blocks to env: variables to eliminate expression injection surface - Move ${{ steps.evaluate.outputs.status }} to env var in the validate action Gate step for the same reason - Add escapeMarkdown() helper and URL-encode branch/path segments in PR comment links to prevent markdown link injection Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR applies the security hardening patterns previously introduced for docs-deploy.yml to the changelog workflows and their composite actions, focusing on concurrency key robustness, preventing TOCTOU checkout races, and reducing injection risk in both shell steps and PR comments.
Changes:
- Harden reusable workflow concurrency + permissions defaults and add a fork-repo execution guard for changelog submit.
- Update composite actions to reduce expression interpolation in
run:blocks and validate ref names. - Add markdown escaping and URL encoding for PR comment content/links.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/changelog-submit.yml |
Adds workflow-level permissions: {}, hardens concurrency group, and prevents running on forked repositories. |
.github/workflows/changelog-validate.yml |
Adds workflow-level permissions: {} for defense-in-depth. |
changelog/submit/action.yml |
Switches checkout to SHA, adds ref allowlist validation, and moves GitHub expressions into env vars. |
changelog/validate/action.yml |
Moves GitHub expressions into env vars (evaluate + gate) to reduce shell interpolation risk. |
changelog/submit/scripts/comment-helper.js |
Introduces escapeMarkdown() helper for PR comment safety. |
changelog/submit/scripts/post-success-comment.js |
Escapes markdown in displayed paths and URL-encodes link components. |
changelog/submit/scripts/post-comment-only.js |
Escapes markdown in displayed file path for comment-only mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { owner, repo } = context.repo; | ||
| const viewUrl = `https://github.com/${owner}/${repo}/blob/${branch}/${changelogFile}`; | ||
| const editUrl = `https://github.com/${owner}/${repo}/edit/${branch}/${changelogFile}`; | ||
| const safeBranch = encodeURIComponent(branch); |
There was a problem hiding this comment.
encodeURIComponent(branch) will encode / as %2F. For common branch names like feature/foo, this typically produces GitHub URLs that don't resolve because the ref segment is no longer parsed as the intended ref. Prefer leaving the ref unescaped (Git refs already forbid many dangerous characters) or encoding in a way that preserves / (e.g., encode segments but keep /).
| const safeBranch = encodeURIComponent(branch); | |
| const safeBranch = branch; |
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ steps.pr-data.outputs.head-ref }} | ||
| ref: ${{ steps.pr-data.outputs.head-sha }} |
There was a problem hiding this comment.
Checking out the PR by SHA will typically leave the repo in a detached HEAD state. The later Commit changelog step runs git push without specifying a ref, which usually fails from detached HEAD (and can also push to an unintended ref if push.default is configured). To keep the TOCTOU fix while still pushing to the PR branch, create/check out a local branch at head-ref after verifying the SHA (or configure checkout to land on the branch) before committing/pushing.
| ref: ${{ steps.pr-data.outputs.head-sha }} | |
| ref: ${{ steps.pr-data.outputs.head-ref }} |
| @@ -1,5 +1,7 @@ | |||
| const TITLE = '### 📋 Changelog'; | |||
|
|
|||
| const escapeMarkdown = (s) => s.replace(/([[\]()\\`*_{}#+\-.!|])/g, '\\$1'); | |||
There was a problem hiding this comment.
escapeMarkdown() is escaping many characters (e.g., ., -, *, etc.), which will introduce visible backslashes in the rendered PR comment because the paths are already wrapped in backticks (inline code). This also diverges from the established pattern in docs-deploy.yml where only []()\\ are escaped for link text. Consider either using a minimal escape that matches docs-deploy (for plain link labels) and avoiding backticks, or adding a separate escape specifically for inline code spans (typically only backticks need escaping).
| const escapeMarkdown = (s) => s.replace(/([[\]()\\`*_{}#+\-.!|])/g, '\\$1'); | |
| const escapeMarkdown = (s) => s.replace(/([[\]()\\])/g, '\\$1'); |
Summary
The recent security hardening of
docs-deploy.yml(#31, #36) fixed concurrency DoS, TOCTOU checkout races, and expression injection vulnerabilities. The changelog workflows (changelog-submit.yml,changelog-validate.yml) and their composite actions had the same classes of exposure. This PR applies equivalent mitigations.Why these changes are necessary
Concurrency DoS (HIGH) — The
changelog-submitconcurrency group keyed only onhead_branch, meaning a fork author could create a branch with the same name as an internal branch and cause their workflow run to cancel legitimate runs (or vice versa). The fix addshead_repository.full_nameandworkflow_run.idto the concurrency key, matching the pattern established in docs-deploy.TOCTOU checkout race (HIGH) — The submit action checked out code by branch name (
head-ref), then verified the SHA afterward. Between checkout and verification, an attacker could push a new commit to the branch, causing the action to execute untrusted code in a workflow withcontents: writeandpull-requests: writepermissions. The fix checks out by SHA directly, closing the race window entirely.Missing fork-repository guard (MEDIUM) —
docs-deployguards against execution whengithub.event.repository.fork == false. The changelog-submit workflow was missing this, which could allow the workflow to run with write permissions on a forked repository.Unrestricted top-level permissions (MEDIUM) — Neither workflow set
permissions: {}at the workflow level. Without this, any new job added in the future would inherit the caller's full permission set rather than defaulting to none. This is the defense-in-depth pattern established in docs-deploy.Expression injection in
run:blocks (MEDIUM) — Severalrun:steps used${{ }}interpolation directly in shell code forgithub.repository_owner,github.event.repository.name, andsteps.evaluate.outputs.status. While these specific values are not typically attacker-controlled, the pattern is fragile — any future change that introduces an attacker-influenced value in the same style would be exploitable. All such values are now passed throughenv:variables.Markdown/URL injection in PR comments (LOW) — Branch names and file paths were embedded directly into markdown links without escaping. A crafted branch name or file path could inject arbitrary markdown into PR comments. The fix adds
escapeMarkdown()(matching the docs-deploy pattern) and URL-encodes path segments.No branch name validation (LOW) — The submit action used branch names in
git showcommands without validation. An allowlist regex (^[a-zA-Z0-9._/+-]+$) is now enforced, matching the docs-deploy pattern.Changes
changelog-submit.ymlpermissions: {}, fork-repo guardchangelog-validate.ymlpermissions: {}changelog/submit/action.yml${{ }}→ env varschangelog/validate/action.yml${{ }}→ env vars in evaluate + gate stepscomment-helper.jsescapeMarkdown()utilitypost-success-comment.jspost-comment-only.jsTest plan
changelog-validate→ confirm validation still passeschangelog-submitgenerates and commits changelog entries on non-fork PRsMade with Cursor