-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update celestia-node.sh to use curl instead of wget #1816
Conversation
…curl is installed by default on new macs
merge from upstream.
…curl is installed by default on new macs
…curl is installed by default on new macs
WalkthroughThe changes in the Changes
Possibly related issues
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
public/celestia-node.sh (2)
114-117
: LGTM with a minor suggestion for error messaging.The curl command is correctly implemented with proper redirect handling and error checking. However, consider enhancing the error message to be more specific.
- echo "Download failed. Exiting." | tee -a "$LOGFILE" + echo "Failed to download celestia-node binary. Please check your internet connection and try again." | tee -a "$LOGFILE"
127-130
: Consider enhancing security by verifying checksums.txt authenticity.While the download mechanism is correct, consider adding GPG signature verification for the checksums.txt file to prevent potential tampering.
This would involve:
- Downloading the GPG signature file
- Importing the Celestia release public key
- Verifying the signature before using the checksums
Example implementation:
# Download signature curl -L "https://github.com/celestiaorg/celestia-node/releases/download/$VERSION/checksums.txt.sig" -o "checksums.txt.sig" # Import Celestia's public key (this should be done once and key fingerprint should be verified) gpg --keyserver keyserver.ubuntu.com --recv-keys <CELESTIA_PUBLIC_KEY> # Verify signature gpg --verify checksums.txt.sig checksums.txt || { echo "Checksums signature verification failed. Possible tampering detected. Exiting." | tee -a "$LOGFILE" exit 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
public/celestia-node.sh
(3 hunks)
🔇 Additional comments (1)
public/celestia-node.sh (1)
Line range hint 114-130
: Verify curl availability and behavior across platforms.
The implementation successfully replaces wget with curl, improving Mac compatibility. Let's verify the curl behavior across different platforms.
✅ Verification successful
Curl implementation verified and secure across platforms
The verification confirms:
- HTTPS protocol support is available
- Proper handling of redirects
- Correct file output handling, including spaces in filenames
- SSL/TLS support for secure downloads
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify curl behavior and version across different platforms
# Test curl availability and SSL support
curl --version | grep -i "protocols.*https"
# Test redirect following behavior
curl -sI -L "https://github.com/celestiaorg/celestia-node/releases/latest" | grep -i "location"
# Test output to file with spaces (edge case)
touch "test file.txt"
if curl -L "https://github.com/celestiaorg/celestia-node/releases/latest" -o "test file.txt" 2>/dev/null; then
echo "Curl handles filenames with spaces correctly"
rm "test file.txt"
fi
Length of output: 595
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolves #1816
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 Celestia Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
This update modifies celestia-node.sh to use curl instead of wget as curl is installed by default on new macs
Summary by CodeRabbit
curl
for improved reliability.