Skip to content

Commit

Permalink
avoid race condition between scdiff comment and fetching PR head sha
Browse files Browse the repository at this point in the history
There is a small window after leaving an scdiff comment, where the
workflow queues then sends an API request to determine the PR head SHA.
An attacker could use this time to push new code that wasn't reviewed.

This change attempts to ensure the code that runs is older than the code
the requester saw when leaving the scdiff comment. Both timestamps used
are controlled by GitHub, not a user controlled timestamp.

There may be some false positives, as `repo.pushed_at` corresponds to
all repo activiy, not just the branch used for the PR. This risk is
acceptable as it's better to be safe; we can always re-run the workflow.

Signed-off-by: Spencer Schrock <sschrock@google.com>
  • Loading branch information
spencerschrock committed Oct 8, 2024
1 parent c1505e1 commit a051f5c
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions .github/workflows/scdiff.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,24 @@ jobs:
authorAssociation = '${{ github.event.comment.author_association }}'
if (!allowedAssociations.includes(authorAssociation)) {
core.setFailed("You don't have access to run scdiff");
return
}
const response = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
})
// avoid race condition between scdiff comment and fetching PR head sha
const commentTime = new Date('${{ github.event.comment.created_at }}');
const prTime = new Date(response.data.head.repo.pushed_at)
if (prTime >= commentTime) {
core.setFailed("The PR may have been updated since the scdiff request, " +
"please review any changes and relaunch if safe.");
return
}
core.setOutput('base', response.data.base.sha)
core.setOutput('head', response.data.head.sha)
Expand Down

0 comments on commit a051f5c

Please sign in to comment.