build: fix docker-compose for evm #3022
Conversation
Summary of ChangesHello @randygrok, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue preventing the EVM Docker Compose setup from functioning correctly. The core changes involve adapting the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @randygrok's task in 1m 31s —— View job Code Review Complete ✅I've reviewed the changes in Summary of ChangesThis PR makes three key improvements:
Positive Observations ✨Security Improvements
Code Quality
Issues and Recommendations🔴 Critical: Command Injection VulnerabilityLocation: Issue: Unquoted variable expansion in command execution creates command injection risk: $INIT_COMMAND # Line 63 - UNSAFE
$START_COMMAND "$@" # Line 123 - UNSAFEProblem: If any environment variable contains special characters (spaces, semicolons, etc.), the shell will interpret them as command separators, potentially executing arbitrary code. Fix: Quote the variable expansions: "$INIT_COMMAND" # Line 63
"$START_COMMAND" "$@" # Line 123Or better yet, use direct command execution instead of building command strings: # Replace lines 60-63 with:
echo "Create default config with command: evm init $init_flags"
exec evm init $init_flags
# Replace lines 120-123 with:
echo "Starting with command: evm start $default_flags $*"
exec evm start $default_flags "$@"Fix command injection vulnerability → 🟡 Medium: Security - Secret File CleanupLocation: Issue: Secret files are created but never cleaned up, persisting sensitive data on disk. Recommendation: Add a trap to clean up secret files on script exit: # After line 34
trap 'rm -f "$PASSPHRASE_FILE" "$JWT_SECRET_FILE"' EXIT INT TERM
# Or better: use /dev/shm for truly temporary secrets
PASSPHRASE_FILE="${TMPDIR:-/tmp}/passphrase-$$.txt"
JWT_SECRET_FILE="${TMPDIR:-/tmp}/jwt-$$hex"Consideration: If these files need to persist between container restarts, document this requirement. Otherwise, ephemeral storage would be more secure. 🟡 Medium: Error Handling - Silent FailuresLocation: Throughout the script Issue: The script uses
Recommendation: Add explicit error checking for critical operations: # Replace line 34 with:
mkdir -p "$CONFIG_HOME" || {
echo "ERROR: Failed to create config directory: $CONFIG_HOME" >&2
exit 1
}
# Add checks after secret file creation:
if [ -n "$EVM_SIGNER_PASSPHRASE" ]; then
echo "$EVM_SIGNER_PASSPHRASE" > "$PASSPHRASE_FILE" || {
echo "ERROR: Failed to write passphrase file" >&2
exit 1
}
chmod 600 "$PASSPHRASE_FILE" || {
echo "ERROR: Failed to set permissions on passphrase file" >&2
exit 1
}
fi🟡 Medium: Consistency - Flag FormatLocation: Issue: Inconsistent flag format - some use
Recommendation: Choose one format and use it consistently. Based on the other flags in the script, space separation appears to be the standard: --evnode.node.aggregator true --evnode.signer.passphrase_file "$PASSPHRASE_FILE"Or use equals consistently: --evnode.node.aggregator=true --evnode.signer.passphrase_file="$PASSPHRASE_FILE"🔵 Minor: Code Quality - Variable QuotingLocation: Multiple locations throughout Issue: Inconsistent variable quoting. Some variables are quoted, others aren't:
Recommendation: Always quote variable expansions to prevent word splitting: --evnode.signer.passphrase_file "$PASSPHRASE_FILE"🔵 Minor: DocumentationIssue: The script lacks comments explaining the security improvements. Recommendation: Add comments to document the design decisions: # Create secret files with restricted permissions instead of passing
# secrets as CLI arguments to prevent exposure in process listings
PASSPHRASE_FILE="$CONFIG_HOME/passphrase.txt"
if [ -n "$EVM_SIGNER_PASSPHRASE" ]; then
echo "$EVM_SIGNER_PASSPHRASE" > "$PASSPHRASE_FILE"
chmod 600 "$PASSPHRASE_FILE" # Readable only by owner
fiTesting RecommendationsSince this changes core initialization logic, please verify:
Decision: Approve with Recommendations✅ The flag migration is correct and necessary - this unblocks the docker-compose setup. The other issues (secret cleanup, error handling, consistency) are improvements that can be addressed in follow-up PRs if needed, but I'd recommend fixing the command injection issue in this PR. Verification Checklist
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the entrypoint.sh script to align with recent command-line flag changes, specifically replacing rollkit. with evnode., and enhances secret handling by reading them from environment variables and writing to files. While this improves security by preventing secrets from being exposed in the process list, a medium-severity argument injection vulnerability was identified in entrypoint.sh. This is due to unsafe command construction where variables are concatenated without proper quoting, leading to potential word splitting and unintended argument injection. This underlying issue should be addressed. Additionally, minor suggestions have been made to improve the robustness of writing secrets to files.
| # Create passphrase file if environment variable is set | ||
| PASSPHRASE_FILE="$CONFIG_HOME/passphrase.txt" | ||
| if [ -n "$EVM_SIGNER_PASSPHRASE" ]; then | ||
| echo "$EVM_SIGNER_PASSPHRASE" > "$PASSPHRASE_FILE" |
There was a problem hiding this comment.
Using echo can add an unwanted trailing newline to the passphrase file. While the Go application appears to handle this by trimming whitespace, it's safer and more precise to use printf '%s' to ensure the secret is written to the file exactly as it is in the environment variable, without any modifications.
| echo "$EVM_SIGNER_PASSPHRASE" > "$PASSPHRASE_FILE" | |
| printf '%s' "$EVM_SIGNER_PASSPHRASE" > "$PASSPHRASE_FILE" |
| # Create JWT secret file if environment variable is set | ||
| JWT_SECRET_FILE="$CONFIG_HOME/jwt.hex" | ||
| if [ -n "$EVM_JWT_SECRET" ]; then | ||
| echo "$EVM_JWT_SECRET" > "$JWT_SECRET_FILE" |
There was a problem hiding this comment.
Using echo can add an unwanted trailing newline to the JWT secret file. It's safer and more precise to use printf '%s' to ensure the secret is written to the file exactly as it is in the environment variable, without any modifications.
| echo "$EVM_JWT_SECRET" > "$JWT_SECRET_FILE" | |
| printf '%s' "$EVM_JWT_SECRET" > "$JWT_SECRET_FILE" |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3022 +/- ##
==========================================
- Coverage 58.04% 55.61% -2.43%
==========================================
Files 110 116 +6
Lines 10522 11477 +955
==========================================
+ Hits 6107 6383 +276
- Misses 3764 4394 +630
- Partials 651 700 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
julienrbrt
changed the title
some flags have changed and the docker compose for the evm was failing. Now it works again.