ci(prereleaser): verify user membership for running workflow #585
Conversation
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
WalkthroughThe pull request modifies the GitHub Actions workflow in Changes
Sequence DiagramsequenceDiagram
participant Trigger as Workflow Trigger
participant Verify as Verify Job
participant GitHub as GitHub API
participant Charts as Charts Job
Trigger->>Verify: Initiate workflow
Verify->>GitHub: Check user membership
alt User is authorized
GitHub-->>Verify: Membership confirmed
Verify->>Charts: Proceed with job
else User is unauthorized
GitHub-->>Verify: Membership denied
Verify-->>Trigger: Workflow fails
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/prereleaser.yaml (1)
21-31: LGTM! Secure token generation implementation.The implementation correctly uses GitHub App token for authentication, which is a security best practice as it provides limited, scoped access compared to personal access tokens.
Ensure the GitHub App has minimal required permissions set in the app settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/prereleaser.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/prereleaser.yaml
32-32: shellcheck reported issue in this script: SC2036:warning:13:1: If you wanted to assign the output of the pipeline, use a=$(b | c)
(shellcheck)
32-32: shellcheck reported issue in this script: SC2030:info:13:1: Modification of gh_response is local (to subshell caused by pipeline)
(shellcheck)
32-32: shellcheck reported issue in this script: SC2031:info:18:8: gh_response was modified in a subshell. That change might be lost
(shellcheck)
🔇 Additional comments (1)
.github/workflows/prereleaser.yaml (1)
57-57: LGTM! Proper job dependency.
The dependency on the verify job ensures that authorization checks complete before proceeding with the release process.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/prereleaser.yaml (1)
47-50:⚠️ Potential issueSecurity bypass: Remove repository_dispatch skip condition
Skipping the verification for repository_dispatch events creates a security bypass path. This is particularly concerning as it allows circumventing the team membership check.
- if [[ "${{ github.event.name }}" == "repository_dispatch" ]]; then - echo "skipping, event is triggered from other repository workflow" - exit 0 - fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/prereleaser.yaml(1 hunks)
🔇 Additional comments (3)
.github/workflows/prereleaser.yaml (3)
21-23: LGTM: Job definition follows best practicesThe verify job is properly defined with a standard runner configuration.
51-55: Fix team name inconsistencyThe team name "releaser" is used here but the PR description mentions "releasers". Verify the correct team name to avoid authentication failures.
✅ Verification successful
Let me gather more information to verify the team name in the repository.
Team name "releaser" is correct
Based on the search results, the team name "releaser" (singular) is consistently used within the codebase, particularly in the workflow files:
.github/workflows/prereleaser.yaml: Uses "releaser" in the team verification step- The workflow also references
signoz/primus.workflows/.github/workflows/releaser.yamlThe PR description's mention of "releasers" appears to be incorrect, as the codebase consistently uses the singular form "releaser".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct team name # Check team name usage across workflows echo "Checking team name usage:" rg -i "releaser[s]?\b" .github/workflows/Length of output: 610
Script:
#!/bin/bash # Check team name in other configuration files and documentation echo "Checking team name in other files:" rg -i "releaser[s]?\b" -g '!.github/workflows/*' --type yaml --type md # Check Makefile for the github-verify-team-membership target echo -e "\nChecking Makefile for team verification:" rg "github-verify-team-membership" -A 5Length of output: 288
24-41: Verify GitHub App token permissionsThe implementation uses a GitHub App token for authentication, which is a security best practice. However, we should verify that the GitHub App has the minimum required permissions.
✅ Verification successful
Let me gather more information about how this token is being used in the workflow.
GitHub App token usage is appropriate and well-scoped
The GitHub App token is used appropriately with minimal scope:
- Token is only used for checking out the
signoz/primusrepository- Verifies releaser team membership when manually triggered
- Token is passed to a reusable workflow for charts release
The implementation follows security best practices:
- Uses GitHub App authentication instead of personal access tokens
- Token is scoped to the repository owner
- Token is generated per workflow run
- Token permissions are implicitly limited to the required operations (repo access)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check GitHub App permissions in the repository # Check for workflow files referencing the GitHub App echo "Checking for GitHub App usage:" rg -l "PRIMUS_APP_ID|PRIMUS_PRIVATE_KEY" .github/workflows/ # Check for other potential token usages echo -e "\nChecking for other token usages:" rg "create-github-app-token" .github/workflows/Length of output: 380
Script:
#!/bin/bash # Check the complete workflow file to understand token usage scope echo "Checking prereleaser.yaml workflow:" cat .github/workflows/prereleaser.yaml # Check if there are any other workflows that might be related echo -e "\nChecking related workflows:" fd -e yaml -e yml . .github/workflows/ --exec head -n 1 {}Length of output: 2327
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
Signed-off-by: Prashant Shahi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/prereleaser.yaml (1)
Line range hint
1-38: Comprehensive security review neededBased on past review comments and current analysis, several security aspects need attention:
- Verify that
release.yamlhas similar team membership checks- Define minimum required permissions for the GitHub App token
- Implement proper security checks for all event types
- Consider implementing additional security measures:
- Branch protection rules
- Required reviewers
- Environment protection rules
Would you like me to help create a comprehensive security model that addresses these concerns?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/prereleaser.yaml(1 hunks)
🔇 Additional comments (1)
.github/workflows/prereleaser.yaml (1)
29-32:⚠️ Potential issueRemove security bypass and improve stability in charts job
Critical security and stability issues:
- The condition
|| github.event.name == 'repository_dispatch'creates a security bypass, allowing unverified users to trigger releases through repository_dispatch events.- Using
@mainreference is unstable.secrets: inheritviolates the principle of least privilege.Apply this diff to fix these issues:
- if: ${{ always() && (needs.verify.result == 'success' || github.event.name == 'repository_dispatch') }} - uses: signoz/primus.workflows/.github/workflows/releaser.yaml@main - secrets: inherit + if: ${{ needs.verify.result == 'success' }} + uses: signoz/primus.workflows/.github/workflows/[email protected] + secrets: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + NPM_TOKEN: ${{ secrets.NPM_TOKEN }} needs:Let's verify the impact of repository_dispatch events:
Signed-off-by: Prashant Shahi <[email protected]>
CI
Summary by CodeRabbit