fix: bad token error of notify_triagers workflow #3477
Conversation
sambhavgupta0705
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.github/workflows/notify-triager.yml (2)
Line range hint
59-77: Add validation for triager extractionThe current implementation assumes a specific CODEOWNERS format without validation.
Add validation to prevent workflow failures:
docTriagers=$(grep '^#' CODEOWNERS | tail -n 2 | head -n 1) +if [ -z "$docTriagers" ]; then + echo "Error: No doc triagers found in CODEOWNERS" + exit 1 +fi echo "docTriagers: $docTriagers" prefix="#docTriagers: " docTriagers=${docTriagers#$prefix} +if [ "$docTriagers" = "#docTriagers: " ]; then + echo "Error: Invalid doc triagers format in CODEOWNERS" + exit 1 +fi echo "docTriagers=$docTriagers" >> $GITHUB_ENV
Line range hint
79-108: Add error handling for GitHub API callsThe current implementation doesn't handle API failures gracefully.
Add error handling and response validation:
curl \ + --fail \ + --retry 3 \ -X POST \ -H "Authorization: token ${{ secrets.GH_TOKEN }}" \ -H "Accept: application/vnd.github.v3+json" \ https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers \ -d "{ \"reviewers\": $reviewers - }" + }" || { + echo "Failed to add reviewers. Status: $?" + exit 1 + }This change will:
- Add automatic retry for transient failures
- Fail explicitly on HTTP errors
- Provide better error reporting
🧹 Nitpick comments (2)
.github/workflows/notify-triager.yml (2)
Line range hint
21-24: Consider using a more robust commit message sanitizationThe current approach with multiple
sedcommands works but could be simplified and made more robust.Consider this alternative:
- commit_message=$(echo "$commit_message" | tr '\n' ' ') - commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | sed 's/[][]//g' | sed 's/(//g' | sed 's/)//g' | xargs) + # Combine all transformations into a single sed command and preserve message length + commit_message=$(echo "$commit_message" | tr '\n' ' ' | sed 's/[<>|()[\]]//g' | awk '{$1=$1};1')
Line range hint
45-57: Enhance file change detection reliabilityThe current implementation correctly handles basic cases, but consider these improvements:
- changed_files=$(git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }}) + # Use --diff-filter to exclude deleted files and ensure we only count actual changes + changed_files=$(git diff --name-only --diff-filter=d ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }}) - md_count=$(echo "$changed_files" | grep -c '\.md$' || true) - non_md_count=$(echo "$changed_files" | grep -vc '\.md$' || true) + # More precise file type detection + md_count=$(echo "$changed_files" | grep -i '\.md$' -c || true) + non_md_count=$(echo "$changed_files" | grep -iv '\.md$' -c || true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/notify-triager.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/notify-triager.yml (2)
15-16: Verify the security implications of checking out external PR code
While the changes correctly use pull_request_target, be cautious when checking out external PR code. The current setup is secure because:
- It's using
pull_request_targetevent - The checkout happens after the event trigger, maintaining the security context
However, ensure that no untrusted code from the PR is executed in subsequent steps.
Line range hint 1-108: Verify workflow security and permissions
Since this workflow uses pull_request_target and handles external PRs, verify:
- No sensitive tokens are exposed to PR code
- The workflow doesn't trigger other workflows with elevated permissions
- The
GH_TOKENhas minimum required permissions
✅ Verification successful
Workflow security verification completed - No critical issues found
The security analysis of the notify-triager workflow shows:
- No other workflows are triggered by this one (no
workflow_runevents found) - The only secret used is
GH_TOKENwhich is appropriate for the PR review assignment task - The workflow follows security best practices:
- Uses
actions/checkoutwith explicit ref and repository parameters - Only performs read operations on PR content
- Token usage is limited to GitHub API calls for reviewer assignment
- No code from the PR is executed, only git commands for metadata extraction
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other workflows that might be triggered by this one
find .github/workflows -type f -name "*.yml" -exec grep -l "workflow_run" {} \;
# Check for sensitive token usage
grep -r "secrets" .github/workflows/
Length of output: 8130
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3477 +/- ##
=======================================
Coverage 86.59% 86.59%
=======================================
Files 21 21
Lines 664 664
=======================================
Hits 575 575
Misses 89 89 ☔ View full report in Codecov by Sentry. |
anshgoyalevil
changed the title
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3477--asyncapi-website.netlify.app/ |
|
/rtm |
|
@asyncapi/bounty_team |
#3460
Summary by CodeRabbit
New Features
Bug Fixes