fix: Address security vulnerabilities and configuration issues from PR #3 review

cli/lib/do_k8s.sh

@@ -14,7 +14,49 @@ else
 fi
 
 if [ -n "$ENV_FILE" ]; then
-	export $(grep -v '^#' "$ENV_FILE" | xargs)
+	# Safely load environment variables from the .env file without executing it as shell.
+	# Only accept simple KEY=VALUE lines, ignore comments and malformed entries.
+	while IFS= read -r line || [ -n "$line" ]; do
+		# Trim leading and trailing whitespace using parameter expansion
+		# ${var##*[![:space:]]} finds the last non-whitespace char, then % removes trailing whitespace
+		# ${var%%[![:space:]]*} finds the first non-whitespace char, then # removes leading whitespace
+		line="${line#"${line%%[![:space:]]*}"}"
+		line="${line%"${line##*[![:space:]]}"}"
+
+		# Skip empty lines and comments
+		if [ -z "$line" ] || [ "${line#\#}" != "$line" ]; then
+			continue
+		fi
+
+		# Require KEY=VALUE form with a safe variable name
+		case "$line" in
+			[A-Za-z_][A-Za-z0-9_]*=*)
+				key=${line%%=*}
+				value=${line#*=}
+
+				# Strip trailing carriage return (Windows CRLF line endings)
+				value="${value%$'\r'}"
+
+				# Strip matching surrounding quotes (both double or both single)
+				# Only strip if quote appears at both start AND end
+				case "$value" in
+					\"*\")
+						value="${value#\"}"
+						value="${value%\"}"
+						;;
+					\'*\')
+						value="${value#\'}"
+						value="${value%\'}"
+						;;
+				esac
+
+				export "$key=$value"
+				;;
+			*)
+				# Ignore lines that are not simple KEY=VALUE assignments
+				;;
+		esac
+	done < "$ENV_FILE"
 fi
 
 check_doctl() {
@@ -41,7 +83,7 @@ check_doctl() {
 ensure_cluster() {
     local cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}}
     local region=${2:-${DO_REGION:-nyc3}}
-    local version="latest"
+    local version=${K8S_VERSION:-"1.33.1-do.0"}
 
     log_info "Checking for cluster: $cluster_name..."
 
@@ -83,18 +125,56 @@ scale_node_pool() {
 }
 
 create_node_pool() {
-    local cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}}
-    local pool_name=$2
-    local size=$3
-    local count=$4
-    local tags=$5 # Optional setup as "role=value"
+    # cluster_name is optional:
+    #   - 3 args  => <pool_name> <size> <count>   (cluster_name from $CLUSTER_NAME or 'weown-cluster')
+    #   - 4+ args => <cluster_name> <pool_name> <size> <count> [label]
+    local cluster_name
+    local pool_name
+    local size
+    local count
+    local tags # Optional setup as "role=value"
+
+    if [ "$#" -eq 3 ]; then
+        # Form: create_node_pool <pool_name> <size> <count>
+        cluster_name=${CLUSTER_NAME:-weown-cluster}
+        pool_name=$1
+        size=$2
+        count=$3
+        tags=
+    elif [ "$#" -ge 4 ]; then
+        # Form: create_node_pool <cluster_name> <pool_name> <size> <count> [label]
+        cluster_name=${1:-${CLUSTER_NAME:-weown-cluster}}
+        pool_name=$2
+        size=$3
+        count=$4
+        tags=$5
+    else
+        log_error "Usage:"
+        log_error "  create_node_pool <pool_name> <size> <count>"
+        log_error "  create_node_pool <cluster_name> <pool_name> <size> <count> [label]"
+        log_error "    (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)"
+        return 1
+    fi
+
+    if [ -z "$pool_name" ] || [ -z "$size" ] || [ -z "$count" ]; then
+        log_error "Usage:"
+        log_error "  create_node_pool <pool_name> <size> <count>"
+        log_error "  create_node_pool <cluster_name> <pool_name> <size> <count> [label]"
+        log_error "    (cluster_name defaults to \$CLUSTER_NAME or 'weown-cluster' if not provided)"
+        return 1
+    fi
 
     log_info "Creating node pool '$pool_name'..."
