From 1ee096cc3303530dde3265fe7d36d4767c5ff0b6 Mon Sep 17 00:00:00 2001 From: Brian Majewski Date: Thu, 18 Jun 2026 10:08:13 -0700 Subject: [PATCH] refactor: dedup external extractor + read guards, warn on legacy-pull nested blobs (EGB-701) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EGB-677 stage-1 structural cleanups, no behavior change for the v2 happy path: 1. cmd_which reuses _json_external_entries (the push/pull extractor) instead of its own duplicated jq @tsv projection, so `which` applies the same properties->gradle-properties normalization + skip-with-warning rules the sync path does and can't drift from it. 2. The two external-manifest read guards are factored into _json_readable (plain regular file, silent) / _legacy_readable (warn+skip a symlinked .secrets-files), shared by _external_entries_for_push/_pull. 3. The legacy (manifest-less) pull path now warns when nested /.age blobs exist that its non-recursive globs can't see (external/ excluded — pull_external_files handles those), so it never silently under-restores. Tests: +4 in test/manifest.bats (normalized which display, malformed external skipped by which, nested-blob warning fires, external-only no false warning). Full suite green (286/286). Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 6 ++-- secrets | 84 +++++++++++++++++++++++++++++++++++----------- test/manifest.bats | 65 +++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 608040a..bd2954f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,8 +76,8 @@ secrets # CLI script (~2000 lines bash) hooks/pre-commit # Pre-commit hook template test/ secrets.bats # bats-core test suite (140 tests) - manifest.bats # EGB-677 .secrets.json manifest tests (78 tests) - migrate.bats # EGB-703 store-format-v2 migration tests (26 tests) + manifest.bats # EGB-677 .secrets.json manifest tests (83 tests) + migrate.bats # EGB-703 store-format-v2 migration tests (35 tests) test_helper.bash # Shared setup/teardown README.md # User-facing documentation CLAUDE.md # This file @@ -107,7 +107,7 @@ 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//external/.gradle-properties.age`. The `external/` subdir keeps them out of the legacy non-recursive `*.age` / `.*.age` globs the dotenv `pull` path uses, so a dotenv pull can never decrypt an external blob into cwd. `cmd_rekey` and `cmd_list` instead walk the **entire** project tree (`find -type f`), so they cover both `external/.age` and nested manifest dotenv blobs (`/.age`) — rekey MUST recurse, or any nested/external blob is orphaned under the old key after rotation = data loss (EGB-677 regression test: "rekey re-encrypts a nested manifest dotenv blob"). `` = 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). `cmd_list --json` (EGB-699) emits the same recursive walk as a machine-readable object (`{store, projects[].entries[]}`, each entry `dotenv`→`path` or `external`→`subtype`+`path`) for tooling/CI (feeds EGB-671); jq assembles it so paths escape correctly and stdout stays pure JSON (the human store hint is suppressed; jq is a hard dep only in `--json` mode). +- **Storage:** blobs live in `$SECRETS_DIR//external/.gradle-properties.age`. The `external/` subdir keeps them out of the legacy non-recursive `*.age` / `.*.age` globs the dotenv `pull` path uses, so a dotenv pull can never decrypt an external blob into cwd. `cmd_rekey` and `cmd_list` instead walk the **entire** project tree (`find -type f`), so they cover both `external/.age` and nested manifest dotenv blobs (`/.age`) — rekey MUST recurse, or any nested/external blob is orphaned under the old key after rotation = data loss (EGB-677 regression test: "rekey re-encrypts a nested manifest dotenv blob"). **EGB-701 cleanups:** (1) the *legacy* (manifest-less) `pull` keeps its non-recursive globs but now **warns** when nested `/.age` blobs exist that those globs can't see (it excludes `external/`, which `pull_external_files` handles) — so a manifest-less pull never silently under-restores; the fix the warning points at is committing a `.secrets.json`. (2) `cmd_which`, push, and pull share one external extractor (`_json_external_entries`), so `which` applies the same `properties`→`gradle-properties` normalization and skip-with-warning rules the sync path does (it shows exactly what will sync, not a stale raw projection). (3) the two external-manifest read guards are factored into `_json_readable` (plain regular file, silent) / `_legacy_readable` (warn-and-skip on a symlinked legacy manifest). `` = 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). `cmd_list --json` (EGB-699) emits the same recursive walk as a machine-readable object (`{store, projects[].entries[]}`, each entry `dotenv`→`path` or `external`→`subtype`+`path`) for tooling/CI (feeds EGB-671); jq assembles it so paths escape correctly and stdout stays pure JSON (the human store hint is suppressed; jq is a hard dep only in `--json` mode). - **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 `.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). diff --git a/secrets b/secrets index 219cfbf..2d55aa7 100755 --- a/secrets +++ b/secrets @@ -1038,6 +1038,31 @@ _json_external_entries() { done < <(jq -r '.external // [] | .[] | [.type, .path, ((.keys // []) | join(" "))] | @tsv' "$manifest") } +# EGB-701 item 2: the read guards for the two external-manifest sources, +# factored out of _external_entries_for_push/_pull so they can't drift. +# +# _json_readable — true when a .secrets.json is a safe regular file to read. +# A symlinked manifest is treated as absent and silently ignored: it's the +# project's own committed file, so a symlink there is just skipped (the fatal +# symlink refusal lives in _check_manifest_file, used by the linting paths). +_json_readable() { + [ -f "$1" ] && [ ! -L "$1" ] +} + +# _legacy_readable — true when a legacy .secrets-files is a safe regular file +# to read, warning (and returning false) when it exists but is a symlink: a +# symlinked legacy manifest's target is attacker-influenceable, so never follow +# it. A missing or non-regular file returns false silently. +_legacy_readable() { + local legacy="$1" + [ -e "$legacy" ] || return 1 + if [ -L "$legacy" ]; then + echo "WARNING: $legacy is a symlink; ignoring." >&2 + return 1 + fi + [ -f "$legacy" ] +} + # External tuples for PUSH: .secrets.json entries first, then legacy # .secrets-files entries whose (type, path) the manifest doesn't cover — # the absorb set, which cmd_push folds into the manifest after a @@ -1046,23 +1071,19 @@ _external_entries_for_push() { local root="$1" local json="$root/$SECRETS_JSON_NAME" legacy="$root/$SECRETS_FILES_NAME" local seen="" t p k - if [ -f "$json" ] && [ ! -L "$json" ]; then + if _json_readable "$json"; then while IFS=$'\t' read -r t p k; do [ -n "$t" ] || continue printf '%s\t%s\t%s\n' "$t" "$p" "$k" seen="$seen$t|$p"$'\n' done < <(_json_external_entries "$json") fi - if [ -e "$legacy" ]; then - if [ -L "$legacy" ]; then - echo "WARNING: $legacy is a symlink; ignoring." >&2 - elif [ -f "$legacy" ]; then - while IFS=$'\t' read -r t p k; do - [ -n "$t" ] || continue - case "$seen" in *"$t|$p"$'\n'*) continue ;; esac - printf '%s\t%s\t%s\n' "$t" "$p" "$k" - done < <(_parse_secrets_files_manifest "$legacy") - fi + if _legacy_readable "$legacy"; then + while IFS=$'\t' read -r t p k; do + [ -n "$t" ] || continue + case "$seen" in *"$t|$p"$'\n'*) continue ;; esac + printf '%s\t%s\t%s\n' "$t" "$p" "$k" + done < <(_parse_secrets_files_manifest "$legacy") fi } @@ -1071,19 +1092,18 @@ _external_entries_for_push() { _external_entries_for_pull() { local root="$1" local json="$root/$SECRETS_JSON_NAME" legacy="$root/$SECRETS_FILES_NAME" - if [ -f "$json" ] && [ ! -L "$json" ]; then - if [ -f "$legacy" ] && [ ! -L "$legacy" ]; then + if _json_readable "$json"; then + # A regular (non-symlink) legacy file alongside the manifest is superseded: + # warn but don't read it. _json_readable is the "plain regular file" test — + # exactly the supersede condition (and unlike _legacy_readable it stays + # silent on a symlink, matching the original no-warn-on-symlink behavior). + if _json_readable "$legacy"; then echo "WARNING: $legacy is superseded by $SECRETS_JSON_NAME and was ignored on pull. Run 'secrets push' to absorb it, then delete it." >&2 fi _json_external_entries "$json" return 0 fi - [ -e "$legacy" ] || return 0 - if [ -L "$legacy" ]; then - echo "WARNING: $legacy is a symlink; ignoring." >&2 - return 0 - fi - [ -f "$legacy" ] && _parse_secrets_files_manifest "$legacy" + _legacy_readable "$legacy" && _parse_secrets_files_manifest "$legacy" return 0 } @@ -1838,6 +1858,26 @@ cmd_pull() { info "Decrypted $count file(s) into $target_dir" + # EGB-701 item 3: the globs above are non-recursive, so a nested dotenv blob + # (/.age) written by a manifest-driven push on another + # machine is invisible here — silently restored nothing, counted nothing. + # external/.age blobs are restored by pull_external_files, so exclude + # them. Warn (don't die) so a manifest-less pull never under-restores in + # silence; the fix is a committed .secrets.json, which the recursive + # manifest-driven branch above handles correctly. + local nested + nested=$(find "$SECRETS_DIR/$project" -mindepth 2 -type f -name '*.age' \ + -not -path "$SECRETS_DIR/$project/external/*" 2>/dev/null) + if [ -n "$nested" ]; then + echo "WARNING: this project has nested encrypted files the manifest-less pull can't restore:" >&2 + while IFS= read -r nf; do + [ -n "$nf" ] || continue + local rel="${nf#"$SECRETS_DIR/$project/"}" + echo " ${rel%.age}" >&2 + done <<< "$nested" + echo " Add a $SECRETS_JSON_NAME manifest (run 'secrets push' on a machine that has these files) so they restore." >&2 + fi + # Merge any external files (.secrets-files) declared in this project. pull_external_files "$PWD" "$project" @@ -2307,11 +2347,15 @@ cmd_which() { echo " dotenv $entry [UNSAFE — will be refused]" fi done < <(jq -r '.dotenv // [] | .[]' "$json_manifest") + # EGB-701 item 1: reuse the one external extractor the sync path uses, + # so `which` applies the same normalization + skip-with-warning rules + # push/pull do — `which` shows exactly what will sync, never a stale + # raw projection that drifts from the helper. local etype epath ekeys while IFS=$'\t' read -r etype epath ekeys; do [ -n "$etype" ] || continue echo " $etype $epath $ekeys" - done < <(jq -r '.external // [] | .[] | [.type, .path, ((.keys // []) | join(" "))] | @tsv' "$json_manifest") + done < <(_json_external_entries "$json_manifest") fi # Read back any external-file manifest in cwd (validates the format and diff --git a/test/manifest.bats b/test/manifest.bats index ce16125..f7aeab9 100644 --- a/test/manifest.bats +++ b/test/manifest.bats @@ -642,6 +642,31 @@ m_nojq_path() { [[ "$output" == *"k1"* ]] || false } +# EGB-701 item 1: `which` and the push/pull external extractor share one +# helper (_json_external_entries), so `which` applies the same +# properties→gradle-properties normalization the sync path uses — no drift. +@test "which normalizes a properties external to the gradle-properties token (EGB-701)" { + create_project_dir whichnorm + printf '{"version":2,"dotenv":[".env"],"external":[{"type":"properties","path":"~/.gradle/gradle.properties","keys":["k1"]}]}\n' > .secrets.json + run "$SECRETS_BIN" which + [ "$status" -eq 0 ] + [[ "$output" == *"gradle-properties"* ]] || false +} + +# EGB-701 item 1: a malformed external (a properties entry with no keys) is +# skipped by the sync path; routing `which` through the shared extractor means +# `which` skips+warns it too, so it faithfully shows what actually syncs +# rather than printing an entry push/pull silently drop. +@test "which skips a malformed external entry the sync path would drop (EGB-701)" { + create_project_dir whichmalformed + printf '{"version":2,"dotenv":[".env"],"external":[{"type":"properties","path":"~/.gradle/gradle.properties"}]}\n' > .secrets.json + run "$SECRETS_BIN" which + [ "$status" -eq 0 ] + [[ "$output" == *"has no keys"* ]] || false + # The skipped entry's path must NOT appear in the printed manifest summary. + [[ "$output" != *" gradle-properties ~/.gradle/gradle.properties"* ]] || false +} + # ─── F: ship Step 7 coverage backfill (audit gaps) ───────────────────── @test "which flags an unsafe dotenv entry with the UNSAFE marker" { @@ -739,6 +764,46 @@ m_nojq_path() { [ "$(cat packages/web/.env.development)" = "N=nested" ] } +@test "legacy (manifest-less) pull warns about nested blobs it can't restore (EGB-701)" { + # The legacy pull path globs only top-level *.age/.*.age. A nested dotenv + # blob (/.age) written by a manifest-driven push on another + # machine is invisible to those globs — restored nothing, counted nothing. + # The fix: warn so a manifest-less pull never silently under-restores. + init_with_remote + create_project_dir nestlegacy + mkdir -p packages/web + echo "N=nested" > packages/web/.env.development + "$SECRETS_BIN" add packages/web/.env.development >/dev/null + "$SECRETS_BIN" push >/dev/null 2>&1 + [ -f "$SECRETS_DIR/nestlegacy/packages/web/.env.development.age" ] + # Simulate a machine with no manifest: drop .secrets.json + local files, + # forcing the legacy non-recursive glob branch. + rm -f .secrets.json + rm -rf packages + run "$SECRETS_BIN" pull nestlegacy + [ "$status" -eq 0 ] + # The warning names the nested blob and points at the manifest as the fix. + [[ "$output" == *"packages/web/.env.development"* ]] || false + [[ "$output" == *"$SECRETS_JSON_NAME"* || "$output" == *".secrets.json"* ]] || false + # The legacy path genuinely can't restore it (the warning is the contract). + [ ! -f packages/web/.env.development ] +} + +@test "legacy pull does NOT warn about external/ blobs (handled separately, EGB-701)" { + # external/.age blobs are restored by pull_external_files, not the + # dotenv globs, so they must not trip the nested-blob warning. + init_with_remote + m_gradle_src $'beaconClerkPkTest=pk_test_abc\n' + create_project_dir extnolwarn + printf 'gradle-properties ~/.gradle/gradle.properties beaconClerkPkTest\n' > .secrets-files + "$SECRETS_BIN" push >/dev/null 2>&1 + [ -d "$SECRETS_DIR/extnolwarn/external" ] + run "$SECRETS_BIN" pull extnolwarn + [ "$status" -eq 0 ] + [[ "$output" != *"can't restore"* ]] || false + [[ "$output" != *"nested encrypted"* ]] || false +} + @test "list shows a nested manifest blob" { init_with_remote create_project_dir nestlist