diff --git a/AGENTS.md b/AGENTS.md index a87e00a..1448685 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,10 +59,12 @@ on the internal packages, and the full eval suite against a real Linux binary. `docs/`) stays host-agnostic. The one sanctioned place to name the AI assistant is attribution: the "Built with Claude Code" badge in `README.md`. Private, unpublished tool names never appear in any committed file. -- **Commits stay clean.** No `Co-Authored-By:` or `Assisted-by:` trailers for - AI, no "Generated with" lines, no session links. The human is the author of - record, responsible for all AI-assisted output — its correctness, licensing, - and fit for the project. +- **AI-assisted commits carry an `Assisted-by:` trailer**, kernel format + (`Assisted-by: Claude:claude-fable-5`) — disclosure, not authorship. Never + `Co-Authored-By:` for AI (it asserts an authorship the tool does not hold and + inflates the contributor graph). No "Generated with" lines or session links. + The human is the author of record, responsible for all AI-assisted output — + its correctness, licensing, and fit for the project. ## Working memory diff --git a/CHANGELOG.md b/CHANGELOG.md index 79843dc..7e7498f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,60 @@ called out in a **Breaking** section. See ## [Unreleased] +### Added + +- **`ferry restore --undo `** re-applies the pre-restore snapshot + ferry takes before every restore, making the "undo is possible" the restore + output has always advertised actually reachable. The post-restore message now + prints the exact command to run. It takes the apply lock and registers the + terminal preference domains, exactly as a restore does. + +### Security + +- **The secret scanner closes several credential-in-repo bypasses.** The scanner + is ferry's automated gate keeping plaintext credentials out of the shared, + pushed repo; these gaps let real secrets through it: + - Quoted multi-word values (`password = "correct horse battery"`) were only + checked up to the first space — the rest slipped through. The whole quoted + value is now scanned. + - Literal secrets beginning with `$` (a leetspeak password, not a variable + reference) or containing an interior `<` / `...` were exempted as + placeholders. Only genuine `${VAR}` / `$(cmd)` / whole-value `$VAR` + references and `<...>` templates are now exempt. + - `Authorization: Bearer ` / `Basic ` header values are now + detected in generic dotfiles (`~/.curlrc`, custom tool configs). + - Credential assignments beyond the first line of an opaque, multi-line value + (e.g. a `defaults export` plist blob) are now scanned. + - More credential key names are recognised (`pass`, `passphrase`, `pwd`, + `credentials`, `*_key`), while path-valued keys (`*_file = /path`) are + excluded to preserve the no-false-positive discipline. + - Bundle members that are binary are now scanned for provider tokens + (GitHub/AWS/GCP/Slack/OpenAI/Stripe) in addition to private-key headers, + symmetric on export and import. +- **The apply lock is published atomically** (temp file + fsync + hardlink), so a + concurrent acquirer can no longer reclaim a live owner's half-written lockfile + and run a second `apply` in parallel. +- **The init-time GitHub push runs under the same untrusted-transport hardening** + as every other ferry git invocation (`protocol.ext.allow=never`, hooks / + fsmonitor / ssh neutralised), so an ambient `pushInsteadOf` rewrite can no + longer redirect it. + +### Fixed + +- **Capturing a file with no trailing newline no longer wedges it** in a + permanent, unresolvable conflict — the captured composition now preserves the + live file's final-newline shape. +- **Crash-safety of ferry's own state.** Journal/snapshot backup blobs and the + dotfile and agents state files are now fsync'd before the metadata that + references them, so a power loss can no longer roll a truncated backup over an + intact file or wedge every command behind a zero-length state file; a + zero-length state file now degrades to first-touch instead of hard-failing. +- **`ferry sync` no longer silently skips remote integration** when the + ahead/behind computation fails — it rolls back with a clear error instead of + reporting "up to date". +- **Custom `ferry bundle import --out` targets are symlink-hardened** against a + redirected parent chain, matching the default target. + ## [0.8.0] - 2026-07-08 ### Breaking diff --git a/cmd/capture.go b/cmd/capture.go index 51d8d19..cb2d3ed 100644 --- a/cmd/capture.go +++ b/cmd/capture.go @@ -363,7 +363,7 @@ func captureOne(cc captureCtx) (bool, error) { } // Compose the captured content = repo content with accepted hunks applied. - captured := applyHunks(string(cc.repoBytes), hunks, accepted) + captured := applyHunks(string(cc.repoBytes), hunks, accepted, endsWithNewline(cc.liveBytes)) spanPatched := false var storedRefs []string // refs Put by the span consent (for honest refusal notices) @@ -893,7 +893,7 @@ func captureZshSidecar(cc captureCtx, home string) (wrote bool, offered bool, er } } - composed := applyHunks(string(repoBytes), hunks, accepted) + composed := applyHunks(string(repoBytes), hunks, accepted, endsWithNewline(reviewLive)) var sidecarStoredRefs []string // refs Put by the span consent (for honest refusal notices) // Re-scan the composed result: never write secret material to the overlay. if secret.IsBlockedFromRepo(composed) { @@ -1291,7 +1291,16 @@ func diffHunks(oldText, newText string) []hunk { // applyHunks reconstructs the captured text from repo's lines, replacing each // ACCEPTED hunk's old region with its new lines and leaving rejected hunks as // repo's original lines. -func applyHunks(oldText string, hunks []hunk, accepted []bool) string { +// +// trailingNewline carries the NEW (live) side's final-newline shape. The line +// model here terminates every emitted line with '\n', so without this the +// composed result ALWAYS ends in '\n'; when the live file has no trailing newline +// the all-accepted composition would then be live+"\n" != live, and the shared +// route would write that, leaving the target wedged in a permanent StateConflict +// that capture can never clear (the only difference is the trailing byte, which +// yields zero reviewable hunks). Stamping the live side's shape keeps composed == +// live when every hunk is accepted. +func applyHunks(oldText string, hunks []hunk, accepted []bool, trailingNewline bool) string { o := splitLines(oldText) var b strings.Builder pos := 0 @@ -1317,7 +1326,17 @@ func applyHunks(oldText string, hunks []hunk, accepted []bool) string { b.WriteString(o[pos]) b.WriteByte('\n') } - return b.String() + out := b.String() + if !trailingNewline && strings.HasSuffix(out, "\n") { + out = out[:len(out)-1] + } + return out +} + +// endsWithNewline reports whether content's final byte is a newline — the shape +// applyHunks must preserve so a captured composition matches the live file. +func endsWithNewline(content []byte) bool { + return len(content) > 0 && content[len(content)-1] == '\n' } // localHunkDelta builds the zsh `.local` sidecar body: ONLY the added/changed diff --git a/cmd/capture_agents.go b/cmd/capture_agents.go index 7de817a..20fcf1d 100644 --- a/cmd/capture_agents.go +++ b/cmd/capture_agents.go @@ -171,7 +171,7 @@ func captureAgentsOne(cc agentsCaptureCtx, ct agents.CaptureTarget) (bool, error } } - composed := applyHunks(string(ct.Content), hunks, accepted) + composed := applyHunks(string(ct.Content), hunks, accepted, endsWithNewline(liveBytes)) // Re-scan the composed content: never write secret material to the repo. if secret.IsBlockedFromRepo(composed) { reportAgentsSecretBlock(cc.out, ct.Label) diff --git a/cmd/capture_render_test.go b/cmd/capture_render_test.go index b2114e2..f2c3320 100644 --- a/cmd/capture_render_test.go +++ b/cmd/capture_render_test.go @@ -511,3 +511,34 @@ func TestCaptureOne_PromptLabelsVisible(t *testing.T) { } } } + +// TestApplyHunks_PreservesTrailingNewlineShape is the F02 regression gate: an +// all-accepted composition must equal the live file byte-for-byte, including its +// trailing-newline shape. Otherwise a live file with no final newline composes to +// live+"\n", which the shared route writes and then wedges the target in a +// permanent, unclearable StateConflict. +func TestApplyHunks_PreservesTrailingNewlineShape(t *testing.T) { + repo := "a\nb\nc\n" + + // Live WITHOUT a trailing newline: composition must not append one. + live := "a\nX\nc" + hunks := diffHunks(repo, live) + accepted := make([]bool, len(hunks)) + for i := range accepted { + accepted[i] = true + } + if got := applyHunks(repo, hunks, accepted, endsWithNewline([]byte(live))); got != live { + t.Errorf("no-trailing-newline: composition = %q, want live %q", got, live) + } + + // Live WITH a trailing newline: composition must keep it. + live2 := "a\nX\nc\n" + hunks2 := diffHunks(repo, live2) + accepted2 := make([]bool, len(hunks2)) + for i := range accepted2 { + accepted2[i] = true + } + if got := applyHunks(repo, hunks2, accepted2, endsWithNewline([]byte(live2))); got != live2 { + t.Errorf("trailing-newline: composition = %q, want live %q", got, live2) + } +} diff --git a/cmd/export.go b/cmd/export.go index a9e6c85..4dee007 100644 --- a/cmd/export.go +++ b/cmd/export.go @@ -214,16 +214,17 @@ func classifyExportEntry(repoRoot, rel string, includeLocal bool) ([]byte, strin return nil, "unreadable", false } // Binary (non-text) content: the line-based text secret gate can't scan it, but a - // binary can still carry embedded private-key material. Run the BINARY-SAFE key - // marker scan (secret.HasKeyMarker) over the raw bytes; if it hits, WITHHOLD it - // (never bundle key material). This scan is symmetric with import/validate, which - // applies the SAME HasKeyMarker check to binary payloads, so an export always - // re-imports and a binary carrying key bytes is refused on both sides. A clean - // binary is included (already tracked/user-vetted, past every path/symlink/~.ssh - // gate). + // binary can still carry embedded private-key material OR a constant-prefix + // provider token (a GitHub PAT, AWS key, …) as an ASCII substring. Run the + // BINARY-SAFE scan (secret.HasBinarySecret) over the raw bytes; if it hits, + // WITHHOLD it (never bundle secret material). This scan is symmetric with + // import/validate, which applies the SAME check to binary payloads, so an export + // always re-imports and a binary carrying key/token bytes is refused on both + // sides. A clean binary is included (already tracked/user-vetted, past every + // path/symlink/~.ssh gate). if !isProbablyText(data) { - if secret.HasKeyMarker(data) { - return nil, "contains key material", false + if secret.HasBinarySecret(data) { + return nil, "contains key material or a provider token", false } return data, "", true } diff --git a/cmd/import.go b/cmd/import.go index 79d8afa..c4d373d 100644 --- a/cmd/import.go +++ b/cmd/import.go @@ -74,15 +74,19 @@ func runImport(c *cobra.Command, args []string) error { if err != nil { return err } - // Guard the target against ~/.ssh and harden its config-dir chain (the default - // lives under ~/.config/ferry): bundle content is only ever written here. + // Guard the target against ~/.ssh and harden its parent chain against symlink + // redirection: bundle content is only ever written here. HardenStoreDir walks + // from $HOME down to the target refusing any symlink component, and is a no-op + // for a target that is not under $HOME — so this applies to BOTH the default + // (~/.config/ferry) and an explicit --out under $HOME, closing the gap where a + // custom target's symlinked parent could redirect the extracted tree while the + // default target was hardened. An --out deliberately placed outside $HOME stays + // unrestricted (HardenStoreDir returns nil there), preserving --out's freedom. if _, err := guardRepoPath("import target", target); err != nil { return err } - if isDefaultImportTarget(outDir) { - if err := hardenConfigDirForRepo(target); err != nil { - return err - } + if err := hardenConfigDirForRepo(target); err != nil { + return err } // No-clobber (N3): refuse a target that exists and is non-empty. Checked before @@ -242,12 +246,6 @@ func resolveImportTarget(outDir string) (string, error) { return defaultRepoDir() } -// isDefaultImportTarget reports whether the import target is ferry's default under -// ~/.config/ferry (so its config-dir chain is hardened, mirroring init). -func isDefaultImportTarget(outDir string) bool { - return strings.TrimSpace(outDir) == "" -} - func init() { importCmd.Flags().String("out", "", "target repo directory (default ~/.config/ferry/repo; must be empty/absent)") importCmd.Flags().String("expect-sha256", "", "verify the bundle's overall SHA256 before importing (out-of-band tamper check)") diff --git a/cmd/restore.go b/cmd/restore.go index 8c7a465..9d5720d 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -26,6 +26,7 @@ func init() { // when stdin is not a terminal, so evals never hang on empty stdin). restoreCmd.Flags().Bool("packages", false, "also uninstall packages ferry recorded as self-installed") restoreCmd.Flags().Bool("yes", false, "skip the confirmation prompt") + restoreCmd.Flags().String("undo", "", "re-apply a pre-restore snapshot by ID (printed after a restore), undoing that restore") restoreCmd.Flags().Bool("purge-without-recovery", false, "remove ferry's own config AND the backup store after restore — DESTROYS the ability to undo this restore or re-restore later (irreversible; the default keeps the backup store)") } @@ -62,6 +63,17 @@ func runRestore(c *cobra.Command, args []string) (retErr error) { out := c.OutOrStdout() + // --undo re-applies a pre-restore snapshot, reversing an earlier restore. + // It is its own self-contained, mutating path (takes the apply lock, registers + // terminal domains, replays the snapshot) and ignores the baseline/scoped/purge + // flow, so guard against combining it with those. + if undoID, _ := c.Flags().GetString("undo"); undoID != "" { + if len(args) > 0 || purge || doPackages { + return errors.New("restore --undo cannot be combined with domain arguments, --packages, or --purge-without-recovery") + } + return runUndoRestore(ctx, undoID, out) + } + // A full restore reverts the machine, so confirm — but stay non-interactive // safe: proceed without prompting when --yes is set or stdin is not a TTY. if !confirmRestore(c, yes, args, out) { @@ -191,6 +203,46 @@ func confirmPurge(c *cobra.Command, yes bool, out io.Writer) bool { return strings.TrimSpace(line) == "yes" } +// runUndoRestore re-applies a previously taken pre-restore snapshot, reversing an +// earlier `ferry restore`. It mutates managed paths, so it takes the SAME apply +// lock as apply/restore and registers the terminal preference domains first (so a +// snapshotted resource entry restores through its Resource hook), then replays the +// snapshot via the engine. This is the user-facing entry point the post-restore +// "undo with ..." message names — without it the snapshot was written but never +// reachable. +func runUndoRestore(ctx *cmdContext, snapID string, out io.Writer) (retErr error) { + eng, err := ctx.Engine() + if err != nil { + return err + } + lock, lockErr := eng.Lock() + if lockErr != nil { + var held *backup.ErrLockHeld + if errors.As(lockErr, &held) { + return fmt.Errorf("another ferry apply is in progress (pid %d); try again later", held.OwnerPID) + } + return fmt.Errorf("acquire apply lock: %w", lockErr) + } + defer func() { + if uErr := lock.Unlock(); uErr != nil { + if retErr == nil { + retErr = fmt.Errorf("release apply lock: %w (the lock may be stale; remove it before the next apply)", uErr) + } else { + fmt.Fprintf(out, "warning: failed to release apply lock: %v; the lock may be stale and block the next apply\n", uErr) + } + } + }() + + if err := registerTerminalDomains(ctx); err != nil { + return err + } + if err := eng.RestoreSnapshot(snapID); err != nil { + return fmt.Errorf("undo restore from snapshot %q: %w (run `ferry restore` first to produce a snapshot, and pass the ID it printed)", snapID, err) + } + fmt.Fprintf(out, "restore: undid the previous restore from snapshot %s\n", snapID) + return nil +} + // fullRestore reverts every managed path to its baseline. The engine snapshots // current state first (built into Restore), so the revert is itself reversible. func fullRestore(ctx *cmdContext, out io.Writer) error { @@ -204,7 +256,7 @@ func fullRestore(ctx *cmdContext, out io.Writer) error { } fmt.Fprintln(out, "restore: reverted all managed paths to their pre-ferry baseline") if snapID != "" { - fmt.Fprintf(out, "restore: pre-restore state snapshotted (%s); undo is possible if needed\n", snapID) + fmt.Fprintf(out, "restore: pre-restore state snapshotted (%s); undo with `ferry restore --undo %s`\n", snapID, snapID) } return nil } @@ -276,7 +328,7 @@ func scopedRestore(ctx *cmdContext, domains []string, out io.Writer) error { } fmt.Fprintf(out, "restore: reverted %s to baseline\n", strings.Join(restored, ", ")) if snapID != "" { - fmt.Fprintf(out, "restore: pre-restore state snapshotted (%s)\n", snapID) + fmt.Fprintf(out, "restore: pre-restore state snapshotted (%s); undo with `ferry restore --undo %s`\n", snapID, snapID) } return nil } diff --git a/cmd/sync.go b/cmd/sync.go index 260b731..e05d344 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -119,7 +119,10 @@ func runSync(c *cobra.Command, _ []string) error { return rollback(snap, repo, err) } - ahead, behind := aheadBehind(repo, "HEAD", upstream) + ahead, behind, aberr := aheadBehind(repo, "HEAD", upstream) + if aberr != nil { + return rollback(snap, repo, aberr) + } switch { case behind > 0 && ahead == 0: // Pure fast-forward: reset the worktree to the remote tip. --hard is safe @@ -839,19 +842,24 @@ func untrackedClobberErr(rel string) error { } // aheadBehind returns how many commits `ref` is ahead of and behind `base` -// (git rev-list --count --left-right base...ref). Zero/zero on error. -func aheadBehind(repo, ref, base string) (ahead, behind int) { +// (git rev-list --count --left-right base...ref). It returns an error rather than +// masking a git failure as 0/0: the only caller runs it against an upstream it has +// already verified exists, so a rev-list failure (or unparseable output) is a real +// fault. Reporting 0/0 there would silently skip remote integration and let a +// divergent branch push without ever pulling — data-loss-shaped. The caller rolls +// back on the error instead. +func aheadBehind(repo, ref, base string) (ahead, behind int, err error) { o, ok := gitSyncOK(repo, "rev-list", "--left-right", "--count", base+"..."+ref) if !ok { - return 0, 0 + return 0, 0, fmt.Errorf("sync: could not compute ahead/behind for %s...%s (git rev-list failed)", base, ref) } f := strings.Fields(o) if len(f) != 2 { - return 0, 0 + return 0, 0, fmt.Errorf("sync: unexpected rev-list output %q computing ahead/behind", strings.TrimSpace(o)) } behind = atoiSafe(f[0]) // commits in base not in ref ahead = atoiSafe(f[1]) // commits in ref not in base - return ahead, behind + return ahead, behind, nil } // pushRangeCommits computes the EXACT list of commit oids that a push would make diff --git a/docs/reference/cli/ferry_restore.md b/docs/reference/cli/ferry_restore.md index 1854860..94915f9 100644 --- a/docs/reference/cli/ferry_restore.md +++ b/docs/reference/cli/ferry_restore.md @@ -21,6 +21,7 @@ ferry restore [flags] -h, --help help for restore --packages also uninstall packages ferry recorded as self-installed --purge-without-recovery remove ferry's own config AND the backup store after restore — DESTROYS the ability to undo this restore or re-restore later (irreversible; the default keeps the backup store) + --undo string re-apply a pre-restore snapshot by ID (printed after a restore), undoing that restore --yes skip the confirmation prompt ``` diff --git a/evals/github_test.go b/evals/github_test.go index 15e0ab3..2c41428 100644 --- a/evals/github_test.go +++ b/evals/github_test.go @@ -307,10 +307,16 @@ func newPushFailingGitStub(t *testing.T) gitStub { logPath := filepath.Join(dir, "git_invocations.log") // Log argv WITH the inherited GIT_TERMINAL_PROMPT so a push line records whether it // was noninteractive (PLAN step 1: every git subprocess runs GIT_TERMINAL_PROMPT=0). + // Skip -C/-c and their VALUES when scanning for the subcommand so the forced + // push-failure still triggers when ferry prepends hardening `-c key=val` global + // options to the push (GitPush mirrors sync's runHardenedGit). script := "#!/bin/sh\n" + "printf '%s [GTP=%s]\\n' \"$*\" \"${GIT_TERMINAL_PROMPT:-}\" >> " + shellQuote(logPath) + "\n" + + "skipnext=0\n" + "for a in \"$@\"; do\n" + + " if [ \"$skipnext\" = 1 ]; then skipnext=0; continue; fi\n" + " case \"$a\" in\n" + + " -C|-c) skipnext=1 ;;\n" + " push) echo 'mock: push failed (network)' 1>&2; exit 1 ;;\n" + " -*) : ;;\n" + " *) break ;;\n" + @@ -345,10 +351,18 @@ func newPushRecordingGitStub(t *testing.T) gitStub { dir := t.TempDir() logPath := filepath.Join(dir, "git_invocations.log") // Log argv WITH the inherited GIT_TERMINAL_PROMPT (noninteractive invariant). + // Skip -C/-c and their VALUES when scanning for the subcommand, so the recorder + // stays correct when ferry prepends hardening `-c key=val` global options to the + // push (as GitPush now does, mirroring sync's runHardenedGit). Without the skip, + // a `-c` value like `core.hooksPath=/dev/null` reads as a positional and the loop + // breaks before reaching `push`, falling through to a real (auth-failing) push. script := "#!/bin/sh\n" + "printf '%s [GTP=%s]\\n' \"$*\" \"${GIT_TERMINAL_PROMPT:-}\" >> " + shellQuote(logPath) + "\n" + + "skipnext=0\n" + "for a in \"$@\"; do\n" + + " if [ \"$skipnext\" = 1 ]; then skipnext=0; continue; fi\n" + " case \"$a\" in\n" + + " -C|-c) skipnext=1 ;;\n" + " push) echo 'mock: push recorded (offline success)'; exit 0 ;;\n" + " -*) : ;;\n" + " *) break ;;\n" + diff --git a/internal/agents/state.go b/internal/agents/state.go index c2013f6..fb17d98 100644 --- a/internal/agents/state.go +++ b/internal/agents/state.go @@ -101,10 +101,32 @@ func RecordTargets(stateDir string, targets map[string]string) error { tmp.Close() return err } + // fsync the temp before rename and the directory after, so a crash cannot leave + // agents-targets.json zero-length/truncated (which would hard-fail the next read). + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } if err := tmp.Close(); err != nil { return err } - return os.Rename(tmpName, path) + if err := os.Rename(tmpName, path); err != nil { + return err + } + return fsyncDir(filepath.Dir(path)) +} + +// fsyncDir flushes a directory entry (the rename that published a state file) so it +// survives a crash. Best-effort: a platform that rejects directory fsync is not +// treated as a write failure, since the file write already succeeded. +func fsyncDir(dir string) error { + d, err := os.Open(dir) + if err != nil { + return nil + } + defer d.Close() + _ = d.Sync() + return nil } // RecordedTargetPaths returns the absolute $HOME destinations the agents @@ -148,6 +170,11 @@ func loadTargetRecord(stateDir string) (record map[string]string, version int, m if err != nil { return nil, 0, false, err } + // A zero-length file is a torn write, not a valid record: treat it as absent so a + // crash degrades to an empty record instead of hard-failing every agents command. + if len(data) == 0 { + return map[string]string{}, targetsVersion, false, nil + } version, migrate, err = statefile.Resolve(path, data, targetsVersion) if err != nil { return nil, 0, false, err diff --git a/internal/backup/baseline.go b/internal/backup/baseline.go index db1a735..6cf2283 100644 --- a/internal/backup/baseline.go +++ b/internal/backup/baseline.go @@ -291,12 +291,13 @@ func stageStoreBlob(dir string, content []byte, origMode os.FileMode) (string, e } // writeStoreBlob writes a content payload into the secret-bearing store at the -// effective (>=0600, stricter preserved) mode. +// effective (>=0600, stricter preserved) mode. It routes through AtomicWrite so +// the blob's DATA is fsync'd before the manifest that references it is flushed: +// journal.record and snapshotCurrent write the blob then AtomicWrite the manifest, +// and if only the manifest were durable after a crash, RollbackIncomplete would +// later read a truncated/empty blob and write it over the user's intact file. A +// plain os.WriteFile makes the blob's directory entry durable (via the manifest's +// syncDir) but not its data pages; AtomicWrite closes that gap. func writeStoreBlob(blobPath string, content []byte, origMode os.FileMode) error { - mode := effectiveMode(origMode) - if err := os.WriteFile(blobPath, content, mode); err != nil { - return err - } - // Force the mode regardless of umask so the store never ends up looser. - return os.Chmod(blobPath, mode) + return AtomicWrite(blobPath, content, effectiveMode(origMode)) } diff --git a/internal/backup/lock.go b/internal/backup/lock.go index 96bbb78..ae51016 100644 --- a/internal/backup/lock.go +++ b/internal/backup/lock.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "os" + "path/filepath" "syscall" "time" ) @@ -72,27 +73,9 @@ func (e *Engine) Lock() (*Lock, error) { // unbounded spin that could livelock under sustained contention. func (e *Engine) lockWithAlive(aliveFn func(int) bool) (*Lock, error) { for attempt := 0; attempt < 2; attempt++ { - f, err := os.OpenFile(e.lockPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, filePerm) + lock, err := e.publishLock() if err == nil { - info := lockInfo{PID: os.Getpid(), AcquiredAt: time.Now().UTC()} - data, _ := json.MarshalIndent(info, "", " ") - if _, werr := f.Write(data); werr != nil { - f.Close() - _ = os.Remove(e.lockPath) - return nil, werr - } - if serr := syncFile(f); serr != nil { - // We created the lockfile but cannot return a usable *Lock, so the caller - // has nothing to Unlock. Remove the just-created file (and close the fd) - // before returning, or it is orphaned: it records THIS live PID, so a - // later apply would wedge behind a lock "owned" by a running process that - // no longer holds it. Clean up on every post-creation error path. - f.Close() - _ = os.Remove(e.lockPath) - return nil, serr - } - f.Close() - return &Lock{path: e.lockPath, pid: info.PID}, nil + return lock, nil } if !errors.Is(err, fs.ErrExist) { return nil, err @@ -122,6 +105,52 @@ func (e *Engine) lockWithAlive(aliveFn func(int) bool) (*Lock, error) { return nil, &ErrLockHeld{OwnerPID: info.PID, AcquiredAt: info.AcquiredAt} } +// publishLock atomically creates the lockfile with COMPLETE contents, or returns +// fs.ErrExist if a lock is already present. It writes the PID/timestamp payload +// to a same-directory temp file, fsyncs it, then os.Link()s it into place: link +// fails with EEXIST when the destination exists, so the lock only ever becomes +// visible AFTER its contents are fully written and durable. +// +// This closes the create-then-write race the plain O_CREATE|O_EXCL path had: a +// concurrent acquirer that failed the create used to be able to read the +// half-written (empty) lockfile of a LIVE owner, conclude it was corrupt/stale, +// and reclaim it — letting two applies run at once. With atomic publish there is +// no window in which a live owner's lock is observable but not yet readable, so +// readLockInfo only ever sees a complete payload (or a genuinely corrupt one from +// out-of-band tampering / a crash, which reclaim still handles). +func (e *Engine) publishLock() (*Lock, error) { + info := lockInfo{PID: os.Getpid(), AcquiredAt: time.Now().UTC()} + data, _ := json.MarshalIndent(info, "", " ") + + tmp, err := os.CreateTemp(filepath.Dir(e.lockPath), "lock-*.tmp") + if err != nil { + return nil, err + } + tmpName := tmp.Name() + if _, werr := tmp.Write(data); werr != nil { + tmp.Close() + _ = os.Remove(tmpName) + return nil, werr + } + if serr := syncFile(tmp); serr != nil { + tmp.Close() + _ = os.Remove(tmpName) + return nil, serr + } + if cerr := tmp.Close(); cerr != nil { + _ = os.Remove(tmpName) + return nil, cerr + } + // Atomic publish. EEXIST (fs.ErrExist) is the "lock already held" signal the + // caller decides held-vs-stale on; the temp is always cleaned up. + if lerr := os.Link(tmpName, e.lockPath); lerr != nil { + _ = os.Remove(tmpName) + return nil, lerr + } + _ = os.Remove(tmpName) + return &Lock{path: e.lockPath, pid: info.PID}, nil +} + // Unlock releases the lock. Releasing a lock not owned by this process is a // no-op guarded by the recorded PID, so a reclaimed-then-rewritten lock is not // removed by the dead owner's stale handle. diff --git a/internal/bundle/validate.go b/internal/bundle/validate.go index 55b45c8..f47fbd9 100644 --- a/internal/bundle/validate.go +++ b/internal/bundle/validate.go @@ -223,16 +223,17 @@ func Validate(path, expectedSHA256 string, includeLocalWanted bool) (*Validated, // Symmetric secret re-gate with export (defense in depth): classify text vs // binary the SAME way export does (a NUL byte marks binary), then apply the SAME // check export applied. A TEXT payload runs the line-based text gate; a BINARY - // payload runs the binary-safe key-marker scan (secret.HasKeyMarker). Using the - // text gate on binary bytes would both mis-scan it and DIVERGE from export — a - // binary that export bundled could then fail its own import. This way an export - // always imports, and a binary carrying key material is refused on both sides. + // payload runs the binary-safe scan (secret.HasBinarySecret — key markers AND + // provider tokens). Using the text gate on binary bytes would both mis-scan it + // and DIVERGE from export — a binary that export bundled could then fail its own + // import. This way an export always imports, and a binary carrying key material + // or a provider token is refused on both sides. if isProbablyText(content) { if secret.IsBlockedFromRepo(string(content)) { return nil, fmt.Errorf("bundle: entry %q contains a secret — refusing to import", clean) } - } else if secret.HasKeyMarker(content) { - return nil, fmt.Errorf("bundle: entry %q contains key material — refusing to import", clean) + } else if secret.HasBinarySecret(content) { + return nil, fmt.Errorf("bundle: entry %q contains key material or a provider token — refusing to import", clean) } validated = append(validated, ValidatedEntry{Path: clean, Size: e.Size, file: f, local: isLocal}) diff --git a/internal/dotfile/state.go b/internal/dotfile/state.go index b4d2e2c..ba17c71 100644 --- a/internal/dotfile/state.go +++ b/internal/dotfile/state.go @@ -155,6 +155,13 @@ func openStoreAt(stateDir string, readOnly bool) (*Store, error) { if err != nil { return nil, err } + // A zero-length file cannot be a valid store (the smallest valid form is a JSON + // object). Treat it as an empty store rather than hard-failing: a torn write from + // a crash then degrades to first-touch reclassification instead of wedging every + // ferry command behind a manual file deletion. + if len(data) == 0 { + return s, nil + } // Resolve the on-disk schema version. A file a newer ferry wrote is refused // here (FutureVersionError) BEFORE any decode or write, so a downgraded ferry // leaves it untouched rather than corrupting it. @@ -334,10 +341,38 @@ func (s *Store) save() error { tmp.Close() return err } + // fsync the temp before rename so the DATA is durable, and fsync the directory + // after rename so the rename itself survives a crash. Without this a power loss + // can leave the store zero-length/truncated, and openStoreAt would then hard-fail + // every apply/capture/status until the file is deleted by hand. Mirrors + // backup.AtomicWrite, the codebase's canonical crash-safe write. + if err := tmp.Sync(); err != nil { + tmp.Close() + return err + } if err := tmp.Close(); err != nil { return err } - return os.Rename(tmpName, s.path) + if err := os.Rename(tmpName, s.path); err != nil { + return err + } + return fsyncDir(filepath.Dir(s.path)) +} + +// fsyncDir flushes a directory entry (e.g. the rename that published a state file) +// so it survives a crash. A missing/unopenable dir is not fatal here — the write +// already succeeded; durability of the rename is best-effort on platforms that +// reject directory fsync. +func fsyncDir(dir string) error { + d, err := os.Open(dir) + if err != nil { + return nil + } + defer d.Close() + if err := d.Sync(); err != nil { + return nil + } + return nil } // pruneDeployed drops every last-deployed snapshot whose hash is no longer diff --git a/internal/ghcli/ghcli.go b/internal/ghcli/ghcli.go index f103bb1..c00bbb5 100644 --- a/internal/ghcli/ghcli.go +++ b/internal/ghcli/ghcli.go @@ -218,12 +218,33 @@ func (c *Client) ViewJSON(owner, name string) (RepoView, error) { return v, nil } -// GitPush runs `git -C push -u origin HEAD` NONINTERACTIVELY -// (GIT_TERMINAL_PROMPT=0). The credential is provided by gh's git credential -// helper; ferry never passes a token on the argv. A non-zero exit is surfaced so -// the caller reports the partial failure (repo created, push failed). +// gitPushHardening is the untrusted-transport hardening prepended (in the git +// global-options slot, before `push`) to the init-time push, mirroring sync's +// runHardenedGit. Without it this one push ran under the user's ambient git +// config, so a global `url."ext::sh -c evil".pushInsteadOf=https://github.com/` +// rewrite would be honored and execute — every OTHER ferry git call already +// blocks that. protocol.ext.allow=never is the decisive control (a rewritten +// ext:: URL is refused by git itself); the rest neutralise hooks, fsmonitor, the +// file transport, and any stray ssh. Set via `-c` (not env) so it targets only +// this call and stays a global option the eval shim skips over to find `push`. +func gitPushHardening() []string { + return []string{ + "-c", "core.hooksPath=/dev/null", + "-c", "core.fsmonitor=false", + "-c", "protocol.ext.allow=never", + "-c", "protocol.file.allow=user", + "-c", "core.sshCommand=/bin/false", + } +} + +// GitPush runs `git push -u origin HEAD` (rooted at repo via cmd.Dir) +// NONINTERACTIVELY (GIT_TERMINAL_PROMPT=0) with the untrusted-transport hardening +// applied. The credential is provided by gh's git credential helper; ferry never +// passes a token on the argv. A non-zero exit is surfaced so the caller reports +// the partial failure (repo created, push failed). func (c *Client) GitPush(repo string) error { - _, stderr, err := c.Run.Run("git", repo, "push", "-u", "origin", "HEAD") + args := append(gitPushHardening(), "push", "-u", "origin", "HEAD") + _, stderr, err := c.Run.Run("git", repo, args...) if err != nil { return fmt.Errorf("git push failed: %s", redact(strings.TrimSpace(stderr))) } diff --git a/internal/secret/scan.go b/internal/secret/scan.go index 728e427..ba9cd18 100644 --- a/internal/secret/scan.go +++ b/internal/secret/scan.go @@ -14,8 +14,10 @@ import ( // plus the STRUCTURAL field names (F5) that carry secrets under non-obvious keys: // private_key_id (GCP service-account), pat (personal access token), bearer. // refresh_token is already covered by the `token` word via the key-prefix chain. -const credKeyword = `(password|passwd|secret|secret[_-]?key|api[_-]?key|apikey|` + +const credKeyword = `(password|passwd|passphrase|pass|pwd|` + + `secret|secret[_-]?key|api[_-]?key|apikey|` + `access[_-]?key|client[_-]?secret|token|auth[_-]?token|` + + `credentials?|(?:encryption|signing|master)[_-]?key|` + `private[_-]?key[_-]?id|bearer|pat)` // Confidence ranks how sure a finding is. Only High findings block repo routing @@ -107,6 +109,16 @@ var ( // prefix), so prose like `# password rotation notes` never matches. An // optional quote on both sides of the key admits the QUOTED-JSON key form // (`"api_key": "value"`) so structural JSON secrets are scanned (F5). + // + // The value is captured by an alternation, NOT a single `["']?([^"'\s]+)["']?` + // group: that earlier form stopped the capture at the first whitespace, so a + // QUOTED multi-word secret (`password = "my dog is rex1"`) yielded only its + // first word — and if that fragment was under the 6-char floor, or the quote + // was space-padded, the assignment produced NO finding at all and a plaintext + // passphrase reached the shared repo. The alternation captures a double-quoted + // (group 2) or single-quoted (group 3) value WHOLE (interior whitespace and + // all), falling back to an unquoted run (group 4). scanLine takes the first + // non-empty of the three and runs the SAME isLikelySecretValue gate on it. secretAssignment = regexp.MustCompile(`(?i)(?:^\s*|export\s+)` + `["']?` + `(?:[A-Za-z0-9]+[._-])*` + @@ -114,7 +126,7 @@ var ( `(?:[._-][A-Za-z0-9]+)*` + `["']?` + `\s*[:=]\s*` + - `["']?([^"'\s]+)["']?`) + `(?:"([^"]*)"|'([^']*)'|([^"'\s]+))`) // credentialKey matches ONLY the KEY portion of a credential assignment // (line-start/after-export, optional quotes, credKeyword as a @@ -200,6 +212,16 @@ var ( // password (redis://:pw@host) blocks. A URL with a user but no password // (rsync://mirror@host) captures an empty group and is likewise not flagged. urlCredential = regexp.MustCompile(`(?i)[a-z][a-z0-9+.-]*://[^/@\s:]*(?::([^/@\s]*))?@`) + + // authHeaderToken matches an HTTP Authorization credential in VALUE position: + // `Authorization: Bearer ` / `Basic `. credKeyword's `bearer` only + // fires when it is the assignment KEY (line-start/after-export), so a token + // carried as a header VALUE inside a generic dotfile (~/.curlrc, ~/.wgetrc, a + // custom tool config) slipped every gate — the entropy backstop needs 24+ + // chars, and these tokens are often shorter. The captured token is gated by + // isNonPlaceholderSecret + isSecretShaped + a small length floor so prose like + // `Bearer authentication` (no digits → not secret-shaped) is not flagged. + authHeaderToken = regexp.MustCompile(`(?i)\b(?:bearer|basic)\s+([^\s"']+)`) ) // entropyThreshold is the Shannon-entropy floor (bits per character) for the @@ -266,6 +288,25 @@ func HasKeyMarker(data []byte) bool { // label between "begin" and "private key" (matched on lowercased bytes). var keyMarkerRE = regexp.MustCompile(`begin[ \-]*[a-z0-9 ]*private key`) +// HasBinarySecret reports whether raw bytes carry either a private-key header +// (PEM/OpenSSH/PGP, via HasKeyMarker) OR a constant-prefix provider token +// (AWS/GitHub/GCP/Slack/OpenAI/Stripe, via the namedToken regex applied to bytes). +// It is the binary-safe secret check for opaque payloads the line-based text +// scanners cannot handle. Bundle EXPORT and IMPORT/validate both use it, so a +// binary carrying key material OR a recognizable token is refused symmetrically on +// both sides — closing the gap where a `ghp_…`/`AKIA…` token embedded in a tracked +// binary (a compiled config, a binary plist) was bundled because only PEM headers +// were checked. +func HasBinarySecret(data []byte) bool { + if len(data) == 0 { + return false + } + if HasKeyMarker(data) { + return true + } + return namedToken.Match(data) +} + // ScanText scans content line-by-line and returns every secret finding, with // 1-based line numbers. Use this for text domains (dotfiles, wg .conf, shell // rc). For opaque/binary domains (a plist value), use ScanValue. @@ -358,12 +399,27 @@ func leadingIndent(s string) int { return n } -// ScanValue scans a single opaque value as a whole (no line splitting) and -// returns findings with Line == 0. Use this for binary/opaque domains where -// line-by-line review is meaningless (e.g. a plist string value). +// ScanValue scans a single opaque value and returns findings with Line == 0. +// Use this for binary/opaque domains where line-by-line review is meaningless +// to the caller (e.g. a plist string value gated whole at capture time). +// +// It splits the value on newlines and runs the per-line detectors over EACH +// line, still stamping Line 0. The per-line pass is required for correctness: +// secretAssignment, credentialKey and npmAuthLine are `^`-anchored WITHOUT the +// `(?m)` flag, so scanning the whole multi-line value at once could only ever +// match its FIRST line — a credential assignment on any later line of a +// multi-line plist/preference blob slipped the gate entirely. Scanning each +// line closes that gap while keeping the Line-0 (no line context) contract the +// opaque-domain callers expect. func ScanValue(value string) Findings { - // Reuse the per-line detectors over the whole value, but report Line 0. - return scanLine(value, 0) + if !strings.ContainsAny(value, "\n\r") { + return scanLine(value, 0) + } + var out Findings + for _, raw := range strings.Split(value, "\n") { + out = append(out, scanLine(strings.TrimRight(raw, "\r"), 0)...) + } + return out } // scanLine runs every detector against a single piece of text and returns the @@ -407,7 +463,18 @@ func scanLine(text string, lineNo int) Findings { } if m := secretAssignment.FindStringSubmatch(text); m != nil { - key, val := m[1], m[2] + key := m[1] + // The value is whichever of the double-quoted (m[2]), single-quoted (m[3]), + // or unquoted (m[4]) alternatives matched; only one is ever non-empty for a + // real value. An empty quoted value ("") leaves all three empty and falls + // through isLikelySecretValue's floor as before. + val := m[2] + if val == "" { + val = m[3] + } + if val == "" { + val = m[4] + } if isLikelySecretValue(val) { out = append(out, Finding{ Rule: "secret-assignment", @@ -455,6 +522,22 @@ func scanLine(text string, lineNo int) Findings { } } + // Authorization header token in value position (Bearer/Basic ). Gated so a + // prose "Bearer" mention or a placeholder token does not fire: the token must be + // non-placeholder, secret-shaped (letters+digits, base64/hex alphabet), and at + // least 8 chars. + if m := authHeaderToken.FindStringSubmatch(text); m != nil { + if tok := m[1]; len(tok) >= 8 && isSecretShaped(tok) && isNonPlaceholderSecret(tok) { + out = append(out, Finding{ + Rule: "auth-header-token", + Confidence: High, + Line: lineNo, + Detail: "Authorization header token", + }) + return out + } + } + // Keyword-gated hex (F3): a long pure-hex value is a secret ONLY when the // line's KEY names a credential. Bare 32+ hex is deliberately NOT flagged // (git SHA / MD5 / stripped UUID); the entropy path also skips pure hex, so @@ -505,6 +588,16 @@ func isLikelySecretValue(val string) bool { if len(val) < 6 { return false } + // A filesystem PATH is not a secret, even under a credential-shaped key: keys + // like `pwd`, `credentials_file`, `signing_key`, or `ssl_key` routinely point at + // a file rather than carrying key material. Excluding path-shaped values here is + // what lets credKeyword cover those abbreviated/adjacent names (pwd, pass, + // credentials, *_key) without turning every `*_file = /some/path` into a false + // positive. Real opaque secrets are not path-shaped (base64/hex segments carry + // uppercase+digits, not lowercase path words), so this does not mask them. + if looksLikePath(val) { + return false + } return isNonPlaceholderSecret(val) } @@ -521,15 +614,32 @@ func isNonPlaceholderSecret(val string) bool { return false } // Reject shell/env interpolation and ferry's own placeholder: these carry no - // literal secret. - if strings.Contains(val, "${") || strings.HasPrefix(val, "$") { + // literal secret. The `$` test is NARROW — only `${VAR}` / `$(cmd)` expansions + // or a value that is ENTIRELY a bare `$VAR` reference. A blanket "starts with $" + // exemption also swallowed real leetspeak passwords (`$tr0ngPassw0rd!`, + // `$uper$ecretPass99`), which are literal secrets, not interpolation. + if strings.Contains(val, "${") || strings.Contains(val, "$(") { + return false + } + if shellVarRef.MatchString(val) { return false } if strings.Contains(val, "{{") { return false } + // A `<...>` angle-bracket span is a template placeholder (``), + // but a bare `<` anywhere used to exempt the whole value — so `abc` +// or ``. Used to exempt structural placeholders without exempting every +// value that merely contains a stray `<`. +var anglePlaceholder = regexp.MustCompile(`<[^<>]*>`) + // candidateTokens splits a line into opaque tokens suitable for the entropy // heuristic. It splits on whitespace and a few separators that commonly bound a // token (= : , ; ( ) " ' ), so an embedded "KEY=" yields the token alone. func candidateTokens(text string) []string { fields := strings.FieldsFunc(text, func(r rune) bool { switch r { - case ' ', '\t', '=', ':', ',', ';', '(', ')', '"', '\'': + case ' ', '\t', '\n', '\r', '=', ':', ',', ';', '(', ')', '"', '\'': return true default: return false diff --git a/internal/secret/scan_test.go b/internal/secret/scan_test.go index 570bac4..28bf42c 100644 --- a/internal/secret/scan_test.go +++ b/internal/secret/scan_test.go @@ -340,3 +340,137 @@ func TestHasKeyMarker(t *testing.T) { t.Errorf("HasKeyMarker fired on ordinary config text") } } + +// TestScanText_QuotedMultiWordCredential is the regression gate for the quoted +// multi-word value bypass: a credential assignment whose QUOTED value contains +// interior whitespace (or a short leading word) must still be flagged. The +// earlier value capture stopped at the first whitespace, so these slipped the +// gate entirely and reached the shared repo in plaintext. +func TestScanText_QuotedMultiWordCredential(t *testing.T) { + cases := []string{ + `password = "my dog is rex1"`, // multi-word, short first word + `password = "my dog is x9"`, // multi-word, sub-6-char first word + `password = " paddedSecret123"`, // leading space inside the quote + `secret: 'correct horse battery st'`, // single-quoted multi-word + } + for _, c := range cases { + if fs := ScanText(c); !fs.HasHigh() { + t.Errorf("expected high finding for quoted multi-word credential %q, got %+v", c, fs) + } + } + // Preserve the no-false-positive discipline: empty and placeholder quoted + // values must still NOT be flagged. + for _, c := range []string{ + `password = ""`, + `api_key = "${API_KEY}"`, + `secret = "{{ferry.secret \"x.y\"}}"`, + `token = "changeme now please"`, // placeholder word inside a multi-word value + } { + if fs := ScanText(c); fs.HasHigh() { + t.Errorf("FALSE POSITIVE: placeholder/empty quoted value %q flagged: %+v", c, fs) + } + } +} + +// TestScanValue_MultiLineAssignment is the regression gate for the multi-line +// ScanValue bypass: a credential assignment on a NON-FIRST line of an opaque +// value (e.g. a defaults-export plist blob) must be caught. The ^-anchored +// detectors previously only ever matched line 1 of the whole value. +func TestScanValue_MultiLineAssignment(t *testing.T) { + value := "line one\nline two\ndb_password = realSecretValue99\nline four" + if fs := ScanValue(value); !fs.HasHigh() { + t.Fatalf("expected high finding for credential on a non-first line, got %+v", fs) + } + // A single-line value keeps the original whole-value behaviour. + if fs := ScanValue(`password = realSecretValue99`); !fs.HasHigh() { + t.Errorf("single-line ScanValue regressed: %+v", fs) + } + // An ordinary multi-line value with no secret must not over-block. + if fs := ScanValue("first line\nsecond line\nthird line"); fs.HasHigh() { + t.Errorf("FALSE POSITIVE: benign multi-line value flagged: %+v", fs) + } +} + +// TestScanText_ExpandedCredentialKeywords is the F09 regression gate: common +// abbreviated/adjacent credential key names must now block, while their +// path-valued cousins (`*_file`, `*_path`) must NOT (the value is a filesystem +// path, not key material). +func TestScanText_ExpandedCredentialKeywords(t *testing.T) { + block := []string{ + `DB_PASS=SuperSecret99x`, + `PASSPHRASE=hunter2hunter2z`, + `ENCRYPTION_KEY=q7GmL2vTn9wRx4z`, + `credentials=abcdef123456ghij`, + `signing_key: myS1gningKeyValue`, + } + for _, c := range block { + if fs := ScanText(c); !fs.HasHigh() { + t.Errorf("expected high finding for %q, got %+v", c, fs) + } + } + // Path-valued credential-shaped keys must NOT be flagged (F09 guard). + pass := []string{ + `pwd = /home/alex/project`, + `credentials_file = ~/.aws/credentials`, + `signing_key_path = /etc/keys/id_rsa`, + `ssl_key = ./certs/server.key`, + } + for _, c := range pass { + if fs := ScanText(c); fs.HasHigh() { + t.Errorf("FALSE POSITIVE: path-valued credential key %q flagged: %+v", c, fs) + } + } +} + +// TestScanText_LiteralDollarAndAngleSecrets is the F08 regression gate: a literal +// secret that merely starts with `$` or contains an interior `<`/`...` must be +// caught, while genuine interpolation and angle-bracket templates stay exempt. +func TestScanText_LiteralDollarAndAngleSecrets(t *testing.T) { + block := []string{ + `password=$tr0ngPassw0rd!`, // leetspeak literal, not a var ref + `password=$uper$ecretPass99`, // interior $, not interpolation + `password=abc`, // angle-bracket template + } + for _, c := range pass { + if fs := ScanText(c); fs.HasHigh() { + t.Errorf("FALSE POSITIVE: interpolation/template %q flagged: %+v", c, fs) + } + } +} + +// TestScanText_AuthHeaderBearerToken is the F21 regression gate: an Authorization +// header token in value position must block even when short, while a prose +// mention of "Bearer" (no secret-shaped token) must not. +func TestScanText_AuthHeaderBearerToken(t *testing.T) { + block := []string{ + `header = "Authorization: Bearer zVbT3q9LmX2"`, + `Authorization: Basic aGVsbG86d29ybGQ99`, + } + for _, c := range block { + if fs := ScanText(c); !fs.HasHigh() { + t.Errorf("expected high finding for auth header token %q, got %+v", c, fs) + } + } + pass := []string{ + `# send a Bearer authentication token here`, // prose, no secret-shaped token + `Authorization: Bearer `, // placeholder + `Authorization: Bearer null`, // too short / not shaped + } + for _, c := range pass { + if fs := ScanText(c); fs.HasHigh() { + t.Errorf("FALSE POSITIVE: non-secret Bearer line %q flagged: %+v", c, fs) + } + } +} diff --git a/internal/statefile/statefile.go b/internal/statefile/statefile.go index 337e93c..f36ca80 100644 --- a/internal/statefile/statefile.go +++ b/internal/statefile/statefile.go @@ -154,6 +154,13 @@ func BackupForMigration(path string, found int) (string, error) { tmp.Close() return "", err } + // fsync the temp before the os.Link publish so the backup's DATA is durable: a + // crash otherwise leaves a zero-length migration backup, defeating the "original + // copy wins" recovery guarantee. + if err := tmp.Sync(); err != nil { + tmp.Close() + return "", err + } if err := tmp.Close(); err != nil { return "", err }