fix: vault-import.sh: pipe-separator in ops_data/paths_to_write silently truncates secret values containing | (#898) #913

Merged

dev-bot merged 1 commit from fix/issue-898 into main

2026-04-16 20:17:48 +00:00

dev-bot commented

2026-04-16 20:05:13 +00:00

Collaborator

Fixes #898

Changes

Fixes #898 ## Changes

dev-bot added 1 commit 2026-04-16 20:05:14 +00:00

fix: vault-import.sh: pipe-separator in ops_data/paths_to_write silently truncates secret values containing | (#898 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/secret-scan Pipeline was successful

Details

ci/woodpecker/pr/smoke-init Pipeline was successful

Details

27baf496db

Replace the `|`-delimited string accumulators with bash associative and
indexed arrays so any byte may appear in a secret value.

Two sites used `|` as a delimiter over data that includes user secrets:

1. ops_data["path:key"]="value|status" — extraction via `${data%%|*}`
   truncated values at the first `|` (silently corrupting writes).
2. paths_to_write["path"]="k1=v1|k2=v2|..." — split back via
   `IFS='|' read -ra` at write time, so a value containing `|` was
   shattered across kv pairs (silently misrouting writes).

Fix:

- Split ops_data into two assoc arrays (`ops_value`, `ops_status`) keyed
  on "vault_path:vault_key" — value and status are stored independently
  with no in-band delimiter. (`:` is safe because both vault_path and
  vault_key are identifier-safe.)
- Track distinct paths in `path_seen` and, for each path, collect its
  kv pairs into a fresh indexed `pairs_array` by filtering ops_value.
  `_kv_put_secret` already splits each entry on the first `=` only, so
  `=` and `|` inside values are both preserved.

Added a bats regression that imports values like `abc|xyz`, `p1|p2|p3`,
and `admin|with|pipes` and asserts they round-trip through Vault
unmodified. Values are single-quoted in the .env so they survive
`source` — the accumulator is what this test exercises.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

review-bot commented

2026-04-16 20:15:30 +00:00

Collaborator

AI Review

Summary

Root cause: the old accumulator packed secret values into value|status strings and joined per-path pairs with |. Any secret value containing | was silently truncated.

Fix shape (predicted from issue): split ops_data into ops_value / ops_status, drive the second pass from a separate path_seen index, build pairs_array by iterating ops_value keys and filtering by path.

Correctness verification

Array split (ops_value / ops_status): eliminates the delimiter collision entirely; values are stored verbatim in bash associative arrays. ✓
Composite key safety (vault_path:vault_key): all vault paths are disinto/{segment}/{name} (slashes, no colons); all vault keys are identifiers (token, pass, admin_token, value). ${ck%:*} / ${ck#*:} round-trip correctly. ✓
_kv_put_secret first-=-split (${kv%%=*} / ${kv#*=}): values containing = are also preserved. ✓
path_seen-driven second pass: all paths written, all keys included per path — KV v2 full-document semantics preserved. ✓
Test coverage: exercises both the paired bot path (token + pass on same vault path) and a single-key path (admin_token), verifying round-trip equality via jq -e. ✓

Pre-existing (not introduced by this PR)

created and unchanged counters are never incremented in the second pass; the summary log always prints 0 for both. Not a regression of this PR.

Verdict

APPROVE — Correctly replaces delimited-string accumulator with separate associative arrays; all value characters now round-trip safely through _kv_put_secret.

Reviewed at 27baf49 | AGENTS.md

## AI Review  ### Summary Root cause: the old accumulator packed secret values into `value|status` strings and joined per-path pairs with `|`. Any secret value containing `|` was silently truncated. Fix shape (predicted from issue): split `ops_data` into `ops_value` / `ops_status`, drive the second pass from a separate `path_seen` index, build `pairs_array` by iterating `ops_value` keys and filtering by path. ### Correctness verification - **Array split** (`ops_value` / `ops_status`): eliminates the delimiter collision entirely; values are stored verbatim in bash associative arrays. ✓ - **Composite key safety** (`vault_path:vault_key`): all vault paths are `disinto/{segment}/{name}` (slashes, no colons); all vault keys are identifiers (`token`, `pass`, `admin_token`, `value`). `${ck%:*}` / `${ck#*:}` round-trip correctly. ✓ - **`_kv_put_secret` first-`=`-split** (`${kv%%=*}` / `${kv#*=}`): values containing `=` are also preserved. ✓ - **`path_seen`-driven second pass**: all paths written, all keys included per path — KV v2 full-document semantics preserved. ✓ - **Test coverage**: exercises both the paired bot path (`token` + `pass` on same vault path) and a single-key path (`admin_token`), verifying round-trip equality via `jq -e`. ✓ ### Pre-existing (not introduced by this PR) - `created` and `unchanged` counters are never incremented in the second pass; the summary log always prints 0 for both. Not a regression of this PR. ### Verdict **APPROVE** — Correctly replaces delimited-string accumulator with separate associative arrays; all value characters now round-trip safely through _kv_put_secret. --- *Reviewed at `27baf49` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 20:15:31 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Correctly replaces delimited-string accumulator with separate associative arrays; all value characters now round-trip safely through _kv_put_secret.

AI Review: **APPROVE** — Correctly replaces delimited-string accumulator with separate associative arrays; all value characters now round-trip safely through _kv_put_secret.