Skip to content

Harden changelog workflows against concurrency DoS, TOCTOU, and injection #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mpdreamz merged 1 commit into from
Mar 26, 2026
Merged

Harden changelog workflows against concurrency DoS, TOCTOU, and injection #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/changelog-submit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,18 @@ on:
type: boolean
default: false

permissions: {}

concurrency:
group: changelog-submit-${{ github.event.workflow_run.head_branch || github.run_id }}
group: changelog-submit-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }}-${{ github.event.workflow_run.id }}
cancel-in-progress: true

jobs:
submit:
if: >
github.event.workflow_run.event == 'pull_request'
&& github.event.workflow_run.conclusion == 'success'
&& github.event.repository.fork == false
runs-on: ubuntu-latest
permissions:
contents: write
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/changelog-validate.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:
type: string
default: 'docs/changelog.yml'

permissions: {}

concurrency:
group: changelog-${{ github.event.pull_request.number || github.run_id }}
cancel-in-progress: true
Expand Down
22 changes: 19 additions & 3 deletions changelog/submit/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,28 @@ runs:
if: steps.pr-data.outputs.proceed == 'true' && steps.pr-data.outputs.is-fork != 'true'
uses: actions/checkout@v6
with:
ref: ${{ steps.pr-data.outputs.head-ref }}
ref: ${{ steps.pr-data.outputs.head-sha }}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
ref: ${{ steps.pr-data.outputs.head-sha }}
ref: ${{ steps.pr-data.outputs.head-ref }}

Copilot uses AI. Check for mistakes.
token: ${{ inputs.github-token }}
persist-credentials: false

- name: Checkout base repo
if: steps.pr-data.outputs.proceed == 'true' && steps.pr-data.outputs.is-fork == 'true'
uses: actions/checkout@v6

- name: Validate ref names
if: steps.pr-data.outputs.proceed == 'true' && steps.pr-data.outputs.is-fork != 'true'
shell: bash
env:
HEAD_REF: ${{ steps.pr-data.outputs.head-ref }}
BASE_REF: ${{ steps.pr-data.outputs.base-ref }}
run: |
for ref_name in "$HEAD_REF" "$BASE_REF"; do
if [[ ! "$ref_name" =~ ^[a-zA-Z0-9._/+-]+$ ]]; then
echo "::error::Ref name contains disallowed characters: ${ref_name}"
exit 1
fi
done

- name: Read config from base branch
id: config
if: steps.pr-data.outputs.proceed == 'true' && steps.pr-data.outputs.is-fork != 'true'
Expand Down Expand Up @@ -102,6 +116,8 @@ runs:
env:
GITHUB_TOKEN: ${{ inputs.github-token }}
CONFIG_FILE: ${{ steps.config.outputs.config-path || inputs.config }}
REPO_OWNER: ${{ github.repository_owner }}
REPO_NAME: ${{ github.event.repository.name }}
PR_NUMBER: ${{ steps.pr.outputs.number }}
PR_TITLE: ${{ steps.pr-data.outputs.title }}
PR_LABELS: ${{ steps.pr-data.outputs.labels }}
Expand All @@ -116,8 +132,8 @@ runs:

docs-builder changelog evaluate-pr \
--config "$CONFIG_FILE" \
--owner "${{ github.repository_owner }}" \
--repo "${{ github.event.repository.name }}" \
--owner "$REPO_OWNER" \
--repo "$REPO_NAME" \
--pr-number "$PR_NUMBER" \
--pr-title "$PR_TITLE" \
--pr-labels "$PR_LABELS" \
Expand Down
4 changes: 3 additions & 1 deletion changelog/submit/scripts/comment-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
const TITLE = '### 📋 Changelog';

