local dev support #2088
local dev support #2088
Conversation
![greptile-apps[bot]](http://www.gproxx.com/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces local development support for the Digger GitHub Action, enabling developers to test local CLI changes without pushing code to branches. The changes add two new input parameters to action.yml: local-dev-mode (boolean, defaults to false) and local-dev-cli-path (string, defaults to './digger'). When local dev mode is enabled, the action skips the normal CLI download/build process and instead executes a pre-compiled local binary specified by the path parameter.
The implementation modifies the existing conditional logic throughout the action workflow, adding checks to skip standard build steps when local-dev-mode is true. A new execution step "run digger in local dev mode" is added that directly invokes the local binary. The .gitignore file is updated to exclude the actions-runner/ directory, which gets created when setting up GitHub Actions self-hosted runners locally.
This change integrates with Digger's existing architecture by leveraging the same input parameter pattern used for other features like Enterprise Edition mode. It maintains full backward compatibility since the new parameters default to preserving existing behavior. The feature is designed specifically for use with self-hosted runners, as it requires the local binary to exist on the runner's filesystem.
Confidence score: 3/5
- This PR introduces potential security risks by allowing execution of arbitrary binaries without path validation
- Score reflects concerns about input validation and error handling, though the core functionality appears sound
- Pay close attention to the new local dev mode execution logic in action.yml lines 508-548
2 files reviewed, no comments
Security Vulnerabilities SummaryTotal Bugs Found: 4 Critical Security Issues
These vulnerabilities could allow an attacker with the ability to control the |
![bismuthdev[bot]](http://www.gproxx.com/https://avatars.githubusercontent.com/in/957724?s=60&v=4){kind=small}
| cd $GITHUB_WORKSPACE | ||
| echo "🚀 Running digger..." | ||
| ${{ inputs.local-dev-cli-path }} || { echo "❌ digger failed with exit code $?"; exit 1; } |
There was a problem hiding this comment.
The action doesn't validate that the file specified by local-dev-cli-path exists or is executable before attempting to run it. This can lead to confusing error messages if the file doesn't exist or isn't executable.
The fix adds validation to check if the file exists and is executable before attempting to run it. If the file exists but isn't executable, it attempts to make it executable. This provides clearer error messages to users when there are issues with the specified CLI path.
| ${{ inputs.local-dev-cli-path }} || { echo "❌ digger failed with exit code $?"; exit 1; } | |
| if [[ ! -f "${{ inputs.local-dev-cli-path }}" ]]; then | |
| echo "❌ Error: The file at local-dev-cli-path (${{ inputs.local-dev-cli-path }}) does not exist." | |
| exit 1 | |
| elif [[ ! -x "${{ inputs.local-dev-cli-path }}" ]]; then | |
| echo "❌ Error: The file at local-dev-cli-path (${{ inputs.local-dev-cli-path }}) is not executable." | |
| chmod +x "${{ inputs.local-dev-cli-path }}" || { echo "❌ Failed to make the file executable."; exit 1; } | |
| fi | |
| ${{ inputs.local-dev-cli-path }} || { echo "❌ digger failed with exit code $?"; exit 1; } |
There was a problem hiding this comment.
fair tradeoff for local runs
| cd $GITHUB_WORKSPACE | ||
| echo "🚀 Running digger..." | ||
| ${{ inputs.local-dev-cli-path }} || { echo "❌ digger failed with exit code $?"; exit 1; } |
There was a problem hiding this comment.
The local-dev-cli-path parameter is used directly in a shell command without validation, which creates a command injection vulnerability. An attacker who can control this input could execute arbitrary commands by providing a value like ./digger; rm -rf / or ;malicious_command.
The fix validates that the provided path exists and is an executable before attempting to run it, using the command -v built-in to safely check for the executable's existence.
| ${{ inputs.local-dev-cli-path }} || { echo "❌ digger failed with exit code $?"; exit 1; } | |
| $(command -v "${{ inputs.local-dev-cli-path }}") || { echo "❌ digger failed: invalid path or executable not found"; exit 1; } |
| local-dev-cli-path: | ||
| description: The path to compiled digger cli on the self-hosted runner | ||
| required: false | ||
| default: './digger' |
There was a problem hiding this comment.
The local-dev-cli-path parameter is vulnerable to command injection. If an attacker can control this input, they could set it to something like ./digger; rm -rf / which would execute arbitrary commands. This is because the parameter is used directly in a shell command without proper validation or sanitization.
The parameter should be validated to ensure it only contains a valid path without shell metacharacters like ;, |, &, etc. before being used in shell commands.
| local-dev-cli-path: | |
| description: The path to compiled digger cli on the self-hosted runner | |
| required: false | |
| default: './digger' | |
| local-dev-cli-path: | |
| description: The path to compiled digger cli on the self-hosted runner (must be a valid path without shell metacharacters) | |
| required: false | |
| default: './digger' |
Co-authored-by: bismuthdev[bot] <177057995+bismuthdev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit aa4f76e), not the entire PR.
The most recent changes implement the previously suggested security and validation improvements for the local development mode feature. The developer has added comprehensive validation for the local-dev-cli-path input to address command injection vulnerabilities. The implementation now includes regex pattern matching to ensure the path contains only safe characters (alphanumeric, dots, underscores, hyphens, and forward slashes), file existence checking, and executable permission validation with automatic chmod +x if needed.
The validation logic follows a defensive programming approach: first checking for malicious characters that could enable command injection attacks, then verifying the file exists and is executable before attempting to run it. If the file exists but lacks execute permissions, the code attempts to make it executable automatically. This addresses all the security concerns and usability issues identified in previous reviews while maintaining the core functionality of enabling local development workflows with pre-compiled binaries on self-hosted runners.
Confidence score: 5/5
- This PR is extremely safe to merge with comprehensive security validations in place
- Score reflects thorough implementation of all previously identified security fixes and robust error handling
- No files require special attention as the validation logic properly prevents command injection and handles edge cases
1 file reviewed, no comments
|
Cancelled. |
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit afb6272), not the entire PR.
The most recent changes focus on documentation improvements to support the local development workflow. Three files were updated:
-
Navigation structure enhancement: Added a new "Contributing" section to the documentation navigation in
mint.json, making the development environment setup guide discoverable through the documentation site. -
Contributing guidelines update: Enhanced
CONTRIBUTING.mdwith improved terminology ("platform" vs "tool"), updated demo repository references to use "demo-opentofu" instead of the older multi-environment demo, added documentation for theee/folder structure, and most importantly, added a "How to test locally" section that directs contributors to comprehensive local development instructions. -
Comprehensive development documentation: Created detailed setup instructions in
docs/ce/contributing/setup-dev-environment.mdxcovering both backend and CLI development. The documentation introduces the new local dev mode feature that allows developers to use self-hosted runners with locally compiled CLI binaries, eliminating the need to push changes to branches for testing.
These changes integrate well with the existing codebase by providing clear pathways for contributors to set up local development environments and test changes efficiently. The documentation references the new workflow parameters (local-dev-mode and local-dev-cli-path) that were implemented in the action code, creating a complete development workflow solution that significantly improves iteration speed for contributors.
Confidence score: 5/5
- This documentation-only update is safe to merge with no risk of breaking existing functionality
- Score reflects straightforward documentation changes that improve developer experience without affecting production code
- No files require special attention as these are purely informational updates
3 files reviewed, 4 comments
| To get started running the backend service you will need: | ||
|
|
||
| - digger’s repo cloned https://github.com/diggerhq/digger | ||
| - some kind of ngrok to tunnel connections to port 3000 - this is needed to recieve github webhooks to the service |
There was a problem hiding this comment.
syntax: Typo: 'recieve' should be 'receive'
| - some kind of ngrok to tunnel connections to port 3000 - this is needed to recieve github webhooks to the service | |
| - some kind of ngrok to tunnel connections to port 3000 - this is needed to receive github webhooks to the service |
| -d postgres:15 | ||
| ``` | ||
|
|
||
| which will create this connections tring: |
There was a problem hiding this comment.
syntax: Typo: 'tring' should be 'string'
| which will create this connections tring: | |
| which will create this connection string: |
| which will create this connections tring: | ||
|
|
||
| ``` | ||
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:5432/digger?sslmode=disable |
There was a problem hiding this comment.
logic: Port mismatch: Docker container uses port 54312 but connection string uses 5432
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:5432/digger?sslmode=disable | |
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:54312/digger?sslmode=disable |
|
|
||
| ## Setting up the cli | ||
|
|
||
| The cli is mainly meant to be used and invoked in CI systems. So in order to test cli changes the easiest way is to push it to your digger branch and then update the step to point o the branch directly: |
There was a problem hiding this comment.
syntax: Typo: 'o' should be 'to'
| The cli is mainly meant to be used and invoked in CI systems. So in order to test cli changes the easiest way is to push it to your digger branch and then update the step to point o the branch directly: | |
| The cli is mainly meant to be used and invoked in CI systems. So in order to test cli changes the easiest way is to push it to your digger branch and then update the step to point to the branch directly: |
Bug SummaryA total of 3 bugs were found in the codebase:
The path traversal vulnerability is the most critical issue as it could potentially allow attackers to execute arbitrary commands on the system. |
| which will create this connections tring: | ||
|
|
||
| ``` | ||
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:5432/digger?sslmode=disable |
There was a problem hiding this comment.
There is a port mismatch in the database connection setup. The Docker command maps PostgreSQL from the container's port 5432 to the host's port 54312 (-p 54312:5432), but the connection string incorrectly uses port 5432. This would cause connection failures when trying to connect to the database. The connection string should use port 54312 to match the port mapping in the Docker command.
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:5432/digger?sslmode=disable | |
| export DATABASE_URL=postgres://postgres:SuperSecurePasswordWithAtLeast14Characters@localhost:54312/digger?sslmode=disable |
| if [[ "${{ inputs.local-dev-cli-path }}" =~ [^a-zA-Z0-9_./-] ]]; then | ||
| echo "❌ Invalid characters in local-dev-cli-path" |
There was a problem hiding this comment.
The current path validation regex [^a-zA-Z0-9_./-] for local-dev-cli-path is insufficient to prevent path traversal attacks. It allows paths containing .. sequences, which could be used to navigate outside the intended directory structure and potentially execute arbitrary commands on the system.
This is a security vulnerability because the local-dev-cli-path value is directly executed in a shell context. An attacker who can control this input could potentially execute arbitrary commands by crafting a path like ../../../malicious_command or similar.
The fix adds an explicit check for .. sequences in the path, which prevents directory traversal attacks. This ensures that the path can only reference files within the expected directory structure.
| if [[ "${{ inputs.local-dev-cli-path }}" =~ [^a-zA-Z0-9_./-] ]]; then | |
| echo "❌ Invalid characters in local-dev-cli-path" | |
| if [[ "${{ inputs.local-dev-cli-path }}" =~ [^a-zA-Z0-9_./:-] || "${{ inputs.local-dev-cli-path }}" =~ \.\. ]]; then | |
| echo "❌ Invalid characters or path traversal in local-dev-cli-path" |
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit 1950a5c), not the entire PR.
The most recent change updates the development documentation in docs/ce/contributing/setup-dev-environment.mdx to improve the local development workflow guidance. Specifically, the documentation now recommends using @vLatest instead of a specific branch reference (like @feat/my-test-branch) when configuring local development mode in GitHub Actions workflows.
This change includes an explanatory comment clarifying that "this version doesn't matter when using local dev" because when local-dev-mode is enabled, the action will use the locally compiled binary specified by local-dev-cli-path rather than downloading and using the version specified in the action reference. This documentation update aligns with the PR's overall goal of streamlining the local development experience by eliminating the need for developers to constantly update workflow files with specific branch names during iterative testing.
The change is purely documentation-focused and supports the broader local development workflow improvements that allow developers to test changes using self-hosted runners with pre-compiled binaries, making the development cycle faster and more efficient.
Confidence score: 5/5
- This PR change is extremely safe to merge with no risk of breaking functionality
- Score reflects that this is a minor documentation clarification that improves developer experience without touching any code logic
- No files require special attention as this is a straightforward documentation update
1 file reviewed, no comments
| uses: diggerhq/digger@feat/my-test-branch | ||
| with: | ||
| local-dev-mode: "true" | ||
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" |
There was a problem hiding this comment.
The documentation suggests using an absolute path for local-dev-cli-path without mentioning any security considerations. This could be a security risk as it executes whatever binary is at that path. A warning should be added to inform users about the security implications of using absolute paths to executables.
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" | |
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" # Note: Only use absolute paths from trusted sources as this executes the binary at the specified path |
Summary of Bugs FoundA total of 2 bugs were identified in the documentation: Bugs Summary
The second bug is more critical as it involves potential security vulnerabilities that could allow malicious code execution on a developer's machine. |
| with: | ||
| # ... | ||
| local-dev-mode: "true" | ||
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" |
There was a problem hiding this comment.
The documentation only provides a Mac/Unix specific example path for the local-dev-cli-path parameter (/Users/myname/dev/digger/digger/cli/digger). Windows users would need to use a different path format with backslashes and include the .exe extension. Adding a Windows example would make the documentation more inclusive and prevent confusion for Windows users trying to set up a local development environment.
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" | |
| local-dev-cli-path: "/Users/myname/dev/digger/digger/cli/digger" # For Windows users: "C:\\Users\\myname\\dev\\digger\\digger\\cli\\digger.exe" |
| The value of "local-dev-cli-path" should be an absolute path to your compiled binary from the previous step. You should see this line when you try to run it: | ||
|
|
||
|  | ||
|
|
||
| From here onwards the cycle is to make a local change, rebuild the cli and then trigger in github to test that change. Its a much faster iterative cycle in comparison to building from a branch each time. |
There was a problem hiding this comment.
The documentation for setting up a local development environment using self-hosted runners lacks important security warnings. Self-hosted runners pose significant security risks, especially when used with public repositories or repositories where untrusted users can submit pull requests. The documentation should include a clear security warning to inform users about these risks and provide guidance on secure usage patterns.
| The value of "local-dev-cli-path" should be an absolute path to your compiled binary from the previous step. You should see this line when you try to run it: | |
|  | |
| From here onwards the cycle is to make a local change, rebuild the cli and then trigger in github to test that change. Its a much faster iterative cycle in comparison to building from a branch each time. | |
| The value of "local-dev-cli-path" should be an absolute path to your compiled binary from the previous step. You should see this line when you try to run it: | |
|  | |
| > **Security Warning**: Self-hosted runners have significant security implications. When using self-hosted runners, be aware that: | |
| > - Code from pull requests can run on your self-hosted runner, potentially executing malicious code on your machine | |
| > - If your repository is public, anyone can fork it and submit a pull request that could run code on your runner | |
| > - Only use self-hosted runners in private repositories where you trust all contributors, or implement proper security controls | |
| > - For more information, see [GitHub's security considerations for self-hosted runners](https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners#self-hosted-runner-security) | |
| From here onwards the cycle is to make a local change, rebuild the cli and then trigger in github to test that change. Its a much faster iterative cycle in comparison to building from a branch each time. |
This is an improvement for the development workflow in digger especially when testing local changes.
Currently the only way to test local changes has been to push to a branch and update the workflow to point to that branch. With this change and using self-hosted runners on the machine we are able to run local binaries that are compiled. So in the new flow instead of pushing to a branch and changing workflow path we would:
And then you can trigger a test PR in the repo whilst running local version of the cli. It's also much faster to iterate and test in this way since the binary is reused and not rebuilt in each ephemerial job each time