refactor: consolidate shell port-spec parsing into shared parse_port_specs()

containers/agent/setup-iptables.sh

@@ -40,25 +40,49 @@ is_valid_port_spec() {
   fi
 }
 
-# Allow AWF_HOST_SERVICE_PORTS entries to a destination IP.
-# Port validation is intentionally strict to prevent malformed iptables rules.
+# Parse a comma-separated port spec string, validating each entry.
+# Mirrors TypeScript parseValidPortSpecs() in src/host-iptables-validation.ts.
+# Usage: parse_port_specs <result_array_name> <input> <label>
+# Populates the named array with valid port specs; warns and skips invalid ones.
+# Requires bash 4.3+ for nameref (declare -n).
+parse_port_specs() {
+  local -n _pps_result="$1"
+  local _pps_input="$2"
+  local _pps_label="$3"
+  _pps_result=()
+
+  if [ -z "$_pps_input" ]; then
+    return
+  fi
+
+  local -a _pps_entries=()
+  local _pps_trimmed=""
+  IFS=',' read -ra _pps_entries <<< "$_pps_input"
+  for _pps_trimmed in "${_pps_entries[@]}"; do
+    _pps_trimmed="${_pps_trimmed#"${_pps_trimmed%%[! ]*}"}"
+    _pps_trimmed="${_pps_trimmed%"${_pps_trimmed##*[! ]}"}"
+    if [ -z "$_pps_trimmed" ]; then
+      continue
+    fi
+    if ! is_valid_port_spec "$_pps_trimmed"; then
+      echo "[iptables] WARNING: Skipping invalid ${_pps_label}: $_pps_trimmed"
+      continue
+    fi
+    _pps_result+=("$_pps_trimmed")
+  done
+}
+
+# Allow AWF_HOST_SERVICE_PORTS entries (pre-validated via parse_port_specs) to a destination IP.
 allow_service_ports_to_ip() {
   local dest_ip="$1"
   local log_each_port="${2:-false}"
-  local port=""
+  local port_spec=""
 
-  for port in "${HSP_PORTS[@]}"; do
-    port=$(echo "$port" | xargs)
-    if ! [[ "$port" =~ ^[1-9][0-9]{0,4}$ ]] || [ "$port" -lt 1 ] || [ "$port" -gt 65535 ]; then
-      echo "[iptables] WARNING: Skipping invalid service port: $port"
-      continue
-    fi
-    if [ -n "$port" ]; then
-      if [ "$log_each_port" = "true" ]; then
-        echo "[iptables]   Allow host service port $port to $dest_ip"
-      fi
-      iptables -A OUTPUT -p tcp -d "$dest_ip" --dport "$port" -j ACCEPT
+  for port_spec in "${HSP_PORTS[@]}"; do
+    if [ "$log_each_port" = "true" ]; then
+      echo "[iptables]   Allow host service port $port_spec to $dest_ip"
     fi
+    iptables -A OUTPUT -p tcp -d "$dest_ip" --dport "$port_spec" -j ACCEPT
   done
 }
 
@@ -268,14 +292,9 @@ allow_host_access_to_gateway() {
   # FILTER: also allow user-specified ports from --allow-host-ports
   if [ -n "$AWF_ALLOW_HOST_PORTS" ]; then
     local -a gw_ports=()
+    parse_port_specs gw_ports "$AWF_ALLOW_HOST_PORTS" "port spec"
     local port_spec=""
-    IFS=',' read -ra gw_ports <<< "$AWF_ALLOW_HOST_PORTS"
     for port_spec in "${gw_ports[@]}"; do
-      port_spec=$(echo "$port_spec" | xargs)
-      if ! is_valid_port_spec "$port_spec"; then
-        echo "[iptables] WARNING: Skipping invalid port spec: $port_spec"
-        continue
-      fi
       echo "[iptables]   Allow ${label} port $port_spec"
       iptables -A OUTPUT -p tcp -d "$gw_ip" --dport "$port_spec" -j ACCEPT
     done
@@ -323,8 +342,8 @@ configure_host_access_rules() {
   # Must be applied BEFORE dangerous port RETURN rules so traffic to host gateway
   # on these ports is accepted, not dropped.
   if [ -n "$AWF_HOST_SERVICE_PORTS" ] && [ -n "$AWF_ENABLE_HOST_ACCESS" ]; then
-    # Parse port list once, before resolving gateway IPs, so both blocks can use it
-    IFS=',' read -ra HSP_PORTS <<< "$AWF_HOST_SERVICE_PORTS"
+    # Parse and validate port list once, before resolving gateway IPs, so both blocks can use it
+    parse_port_specs HSP_PORTS "$AWF_HOST_SERVICE_PORTS" "host service port"
 
     # Resolve host gateway IP (with AWF_HOST_GATEWAY_IP fallback, same as host access block)
     HSP_HOST_GW_IP=$(getent hosts host.docker.internal 2>/dev/null | awk 'NR==1 { print $1 }')
@@ -374,29 +393,12 @@ configure_http_dnat() {
   # If user specified additional ports via --allow-host-ports, redirect those too
   if [ -n "$AWF_ALLOW_HOST_PORTS" ]; then
     echo "[iptables] Redirect user-specified ports to Squid..."
-
-    # Parse comma-separated port list
-    IFS=',' read -ra PORTS <<< "$AWF_ALLOW_HOST_PORTS"
-
-    for port_spec in "${PORTS[@]}"; do
-      # Remove leading/trailing spaces
-      port_spec=$(echo "$port_spec" | xargs)
-
-      if ! is_valid_port_spec "$port_spec"; then
-        echo "[iptables] WARNING: Skipping invalid port spec: $port_spec"
-        continue
-      fi
-
-      if [[ $port_spec == *"-"* ]]; then
-        # Port range (e.g., "3000-3010")
-        echo "[iptables]   Redirect port range $port_spec to Squid..."
-        # For port ranges, use --dport with range syntax (without multiport)
-        iptables -t nat -A OUTPUT -p tcp --dport "$port_spec" -j DNAT --to-destination "${SQUID_IP}:${SQUID_PORT}"
-      else
-        # Single port (e.g., "3000")
-        echo "[iptables]   Redirect port $port_spec to Squid..."
-        iptables -t nat -A OUTPUT -p tcp --dport "$port_spec" -j DNAT --to-destination "${SQUID_IP}:${SQUID_PORT}"
-      fi
+    local -a dnat_ports=()
+    parse_port_specs dnat_ports "$AWF_ALLOW_HOST_PORTS" "port spec"
+    local port_spec=""
+    for port_spec in "${dnat_ports[@]}"; do
+      echo "[iptables]   Redirect port $port_spec to Squid..."
+      iptables -t nat -A OUTPUT -p tcp --dport "$port_spec" -j DNAT --to-destination "${SQUID_IP}:${SQUID_PORT}"
     done
   else
     echo "[iptables] No additional ports specified (only 80, 443 allowed)"

tests/setup-iptables-port-spec.test.sh

@@ -1,5 +1,6 @@
 #!/bin/bash
-# Shell unit tests for is_valid_port_spec() in containers/agent/setup-iptables.sh.
+# Shell unit tests for is_valid_port_spec() and parse_port_specs() in
+# containers/agent/setup-iptables.sh.
 #
 # Runs every case from tests/port-spec-fixtures.json against the shell
 # implementation to ensure it stays aligned with the TypeScript isValidPortSpec()
@@ -38,33 +39,74 @@ pass() { echo "✓ $1"; PASS=$((PASS + 1)); }
 fail() { echo "❌ FAIL: $1"; FAIL=$((FAIL + 1)); }
 
 # ---------------------------------------------------------------------------
-# Source only the is_valid_port_spec() function from setup-iptables.sh.
+# Source is_valid_port_spec() and parse_port_specs() from setup-iptables.sh.
 # ---------------------------------------------------------------------------
 
-# Extract function definition (up to and including the closing brace of
-# is_valid_port_spec) so we can source it in isolation without side-effects
-# from the rest of the script.
-FUNC_DEF=$(awk '
-  /^is_valid_port_spec\(\)/ { capture=1 }
-  capture { print }
-  capture && /^}/ { capture=0; exit }
-' "${SETUP_IPTABLES}")
+# Extract both function definitions so we can source them in isolation without
+# side-effects from the rest of the script.
+extract_func() {
+  local func_name="$1"
+  awk -v fn="${func_name}" '
+    $0 ~ "^"fn"\\(\\)" { capture=1 }
+    capture { print }
+    capture && /^}/ { capture=0; exit }
+  ' "${SETUP_IPTABLES}"
+}
+
+IS_VALID_FUNC_DEF=$(extract_func "is_valid_port_spec")
+PARSE_SPECS_FUNC_DEF=$(extract_func "parse_port_specs")
 
-if [ -z "${FUNC_DEF}" ]; then
+if [ -z "${IS_VALID_FUNC_DEF}" ]; then
   echo "❌ is_valid_port_spec() not found in ${SETUP_IPTABLES}"
   exit 1
 fi
 
+if [ -z "${PARSE_SPECS_FUNC_DEF}" ]; then
+  echo "❌ parse_port_specs() not found in ${SETUP_IPTABLES}"
+  exit 1
+fi
+
 run_is_valid_port_spec() {
   local spec="$1"
   # Run in a subshell to isolate the eval so the function definition
   # doesn't leak into the outer shell's namespace.
   (
-    eval "${FUNC_DEF}"
+    eval "${IS_VALID_FUNC_DEF}"
     is_valid_port_spec "$spec"
   )
 }
 
+# run_parse_port_specs <input> <label>
+# Outputs the resulting array elements, one per line.
+# Warnings (lines starting with "[iptables] WARNING:") go to stdout from the
+# function but are filtered out here so callers get only valid specs.
+run_parse_port_specs() {
+  local input="$1"
+  local label="${2:-port spec}"
+  (
+    eval "${IS_VALID_FUNC_DEF}"
+    eval "${PARSE_SPECS_FUNC_DEF}"
+    declare -a _result=()
+    parse_port_specs _result "$input" "$label"
+    for elem in "${_result[@]}"; do
+      echo "$elem"
+    done
+  ) 2>/dev/null | grep -v '^\[iptables\] WARNING:' || true
+}
+
+# run_parse_port_specs_warnings <input> <label>
+# Outputs only the WARNING lines emitted by parse_port_specs.
+run_parse_port_specs_warnings() {
+  local input="$1"
+  local label="${2:-port spec}"
+  (
+    eval "${IS_VALID_FUNC_DEF}"
+    eval "${PARSE_SPECS_FUNC_DEF}"
+    declare -a _result=()
+    parse_port_specs _result "$input" "$label"
+  ) 2>/dev/null | grep '^\[iptables\] WARNING:' || true
+}
+
 # ---------------------------------------------------------------------------
 # Load test vectors from the shared fixture file
 # ---------------------------------------------------------------------------
@@ -86,29 +128,105 @@ for s in data['invalid']:
 ")
 
 # ---------------------------------------------------------------------------
-# Test valid specs — is_valid_port_spec() should return 0 (success)
+# is_valid_port_spec — valid specs
 # ---------------------------------------------------------------------------
 
 for spec in "${VALID_SPECS[@]}"; do
   if run_is_valid_port_spec "${spec}" &>/dev/null; then
-    pass "accepts valid spec '${spec}'"
+    pass "is_valid_port_spec accepts valid spec '${spec}'"
   else
-    fail "should accept '${spec}' but rejected it"
+    fail "is_valid_port_spec should accept '${spec}' but rejected it"
   fi
 done
 
 # ---------------------------------------------------------------------------
-# Test invalid specs — is_valid_port_spec() should return non-zero (failure)
+# is_valid_port_spec — invalid specs
 # ---------------------------------------------------------------------------
 
 for spec in "${INVALID_SPECS[@]}"; do
   if run_is_valid_port_spec "${spec}" &>/dev/null; then
-    fail "should reject '${spec}' but accepted it"
+    fail "is_valid_port_spec should reject '${spec}' but accepted it"
   else
-    pass "rejects invalid spec '${spec}'"
+    pass "is_valid_port_spec rejects invalid spec '${spec}'"
   fi
 done
 
+# ---------------------------------------------------------------------------
+# parse_port_specs — functional tests
+# ---------------------------------------------------------------------------
+
+# Empty input produces empty result
+result=$(run_parse_port_specs "" "port spec")
+if [ -z "$result" ]; then
+  pass "parse_port_specs returns empty array for empty input"
+else
+  fail "parse_port_specs should return empty array for empty input, got: ${result}"
+fi
+
+# Single valid port
+result=$(run_parse_port_specs "80" "port spec")
+if [ "$result" = "80" ]; then
+  pass "parse_port_specs returns single valid port"
+else
+  fail "parse_port_specs should return '80' for input '80', got: '${result}'"
+fi
+
+# Multiple valid ports
+mapfile -t result_arr < <(run_parse_port_specs "80,443,3128" "port spec")
+if [ "${#result_arr[@]}" -eq 3 ] && [ "${result_arr[0]}" = "80" ] && [ "${result_arr[1]}" = "443" ] && [ "${result_arr[2]}" = "3128" ]; then
+  pass "parse_port_specs returns all valid ports from comma-separated input"
+else
+  fail "parse_port_specs should return [80,443,3128], got: ${result_arr[*]}"
+fi
+
+# Port with surrounding whitespace is trimmed
+result=$(run_parse_port_specs " 80 " "port spec")
+if [ "$result" = "80" ]; then
+  pass "parse_port_specs trims leading/trailing whitespace from port spec"
+else
+  fail "parse_port_specs should trim ' 80 ' to '80', got: '${result}'"
+fi
+
+# Valid port range
+result=$(run_parse_port_specs "3000-3010" "port spec")
+if [ "$result" = "3000-3010" ]; then
+  pass "parse_port_specs accepts a valid port range"
+else
+  fail "parse_port_specs should accept range '3000-3010', got: '${result}'"
+fi
+
+# Invalid specs are filtered out; valid ones are kept
+mapfile -t result_arr < <(run_parse_port_specs "80,0,443,65536" "port spec")
+if [ "${#result_arr[@]}" -eq 2 ] && [ "${result_arr[0]}" = "80" ] && [ "${result_arr[1]}" = "443" ]; then
+  pass "parse_port_specs filters invalid specs and keeps valid ones"
+else
+  fail "parse_port_specs should keep [80,443] from '80,0,443,65536', got: ${result_arr[*]}"
+fi
+
+# Invalid spec produces a warning message
+warning_output=$(run_parse_port_specs_warnings "0" "port spec")
+if echo "$warning_output" | grep -q "WARNING"; then
+  pass "parse_port_specs emits WARNING for invalid spec"
+else
+  fail "parse_port_specs should emit WARNING for invalid spec '0', got: '${warning_output}'"
+fi
+
+# All-invalid input returns empty array
+result=$(run_parse_port_specs "0,65536,abc" "port spec")
+if [ -z "$result" ]; then
+  pass "parse_port_specs returns empty array when all specs are invalid"
+else
+  fail "parse_port_specs should return empty array for all-invalid input, got: '${result}'"
+fi
+
+# Mix of valid ports and ranges
+mapfile -t result_arr < <(run_parse_port_specs "80,3000-3010,443" "port spec")
+if [ "${#result_arr[@]}" -eq 3 ] && [ "${result_arr[0]}" = "80" ] && [ "${result_arr[1]}" = "3000-3010" ] && [ "${result_arr[2]}" = "443" ]; then
+  pass "parse_port_specs handles mix of single ports and ranges"
+else
+  fail "parse_port_specs should return [80,3000-3010,443], got: ${result_arr[*]}"
+fi
+
 # ---------------------------------------------------------------------------
 # Summary
 # ---------------------------------------------------------------------------