const escapeMarkdown = (s) => s.replace(/([[\]()\\`*_{}#+\-.!|])/g, '\\$1');
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
const escapeMarkdown = (s) => s.replace(/([[\]()\\`*_{}#+\-.!|])/g, '\\$1');
const escapeMarkdown = (s) => s.replace(/([[\]()\\])/g, '\\$1');

Copilot uses AI. Check for mistakes.

async function upsertComment({ github, context, prNumber, body }) {
const { owner, repo } = context.repo;
const comments = await github.paginate(github.rest.issues.listComments, {
Expand All @@ -15,4 +17,4 @@ async function upsertComment({ github, context, prNumber, body }) {
}
}

module.exports = { TITLE, upsertComment };
module.exports = { TITLE, upsertComment, escapeMarkdown };
4 changes: 2 additions & 2 deletions changelog/submit/scripts/post-comment-only.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const fs = require('fs');
const { TITLE, upsertComment } = require('./comment-helper');
const { TITLE, upsertComment, escapeMarkdown } = require('./comment-helper');

module.exports = async ({ github, context, core }) => {
const prNumber = parseInt(process.env.PR_NUMBER, 10);
Expand All @@ -14,7 +14,7 @@ module.exports = async ({ github, context, core }) => {
const bodyParts = [TITLE, ''];
if (content) {
bodyParts.push(
`Generated changelog entry for \`${changelogDir}/${files[0]}\`:`,
`Generated changelog entry for \`${escapeMarkdown(changelogDir + '/' + files[0])}\`:`,
'',
'```yaml',
content,
Expand Down
10 changes: 6 additions & 4 deletions changelog/submit/scripts/post-success-comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
const { TITLE, upsertComment } = require('./comment-helper');
const { TITLE, upsertComment, escapeMarkdown } = require('./comment-helper');

module.exports = async ({ github, context, core }) => {
const prNumber = parseInt(process.env.PR_NUMBER, 10);
const branch = process.env.HEAD_REF;
const changelogFile = process.env.CHANGELOG_FILE;
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);
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 /).

Suggested change
const safeBranch = encodeURIComponent(branch);
const safeBranch = branch;

Copilot uses AI. Check for mistakes.
const safePath = changelogFile.split('/').map(encodeURIComponent).join('/');
const viewUrl = `https://github.com/${owner}/${repo}/blob/${safeBranch}/${safePath}`;
const editUrl = `https://github.com/${owner}/${repo}/edit/${safeBranch}/${safePath}`;

const body = [
TITLE,
'',
`📝 Changelog entry committed: [\`${changelogFile}\`](${viewUrl})`,
`📝 Changelog entry committed: [\`${escapeMarkdown(changelogFile)}\`](${viewUrl})`,
'',
`✏️ [Edit this changelog](${editUrl})`,
].join('\n');
Expand Down
9 changes: 6 additions & 3 deletions changelog/validate/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ runs:
env:
GITHUB_TOKEN: ${{ inputs.github-token }}
CONFIG_FILE: ${{ inputs.config }}
REPO_OWNER: ${{ github.repository_owner }}
REPO_NAME: ${{ github.event.repository.name }}
PR_NUMBER: ${{ github.event.pull_request.number }}
PR_TITLE: ${{ github.event.pull_request.title }}
PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }}
Expand All @@ -59,8 +61,8 @@ runs:

docs-builder changelog evaluate-pr \
--config "$CONFIG_FILE" \
--owner "${{ github.repository_owner }}" \
--repo "${{ github.event.repository.name }}" \
--owner "$REPO_OWNER" \
--repo "$REPO_NAME" \
--pr-number "$PR_NUMBER" \
--pr-title "$PR_TITLE" \
--pr-labels "$PR_LABELS" \
Expand All @@ -72,8 +74,9 @@ runs:

- name: Gate
shell: bash
env:
STATUS: ${{ steps.evaluate.outputs.status }}
run: |
STATUS="${{ steps.evaluate.outputs.status }}"
case "$STATUS" in
proceed|no-label|skipped|manually-edited)
echo "::notice::Changelog validation: $STATUS"
Expand Down
Loading