From 9d20acd35eef84dbb6536e916bc9d5089f318ee2 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 19 Nov 2025 10:55:01 +0000 Subject: [PATCH] fix(sensor-proxy): eliminate all uncoordinated config writers Remove all code paths that manipulate config files without Phase 2 locking: 1. Installer: Remove ensure_allowed_nodes_file_reference() call (line 1674) - Migration now handled exclusively by config migrate-to-file 2. Installer: Make migration failures fatal in update_allowed_nodes() - Prevents fallback to unsafe Python manipulation 3. Daemon sanitizer: Remove os.WriteFile() call - Now only sanitizes in-memory copy, doesn't write back to disk - Logs warning instructing admin to run `config migrate-to-file` 4. Self-heal script: Replace 132 lines of Python with CLI call - sanitize_allowed_nodes() now calls `config migrate-to-file` - Eliminates uncoordinated Python-based config rewriting All config mutations now flow exclusively through Phase 2 CLI with atomic operations and file locking. No code paths remain that can create duplicate allowed_nodes blocks. Addresses Codex review feedback on Phase 2 gaps. --- cmd/pulse-sensor-proxy/config.go | 21 ++--- scripts/install-sensor-proxy.sh | 140 +++---------------------------- 2 files changed, 21 insertions(+), 140 deletions(-) diff --git a/cmd/pulse-sensor-proxy/config.go b/cmd/pulse-sensor-proxy/config.go index 2cca5bc..e2147b0 100644 --- a/cmd/pulse-sensor-proxy/config.go +++ b/cmd/pulse-sensor-proxy/config.go @@ -545,20 +545,15 @@ func sanitizeDuplicateAllowedNodesBlocks(path string, data []byte) (bool, []byte buf.Write(data[last:]) cleaned := buf.Bytes() + + // Phase 2: DO NOT write the file back - that would create an uncoordinated writer + // The sanitizer only fixes the in-memory copy for this startup + // Admins should run `pulse-sensor-proxy config migrate-to-file` to fix the file atomically if path != "" { - if err := os.WriteFile(path, cleaned, 0600); err != nil { - log.Warn(). - Err(err). - Str("config_file", path). - Msg("Failed to rewrite sanitized configuration; using in-memory copy only") - } else { - // Phase 2: This sanitizer should never fire with the new config CLI - // If it does, it indicates a bug or manual file editing - log.Warn(). - Str("config_file", path). - Int("removed_duplicate_blocks", len(matches)-1). - Msg("CONFIG SANITIZED – duplicate allowed_nodes blocks removed; this should not happen with Phase 2 config CLI – please report if you see this") - } + log.Warn(). + Str("config_file", path). + Int("removed_duplicate_blocks", len(matches)-1). + Msg("CONFIG SANITIZED (in-memory only) – duplicate allowed_nodes blocks detected; run 'pulse-sensor-proxy config migrate-to-file' to fix the file permanently") } else { log.Warn(). Int("removed_duplicate_blocks", len(matches)-1). diff --git a/scripts/install-sensor-proxy.sh b/scripts/install-sensor-proxy.sh index 1917c5a..f0494d2 100755 --- a/scripts/install-sensor-proxy.sh +++ b/scripts/install-sensor-proxy.sh @@ -612,9 +612,10 @@ update_allowed_nodes() { local nodes=("$@") # Phase 2: Use config CLI exclusively - no shell/Python manipulation - # First, migrate any inline allowed_nodes to file mode - if "$BINARY_PATH" config migrate-to-file --config "$CONFIG_FILE" --allowed-nodes "$ALLOWED_NODES_FILE" 2>/dev/null; then - print_info "Migrated inline allowed_nodes to file mode" + # First, migrate any inline allowed_nodes to file mode (failures are fatal) + if ! "$BINARY_PATH" config migrate-to-file --config "$CONFIG_FILE" --allowed-nodes "$ALLOWED_NODES_FILE"; then + print_error "Failed to migrate config to file mode" + return 1 fi # Build --merge flags for the CLI @@ -1671,7 +1672,8 @@ EOF chmod 0644 /etc/pulse-sensor-proxy/config.yaml fi -ensure_allowed_nodes_file_reference +# Phase 2: Migration handled by update_allowed_nodes() -> config migrate-to-file +# No need to call ensure_allowed_nodes_file_reference anymore # Register socket-mode proxy with Pulse if server provided if [[ "$HTTP_MODE" != true ]]; then @@ -3104,135 +3106,19 @@ log() { } sanitize_allowed_nodes() { + # Phase 2: Use config CLI instead of Python manipulation if [[ ! -f "$CONFIG_FILE" ]]; then return fi - if ! command -v python3 >/dev/null 2>&1; then + + if [[ ! -x "$BINARY_PATH" ]]; then + log "Binary not available; skipping sanitization" return fi - local result - if ! result=$(python3 - "$CONFIG_FILE" <<'PY' -import sys -from pathlib import Path - -path = Path(sys.argv[1]) -lines = path.read_text().splitlines() -blocks = [] -i = 0 -while i < len(lines): - line = lines[i] - stripped = line.lstrip() - if stripped.startswith("allowed_nodes:"): - start = i - entries = [] - j = i + 1 - while j < len(lines): - nxt = lines[j] - nxt_stripped = nxt.lstrip() - if nxt_stripped.startswith("-"): - entries.append(nxt_stripped[1:].strip()) - j += 1 - continue - if ( - nxt_stripped.startswith("#") - or nxt_stripped == "" - or nxt.startswith((" ", "\t")) - ): - j += 1 - continue - break - - comment_indices = set() - comment_text = [] - k = start - 1 - while k >= 0 and lines[k].strip() == "": - comment_indices.add(k) - k -= 1 - while k >= 0 and lines[k].lstrip().startswith("#"): - comment_indices.add(k) - comment_text.append(lines[k]) - k -= 1 - comment_text.reverse() - - blocks.append( - { - "start": start, - "end": j, - "comment_indices": comment_indices, - "comment_text": comment_text, - "entries": entries, - } - ) - i = j - continue - i += 1 - -if len(blocks) <= 1: - sys.exit(0) - -seen = set() -merged = [] -for block in blocks: - for entry in block["entries"]: - key = entry.lower() - if not key or key in seen: - continue - seen.add(key) - merged.append(entry) - -if not merged: - sys.exit(0) - -first_block = blocks[0] -insert_at = min( - [first_block["start"]] + list(first_block["comment_indices"]) -) if first_block["comment_indices"] else first_block["start"] - -def build_comment(): - if first_block["comment_text"]: - return first_block["comment_text"] - return ["# Cluster nodes (auto-discovered during installation)"] - -comment_block = build_comment() -replacement = [] -replacement.extend(comment_block) -if replacement and replacement[-1].strip() != "": - replacement.append("") -replacement.append("allowed_nodes:") -for entry in merged: - replacement.append(f" - {entry}") -replacement.append("") - -indices_to_remove = set() -for block in blocks: - indices_to_remove.update(range(block["start"], block["end"])) - indices_to_remove.update(block["comment_indices"]) - -result = [] -inserted = False -for idx, line in enumerate(lines): - if not inserted and idx == insert_at: - result.extend(replacement) - inserted = True - if idx in indices_to_remove: - continue - result.append(line) - -if not inserted: - if result and result[-1].strip() != "": - result.append("") - result.extend(replacement) - -path.write_text("\n".join(result).rstrip() + "\n") -print(f"Sanitized duplicate allowed_nodes blocks ({len(blocks) - 1} removed)") -PY - ); then - return - fi - - if [[ -n "$result" ]]; then - log "$result" + # Use CLI to atomically migrate any inline blocks to file mode + if "$BINARY_PATH" config migrate-to-file --config "$CONFIG_FILE" 2>&1 | grep -q "Migration complete"; then + log "Migrated inline allowed_nodes to file mode" fi }