fix(desktop): redact secrets and structure session export in problem report#1470
Merged
Conversation
…report
The full problem report a user uploads to the feedback form carried logTail
and sessionExport.messages with zero redaction, while rendererDiagnostics
already used a strict allowlist. Secrets the user cannot see (API keys,
Bearer tokens, absolute paths, usernames, emails) leaked into the uploaded
file.
Add problem-report-redact.ts with two regimes, by data shape:
- Free text (logTail, rendererError, message/tool text): a blacklist scrubber
for PEM keys, provider token prefixes (sk-/AKIA/AIza/glpat-/hf_/npm_/xox*/
*k_live_), Bearer/basic-auth, named credential assignments, cookies,
absolute paths (Windows/UNC/POSIX/file://-/~) and the usernames in them, and
emails. Plus exact runtime terms (OS username, home dir) that no regex can
infer, passed from feedback.ts.
- Session messages (structured /session/{id}/message objects): a field
allowlist mirroring the renderer-diagnostics paradigm — keep role/time/
part-type/tool-name/byte-size + per-part length-capped, redacted body; omit
the system prompt, structured output, and unknown shapes (reported, never
passed through).
Redaction runs before the truncation ladder so a cut never splits a secret,
and the ladder operates on already-redacted data. Output stays the existing
JSON-in-markdown format and parseable payload (PR1 of the rebuild; format,
budgets, and flow follow in later PRs).
Verification: bun test src/main (373 pass) incl. a new redaction gate test
asserting the full report contains none of the seeded token/path/username/
email samples; tsgo -b clean; eslint 0 errors. Tracking: #1465.
Claude-Session: https://claude.ai/code/session_01TJzRvF1KAeM78fAc1gVufV
github-actions
Bot
added
the
platform
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughA new ChangesProblem Report Redaction Pipeline
Sequence Diagram(s)sequenceDiagram
participant feedback as feedback.ts
participant makeRedactor
participant buildReport as buildProblemReport
participant redactHelpers as redact helpers
participant buildSummary as buildProblemReportSummary
participant truncate
feedback->>feedback: localRedactTerms()
feedback->>makeRedactor: redactTerms
makeRedactor-->>buildReport: Redactor fn
buildReport->>redactHelpers: redact diagnostics<br/>sessionExport<br/>rendererDiagnostics
buildReport->>truncate: truncate redacted data
truncate-->>feedback: markdown report
feedback->>buildSummary: redactTerms
buildSummary->>redactHelpers: redactLocalPathFragments
buildSummary-->>feedback: clipboard summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/feedback.ts, packages/desktop-electron/src/main/problem-report-redact.test.ts, packages/desktop-electron/src/main/problem-report-redact.ts, packages/desktop-electron/src/main/problem-report.test.ts, packages/desktop-electron/src/main/problem-report.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/desktop-electron/src/main/feedback.ts (1)
246-267: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReuse local redaction terms for the clipboard summary.
localRedactTerms()is only passed to the full report. The summary below is copied outbound from raw diagnostics/logTail/rendererError, and the summary redactor cannot infer bare OS usernames or non-standard home directories. Compute the terms once and thread them intobuildProblemReportSummary/ its redactor too.🛡️ Suggested direction
+ const redactTerms = localRedactTerms() + if (!fullReportFailure) { try { const report = buildProblemReport( { diagnostics, logTail, sessionExport, rendererDiagnostics, rendererError: input.rendererError }, - { reportId: id, generatedAt, maxBytes: DEFAULT_PROBLEM_REPORT_MAX_BYTES, redactTerms: localRedactTerms() }, + { reportId: id, generatedAt, maxBytes: DEFAULT_PROBLEM_REPORT_MAX_BYTES, redactTerms }, ) savedReport = await deps.saveReport({ reportId: id, generatedAt, markdown: report.markdown }) } catch (error) { fullReportFailure = safeFailureReason(error) } @@ - const summary = buildProblemReportSummary({ + const summary = buildProblemReportSummary({ reportId: id, generatedAt, diagnostics, @@ rendererDiagnostics, rendererError: input.rendererError, - }) + }, { redactTerms })🤖 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 `@packages/desktop-electron/src/main/feedback.ts` around lines 246 - 267, The redaction terms from localRedactTerms() are only being applied to the full report built by buildProblemReport, but the summary built by buildProblemReportSummary is missing these same redaction terms, leaving sensitive information unredacted in the summary. Compute localRedactTerms() once before calling buildProblemReport and store the result in a variable, then pass this same variable to buildProblemReportSummary to ensure both the full report and summary apply consistent redaction of OS usernames and home directories.
🤖 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 `@packages/desktop-electron/src/main/problem-report-redact.ts`:
- Around line 356-359: The redaction of executionContext uses a blacklist
approach that only scrubs known strings, which can leak sensitive paths if new
fields are added or existing paths are missed. Replace the call to
redactJsonValue(toJsonSafe(info.executionContext), ctx.redact) with an allowlist
approach that explicitly selects only the known safe fields from
info.executionContext and maps any path-valued fields to the string [path]
instead of preserving actual path values. This whitelist strategy ensures that
unexpected fields or nested paths cannot expose private project details.
- Around line 249-253: The source.path field is being free-text redacted using
ctx.redact() when it should use a consistent shape token for structured path
redaction. In the conditional block checking source.path, replace the
ctx.redact(String(source.path)) call with the [path] shape token that is used
consistently for other working-directory path fields throughout the codebase to
ensure uniform path redaction.
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 151-158: In the redactDiagnostics function, the path fields
directory and logPath should be mapped to a standardized token rather than
passed through the blacklist redactor. For both directory (when not null) and
logPath, replace the redact() calls with the string literal '[path]' to
shape-token these known path fields, preventing exposure of unrecognized
absolute-root or path-suffix details that the redactor might miss.
---
Outside diff comments:
In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 246-267: The redaction terms from localRedactTerms() are only
being applied to the full report built by buildProblemReport, but the summary
built by buildProblemReportSummary is missing these same redaction terms,
leaving sensitive information unredacted in the summary. Compute
localRedactTerms() once before calling buildProblemReport and store the result
in a variable, then pass this same variable to buildProblemReportSummary to
ensure both the full report and summary apply consistent redaction of OS
usernames and home directories.
🪄 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: 572598cb-a1e3-405a-8399-b3bd04a35d0a
📒 Files selected for processing (5)
packages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report-redact.test.tspackages/desktop-electron/src/main/problem-report-redact.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
13 tasks
…mary terms Apply the allowlist/shape-token discipline to the remaining structured path fields instead of free-text scrubbing, and share the exact runtime redact terms with the clipboard summary. Closes the false-negative gaps surfaced in review. - diagnostics.directory/logPath -> [path] wholesale (the path regex only catches allowlisted roots, so a path under a non-listed root leaked the project name) - file part source.path -> [path] wholesale - executionContext -> field allowlist: every directory/worktree path becomes [path], worktree name/branch kept (capped+scrubbed), unknown fields dropped (previously the whole object was spread through, leaking new/path fields) - buildProblemReportSummary now takes redactTerms and builds its redactor per call (module-level summaryRedactor removed), so the OS username / home dir are scrubbed from the summary too, not just the full report Regression tests cover non-allowlisted roots, nested executionContext, dropped unknown fields, and the summary term sharing. Claude-Session: https://claude.ai/code/session_011KyY9wTxQu9oZLy4yPEi3W
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/desktop-electron/src/main/problem-report.test.ts (1)
311-332: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGood coverage of
redactTermsin the summary channel. Consider also seedingreportLocationHint/reportFileNamewith an identity-bearing path here so the summary's "Report location/file" lines are guarded against regression (see related finding inproblem-report.ts).🤖 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 `@packages/desktop-electron/src/main/problem-report.test.ts` around lines 311 - 332, In the test named "summary scrubs the bare OS username and non-allowlisted home via shared redactTerms", update the buildProblemReportSummary call to seed reportLocationHint and reportFileName with identity-bearing paths that contain the redactTerms (such as "/customroot/zoe/report.md" or similar values containing "zoe" or "/customroot/zoe"). Then add additional expect assertions to verify that these fields in the summary output are also properly redacted and do not contain "zoe" or "/customroot/zoe", ensuring the "Report location/file" lines in the summary are guarded against leaking identity information.
🤖 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 `@packages/desktop-electron/src/main/problem-report.test.ts`:
- Around line 311-332: In the test named "summary scrubs the bare OS username
and non-allowlisted home via shared redactTerms", update the
buildProblemReportSummary call to seed reportLocationHint and reportFileName
with identity-bearing paths that contain the redactTerms (such as
"/customroot/zoe/report.md" or similar values containing "zoe" or
"/customroot/zoe"). Then add additional expect assertions to verify that these
fields in the summary output are also properly redacted and do not contain "zoe"
or "/customroot/zoe", ensuring the "Report location/file" lines in the summary
are guarded against leaking identity information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07d61c25-6b5e-4db0-8b48-1103813864b6
📒 Files selected for processing (5)
packages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report-redact.test.tspackages/desktop-electron/src/main/problem-report-redact.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-electron/src/main/feedback.ts
- packages/desktop-electron/src/main/problem-report-redact.ts
… summary
The exact-term redactor wrapped 1-2 char terms in JS \b word boundaries, but \b
is ASCII-only: a short CJK/JP username ("张"/"山田") had no boundary next to it
and leaked into logs, renderer error, session text, and the clipboard summary.
Gate \b on a short *ASCII-word* term; redact other terms as exact matches. The
ASCII whole-word sparing ("x" in "0x1f", "yu" in "yuan") is preserved.
Also run the summary's Report file / Report location lines through the same
path-fragment + term scrubber as the rest of the summary, instead of inserting
saveReport's filename/hint raw — defense in depth against an identity-bearing
path slipping into the clipboard.
Claude-Session: https://claude.ai/code/session_011KyY9wTxQu9oZLy4yPEi3W
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add structure-aware privacy redaction to the "Prepare Diagnostics Package" export — PR1 of the diagnostics-package rebuild (#1465). Two redaction regimes, chosen by data shape, applied to every report component (diagnostics,
logTail,sessionExportinfo + messages,rendererDiagnostics,rendererError, and the clipboard summary):sanitizeSessionMessages,sanitizeSessionInfo): keep role / time / part-type / tool-name / byte-size + a per-part length-capped body; omit the system prompt; drop unknown shapes (reported as{ unrecognized, bytes }, never passed through); drop diffs / permission / share. A finalredactJsonValuepass scrubs every surviving string and object key, and redacts a value wholesale when its field name is sensitive (apiKey,clientSecret,Authorization, … —tokens/tokenizerusage counts are deliberately preserved).makeRedactor): PEM/PGP private keys, URL basic-auth (all userinfo forms), JWT, provider token prefixes,Authorization: Basic/Bearer, named credential assignments, cookies, absolute paths, and emails (incl. local domains). The OS username and home dir are always redacted as exact terms — the hard identity guarantee.Known path / structured fields are shape-tokened, not regex-guessed (the path regex only catches allowlisted roots, so a non-listed root would leak the project/dir name):
diagnostics.directory/diagnostics.logPath→[path]wholesale.source.path→[path]wholesale.executionContext→ field allowlist: every directory/worktree path →[path], worktreename/branchkept (capped + scrubbed), unknown fields dropped (previously the whole subtree was spread through).Why
A Windows user exported a problem report that leaked private data and was unreviewable.
rendererDiagnosticsalready used a strict allowlist, butlogTailandsessionExport.messageshad zero redaction — API keys,Bearer …,C:\Users\<name>\…, usernames, and emails went straight into the file the user hands us. Several known path/structured fields were also redacted only by regex (allowlisted roots), so a path under a non-listed root or a newly addedexecutionContextfield leaked the project/dir name; and the clipboard summary did not apply the username/home exact terms.This is privacy redaction first, standalone and format-agnostic. Size budgets (PR2 #1471), flow + in-app review panel (PR3 #1472), and structural cleanup (PR4) follow.
Related Issue
Refs #1465 (umbrella). This is PR1; PR2 #1471 and PR3 #1472 follow.
Human Review Status
PendingReview Focus
diagnostics.directory/logPath, filesource.path,executionContext) under a non-allowlisted root.executionContextfield allowlist: paths →[path], worktreename/branchkept, unknown fields dropped — confirm nothing useful is lost and nothing identifying is kept./codexrounds (final fresh-eye, no P0/P1). This revision additionally addresses the Web GPT Pro "needs fixes" verdict and CodeRabbit's 4 Major path-field findings (all 3 review threads resolved).Risk Notes
/a/bmatcher would shred the diagnostics route and URLs); a non-allowlisted-root, identity-free path (e.g. a CI path) may remain in free text — it carries no secret or identity. Known structured path fields are now shape-tokened regardless of root.C:\, UNC\\server\…, and POSIX path shapes; considered macOS + Windows.How To Verify
Screenshots or Recordings
None — no visible UI in this PR (the in-app review panel is PR3 #1472).
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.https://claude.ai/code/session_011KyY9wTxQu9oZLy4yPEi3W
Summary by CodeRabbit
Release Notes
New Features
Tests