Skip to content

Commit

Permalink
🌱 Tighten restrictions for running scdiff workflow (#4376)
Browse files Browse the repository at this point in the history
* remove ability for contributors to trigger scdiff

Previously we matched GitHub's "Require approval for first-time
contributors", which represents a minor barrier for attackers
(e.g. submitting a typo fix). Project members should ensure their
visibility in the "ossf" GitHub org is "Public" to be able to run
scdiff.

Signed-off-by: Spencer Schrock <[email protected]>

* avoid race condition between scdiff comment and fetching PR head sha

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 <[email protected]>

---------

Signed-off-by: Spencer Schrock <[email protected]>
  • Loading branch information
spencerschrock authored Oct 8, 2024
1 parent 1bbae1a commit 28db9a9
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion .github/workflows/scdiff.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,28 @@ jobs:
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
script: |
const allowedAssociations = ["COLLABORATOR", "CONTRIBUTOR", "MEMBER", "OWNER"];
const allowedAssociations = ["COLLABORATOR", "MEMBER", "OWNER"];
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 28db9a9

Please sign in to comment.