Skip to content

A better security-wise style bot GH Action #2914

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
merged 14 commits into from
Apr 3, 2025
+84 −43
Merged

A better security-wise style bot GH Action #2914

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5828734
better security-wise gh action
hanouticelina Mar 7, 2025
1831f66
run workflow manually
hanouticelina Mar 7, 2025
61c5cbb
nit
hanouticelina Mar 7, 2025
4d42d1b
fix
hanouticelina Mar 7, 2025
d81b168
Add credits comment
hanouticelina Mar 10, 2025
3fc12e3
make pr_number required
hanouticelina Mar 14, 2025
cfb6aae
remove file protected checking
hanouticelina Mar 14, 2025
09d0b68
Merge branch 'main' into fix-security-vulnerability-style-bot-action
hanouticelina Mar 14, 2025
b1568e7
fix
hanouticelina Mar 14, 2025
5dd2c6e
Merge branch 'fix-security-vulnerability-style-bot-action' of github.…
hanouticelina Mar 14, 2025
af3bb09
add timestamp, remove manual trigger and allow write access
hanouticelina Apr 1, 2025
3b03d35
checkout PR branch again
hanouticelina Apr 1, 2025
1063565
review suggestions
hanouticelina Apr 1, 2025
264bc0c
Merge branch 'main' into fix-security-vulnerability-style-bot-action
hanouticelina Apr 3, 2025
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
55 changes: 26 additions & 29 deletions .github/workflows/style-bot-action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ on:
type: string
description: "Which style command to run (options: 'default' (make style && make quality), 'quality_only', 'style_only')"
default: "default"
pr_number:
required: true
type: number
description: "Pull Request number to process"
python_quality_dependencies:
required: true
type: string
Expand All @@ -38,8 +34,7 @@ jobs:
if: >
(github.event_name == 'issue_comment' &&
contains(github.event.comment.body, '@bot /style') &&
github.event.issue.pull_request != null) ||
github.event_name == 'workflow_dispatch'
github.event.issue.pull_request != null)
runs-on: ubuntu-latest
outputs:
is_authorized: ${{ steps.check_user_permission.outputs.has_permission }}
Expand All @@ -49,21 +44,16 @@ jobs:
uses: actions/github-script@v6
with:
script: |
let comment_user;
if (context.eventName === 'workflow_dispatch') {
comment_user = context.actor;
console.log('Workflow triggered manually by:', comment_user);
} else {
comment_user = context.payload.comment.user.login;
console.log('Workflow triggered by comment from:', comment_user);
}

comment_user = context.payload.comment.user.login;
const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: comment_user
});
const authorized = permission.permission === 'admin';
console.log(`User ${comment_user} has permission level: ${permission.permission}, authorized: ${authorized} (only admins allowed)`);

const authorized = ['admin', 'maintain', 'push'].includes(permission.permission);
console.log(`User ${comment_user} has permission level: ${permission.permission}, authorized: ${authorized} (only users with at least write access are allowed to run this action)`);
Comment on lines +55 to +56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added directly the changes from #2939 here and i will close the other PR

core.setOutput('has_permission', authorized);

run-style-bot:
Expand All @@ -74,11 +64,9 @@ jobs:
- name: Extract PR details
id: pr_info
uses: actions/github-script@v6
env:
PR_NUMBER: ${{ inputs.pr_number }}
with:
script: |
const prNumber = process.env.PR_NUMBER;
const prNumber = context.payload.issue.number;
console.log(`PR number from env: "${prNumber}"`);

const { data: pr } = await github.rest.pulls.get({
Expand All @@ -99,7 +87,7 @@ jobs:
headRef: pr.head.ref,
baseRef: pr.base.ref
});

- name: Check out PR branch
uses: actions/checkout@v3
env:
Expand All @@ -112,16 +100,25 @@ jobs:
# You may need fetch-depth: 0 for being able to push
fetch-depth: 0
token: ${{ secrets.bot_token }}

