Merge pull request 'fix: vault-import.sh: pipe-separator in ops_data/paths_to_write silently truncates secret values containing | (#898)' (#913) from fix/issue-898 into main

tests/vault-import.bats

@@ -199,6 +199,46 @@ setup() {
   echo "$output" | jq -e '.data.data.token == "MODIFIED-LLAMA-TOKEN"'
 }
 
+# --- Delimiter-in-value regression (#898) ────────────────────────────────────
+
+@test "preserves secret values that contain a pipe character" {
+  # Regression: previous accumulator packed values into "value|status" and
+  # joined per-path kv pairs with '|', so any value containing '|' was
+  # silently truncated or misrouted.
+  local piped_env="${BATS_TEST_TMPDIR}/dot-env-piped"
+  cp "$FIXTURES_DIR/dot-env-complete" "$piped_env"
+
+  # Swap in values that contain the old delimiter. Exercise both:
+  #  - a paired bot path (token + pass on same vault path, hitting the
+  #    per-path kv-pair join)
+  #  - a single-key path (admin token)
+  # Values are single-quoted so they survive `source` of the .env file;
+  # `|` is a shell metachar and unquoted would start a pipeline. That is
+  # orthogonal to the accumulator bug under test — users are expected to
+  # quote such values in .env, and the accumulator must then preserve them.
+  sed -i "s#^FORGE_REVIEW_TOKEN=.*#FORGE_REVIEW_TOKEN='abc|xyz'#" "$piped_env"
+  sed -i "s#^FORGE_REVIEW_PASS=.*#FORGE_REVIEW_PASS='p1|p2|p3'#" "$piped_env"
+  sed -i "s#^FORGE_ADMIN_TOKEN=.*#FORGE_ADMIN_TOKEN='admin|with|pipes'#" "$piped_env"
+
+  run "$IMPORT_SCRIPT" \
+    --env "$piped_env" \
+    --sops "$FIXTURES_DIR/.env.vault.enc" \
+    --age-key "$FIXTURES_DIR/age-keys.txt"
+  [ "$status" -eq 0 ]
+
+  # Verify each value round-trips intact.
+  run curl -sf -H "X-Vault-Token: ${VAULT_TOKEN}" \
+    "${VAULT_ADDR}/v1/secret/data/disinto/bots/review"
+  [ "$status" -eq 0 ]
+  echo "$output" | jq -e '.data.data.token == "abc|xyz"'
+  echo "$output" | jq -e '.data.data.pass == "p1|p2|p3"'
+
+  run curl -sf -H "X-Vault-Token: ${VAULT_TOKEN}" \
+    "${VAULT_ADDR}/v1/secret/data/disinto/shared/forge"
+  [ "$status" -eq 0 ]
+  echo "$output" | jq -e '.data.data.admin_token == "admin|with|pipes"'
+}
+
 # --- Incomplete fixture ───────────────────────────────────────────────────────
 
 @test "handles incomplete fixture gracefully" {

tools/vault-import.sh

@@ -421,13 +421,21 @@ EOF
   local updated=0
   local unchanged=0
 
-  # First pass: collect all operations with their parsed values
-  # Store as: ops_data["vault_path:kv_key"] = "source_value|status"
-  declare -A ops_data
+  # First pass: collect all operations with their parsed values.
+  # Store value and status in separate associative arrays keyed by
+  # "vault_path:kv_key". Secret values may contain any character, so we
+  # never pack them into a delimited string — the previous `value|status`
+  # encoding silently truncated values containing '|' (see issue #898).
+  declare -A ops_value
+  declare -A ops_status
+  declare -A path_seen
 
   for op in "${operations[@]}"; do
     # Parse operation: category|field|subkey|file|envvar (5 fields for bots/runner)
-    # or category|field|file|envvar (4 fields for forge/woodpecker/chat)
+    # or category|field|file|envvar (4 fields for forge/woodpecker/chat).
+    # These metadata strings are built from safe identifiers (role names,
+    # env-var names, file paths) and do not carry secret values, so '|' is
+    # still fine as a separator here.
     local category field subkey file envvar=""
     local field_count
     field_count="$(printf '%s' "$op" | awk -F'|' '{print NF}')"
@@ -494,51 +502,40 @@ EOF
       fi
     fi
 
-    # Store operation data: key = "vault_path:kv_key", value = "source_value|status"
-    ops_data["${vault_path}:${vault_key}"]="${source_value}|${status}"
+    # vault_path and vault_key are identifier-safe (no ':' in either), so
+    # the composite key round-trips cleanly via ${ck%:*} / ${ck#*:}.
+    local ck="${vault_path}:${vault_key}"
+    ops_value["$ck"]="$source_value"
+    ops_status["$ck"]="$status"
+    path_seen["$vault_path"]=1
   done
 
-  # Second pass: group by vault_path and write
+  # Second pass: group by vault_path and write.
   # IMPORTANT: Always write ALL keys for a path, not just changed ones.
   # KV v2 POST replaces the entire document, so we must include unchanged keys
   # to avoid dropping them. The idempotency guarantee comes from KV v2 versioning.
-  declare -A paths_to_write
-  declare -A path_has_changes
+  for vault_path in "${!path_seen[@]}"; do
+    # Collect this path's "vault_key=source_value" pairs into a bash
+    # indexed array. Each element is one kv pair; '=' inside the value is
+    # preserved because _kv_put_secret splits on the *first* '=' only.
+    local pairs_array=()
+    local path_has_changes=0
 
-  for key in "${!ops_data[@]}"; do
-    local data="${ops_data[$key]}"
-    local source_value="${data%%|*}"
-    local status="${data##*|}"
-    local vault_path="${key%:*}"
-    local vault_key="${key#*:}"
+    for ck in "${!ops_value[@]}"; do
+      [ "${ck%:*}" = "$vault_path" ] || continue
+      local vault_key="${ck#*:}"
+      pairs_array+=("${vault_key}=${ops_value[$ck]}")
+      if [ "${ops_status[$ck]}" != "unchanged" ]; then
+        path_has_changes=1
+      fi
+    done
 
-    # Always add to paths_to_write (all keys for this path)
-    if [ -z "${paths_to_write[$vault_path]:-}" ]; then
-      paths_to_write[$vault_path]="${vault_key}=${source_value}"
-    else
-      paths_to_write[$vault_path]="${paths_to_write[$vault_path]}|${vault_key}=${source_value}"
-    fi
-
-    # Track if this path has any changes (for status reporting)
-    if [ "$status" != "unchanged" ]; then
-      path_has_changes[$vault_path]=1
-    fi
-  done
-
-  # Write each path with all its key-value pairs
-  for vault_path in "${!paths_to_write[@]}"; do
     # Determine effective status for this path (updated if any key changed)
     local effective_status="unchanged"
-    if [ "${path_has_changes[$vault_path]:-}" = "1" ]; then
+    if [ "$path_has_changes" = 1 ]; then
       effective_status="updated"
     fi
 
-    # Read pipe-separated key-value pairs and write them
-    local pairs_string="${paths_to_write[$vault_path]}"
-    local pairs_array=()
-    local IFS='|'
-    read -r -a pairs_array <<< "$pairs_string"
-
     if ! _kv_put_secret "$vault_path" "${pairs_array[@]}"; then
       _err "Failed to write to $vault_path"
       exit 1