Repository navigation
fix: redact URL query secrets in error text shown by the web and TUI clients - #2588
Conversation
…clients Move the CLI's redactUrlsInText into core/mcp/fetchTracking.ts beside redactUrlQuery and import it from there in the CLI. Route on-screen error text through it at one display boundary per client: the web client's utils/errorFormat errorMessage/formatErrorDetails (now used by every toast and inline error that rendered err.message), and a new TUI utils/errorText helper used by App, the tabs and the test modals. Closes #2490 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several display paths still bypass redaction, and deep-link classification now uses altered text.
Review effort: Balanced
Findings: 5
Open (6)
What changed in this PR
Centralizes URL-query secret redaction and applies it to displayed web/TUI errors while preserving CLI behavior.
Changes:
- Moves free-text URL redaction into shared core code.
- Adds web and TUI display-boundary helpers.
- Adds helper-focused regression tests.
| File | Description |
|---|---|
core/mcp/fetchTracking.ts |
Adds shared free-text URL redaction. |
clients/cli/src/error-handler.ts |
Uses the shared redactor. |
clients/web/src/utils/errorFormat.ts |
Redacts formatted display errors. |
clients/web/src/utils/errorFormat.test.ts |
Tests web error redaction. |
clients/web/src/test/core/mcp/fetchTracking.test.ts |
Tests shared redaction behavior. |
clients/web/src/App.tsx |
Redacts application-level toasts. |
clients/web/src/hooks/useConnectionLifecycle.ts |
Redacts connection errors. |
clients/web/src/hooks/useExportActions.ts |
Redacts replay failures. |
clients/web/src/hooks/useImportClientConfig.ts |
Redacts import failures. |
clients/web/src/hooks/useMcpApps.ts |
Redacts app resource failures. |
clients/web/src/hooks/useOAuthRecovery.ts |
Redacts OAuth recovery errors. |
clients/web/src/hooks/useServerCommands.tsx |
Redacts command errors. |
clients/web/src/hooks/useServerJsonImport.ts |
Redacts server-import errors. |
clients/web/src/components/screens/AppsScreen/AppsScreen.tsx |
Redacts rendered app errors. |
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx |
Redacts skill-operation errors. |
clients/web/src/components/elements/ListLoadError/ListLoadError.tsx |
Redacts list errors. |
clients/web/src/components/groups/ResourceLink/ResourceLink.tsx |
Redacts resource errors. |
clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx |
Redacts configuration failures. |
clients/web/src/components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal.tsx |
Redacts removal failures. |
clients/tui/src/utils/errorText.ts |
Adds TUI display-redaction helpers. |
clients/tui/__tests__/errorText.test.ts |
Tests TUI helpers. |
clients/tui/src/App.tsx |
Redacts primary TUI errors. |
clients/tui/src/components/AuthTab.tsx |
Redacts authorization errors. |
clients/tui/src/components/PromptsTab.tsx |
Redacts prompt failures. |
clients/tui/src/components/PromptTestModal.tsx |
Redacts prompt test details. |
clients/tui/src/components/ResourcesTab.tsx |
Redacts resource failures. |
clients/tui/src/components/ResourceTestModal.tsx |
Redacts resource test details. |
clients/tui/src/components/SkillsTab.tsx |
Redacts skill verification errors. |
clients/tui/src/components/ToolTestModal.tsx |
Redacts tool test details. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Redact the TUI's nested OAuth failure and skills-list error, the web skills-list alert, the failed step-up outcome and the pagination ServerListReloadError toast. Classify the deep-link 409 on the raw message and redact only the recorded copy, with a regression test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
|
Copilot round 1: 6 findings, all in scope, all fixed in 411bb7f.
I also swept the remaining direct Re-ran |
…eview Redact the TUI's interactive-reauth and standard step-up OAuth catches and the failed-revocation detail in both clients' clear warnings, with regression tests for each. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
|
Copilot round 2: 3 findings, all in scope, all fixed in 55a1fe8.
Round 1 missed these because they were multi-line ternaries. A multi-line sweep now finds no remaining Re-ran |
| return text.replace(EMBEDDED_URL_PATTERN, (match) => { | ||
| const end = trailingPunctuationStart(match); | ||
| return redactUrlQuery(match.slice(0, end)) + match.slice(end); |
There was a problem hiding this comment.
Declining as out of scope for #2490. This PR moves redactUrlsInText into core unchanged. Splitting off trailing punctuation was a deliberate choice in #2488/#2540, so a URL that ends a sentence keeps its full stop, and the CLI's error envelope behaves the same way today. Changing that trade-off is new hardening of the shared helper, not a defect in what this PR added. It's also low-risk in practice: OAuth codes, access tokens and client secrets are base64url or hex, so they don't end in .,;:!?)]' (a JWT's dots are internal, never trailing). If we want to change it, it should be its own issue covering all three consumers.
|
Copilot round 3: 1 finding, declined as out of scope (reply in thread). It asks to change how the shared Copilot review loop closed. The round held only an out-of-scope finding, so nothing was changed and another round would only re-argue scope. Rounds: 1 (6 fixed), 2 (3 fixed), 3 (1 declined). |
v2/main gained the #2568 CLI rollup PRs (incl. #2582 `--output`) and #2587/#2588. Conflicts were import blocks in the TUI Prompts/Resources/ Skills tabs and ToolTestModal (#2430 / #2571 imports next to #2588's errorText helpers) — both sides kept — and the CLI test README, where v2/main's table is kept minus the `open-url.test.ts` row #2533 moved to core. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
Closes #2490
What
Error text the web and TUI clients draw on screen is now URL-query-redacted, the same guarantee the CLI's stderr envelope got in #2488 and the web Network log already had: a server or SDK error quoting
https://…?code=…showscode=%5BREDACTED%5Dinstead of the live value, while the path and non-sensitive params stay readable.Follows the issue's suggested shape:
redactUrlsInText(and its privateEMBEDDED_URL_PATTERN/ trailing-punctuation scan) moved verbatim fromclients/cli/src/error-handler.tsintocore/mcp/fetchTracking.ts, next toredactUrlQuery. The CLI now imports it from core; its envelope behavior is unchanged and its existing tests still pass. The helper's own tests live inclients/web/src/test/core/mcp/fetchTracking.test.ts.utils/errorFormat'serrorMessage()andformatErrorDetails()now redact. Every site that puterr instanceof Error ? err.message : String(err)(or a bareerr.message) from a caught error into a toast or inline error now callserrorMessage(err)instead:App.tsx,useConnectionLifecycle,useOAuthRecovery,useServerCommands,useMcpApps,useExportActions,useImportClientConfig,useServerJsonImport,SkillsScreen,ServerConfigModal,ServerRemoveConfirmModal,ResourceLink,ListLoadError,AppsScreen. The mid-sessionlastErrortoast and the re-auth banner detail (formatOAuthFailureDetail) are redacted as well.clients/tui/src/utils/errorText.ts(errorMessage,redactErrorText,redactedJson). Used for the connect/OAuth/disconnect errors inApp.tsx(plus core'slastErrorwhere it reaches InfoTab),AuthTab,ResourcesTab,PromptsTab,SkillsTab. The three*TestModals redact at render, both the red error line and the pretty-printed error-details JSON.Judgment calls
instanceof,isEmaClientNotConfiguredError, the deep-link "already exists" check) still reads the original error.errorfield and the Protocol panel's raw JSON-RPC messages. Those are recorded traffic, not error text formatted for display. Messages the app writes itself with no server text in them (e.g.UrlElicitationLoopError) were also left alone.Tests
fetchTracking.test.ts:redactUrlsInTextcases (prose, trailing punctuation, the CLI error-envelope URL redaction uses a quadratic regex on server-controlled text (CodeQL, ReDoS) #2540 long-run case, mixed-case schemes, comma-joined URLs, apostrophes and quotes, URLs inside serialized JSON, untouched text).errorFormat.test.ts:errorMessage/formatErrorDetailsredact.clients/tui/__tests__/errorText.test.ts: tests for the new helper.local:validate,verify:skills:cli,coverage:{web,cli,tui,launcher}(all changed files ≥90 on all four dimensions),verify:build-gateandverify:bundle-externalsare green. The run was in a shared sandbox with many concurrent agent gates. Three things to know:secret-store-selection.test.ts > isOnMountPoint > is false when the path itself can't be resolvedfails only in this sandbox. The worktree sits under a bind mount (/proc/self/mountinfo), so a cwd-relative path is legitimately "on a mount". It is unrelated to this diff.inspectorClient-timeout-diagnosticsfailed under load. This is the known flake tracked in Flaky integration test: inspectorClient-timeout-diagnostics 'emits connectionDiagnosticsChange' reads an undefined GET log entry #2580. It passes when run alone.HTTP(S)_PROXYunset. With the proxy set,transport.test.tsandserver-extra-coverage.test.tsfail withProxy response (502).smoke,smoke:web:firefoxandlocal:storybookwere not run locally. I queued them behind the machine-wide gate lease three times, and each attempt hit the lease's 45-minute cap with 10+ gates ahead of it. CI runs all three. The diff only changes the text of error messages, so it shouldn't affect the smokes or stories.🤖 Generated with Claude Code