fix: pre-landing review fixes for external-file sync (EGB-531)

Adversarial review of the diff surfaced 6 non-critical findings, all fixed:
- pull now warns + reports when a merge fails (read-only $HOME, full disk)
  instead of silently exiting 0 as if it synced
- push skips a multi-line (continuation) managed value with a warning rather
  than writing a dangling backslash that would corrupt the target
- absent-key pull check escapes regex '.' so systemProp.foo can't false-match
- fix garbled 'Merged 0\n0 key(s)' count when a blob has no '=' lines
- slug gets a cksum suffix so paths that clean to the same string (a/b vs a_b)
  don't overwrite each other's blob
- docs: backup happens before each merge, not just the first

Tests: 112 -> 113 (continuation-value skip).
This commit is contained in:
Brian Majewski 2026-05-26 14:40:11 -07:00
parent 31f2741c44
commit ffdff4dafb
5 changed files with 42 additions and 13 deletions

View file

@ -37,7 +37,7 @@ Single bash script (`secrets`) with subcommands: init, push, pull, list, rm, rek
secrets # CLI script (~600 lines bash)
hooks/pre-commit # Pre-commit hook template
test/
secrets.bats # bats-core test suite (112 tests)
secrets.bats # bats-core test suite (113 tests)
test_helper.bash # Shared setup/teardown
README.md # User-facing documentation
CLAUDE.md # This file
@ -67,8 +67,8 @@ The active store directory is picked by `resolve_store()` using these rules, hig
Key design decisions (all driven by /autoplan review):
- **Wire-in is at command scope** (`cmd_push`/`cmd_pull`), via `push_external_files` / `pull_external_files`, **not** inside `push_dir_to_project` / `pull_project_to_dir` (those loop per-workspace and `pull_project_to_dir` uses stdout as a data channel).
- **Storage:** blobs live in `$SECRETS_DIR/<project>/external/<slug>.gradle-properties.age`. The `external/` subdir keeps them out of the existing non-recursive `*.age` / `.*.age` globs (pull, list, rekey), so the old dotenv path can never decrypt a blob into cwd. `cmd_rekey` and `cmd_list` recurse into `external/` explicitly (rekey MUST, or the blob is orphaned after rotation = data loss). `<slug>` = manifest path token with non-`[A-Za-z0-9._-]` chars → `_`, so it's machine-independent.
- **Merge is pure bash, no `sed`/regex** (`merge_gradle_keys`): exact-string key comparison (avoids `beaconClerkPk` vs `beaconClerkPkTest` substring bug), value treated as opaque literal (survives `& \ /` in values). Updates a managed key in place at its first occurrence, collapses duplicates, appends new keys, preserves unrelated lines/comments/order. Continuation lines (trailing odd backslashes, tracked by `_trailing_bs_odd`) are never matched as keys. Atomic write: temp in the same dir → `chmod` to match (or `600` on create) → `mv`. Backs up to `<target>.secrets-bak` before the first merge.
- **Storage:** blobs live in `$SECRETS_DIR/<project>/external/<slug>.gradle-properties.age`. The `external/` subdir keeps them out of the existing non-recursive `*.age` / `.*.age` globs (pull, list, rekey), so the old dotenv path can never decrypt a blob into cwd. `cmd_rekey` and `cmd_list` recurse into `external/` explicitly (rekey MUST, or the blob is orphaned after rotation = data loss). `<slug>` = manifest path token with non-`[A-Za-z0-9._-]` chars → `_`, plus a `cksum` suffix of the original path so paths that clean to the same string (`a/b` vs `a_b`) don't collide. Machine-independent (derived from the committed manifest token, not the expanded path).
- **Merge is pure bash, no `sed`/regex** (`merge_gradle_keys`): exact-string key comparison (avoids `beaconClerkPk` vs `beaconClerkPkTest` substring bug), value treated as opaque literal (survives `& \ /` in values). Updates a managed key in place at its first occurrence, collapses duplicates, appends new keys, preserves unrelated lines/comments/order. Continuation lines (trailing odd backslashes, tracked by `_trailing_bs_odd`) are never matched as keys. Atomic write: temp in the same dir → `chmod` to match (or `600` on create) → `mv`. Backs up to `<target>.secrets-bak` before each merge.
- **Properties separator parsing** (`_props_get`): key ends at the first `=`, `:`, or whitespace (after lstrip); handles `key=value`, `key = value`, `key:value`, `key value`; last definition wins.
- **Security:** the write target comes from a committed file, so `_validate_external_target_path` locks it down — basename must be `gradle.properties`, must resolve inside `$HOME` (deepest-existing-ancestor resolved, symlink target/parent refused, `..` rejected). This blocks a malicious manifest from appending decrypted keys to `~/.gitconfig`/`~/.bashrc`. `_parse_secrets_files_manifest` rejects shell metacharacters/control chars in path and keys (path allows `[A-Za-z0-9/._~-]` only; keys allow `[A-Za-z0-9._-]` + space), mirrors the `.secrets-store` posture (no shell expansion, symlinked manifest skipped).
- **Plaintext tradeoff (accepted, documented):** merged keys are permanent plaintext in the target; `secrets clear` does not remove them. Fine for the Clerk *publishable* keys this was built for; not for high-value secrets (use `secrets run` + `.env`).