-    local cmd="doctl kubernetes cluster node-pool create $cluster_name --name $pool_name --size $size --count $count"
+    local args=(
+        "kubernetes" "cluster" "node-pool" "create" "$cluster_name"
+        "--name" "$pool_name"
+        "--size" "$size"
+        "--count" "$count"
+    )
     if [ -n "$tags" ]; then
-        cmd="$cmd --label $tags"
+        args+=("--label" "$tags")
     fi
-    eval $cmd
+    doctl "${args[@]}"
 }
 
 delete_node_pool() {

cli/lib/helm_utils.sh

@@ -24,14 +24,13 @@ deploy_chart() {
     # Create namespace if needed
     kubectl create namespace "$namespace" --dry-run=client -o yaml | kubectl apply -f -
 
-    local cmd="helm upgrade --install $release_name $chart_path --namespace $namespace"
-
+    # Execute helm upgrade/install with proper argument quoting, avoiding eval
+    local cmd=(helm upgrade --install "$release_name" "$chart_path" --namespace "$namespace")
     if [ -f "$values_file" ]; then
-        cmd="$cmd -f $values_file"
+        cmd+=(-f "$values_file")
     fi
 
-    # Execute
-    if eval $cmd; then
+    if "${cmd[@]}"; then
         log_success "Deployed $release_name successfully."
     else
         log_error "Failed to deploy $release_name."

cli/lib/stacks.sh

@@ -6,10 +6,10 @@ source "$(dirname "${BASH_SOURCE[0]}")/do_k8s.sh"
 # Define available stacks/apps
 # Format: "DisplayName|ReleaseName|Description|ChartPath|Namespace|ValuesFile"
 APPS=(
-    "Infra: Nginx Ingress|ingress-nginx|Core ingress controller|ingress-nginx/ingress-nginx|infra|"
-    "Infra: Cert-Manager|cert-manager|SSL Certificates|jetstack/cert-manager|infra|"
-    "Infra: ExternalDNS|external-dns|DO DNS Sync|bitnami/external-dns|infra|"
-    "Infra: Monitoring|kube-prometheus-stack|Prometheus & Grafana|prometheus-community/kube-prometheus-stack|infra|"
+    "Infra: Nginx Ingress|ingress-nginx|Core ingress controller|ingress-nginx/ingress-nginx|ingress-nginx|"
+    "Infra: Cert-Manager|cert-manager|SSL Certificates|jetstack/cert-manager|cert-manager|"
+    "Infra: ExternalDNS|external-dns|DO DNS Sync|bitnami/external-dns|external-dns|"
+    "Infra: Monitoring|kube-prometheus-stack|Prometheus & Grafana|prometheus-community/kube-prometheus-stack|monitoring|"
     "App: WordPress|wordpress|CMS Blog|wordpress/helm|wordpress|wordpress/helm/values.yaml"
     "App: n8n|n8n|Workflow Automation|n8n/helm|n8n|n8n/helm/values.yaml"
     "App: Matomo|matomo|Web Analytics|matomo/helm|matomo|matomo/helm/values.yaml"
@@ -53,15 +53,40 @@ install_selection() {
     # Special handling for infra apps (flags passed via cli often, not just values)
     # This is a simplified logic. Real "extensive" logic would have specific functions per app.
 
+    # Build extra_args as an array from the start to safely handle values with spaces
+    local extra_args_array=()
+
     # E.g., Cert-Manager needs --set installCRDs=true
-    local extra_args=""
     if [[ "$rn" == "cert-manager" ]]; then
-        extra_args="--set installCRDs=true"
+        extra_args_array+=(--set installCRDs=true)
     fi
 
-    # External DNS needs DO Token
+    # External DNS needs DigitalOcean token stored in a Kubernetes Secret
     if [[ "$rn" == "external-dns" ]]; then
-        extra_args="--set provider=digitalocean --set digitalocean.apiToken=$DO_TOKEN --set policy=sync --set txtOwnerId=${CLUSTER_NAME:-weown-cluster}"
+        if [ -z "${DO_TOKEN_SECRET_NAME:-}" ]; then
+            log_error "ExternalDNS deployment requires DO_TOKEN_SECRET_NAME environment variable."
+            log_error "This should be the name of a Kubernetes Secret containing your DigitalOcean API token."
+            log_error "Expected Secret key: 'digitalocean_api_token'"
+            log_error ""
+            log_error "Create the Secret securely using a temporary env file:"
+            log_error "  AUTH_FILE=\"\$(mktemp)\""
+            log_error "  trap 'rm -f \"\$AUTH_FILE\"' EXIT"
+            log_error "  cat > \"\$AUTH_FILE\" << 'EOF'"
+            log_error "digitalocean_api_token=<your-token>"
+            log_error "EOF"
+            log_error "  kubectl create secret generic <secret-name> --from-env-file=\"\$AUTH_FILE\" -n external-dns"
+            log_error ""
+            log_error "Alternatively, pipe the value from stdin without writing to disk:"
+            log_error "  printf 'digitalocean_api_token=' | tr -d '\\n'; read -s TOKEN; echo; \\"
+            log_error "    kubectl create secret generic <secret-name> --from-env-file=<(printf 'digitalocean_api_token=%s\\n' \"\$TOKEN\") -n external-dns"
+            log_error ""
+            log_error "Then set: export DO_TOKEN_SECRET_NAME=<secret-name>"
+            return 1
+        fi
+        extra_args_array+=(--set provider=digitalocean)
+        extra_args_array+=(--set digitalocean.secretName="${DO_TOKEN_SECRET_NAME}")
+        extra_args_array+=(--set policy=sync)
+        extra_args_array+=(--set txtOwnerId="${CLUSTER_NAME:-weown-cluster}")
     fi
 
     # WordPress: derive domain & email from env if not set in values
@@ -82,13 +107,15 @@ install_selection() {
             return 1
         fi
         # Populate chart values overriding placeholders / empty hosts
-        extra_args+=" --set wordpress.domain=${wp_domain}"
-        extra_args+=" --set wordpress.wordpressPassword=${wp_admin_password}"
-        extra_args+=" --set ingress.hosts[0].host=${wp_domain}"
-        extra_args+=" --set ingress.tls[0].hosts[0]=${wp_domain}"
+        extra_args_array+=(--set "wordpress.domain=${wp_domain}")
+        # Use --set-string for password to avoid Helm parsing issues with commas/booleans/numbers
+        extra_args_array+=(--set-string "wordpress.wordpressPassword=${wp_admin_password}")
+        extra_args_array+=(--set "ingress.hosts[0].host=${wp_domain}")
+        extra_args_array+=(--set "ingress.tls[0].hosts[0]=${wp_domain}")
+
         # Optionally wire email into WordPress config
         if [ -n "${LETSENCRYPT_EMAIL:-}" ]; then
-            extra_args+=" --set wordpress.wordpressEmail=${LETSENCRYPT_EMAIL}"
+            extra_args_array+=(--set "wordpress.wordpressEmail=${LETSENCRYPT_EMAIL}")
         fi
     fi
 
@@ -102,15 +129,14 @@ install_selection() {
             log_error "n8n deployment requires N8N_DOMAIN or BASE_DOMAIN in .env to derive the ingress host."
             return 1
         fi
-        extra_args+=" --set global.domain=${n8n_domain}"
+        extra_args_array+=(--set "global.domain=${n8n_domain}")
         if [ -n "${LETSENCRYPT_EMAIL:-}" ]; then
-            extra_args+=" --set global.email=${LETSENCRYPT_EMAIL}"
+            extra_args_array+=(--set "global.email=${LETSENCRYPT_EMAIL}")
         fi
-        extra_args+=" --set n8n.config.N8N_HOST=${n8n_domain}"
-        extra_args+=" --set n8n.config.WEBHOOK_URL=https://${n8n_domain}/"
-        extra_args+=" --set ingress.hosts[0].host=${n8n_domain}"
-        extra_args+=" --set ingress.tls[0].hosts[0]=${n8n_domain}"
-        extra_args+=" --set networkPolicy.ingress[0].from[0].namespaceSelector.matchLabels.name=infra"
+        extra_args_array+=(--set "n8n.config.N8N_HOST=${n8n_domain}")
+        extra_args_array+=(--set "n8n.config.WEBHOOK_URL=https://${n8n_domain}/")
+        extra_args_array+=(--set "ingress.hosts[0].host=${n8n_domain}")
+        extra_args_array+=(--set "ingress.tls[0].hosts[0]=${n8n_domain}")
     fi
 
     # Nextcloud: derive domain from env to replace DOMAIN_PLACEHOLDER
@@ -123,15 +149,14 @@ install_selection() {
             log_error "Nextcloud deployment requires NEXTCLOUD_DOMAIN or BASE_DOMAIN in .env to derive the ingress host."
             return 1
         fi
-        extra_args+=" --set global.domain=${nextcloud_domain}"
+        extra_args_array+=(--set "global.domain=${nextcloud_domain}")
         if [ -n "${LETSENCRYPT_EMAIL:-}" ]; then
-            extra_args+=" --set global.email=${LETSENCRYPT_EMAIL}"
+            extra_args_array+=(--set "global.email=${LETSENCRYPT_EMAIL}")
         fi
-        extra_args+=" --set nextcloud.config.NEXTCLOUD_HOST=${nextcloud_domain}"
-        extra_args+=" --set nextcloud.config.NEXTCLOUD_TRUSTED_DOMAINS=${nextcloud_domain}"
-        extra_args+=" --set ingress.hosts[0].host=${nextcloud_domain}"
-        extra_args+=" --set ingress.tls[0].hosts[0]=${nextcloud_domain}"
-        extra_args+=" --set networkPolicy.ingress[0].from[0].namespaceSelector.matchLabels.name=infra"
+        extra_args_array+=(--set "nextcloud.config.NEXTCLOUD_HOST=${nextcloud_domain}")
+        extra_args_array+=(--set "nextcloud.config.NEXTCLOUD_TRUSTED_DOMAINS=${nextcloud_domain}")
+        extra_args_array+=(--set "ingress.hosts[0].host=${nextcloud_domain}")
+        extra_args_array+=(--set "ingress.tls[0].hosts[0]=${nextcloud_domain}")
     fi
 
     # Matomo: derive domain & tracking host from env to replace DOMAIN_PLACEHOLDER
@@ -157,18 +182,18 @@ install_selection() {
             return 1
         fi
         # Override ingress + global + website host and DB auth values
-        extra_args+=" --set global.domain=${matomo_domain}"
+        extra_args_array+=(--set "global.domain=${matomo_domain}")
         if [ -n "${LETSENCRYPT_EMAIL:-}" ]; then
-            extra_args+=" --set global.email=${LETSENCRYPT_EMAIL}"
-            extra_args+=" --set matomo.admin.email=${LETSENCRYPT_EMAIL}"
-        fi
-        extra_args+=" --set mariadb.auth.rootPassword=${matomo_db_root_password}"
-        extra_args+=" --set mariadb.auth.password=${matomo_db_password}"
-        extra_args+=" --set ingress.hosts[0].host=${matomo_domain}"
-        extra_args+=" --set ingress.tls[0].hosts[0]=${matomo_domain}"
-        extra_args+=" --set networkPolicy.ingress[0].from[0].namespaceSelector.matchLabels.name=infra"
+            extra_args_array+=(--set "global.email=${LETSENCRYPT_EMAIL}")
+            extra_args_array+=(--set "matomo.admin.email=${LETSENCRYPT_EMAIL}")
+        fi
+        # Use --set-string for passwords to avoid Helm parsing issues with commas/booleans/numbers
+        extra_args_array+=(--set-string "mariadb.auth.rootPassword=${matomo_db_root_password}")
+        extra_args_array+=(--set-string "mariadb.auth.password=${matomo_db_password}")
+        extra_args_array+=(--set "ingress.hosts[0].host=${matomo_domain}")
+        extra_args_array+=(--set "ingress.tls[0].hosts[0]=${matomo_domain}")
         if [ -n "$wp_tracking_host" ]; then
-            extra_args+=" --set matomo.website.host=${wp_tracking_host}"
+            extra_args_array+=(--set "matomo.website.host=${wp_tracking_host}")
         fi
     fi
 
@@ -183,33 +208,45 @@ install_selection() {
             log_error "Vaultwarden deployment requires VAULTWARDEN_DOMAIN or BASE_DOMAIN in .env to derive the domain."
             return 1
         fi
-        extra_args+=" --set global.domain=${vaultwarden_domain}"
-        extra_args+=" --set global.subdomain=${vaultwarden_subdomain}"
+        extra_args_array+=(--set "global.domain=${vaultwarden_domain}")
+        extra_args_array+=(--set "global.subdomain=${vaultwarden_subdomain}")
         if [ -n "${LETSENCRYPT_EMAIL:-}" ]; then
-            extra_args+=" --set certManager.email=${LETSENCRYPT_EMAIL}"
+            extra_args_array+=(--set "certManager.email=${LETSENCRYPT_EMAIL}")
         fi
-        extra_args+=" --set vaultwarden.domain=https://${vaultwarden_subdomain}.${vaultwarden_domain}"
-        extra_args+=" --set security.networkPolicy.ingress[0].from[0].namespaceSelector.matchLabels.name=infra"
+        extra_args_array+=(--set "vaultwarden.domain=https://${vaultwarden_subdomain}.${vaultwarden_domain}")
     fi
 
     log_info "Processing $display_name ($release_name)..."
-    log_info "Helm extra args: ${extra_args:-<none>}"
+    if [ ${#extra_args_array[@]} -gt 0 ]; then
+        log_info "Helm extra args count: ${#extra_args_array[@]}"
+    else
+        log_info "Helm extra args: <none>"
+    fi
 
     # Call helm deploy
-    # Note: extra_args handling needs to be passed to deploy_chart or handled here
-    # We'll modify deploy_chart to accept extra args in a future refactor, 
-    # for now, we just append to the helm command line inside the function via a hack or direct call.
-
-    # Direct call for flexibility
-    local cmd="helm upgrade --install $release_name $resolved_chart --namespace $ns --create-namespace $extra_args"
+    # Build Helm command as an array to avoid eval/command injection
+    local cmd=(helm upgrade --install "$release_name" "$resolved_chart" --namespace "$ns" --create-namespace)
     if [ -n "$resolved_values" ]; then
-        cmd="$cmd -f $resolved_values"
+        cmd+=(-f "$resolved_values")
+    fi
+    if [ ${#extra_args_array[@]} -gt 0 ]; then
+        cmd+=("${extra_args_array[@]}")
     fi
 
-    log_info "Executing: $cmd"
-    if eval $cmd; then
+    # Avoid logging the full command to prevent leaking secrets in extra_args
+    log_info "Executing Helm upgrade for release '$release_name' in namespace '$ns'."
+    if "${cmd[@]}"; then
         log_success "$display_name Installed."
 
+        # Special post-deployment for ingress-nginx namespace: label for NetworkPolicy
+        if [[ "$ns" == "ingress-nginx" ]]; then
+            if ! kubectl label namespace "$ns" name=ingress-nginx --overwrite; then
+                log_error "Failed to label namespace '$ns' - NetworkPolicy will not work correctly; treating as hard failure"
+                return 1
+            fi
+            log_success "Labeled namespace '$ns' with name=ingress-nginx for NetworkPolicy access"
+        fi
+
         # Post-deploy status logs
         log_info "Helm status for $release_name (namespace: $ns):"
         helm status "$release_name" -n "$ns" || log_warn "helm status failed for $release_name in $ns"
@@ -226,7 +263,9 @@ install_selection() {
               --format Name,Status,Region,Version,NodePools --no-header \
               || log_warn "doctl cluster get failed for $CLUSTER_NAME"
         fi
+        return 0
     else
         log_error "$display_name Installation Failed."
+        return 1
     fi
 }