- name: Debug
env:
HEADREPOFULLNAME: ${{ steps.pr_info.outputs.headRepoFullName }}
HEADREF: ${{ steps.pr_info.outputs.headRef }}
PRNUMBER: ${{ steps.pr_info.outputs.prNumber }}

- name: Check commit timestamps
env:
COMMENT_DATE: ${{ github.event.comment.created_at }}
PR_NUMBER: ${{ steps.pr_info.outputs.prNumber }}
run: |
echo "PR number: $PRNUMBER"
echo "Head Ref: $HEADREF"
echo "Head Repo Full Name: $HEADREPOFULLNAME"
git fetch origin refs/pull/$PR_NUMBER/merge:refs/remotes/pull/$PR_NUMBER/merge
git checkout refs/remotes/pull/$PR_NUMBER/merge
echo "PR_MERGE_SHA: $(git log -1 --format=%H)"
echo "PR_MERGE_SHA=$(git log -1 --format=%H)" >> "$GITHUB_OUTPUT"
PR_MERGE_COMMIT_TIMESTAMP=$(git log -1 --date=unix --format=%cd)
echo "PR_MERGE_COMMIT_TIMESTAMP: $PR_MERGE_COMMIT_TIMESTAMP"
COMMENT_TIMESTAMP=$(date -d "${COMMENT_DATE}" +"%s")
echo "COMMENT_DATE: $COMMENT_DATE"
echo "COMMENT_TIMESTAMP: $COMMENT_TIMESTAMP"
if [ $COMMENT_TIMESTAMP -le $PR_MERGE_COMMIT_TIMESTAMP ]; then
echo "❌ Last commit on the pull request is newer than the issue comment triggering this run! Abort!";
exit -1;
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

after this, we have to checkout back to the

          # Instead of checking out the base repo, use the contributor's repo name
          repository: ${{ env.HEADREPOFULLNAME }}
          ref: ${{ env.HEADREF }}

if I understand correctly.

Copy link
Contributor

@ydshieh ydshieh Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(and if the above is true)

And because of this time of switching from HF's repo (with merge commit) to the contributor's repo., there is also a possibility of attack by pushing new commit after the security check is finished.

Ok, nothing big, if we specify the already created HEADREF when we switch back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think you're right, we need to checkout the PR's head branch again. what do you suggest to avoid any attack in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I added another checkout with the same HEADREF after the timestamp check step in 3b03d35.

Copy link
Contributor

Choose a reason for hiding this comment

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

(sorry, my previous message might be a bit misleading , as I didn't think it thoroughly)

I think it could be simple as just git checkout $HEADREF.

From the first place where we checkout with

        uses: actions/checkout@v3
        env: 	        env: 
          HEADREPOFULLNAME: ${{ steps.pr_info.outputs.headRepoFullName }}	          HEADREPOFULLNAME: ${{ steps.pr_info.outputs.headRepoFullName }}

the origin is headRepoFullName . Later when we fetch/checkout , it doesn't change the origin although it checkout to the content of merge commit from refs/pull/$PR_NUMBER/merge.

So later when we do git push origin HEAD:$HEADREF, the origin is still the contributor's repo.

If this works, we don't need an extra step. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 1063565

- name: Set up Python
uses: actions/setup-python@v4
with:
Expand Down
7 changes: 0 additions & 7 deletions .github/workflows/style-bot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@ name: Style Bot
on:
issue_comment:
types: [created]
workflow_dispatch:
inputs:
pr_number:
description: 'Pull Request number to run style checks on'
required: true
type: number

permissions:
contents: write
Expand All @@ -20,6 +14,5 @@ jobs:
with:
python_quality_dependencies: "[quality]"
style_command_type: "style_only"
pr_number: ${{ fromJSON(github.event_name == 'workflow_dispatch' && github.event.inputs.pr_number || github.event.issue.number) }}
secrets:
bot_token: ${{ secrets.GITHUB_TOKEN }}
Loading