fix(security): validate RALPH_DIR, parse .ralphrc safely, check PLATFORM_DRIVER (#76, #77, #79)

LarsCowe · LarsCowe · commit 9bc7cc1e6ea8 · 2026-03-26T21:34:58.000+01:00
Replace `source .ralphrc` with a safe key=value parser that rejects command substitution ($(), backticks) and only sets allowlisted config keys. Validate RALPH_DIR to reject path traversal (..) and non-default relative paths. Add path traversal checks to PLATFORM_DRIVER in both ralph_loop.sh and ralph_import.sh. Guard RALPH_DIR in library files to prevent silent fallback to unvalidated defaults. Closes #76, closes #77, closes #79
diff --git a/ralph/lib/circuit_breaker.sh b/ralph/lib/circuit_breaker.sh
@@ -12,8 +12,11 @@ CB_STATE_HALF_OPEN="HALF_OPEN"  # Monitoring mode, checking for recovery
 CB_STATE_OPEN="OPEN"            # Failure detected, execution halted
 
 # Circuit Breaker Configuration
-# Use RALPH_DIR if set by main script, otherwise default to .ralph
-RALPH_DIR="${RALPH_DIR:-.ralph}"
+# RALPH_DIR must be set by the caller (ralph_loop.sh validates it)
+if [[ -z "${RALPH_DIR:-}" ]]; then
+    echo "Error: RALPH_DIR is not set. Source ralph_loop.sh first." >&2
+    return 1
+fi
 CB_STATE_FILE="$RALPH_DIR/.circuit_breaker_state"
 CB_HISTORY_FILE="$RALPH_DIR/.circuit_breaker_history"
 # Configurable thresholds - override via environment variables:
diff --git a/ralph/lib/response_analyzer.sh b/ralph/lib/response_analyzer.sh
@@ -15,8 +15,11 @@ YELLOW='\033[1;33m'
 BLUE='\033[0;34m'
 NC='\033[0m'
 
-# Use RALPH_DIR if set by main script, otherwise default to .ralph
-RALPH_DIR="${RALPH_DIR:-.ralph}"
+# RALPH_DIR must be set by the caller (ralph_loop.sh validates it)
+if [[ -z "${RALPH_DIR:-}" ]]; then
+    echo "Error: RALPH_DIR is not set. Source ralph_loop.sh first." >&2
+    return 1
+fi
 
 # Analysis configuration
 COMPLETION_KEYWORDS=("done" "complete" "finished" "all tasks complete" "project complete" "ready for review")
diff --git a/ralph/lib/write_heartbeat.sh b/ralph/lib/write_heartbeat.sh
@@ -82,7 +82,11 @@ start_write_heartbeat() {
 # Stop the heartbeat monitor and clean up marker files.
 # Safe to call even if no monitor is running.
 stop_write_heartbeat() {
-    local ralph_dir="${RALPH_DIR:-.ralph}"
+    if [[ -z "${RALPH_DIR:-}" ]]; then
+        echo "Error: RALPH_DIR is not set" >&2
+        return 1
+    fi
+    local ralph_dir="$RALPH_DIR"
     local pid_file="$ralph_dir/.write_heartbeat_pid"
 
     if [[ -f "$pid_file" ]]; then
@@ -102,6 +106,10 @@ stop_write_heartbeat() {
 # Returns 0 (true) if the timeout marker exists.
 # Does NOT delete the marker — the caller is responsible for cleanup.
 was_write_heartbeat_timeout() {
-    local ralph_dir="${RALPH_DIR:-.ralph}"
+    if [[ -z "${RALPH_DIR:-}" ]]; then
+        echo "Error: RALPH_DIR is not set" >&2
+        return 1
+    fi
+    local ralph_dir="$RALPH_DIR"
     [[ -f "$ralph_dir/.write_heartbeat_triggered" ]]
 }
diff --git a/ralph/ralph_import.sh b/ralph/ralph_import.sh
@@ -15,6 +15,12 @@ CLAUDE_CODE_CMD="claude"
 SCRIPT_DIR="$(dirname "${BASH_SOURCE[0]}")"
 PLATFORM_DRIVER="${PLATFORM_DRIVER:-claude-code}"
 
+# Reject path traversal in PLATFORM_DRIVER (#77)
+if [[ "$PLATFORM_DRIVER" =~ [/] ]] || [[ "$PLATFORM_DRIVER" == *".."* ]]; then
+    echo "Error: Invalid PLATFORM_DRIVER: $PLATFORM_DRIVER" >&2
+    exit 1
+fi
+
 # Source platform driver if available
 if [[ -f "$SCRIPT_DIR/drivers/${PLATFORM_DRIVER}.sh" ]]; then
     # shellcheck source=/dev/null
diff --git a/ralph/ralph_loop.sh b/ralph/ralph_loop.sh
@@ -10,7 +10,23 @@ set -e  # Exit on any error
 # via --allowedTools flag in CLAUDE_CMD_ARGS, which is the proper approach.
 # Exporting sandbox variables without a verified sandbox would be misleading.
 
-# Source library components
+# validate_ralph_dir - Reject dangerous RALPH_DIR values (#79)
+# Accepts: ".ralph" (default), empty (becomes .ralph), absolute paths (tests)
+# Rejects: paths with ".." (traversal), non-default relative paths
+validate_ralph_dir() {
+    local dir="$1"
+    [[ "$dir" == ".ralph" || -z "$dir" ]] && return 0
+    [[ "$dir" == *".."* ]] && { echo "Error: RALPH_DIR must not contain '..': '$dir'" >&2; return 1; }
+    [[ "$dir" != /* ]] && { echo "Error: RALPH_DIR must be '.ralph' or an absolute path, got: '$dir'" >&2; return 1; }
+    return 0
+}
+
+# Validate and default RALPH_DIR BEFORE sourcing libraries that depend on it
+validate_ralph_dir "${RALPH_DIR:-}" || exit 1
+RALPH_DIR="${RALPH_DIR:-.ralph}"
+export RALPH_DIR
+
+# Source library components (RALPH_DIR must be set before this point)
 SCRIPT_DIR="$(dirname "${BASH_SOURCE[0]}")"
 source "$SCRIPT_DIR/lib/date_utils.sh"
 source "$SCRIPT_DIR/lib/timeout_utils.sh"
@@ -20,7 +36,57 @@ source "$SCRIPT_DIR/lib/write_heartbeat.sh"
 
 # Configuration
 # Ralph-specific files live in .ralph/ subfolder
-RALPH_DIR="${RALPH_DIR:-.ralph}"
+
+# Allowlist of known .ralphrc config keys (#76)
+# Space-delimited string (avoids declare -A scoping issues when sourced)
+RALPHRC_ALLOWED_KEYS=" PLATFORM_DRIVER PROJECT_NAME PROJECT_TYPE MAX_CALLS_PER_HOUR CLAUDE_TIMEOUT_MINUTES CLAUDE_OUTPUT_FORMAT WRITE_TIMEOUT_MINUTES ALLOWED_TOOLS CLAUDE_PERMISSION_MODE PERMISSION_DENIAL_MODE SESSION_CONTINUITY SESSION_EXPIRY_HOURS TASK_SOURCES GITHUB_TASK_LABEL BEADS_FILTER CB_NO_PROGRESS_THRESHOLD CB_SAME_ERROR_THRESHOLD CB_OUTPUT_DECLINE_THRESHOLD CB_READ_ONLY_TIMEOUT_THRESHOLD CB_COOLDOWN_MINUTES CB_AUTO_RESET TEST_COMMAND QUALITY_GATES QUALITY_GATE_MODE QUALITY_GATE_TIMEOUT QUALITY_GATE_ON_COMPLETION_ONLY REVIEW_MODE REVIEW_ENABLED REVIEW_INTERVAL CLAUDE_MIN_VERSION RALPH_VERBOSE PROMPT_FILE FIX_PLAN_FILE AGENT_FILE "
+
+# parse_ralphrc - Safely parse .ralphrc as key=value config (#76)
+# Rejects command substitution ($(), backticks). Only sets allowlisted keys.
+parse_ralphrc() {
+    local config_file="$1"
+    local line_num=0
+    while IFS= read -r line || [[ -n "$line" ]]; do
+        line_num=$((line_num + 1))
+        # Strip leading/trailing whitespace
+        line="${line#"${line%%[![:space:]]*}"}"
+        line="${line%"${line##*[![:space:]]}"}"
+        # Skip empty lines and comments
+        [[ -z "$line" || "$line" == \#* ]] && continue
+        # Reject command substitution ($() and backticks)
+        if [[ "$line" == *'$('* ]] || [[ "$line" == *'`'* ]]; then
+            log_status "WARN" ".ralphrc:$line_num: rejected (command substitution): $line"
+            continue
+        fi
+        # Match KEY=VALUE
+        if [[ "$line" =~ ^([A-Z_][A-Z0-9_]*)=(.*) ]]; then
+            local key="${BASH_REMATCH[1]}"
+            local raw_value="${BASH_REMATCH[2]}"
+            # Check allowlist
+            if [[ "$RALPHRC_ALLOWED_KEYS" != *" $key "* ]]; then
+                log_status "WARN" ".ralphrc:$line_num: unknown key ignored: $key"
+                continue
+            fi
+            # Strip outer quotes
+            local value="$raw_value"
+            if [[ "$value" =~ ^\"(.*)\"$ ]]; then
+                value="${BASH_REMATCH[1]}"
+            elif [[ "$value" =~ ^\'(.*)\'$ ]]; then
+                value="${BASH_REMATCH[1]}"
+            fi
+            # Handle ${VAR:-default} and ${VAR:-} (empty default)
+            if [[ "$value" =~ ^\$\{([A-Z_][A-Z0-9_]*):-([^}]*)\}$ ]]; then
+                local ref_var="${BASH_REMATCH[1]}"
+                local default_val="${BASH_REMATCH[2]}"
+                local current="${!ref_var:-}"
+                value="${current:-$default_val}"
+            fi
+            printf -v "$key" '%s' "$value"
+        else
+            log_status "WARN" ".ralphrc:$line_num: skipped (not KEY=VALUE): $line"
+        fi
+    done < "$config_file"
+}
 PROMPT_FILE="$RALPH_DIR/PROMPT.md"
 LOG_DIR="$RALPH_DIR/logs"
 DOCS_DIR="$RALPH_DIR/docs/generated"
@@ -164,7 +230,7 @@ TEST_PERCENTAGE_THRESHOLD=30  # If more than 30% of recent loops are test-only,
 # Ralph configuration file
 # bmalph installs .ralph/.ralphrc. Fall back to a project-root .ralphrc for
 # older standalone Ralph layouts.
-RALPHRC_FILE="${RALPHRC_FILE:-$RALPH_DIR/.ralphrc}"
+RALPHRC_FILE="$RALPH_DIR/.ralphrc"
 RALPHRC_LOADED=false
 
 # Platform driver (set from .ralphrc or environment)
@@ -214,9 +280,8 @@ load_ralphrc() {
         return 0
     fi
 
-    # Source config (this may override default values)
-    # shellcheck source=/dev/null
-    source "$config_file"
+    # Parse config as safe key=value pairs (#76 — no longer sourced as bash)
+    parse_ralphrc "$config_file"
 
     # Map config variable names to internal names
     if [[ -n "${ALLOWED_TOOLS:-}" ]]; then
@@ -300,6 +365,11 @@ driver_permission_denial_help() {
 
 # Source platform driver
 load_platform_driver() {
+    # Reject path traversal in PLATFORM_DRIVER (#77)
+    if [[ "$PLATFORM_DRIVER" =~ [/] ]] || [[ "$PLATFORM_DRIVER" == *".."* ]]; then
+        log_status "ERROR" "Invalid PLATFORM_DRIVER (path traversal): $PLATFORM_DRIVER"
+        return 1
+    fi
     local driver_file="$SCRIPT_DIR/drivers/${PLATFORM_DRIVER}.sh"
     if [[ ! -f "$driver_file" ]]; then
         log_status "ERROR" "Platform driver not found: $driver_file"
diff --git a/ralph/templates/ralphrc.template b/ralph/templates/ralphrc.template
@@ -1,6 +1,10 @@
 # .ralphrc - Ralph project configuration
-# Generated by: ralph enable
-# Documentation: https://github.com/frankbria/ralph-claude-code
+# Generated by: bmalph init
+# Documentation: https://github.com/LarsCowe/bmalph
+#
+# SECURITY: This file is parsed as KEY=VALUE pairs, NOT sourced as bash.
+# Only recognized config keys are accepted. Shell constructs ($(), backticks)
+# are rejected. Supported value forms: VALUE, "VALUE", 'VALUE', "${VAR:-default}"
 #
 # This file configures Ralph's behavior for this specific project.
 # Values here override global Ralph defaults.
diff --git a/src/installer/template-files.ts b/src/installer/template-files.ts
@@ -14,6 +14,18 @@ const CLAUDE_ALLOWED_TOOLS_TEMPLATE_LINE =
   'ALLOWED_TOOLS="Write,Read,Edit,MultiEdit,Glob,Grep,Task,TodoWrite,WebFetch,WebSearch,EnterPlanMode,ExitPlanMode,NotebookEdit,Bash"';
 const PREVIOUS_CLAUDE_ALLOWED_TOOLS_TEMPLATE_LINE =
   'ALLOWED_TOOLS="Write,Read,Edit,MultiEdit,Glob,Grep,Task,TodoWrite,WebFetch,WebSearch,NotebookEdit,Bash"';
+const RALPHRC_HEADER_BLOCK = `# .ralphrc - Ralph project configuration
+# Generated by: bmalph init
+# Documentation: https://github.com/LarsCowe/bmalph
+#
+# SECURITY: This file is parsed as KEY=VALUE pairs, NOT sourced as bash.
+# Only recognized config keys are accepted. Shell constructs ($(), backticks)
+# are rejected. Supported value forms: VALUE, "VALUE", 'VALUE', "\${VAR:-default}"
+#`;
+const PREVIOUS_RALPHRC_HEADER_BLOCK = `# .ralphrc - Ralph project configuration
+# Generated by: ralph enable
+# Documentation: https://github.com/frankbria/ralph-claude-code
+#`;
 const RALPHRC_DRIVER_COMMENT_LINE =
   "# Platform driver for Ralph loop (claude-code, codex, opencode, copilot, or cursor)";
 const PREVIOUS_RALPHRC_DRIVER_COMMENT_LINE =
@@ -210,6 +222,7 @@ function renderLegacyRalphrcTemplate(platformId: string): string {
 
 function normalizeManagedRalphrcContent(content: string): string {
   return content
+    .replace(PREVIOUS_RALPHRC_HEADER_BLOCK, RALPHRC_HEADER_BLOCK)
     .replace(PREVIOUS_RALPHRC_DRIVER_COMMENT_LINE, RALPHRC_DRIVER_COMMENT_LINE)
     .replace(PREVIOUS_RALPHRC_IGNORED_DRIVERS_COMMENT_LINE, RALPHRC_IGNORED_DRIVERS_COMMENT_LINE);
 }
diff --git a/tests/bash/ralph_loop.bats b/tests/bash/ralph_loop.bats
@@ -123,6 +123,164 @@ _enable_assertions() {
     eval "${_bats_err_trap:-}"
 }
 
+# ===========================================================================
+# validate_ralph_dir
+# ===========================================================================
+
+@test "validate_ralph_dir accepts .ralph" {
+    _enable_assertions
+    run validate_ralph_dir ".ralph"
+    assert_success
+}
+
+@test "validate_ralph_dir accepts empty string (will default to .ralph)" {
+    _enable_assertions
+    run validate_ralph_dir ""
+    assert_success
+}
+
+@test "validate_ralph_dir accepts absolute path" {
+    _enable_assertions
+    run validate_ralph_dir "/tmp/bats-test-ralph"
+    assert_success
+}
+
+@test "validate_ralph_dir rejects path traversal" {
+    _enable_assertions
+    run validate_ralph_dir "../../etc"
+    assert_failure
+    assert_output --partial "must not contain '..'"
+}
+
+@test "validate_ralph_dir rejects relative non-default path" {
+    _enable_assertions
+    run validate_ralph_dir "foo/bar"
+    assert_failure
+    assert_output --partial "must be '.ralph' or an absolute path"
+}
+
+# ===========================================================================
+# load_platform_driver
+# ===========================================================================
+
+@test "load_platform_driver rejects path traversal" {
+    _enable_assertions
+    PLATFORM_DRIVER="../../evil"
+    run load_platform_driver
+    assert_failure
+    assert_output --partial "Invalid PLATFORM_DRIVER"
+}
+
+@test "load_platform_driver rejects slash in name" {
+    _enable_assertions
+    PLATFORM_DRIVER="foo/bar"
+    run load_platform_driver
+    assert_failure
+    assert_output --partial "Invalid PLATFORM_DRIVER"
+}
+
+@test "load_platform_driver rejects nonexistent driver" {
+    _enable_assertions
+    PLATFORM_DRIVER="nonexistent"
+    run load_platform_driver
+    assert_failure
+    assert_output --partial "not found"
+}
+
+# ===========================================================================
+# parse_ralphrc
+# ===========================================================================
+
+@test "parse_ralphrc sets known key" {
+    _enable_assertions
+    echo 'MAX_CALLS_PER_HOUR=50' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$MAX_CALLS_PER_HOUR" == "50" ]]
+}
+
+@test "parse_ralphrc handles double-quoted value" {
+    _enable_assertions
+    echo 'CLAUDE_OUTPUT_FORMAT="json"' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$CLAUDE_OUTPUT_FORMAT" == "json" ]]
+}
+
+@test "parse_ralphrc handles single-quoted value" {
+    _enable_assertions
+    echo "CLAUDE_OUTPUT_FORMAT='text'" > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$CLAUDE_OUTPUT_FORMAT" == "text" ]]
+}
+
+@test "parse_ralphrc handles \${VAR:-default} with env unset" {
+    _enable_assertions
+    unset PROJECT_NAME
+    echo 'PROJECT_NAME="${PROJECT_NAME:-my-project}"' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$PROJECT_NAME" == "my-project" ]]
+}
+
+@test "parse_ralphrc handles \${VAR:-default} with env set" {
+    _enable_assertions
+    export PROJECT_NAME="real-project"
+    echo 'PROJECT_NAME="${PROJECT_NAME:-my-project}"' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$PROJECT_NAME" == "real-project" ]]
+}
+
+@test "parse_ralphrc handles \${VAR:-} empty default" {
+    _enable_assertions
+    unset TEST_COMMAND
+    echo 'TEST_COMMAND="${TEST_COMMAND:-}"' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$TEST_COMMAND" == "" ]]
+}
+
+@test "parse_ralphrc allows semicolons in value" {
+    _enable_assertions
+    echo 'QUALITY_GATES="npm run lint;npm test"' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$QUALITY_GATES" == "npm run lint;npm test" ]]
+}
+
+@test "parse_ralphrc rejects command substitution" {
+    _enable_assertions
+    echo 'PROJECT_NAME=$(echo pwned)' > "$RALPH_DIR/test.conf"
+    MAX_CALLS_PER_HOUR=100
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    # PROJECT_NAME should not be set to the result of command substitution
+    [[ "${PROJECT_NAME:-}" != "pwned" ]]
+}
+
+@test "parse_ralphrc rejects backtick execution" {
+    _enable_assertions
+    echo 'PROJECT_NAME=`echo pwned`' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "${PROJECT_NAME:-}" != "pwned" ]]
+}
+
+@test "parse_ralphrc ignores unknown key with warning" {
+    _enable_assertions
+    echo 'UNKNOWN_KEY=foobar' > "$RALPH_DIR/test.conf"
+    run parse_ralphrc "$RALPH_DIR/test.conf"
+    assert_success
+    assert_output --partial "unknown key ignored"
+}
+
+@test "parse_ralphrc skips comments and blank lines" {
+    _enable_assertions
+    printf '# comment\n\nMAX_CALLS_PER_HOUR=42\n' > "$RALPH_DIR/test.conf"
+    parse_ralphrc "$RALPH_DIR/test.conf"
+    [[ "$MAX_CALLS_PER_HOUR" == "42" ]]
+}
+
+@test "RALPHRC_FILE env override is ignored" {
+    _enable_assertions
+    # After sourcing ralph_loop.sh, RALPHRC_FILE should be derived from RALPH_DIR
+    # not from an env var
+    [[ "$RALPHRC_FILE" == "$RALPH_DIR/.ralphrc" ]]
+}
+
 # ===========================================================================
 # load_ralphrc
 # ===========================================================================
diff --git a/tests/bash/write_heartbeat.bats b/tests/bash/write_heartbeat.bats
