feat(capture): mask secrets (tokens, passwords, API keys) with stars before persist/embed#307
feat(capture): mask secrets (tokens, passwords, API keys) with stars before persist/embed#307khustup2 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCaptured session events, OpenClaw messages, and uploaded wiki summaries now pass through shared secret redaction before storage, embedding, size calculation, and derived metadata processing. Tests cover provider tokens, credentials, structured values, precision safeguards, entropy detection, idempotency, and capture integration. ChangesSession secret redaction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CaptureOrWikiWorker
participant redactSecrets
participant Storage
participant Embedding
CaptureOrWikiWorker->>redactSecrets: serialize or transform captured content
redactSecrets-->>CaptureOrWikiWorker: return masked content
CaptureOrWikiWorker->>Storage: persist redacted payload or summary
CaptureOrWikiWorker->>Embedding: embed redacted content
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 11 files changed
Generated for commit d54e602. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/shared/redact.ts (2)
135-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
as stringcast — it's incorrect whenrule.replaceis a function.The
Rule.replacetype isstring | ((match: string, ...groups: string[]) => string), but theas stringcast forces TypeScript to use the string overload ofString.replace. This works at runtime because JavaScript dispatches dynamically, but it suppresses type-checking for the function replacements in rules 7 and 8.♻️ Suggested fix: type-narrow with typeof
for (const rule of RULES) { - out = out.replace(rule.re, rule.replace as string); + if (typeof rule.replace === "string") { + out = out.replace(rule.re, rule.replace); + } else { + out = out.replace(rule.re, rule.replace); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/shared/redact.ts` around lines 135 - 137, Remove the `as string` cast in the `RULES` loop and type-narrow `rule.replace` with `typeof`: pass it directly when it is a string, otherwise invoke or pass it as the replacement callback so both `Rule.replace` variants remain type-checked, including rules 7 and 8.
77-86: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueScheme prefix hint is lost when known tokens appear in
key=valueassignments.When a known token like
ghp_…appears in an assignment with a secret-ish key (e.g.,GITHUB_TOKEN=ghp_…), rule 2 correctly keeps the prefix (ghp_********), but rule 7 subsequently matchesTOKEN=ghp_********and replaces the entire value with********, discarding theghp_hint. The secret remains fully masked and the key name still identifies the secret type, so this is not a security concern — just a minor precision trade-off worth being aware of.If preserving the scheme prefix in this case is desired, rule 7's replace function could skip values that are already partially masked (e.g., contain
********).Also applies to: 110-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/shared/redact.ts` around lines 77 - 86, The redact rules cause the generic key=value masking rule to remove scheme prefixes already preserved by token-specific rules. Update the rule 7 replacement logic in the shared redaction rules to detect values containing the MASK marker and leave those assignments unchanged, while continuing to fully mask unrecognized secret values; apply the same change to the corresponding duplicate rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/hooks/shared/redact.ts`:
- Around line 135-137: Remove the `as string` cast in the `RULES` loop and
type-narrow `rule.replace` with `typeof`: pass it directly when it is a string,
otherwise invoke or pass it as the replacement callback so both `Rule.replace`
variants remain type-checked, including rules 7 and 8.
- Around line 77-86: The redact rules cause the generic key=value masking rule
to remove scheme prefixes already preserved by token-specific rules. Update the
rule 7 replacement logic in the shared redaction rules to detect values
containing the MASK marker and leave those assignments unchanged, while
continuing to fully mask unrecognized secret values; apply the same change to
the corresponding duplicate rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87d2787b-58f9-433f-825d-b1c9dec2e3b4
📒 Files selected for processing (9)
src/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/cursor/capture.tssrc/hooks/hermes/capture.tssrc/hooks/shared/redact.tssrc/hooks/upload-summary.tssrc/mcp/cowork-ingest.tstests/claude-code/capture-hook.test.tstests/shared/redact.test.ts
khustup2
force-pushed
the
feat/redact-secrets-on-capture
branch
from
df7a218 to
05537dd
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/shared/redact.ts`:
- Around line 166-179: Update the secret redaction regexes and replacement logic
in the shared redaction rules so quoted assignment and CLI values are consumed
through their closing quote, masking the entire secret while preserving
surrounding syntax. Extend the CLI pattern to recognize the documented short -p
form, including -p=VALUE, and keep unquoted value handling unchanged.
- Around line 56-66: Rework the looksLikeSecret heuristic so long benign
identifiers such as CustomerAuthenticationServiceV2 are not redacted, while
secrets containing characters like / or + remain protected when tokenized into
shorter fragments. Apply the improved detection consistently across the
redaction flow before output is persisted or embedded, and add regression cases
covering both false-positive and split-secret scenarios.
- Around line 155-161: Update the authorization redaction rules in the shared
redaction pattern set to mask the complete supported Digest credential,
including parameters such as username and response, rather than only its initial
fragment. Make the bare Bearer rule case-insensitive so lowercase and mixed-case
schemes are redacted, while preserving the existing token character and length
constraints.
In `@tests/shared/redact.test.ts`:
- Around line 26-31: Update the test case around redactSecrets to assert the
complete expected transformed string, including the surrounding text and fully
masked secret, rather than using toContain checks. Apply the same exact-output
assertion to the additional covered case, while retaining verification that the
original secret is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 820aea27-62bb-4fd4-ae45-f8b1e628ae87
📒 Files selected for processing (9)
src/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/cursor/capture.tssrc/hooks/hermes/capture.tssrc/hooks/shared/redact.tssrc/hooks/upload-summary.tssrc/mcp/cowork-ingest.tstests/claude-code/capture-hook.test.tstests/shared/redact.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/hooks/hermes/capture.ts
- src/hooks/codex/capture.ts
- src/hooks/upload-summary.ts
- tests/claude-code/capture-hook.test.ts
- src/hooks/capture.ts
- src/mcp/cowork-ingest.ts
- src/hooks/cursor/capture.ts
| // Authorization / Proxy-Authorization headers. | ||
| { | ||
| re: /((?:proxy-)?authorization["']?\s*[:=]\s*["']?(?:bearer|basic|token|digest)\s+)([A-Za-z0-9._~+/=-]{8,})/gi, | ||
| replace: `$1${MASK}`, | ||
| }, | ||
| // Bare `Bearer <token>`. | ||
| { re: /\b(Bearer\s+)([A-Za-z0-9._~+/=-]{12,})/g, replace: `$1${MASK}` }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fully redact supported Authorization variants.
A Digest header only has its initial username= fragment replaced, leaving parameters such as a hexadecimal response exposed. Bare bearer is also case-sensitive, so lowercase schemes with hex tokens bypass both this rule and the entropy backstop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/shared/redact.ts` around lines 155 - 161, Update the authorization
redaction rules in the shared redaction pattern set to mask the complete
supported Digest credential, including parameters such as username and response,
rather than only its initial fragment. Make the bare Bearer rule
case-insensitive so lowercase and mixed-case schemes are redacted, while
preserving the existing token character and length constraints.
96d9e92 to
a7167c6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/shared/redact.test.ts (1)
136-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis case doesn't exercise the entropy threshold it claims to.
"Ab1".repeat(10)contains onlyA,b,1— all hex digits — solooksLikeSecretreturns early at the hex guard (Line 59:/^[0-9a-f]+$/i) and never reaches theshannonEntropy(...) >= 3.5check. The test passes for the wrong reason. Use non-hex characters so the token clears the hex/UUID guards and genuinely tests the low-entropy path.💚 Proposed fixture change
- const repeated = "Ab1".repeat(10); // 30 chars, entropy ~1.58 bits/char + const repeated = "Xy9".repeat(10); // 30 chars, mixed non-hex classes, entropy ~1.58 bits/char🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/redact.test.ts` around lines 136 - 141, Update the repeated-token fixture in the “does NOT mask a long but low-entropy repeated token” test to include non-hex characters while retaining mixed character classes and low repetition entropy, so it bypasses the hex/UUID guards and exercises the shannonEntropy threshold in looksLikeSecret.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/shared/redact.test.ts`:
- Around line 136-141: Update the repeated-token fixture in the “does NOT mask a
long but low-entropy repeated token” test to include non-hex characters while
retaining mixed character classes and low repetition entropy, so it bypasses the
hex/UUID guards and exercises the shannonEntropy threshold in looksLikeSecret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b8fa630-a4fb-4b82-a85d-8ccb54a35c9d
📒 Files selected for processing (8)
src/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/cursor/capture.tssrc/hooks/hermes/capture.tssrc/hooks/shared/redact.tssrc/hooks/upload-summary.tstests/claude-code/capture-hook.test.tstests/shared/redact.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/hooks/cursor/capture.ts
- tests/claude-code/capture-hook.test.ts
- src/hooks/hermes/capture.ts
- src/hooks/codex/capture.ts
- src/hooks/capture.ts
- src/hooks/upload-summary.ts
khustup2
force-pushed
the
feat/redact-secrets-on-capture
branch
from
a7167c6 to
ee61a8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/wiki-worker.ts (1)
288-288: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider stamping the offset after redaction, not before.
The
**JSONL offset**: ${jsonlLines}marker is stamped first, then passed throughredactSecrets(...). On resume this exact line is parsed back with/\*\*JSONL offset\*\*:\s*(\d+)/(Line 195). If any labeled-assignment or entropy rule ever matches the marker, the persisted offset is corrupted and resume slicing breaks. Redactingrawfirst and stamping afterward keeps the authoritative bookkeeping line outside the redactor's reach (the offset line carries no secrets). This pattern is repeated in the codex/cursor/hermes/pi wiki-workers.♻️ Suggested reorder
- const text = redactSecrets(stampOffset(raw, jsonlLines)); + const text = stampOffset(redactSecrets(raw), jsonlLines);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/wiki-worker.ts` at line 288, In the wiki-worker processing flow, update the expression around stampOffset and redactSecrets so raw content is redacted before the JSONL offset is stamped. Preserve the existing offset value and ensure the authoritative “JSONL offset” marker remains outside redactSecrets, matching the ordering used by the related worker implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/shared/redact.ts`:
- Around line 165-166: Update the URL credential pattern in the redaction rules
to allow colons within the password while still identifying the username
separator and trailing @ delimiter. Preserve masking behavior for the password
and ensure URLs such as postgres://user:pa:ss@host/db are redacted.
---
Nitpick comments:
In `@src/hooks/wiki-worker.ts`:
- Line 288: In the wiki-worker processing flow, update the expression around
stampOffset and redactSecrets so raw content is redacted before the JSONL offset
is stamped. Preserve the existing offset value and ensure the authoritative
“JSONL offset” marker remains outside redactSecrets, matching the ordering used
by the related worker implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04aefb8e-f3ba-43e6-9224-f66e108891ce
📒 Files selected for processing (14)
harnesses/openclaw/src/index.tssrc/hooks/capture.tssrc/hooks/codex/capture.tssrc/hooks/codex/wiki-worker.tssrc/hooks/cursor/capture.tssrc/hooks/cursor/wiki-worker.tssrc/hooks/hermes/capture.tssrc/hooks/hermes/wiki-worker.tssrc/hooks/pi/wiki-worker.tssrc/hooks/shared/redact.tssrc/hooks/upload-summary.tssrc/hooks/wiki-worker.tstests/claude-code/capture-hook.test.tstests/shared/redact.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/hooks/capture.ts
- src/hooks/cursor/capture.ts
- src/hooks/codex/capture.ts
- tests/claude-code/capture-hook.test.ts
- src/hooks/hermes/capture.ts
- src/hooks/upload-summary.ts
khustup2
force-pushed
the
feat/redact-secrets-on-capture
branch
from
ee61a8d to
8719711
Compare
…before persist/embed Captured prompts, tool inputs, tool outputs and assistant messages routinely contain credentials — a Bash `export GITHUB_TOKEN=…`, an `Authorization: Bearer …` echoed by curl, a `postgres://user:pass@host` connection string. We were embedding and storing those verbatim. Add a shared, pure `redactSecrets()` helper (src/hooks/shared/redact.ts) and apply it at every capture chokepoint, before the payload is embedded or written to the store: - raw events: redact the serialized `line` in all four capturers (claude, cursor, codex, hermes) + the cowork transcript ingest. Redacting the serialized JSON covers content / tool_input / tool_response and both the embedding and the DB write in one shot, and is star-safe (keeps the JSON parseable). - summaries: redact the text at the top of the shared uploadSummary(). Redactor design: - Fixed-length star mask — never reveals the secret's length. - Keeps a non-secret hint where useful: the scheme prefix of a known token (`ghp_********`) or the key name of an assignment (`password=********`). - Covers: PEM private keys; GitHub/OpenAI/Anthropic/Stripe/Slack/AWS/Google/ HuggingFace/GitLab tokens; JWTs; Authorization Bearer/Basic; bare Bearer; URL basic-auth creds; generic KEY=VALUE / "key":"value" assignments; and --password/--token CLI flags. - Precision-first: won't match look-alikes (`tokenizer`, `max_tokens`) and leaves literal true/false/null values readable. Idempotent. Tests: 31-case unit suite for the redactor + an end-to-end capture-hook test proving a token/password in a Bash tool call is masked in the emitted INSERT (and therefore in the embedding, which is derived from the same line).
khustup2
force-pushed
the
feat/redact-secrets-on-capture
branch
from
8719711 to
1631f2d
Compare
2 participants
What
Hivemind captures prompts, tool inputs, tool outputs and assistant messages, then embeds and stores them. Those payloads routinely contain credentials. This masks secrets with stars before anything is embedded or written to the store.
One shared helper (
src/hooks/shared/redact.ts), applied at every capture chokepoint (6 sites): the serializedlinein all four capturers (capture.ts,cursor/,codex/,hermes/) and the text in the shareduploadSummary(). Redacting the serialized JSON coverscontent/tool_input/tool_responseand both the embedding and the DB write at once, star-safe (stays valid JSON).Detection coverage (layered, most-specific first)
ghp_********): GitHub, OpenAI/Anthropic, Stripe (secret/restricted/webhook), Slack (bot/user/app + incoming webhooks), AWS access-key ids, Google API key +ya29.OAuth, HuggingFace, GitLab, npm, PyPI, Shopify, DigitalOcean, Doppler, Databricks, Linear, Postman, SendGrid, Atlassian, Notion, Groq, xAI, Replicate, Mailgun, Telegram bot, Sentry DSN, JWTs.Authorization/Proxy-Authorization(Bearer/Basic/Digest), bareBearer,scheme://user:password@hostURL creds.KEY=VALUE/"key":"value"where KEY is a secret-ish word.Precision (don't shred traces)
tokenizer,max_tokens); leaves literaltrue/false/nullvalues readable.=/+//excluded from the candidate charset sokey=valueand base64/paths break into short segments rather than gluing into one blob).Tests
tests/shared/redact.test.ts— 49 cases: every provider scheme, structured secrets, labeled assignments, the entropy backstop (recall + full precision-exclusion set), idempotency, multi-secret blobs. 100% line coverage of the helper.tests/claude-code/capture-hook.test.ts— end-to-end: a token + password in a real Bash tool call are masked in the emittedINSERT(and therefore in the embedding).graph/*tree-sitter +flush-memorynetwork failures, identical onmain).Note
Test fixtures build vendor tokens from split literals at runtime so GitHub push protection doesn't flag this PR.
Follow-up (tracked separately)
src/mcp/cowork-ingest.ts) redaction is deferred to a focused follow-up PR that ships it with a proper test suite (the file sits at ~35% coverage today). Tracked in Redact secrets on the Cowork transcript ingest path (+ tests) #308.uploadSummaryredacts the stored summary text; the summary embedding is computed by each wiki worker before the call, so full parity could redact at the worker's embed site too. Raw-capture embeddings are already fully covered.Summary by CodeRabbit