fix: restrict markdown tests to ubuntu runner #3318
Conversation
anshgoyalevil
requested review from
derberg,
akshatnema,
magicmatatjahu,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request modifies the GitHub Actions workflow configuration for Node.js pull request testing. It changes the event trigger from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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 (
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3318--asyncapi-website.netlify.app/ |
![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 and nitpick comments (2)
.github/workflows/if-nodejs-pr-testing.yml (2)
Line range hint
90-112: LGTM: Well-structured comment handling with proper OS restrictionsThe comment handling is properly restricted to Ubuntu and uses pinned action versions for security. The comment format is clear and helpful.
Consider adding a timestamp to the comment to help track when the check was performed:
message: | ### Markdown Check Results + _Generated at: ${{ github.event.pull_request.updated_at }}_ We found issues in the following markdown files:
Line range hint
12-17: Consider optimizing the workflow structureCurrently, the workflow runs most steps on all OS versions, but markdown-specific steps only run on Ubuntu. Consider splitting this into two jobs:
- A markdown-specific job that only runs on Ubuntu
- A general testing job that runs on all OS versions
This would reduce unnecessary job executions and make the workflow more maintainable.
Here's a suggested structure:
jobs: markdown-checks: runs-on: ubuntu-latest steps: # Markdown-specific steps here test-nodejs-pr: runs-on: ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest, macos-13, windows-latest] steps: # General testing steps here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/if-nodejs-pr-testing.yml (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/if-nodejs-pr-testing.yml (2)
Line range hint
79-85: LGTM: Appropriate OS restriction for markdown checksThe restriction to Ubuntu is well-justified as it relies on Unix-specific
sedcommand. The error capture logic is implemented correctly.🧰 Tools
🪛 actionlint
82-82: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:2:32: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:3:19: Double quote to prevent globbing and word splitting
(shellcheck)
82-82: shellcheck reported issue in this script: SC2086:info:4:15: Double quote to prevent globbing and word splitting
(shellcheck)
5-7:⚠️ Potential issueSecurity Warning: Validate the use of pull_request_target
The change from
pull_requesttopull_request_targetgrants workflows access to repository secrets and write permissions. This could be dangerous if the workflow executes code from the PR context, as it might expose sensitive data to malicious pull requests.Let's verify if any steps execute untrusted code:
Consider these alternatives:
- Keep using
pull_requestif write permissions aren't needed- If write permissions are required, ensure no code from the PR is executed before checking its contents
✅ Verification successful
Let's gather more specific information about the workflow file to understand how it handles PR content.
This will help us understand:
- The complete workflow context
- Whether the workflow executes any untrusted code from PR
- What security measures, if any, are in place before code execution
The previous results showed various files containing npm/node commands, but we need to specifically focus on what's being executed in this workflow file and how it handles PR content.
After analyzing the workflow file, I can now provide a final response:
Security Warning: pull_request_target usage is safe in this context
The use of
pull_request_targetin this workflow is actually safe because:
The workflow has proper security checks in place:
- It first validates if the PR is from trusted bots (asyncapi-bot, asyncapi-bot-eve, allcontributors[bot])
- All code execution steps are conditional on
steps.should_run.outputs.shouldrun == 'true'- Package installation is done via
npm ciwhich uses package-lock.json from the base repository- All npm commands use the
--if-presentflag, ensuring they only run if defined in the base repository's package.jsonThe workflow doesn't execute any untrusted code from the PR context:
- The checkout action runs after initial validation
- All npm scripts (test, lint, generate:assets) are from the base repository's package.json
- The markdown checks are only run on Ubuntu and use built-in commands (sed)
The
pull_request_targetis appropriately used here to enable PR comments with repository tokens, which is a valid use case.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential code execution from PR context rg -l "npm run|npm test|node .*\$\{"Length of output: 398
Script:
#!/bin/bash # Check the complete workflow file content cat .github/workflows/if-nodejs-pr-testing.yml # Also search for any npm or node commands specifically in workflow files rg "npm|node" .github/workflows/Length of output: 24710
anshgoyalevil
changed the base branch from
master
to
update-cli-docs-f2d8fb01141b95baffc8dfddf9d1ae6ed3f4faaa
anshgoyalevil
changed the base branch from
update-cli-docs-f2d8fb01141b95baffc8dfddf9d1ae6ed3f4faaa
to
master
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
anshgoyalevil
removed request for
quetzalliwrites,
VaishnaviNandakumar,
J0SAL,
BhaswatiRoy and
TRohit20
This PR restricts markdown tests to Ubuntu Runner since we cannot make use of commands like sed in Windows bash.
Summary by CodeRabbit