From 2a7afc34dd11796186d7ca01a6830d2d1124ce17 Mon Sep 17 00:00:00 2001 From: Brian Majewski Date: Fri, 5 Jun 2026 10:14:46 -0700 Subject: [PATCH] fix: pre-landing review fixes (heal ordering, key untrack, content-aware gitignore) Red-team + specialist findings from /ship pre-landing review: - CRITICAL: ensure_store_protections ran BEFORE `git pull --ff-only` in commit_and_push_secrets; a pull bringing history without .gitignore left the post-pull window unprotected and `git add -A` committed AND pushed key.txt (reproduced end-to-end). Heal now runs after the pull, immediately before staging. - CRITICAL: .gitignore can't untrack an already-tracked key.txt. Heal now defensively `git rm --cached`s a tracked key with a history-scrub warning. - Content-aware heal: a present .gitignore missing the key.txt line is rewritten, not skipped (security specialist). - Init guard now renders the real sanitized clone URL from .secrets-store when available, mirroring EGB-282's check_initialized. - Tests: positive heal-message assertions, non-empty key guard in the init test, 4 new regression tests (126 total). --- CLAUDE.md | 2 +- secrets | 27 ++++++++++++++++----- test/secrets.bats | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6882141..0a1a49f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 (118 tests) + secrets.bats # bats-core test suite (126 tests) test_helper.bash # Shared setup/teardown README.md # User-facing documentation CLAUDE.md # This file diff --git a/secrets b/secrets index b50122f..97d9640 100755 --- a/secrets +++ b/secrets @@ -724,10 +724,20 @@ EOF # pre-commit hook (hooks aren't cloned), and a half-initialized store may # lack .gitignore — without it, `git add -A` would commit key.txt. ensure_store_protections() { - if [ ! -f "$SECRETS_DIR/.gitignore" ]; then + # Content-aware: a present-but-corrupted .gitignore missing the key.txt + # line is just as dangerous as a missing one. + if [ ! -f "$SECRETS_DIR/.gitignore" ] || ! grep -qx 'key.txt' "$SECRETS_DIR/.gitignore"; then write_store_gitignore info "Restored store .gitignore" fi + # .gitignore can't untrack an already-tracked key (legacy damage, or a + # past window where .gitignore was missing). Remove it from the index so + # the next commit drops it from the tip. + if git -C "$SECRETS_DIR" ls-files --error-unmatch key.txt >/dev/null 2>&1; then + git -C "$SECRETS_DIR" rm --cached --quiet key.txt + echo "WARNING: key.txt was tracked in the store repo — untracked it now." >&2 + echo "It may still exist in git history; consider 'secrets rekey' and scrubbing history." >&2 + fi if [ ! -x "$SECRETS_DIR/.git/hooks/pre-commit" ]; then mkdir -p "$SECRETS_DIR/.git/hooks" install_hook @@ -750,10 +760,14 @@ cmd_init() { # should clone their existing secrets repo, not init a fresh one. # Catch it BEFORE git init so we don't leave a half-initialized store. if [ -f "$KEY_FILE" ]; then + # Render a runnable clone command when .secrets-store carried a remote + # URL (already sanitized by resolve_store), mirroring check_initialized. + local clone_src="" + [ -n "${_REMOTE_URL:-}" ] && clone_src="$_REMOTE_URL" die "Found an existing key at $KEY_FILE but no repo at $SECRETS_DIR. If this is a second machine, don't run 'secrets init' — clone your existing secrets repo instead: - git clone $SECRETS_DIR + git clone $clone_src $SECRETS_DIR Your key file has been left untouched." fi @@ -814,16 +828,17 @@ push_dir_to_project() { commit_and_push_secrets() { local message="$1" - # Must run before `git add -A`: a store missing its .gitignore would - # otherwise stage and push key.txt. - ensure_store_protections - if git -C "$SECRETS_DIR" remote get-url origin >/dev/null 2>&1; then if ! git -C "$SECRETS_DIR" pull --ff-only 2>/dev/null; then die "Fast-forward pull failed. Run 'secrets pull' first, then retry push." fi fi + # Must run AFTER the pull and immediately before `git add -A`: the pull + # can remove or alter .gitignore (remote history that lacks it), and a + # store missing the key.txt line would stage and push the private key. + ensure_store_protections + git -C "$SECRETS_DIR" add -A if git -C "$SECRETS_DIR" diff --cached --quiet 2>/dev/null; then info "No changes to push (secrets unchanged)" diff --git a/test/secrets.bats b/test/secrets.bats index 03c8138..f5565aa 100644 --- a/test/secrets.bats +++ b/test/secrets.bats @@ -1484,6 +1484,8 @@ gradle_project() { # `secrets init` instead of cloning their secrets repo. mkdir -p "$SECRETS_DIR" age-keygen -o "$SECRETS_DIR/key.txt" 2>/dev/null + # Guard against a vacuous '' = '' comparison if age-keygen failed + [ -s "$SECRETS_DIR/key.txt" ] local key_before key_before=$(cat "$SECRETS_DIR/key.txt") @@ -1503,6 +1505,7 @@ gradle_project() { run "$SECRETS_BIN" push [ "$status" -eq 0 ] + [[ "$output" == *"Restored store .gitignore"* ]] [ -f "$SECRETS_DIR/.gitignore" ] grep -q "key.txt" "$SECRETS_DIR/.gitignore" # key.txt must never be tracked (push does `git add -A` in the store) @@ -1560,6 +1563,7 @@ gradle_project() { run "$SECRETS_BIN" push [ "$status" -eq 0 ] + [[ "$output" == *"Reinstalled pre-commit hook"* ]] [ -x "$SECRETS_DIR/.git/hooks/pre-commit" ] } @@ -1597,3 +1601,59 @@ gradle_project() { grep -qF '!**/.env.*.age' "$SECRETS_DIR/.gitignore" grep -qF '!**/.dev.vars.age' "$SECRETS_DIR/.gitignore" } + +@test "push heals .gitignore removed by remote history before staging (key never pushed)" { + init_with_remote + create_project_dir + "$SECRETS_BIN" push >/dev/null 2>&1 + # Remote history drops .gitignore (e.g. an old machine committed without it) + git clone -q "$REMOTE_DIR" "$TEST_TMPDIR/other" + git -C "$TEST_TMPDIR/other" rm -q .gitignore + git -C "$TEST_TMPDIR/other" -c user.email=t@t -c user.name=t commit -qm "drop gitignore" + git -C "$TEST_TMPDIR/other" push -q + + echo "B=2" >> .env + run "$SECRETS_BIN" push + [ "$status" -eq 0 ] + [ -f "$SECRETS_DIR/.gitignore" ] + run git -C "$SECRETS_DIR" ls-files + [[ "$output" != *"key.txt"* ]] +} + +@test "push untracks a previously committed key.txt with a warning" { + init_with_remote + # Simulate legacy damage: key.txt got committed in the past + git -C "$SECRETS_DIR" add -f key.txt + git -C "$SECRETS_DIR" -c user.email=t@t -c user.name=t commit -qm "oops" + create_project_dir + + run "$SECRETS_BIN" push + [ "$status" -eq 0 ] + [[ "$output" == *"key.txt was tracked"* ]] + run git -C "$SECRETS_DIR" ls-files + [[ "$output" != *"key.txt"* ]] +} + +@test "push rewrites a store .gitignore that is missing the key.txt line" { + init_with_remote + printf '%s\n' '**/.env' > "$SECRETS_DIR/.gitignore" + create_project_dir + + run "$SECRETS_BIN" push + [ "$status" -eq 0 ] + grep -qx 'key.txt' "$SECRETS_DIR/.gitignore" + run git -C "$SECRETS_DIR" ls-files + [[ "$output" != *"key.txt"* ]] +} + +@test "init guard renders the real clone URL when .secrets-store carries a remote" { + mkdir -p "$HOME/.secrets-work" + age-keygen -o "$HOME/.secrets-work/key.txt" 2>/dev/null + [ -s "$HOME/.secrets-work/key.txt" ] + cd "$WORK_DIR" + echo "work git@example.com:me/secrets-work.git" > .secrets-store + + run "$SECRETS_BIN" init + [ "$status" -eq 1 ] + [[ "$output" == *"git clone git@example.com:me/secrets-work.git"* ]] +}