From 54f67852371d924cdcd0614319c59a41e28ea348 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Fri, 3 Jul 2026 17:57:36 -0700 Subject: [PATCH] Address CodeRabbit review: redact-before-truncate, unmount feedback, label truncation flag - Redact oversized extra records before truncation so a sliced secret fragment can never evade the pattern redactor on the upload path - Report perf-dump success (toast + reveal) even if Settings unmounted during the ~10s capture; the artifact is already on disk - Only flag worktree labels as truncated when the 80-char cap actually dropped data, not on whitespace trimming - Document the fixed 64 KiB extra-record budget; fix a 9-char locale key Co-authored-by: Orca --- src/main/observability/bundle.test.ts | 31 +++++++++++++++++++ src/main/observability/bundle.ts | 6 +++- .../settings/PrivacyDiagnosticsSection.tsx | 4 ++- .../src/components/settings/privacy-search.ts | 2 +- src/shared/renderer-perf-metrics.ts | 5 ++- 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/main/observability/bundle.test.ts b/src/main/observability/bundle.test.ts index e814acdb6ea..8fc6c5631a8 100644 --- a/src/main/observability/bundle.test.ts +++ b/src/main/observability/bundle.test.ts @@ -255,6 +255,37 @@ describe('bundle — collection', () => { expect(bundle.payload).toContain('"name":"kept"') }) + it('redacts secrets in oversized extra records before truncation can bisect them', () => { + writeFileSync(traceFile, makeNDJSON([makeSpan({ name: 'kept' })])) + // A secret placed past the 1024-char string-truncation boundary: if + // truncation ran before redaction, the sliced fragment would no longer + // pattern-match and partial key material could ride the upload. + const secret = `sk-ant-api03-${'a'.repeat(80)}` + const bundle = collectBundle({ + traceFilePath: traceFile, + maxFiles: 10, + appVersion: '1', + platform: 'darwin', + arch: 'arm64', + osRelease: '24', + orcaChannel: 'dev', + extraRecords: [ + { + type: 'renderer-perf', + note: `${'x'.repeat(1000)}${secret}`, + payload: 'y'.repeat(_internalsForTests.MAX_EXTRA_RECORD_BYTES * 2) + } + ] + }) + expect(bundle.payload).not.toContain('sk-ant-api03-') + const extraLine = bundle.payload + .split('\n') + .filter(Boolean) + .map((line) => JSON.parse(line))[1] + expect(extraLine.type).toBe('renderer-perf') + expect(extraLine.truncated).toBe(true) + }) + it('does not append a span that would push the payload over the upload cap', () => { const giantSpan = makeSpan({ attributes: { diff --git a/src/main/observability/bundle.ts b/src/main/observability/bundle.ts index f3246c428c5..28a3ecfc964 100644 --- a/src/main/observability/bundle.ts +++ b/src/main/observability/bundle.ts @@ -71,6 +71,8 @@ type BundleHeader = { readonly schema_version: 1 } +// Why: extra records are metrics riders, not span payload — a fixed 64 KiB +// ceiling keeps them from crowding spans out of the 4 MiB bundle. const MAX_EXTRA_RECORD_BYTES = 64 * 1024 function* readLinesNewestFirst(text: string): Iterable { @@ -250,7 +252,9 @@ function serializeExtraRecord( return serialized } - const truncated = JSON.stringify(redactValue(markExtraRecordTruncated(record), 'server')) + // Why: truncate the already-redacted value — truncating first could bisect + // a long secret so the redactor no longer pattern-matches the fragment. + const truncated = JSON.stringify(markExtraRecordTruncated(redacted)) if (Buffer.byteLength(truncated) + 1 <= budget) { return truncated } diff --git a/src/renderer/src/components/settings/PrivacyDiagnosticsSection.tsx b/src/renderer/src/components/settings/PrivacyDiagnosticsSection.tsx index 15f9069e45b..5131c2676ea 100644 --- a/src/renderer/src/components/settings/PrivacyDiagnosticsSection.tsx +++ b/src/renderer/src/components/settings/PrivacyDiagnosticsSection.tsx @@ -252,9 +252,11 @@ export function PrivacyDiagnosticsSection(): React.JSX.Element { setPerfDumpStage(null) try { const result = await window.api.diagnostics.capturePerfDump() - if (!mountedRef.current || 'canceled' in result) { + if ('canceled' in result) { return } + // Why: capture runs ~10+ s and the artifact is already on disk — + // report it (global toast + reveal) even if Settings unmounted. toast.success( translate( 'auto.components.settings.PrivacyDiagnosticsSection.c4cd74809e', diff --git a/src/renderer/src/components/settings/privacy-search.ts b/src/renderer/src/components/settings/privacy-search.ts index 1a2585bb1b5..8741fd06dbe 100644 --- a/src/renderer/src/components/settings/privacy-search.ts +++ b/src/renderer/src/components/settings/privacy-search.ts @@ -63,7 +63,7 @@ export const getPrivacyPaneSearchEntries = createLocalizedCatalog(() => [ ...translateSearchKeyword('auto.components.settings.privacy.search.048b49260d', 'heap'), ...translateSearchKeyword('auto.components.settings.privacy.search.c1d94c8566', 'trace'), ...translateSearchKeyword('auto.components.settings.privacy.search.ec083fb9e1', 'slow'), - ...translateSearchKeyword('auto.components.settings.privacy.search.07241d4d1', 'lag') + ...translateSearchKeyword('auto.components.settings.privacy.search.900d529b12', 'lag') ] }, { diff --git a/src/shared/renderer-perf-metrics.ts b/src/shared/renderer-perf-metrics.ts index ab7718647df..93fc5484203 100644 --- a/src/shared/renderer-perf-metrics.ts +++ b/src/shared/renderer-perf-metrics.ts @@ -191,7 +191,10 @@ function narrowWorktree(value: unknown): [RendererPerfWorktreeMetrics, boolean] scrollbackRowsTotal: clampRendererPerfCount(value.scrollbackRowsTotal), panes }, - rawPanes.length > panes.length || label.length < String(value.worktreeLabel ?? '').length + // Compare against the trimmed input so whitespace padding does not + // count as truncation — `truncated` must mean data was dropped. + rawPanes.length > panes.length || + (typeof value.worktreeLabel === 'string' && label.length < value.worktreeLabel.trim().length) ] }