Allow TestDriver to run on outside contributions #1622
Conversation
WalkthroughThe pull request introduces modifications to three GitHub Actions workflow configuration files. In 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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
🔭 Outside diff range comments (1)
.github/workflows/testdriver.yml (1)
Line range hint
82-157: Critical security concerns in artifact handlingThe current artifact handling implementation has several security risks:
- Direct execution of downloaded artifacts without validation
- Potential path traversal vulnerabilities
- Insufficient error handling for network issues
Consider implementing these security improvements:
- Add checksum validation:
# After download, before execution $expectedHash = "..." # Get from a secure source $fileHash = Get-FileHash -Path $artifactFilePath -Algorithm SHA256 if ($fileHash.Hash -ne $expectedHash) { Write-Error "Artifact validation failed" exit 1 }
- Sanitize file paths:
# Before using any file paths $downloadFolder = Join-Path ([System.IO.Path]::GetTempPath()) ([System.IO.Path]::GetRandomFileName())
- Add timeout and retry logic:
$maxRetries = 3 $retryCount = 0 do { try { # Download logic here break } catch { $retryCount++ if ($retryCount -eq $maxRetries) { throw } Start-Sleep -Seconds (2 * $retryCount) } } while ($retryCount -lt $maxRetries)
🧹 Nitpick comments (1)
.github/workflows/testdriver.yml (1)
Line range hint
1-157: Architectural security review for external PR handlingWhile the approach to enabling external PR testing is fundamentally sound, consider implementing these additional security measures:
- Add explicit workflow concurrency limits to prevent resource exhaustion
- Implement artifact allow-listing to restrict what can be executed
- Consider adding a manual approval step for external PR testing
Add these configurations:
# Add at root level concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true # Add job-level condition for external PRs jobs: build_and_upload: # Add this condition if: | github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository # ... rest of job config
.github/workflows/testdriver.yml
Outdated
| env: | ||
| GITHUB_TOKEN: "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance GITHUB_TOKEN security handling
While unsetting GITHUB_TOKEN is a good security practice, consider these improvements:
- Use job-level environment variable control
- Add explicit environment variable blocking for all sensitive tokens
Apply this change at the job level:
jobs:
build_and_upload:
name: Test Onboarding
runs-on: windows-latest
+ env:
+ # Explicitly unset sensitive tokens for all steps
+ GITHUB_TOKEN: ""
+ # List other sensitive tokens that should be blocked
+ ACTIONS_RUNTIME_TOKEN: ""
+ ACTIONS_ID_TOKEN_REQUEST_TOKEN: ""Then remove the individual GITHUB_TOKEN: "" settings from steps.
Also applies to: 68-68
.github/workflows/testdriver.yml
Outdated
| @@ -6,7 +6,7 @@ | |||
| - main | |||
| tags: | |||
| - "v[0-9]+.[0-9]+.[0-9]+*" | |||
| pull_request: | |||
| pull_request_target: | |||
There was a problem hiding this comment.
💡 Codebase verification
Critical security vulnerability in pull_request_target workflow
The workflow configuration has a significant security risk. After reviewing the complete workflow file, I found that it checks out and executes code from external PRs without explicitly checking out the base ref, and more importantly, it builds and runs untrusted code from PRs with access to secrets:
- The workflow uses
pull_request_targetbut doesn't specify therefin the checkout action - It proceeds to build and execute the code from the PR using
task package - It has access to sensitive secrets (
secrets.DASHCAM_APIandsecrets.GITHUB_TOKEN) - The code is not only built but also executed in the prerun script of testdriver action
This is a serious security concern as malicious code in external PRs could potentially exfiltrate secrets.
Required changes:
- Add explicit base ref checkout:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.sha }}- Move the build and test steps to a separate workflow triggered by
pull_request(without _target) - Keep only the comment/status operations in the
pull_request_targetworkflow
🔗 Analysis chain
Security considerations for pull_request_target
The switch to pull_request_target allows external PR access while maintaining workflow security. However, this trigger requires additional safeguards:
- Ensure that no user-controlled code from the PR is checked out or executed before validation
- Consider adding explicit checkout of the base ref
Add this step immediately after the checkout:
- uses: actions/checkout@v4
+ with:
+ ref: ${{ github.event.pull_request.base.sha }}Let me verify the workflow configuration and security implications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the complete workflow file content
cat .github/workflows/testdriver.yml
Length of output: 6853
Script:
#!/bin/bash
# Look for other workflow files that might be affected
fd -e yml -e yaml . .github/workflows/
Length of output: 286
Script:
#!/bin/bash
# Search for checkout actions usage across workflows
rg "actions/checkout" .github/workflows/
Length of output: 721
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.github/workflows/testdriver.yml (1)
Line range hint
29-97: Enhance security for artifact handling and executionThe current implementation has several security concerns in artifact handling and execution:
- No validation of artifact integrity
- Direct execution of downloaded executables
- No sandboxing of executed applications
Implement these security measures:
- Add checksum validation for downloaded artifacts
- Implement signature verification for executables
- Add execution policy restrictions
- Run in isolated environment
Example implementation for checksum validation:
# Add after artifact download $expectedHash = (Invoke-RestMethod -Uri "https://api.github.com/repos/${{ github.repository }}/actions/artifacts/$artifactId/hash" -Headers $headers).sha256 $actualHash = Get-FileHash -Path $artifactZipPath -Algorithm SHA256 if ($actualHash.Hash -ne $expectedHash) { Write-Error "Artifact integrity check failed" exit 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/merge-gatekeeper.yml(1 hunks).github/workflows/testdriver-build.yml(1 hunks).github/workflows/testdriver.yml(3 hunks)
🔇 Additional comments (1)
.github/workflows/merge-gatekeeper.yml (1)
26-26: Verify workflow name in ignored list
The ignored workflow name "Run TestDriver.ai" should exactly match the name of the job in the TestDriver workflow.
✅ Verification successful
Workflow name in ignored list matches exactly
The workflow name "Run TestDriver.ai" in the ignored list matches exactly with the job name defined in .github/workflows/testdriver.yml.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the workflow name matches exactly
rg "name: Run TestDriver\.ai" .github/workflows/
Length of output: 115
| - name: Build | ||
| run: task package | ||
| env: | ||
| USE_SYSTEM_FPM: true # Ensure that the installed version of FPM is used rather than the bundled one. | ||
| CSC_IDENTITY_AUTO_DISCOVERY: false # disable codesign | ||
| shell: powershell # electron-builder's Windows code signing package has some compatibility issues with pwsh, so we need to use Windows Powershell |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security for package task execution
The task package step runs with write permissions and could potentially execute malicious code from external PRs.
- Add explicit base ref checkout before this step:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.sha }}- Consider moving build steps to a separate job with minimal permissions
- Add input validation for external PR code before execution
| workflow_dispatch: null | ||
|
|
||
| env: | ||
| GO_VERSION: "1.23" |
There was a problem hiding this comment.
Invalid Go version specified
The Go version "1.23" is incorrect as it doesn't exist. The latest stable version is 1.22.
- GO_VERSION: "1.23"
+ GO_VERSION: "1.22"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GO_VERSION: "1.23" | |
| GO_VERSION: "1.22" |
|
|
||
| env: | ||
| GO_VERSION: "1.23" | ||
| NODE_VERSION: 22 |
There was a problem hiding this comment.
💡 Codebase verification
Node.js version 22 is not recommended for this project
The package.json files reveal that:
- The main project has dependencies on packages like electron v33.2.1 and other modern packages
- The docs project specifically requires Node.js >= 18.0
- The project uses advanced development tools and frameworks that are better suited for LTS versions
Using Node.js 22 (which is currently in development) could lead to compatibility issues. Node.js 20 is the current LTS version and would be more appropriate for production use.
🔗 Analysis chain
Verify Node.js version compatibility
Node.js version 22 is currently in development. Consider using the latest LTS version 20.
- NODE_VERSION: 22
+ NODE_VERSION: 20Let me verify the Node.js version compatibility by checking the project's dependencies and requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for package.json files and their Node.js version requirements
fd package.json --exec cat {}
# Check for .nvmrc or .node-version files
fd -e nvmrc -e node-version --exec cat {}
# Look for any Node.js version specifications in workflow files
fd -e yml -e yaml -p '.github/workflows' --exec cat {}
Length of output: 8150
.github/workflows/testdriver.yml
Outdated
| - uses: testdriverai/action@main | ||
| id: testdriver | ||
| env: | ||
| FORCE_COLOR: "3" | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Consolidate and secure GITHUB_TOKEN usage
The GITHUB_TOKEN is exposed in multiple places and could be leaked during execution.
- Use a dedicated environment variable for the token
- Implement token rotation for artifact downloads
- Add token validation before usage
env:
+ ARTIFACT_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}Then update all token usages to reference ARTIFACT_TOKEN.
Also applies to: 50-50
|
Hey @esimkowitz I think as Ian said, gh sometimes passes empty secrets. |
|
It shouldn't, since I switched the trigger type. Though is that token really necessary? The artifacts should be public. If you have your own token on your end that would be better to use so we don't risk leaking secrets. Let's discuss later, we should really be using GitHub Apps to manage your workflow's access to our repo. Happy to show you how we do it for our own workflows. |
|
Ofc, always open to learn. |
Discussed in #1621, the current setup for the TestDriver workflow doesn't allow it to run for PRs from outside contributors, since GitHub restricts secrets access for external PRs using the
pull_requesttrigger.This PR splits the TestDriver workflow into two separate workflows, a
pull_request-triggered one which builds the artifact, and aworkflow_run-triggered one which can access our secrets to upload the job to TestDriver.resolves #1621