refactor: dedup external extractor + read guards, warn on legacy-pull nested blobs (EGB-701)
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 <project>/<relpath>.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) <noreply@anthropic.com>
This commit is contained in:
Brian Majewski
parent
57f1280262
commit
1ee096cc33
3 changed files with 132 additions and 23 deletions
|
|
@ -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/<project>/external/<slug>.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/<slug>.age` and nested manifest dotenv blobs (`<project>/<relpath>.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"). `<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). `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/<project>/external/<slug>.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/<slug>.age` and nested manifest dotenv blobs (`<project>/<relpath>.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 `<project>/<relpath>.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). `<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). `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 `<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).
|
||||
|
|
|
|||
74
secrets
74
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,24 +1071,20 @@ _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
|
||||
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
|
||||
fi
|
||||
}
|
||||
|
||||
# External tuples for PULL: the manifest wins entirely when present;
|
||||
|
|
@ -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
|
||||
# (<project>/<relpath>.age) written by a manifest-driven push on another
|
||||
# machine is invisible here — silently restored nothing, counted nothing.
|
||||
# external/<slug>.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
|
||||
|
|
|
|||
|
|
@ -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 (<project>/<relpath>.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/<slug>.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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue