feat: Add CLI args and Caddy HTTPS support to deploy script #242
Conversation
Add command-line argument parsing for non-interactive/CI/CD deployments: - Support --admin-token, --port, --branch, --db-password, --app-url options - Support --deploy-dir for custom deployment directory - Support -y/--non-interactive for fully automated deployments - Environment variable fallbacks (CCH_ADMIN_TOKEN, CCH_PORT, etc.) Add optional Caddy reverse proxy for automatic HTTPS: - Enable with --domain/-D parameter - Auto-generates Caddyfile with security headers - Supports --email for Let's Encrypt notifications - Supports custom SSL certificates via --ssl-cert/--ssl-key - Port 80/443 availability checking 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @ding113, 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 significantly upgrades the one-click deployment script by introducing robust command-line argument parsing, making it suitable for automated environments, and by adding an optional Caddy reverse proxy for seamless, automatic HTTPS configuration. These enhancements streamline the deployment process and improve security. Highlights
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
|
![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 significantly enhances the deployment script by adding command-line argument parsing for non-interactive use and integrating Caddy for automatic HTTPS. The changes are well-structured and improve the script's utility for automated environments. My review identifies a few areas for improvement to increase the script's robustness and portability, specifically regarding argument parsing and compatibility with different shell environments and operating systems.
| -t|--admin-token) | ||
| ADMIN_TOKEN="$2" | ||
| shift 2 | ||
| ;; |
There was a problem hiding this comment.
The current argument parsing logic is fragile. If an option that requires a value (like --admin-token) is provided without one, or if the value provided is actually another option (e.g., ./deploy.sh -t -y), the script will misbehave. It might consume the next option as a value or fail with a shift error. It's crucial to validate that an argument is provided and that it's not another option.
This same logic should be applied to all other options that expect a value (e.g., -p, -b, --db-password).
| -t|--admin-token) | |
| ADMIN_TOKEN="$2" | |
| shift 2 | |
| ;; | |
| -t|--admin-token) | |
| if [[ -z "$2" || "$2" =~ ^- ]]; then | |
| log_error "Option '$1' requires an argument." | |
| exit 1 | |
| fi | |
| ADMIN_TOKEN="$2" | |
| shift 2 | |
| ;; |
| if ss -tuln | grep -q ":${port} "; then | ||
| log_warning "Port $port is already in use" | ||
| port_in_use=true | ||
| fi | ||
| elif command -v netstat &> /dev/null; then | ||
| if netstat -tuln | grep -q ":${port} "; then |
There was a problem hiding this comment.
The grep pattern ":${port} " used to check for open ports is not fully portable. It assumes a space character follows the port number in the output of ss and netstat. While this might be true on some Linux distributions, it can fail on others or on different operating systems like macOS, where the spacing might differ. This could lead to the script failing to detect a port that is already in use.
A more robust solution is to use a regular expression that matches the port followed by any whitespace character or the end of the line.
| if ss -tuln | grep -q ":${port} "; then | |
| log_warning "Port $port is already in use" | |
| port_in_use=true | |
| fi | |
| elif command -v netstat &> /dev/null; then | |
| if netstat -tuln | grep -q ":${port} "; then | |
| if ss -tuln | grep -qE ":${port}(\s|$)"; then | |
| log_warning "Port $port is already in use" | |
| port_in_use=true | |
| fi | |
| elif command -v netstat &> /dev/null; then | |
| if netstat -tuln | grep -qE ":${port}(\s|$)"; then |
| log_warning "Ports 80 and/or 443 are in use. Caddy may not start correctly." | ||
| if [[ "$NON_INTERACTIVE" != true ]]; then | ||
| read -p "Continue anyway? [y/N]: " confirm | ||
| if [[ "${confirm,,}" != "y" ]]; then |
There was a problem hiding this comment.
The ${confirm,,} syntax for converting a string to lowercase is a feature of Bash 4.0 and newer. Some common environments, notably the default bash on macOS (which is often version 3.2), do not support this. This will cause a "bad substitution" error and break the script on such systems. For better portability, it's recommended to use a more widely supported command like tr.
| if [[ "${confirm,,}" != "y" ]]; then | |
| if [[ "$(echo "$confirm" | tr '[:upper:]' '[:lower:]')" != "y" ]]; then |
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR adds valuable CLI argument parsing and Caddy HTTPS support to the deployment script. However, there are several input validation issues that should be addressed to prevent runtime failures and improve security.
🔍 Issues Found
- Critical (🔴): 0 issues
- High (🟠): 3 issues
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
- Add port number validation (line ~133) - invalid ports will cause Docker failures
- Add SSL certificate file existence checks (line ~192) - missing files cause confusing errors
- Add domain format validation (line ~157) - malformed domains break Caddy config
- Set restrictive permissions on copied SSL private keys (line ~508)
- Add email format validation for Let's Encrypt (line ~161)
💡 General Observations
The implementation pattern is good overall, but input validation is consistently missing across multiple CLI arguments. Consider adding a validation helper function that can be reused.
🤖 Automated review by Claude AI - focused on identifying issues for improvement
| ;; | ||
| -b|--branch) | ||
| BRANCH_NAME="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🟠 High: Missing port number validation
Why this is a problem: The APP_PORT value from CLI args is used directly without validation. Invalid port values (e.g., negative numbers, non-numeric strings, or privileged ports) could cause Docker to fail at runtime or create security issues.
Suggested fix:
-p|--port)
APP_PORT="$2"
# Validate port number
if ! [[ "$APP_PORT" =~ ^[0-9]+$ ]] || [ "$APP_PORT" -lt 1 ] || [ "$APP_PORT" -gt 65535 ]; then
log_error "Invalid port: $APP_PORT. Must be a number between 1 and 65535"
exit 1
fi
shift 2
;;| case "$BRANCH_NAME" in | ||
| main) | ||
| IMAGE_TAG="latest" | ||
| ;; |
There was a problem hiding this comment.
🟠 High: Missing validation for SSL certificate file existence
Why this is a problem: When custom SSL certificates are provided via --ssl-cert and --ssl-key, the script validates that both are provided together but doesn't check if the files actually exist and are readable. This will cause a confusing error later during cp operations.
Suggested fix:
# Validate SSL certificate files exist and are readable
if [[ -n "$CUSTOM_SSL_CERT" ]]; then
if [[ ! -f "$CUSTOM_SSL_CERT" ]]; then
log_error "SSL certificate file not found: $CUSTOM_SSL_CERT"
exit 1
fi
if [[ ! -r "$CUSTOM_SSL_CERT" ]]; then
log_error "SSL certificate file not readable: $CUSTOM_SSL_CERT"
exit 1
fi
fi
if [[ -n "$CUSTOM_SSL_KEY" ]]; then
if [[ ! -f "$CUSTOM_SSL_KEY" ]]; then
log_error "SSL key file not found: $CUSTOM_SSL_KEY"
exit 1
fi
if [[ ! -r "$CUSTOM_SSL_KEY" ]]; then
log_error "SSL key file not readable: $CUSTOM_SSL_KEY"
exit 1
fi
fi| ;; | ||
| --email) | ||
| CADDY_EMAIL="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🟡 Medium: Missing domain format validation
Why this is a problem: The DOMAIN parameter is used directly in Caddyfile generation and URL construction without validation. Malformed domain names (containing spaces, special characters, or shell metacharacters) could cause unexpected behavior or Caddy configuration errors.
Suggested fix:
-D|--domain)
DOMAIN="$2"
# Basic domain format validation
if ! [[ "$DOMAIN" =~ ^[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?)*$ ]]; then
log_error "Invalid domain format: $DOMAIN"
exit 1
fi
shift 2
;;| local app_ports_config | ||
| if [[ -n "$DOMAIN" ]]; then | ||
| # When using Caddy, don't expose app port externally | ||
| app_ports_config="" |
There was a problem hiding this comment.
🟠 High: SSL private key copied without restrictive permissions
Why this is a problem: The SSL private key is copied to the deployment directory without setting restrictive file permissions. Private keys should have mode 600 (owner read/write only) to prevent unauthorized access.
Suggested fix:
# Copy custom SSL certificates if provided
if [[ -n "$CUSTOM_SSL_CERT" ]] && [[ -n "$CUSTOM_SSL_KEY" ]]; then
mkdir -p "$DEPLOY_DIR/certs"
cp "$CUSTOM_SSL_CERT" "$DEPLOY_DIR/certs/cert.pem"
cp "$CUSTOM_SSL_KEY" "$DEPLOY_DIR/certs/key.pem"
# Set restrictive permissions on private key
chmod 600 "$DEPLOY_DIR/certs/key.pem"
chmod 644 "$DEPLOY_DIR/certs/cert.pem"
log_success "Custom SSL certificates copied"
fi| ;; | ||
| --ssl-cert) | ||
| CUSTOM_SSL_CERT="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🟡 Medium: Missing email format validation for Let's Encrypt
Why this is a problem: The --email parameter is used for Let's Encrypt certificate notifications but is not validated. An invalid email format could cause Let's Encrypt registration to fail silently or produce confusing error messages.
Suggested fix:
--email)
CADDY_EMAIL="$2"
# Basic email format validation
if ! [[ "$CADDY_EMAIL" =~ ^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$ ]]; then
log_error "Invalid email format: $CADDY_EMAIL"
exit 1
fi
shift 2
;;
github-project-automation
bot
moved this from In progress
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Closes #240
This PR enhances the one-click deployment script with:
Changes
Phase 1: CLI Arguments Support
parse_args()function with full argument parsing--admin-token,--port,--branch,--db-password,--app-url,--deploy-dir-y/--non-interactiveflag for fully automated deploymentsCCH_ADMIN_TOKEN,CCH_PORT, etc.)--helpand--versioncommandsselect_branch()to skip prompts in non-interactive modePhase 2: Caddy HTTPS Integration
--domain/-Dparameter to enable Caddy reverse proxyCaddyfilewith security headers (HSTS, X-Content-Type-Options, etc.)--emailfor Let's Encrypt certificate notifications--ssl-cert/--ssl-keycheck_ports()function for port 80/443 availability checkingdocker-compose.yamlgeneration to include Caddy serviceUsage Examples
Test Plan
--helpdisplays usage information--versiondisplays version🤖 Generated with Claude Code