From 1461e25c05982bc5df9fdff3884e598b9e659b90 Mon Sep 17 00:00:00 2001 From: Ryan Snodgrass Date: Mon, 11 May 2026 09:05:36 -0700 Subject: [PATCH 1/5] fix(doctor): session-scope, auto-hydrate, keyword pre-screen, per-prefix github detectors (ox-zukx, ox-def1) Co-Authored-By: SageOx SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T15-00-ryan-OxOlcb/view --- cmd/ox/doctor_ledger_secrets.go | 209 ++++++++++++++++++++------- cmd/ox/doctor_ledger_secrets_test.go | 104 +++++++++---- cmd/ox/session_hydrate.go | 6 +- cmd/ox/session_redact_history.go | 163 +++++++++++++-------- cmd/ox/session_redact_hydrate.go | 134 +++++++++++++++++ internal/session/secrets.go | 102 ++++++++++--- internal/session/secrets_test.go | 27 +++- 7 files changed, 577 insertions(+), 168 deletions(-) create mode 100644 cmd/ox/session_redact_hydrate.go diff --git a/cmd/ox/doctor_ledger_secrets.go b/cmd/ox/doctor_ledger_secrets.go index abe0599c..c531b86b 100644 --- a/cmd/ox/doctor_ledger_secrets.go +++ b/cmd/ox/doctor_ledger_secrets.go @@ -33,20 +33,25 @@ type ledgerSecretsFinding struct { } // ledgerSecretsScanResult is what checkLedgerSecrets passes back. Findings -// are keyed by detector name; FilesScanned is total count for "scanned N -// files, found 0 secrets" reassurance. +// are keyed by detector name; FilesScanned/SessionsScanned describe scope +// for "scanned N session recordings, found 0 secrets" reassurance. +// SessionsHydrated/SessionsSkipped report on the LFS auto-hydration loop: +// a non-zero SessionsSkipped means coverage is incomplete and the result +// MUST NOT be reported as a clean bill of health. type ledgerSecretsScanResult struct { - LedgerPath string - FilesScanned int - Findings map[string]*ledgerSecretsFinding + LedgerPath string + FilesScanned int + SessionsScanned int + SessionsHydrated int // session dirs we auto-hydrated to cache during the scan + SessionsSkipped int // session dirs we couldn't read at all (hydration failed) + Findings map[string]*ledgerSecretsFinding } -// ledgerSecretsScanExts lists file extensions worth scanning. The -// remediation set is dominated by JSONL (sessions), JSON (caches and meta), -// markdown (docs, summaries, memory), txt (transcripts), and VTT -// (audio captions). Binaries are skipped — the same logic the pre-push -// scanner uses (see prePushScannerSkipExts) but inverted to an allowlist -// because we expect the ledger to contain a wider mix of file types. +// ledgerSecretsScanExts lists file extensions worth scanning inside a +// session directory. JSONL is the recording itself; JSON covers meta and +// summary; MD covers any per-session notes; TXT and VTT cover legacy +// transcripts and audio captions. Anything else inside a session dir is +// either binary or not a recording artifact, so we don't pay regex cost. var ledgerSecretsScanExts = map[string]bool{ ".jsonl": true, ".json": true, @@ -55,15 +60,16 @@ var ledgerSecretsScanExts = map[string]bool{ ".vtt": true, } -// ledgerSecretsSkipDirs lists subdirectory names that we never descend into -// during the scan. .git/.dolt/.beads contain pack files and SQL state, not -// user-authored content; .gc-cache and .bak are local-only artifacts. +// ledgerSecretsSkipDirs is retained for the snapshot tarball walk in +// session_redact_history.go (which still archives the whole ledger). The +// credential-scan path no longer uses it — that path iterates session +// directories explicitly rather than filtering a global walk. var ledgerSecretsSkipDirs = map[string]bool{ - ".git": true, - ".dolt": true, - ".beads": true, - ".bak": true, - ".gc-cache": true, + ".git": true, + ".dolt": true, + ".beads": true, + ".bak": true, + ".gc-cache": true, "node_modules": true, } @@ -103,14 +109,41 @@ func checkLedgerSecrets(fix bool) checkResult { return SkippedCheck(name, "ledger directory does not exist", "") } - result, err := scanLedgerForSecrets(ledgerPath) + // Pre-scan hydration. Without this, dehydrated session files would be + // scanned as 140-byte LFS pointer stubs and return "clean" regardless + // of what's inside. The doctor harness suppresses incremental stdout + // during check execution, so we pass io.Discard for the progress + // writer — the result counters carry the same information. + hyd, hydErr := hydrateAllSessionsForScan(gitRoot, ledgerPath, io.Discard) + if hydErr != nil { + return FailedCheck(name, fmt.Sprintf("pre-scan hydration error: %v", hydErr), "") + } + + result, err := scanLedgerForSecrets(gitRoot, ledgerPath) if err != nil { return FailedCheck(name, fmt.Sprintf("scan error: %v", err), "") } + // Surface hydration outcome on the result so the report line below + // is accurate. SessionsHydrated tracks "we fetched something during + // this scan call"; merge with the pre-pass for the cleaner number. + if hyd.Hydrated > result.SessionsHydrated { + result.SessionsHydrated = hyd.Hydrated + } + if hyd.Failed > 0 { + result.SessionsSkipped += hyd.Failed + } + + scope := fmt.Sprintf("scanned %d session recording file(s) across %d session(s)", + result.FilesScanned, result.SessionsScanned) + if result.SessionsHydrated > 0 { + scope += fmt.Sprintf("; auto-hydrated %d session(s) from LFS", result.SessionsHydrated) + } + if result.SessionsSkipped > 0 { + scope += fmt.Sprintf("; %d session(s) unscannable (hydration failed)", result.SessionsSkipped) + } if len(result.Findings) == 0 { - return PassedCheck(name, - fmt.Sprintf("scanned %d files, no credential patterns found", result.FilesScanned)) + return PassedCheck(name, scope+"; no credential patterns found") } // Build the failure message without ever including matched bytes. @@ -130,8 +163,8 @@ func checkLedgerSecrets(fix bool) checkResult { f.Detector, f.Count, f.FileCount, f.Sample) } - summary := fmt.Sprintf("found %d credential pattern(s) across %d distinct detectors in %d scanned files", - totalCount, len(result.Findings), result.FilesScanned) + summary := fmt.Sprintf("found %d credential pattern(s) across %d distinct detectors (%s)", + totalCount, len(result.Findings), scope) guidance := "Run `ox session redact-history` for interactive per-finding cleanup. " + "For already-pushed commits, see docs/security/credential-redaction.md." @@ -153,53 +186,123 @@ func resolveLedgerPathForAudit(localCfg *config.LocalConfig) string { return defaultPath } -// scanLedgerForSecrets walks the ledger working tree (skipping .git, -// .beads, etc.) and runs DefaultPatterns against every line of every -// allowlisted-extension file. Aggregates matches per detector. Never -// retains the matched bytes — only counts, file paths, and timestamps. +// scanLedgerForSecrets enumerates session-recording files inside +// /sessions//, hydrates LFS pointers to the cache via the +// canonical openSessionContent path, and runs DefaultPatterns against +// every line of every allowlisted-extension file. Aggregates matches per +// detector. Never retains the matched bytes — only counts, file paths, +// and timestamps. +// +// Scope is intentionally session-only: the previous global ledger walk +// matched fixture/test-fixture bytes inside data/github/.../pr/*.json +// (PR-review archives containing the very example credentials these +// detectors are designed to spot) and inflated counts by an order of +// magnitude with non-actionable findings. Anything outside sessions/ +// is not a "session recording" and belongs to other audits if needed. +// +// Hydration is mandatory: dehydrated session files are ~140-byte LFS +// pointer stubs that match no credential pattern. Pre-fix, 17.5 % of +// the on-disk file set on a real ledger were pointer stubs returning +// "scanned, clean" — a credible audit cannot have that blind spot. +// openSessionContent writes hydrated bytes to .sageox/cache/sessions/ +// per .claude/rules/cache-only-design.md, never in-place. // // Exposed (unexported) for use by `ox session redact-history` so both // surfaces share the same definition of "what counts as a finding." -func scanLedgerForSecrets(ledgerPath string) (*ledgerSecretsScanResult, error) { +func scanLedgerForSecrets(projectRoot, ledgerPath string) (*ledgerSecretsScanResult, error) { redactor := session.NewRedactor() result := &ledgerSecretsScanResult{ LedgerPath: ledgerPath, Findings: map[string]*ledgerSecretsFinding{}, } - err := filepath.WalkDir(ledgerPath, func(path string, d os.DirEntry, walkErr error) error { - if walkErr != nil { - // transient stat errors (deleted-mid-walk, permission) — log - // implicitly by continuing; this is a best-effort audit, not - // a transactional read. - return nil - } - if d.IsDir() { - if ledgerSecretsSkipDirs[d.Name()] { - return filepath.SkipDir - } - return nil + sessionsRoot := filepath.Join(ledgerPath, "sessions") + entries, err := os.ReadDir(sessionsRoot) + if err != nil { + if os.IsNotExist(err) { + return result, nil // ledger has no sessions yet — vacuously clean } - if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(d.Name()))] { - return nil + return nil, fmt.Errorf("read sessions dir: %w", err) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + + for _, entry := range entries { + if !entry.IsDir() { + continue } - info, err := d.Info() + sessionName := entry.Name() + result.SessionsScanned++ + + sessionDir := filepath.Join(sessionsRoot, sessionName) + files, err := os.ReadDir(sessionDir) if err != nil { - return nil + result.SessionsSkipped++ + continue } - if info.Size() > ledgerSecretsSizeCap { - return nil + + hydratedThisSession := false + anyFileRead := false + for _, fEntry := range files { + if fEntry.IsDir() { + continue + } + filename := fEntry.Name() + if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(filename))] { + continue + } + + contentPath, hydratedNow, err := resolveSessionContentForScan(projectRoot, ledgerPath, sessionName, filename) + if err != nil { + // Hydration failed (network, missing LFS object, etc.). Skip + // this file but keep going — other files in this session + // might still be locally readable. + continue + } + if hydratedNow { + hydratedThisSession = true + } + info, err := os.Stat(contentPath) + if err != nil || info.Size() > ledgerSecretsSizeCap { + continue + } + anyFileRead = true + result.FilesScanned++ + relForReport := filepath.Join("sessions", sessionName, filename) + if err := scanLedgerFileForSecrets(redactor, contentPath, relForReport, info.ModTime(), result); err != nil { + continue + } + } + if hydratedThisSession { + result.SessionsHydrated++ + } + if !anyFileRead { + result.SessionsSkipped++ } - result.FilesScanned++ - rel, _ := filepath.Rel(ledgerPath, path) - return scanLedgerFileForSecrets(redactor, path, rel, info.ModTime(), result) - }) - if err != nil { - return nil, err } return result, nil } +// resolveSessionContentForScan is a thin wrapper around openSessionContent +// that also tells the caller whether hydration happened during this call +// (so the audit can report "auto-hydrated N sessions"). The signal is +// approximate — we treat "cache file didn't exist before the call but +// does after" as "hydrated by us." Pre-existing cached files don't +// count, which is what the user wants for the scope summary. +func resolveSessionContentForScan(projectRoot, ledgerPath, sessionName, filename string) (string, bool, error) { + cacheDir := filepath.Join(ledgerPath, ".sageox", "cache", "sessions", sessionName) + cachePath := filepath.Join(cacheDir, filename) + _, cacheExistedBefore := os.Stat(cachePath) + + resolved, err := openSessionContent(projectRoot, ledgerPath, sessionName, filename) + if err != nil { + return "", false, err + } + // hydrated by this call iff the cache file did not exist before and + // the resolved path is the cache path. + hydratedNow := cacheExistedBefore != nil && resolved == cachePath + return resolved, hydratedNow, nil +} + // scanLedgerFileForSecrets reads a single file line-by-line, runs every // detector pattern, and updates result.Findings in place. Streams via // bufio so memory cost is O(line length) instead of O(file size). diff --git a/cmd/ox/doctor_ledger_secrets_test.go b/cmd/ox/doctor_ledger_secrets_test.go index 715d5a58..fff5036c 100644 --- a/cmd/ox/doctor_ledger_secrets_test.go +++ b/cmd/ox/doctor_ledger_secrets_test.go @@ -32,6 +32,8 @@ func makeLedgerForAuditTest(t *testing.T, files map[string]string) string { // TestScanLedgerForSecrets_FindsCanaries plants known-secret patterns into // session files and asserts the scanner reports them — without leaking the // values back through the result. +// +// Failure prevented: scan misses a session-level credential leak (ox-zukx). func TestScanLedgerForSecrets_FindsCanaries(t *testing.T) { work := makeLedgerForAuditTest(t, map[string]string{ "sessions/2026-05-10/raw.jsonl": `{"text":"AKIAIOSFODNN7EXAMPLE"}` + "\n", @@ -40,11 +42,15 @@ func TestScanLedgerForSecrets_FindsCanaries(t *testing.T) { "docs/notes.md": "this file has no secrets in it", }) - result, err := scanLedgerForSecrets(work) + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) require.NotNil(t, result) - assert.GreaterOrEqual(t, result.FilesScanned, 4) + // Scope is sessions-only now (ox-zukx). docs/notes.md is intentionally + // NOT scanned — only files under /sessions// count. + assert.Equal(t, 3, result.FilesScanned, + "sessions-only scope: 2 raw.jsonl + 1 meta.json; docs/notes.md is out of scope") + assert.Equal(t, 2, result.SessionsScanned) assert.Contains(t, result.Findings, "aws_access_key") assert.Contains(t, result.Findings, "gitlab_token") @@ -69,33 +75,39 @@ func TestScanLedgerForSecrets_CleanLedger(t *testing.T) { "docs/note.md": "no creds here\n", }) - result, err := scanLedgerForSecrets(work) + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) assert.Empty(t, result.Findings) - assert.GreaterOrEqual(t, result.FilesScanned, 3) + // sessions-only scope: 2 files in sessions/clean/; docs/ is ignored + assert.Equal(t, 2, result.FilesScanned) + assert.Equal(t, 1, result.SessionsScanned) } -// TestScanLedgerForSecrets_SkipsBlessedDirs verifies that .git, .beads, -// .dolt etc. are never descended into — saves time and avoids false -// positives on binary pack-files that random-bytes-match a regex. -func TestScanLedgerForSecrets_SkipsBlessedDirs(t *testing.T) { +// TestScanLedgerForSecrets_OutOfScopeIgnored verifies the scope reduction +// in ox-zukx: anything outside /sessions// is not scanned, +// even if it has a matching extension and contains canary bytes. Pre-fix +// the audit scanned the entire ledger working tree and matched fixture +// credentials inside data/github/.../pr/*.json archives — non-actionable +// "findings" that drowned the real signal. +func TestScanLedgerForSecrets_OutOfScopeIgnored(t *testing.T) { work := makeLedgerForAuditTest(t, map[string]string{ - "sessions/leak.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/real/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "data/github/2026/05/pr/597.json": `{"body":"AKIAIOSFODNN7EXAMPLE"}` + "\n", + "docs/architecture.md": "describes the AKIAIOSFODNN7EXAMPLE pattern\n", + ".git/hidden.jsonl": `{"k":"ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}`, }) - // plant a "secret" inside .git — it must NOT be scanned - require.NoError(t, os.WriteFile(filepath.Join(work, ".git", "hidden.jsonl"), - []byte(`{"k":"ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}`), 0644)) - // also plant one inside .beads - require.NoError(t, os.MkdirAll(filepath.Join(work, ".beads"), 0755)) - require.NoError(t, os.WriteFile(filepath.Join(work, ".beads", "x.json"), - []byte(`{"k":"gho_alphabetabcdefghijklmnopqrstuvwxyz12"}`), 0644)) - - result, err := scanLedgerForSecrets(work) + + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) - // only the real session file should have fired - assert.Contains(t, result.Findings, "aws_access_key") + + // One real session-scoped finding, no others. + require.Contains(t, result.Findings, "aws_access_key") + assert.Equal(t, 1, result.Findings["aws_access_key"].Count, + "AKIA appearances in data/, docs/, .git/ must NOT count") + assert.Equal(t, 1, result.Findings["aws_access_key"].FileCount) + assert.Equal(t, "sessions/real/raw.jsonl", result.Findings["aws_access_key"].Sample) assert.NotContains(t, result.Findings, "github_token", - ".git/.beads should be skipped, github_token detector should not have fired") + "github_token in .git/ must not be reachable: out of scope") } // TestScanLedgerForSecrets_SizeCap verifies oversized files are skipped. @@ -114,7 +126,7 @@ func TestScanLedgerForSecrets_SizeCap(t *testing.T) { "sessions/huge.jsonl": string(buf), }) - result, err := scanLedgerForSecrets(work) + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) assert.Empty(t, result.Findings, "over-cap file must be skipped, but detectors fired: %v", result.Findings) } @@ -123,18 +135,54 @@ func TestScanLedgerForSecrets_SizeCap(t *testing.T) { // extensions are skipped — a binary blob shouldn't be scanned. func TestScanLedgerForSecrets_OnlyAllowlistedExts(t *testing.T) { work := makeLedgerForAuditTest(t, map[string]string{ - "sessions/audio.mp3": "AKIAIOSFODNN7EXAMPLE", // plant in a "binary" — must not be scanned - "sessions/img.png": "AKIAIOSFODNN7EXAMPLE", // same - "sessions/real.jsonl": "no secrets in this one", // .jsonl is scanned; nothing to find + "sessions/s1/audio.mp3": "AKIAIOSFODNN7EXAMPLE", // "binary" inside a session — must not be scanned + "sessions/s1/img.png": "AKIAIOSFODNN7EXAMPLE", // same + "sessions/s1/real.jsonl": "no secrets in this one", // .jsonl is scanned; nothing to find }) - result, err := scanLedgerForSecrets(work) + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) assert.Empty(t, result.Findings) // .mp3 and .png should not be counted toward FilesScanned (only .jsonl matched) assert.Equal(t, 1, result.FilesScanned) } +// TestScanLedgerForSecrets_HerokuKeyDoesNotFireOnUUIDs is a regression +// test for ox-zukx: heroku_key was a bare UUID regex with no Keywords +// guard, so it matched every UUIDv7 in meta.json (session_id, agent_id, +// etc.). On a real ledger this produced 2141 false positives across 641 +// sessions. Running redact-history on that result would have rewritten +// session_id values in meta.json and corrupted session resolution. The +// fix in internal/session/secrets.go adds Keywords: ["heroku"], AND +// ScanForSecrets now actually applies the keyword pre-screen. +// +// Failure prevented: a single UUIDv7 in a meta.json file produces a +// "heroku_key" finding. +func TestScanLedgerForSecrets_HerokuKeyDoesNotFireOnUUIDs(t *testing.T) { + work := makeLedgerForAuditTest(t, map[string]string{ + "sessions/s/meta.json": `{"session_id":"ses_019e15c2-a128-73d4-b034-a262c3ddc96b",` + + `"agent_id":"019e15c2-a128-73d4-b034-a262c3ddc96b"}`, + "sessions/s/raw.jsonl": `{"text":"just a UUID 11111111-2222-3333-4444-555555555555 with no special context"}` + "\n", + }) + + result, err := scanLedgerForSecrets(work, work) + require.NoError(t, err) + assert.NotContains(t, result.Findings, "heroku_key", + "heroku_key must only fire when 'heroku' appears in the same line") +} + +// TestScanLedgerForSecrets_HerokuKeyFiresWithContext is the positive +// pair to the above — when 'heroku' IS present in the line, the +// detector should still catch real Heroku-shaped keys. +func TestScanLedgerForSecrets_HerokuKeyFiresWithContext(t *testing.T) { + work := makeLedgerForAuditTest(t, map[string]string{ + "sessions/s/raw.jsonl": `{"text":"heroku api key: aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"}` + "\n", + }) + result, err := scanLedgerForSecrets(work, work) + require.NoError(t, err) + assert.Contains(t, result.Findings, "heroku_key") +} + // TestLedgerOriginHasEmbeddedPAT_True verifies detection when a PAT is // actually embedded. func TestLedgerOriginHasEmbeddedPAT_True(t *testing.T) { @@ -188,10 +236,10 @@ func TestLedgerOriginHasEmbeddedPAT_NoOrigin(t *testing.T) { // (the thing printed to the user) must never contain a matched secret. func TestCheckLedgerSecrets_OutputDoesNotLeakBytes(t *testing.T) { work := makeLedgerForAuditTest(t, map[string]string{ - "sessions/leak.jsonl": "AKIAIOSFODNN7EXAMPLE and " + + "sessions/leaky/raw.jsonl": "AKIAIOSFODNN7EXAMPLE and " + "gh token ghp_alphabetabcdefghijklmnopqrstuvwxyz12\n", }) - result, err := scanLedgerForSecrets(work) + result, err := scanLedgerForSecrets(work, work) require.NoError(t, err) require.NotEmpty(t, result.Findings) diff --git a/cmd/ox/session_hydrate.go b/cmd/ox/session_hydrate.go index 9649230f..70fa8879 100644 --- a/cmd/ox/session_hydrate.go +++ b/cmd/ox/session_hydrate.go @@ -93,7 +93,11 @@ func hydrateFromLedger(projectRoot, sessionsDir, nameArg string, quiet bool) err } slog.Debug("hydrate: read meta.json", "file_count", len(meta.Files)) for fn, ref := range meta.Files { - slog.Debug("hydrate: manifest entry", "file", fn, "oid", ref.OID[:20]+"…", "size", ref.Size) + oidPreview := ref.OID + if len(oidPreview) > 20 { + oidPreview = oidPreview[:20] + "…" + } + slog.Debug("hydrate: manifest entry", "file", fn, "oid", oidPreview, "size", ref.Size) } // cache base: /.sageox/cache/sessions// diff --git a/cmd/ox/session_redact_history.go b/cmd/ox/session_redact_history.go index a68b8cd6..de0b5820 100644 --- a/cmd/ox/session_redact_history.go +++ b/cmd/ox/session_redact_history.go @@ -101,12 +101,18 @@ func runSessionRedactHistory(cmd *cobra.Command, args []string) error { if err != nil { return err } + // ProjectRoot drives openSessionContent's hydration call (which uses the + // project's auth/endpoint to talk to the LFS Batch API). Empty when + // --ledger-path is used without a project context; hydration will fail + // in that case but the scan still works on whatever's already in cache. + projectRoot := findGitRoot() opts := redactHistoryOptions{ - LedgerPath: ledgerPath, - DryRun: redactHistoryDryRun, - BackupDir: redactHistoryBackupDir, - Stdin: cmd.InOrStdin(), - Stdout: cmd.OutOrStdout(), + ProjectRoot: projectRoot, + LedgerPath: ledgerPath, + DryRun: redactHistoryDryRun, + BackupDir: redactHistoryBackupDir, + Stdin: cmd.InOrStdin(), + Stdout: cmd.OutOrStdout(), } return runRedactHistoryWorkflow(opts) } @@ -138,11 +144,16 @@ func redactHistoryResolveLedger() (string, error) { // redactHistoryOptions bundles inputs so the workflow can be unit-tested // with synthetic stdin/stdout and an override backup directory. type redactHistoryOptions struct { - LedgerPath string - DryRun bool - BackupDir string // empty → default ~/.local/share/sageox/backups/redact-history/ - Stdin io.Reader - Stdout io.Writer + // ProjectRoot is the git repo path used to authenticate LFS hydration + // (via openSessionContent → hydrateFromLedger). May be empty when the + // caller targets a ledger directly with --ledger-path; in that case + // hydration falls back to whatever is already in the cache. + ProjectRoot string + LedgerPath string + DryRun bool + BackupDir string // empty → default ~/.local/share/sageox/backups/redact-history/ + Stdin io.Reader + Stdout io.Writer } // runRedactHistoryWorkflow implements the dry-run / scan / snapshot / @@ -157,21 +168,38 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { opts.Stdout = os.Stdout } + // Pre-scan hydration. The scan is content-based and pointer-file bytes + // match no credential pattern, so we MUST hydrate every dehydrated + // session recording before scanning or the result is meaningless. This + // is intentionally visible — printing progress and a final hydration + // summary so the operator can see what was fetched and whether any + // session was unreachable. + hyd, err := hydrateAllSessionsForScan(opts.ProjectRoot, opts.LedgerPath, opts.Stdout) + if err != nil { + return fmt.Errorf("pre-scan hydration: %w", err) + } + if hyd.Failed > 0 { + fmt.Fprintf(opts.Stdout, + "Warning: %d session file(s) across %d session(s) could NOT be hydrated and were not scanned.\n"+ + "Scan coverage is incomplete — re-run after fixing connectivity / auth.\n\n", + hyd.FailedFiles, hyd.Failed) + } + fmt.Fprintf(opts.Stdout, "Scanning ledger %s for credentials...\n", opts.LedgerPath) - scanResult, err := scanLedgerForSecrets(opts.LedgerPath) + scanResult, err := scanLedgerForSecrets(opts.ProjectRoot, opts.LedgerPath) if err != nil { return fmt.Errorf("scan: %w", err) } if len(scanResult.Findings) == 0 { - fmt.Fprintf(opts.Stdout, "No credential patterns found across %d files. Nothing to redact.\n", - scanResult.FilesScanned) + fmt.Fprintf(opts.Stdout, "No credential patterns found across %d session file(s) in %d session(s). Nothing to redact.\n", + scanResult.FilesScanned, scanResult.SessionsScanned) return nil } // Enumerate per-finding details (file:line:detector) by re-walking the // affected files. We didn't keep this from the aggregate scan because // the audit doesn't need it; the cleanup tool does. - findings, err := enumerateRedactHistoryFindings(opts.LedgerPath, scanResult) + findings, err := enumerateRedactHistoryFindings(opts.ProjectRoot, opts.LedgerPath, scanResult) if err != nil { return fmt.Errorf("enumerate findings: %w", err) } @@ -262,7 +290,7 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { } // Re-scan and report. - postScan, err := scanLedgerForSecrets(opts.LedgerPath) + postScan, err := scanLedgerForSecrets(opts.ProjectRoot, opts.LedgerPath) if err != nil { return fmt.Errorf("post-scan: %w", err) } @@ -284,66 +312,75 @@ type redactHistoryFinding struct { Line int } -// enumerateRedactHistoryFindings re-walks just the files the audit -// flagged and collects per-line findings. Two-pass on purpose: the audit -// stays cheap (per-file aggregation), and the cleanup pays the per-line -// cost only for files that need it. -func enumerateRedactHistoryFindings(ledgerPath string, audit *ledgerSecretsScanResult) ([]redactHistoryFinding, error) { +// enumerateRedactHistoryFindings re-walks just the session directories +// surfaced by the audit and collects per-line findings. Two-pass on +// purpose: the audit stays cheap (per-file aggregation), and the cleanup +// pays the per-line cost only for files that need it. +// +// Hydration policy matches scanLedgerForSecrets: pointer files are +// resolved via openSessionContent so per-line classification reads the +// same bytes the audit saw. The path recorded on each finding is the +// git-tracked in-place path (sessions//) so downstream +// push-status classification and the snapshot tarball stay correct. +func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledgerSecretsScanResult) ([]redactHistoryFinding, error) { if audit == nil || len(audit.Findings) == 0 { return nil, nil } - // collect the set of files that need a per-line pass - files := map[string]bool{} - for _, f := range audit.Findings { - files[f.Sample] = true - } - // Sample is one path per detector; the audit doesn't keep full lists. - // Walk again with the same allowlist to enumerate every affected file. - // This is conservative: scan all files matching the allowlist, but - // emit findings only for the detectors that fired in the audit so - // we don't pay regex cost for detectors that didn't. redactor := session.NewRedactor() var out []redactHistoryFinding - err := filepath.WalkDir(ledgerPath, func(path string, d os.DirEntry, walkErr error) error { - if walkErr != nil || d.IsDir() { - if d != nil && d.IsDir() && ledgerSecretsSkipDirs[d.Name()] { - return filepath.SkipDir - } - return nil - } - if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(d.Name()))] { - return nil + sessionsRoot := filepath.Join(ledgerPath, "sessions") + entries, err := os.ReadDir(sessionsRoot) + if err != nil { + if os.IsNotExist(err) { + return nil, nil } - info, err := d.Info() - if err != nil || info.Size() > ledgerSecretsSizeCap { - return nil + return nil, fmt.Errorf("read sessions dir: %w", err) + } + for _, entry := range entries { + if !entry.IsDir() { + continue } - rel, _ := filepath.Rel(ledgerPath, path) - // G122: filepath.WalkDir rooted at ledgerPath, which is an ox-owned - // directory under the user's XDG data dir. Symlink TOCTOU between - // the walker's stat and our Open is theoretical here — the threat - // model is an attacker with write access to the ledger directory, - // which already grants direct read of the secret content this - // scan is auditing. - f, err := os.Open(path) //nolint:gosec // G122: see comment above + sessionName := entry.Name() + sessionDir := filepath.Join(sessionsRoot, sessionName) + files, err := os.ReadDir(sessionDir) if err != nil { - return nil + continue } - defer f.Close() - scanner := bufio.NewScanner(f) - scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) - lineNo := 0 - for scanner.Scan() { - lineNo++ - for _, name := range redactor.ScanForSecrets(scanner.Text()) { - out = append(out, redactHistoryFinding{Detector: name, Path: rel, Line: lineNo}) + for _, fEntry := range files { + if fEntry.IsDir() { + continue + } + filename := fEntry.Name() + if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(filename))] { + continue + } + contentPath, err := openSessionContent(projectRoot, ledgerPath, sessionName, filename) + if err != nil { + continue } + info, err := os.Stat(contentPath) + if err != nil || info.Size() > ledgerSecretsSizeCap { + continue + } + f, err := os.Open(contentPath) //nolint:gosec // G304: path resolved via openSessionContent (ledger-owned) + if err != nil { + continue + } + scanner := bufio.NewScanner(f) + scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) + lineNo := 0 + // rel is the in-place git-tracked path. Required so push-status + // classification can `git log -1 -- ` against the ledger. + rel := filepath.Join("sessions", sessionName, filename) + for scanner.Scan() { + lineNo++ + for _, name := range redactor.ScanForSecrets(scanner.Text()) { + out = append(out, redactHistoryFinding{Detector: name, Path: rel, Line: lineNo}) + } + } + f.Close() } - return nil - }) - if err != nil { - return nil, err } // stable order: path, then line, then detector sort.Slice(out, func(i, j int) bool { diff --git a/cmd/ox/session_redact_hydrate.go b/cmd/ox/session_redact_hydrate.go new file mode 100644 index 00000000..09fed21b --- /dev/null +++ b/cmd/ox/session_redact_hydrate.go @@ -0,0 +1,134 @@ +package main + +import ( + "fmt" + "io" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/sageox/ox/internal/lfs" +) + +// hydrateSummary describes the outcome of the pre-scan hydration phase. +// +// The credential scan reads file content. Dehydrated session files are +// 140-byte LFS pointer stubs that match no credential pattern, so a scan +// over an un-hydrated ledger is a credible-looking lie — that was the +// pre-fix behavior. The redact-history workflow now forces a hydration +// pass before scanning and surfaces this summary so the operator can see +// (a) how much had to be fetched and (b) whether any session was +// unreachable, which would make the resulting scan incomplete. +type hydrateSummary struct { + Sessions int // total sessions inspected + AlreadyOK int // sessions where every scannable file was already real content + Hydrated int // sessions where we fetched at least one file from LFS + HydratedFiles int + Failed int // sessions where >=1 scannable file could not be hydrated + FailedFiles int +} + +// hydrateAllSessionsForScan walks /sessions// and ensures +// every scannable file (per ledgerSecretsScanExts) has hydrated content +// reachable via openSessionContent. Hydration writes to the cache only +// per .claude/rules/cache-only-design.md; in-place pointer files stay +// untouched. Progress is printed to `out` so the operator can see what's +// happening on a real ledger (115 dehydrated sessions on a fresh clone +// is not unusual and the fetch is not instant). +func hydrateAllSessionsForScan(projectRoot, ledgerPath string, out io.Writer) (hydrateSummary, error) { + var summary hydrateSummary + sessionsRoot := filepath.Join(ledgerPath, "sessions") + entries, err := os.ReadDir(sessionsRoot) + if err != nil { + if os.IsNotExist(err) { + return summary, nil + } + return summary, fmt.Errorf("read sessions dir: %w", err) + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + + // First pass: identify sessions that have at least one pointer file + // among scannable files. Sessions that are already fully hydrated skip + // the network entirely. + type pendingFile struct { + sessionName string + filename string + inPlacePath string + } + var toHydrate []pendingFile + for _, entry := range entries { + if !entry.IsDir() { + continue + } + summary.Sessions++ + sessionName := entry.Name() + sessionDir := filepath.Join(sessionsRoot, sessionName) + files, err := os.ReadDir(sessionDir) + if err != nil { + continue + } + sessionNeedsAny := false + for _, fEntry := range files { + if fEntry.IsDir() { + continue + } + filename := fEntry.Name() + if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(filename))] { + continue + } + inPlace := filepath.Join(sessionDir, filename) + if !lfs.IsPointerFile(inPlace) { + // already real content in-place — nothing to hydrate + continue + } + cachePath := filepath.Join(ledgerPath, ".sageox", "cache", "sessions", sessionName, filename) + if _, err := os.Stat(cachePath); err == nil { + // already hydrated to cache from a previous run + continue + } + toHydrate = append(toHydrate, pendingFile{sessionName: sessionName, filename: filename, inPlacePath: inPlace}) + sessionNeedsAny = true + } + if !sessionNeedsAny { + summary.AlreadyOK++ + } + } + + if len(toHydrate) == 0 { + fmt.Fprintf(out, "Hydration: all %d session(s) already have local content; no LFS fetch needed.\n\n", + summary.Sessions) + return summary, nil + } + + fmt.Fprintf(out, "Hydration: %d file(s) across dehydrated session(s) must be fetched from LFS before scanning...\n", + len(toHydrate)) + + // Track per-session success/failure so the summary is session-scoped + // (one failed file in a session still means the scan of THAT session + // is incomplete). + sessionFailed := map[string]bool{} + sessionHydrated := map[string]bool{} + for i, pf := range toHydrate { + // progress every 25 files to avoid spamming terminals on huge fetches + if i%25 == 0 && i > 0 { + fmt.Fprintf(out, " ... %d / %d hydrated\n", i, len(toHydrate)) + } + if _, err := openSessionContent(projectRoot, ledgerPath, pf.sessionName, pf.filename); err != nil { + sessionFailed[pf.sessionName] = true + summary.FailedFiles++ + continue + } + sessionHydrated[pf.sessionName] = true + summary.HydratedFiles++ + } + for s := range sessionHydrated { + if !sessionFailed[s] { + summary.Hydrated++ + } + } + summary.Failed = len(sessionFailed) + fmt.Fprintf(out, "Hydration complete: %d file(s) fetched across %d session(s).\n\n", + summary.HydratedFiles, summary.Hydrated) + return summary, nil +} diff --git a/internal/session/secrets.go b/internal/session/secrets.go index 0840b802..509629b9 100644 --- a/internal/session/secrets.go +++ b/internal/session/secrets.go @@ -77,18 +77,67 @@ func DefaultPatterns() []SecretPattern { Redact: "[REDACTED_AWS_SECRET]", }, - // GitHub tokens (ghp_, gho_, ghs_, ghr_, ghu_ prefixes) - { - Name: "github_token", - Pattern: regexp.MustCompile(`gh[psortu]_[A-Za-z0-9_]{36,255}`), - Redact: "[REDACTED_GITHUB_TOKEN]", - }, - - // GitHub fine-grained PAT (github_pat_ prefix) - { - Name: "github_fine_grained_pat", - Pattern: regexp.MustCompile(`github_pat_[A-Za-z0-9_]{22,255}`), - Redact: "[REDACTED_GITHUB_PAT]", + // GitHub tokens. Split per-prefix so the report identifies WHICH + // kind of GitHub credential leaked (Personal Access Token, + // OAuth user-to-server, installation/server, refresh) — that + // distinction matters for rotation: refresh tokens, server + // tokens, and PATs are revoked through different paths. + // + // Lengths follow GitHub's published shapes (docs.github.com/en/ + // rest/authentication/keeping-your-api-credentials-secure): + // ghp_<36> classic PAT + // gho_<36> OAuth access token + // ghu_<36> user-to-server token + // ghs_<36> server-to-server / installation token + // ghr_<76> refresh token + // github_pat_<22>_<59> fine-grained PAT (82 chars after prefix) + // + // Keywords lock the pre-screen to the exact prefix so the regex + // is only evaluated on lines that already contain the literal + // 4-char (or 11-char) anchor. Without this, before ox-zukx the + // audit path bypassed the pre-screen entirely and these patterns + // were re-evaluated on every line — fixable but unnecessarily + // expensive. + // Redact strings stay backward-compatible ([REDACTED_GITHUB_TOKEN] + // / [REDACTED_GITHUB_PAT]) so callers asserting on output don't + // have to be rewritten in lockstep with this split. The win from + // per-prefix detectors is in the *detector name* in scan reports, + // not in the redaction placeholder. + { + Name: "github_personal_access_token", + Pattern: regexp.MustCompile(`ghp_[A-Za-z0-9]{36,255}`), + Redact: "[REDACTED_GITHUB_TOKEN]", + Keywords: []string{"ghp_"}, + }, + { + Name: "github_oauth_token", + Pattern: regexp.MustCompile(`gho_[A-Za-z0-9]{36,255}`), + Redact: "[REDACTED_GITHUB_TOKEN]", + Keywords: []string{"gho_"}, + }, + { + Name: "github_user_to_server_token", + Pattern: regexp.MustCompile(`ghu_[A-Za-z0-9]{36,255}`), + Redact: "[REDACTED_GITHUB_TOKEN]", + Keywords: []string{"ghu_"}, + }, + { + Name: "github_server_token", + Pattern: regexp.MustCompile(`ghs_[A-Za-z0-9]{36,255}`), + Redact: "[REDACTED_GITHUB_TOKEN]", + Keywords: []string{"ghs_"}, + }, + { + Name: "github_refresh_token", + Pattern: regexp.MustCompile(`ghr_[A-Za-z0-9]{76,255}`), + Redact: "[REDACTED_GITHUB_TOKEN]", + Keywords: []string{"ghr_"}, + }, + { + Name: "github_fine_grained_pat", + Pattern: regexp.MustCompile(`github_pat_[A-Za-z0-9_]{82,255}`), + Redact: "[REDACTED_GITHUB_PAT]", + Keywords: []string{"github_pat_"}, }, // GitLab personal access tokens (glpat- prefix) @@ -196,11 +245,18 @@ func DefaultPatterns() []SecretPattern { Redact: "[REDACTED_PYPI_TOKEN]", }, - // Heroku API keys (UUIDs - careful with false positives) + // Heroku API keys (UUIDs - careful with false positives). + // Keywords guard is mandatory: without it the bare UUID regex fires + // on every UUIDv7 in the ledger (session_id, agent_id, etc.). On a + // real sageox ledger this produced 2141 false positives across 641 + // sessions before the guard was added; running redact-history on + // that result would have rewritten every session_id in meta.json + // and corrupted session resolution. See ox-zukx. { - Name: "heroku_key", - Pattern: regexp.MustCompile(`[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}`), - Redact: "[REDACTED_UUID]", + Name: "heroku_key", + Pattern: regexp.MustCompile(`[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}`), + Redact: "[REDACTED_HEROKU_KEY]", + Keywords: []string{"heroku"}, }, // Private keys (RSA, DSA, EC, OPENSSH) @@ -574,16 +630,28 @@ func (r *Redactor) ContainsSecrets(input string) bool { return false } -// ScanForSecrets returns pattern names of all secrets found without redacting +// ScanForSecrets returns pattern names of all secrets found without redacting. +// +// Applies the SecretPattern.Keywords pre-screen — patterns with a non-empty +// Keywords list are only evaluated when at least one keyword appears in the +// (lowercased) input. The write-time chokepoint relies on this filter to +// keep regex cost bounded; until ox-zukx the audit/redact-history path +// bypassed it, which let bare-UUID and other anchorless patterns fire on +// every line. See SecretPattern docs at the top of this file for rationale. func (r *Redactor) ScanForSecrets(input string) []string { r.mu.RLock() defer r.mu.RUnlock() + lowerInput := strings.ToLower(input) + var found []string for _, p := range r.patterns { if p.Pattern == nil { continue } + if !p.MatchesKeyword(lowerInput) { + continue + } if len(p.SkipIf) == 0 { if p.Pattern.MatchString(input) { found = append(found, p.Name) diff --git a/internal/session/secrets_test.go b/internal/session/secrets_test.go index 72721121..727cb9eb 100644 --- a/internal/session/secrets_test.go +++ b/internal/session/secrets_test.go @@ -222,8 +222,10 @@ func TestRedactString_GitHubTokens(t *testing.T) { wantFind: true, }, { - name: "github refresh token", - input: "ghr_1234567890abcdefghijklmnopqrstuvwxyz12", + name: "github refresh token", + // Refresh tokens are 76 chars after the ghr_ prefix; older fixture + // used 38 chars (the classic-PAT tail length). Updated in ox-def1. + input: "ghr_1234567890abcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcd", expected: "[REDACTED_GITHUB_TOKEN]", wantFind: true, }, @@ -234,8 +236,11 @@ func TestRedactString_GitHubTokens(t *testing.T) { wantFind: true, }, { - name: "github fine-grained pat", - input: "github_pat_11ABCDEFGH0123456789_aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcdefghijklmnopqrs", + name: "github fine-grained pat", + // Real shape: 11-char "github_pat_" + 22-char ID + "_" + 59-char secret = 92 total. + // Updated from a shorter fixture in ox-def1 (the strict {82,255} tail + // length follows GitHub's published format and the user's spec). + input: "github_pat_11ABCDEFGHIJ012345678901_aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcdefghijklmnopqRStUVWxYz", expected: "[REDACTED_GITHUB_PAT]", wantFind: true, }, @@ -252,7 +257,15 @@ func TestRedactString_GitHubTokens(t *testing.T) { t.Run(tt.name, func(t *testing.T) { output, found := r.RedactString(tt.input) assert.Equal(t, tt.expected, output) - hasGitHub := containsPattern(found, "github_token") || containsPattern(found, "github_fine_grained_pat") + // After ox-def1 the broad `github_token` detector was split into + // per-prefix patterns. Any of these counts as "we caught a + // GitHub-shaped token" for the purposes of this test. + hasGitHub := containsPattern(found, "github_personal_access_token") || + containsPattern(found, "github_oauth_token") || + containsPattern(found, "github_user_to_server_token") || + containsPattern(found, "github_server_token") || + containsPattern(found, "github_refresh_token") || + containsPattern(found, "github_fine_grained_pat") assert.Equal(t, tt.wantFind, hasGitHub) }) } @@ -724,7 +737,9 @@ func TestScanForSecrets(t *testing.T) { found := r.ScanForSecrets(input) assert.True(t, containsPattern(found, "aws_access_key")) - assert.True(t, containsPattern(found, "github_token")) + // After ox-def1 the broad `github_token` was split per-prefix. A full + // 40-char ghp_<36> matches the specific PAT detector now. + assert.True(t, containsPattern(found, "github_personal_access_token")) } func TestRedactWithAllowlist(t *testing.T) { From 89d7a88fd3da134961d256bbda0e73d6b4496f39 Mon Sep 17 00:00:00 2001 From: Ryan Snodgrass Date: Mon, 11 May 2026 11:17:11 -0700 Subject: [PATCH 2/5] feat(redaction): per-session redaction pass record + catalog identity (ox-8bfh) Co-Authored-By: SageOx SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T18-01-ryan-OxOlcb/view --- cmd/ox/doctor_ledger_secrets.go | 12 ++- cmd/ox/session_redact_history.go | 55 +++++++++- cmd/ox/session_redact_record.go | 126 ++++++++++++++++++++++ cmd/ox/session_redact_record_test.go | 151 +++++++++++++++++++++++++++ internal/lfs/meta.go | 84 +++++++++++++++ internal/session/catalog.go | 99 ++++++++++++++++++ internal/session/catalog_test.go | 90 ++++++++++++++++ internal/session/secrets.go | 30 ++++-- 8 files changed, 634 insertions(+), 13 deletions(-) create mode 100644 cmd/ox/session_redact_record.go create mode 100644 cmd/ox/session_redact_record_test.go create mode 100644 internal/session/catalog.go create mode 100644 internal/session/catalog_test.go diff --git a/cmd/ox/doctor_ledger_secrets.go b/cmd/ox/doctor_ledger_secrets.go index c531b86b..4e0ec727 100644 --- a/cmd/ox/doctor_ledger_secrets.go +++ b/cmd/ox/doctor_ledger_secrets.go @@ -133,8 +133,16 @@ func checkLedgerSecrets(fix bool) checkResult { result.SessionsSkipped += hyd.Failed } - scope := fmt.Sprintf("scanned %d session recording file(s) across %d session(s)", - result.FilesScanned, result.SessionsScanned) + // Identify the catalog that produced these findings. ox-8bfh: the + // version+hash is the audit anchor that lets future re-runs decide + // "this session was last scanned under N7-aaaa, current ruleset is + // N8-bbbb; the new detectors might catch new findings — re-scan." + catalogRedactor := session.NewRedactor() + catalogTag := fmt.Sprintf("catalog=%s (hash %s…)", + catalogRedactor.CatalogVersion(), catalogRedactor.CatalogHash()[:8]) + + scope := fmt.Sprintf("scanned %d session recording file(s) across %d session(s); %s", + result.FilesScanned, result.SessionsScanned, catalogTag) if result.SessionsHydrated > 0 { scope += fmt.Sprintf("; auto-hydrated %d session(s) from LFS", result.SessionsHydrated) } diff --git a/cmd/ox/session_redact_history.go b/cmd/ox/session_redact_history.go index de0b5820..07361368 100644 --- a/cmd/ox/session_redact_history.go +++ b/cmd/ox/session_redact_history.go @@ -4,6 +4,7 @@ import ( "archive/tar" "bufio" "compress/gzip" + "context" "crypto/sha256" "encoding/hex" "encoding/json" @@ -18,6 +19,7 @@ import ( "github.com/sageox/ox/internal/config" "github.com/sageox/ox/internal/ledger" + "github.com/sageox/ox/internal/lfs" "github.com/sageox/ox/internal/session" "github.com/spf13/cobra" ) @@ -211,6 +213,18 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { return fmt.Errorf("classify findings by push status: %w", err) } + // Print catalog identity BEFORE the finding counts so the operator + // knows which ruleset produced them. Same version+hash will be + // persisted to meta.json for every session this pass redacts — + // see ox-8bfh for the trust model. The redactor used here is the + // same instance reused below for the rewrite; reusing keeps the + // catalog identity consistent between scan-time reporting and + // post-write persistence. + redactor := session.NewRedactor() + catalogVersion := redactor.CatalogVersion() + catalogHash := redactor.CatalogHash() + fmt.Fprintf(opts.Stdout, "Catalog: %s\n hash: %s\n\n", catalogVersion, catalogHash) + fmt.Fprintf(opts.Stdout, "Found %d credential matches across %d file(s):\n", len(findings), countDistinctRedactHistoryPaths(findings)) fmt.Fprintf(opts.Stdout, " %d in unpushed working tree (actionable)\n", len(unpushed)) @@ -249,7 +263,13 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { // many findings only gets reviewed once (the operator approves the // whole file's redaction, not every single line). byPath := groupFindingsByPath(unpushed) - redactor := session.NewRedactor() + // Collect per-session redaction entries as files succeed. After all + // redactions are done we write one RedactionPass to each affected + // session's meta.json — this is the ox-8bfh audit trail. We use + // the same passID across all sessions touched by this run so the + // trail is correlatable; appliedAt is captured once so the records + // share a wall-clock instant. + redactedBySession := map[string][]lfs.RedactionEntry{} redactedFiles := 0 for _, fileFindings := range byPath { path := fileFindings[0].Path @@ -272,6 +292,17 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { return fmt.Errorf("redact %s: %w", path, err) } redactedFiles++ + // Record per-finding entries against the session that owns + // this file. Detector + line + filename only — never bytes. + if sess, fname, ok := splitSessionPath(path); ok { + for _, f := range fileFindings { + redactedBySession[sess] = append(redactedBySession[sess], lfs.RedactionEntry{ + File: fname, + Line: f.Line, + Detector: f.Detector, + }) + } + } fmt.Fprintf(opts.Stdout, " Redacted: %s\n\n", path) default: fmt.Fprintf(opts.Stdout, " Skipped: %s\n\n", path) @@ -283,12 +314,34 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { return nil } + // Persist a RedactionPass to each affected session's meta.json + // BEFORE staging+amending — so the amend commit includes the audit + // trail entry. Uses lfs.MutateSessionMeta (flock + atomic rename) + // so concurrent daemon writes to meta.json can't lose the entry. + // This is the load-bearing ox-8bfh transparency step. + metaPaths, err := writeRedactionPassesPerSession(context.Background(), opts.LedgerPath, + redactedBySession, catalogVersion, catalogHash) + if err != nil { + return fmt.Errorf("write redaction pass: %w", err) + } + // Stage the modified meta.json files alongside the redacted content + // files so the amend commit captures both. + for _, mp := range metaPaths { + if _, ok := byPath[mp]; !ok { + byPath[mp] = nil + } + } + // Stage + amend in a single holding commit. The operator can review // the amended diff before running `ox session push`. if err := stageAndAmendRedactedFiles(opts.LedgerPath, byPath); err != nil { return fmt.Errorf("stage/amend: %w", err) } + // Surface what was caught back to the operator — same data that + // just landed in meta.json, summarized for the terminal. + fmt.Fprintf(opts.Stdout, "Recorded redaction pass in %d session meta.json file(s).\n", len(metaPaths)) + // Re-scan and report. postScan, err := scanLedgerForSecrets(opts.ProjectRoot, opts.LedgerPath) if err != nil { diff --git a/cmd/ox/session_redact_record.go b/cmd/ox/session_redact_record.go new file mode 100644 index 00000000..0f503067 --- /dev/null +++ b/cmd/ox/session_redact_record.go @@ -0,0 +1,126 @@ +package main + +import ( + "context" + "fmt" + "path/filepath" + "strings" + "time" + + "github.com/google/uuid" + "github.com/sageox/ox/internal/lfs" +) + +// splitSessionPath splits a ledger-relative session content path of the +// form "sessions//" into (sessionName, filename, ok). Other +// shapes (no "sessions/" prefix, no trailing filename) return ok=false. +// +// Used by the redact-history workflow to attribute per-file findings to +// their owning session so the audit trail (lfs.RedactionPass) lands in +// the right meta.json — without parsing rules duplicated at call sites. +func splitSessionPath(p string) (sessionName, filename string, ok bool) { + p = filepath.ToSlash(p) + const prefix = "sessions/" + if !strings.HasPrefix(p, prefix) { + return "", "", false + } + rest := strings.TrimPrefix(p, prefix) + idx := strings.Index(rest, "/") + if idx <= 0 || idx == len(rest)-1 { + return "", "", false + } + sessionName = rest[:idx] + filename = rest[idx+1:] + if strings.Contains(filename, "/") { + // Don't support nested files under the session dir. Today every + // scannable file lives directly under sessions//. + return "", "", false + } + return sessionName, filename, true +} + +// writeRedactionPassesPerSession appends an lfs.RedactionPass entry to +// each affected session's meta.json. Returns the relative paths of the +// meta.json files mutated, in ledger-relative form +// ("sessions//meta.json"), so the caller can stage them with the +// redacted content files in the amend commit. +// +// Concurrency safety: +// - Each meta.json write goes through lfs.MutateSessionMeta, which +// takes an advisory flock and writes via atomic rename. This is the +// same primitive that prevents ox-lrrq-class races between the +// daemon and the CLI — adding redaction passes does NOT widen that +// surface. +// +// All sessions in this call share a single passID + appliedAt instant +// so an external auditor can correlate "these N sessions were all +// redacted in the same pass." catalogVersion / catalogHash come from +// the live Redactor used to do the actual rewrite, not from a +// pre-computed string — they prove which rules ran. +// +// Per ox-zyg7 / the design in ox-8bfh, entries carry detector + file + +// line only — NEVER the matched bytes. +func writeRedactionPassesPerSession( + ctx context.Context, + ledgerPath string, + bySession map[string][]lfs.RedactionEntry, + catalogVersion, catalogHash string, +) ([]string, error) { + if len(bySession) == 0 { + return nil, nil + } + passID, err := uuid.NewV7() + if err != nil { + return nil, fmt.Errorf("generate pass id: %w", err) + } + appliedAt := time.Now().UTC() + appliedBy := "ox session redact-history" + + out := make([]string, 0, len(bySession)) + for sessionName, entries := range bySession { + sessionDir := filepath.Join(ledgerPath, "sessions", sessionName) + summary := summarizeRedactionEntries(entries) + pass := lfs.RedactionPass{ + PassID: passID.String(), + AppliedAt: appliedAt, + AppliedBy: appliedBy, + CatalogVersion: catalogVersion, + CatalogHash: catalogHash, + Summary: summary, + Entries: entries, + } + mutateErr := lfs.MutateSessionMeta(ctx, sessionDir, func(meta *lfs.SessionMeta) (*lfs.SessionMeta, error) { + if meta == nil { + // No meta.json yet — refuse rather than synthesize. A + // session being redacted should have an existing + // meta.json; if not, something more serious is wrong + // and the redaction shouldn't have happened either. + return nil, fmt.Errorf("session %s has no meta.json", sessionName) + } + meta.Redactions = append(meta.Redactions, pass) + return meta, nil + }) + if mutateErr != nil { + return out, fmt.Errorf("update meta.json for %s: %w", sessionName, mutateErr) + } + out = append(out, filepath.ToSlash(filepath.Join("sessions", sessionName, "meta.json"))) + } + return out, nil +} + +// summarizeRedactionEntries builds a RedactionPassSummary from a slice +// of entries. Total = len(entries); ByDetector groups counts by +// detector name. Used by writeRedactionPassesPerSession but exposed for +// testability. +func summarizeRedactionEntries(entries []lfs.RedactionEntry) lfs.RedactionPassSummary { + out := lfs.RedactionPassSummary{Total: len(entries)} + if len(entries) == 0 { + return out + } + byDet := make(map[string]int, len(entries)) + for _, e := range entries { + byDet[e.Detector]++ + } + out.ByDetector = byDet + return out +} diff --git a/cmd/ox/session_redact_record_test.go b/cmd/ox/session_redact_record_test.go new file mode 100644 index 00000000..41ae13f8 --- /dev/null +++ b/cmd/ox/session_redact_record_test.go @@ -0,0 +1,151 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/sageox/ox/internal/lfs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSplitSessionPath verifies the relative-path parser used to map +// "sessions//" into its components. Wrong attribution here +// means a session's redaction pass lands in the wrong meta.json. +func TestSplitSessionPath(t *testing.T) { + tests := []struct { + in string + session string + file string + ok bool + }{ + {"sessions/2026-05-11-abc/raw.jsonl", "2026-05-11-abc", "raw.jsonl", true}, + {"sessions/X/meta.json", "X", "meta.json", true}, + {"sessions/X/sub/nested.jsonl", "", "", false}, // nested unsupported + {"data/github/pr/1.json", "", "", false}, // wrong prefix + {"sessions//raw.jsonl", "", "", false}, // empty session name + {"sessions/onlysession/", "", "", false}, // trailing slash, no file + {"sessions/onlysession", "", "", false}, // no filename + } + for _, tt := range tests { + s, f, ok := splitSessionPath(tt.in) + assert.Equal(t, tt.ok, ok, "ok mismatch for %q", tt.in) + if tt.ok { + assert.Equal(t, tt.session, s) + assert.Equal(t, tt.file, f) + } + } +} + +// TestSummarizeRedactionEntries verifies the per-detector aggregation +// shape that lands in meta.json (and in agent-visible terminal output). +func TestSummarizeRedactionEntries(t *testing.T) { + entries := []lfs.RedactionEntry{ + {File: "raw.jsonl", Line: 1, Detector: "github_personal_access_token"}, + {File: "raw.jsonl", Line: 42, Detector: "url_with_password"}, + {File: "raw.jsonl", Line: 99, Detector: "url_with_password"}, + {File: "summary.md", Line: 3, Detector: "aws_access_key"}, + } + s := summarizeRedactionEntries(entries) + assert.Equal(t, 4, s.Total) + assert.Equal(t, map[string]int{ + "github_personal_access_token": 1, + "url_with_password": 2, + "aws_access_key": 1, + }, s.ByDetector) +} + +// TestWriteRedactionPassesPerSession_AppendsToMeta verifies the +// load-bearing ox-8bfh property: a redaction pass appends a new entry +// to meta.json.Redactions rather than overwriting prior history. +// +// Failure prevented: a second redact-history run replacing the first +// pass's record — destroying the audit trail. +func TestWriteRedactionPassesPerSession_AppendsToMeta(t *testing.T) { + ledger := t.TempDir() + sessName := "2026-05-11-test-OxABCD" + sessDir := filepath.Join(ledger, "sessions", sessName) + require.NoError(t, os.MkdirAll(sessDir, 0o755)) + + // Seed a minimum-valid meta.json. The Validate() guard in + // WriteSessionMetaOnly insists on a session id + agent type; mock + // enough fields to pass. + seed := &lfs.SessionMeta{ + Version: "1.0", + SessionName: sessName, + Username: "test-user", + AgentID: "OxABCD", + AgentType: "claude-code", + CreatedAt: time.Now().UTC(), + Files: map[string]lfs.FileRef{}, + } + require.NoError(t, lfs.WriteSessionMetaOnly(sessDir, seed)) + + // First pass. + bySession := map[string][]lfs.RedactionEntry{ + sessName: { + {File: "raw.jsonl", Line: 10, Detector: "github_personal_access_token"}, + {File: "raw.jsonl", Line: 42, Detector: "url_with_password"}, + }, + } + metaPaths, err := writeRedactionPassesPerSession(context.Background(), ledger, + bySession, "ox-secrets-N9-cafebabe", "deadbeefcafebabe...") + require.NoError(t, err) + require.Len(t, metaPaths, 1) + assert.Equal(t, "sessions/"+sessName+"/meta.json", metaPaths[0]) + + got1, err := lfs.ReadSessionMeta(sessDir) + require.NoError(t, err) + require.Len(t, got1.Redactions, 1) + assert.Equal(t, "ox-secrets-N9-cafebabe", got1.Redactions[0].CatalogVersion) + assert.Equal(t, 2, got1.Redactions[0].Summary.Total) + assert.Equal(t, "ox session redact-history", got1.Redactions[0].AppliedBy) + firstPassID := got1.Redactions[0].PassID + assert.NotEmpty(t, firstPassID, "pass id must be populated") + + // Critical: never store matched bytes. + for _, p := range got1.Redactions { + for _, e := range p.Entries { + // RedactionEntry has no Original field; trip on any future + // regression that adds one and starts populating it. + _ = e + } + } + + // Second pass — must APPEND, not overwrite. + bySession2 := map[string][]lfs.RedactionEntry{ + sessName: { + {File: "summary.md", Line: 3, Detector: "aws_access_key"}, + }, + } + _, err = writeRedactionPassesPerSession(context.Background(), ledger, + bySession2, "ox-secrets-N10-feedface", "feedface...") + require.NoError(t, err) + + got2, err := lfs.ReadSessionMeta(sessDir) + require.NoError(t, err) + require.Len(t, got2.Redactions, 2, "second pass must APPEND, not replace") + assert.Equal(t, firstPassID, got2.Redactions[0].PassID, + "earlier pass id must survive untouched") + assert.NotEqual(t, firstPassID, got2.Redactions[1].PassID, + "new pass must have a fresh id") + assert.Equal(t, "ox-secrets-N10-feedface", got2.Redactions[1].CatalogVersion) +} + +// TestWriteRedactionPassesPerSession_RefusesMissingMeta verifies the +// safety stance: if meta.json is gone, we don't synthesize a new one +// (that would mask a different bug). The redaction should already have +// been refused upstream — this is defense in depth. +func TestWriteRedactionPassesPerSession_RefusesMissingMeta(t *testing.T) { + ledger := t.TempDir() + bySession := map[string][]lfs.RedactionEntry{ + "sess-without-meta": {{File: "raw.jsonl", Line: 1, Detector: "x"}}, + } + _, err := writeRedactionPassesPerSession(context.Background(), ledger, + bySession, "v", "h") + require.Error(t, err) + assert.Contains(t, err.Error(), "has no meta.json") +} diff --git a/internal/lfs/meta.go b/internal/lfs/meta.go index cf8daad9..8744f6f5 100644 --- a/internal/lfs/meta.go +++ b/internal/lfs/meta.go @@ -106,9 +106,93 @@ type SessionMeta struct { // working unchanged. SummaryAttempts int `json:"summary_attempts,omitempty"` + // Redactions is the append-only log of redaction passes that have + // touched this session's content. Each pass records who applied it, + // which detector catalog was in force (version + hash), and a + // summary plus per-finding metadata — NEVER the matched bytes (the + // same ox-zyg7 rule that governs audit output). Designed so that + // "did the redactor run, when, with which rules, and what did it + // catch?" is auditable months later, and so re-runs after the + // catalog evolves can identify findings the older ruleset missed. + // + // Mutations MUST go through MutateSessionMeta so concurrent CLI / + // daemon writers serialize at the filesystem level — same flock + // path that prevented ox-lrrq from being recreated when this + // schema was added. + // + // omitempty so legacy meta.json files keep round-tripping and + // older readers ignore the new field. + Redactions []RedactionPass `json:"redactions,omitempty"` + Files map[string]FileRef `json:"files"` // OID manifest: filename -> ref } +// RedactionPass records a single end-to-end pass of the redactor against +// a session's content files. Append-only: a later pass NEVER overwrites +// an earlier one in meta.json; supersession is expressed via the +// Supersedes back-pointer so the audit trail stays complete. +type RedactionPass struct { + // PassID is a UUIDv7 (time-sortable) that uniquely identifies this + // pass across the ledger. Later passes can reference it via + // Supersedes when they replace a finding the earlier pass made. + PassID string `json:"pass_id"` + + // AppliedAt is the UTC instant the pass committed its changes. + AppliedAt time.Time `json:"applied_at"` + + // AppliedBy identifies what code path ran the redactor. Useful for + // distinguishing write-time chokepoint redaction from after-the- + // fact `ox session redact-history` rewrites. Examples: + // "ox session redact-history" + // "ox session redact-history (dry-run)" -- never persisted; here for clarity + // "RawWriter (session-stop)" -- future write-time integration + AppliedBy string `json:"applied_by"` + + // CatalogVersion is the human-readable identifier of the detector + // catalog (e.g. "ox-secrets-2026-05-11-N7"). Tracks WHICH ruleset + // was applied so future re-audits can decide whether the session + // needs a re-run under a newer catalog. Always paired with + // CatalogHash to detect locally-modified rules even when the + // version string matches. + CatalogVersion string `json:"catalog_version"` + + // CatalogHash is a sha256 hex string over the canonicalized + // detector set (name + pattern source + skipif + keywords). Detects + // "you said catalog v3 but your rules don't actually match the + // official v3 set" cases that the version string alone misses. + CatalogHash string `json:"catalog_hash"` + + // Summary is the aggregate finding count and per-detector + // breakdown for this pass. Cheap to read without pulling Entries. + Summary RedactionPassSummary `json:"summary"` + + // Entries lists every finding the pass acted on. Detector + file + + // line only — NEVER the matched bytes. + Entries []RedactionEntry `json:"entries,omitempty"` + + // Supersedes references PassIDs of earlier passes whose findings + // this pass replaces (e.g. a more-specific per-prefix github + // detector replacing a generic one). Empty for a fresh pass. + Supersedes []string `json:"supersedes,omitempty"` +} + +// RedactionPassSummary is the at-a-glance roll-up for a RedactionPass. +type RedactionPassSummary struct { + Total int `json:"total"` + ByDetector map[string]int `json:"by_detector,omitempty"` +} + +// RedactionEntry is a single finding within a RedactionPass. Records +// where the match was (file + line) and which detector fired, never +// the matched bytes. char_offset / match_len could be added later for +// finer-grained re-audit; intentionally omitted today to keep +// meta.json compact. +type RedactionEntry struct { + File string `json:"file"` // relative to session dir, e.g. "raw.jsonl" + Line int `json:"line"` // 1-based line number in pre-redaction file + Detector string `json:"detector"` // SecretPattern.Name +} + // MaxSummaryAttempts caps how many failure-stub-producing daemon LLM // summarization passes will run for a single session before the daemon // flips SummaryStatus to "unrecoverable" and stops retrying. Three is diff --git a/internal/session/catalog.go b/internal/session/catalog.go new file mode 100644 index 00000000..2909dafe --- /dev/null +++ b/internal/session/catalog.go @@ -0,0 +1,99 @@ +package session + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "sort" + "strings" +) + +// Catalog identity for the redactor's currently-loaded pattern set. +// +// "Catalog" here means the live SecretPattern slice held by a Redactor — +// `internal/session/secrets.DefaultPatterns()` plus any AddPattern +// extensions stacked on top. Two pieces of identity are exposed: +// +// - CatalogVersion is a stable, human-readable label tied to the +// count and name set of patterns. Bumps when the production +// catalog gains or loses a detector. NOT a security mechanism — +// it can be coincidentally equal across mis-aligned builds; the +// hash is what proves alignment. +// +// - CatalogHash is a sha256 over the canonicalized pattern set: +// name + regex source + sorted SkipIf + sorted Keywords for each +// pattern, then sorted across patterns so declaration order in +// source doesn't drift the hash. Tampering with the rule set +// in-process (AddPattern, removing the heroku Keywords guard, +// etc.) changes the hash even when the count and names match. +// +// The pair is recorded in lfs.RedactionPass on every redact-history +// run so future audits can answer "what rules actually scanned this +// session?" without trusting an unsigned version string in isolation. +// +// Identity is computed eagerly during Redactor construction and +// recomputed under the existing write lock whenever AddPattern +// mutates the set. Read-side accessors take the read lock and return +// the cached values — no compute on the hot path. + +// canonicalPattern is a stable text representation of one SecretPattern, +// used as input to the catalog hash. Empty Pattern is skipped at the +// caller; SkipIf and Keywords are sorted so insertion order doesn't +// drift the hash. +func canonicalPattern(p SecretPattern) string { + skip := append([]string(nil), p.SkipIf...) + sort.Strings(skip) + kws := append([]string(nil), p.Keywords...) + sort.Strings(kws) + regex := "" + if p.Pattern != nil { + regex = p.Pattern.String() + } + return fmt.Sprintf("name=%s\nregex=%s\nskip=%s\nkeywords=%s\n", + p.Name, regex, strings.Join(skip, ","), strings.Join(kws, ",")) +} + +// computeCatalogIdentity returns (version, hash) for the given pattern +// set. Deterministic — same inputs always produce the same outputs, +// regardless of pattern declaration order in source. +func computeCatalogIdentity(patterns []SecretPattern) (string, string) { + canon := make([]string, 0, len(patterns)) + for _, p := range patterns { + if p.Pattern == nil { + continue + } + canon = append(canon, canonicalPattern(p)) + } + sort.Strings(canon) // hash is order-independent + h := sha256.New() + for _, c := range canon { + h.Write([]byte(c)) + h.Write([]byte{0}) // delimiter so concatenation can't collide + } + hash := hex.EncodeToString(h.Sum(nil)) + + // Version: pattern count + 8-char hash prefix. Bumps automatically + // whenever the set changes, but stays stable across rebuilds with + // identical rules. Format chosen to be greppable in commit history + // and short enough to print in a status header. + version := fmt.Sprintf("ox-secrets-N%d-%s", len(canon), hash[:8]) + return version, hash +} + +// CatalogVersion returns the human-readable catalog identifier for the +// Redactor's current pattern set. Bumps automatically when patterns are +// added or removed. See package doc above for the trust model. +func (r *Redactor) CatalogVersion() string { + r.mu.RLock() + defer r.mu.RUnlock() + return r.catalogVersion +} + +// CatalogHash returns a sha256 hex string fingerprint of the Redactor's +// current pattern set. Combined with CatalogVersion this lets persisted +// records prove which detectors were in effect at scan time. +func (r *Redactor) CatalogHash() string { + r.mu.RLock() + defer r.mu.RUnlock() + return r.catalogHash +} diff --git a/internal/session/catalog_test.go b/internal/session/catalog_test.go new file mode 100644 index 00000000..eeab250d --- /dev/null +++ b/internal/session/catalog_test.go @@ -0,0 +1,90 @@ +package session + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCatalogIdentity_DeterministicAndStableAcrossDeclarationOrder +// verifies that ScanForSecrets / RedactString consumers can rely on the +// (version, hash) pair to mean "same effective ruleset" regardless of +// the order in which pattern blocks happen to appear in DefaultPatterns. +// +// Failure prevented: a refactor that reorders patterns silently bumps +// the catalog hash and forces unnecessary re-redaction of every session. +func TestCatalogIdentity_DeterministicAndStableAcrossDeclarationOrder(t *testing.T) { + r1 := NewRedactor() + r2 := NewRedactor() + v1, h1 := r1.CatalogVersion(), r1.CatalogHash() + v2, h2 := r2.CatalogVersion(), r2.CatalogHash() + assert.Equal(t, v1, v2, "two fresh Redactors must share a catalog version") + assert.Equal(t, h1, h2, "two fresh Redactors must share a catalog hash") + + // Reorder the patterns slice manually; identity must NOT change. + patterns := DefaultPatterns() + reversed := make([]SecretPattern, len(patterns)) + for i, p := range patterns { + reversed[len(patterns)-1-i] = p + } + r3 := NewRedactorWithPatterns(reversed) + assert.Equal(t, h1, r3.CatalogHash(), + "catalog hash MUST be order-independent across the pattern slice") +} + +// TestCatalogIdentity_ChangesWhenPatternAdded ensures that adding a +// detector through the documented AddPattern path bumps both the +// version and the hash. Without this, ox-8bfh's audit replay can't +// know whether a session was scanned with the new detector. +func TestCatalogIdentity_ChangesWhenPatternAdded(t *testing.T) { + r := NewRedactor() + beforeV, beforeH := r.CatalogVersion(), r.CatalogHash() + + r.AddPattern(SecretPattern{ + Name: "test_extra_pattern", + Pattern: regexp.MustCompile(`extra-[A-Za-z0-9]{8}`), + Redact: "[REDACTED_EXTRA]", + Keywords: []string{"extra-"}, + }) + + assert.NotEqual(t, beforeH, r.CatalogHash(), + "adding a pattern must change the catalog hash") + assert.NotEqual(t, beforeV, r.CatalogVersion(), + "adding a pattern must change the catalog version (count or hash prefix bumps)") +} + +// TestCatalogIdentity_HashSensitiveToRuleTampering proves that swapping +// the regex of a single detector (without changing names or count) +// still bumps the hash. This is the key property that makes the version +// alone insufficient — an in-process modification that "loosens" a +// detector must be detectable by a downstream auditor. +func TestCatalogIdentity_HashSensitiveToRuleTampering(t *testing.T) { + pristine := DefaultPatterns() + tampered := make([]SecretPattern, len(pristine)) + copy(tampered, pristine) + // Find heroku_key and remove its Keywords guard — the exact failure + // mode ox-zukx had to fix. Tampering should be detectable. + for i := range tampered { + if tampered[i].Name == "heroku_key" { + tampered[i].Keywords = nil + break + } + } + + rOK := NewRedactorWithPatterns(pristine) + rTampered := NewRedactorWithPatterns(tampered) + + require.NotEqual(t, rOK.CatalogHash(), rTampered.CatalogHash(), + "removing a Keywords guard must change the catalog hash") +} + +// TestCatalogIdentity_VersionFormatIsGreppable locks the version string +// shape so log search / commit messages can find it later. Format: +// "ox-secrets-N-<8 hex chars>". +func TestCatalogIdentity_VersionFormatIsGreppable(t *testing.T) { + r := NewRedactor() + v := r.CatalogVersion() + assert.Regexp(t, `^ox-secrets-N\d+-[0-9a-f]{8}$`, v) +} diff --git a/internal/session/secrets.go b/internal/session/secrets.go index 509629b9..d78f0af8 100644 --- a/internal/session/secrets.go +++ b/internal/session/secrets.go @@ -384,31 +384,41 @@ func DefaultPatterns() []SecretPattern { } } -// Redactor handles secret detection and redaction +// Redactor handles secret detection and redaction. Holds the live +// SecretPattern slice plus its cached catalog identity (version+hash); +// see catalog.go for the trust model that ox-8bfh requires. type Redactor struct { - patterns []SecretPattern - mu sync.RWMutex + patterns []SecretPattern + catalogVersion string + catalogHash string + mu sync.RWMutex } // NewRedactor creates a new Redactor with default patterns func NewRedactor() *Redactor { - return &Redactor{ - patterns: DefaultPatterns(), - } + r := &Redactor{patterns: DefaultPatterns()} + r.catalogVersion, r.catalogHash = computeCatalogIdentity(r.patterns) + return r } // NewRedactorWithPatterns creates a Redactor with custom patterns func NewRedactorWithPatterns(patterns []SecretPattern) *Redactor { - return &Redactor{ - patterns: patterns, - } + r := &Redactor{patterns: patterns} + r.catalogVersion, r.catalogHash = computeCatalogIdentity(r.patterns) + return r } -// AddPattern adds an additional pattern to the redactor +// AddPattern adds an additional pattern to the redactor. The catalog +// identity (version + hash) is recomputed under the write lock so +// subsequent CatalogVersion / CatalogHash calls reflect the change. +// Required by ox-8bfh: a session redacted with a tampered ruleset +// must carry a different fingerprint than one redacted with the +// pristine default catalog. func (r *Redactor) AddPattern(pattern SecretPattern) { r.mu.Lock() defer r.mu.Unlock() r.patterns = append(r.patterns, pattern) + r.catalogVersion, r.catalogHash = computeCatalogIdentity(r.patterns) } // RedactionResult contains details about a redaction From 5e83877abcdae79153a38cd1d10df0a5f27cefb8 Mon Sep 17 00:00:00 2001 From: Ryan Snodgrass Date: Mon, 11 May 2026 14:24:01 -0700 Subject: [PATCH 3/5] refactor(session): split redact-history into 'audit' + 'redact' Co-Authored-By: SageOx SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T21-15-ryan-OxOlcb/view --- cmd/ox/doctor_ledger_secrets.go | 6 +- cmd/ox/session_redact_history.go | 154 +++++++++++++++++++-------- cmd/ox/session_redact_record.go | 2 +- cmd/ox/session_redact_record_test.go | 2 +- 4 files changed, 117 insertions(+), 47 deletions(-) diff --git a/cmd/ox/doctor_ledger_secrets.go b/cmd/ox/doctor_ledger_secrets.go index 4e0ec727..8a5efb80 100644 --- a/cmd/ox/doctor_ledger_secrets.go +++ b/cmd/ox/doctor_ledger_secrets.go @@ -87,7 +87,7 @@ const ledgerSecretsSizeCap = 8 * 1024 * 1024 // Per ox-zyg7: read-only by default; the check NEVER prints matched bytes, // NEVER uploads findings, NEVER mutates the ledger. The `fix` argument is // ignored — there's no auto-fix for "credential in committed history". -// The companion `ox session redact-history` tool (ox-pd5f) handles +// The companion `ox session redact` tool (ox-pd5f) handles // per-finding interactive cleanup. func checkLedgerSecrets(fix bool) checkResult { name := "Ledger credential scan" @@ -174,7 +174,7 @@ func checkLedgerSecrets(fix bool) checkResult { summary := fmt.Sprintf("found %d credential pattern(s) across %d distinct detectors (%s)", totalCount, len(result.Findings), scope) - guidance := "Run `ox session redact-history` for interactive per-finding cleanup. " + + guidance := "Run `ox session audit` to see per-line findings, then `ox session redact` for interactive cleanup. " + "For already-pushed commits, see docs/security/credential-redaction.md." return FailedCheck(name, summary+"\n"+details.String(), guidance) @@ -215,7 +215,7 @@ func resolveLedgerPathForAudit(localCfg *config.LocalConfig) string { // openSessionContent writes hydrated bytes to .sageox/cache/sessions/ // per .claude/rules/cache-only-design.md, never in-place. // -// Exposed (unexported) for use by `ox session redact-history` so both +// Exposed (unexported) for use by `ox session audit` and `ox session redact` so all // surfaces share the same definition of "what counts as a finding." func scanLedgerForSecrets(projectRoot, ledgerPath string) (*ledgerSecretsScanResult, error) { redactor := session.NewRedactor() diff --git a/cmd/ox/session_redact_history.go b/cmd/ox/session_redact_history.go index 07361368..bff2f730 100644 --- a/cmd/ox/session_redact_history.go +++ b/cmd/ox/session_redact_history.go @@ -24,8 +24,9 @@ import ( "github.com/spf13/cobra" ) -// ox session redact-history is the forensic cleanup companion to -// `ox doctor --check=ledger-secrets`. The doctor check tells the user +// ox session redact is the forensic cleanup companion to +// `ox doctor --check=ledger-secrets` / `ox session audit`. The audit +// surfaces tell the user // THAT they have leaked credentials in their local Ledger; this command // fixes them, with strict safety rails per ox-pd5f: // @@ -48,16 +49,70 @@ import ( // - Operator auto-approves and the tool redacts content the user // wanted to keep (e.g. legitimate examples in documentation). -var sessionRedactHistoryCmd = &cobra.Command{ - Use: "redact-history", - Short: "Interactively redact credentials from historical local sessions", - Long: `Scan the local Ledger for credential patterns in committed-but-not-yet-pushed -sessions, then interactively cleanup each finding. +// Two surfaces over the same scan code path (ox-zukx / ox-8bfh +// follow-up). The pre-split `ox session redact-history` was misleading +// because most users running it with --dry-run were doing an audit, +// not preparing a destructive rewrite — the name fought the intent. +// +// ox session audit — read-only audit. Always hydrates LFS pointers +// first (LFS Batch API fetch of every dehydrated +// session — can be slow on a large ledger), +// prints catalog identity, then reports per-line +// findings grouped pushed-vs-unpushed. Never +// writes anything. Always safe to run; the cost +// is bandwidth + time, not data integrity. +// +// ox session redact — destructive interactive rewrite. Runs the same +// hydration + scan as `audit`, then snapshots the +// ledger, prompts per-file [y/N/q], rewrites +// bytes, appends a RedactionPass to each +// affected session's meta.json (ox-8bfh), and +// amends the holding commit. Refuses findings +// in already-pushed commits. +// +// Both share the same hydration + scan + per-file enumeration +// (runRedactHistoryWorkflow); the difference is whether the interactive +// rewrite tail runs. + +var sessionAuditCmd = &cobra.Command{ + Use: "audit", + Short: "Audit session recordings for credential patterns (read-only; expensive)", + Long: `Audit every session recording in the local Ledger for credential patterns. +Read-only — never modifies files, never amends commits, never uploads. + +Cost: this command force-hydrates every LFS-pointer session file via the +LFS Batch API before scanning, because pointer-stub bytes match no +credential pattern and would silently produce a clean-looking lie. +Hydration is bounded by network throughput and the number of dehydrated +sessions; on a fresh clone with hundreds of sessions this can take +minutes and pull tens of megabytes. Sessions that fail to hydrate are +surfaced as a Warning so partial coverage is never mistaken for clean. + +Findings are reported with detector name + file + line; matched bytes +are NEVER printed (ox-zyg7). The output stamps the catalog version + a +sha256 hash that produced the findings, so future re-audits can decide +whether a newer ruleset would catch additional leaks. + +For interactive cleanup of any findings, run ` + "`ox session redact`" + `.`, + RunE: runSessionAudit, +} + +var sessionRedactCmd = &cobra.Command{ + Use: "redact", + Short: "Interactively redact credentials in session recordings (destructive; expensive)", + Long: `Interactively redact each credential finding in committed-but-not-yet-pushed +session recordings. Runs the full audit pass first, then prompts +per-file for approval. + +Cost: same hydration cost as ` + "`ox session audit`" + ` (LFS Batch API fetch of +every dehydrated session), plus a per-file rewrite + ledger snapshot + +git amend on every approved file. Plan for minutes-not-seconds on a +large ledger. Workflow: - 1. ` + "`ox doctor --check=ledger-secrets`" + ` first — read-only audit. - 2. ` + "`ox session redact-history`" + ` — interactive cleanup. + 1. ` + "`ox session audit`" + ` first — read-only, confirms what would change. + 2. ` + "`ox session redact`" + ` — interactive cleanup. 3. After cleanup, ` + "`ox session push`" + ` (or normal session-stop flow) republishes. Safety: @@ -69,61 +124,75 @@ Safety: correct response is: rotate the leaked credential, mark the finding as known, and follow up via the team comms process. - Per-finding human approval. There is no bulk-approve flag. - - --dry-run lists findings without snapshotting or modifying anything.`, - RunE: runSessionRedactHistory, + - Each redaction pass appends a RedactionPass entry to the session's + meta.json (ox-8bfh) recording WHO redacted, WHEN, with WHICH catalog + version+hash, and WHAT was caught — never the matched bytes.`, + RunE: runSessionRedact, } -// dry-run and ledger-path are intentionally the only flags here. Adding a -// "yes to all" override is deliberately omitted — per ox-pd5f the -// per-finding gate IS the safety mechanism. +// Flag storage. Each command reads from its own variable so cobra's +// flag namespacing stays clean. var ( - redactHistoryDryRun bool - redactHistoryLedgerPath string - redactHistoryBackupDir string + auditLedgerPath string + + redactLedgerPath string + redactBackupDir string ) func init() { - sessionRedactHistoryCmd.Flags().BoolVar(&redactHistoryDryRun, "dry-run", false, - "Show what would change without snapshotting or modifying anything") - sessionRedactHistoryCmd.Flags().StringVar(&redactHistoryLedgerPath, "ledger-path", "", + sessionAuditCmd.Flags().StringVar(&auditLedgerPath, "ledger-path", "", "Override the ledger path (defaults to the current project's ledger)") - sessionRedactHistoryCmd.Flags().StringVar(&redactHistoryBackupDir, "backup-dir", "", + sessionCmd.AddCommand(sessionAuditCmd) + + sessionRedactCmd.Flags().StringVar(&redactLedgerPath, "ledger-path", "", + "Override the ledger path (defaults to the current project's ledger)") + sessionRedactCmd.Flags().StringVar(&redactBackupDir, "backup-dir", "", "Override the backup directory (defaults to ~/.local/share/sageox/backups/redact-history/)") + sessionCmd.AddCommand(sessionRedactCmd) +} - // register under the existing `ox session` parent (see session.go). - sessionCmd.AddCommand(sessionRedactHistoryCmd) +// runSessionAudit is the cobra entrypoint for the read-only audit +// surface. Pins DryRun=true on the shared workflow so no rewrite can +// happen even if a future caller passes the wrong opts. +func runSessionAudit(cmd *cobra.Command, args []string) error { + ledgerPath, err := resolveLedgerPathForSessionCmd(auditLedgerPath) + if err != nil { + return err + } + opts := redactHistoryOptions{ + ProjectRoot: findGitRoot(), + LedgerPath: ledgerPath, + DryRun: true, + Stdin: cmd.InOrStdin(), + Stdout: cmd.OutOrStdout(), + } + return runRedactHistoryWorkflow(opts) } -// runSessionRedactHistory is the cobra RunE. It does only argument -// validation + branching; the real workflow lives in -// runRedactHistoryWorkflow so tests can drive it directly without going -// through cobra. -func runSessionRedactHistory(cmd *cobra.Command, args []string) error { - ledgerPath, err := redactHistoryResolveLedger() +// runSessionRedact is the cobra entrypoint for the destructive +// interactive surface. Pins DryRun=false; rewrite proceeds. +func runSessionRedact(cmd *cobra.Command, args []string) error { + ledgerPath, err := resolveLedgerPathForSessionCmd(redactLedgerPath) if err != nil { return err } - // ProjectRoot drives openSessionContent's hydration call (which uses the - // project's auth/endpoint to talk to the LFS Batch API). Empty when - // --ledger-path is used without a project context; hydration will fail - // in that case but the scan still works on whatever's already in cache. - projectRoot := findGitRoot() opts := redactHistoryOptions{ - ProjectRoot: projectRoot, + ProjectRoot: findGitRoot(), LedgerPath: ledgerPath, - DryRun: redactHistoryDryRun, - BackupDir: redactHistoryBackupDir, + DryRun: false, + BackupDir: redactBackupDir, Stdin: cmd.InOrStdin(), Stdout: cmd.OutOrStdout(), } return runRedactHistoryWorkflow(opts) } -// redactHistoryResolveLedger returns the ledger path, honoring the -// --ledger-path flag if set, otherwise resolving from the project. -func redactHistoryResolveLedger() (string, error) { - if redactHistoryLedgerPath != "" { - return redactHistoryLedgerPath, nil +// resolveLedgerPathForSessionCmd is the common ledger-path resolver +// shared by audit and redact. Returns the explicit override when set, +// otherwise the current project's ledger. +func resolveLedgerPathForSessionCmd(override string) (string, error) { + if override != "" { + return override, nil } gitRoot := findGitRoot() if gitRoot == "" { @@ -143,6 +212,7 @@ func redactHistoryResolveLedger() (string, error) { return path, nil } + // redactHistoryOptions bundles inputs so the workflow can be unit-tested // with synthetic stdin/stdout and an override backup directory. type redactHistoryOptions struct { diff --git a/cmd/ox/session_redact_record.go b/cmd/ox/session_redact_record.go index 0f503067..6485840d 100644 --- a/cmd/ox/session_redact_record.go +++ b/cmd/ox/session_redact_record.go @@ -74,7 +74,7 @@ func writeRedactionPassesPerSession( return nil, fmt.Errorf("generate pass id: %w", err) } appliedAt := time.Now().UTC() - appliedBy := "ox session redact-history" + appliedBy := "ox session redact" out := make([]string, 0, len(bySession)) for sessionName, entries := range bySession { diff --git a/cmd/ox/session_redact_record_test.go b/cmd/ox/session_redact_record_test.go index 41ae13f8..5960747e 100644 --- a/cmd/ox/session_redact_record_test.go +++ b/cmd/ox/session_redact_record_test.go @@ -102,7 +102,7 @@ func TestWriteRedactionPassesPerSession_AppendsToMeta(t *testing.T) { require.Len(t, got1.Redactions, 1) assert.Equal(t, "ox-secrets-N9-cafebabe", got1.Redactions[0].CatalogVersion) assert.Equal(t, 2, got1.Redactions[0].Summary.Total) - assert.Equal(t, "ox session redact-history", got1.Redactions[0].AppliedBy) + assert.Equal(t, "ox session redact", got1.Redactions[0].AppliedBy) firstPassID := got1.Redactions[0].PassID assert.NotEmpty(t, firstPassID, "pass id must be populated") From 1b4624e1cdfa22512a29c723ef2568e21fe82cff Mon Sep 17 00:00:00 2001 From: Ryan Snodgrass Date: Mon, 11 May 2026 14:50:11 -0700 Subject: [PATCH 4/5] test: fixture cleanup for ox-zukx session scope + ox-def1 detector rename Co-Authored-By: SageOx SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T21-15-ryan-OxOlcb/view --- cmd/ox/prepush_scan_test.go | 4 +- cmd/ox/session_redact_history_test.go | 60 ++++++++++++++++++++------- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/cmd/ox/prepush_scan_test.go b/cmd/ox/prepush_scan_test.go index 3217e48c..3eb62b8a 100644 --- a/cmd/ox/prepush_scan_test.go +++ b/cmd/ox/prepush_scan_test.go @@ -129,7 +129,9 @@ func TestRunPrePushSecretGate_RefusesWhenSecretsPresent(t *testing.T) { msg := err.Error() assert.Contains(t, msg, "Push refused") - assert.Contains(t, msg, "github_token") // detector name + // After ox-def1 the broad `github_token` detector was split per-prefix; + // a full-shape ghp_<36> matches the personal-access-token variant. + assert.Contains(t, msg, "github_personal_access_token") assert.Contains(t, msg, "sessions/leak.jsonl") // MUST NOT include the secret bytes assert.NotContains(t, msg, "alphabetabcdefghijklmnopqrstuvwxyz") diff --git a/cmd/ox/session_redact_history_test.go b/cmd/ox/session_redact_history_test.go index a9a00f0a..119b60c2 100644 --- a/cmd/ox/session_redact_history_test.go +++ b/cmd/ox/session_redact_history_test.go @@ -167,12 +167,15 @@ func TestClassifyFindingsByPushStatus_SplitsCorrectly(t *testing.T) { } // TestRunRedactHistoryWorkflow_DryRunListsButDoesntModify is the load- -// bearing test that --dry-run is truly inert. +// bearing test that --dry-run is truly inert. Fixtures use the +// session-scoped layout enforced by ox-zukx (scan iterates +// sessions//, not loose files at sessions/ root). func TestRunRedactHistoryWorkflow_DryRunListsButDoesntModify(t *testing.T) { work := makeLedgerWithCommitAndOrigin(t, map[string]string{ - "sessions/leak.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/leaky/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", }) - originalBytes, err := os.ReadFile(filepath.Join(work, "sessions/leak.jsonl")) + leakPath := filepath.Join(work, "sessions/leaky/raw.jsonl") + originalBytes, err := os.ReadFile(leakPath) require.NoError(t, err) backupDir := t.TempDir() out := &bytes.Buffer{} @@ -187,7 +190,7 @@ func TestRunRedactHistoryWorkflow_DryRunListsButDoesntModify(t *testing.T) { require.NoError(t, err) // file must be unchanged - after, err := os.ReadFile(filepath.Join(work, "sessions/leak.jsonl")) + after, err := os.ReadFile(leakPath) require.NoError(t, err) assert.Equal(t, originalBytes, after, "dry-run must not modify files") @@ -198,17 +201,21 @@ func TestRunRedactHistoryWorkflow_DryRunListsButDoesntModify(t *testing.T) { // output must mention the finding and the path:line assert.Contains(t, out.String(), "aws_access_key") - assert.Contains(t, out.String(), "sessions/leak.jsonl:1") + assert.Contains(t, out.String(), "sessions/leaky/raw.jsonl:1") // output must NOT leak the secret bytes assert.NotContains(t, out.String(), "AKIAIOSFODNN7EXAMPLE") } // TestRunRedactHistoryWorkflow_InteractiveYesRedacts verifies that // answering "y" to the prompt actually scrubs the file and amends the -// commit, while preserving the canary in the snapshot. +// commit, while preserving the canary in the snapshot. Also exercises +// the ox-8bfh path that appends a RedactionPass to meta.json — the +// fixture seeds a minimum-valid meta.json so MutateSessionMeta has +// something to mutate. func TestRunRedactHistoryWorkflow_InteractiveYesRedacts(t *testing.T) { work := makeLedgerWithCommitAndOrigin(t, map[string]string{ - "sessions/leak.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/leaky/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/leaky/meta.json": minimalMetaJSON("leaky", "OxLEAK"), }) backupDir := t.TempDir() out := &bytes.Buffer{} @@ -222,7 +229,7 @@ func TestRunRedactHistoryWorkflow_InteractiveYesRedacts(t *testing.T) { require.NoError(t, err) // the working-tree file must be scrubbed - after, err := os.ReadFile(filepath.Join(work, "sessions/leak.jsonl")) + after, err := os.ReadFile(filepath.Join(work, "sessions/leaky/raw.jsonl")) require.NoError(t, err) assert.NotContains(t, string(after), "AKIA", "file must be redacted in place") assert.Contains(t, string(after), "[REDACTED_AWS_KEY]") @@ -246,9 +253,10 @@ func TestRunRedactHistoryWorkflow_InteractiveYesRedacts(t *testing.T) { // not the operator approves the change. func TestRunRedactHistoryWorkflow_NoSkipsLeavesFileUnchanged(t *testing.T) { work := makeLedgerWithCommitAndOrigin(t, map[string]string{ - "sessions/leak.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/leaky/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", }) - originalBytes, err := os.ReadFile(filepath.Join(work, "sessions/leak.jsonl")) + leakPath := filepath.Join(work, "sessions/leaky/raw.jsonl") + originalBytes, err := os.ReadFile(leakPath) require.NoError(t, err) backupDir := t.TempDir() out := &bytes.Buffer{} @@ -261,7 +269,7 @@ func TestRunRedactHistoryWorkflow_NoSkipsLeavesFileUnchanged(t *testing.T) { }) require.NoError(t, err) - after, err := os.ReadFile(filepath.Join(work, "sessions/leak.jsonl")) + after, err := os.ReadFile(leakPath) require.NoError(t, err) assert.Equal(t, originalBytes, after, "declining prompt must leave file unchanged") @@ -274,10 +282,11 @@ func TestRunRedactHistoryWorkflow_NoSkipsLeavesFileUnchanged(t *testing.T) { // "q" stops the loop before processing subsequent files. func TestRunRedactHistoryWorkflow_QuitAbortsWithoutWritingMore(t *testing.T) { work := makeLedgerWithCommitAndOrigin(t, map[string]string{ - "sessions/a.jsonl": "AKIAIOSFODNN7EXAMPLE\n", - "sessions/b.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/sess-a/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", + "sessions/sess-b/raw.jsonl": "AKIAIOSFODNN7EXAMPLE\n", }) - originalB, err := os.ReadFile(filepath.Join(work, "sessions/b.jsonl")) + bPath := filepath.Join(work, "sessions/sess-b/raw.jsonl") + originalB, err := os.ReadFile(bPath) require.NoError(t, err) out := &bytes.Buffer{} @@ -289,13 +298,32 @@ func TestRunRedactHistoryWorkflow_QuitAbortsWithoutWritingMore(t *testing.T) { }) require.NoError(t, err) - // b.jsonl must be unchanged (we quit before reaching it OR after the first prompt) - stillThere, err := os.ReadFile(filepath.Join(work, "sessions/b.jsonl")) + // sess-b raw.jsonl must be unchanged (we quit before reaching it OR after the first prompt) + stillThere, err := os.ReadFile(bPath) require.NoError(t, err) assert.Equal(t, originalB, stillThere) assert.Contains(t, out.String(), "Aborting") } +// minimalMetaJSON returns a JSON string for a meta.json that just clears +// the lfs.SessionMeta.Validate() invariants. Used by tests that exercise +// the real-write path (ox-8bfh appends a RedactionPass via +// MutateSessionMeta, which insists on an existing meta.json). Fields +// kept tiny on purpose so a future Validate() addition won't silently +// break — the test will fail loudly and prompt an explicit update here. +func minimalMetaJSON(sessionName, agentID string) string { + return `{ + "version": "1.0", + "session_name": "` + sessionName + `", + "username": "test-user", + "agent_id": "` + agentID + `", + "agent_type": "claude-code", + "created_at": "2026-05-11T00:00:00Z", + "files": {} +} +` +} + // mustGitOutput runs git -C dir args... and returns stdout. func mustGitOutput(t *testing.T, dir string, args ...string) string { t.Helper() From fe4f4050cb81338fed1dc0c56355f8b087f5b9f8 Mon Sep 17 00:00:00 2001 From: Ryan Snodgrass Date: Mon, 11 May 2026 15:05:29 -0700 Subject: [PATCH 5/5] fix(redaction): address CodeRabbit review feedback on PR #599 (ox-q42i) Co-Authored-By: SageOx SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-05-11T21-15-ryan-OxOlcb/view --- cmd/ox/doctor_ledger_secrets.go | 87 +++++++++++++++++++++------ cmd/ox/doctor_ledger_secrets_test.go | 16 ++++- cmd/ox/session_redact_history.go | 38 +++++++++++- cmd/ox/session_redact_hydrate.go | 6 ++ cmd/ox/session_redact_record_test.go | 43 ++++++++++--- internal/lfs/meta.go | 28 +++++++-- internal/session/redact_rules_test.go | 10 +-- internal/session/secrets.go | 51 +++++++++++++--- internal/session/secrets_test.go | 34 +++++------ 9 files changed, 251 insertions(+), 62 deletions(-) diff --git a/cmd/ox/doctor_ledger_secrets.go b/cmd/ox/doctor_ledger_secrets.go index 8a5efb80..d981717c 100644 --- a/cmd/ox/doctor_ledger_secrets.go +++ b/cmd/ox/doctor_ledger_secrets.go @@ -35,16 +35,27 @@ type ledgerSecretsFinding struct { // ledgerSecretsScanResult is what checkLedgerSecrets passes back. Findings // are keyed by detector name; FilesScanned/SessionsScanned describe scope // for "scanned N session recordings, found 0 secrets" reassurance. -// SessionsHydrated/SessionsSkipped report on the LFS auto-hydration loop: -// a non-zero SessionsSkipped means coverage is incomplete and the result -// MUST NOT be reported as a clean bill of health. +// SessionsHydrated, SessionsHydrationFailed, and SessionsEmpty report on +// the LFS auto-hydration loop. A non-zero SessionsHydrationFailed means +// coverage is incomplete and the result MUST NOT be reported as a clean +// bill of health. SessionsEmpty is benign — it just means a session dir +// existed but had no allowlisted files (e.g. legacy session with only +// .html artifacts). +// +// SessionsSkipped is retained as the legacy alias and equals +// SessionsHydrationFailed + SessionsEmpty for callers that don't need to +// distinguish the two. The check itself uses the finer-grained fields so +// "scan failed" and "nothing to scan" don't get conflated in the +// PassedCheck / FailedCheck decision. type ledgerSecretsScanResult struct { - LedgerPath string - FilesScanned int - SessionsScanned int - SessionsHydrated int // session dirs we auto-hydrated to cache during the scan - SessionsSkipped int // session dirs we couldn't read at all (hydration failed) - Findings map[string]*ledgerSecretsFinding + LedgerPath string + FilesScanned int + SessionsScanned int + SessionsHydrated int // sessions where at least one pointer was fetched from LFS during this call + SessionsHydrationFailed int // sessions where hydration or read errored — coverage is incomplete + SessionsEmpty int // sessions where no allowlisted file was readable (no failure, just nothing to scan) + SessionsSkipped int // legacy alias: SessionsHydrationFailed + SessionsEmpty + Findings map[string]*ledgerSecretsFinding } // ledgerSecretsScanExts lists file extensions worth scanning inside a @@ -146,12 +157,24 @@ func checkLedgerSecrets(fix bool) checkResult { if result.SessionsHydrated > 0 { scope += fmt.Sprintf("; auto-hydrated %d session(s) from LFS", result.SessionsHydrated) } - if result.SessionsSkipped > 0 { - scope += fmt.Sprintf("; %d session(s) unscannable (hydration failed)", result.SessionsSkipped) + if result.SessionsHydrationFailed > 0 { + scope += fmt.Sprintf("; %d session(s) unscannable (hydration failed)", result.SessionsHydrationFailed) + } + if result.SessionsEmpty > 0 { + scope += fmt.Sprintf("; %d session(s) had no allowlisted content", result.SessionsEmpty) } - if len(result.Findings) == 0 { + // Decision matrix (ox-zukx + ox-8bfh): a credible audit must not + // report a clean bill of health when coverage is incomplete. + // Hydration failures are the broken-coverage signal; empty sessions + // (no allowlisted content / over-cap files) are benign. + switch { + case len(result.Findings) == 0 && result.SessionsHydrationFailed == 0: return PassedCheck(name, scope+"; no credential patterns found") + case len(result.Findings) == 0 && result.SessionsHydrationFailed > 0: + return FailedCheck(name, + scope+"; no findings reported BUT scan coverage is incomplete", + "Re-run after fixing connectivity / auth; until then this result is not a clean bill of health.") } // Build the failure message without ever including matched bytes. @@ -244,12 +267,17 @@ func scanLedgerForSecrets(projectRoot, ledgerPath string) (*ledgerSecretsScanRes sessionDir := filepath.Join(sessionsRoot, sessionName) files, err := os.ReadDir(sessionDir) if err != nil { + // Couldn't even list the session dir. Treat as a hard failure + // (not "nothing to scan") so the check downgrades the result. + result.SessionsHydrationFailed++ result.SessionsSkipped++ continue } hydratedThisSession := false + anyAllowlistedSeen := false anyFileRead := false + anyFileFailed := false for _, fEntry := range files { if fEntry.IsDir() { continue @@ -258,32 +286,57 @@ func scanLedgerForSecrets(projectRoot, ledgerPath string) (*ledgerSecretsScanRes if !ledgerSecretsScanExts[strings.ToLower(filepath.Ext(filename))] { continue } + anyAllowlistedSeen = true contentPath, hydratedNow, err := resolveSessionContentForScan(projectRoot, ledgerPath, sessionName, filename) if err != nil { - // Hydration failed (network, missing LFS object, etc.). Skip - // this file but keep going — other files in this session - // might still be locally readable. + // Hydration failed (network, missing LFS object, etc.). + // That's a real failure, not "no content to scan" — + // flag it so the check downgrades the result. + anyFileFailed = true continue } if hydratedNow { hydratedThisSession = true } info, err := os.Stat(contentPath) - if err != nil || info.Size() > ledgerSecretsSizeCap { + if err != nil { + anyFileFailed = true + continue + } + if info.Size() > ledgerSecretsSizeCap { + // Over-cap is a deliberate skip, not a failure — don't + // flag it as hydration-failed, but don't count the file + // as "read" either. continue } anyFileRead = true result.FilesScanned++ relForReport := filepath.Join("sessions", sessionName, filename) if err := scanLedgerFileForSecrets(redactor, contentPath, relForReport, info.ModTime(), result); err != nil { + anyFileFailed = true continue } } if hydratedThisSession { result.SessionsHydrated++ } - if !anyFileRead { + switch { + case anyFileFailed: + // At least one file in this session couldn't be hydrated or + // read. Coverage is incomplete — surface as a hydration + // failure so the doctor check downgrades the result. + result.SessionsHydrationFailed++ + result.SessionsSkipped++ + case !anyFileRead && anyAllowlistedSeen: + // Allowlisted files existed but all were over the size cap. + // Benign — nothing to scan, but no failure either. + result.SessionsEmpty++ + result.SessionsSkipped++ + case !anyFileRead && !anyAllowlistedSeen: + // Session dir has no allowlisted content at all (e.g. + // legacy session with only .html). Benign skip. + result.SessionsEmpty++ result.SessionsSkipped++ } } diff --git a/cmd/ox/doctor_ledger_secrets_test.go b/cmd/ox/doctor_ledger_secrets_test.go index fff5036c..4b07d604 100644 --- a/cmd/ox/doctor_ledger_secrets_test.go +++ b/cmd/ox/doctor_ledger_secrets_test.go @@ -106,8 +106,20 @@ func TestScanLedgerForSecrets_OutOfScopeIgnored(t *testing.T) { "AKIA appearances in data/, docs/, .git/ must NOT count") assert.Equal(t, 1, result.Findings["aws_access_key"].FileCount) assert.Equal(t, "sessions/real/raw.jsonl", result.Findings["aws_access_key"].Sample) - assert.NotContains(t, result.Findings, "github_token", - "github_token in .git/ must not be reachable: out of scope") + // After ox-def1 the broad `github_token` slug was retired. Assert + // absence of EVERY current GitHub slug so this test fails loudly if + // any of them starts firing on a file outside scope. + for _, slug := range []string{ + "github_personal_access_token", + "github_oauth_token", + "github_user_to_server_token", + "github_server_token", + "github_refresh_token", + "github_fine_grained_pat", + } { + assert.NotContains(t, result.Findings, slug, + "%s in .git/ must not be reachable: out of scope", slug) + } } // TestScanLedgerForSecrets_SizeCap verifies oversized files are skipped. diff --git a/cmd/ox/session_redact_history.go b/cmd/ox/session_redact_history.go index bff2f730..3749e281 100644 --- a/cmd/ox/session_redact_history.go +++ b/cmd/ox/session_redact_history.go @@ -273,7 +273,15 @@ func runRedactHistoryWorkflow(opts redactHistoryOptions) error { // the audit doesn't need it; the cleanup tool does. findings, err := enumerateRedactHistoryFindings(opts.ProjectRoot, opts.LedgerPath, scanResult) if err != nil { - return fmt.Errorf("enumerate findings: %w", err) + // Partial enumeration is useful — we still have whatever findings + // the successful files produced. Surface the issue as a Warning + // (loudly, so the operator can't miss it) but keep going. A + // total-loss return is reserved for cases where `findings` is + // nil; here we have actionable data alongside the error. + fmt.Fprintf(opts.Stdout, "\nWarning: enumeration partial — %v\n\n", err) + if findings == nil { + return fmt.Errorf("enumerate findings: %w", err) + } } // Classify each finding by pushed/unpushed. Pushed-commit findings are @@ -460,6 +468,11 @@ func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledge } return nil, fmt.Errorf("read sessions dir: %w", err) } + // Enumeration errors that previously got swallowed by `continue` are + // now collected so the caller surfaces them. A file that was visible + // in the aggregate scan can't silently vanish from the per-line list + // without warning — that pattern hid a real failure mode in ox-zukx. + var enumErrs []string for _, entry := range entries { if !entry.IsDir() { continue @@ -468,6 +481,7 @@ func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledge sessionDir := filepath.Join(sessionsRoot, sessionName) files, err := os.ReadDir(sessionDir) if err != nil { + enumErrs = append(enumErrs, fmt.Sprintf("%s: list dir: %v", sessionName, err)) continue } for _, fEntry := range files { @@ -480,14 +494,21 @@ func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledge } contentPath, err := openSessionContent(projectRoot, ledgerPath, sessionName, filename) if err != nil { + enumErrs = append(enumErrs, fmt.Sprintf("%s/%s: open: %v", sessionName, filename, err)) continue } info, err := os.Stat(contentPath) - if err != nil || info.Size() > ledgerSecretsSizeCap { + if err != nil { + enumErrs = append(enumErrs, fmt.Sprintf("%s/%s: stat: %v", sessionName, filename, err)) + continue + } + if info.Size() > ledgerSecretsSizeCap { + // Over-cap is a deliberate skip, not an error. continue } f, err := os.Open(contentPath) //nolint:gosec // G304: path resolved via openSessionContent (ledger-owned) if err != nil { + enumErrs = append(enumErrs, fmt.Sprintf("%s/%s: read: %v", sessionName, filename, err)) continue } scanner := bufio.NewScanner(f) @@ -502,6 +523,9 @@ func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledge out = append(out, redactHistoryFinding{Detector: name, Path: rel, Line: lineNo}) } } + if scanErr := scanner.Err(); scanErr != nil { + enumErrs = append(enumErrs, fmt.Sprintf("%s: scan: %v", rel, scanErr)) + } f.Close() } } @@ -515,6 +539,16 @@ func enumerateRedactHistoryFindings(projectRoot, ledgerPath string, audit *ledge } return out[i].Detector < out[j].Detector }) + if len(enumErrs) > 0 { + // Don't return an error that aborts the workflow — partial + // enumeration is still useful and the redact-history caller + // already prints a Warning summary. Just attach the detail so + // the caller can surface it. Today the caller logs the count + // via the existing hydration Warning path; future work can + // pass the slice through a richer return type if needed. + return out, fmt.Errorf("enumeration incomplete (%d issue(s)):\n %s", + len(enumErrs), strings.Join(enumErrs, "\n ")) + } return out, nil } diff --git a/cmd/ox/session_redact_hydrate.go b/cmd/ox/session_redact_hydrate.go index 09fed21b..3cb525b0 100644 --- a/cmd/ox/session_redact_hydrate.go +++ b/cmd/ox/session_redact_hydrate.go @@ -66,6 +66,12 @@ func hydrateAllSessionsForScan(projectRoot, ledgerPath string, out io.Writer) (h sessionDir := filepath.Join(sessionsRoot, sessionName) files, err := os.ReadDir(sessionDir) if err != nil { + // Can't even list this session's directory. Don't silently + // drop it from coverage — count it as Failed and tell the + // operator. Otherwise the hydration summary could claim + // full coverage while a whole session was never inspected. + summary.Failed++ + fmt.Fprintf(out, " failed to inspect %s: %v\n", sessionName, err) continue } sessionNeedsAny := false diff --git a/cmd/ox/session_redact_record_test.go b/cmd/ox/session_redact_record_test.go index 5960747e..2854eb6d 100644 --- a/cmd/ox/session_redact_record_test.go +++ b/cmd/ox/session_redact_record_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "reflect" "testing" "time" @@ -106,13 +107,41 @@ func TestWriteRedactionPassesPerSession_AppendsToMeta(t *testing.T) { firstPassID := got1.Redactions[0].PassID assert.NotEmpty(t, firstPassID, "pass id must be populated") - // Critical: never store matched bytes. - for _, p := range got1.Redactions { - for _, e := range p.Entries { - // RedactionEntry has no Original field; trip on any future - // regression that adds one and starts populating it. - _ = e - } + // Critical: never store matched bytes. Use reflection so a future + // `Original`/`Match`/`Bytes` field added to lfs.RedactionEntry can't + // silently start carrying secret material. Pair this with a JSON + // round-trip check on the serialized payload — if anything secret-y + // makes it into the bytes that hit disk, we want to know. + bannedFieldNames := map[string]bool{ + "Original": true, // legacy session.RedactionResult shape + "Match": true, + "Matched": true, + "Bytes": true, + "RawBytes": true, + "Plaintext": true, + "Secret": true, + "Value": true, + } + entryType := reflect.TypeOf(lfs.RedactionEntry{}) + for i := 0; i < entryType.NumField(); i++ { + fname := entryType.Field(i).Name + assert.Falsef(t, bannedFieldNames[fname], + "RedactionEntry must NEVER add a field named %q (would risk leaking matched bytes — ox-zyg7)", fname) + } + // Also verify the field count stays minimal so a stealth `Original` + // under a different name (Capture, Span, etc.) gets caught by the + // reviewer. If a legitimate new field needs to land, bump this + // number AND explain in the assertion message why it's safe. + assert.LessOrEqual(t, entryType.NumField(), 3, + "RedactionEntry should stay {File, Line, Detector}; adding fields requires audit") + + // Belt-and-braces: serialize the persisted meta.json and assert no + // canary bytes leaked into the JSON payload. + persistedBytes, err := os.ReadFile(filepath.Join(sessDir, "meta.json")) + require.NoError(t, err) + for _, canary := range []string{"AKIA", "ghp_", "ghs_", "github_pat_"} { + assert.NotContainsf(t, string(persistedBytes), canary, + "persisted meta.json must not contain detector canary %q", canary) } // Second pass — must APPEND, not overwrite. diff --git a/internal/lfs/meta.go b/internal/lfs/meta.go index 8744f6f5..b1492f82 100644 --- a/internal/lfs/meta.go +++ b/internal/lfs/meta.go @@ -115,10 +115,30 @@ type SessionMeta struct { // catch?" is auditable months later, and so re-runs after the // catalog evolves can identify findings the older ruleset missed. // - // Mutations MUST go through MutateSessionMeta so concurrent CLI / - // daemon writers serialize at the filesystem level — same flock - // path that prevented ox-lrrq from being recreated when this - // schema was added. + // # Concurrent-write safety (LOAD-BEARING) + // + // Mutations to this field MUST go through MutateSessionMeta so + // concurrent CLI / daemon writers serialize at the filesystem + // level — same flock path that prevented ox-lrrq from being + // recreated when this schema was added. + // + // WARNING (tracked in ox-q42i): there are pre-existing + // read-modify-write call sites that bypass MutateSessionMeta and + // call WriteSessionMetaOnly / WriteSessionMeta directly: + // + // - cmd/ox/session_push_summary.go (Files update) + // - cmd/ox/session_regenerate.go (Files update) + // - cmd/ox/session_upload_cmd.go (Files update) + // - cmd/ox/agent_session.go (initial write + Files update) + // - internal/daemon/agentwork/session_finalize.go + // + // Each of those races with concurrent writers and can lose + // updates — including new Redactions entries written by + // ox session redact between the racing reader's read and write. + // The risk is bounded today because redact-history runs are + // operator-initiated and the daemon doesn't run during them in + // practice, but the invariant is fragile. Tracked separately so + // the cleanup lands in a focused PR. // // omitempty so legacy meta.json files keep round-tripping and // older readers ignore the new field. diff --git a/internal/session/redact_rules_test.go b/internal/session/redact_rules_test.go index 21a6200d..25fd7da6 100644 --- a/internal/session/redact_rules_test.go +++ b/internal/session/redact_rules_test.go @@ -405,10 +405,10 @@ regex "emp_[0-9]{6}" -> [REDACTED_EMPLOYEE_ID] }, { name: "mixed builtin and custom secrets", - input: `token=ghp_1234567890abcdefghijklmnopqrstuvwxyz12 + input: `token=ghp_1234567890abcdefghijklmnopqrstuvwxyz curl -H "Authorization: Bearer $TOKEN" https://auth-service.k8s.acmecorp.net/verify`, contains: []string{"[REDACTED_GITHUB_TOKEN]", "[REDACTED_INTERNAL_HOST]"}, - absent: []string{"ghp_1234567890abcdefghijklmnopqrstuvwxyz12", "auth-service.k8s.acmecorp.net"}, + absent: []string{"ghp_1234567890abcdefghijklmnopqrstuvwxyz", "auth-service.k8s.acmecorp.net"}, }, { name: "no false positive on similar but non-matching text", @@ -588,7 +588,7 @@ func TestCustomRules_OverlappingRules(t *testing.T) { redactor, errs := NewRedactorWithCustomRules(tmpDir) require.Empty(t, errs) - input := "token=ghp_1234567890abcdefghijklmnopqrstuvwxyz12" + input := "token=ghp_1234567890abcdefghijklmnopqrstuvwxyz" output, _ := redactor.RedactString(input) // builtin github_token pattern runs first and wins @@ -782,7 +782,7 @@ func TestCustomRules_SessionStopPath(t *testing.T) { Content: "bash output", ToolName: "bash", ToolInput: "curl https://payments.internal.acme.net/health", - ToolOutput: "Connected. Token: ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + ToolOutput: "Connected. Token: ghp_1234567890abcdefghijklmnopqrstuvwxyz", }, { Type: EntryTypeAssistant, @@ -812,7 +812,7 @@ func TestCustomRules_SessionStopPath(t *testing.T) { "content": "connecting to payments.internal.acme.net", "tool_input": "ACME-KEY-deadbeef0123456789abcdef01234567", "tool_output": map[string]any{ - "result": "token ghp_1234567890abcdefghijklmnopqrstuvwxyz12 accepted", + "result": "token ghp_1234567890abcdefghijklmnopqrstuvwxyz accepted", }, "metadata": []any{ "host=payments.internal.acme.net", diff --git a/internal/session/secrets.go b/internal/session/secrets.go index d78f0af8..b4ea9855 100644 --- a/internal/session/secrets.go +++ b/internal/session/secrets.go @@ -103,39 +103,50 @@ func DefaultPatterns() []SecretPattern { // have to be rewritten in lockstep with this split. The win from // per-prefix detectors is in the *detector name* in scan reports, // not in the redaction placeholder. + // Lengths are EXACT per GitHub's published shapes — a token that + // runs longer is not a real PAT and shouldn't pretend to be one. + // The keyword pre-screen anchors on the prefix; the exact-length + // quantifier prevents an alphanumeric run starting with `ghp_` + // from matching arbitrarily far. Greedy-then-truncated lookalikes + // (e.g. a base64 blob that happens to start with `ghp_`) won't + // pretend to be a real PAT. { Name: "github_personal_access_token", - Pattern: regexp.MustCompile(`ghp_[A-Za-z0-9]{36,255}`), + Pattern: regexp.MustCompile(`ghp_[A-Za-z0-9]{36}`), Redact: "[REDACTED_GITHUB_TOKEN]", Keywords: []string{"ghp_"}, }, { Name: "github_oauth_token", - Pattern: regexp.MustCompile(`gho_[A-Za-z0-9]{36,255}`), + Pattern: regexp.MustCompile(`gho_[A-Za-z0-9]{36}`), Redact: "[REDACTED_GITHUB_TOKEN]", Keywords: []string{"gho_"}, }, { Name: "github_user_to_server_token", - Pattern: regexp.MustCompile(`ghu_[A-Za-z0-9]{36,255}`), + Pattern: regexp.MustCompile(`ghu_[A-Za-z0-9]{36}`), Redact: "[REDACTED_GITHUB_TOKEN]", Keywords: []string{"ghu_"}, }, { Name: "github_server_token", - Pattern: regexp.MustCompile(`ghs_[A-Za-z0-9]{36,255}`), + Pattern: regexp.MustCompile(`ghs_[A-Za-z0-9]{36}`), Redact: "[REDACTED_GITHUB_TOKEN]", Keywords: []string{"ghs_"}, }, { Name: "github_refresh_token", - Pattern: regexp.MustCompile(`ghr_[A-Za-z0-9]{76,255}`), + Pattern: regexp.MustCompile(`ghr_[A-Za-z0-9]{76}`), Redact: "[REDACTED_GITHUB_TOKEN]", Keywords: []string{"ghr_"}, }, + // Fine-grained PAT structure: 11-char "github_pat_" + 22-char ID + // + "_" + 59-char secret. No underscores inside the 22 or 59 + // portions — using a tighter character class than the broad + // [A-Za-z0-9_] catches that constraint too. { Name: "github_fine_grained_pat", - Pattern: regexp.MustCompile(`github_pat_[A-Za-z0-9_]{82,255}`), + Pattern: regexp.MustCompile(`github_pat_[A-Za-z0-9]{22}_[A-Za-z0-9]{59}`), Redact: "[REDACTED_GITHUB_PAT]", Keywords: []string{"github_pat_"}, }, @@ -441,17 +452,28 @@ func matchSkipped(match string, skipIf []string) bool { // RedactString scans and redacts secrets from a string. // Returns the redacted output and a list of pattern names that matched. +// +// Applies the SecretPattern.Keywords pre-screen — patterns with a +// non-empty Keywords list are only evaluated when at least one keyword +// appears in the (lowercased) input. Without this, a bare-UUID regex +// gated only by Keywords (e.g. heroku_key) would over-redact every UUID +// in the input — a corruption far worse than the leak it's trying to +// catch. See ox-zukx for the audit-path version of the same bug. func (r *Redactor) RedactString(input string) (output string, found []string) { r.mu.RLock() defer r.mu.RUnlock() output = input + lowerInput := strings.ToLower(input) foundMap := make(map[string]bool) for _, p := range r.patterns { if p.Pattern == nil { continue } + if !p.MatchesKeyword(lowerInput) { + continue + } matched := false output = p.Pattern.ReplaceAllStringFunc(output, func(match string) string { @@ -474,17 +496,23 @@ func (r *Redactor) RedactString(input string) (output string, found []string) { return output, found } -// RedactStringWithDetails provides detailed redaction results +// RedactStringWithDetails provides detailed redaction results. +// Applies the same SecretPattern.Keywords pre-screen as RedactString +// to keep detection semantics consistent across the public API. func (r *Redactor) RedactStringWithDetails(input string) (output string, results []RedactionResult) { r.mu.RLock() defer r.mu.RUnlock() output = input + lowerInput := strings.ToLower(input) for _, p := range r.patterns { if p.Pattern == nil { continue } + if !p.MatchesKeyword(lowerInput) { + continue + } // find all matches with positions; record only those not skipped matches := p.Pattern.FindAllStringIndex(output, -1) @@ -614,15 +642,22 @@ func (r *Redactor) RedactCapturedHistory(history *CapturedHistory) (count int) { return r.RedactHistoryEntries(history.Entries) } -// ContainsSecrets checks if a string contains any secrets without modifying it +// ContainsSecrets checks if a string contains any secrets without modifying it. +// Applies the SecretPattern.Keywords pre-screen so a bare-UUID pattern +// gated only by Keywords doesn't fire on every UUID-shaped input. func (r *Redactor) ContainsSecrets(input string) bool { r.mu.RLock() defer r.mu.RUnlock() + lowerInput := strings.ToLower(input) + for _, p := range r.patterns { if p.Pattern == nil { continue } + if !p.MatchesKeyword(lowerInput) { + continue + } if len(p.SkipIf) == 0 { if p.Pattern.MatchString(input) { return true diff --git a/internal/session/secrets_test.go b/internal/session/secrets_test.go index 727cb9eb..7fef0470 100644 --- a/internal/session/secrets_test.go +++ b/internal/session/secrets_test.go @@ -205,19 +205,19 @@ func TestRedactString_GitHubTokens(t *testing.T) { }{ { name: "github personal access token", - input: "token: ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + input: "token: ghp_1234567890abcdefghijklmnopqrstuvwxyz", expected: "token: [REDACTED_GITHUB_TOKEN]", wantFind: true, }, { name: "github oauth token", - input: "gho_1234567890abcdefghijklmnopqrstuvwxyz12", + input: "gho_1234567890abcdefghijklmnopqrstuvwxyz", expected: "[REDACTED_GITHUB_TOKEN]", wantFind: true, }, { name: "github server token", - input: "ghs_1234567890abcdefghijklmnopqrstuvwxyz12", + input: "ghs_1234567890abcdefghijklmnopqrstuvwxyz", expected: "[REDACTED_GITHUB_TOKEN]", wantFind: true, }, @@ -225,13 +225,13 @@ func TestRedactString_GitHubTokens(t *testing.T) { name: "github refresh token", // Refresh tokens are 76 chars after the ghr_ prefix; older fixture // used 38 chars (the classic-PAT tail length). Updated in ox-def1. - input: "ghr_1234567890abcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcd", + input: "ghr_abcdefghijklmnopqrstuvwxyz0987654321XYZWVUTSRQPONMLKJIHGFEDCBAzyxwvutsrqponm", expected: "[REDACTED_GITHUB_TOKEN]", wantFind: true, }, { name: "github user token", - input: "ghu_1234567890abcdefghijklmnopqrstuvwxyz12", + input: "ghu_1234567890abcdefghijklmnopqrstuvwxyz", expected: "[REDACTED_GITHUB_TOKEN]", wantFind: true, }, @@ -240,7 +240,7 @@ func TestRedactString_GitHubTokens(t *testing.T) { // Real shape: 11-char "github_pat_" + 22-char ID + "_" + 59-char secret = 92 total. // Updated from a shorter fixture in ox-def1 (the strict {82,255} tail // length follows GitHub's published format and the user's spec). - input: "github_pat_11ABCDEFGHIJ012345678901_aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcdefghijklmnopqRStUVWxYz", + input: "github_pat_AABBCCDDEEFFGGHHIIJJ00_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456", expected: "[REDACTED_GITHUB_PAT]", wantFind: true, }, @@ -619,7 +619,7 @@ func TestRedactString_MultipleSecrets(t *testing.T) { input := ` AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY -GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz12 +GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz ` r := NewRedactor() output, found := r.RedactString(input) @@ -701,7 +701,7 @@ func TestRedactEntries(t *testing.T) { Type: EntryTypeTool, Content: "Result output", ToolName: "bash", - ToolInput: "export GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + ToolInput: "export GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz", }, } count := r.RedactEntries(entries) @@ -718,7 +718,7 @@ func TestContainsSecrets(t *testing.T) { expected bool }{ {"AKIAIOSFODNN7EXAMPLE", true}, - {"ghp_1234567890abcdefghijklmnopqrstuvwxyz12", true}, + {"ghp_1234567890abcdefghijklmnopqrstuvwxyz", true}, {"-----BEGIN RSA PRIVATE KEY-----", true}, {"Normal text", false}, {"", false}, @@ -733,7 +733,7 @@ func TestContainsSecrets(t *testing.T) { func TestScanForSecrets(t *testing.T) { r := NewRedactor() - input := "AKIAIOSFODNN7EXAMPLE and ghp_1234567890abcdefghijklmnopqrstuvwxyz12" + input := "AKIAIOSFODNN7EXAMPLE and ghp_1234567890abcdefghijklmnopqrstuvwxyz" found := r.ScanForSecrets(input) assert.True(t, containsPattern(found, "aws_access_key")) @@ -936,7 +936,7 @@ func TestRedactHistorySecrets_TokenInToolOutput(t *testing.T) { Content: "Command completed", ToolName: "bash", ToolInput: "cat config.json", - ToolOutput: `{"token": "ghp_1234567890abcdefghijklmnopqrstuvwxyz12"}`, + ToolOutput: `{"token": "ghp_1234567890abcdefghijklmnopqrstuvwxyz"}`, Timestamp: time.Now(), }, }, @@ -945,7 +945,7 @@ func TestRedactHistorySecrets_TokenInToolOutput(t *testing.T) { count := RedactHistorySecrets(history) assert.Equal(t, 1, count) assert.Contains(t, history.Entries[0].ToolOutput, "[REDACTED_GITHUB_TOKEN]") - assert.NotContains(t, history.Entries[0].ToolOutput, "ghp_1234567890abcdefghijklmnopqrstuvwxyz12") + assert.NotContains(t, history.Entries[0].ToolOutput, "ghp_1234567890abcdefghijklmnopqrstuvwxyz") } func TestRedactHistorySecrets_MultipleSecrets(t *testing.T) { @@ -973,7 +973,7 @@ func TestRedactHistorySecrets_MultipleSecrets(t *testing.T) { Type: "tool", Content: "Output", ToolName: "bash", - ToolInput: "export GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + ToolInput: "export GITHUB_TOKEN=ghp_1234567890abcdefghijklmnopqrstuvwxyz", ToolOutput: `{"secret": "client_secret=abc123def456789012345"}`, Timestamp: time.Now(), }, @@ -992,7 +992,7 @@ func TestRedactHistorySecrets_MultipleSecrets(t *testing.T) { // verify original secrets are gone assert.NotContains(t, history.Entries[0].Content, "AKIAIOSFODNN7EXAMPLE") assert.NotContains(t, history.Entries[1].Content, "password@localhost") - assert.NotContains(t, history.Entries[2].ToolInput, "ghp_1234567890abcdefghijklmnopqrstuvwxyz12") + assert.NotContains(t, history.Entries[2].ToolInput, "ghp_1234567890abcdefghijklmnopqrstuvwxyz") } func TestRedactHistorySecrets_NoSecrets(t *testing.T) { @@ -1113,7 +1113,7 @@ func TestRedactHistorySecrets_AllFieldsWithSecrets(t *testing.T) { Content: "AWS: AKIAIOSFODNN7EXAMPLE", ToolName: "bash", ToolInput: `password="verysecret123"`, - ToolOutput: "ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + ToolOutput: "ghp_1234567890abcdefghijklmnopqrstuvwxyz", Timestamp: time.Now(), }, }, @@ -1167,7 +1167,7 @@ func TestRedactCapturedHistory_DirectCall(t *testing.T) { { Seq: 1, Type: "user", - Content: "Token: ghp_1234567890abcdefghijklmnopqrstuvwxyz12", + Content: "Token: ghp_1234567890abcdefghijklmnopqrstuvwxyz", Timestamp: time.Now(), }, }, @@ -1472,7 +1472,7 @@ func BenchmarkRedactString_NoSecrets(b *testing.B) { func BenchmarkRedactString_WithSecrets(b *testing.B) { r := NewRedactor() - input := "AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE and token=ghp_1234567890abcdefghijklmnopqrstuvwxyz12" + input := "AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE and token=ghp_1234567890abcdefghijklmnopqrstuvwxyz" b.ResetTimer() for i := 0; i < b.N; i++ {