Skip to content

feat(network): show auth response bodies with masked secrets + reveal toggle (#1386)#1387

Open
cliffhall wants to merge 18 commits into
v2/mainfrom
v2/auth-response-bodies
Open

feat(network): show auth response bodies with masked secrets + reveal toggle (#1386)#1387
cliffhall wants to merge 18 commits into
v2/mainfrom
v2/auth-response-bodies

Conversation

@cliffhall
Copy link
Copy Markdown
Member

Closes #1386.

Stacked on #1385 (#1384). Base is v2/oauth-network-persist so the diff is just this feature. Rebase down the chain after the parents merge.

Problem

auth Network entries showed request body/headers/status but never the response body (rendered (empty)), because buildEffectiveAuthFetch deliberately skipped capturing it to avoid surfacing access_token/refresh_token. That hid the most useful thing to inspect when debugging OAuth — the token exchange (see the reported /token screenshot: content-length 115, but Response Body empty).

Change

Capture auth response bodies, but mask sensitive OAuth fields by default behind a click-to-reveal toggle (per maintainer direction — safest for screen-shares):

  • inspectorClient — wire updateResponseBody on the auth fetcher so bodies are captured.
  • src/utils/maskSecrets.tsmaskSecretsInBody() masks access_token, refresh_token, id_token, client_secret (case-insensitive, nested in objects/arrays) in JSON bodies and reports whether anything was masked. Non-JSON or secret-free bodies pass through untouched (no toggle).
  • NetworkEntry BodyPreview — when a body has masked fields, render masked with a Reveal/Hide toggle and a "Secrets hidden/revealed" label. Copy honors the shown view. Masking is UI-only; the captured entry is unchanged so reveal shows the real value.

Security note

access_token/refresh_token are in the post-redirect /token response, which is never written to the session-restore files (#1384). Only pre-redirect bodies (public discovery metadata, DCR /register) persist — for a public PKCE client /register has no secret — so no bearer token hits disk.

Verification

End-to-end against the MCP SDK demo OAuth server: the /token response now shows masked by default (access_token: "••••••••", "Secrets hidden"); Reveal exposes the raw value (51e64838-…, "Secrets revealed", button → "Hide"); discovery and the public /register response render with no toggle (content-driven). npm run validate (1864), test:integration (491), test:storybook (322, incl. a new AuthSuccess play function) all green.

Acceptance criteria

  • Successful /token exchange shows its JSON response body, with secrets masked until revealed.
  • Non-sensitive bodies (discovery) show fully with no toggle.
  • No regression to transport-entry body display.

cliffhall and others added 4 commits May 30, 2026 17:15
)

Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…`
query string) with empty sessionStorage made every `/api/*` request 401 —
the browser had no way to recover the backend's auth token.

Embed the token into `index.html` on every page load so the browser no
longer depends on the query string surviving navigation:

- New shared helper `clients/web/server/inject-auth-token.ts` embeds
  `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against
  `</script>` injection; no-op when auth is dangerously omitted).
- Dev: the Vite plugin injects via `transformIndexHtml`.
- Prod: the Hono server injects on the `/` route.
- `App.tsx` `getAuthToken()` now reads the injected global first, then the
  query string, then sessionStorage (both fallbacks preserved).
- Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL`
  (`core/mcp/remote/constants.ts`).

Tests: helper unit coverage + an integration test exercising the real
prod server's `/` → `/api/*` flow (injected token authenticates; missing
token 401s). AGENTS.md documents the token-recovery order.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OAuth-protected MCP servers could not be connected to from the v2 web
client: the core OAuth pipeline exists, but App.tsx never invoked it, so a
connect attempt 401'd and surfaced "Remote send failed (401): … Missing
Authorization header" as a toast.

Wire the two missing entry points (all core primitives already in place):

- Auto-trigger on 401: in onToggleConnection's catch, detect an upstream
  401 (isUnauthorizedError) and call client.authenticate(), which runs
  discovery + DCR (backend-proxied) and redirects the page to the auth
  server via BrowserNavigation. The initiating server id is persisted to
  sessionStorage first, since the OAuth `state` carries only mode+authId
  and the full-page redirect wipes React state.
- /oauth/callback handler: a mount effect that, once `servers` hydrate,
  parses the callback params, recovers the pending server, rebuilds its
  InspectorClient, runs completeOAuthFlow(code) (PKCE verifier + DCR client
  info survive in BrowserOAuthStorage), replaceState("/") so a reload can't
  replay the single-use code, then connect(). An `error=` callback toasts
  instead of retrying.

connect() already attaches the OAuth provider to the transport
(inspectorClient.ts), so once tokens land in BrowserOAuthStorage the
outbound request carries the bearer token.

Extracted the pure pieces (constants + isUnauthorizedError) to
src/utils/oauthFlow.ts with unit tests. Verified end-to-end in a real
browser against the MCP SDK demo OAuth server: Connect -> redirect ->
auto-approve -> callback -> Connected, with the access token shown in the
Connection Info modal (#1377).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#1384)

After #1379, the Network tab showed only the post-redirect auth HTTP
(discovery re-run + POST /token); the pre-redirect discovery and the DCR
POST /register that run during authenticate() were lost when the page
navigated to the auth server.

Root causes:

1. Ordering — BrowserNavigation set `window.location.href` before the
   client's `saveSession` event fired (OAuthManager calls onBeforeOAuthRedirect
   *after* auth() already navigated), so the save raced the unload and was
   dropped. Fix: BrowserNavigation now runs a synchronous `beforeNavigate`
   hook immediately before assigning location.href; App wires it through
   createWebEnvironment to flush the active fetch log to RemoteInspectorClient
   Storage (keyed by the authId parsed from the auth URL) via a keepalive POST
   that outlives the unload.

2. Illegal invocation — RemoteInspectorClientStorage defaulted to
   `this.fetchFn = globalThis.fetch` and called `this.fetchFn(...)`, which
   re-binds `this` and makes native fetch throw "Illegal invocation"
   (swallowed by the catch). This silently broke *all* session save/load.
   Fix: default to a wrapper that preserves the global receiver.

3. Restore race — hydrateFetchRequests replaced the list, so a load that
   resolved after the resuming connect appended live entries would clobber
   them. Fix: merge restored (older) entries ahead of live ones, dedupe by id.

saveSession also now uses keepalive: true.

Verified end-to-end against the MCP SDK demo OAuth server: the connected
page's Network tab shows the full handshake — pre-redirect discovery + DCR
/register plus post-redirect discovery + /token as `auth`, alongside
`transport`. Added unit tests for the beforeNavigate ordering and the
hydrate merge/dedupe.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… toggle (#1386)

Auth-category Network entries showed request body/headers/status but never
the response body (rendered "(empty)"), because buildEffectiveAuthFetch
deliberately skipped capturing it to avoid surfacing access_token /
refresh_token. That hid the most useful thing to inspect when debugging
OAuth — the token exchange.

Capture auth response bodies, but mask sensitive OAuth fields by default
behind a click-to-reveal toggle so they aren't exposed at a glance during
a screen-share:

- inspectorClient: wire updateResponseBody on the auth fetcher.
- src/utils/maskSecrets.ts: maskSecretsInBody() masks access_token,
  refresh_token, id_token, client_secret (case-insensitive, nested) in JSON
  bodies; reports whether anything was masked. Non-JSON / secret-free bodies
  pass through untouched.
- NetworkEntry BodyPreview: when a body has masked fields, render it masked
  with a Reveal/Hide toggle (copy honors the shown view). Masking is a UI
  concern; the raw entry is unchanged so reveal shows the real value.

access_token / refresh_token live in the post-redirect /token response,
which is never written to the session-restore files (#1384); only
pre-redirect bodies (public discovery, DCR /register) persist, so no bearer
token hits disk.

Verified end-to-end: the /token response shows masked by default
(access_token: "••••••••"), Reveal exposes the raw value, and discovery /
the public DCR /register response (no secret) render with no toggle. Added
util + component unit tests and a story play function.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall cliffhall added auth Issues and PRs related to authorization v2 Issues and PRs for v2 labels May 30, 2026
@cliffhall
Copy link
Copy Markdown
Member Author

cliffhall commented May 30, 2026

How V1 Inspector Hides Entire Secrets-containing Body in the Auth Debugger:

Screenshot 2026-05-30 at 7 57 29 PM

V2 Just Masks the Actual Secret

Secrets Hidden

Screenshot 2026-05-30 at 7 54 31 PM

Secrets Revealed

Screenshot 2026-05-30 at 7 54 17 PM

… + no-store (#1378)

Addresses code-review feedback on the token-injection PR:

- Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously
  served the raw index.html off disk via serveStatic, bypassing injection — so
  a bookmark/reload at a non-`/` route with empty sessionStorage would 401.
  Route the SPA fallback through the same `serveIndexHtml` (inject) helper;
  real static assets (paths with a dot) still serve verbatim. Dev already
  injected on every HTML serve via Vite `transformIndexHtml`.
- `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__`
  to sessionStorage (not just the URL-param branch), priming the backstop for
  any later navigation that loses the global.
- Injected HTML responses now send `Cache-Control: no-store`, so a page
  carrying a token isn't cached and served stale after a restart regenerates
  the token.

Integration tests added: SPA fallback (`/oauth/callback`) carries the token,
`Cache-Control: no-store` on injected HTML, real assets served verbatim, and
unknown `/api` routes 404 rather than falling through to the HTML shell.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 31, 2026

Claude finished @cliffhall's task in 2m 46s —— View job


Claude Code Review

Reviewed the diff against origin/v2/oauth-network-persist. Overall this is a clean, well-scoped change with thoughtful tests and a sound security story. A few small notes below.

  • Gather PR context
  • Read changed files
  • Analyze for correctness, security, tests
  • Post review

Summary

  • Masking logic (maskSecretsInBody) is small, well-isolated, and covered by focused unit tests (case-insensitive keys, nested objects/arrays, non-JSON pass-through, empty-string non-flagging, pure-reformatting no-op).
  • The BodyPreview reveal toggle is correctly placed after the hook declaration and before the early "too large" return — hook order stays stable.
  • The Storybook AuthSuccess play function exercises the mask-then-reveal flow end-to-end.
  • Backend side just wires updateResponseBody for the auth fetcher; the rest is display logic, keeping captured state untouched (good for diff against v2/oauth-network-persist).

Security claim verification

The PR description claims post-redirect /token bodies never persist. I traced this:

  • inspectorClient.ts:241saveSession() is called only from onBeforeOAuthRedirect.
  • fetchRequestLogState.ts:106-119 — the saveSession event listener is the only path that writes fetchRequests to storage.
  • No other call site invokes client.saveSession() (verified via grep).

So once the browser redirects to the IdP, the /token exchange and any subsequent refresh exchanges happen post-restore and are never re-persisted. The security story checks out: access_token / refresh_token stay in memory only. Worth keeping an eye on this if a future PR adds another saveSession() trigger (e.g., a periodic snapshot) — the masking-is-UI-only design would then leak secrets to disk. A brief comment near saveSession() saying "never call this post-token-exchange — token bodies in the log are intentionally unmasked at the source" would be a useful tripwire.

Findings (none blocking)

1. Form-encoded bodies bypass masking entirelyclients/web/src/utils/maskSecrets.ts:64-77

maskSecretsInBody only handles JSON. OAuth token/refresh requests are application/x-www-form-urlencoded, so a confidential-client client_secret=... or a refresh_token=... in a refresh request body would be displayed raw with no toggle. The PR is targeting public PKCE clients, where this doesn't matter today, but it's a known gap. Either:

  • Document this explicitly ("masking applies to JSON response bodies; form-encoded request bodies are out of scope for this PR"), or
  • Extend maskSecretsInBody to also recognize a form-encoded body (sniff &/= shape or take a content-type hint) and mask client_secret, refresh_token, code, code_verifier. The reveal toggle UX already works.

If keeping JSON-only, consider renaming to maskSecretsInJsonBody for clarity.

2. hasSecrets detection is structurally fragilemaskSecrets.ts:71-76

const maskedNode = maskNode(parsed);
const masked = JSON.stringify(maskedNode, null, 2);
const hasSecrets = JSON.stringify(maskedNode) !== JSON.stringify(parsed);

The double-stringify is clever but relies on maskNode being a pure non-mutating identity transform for non-secret nodes (currently true). If anyone ever adds, say, key normalization or default-stripping to maskNode, this would silently flag every body as "has secrets". Cleaner: have maskNode return { node, masked: boolean } and propagate the flag up. Cheaper too (one stringify instead of two). Not blocking.

3. BodyPreview reveal state doesn't reset on body change

useState(false) in BodyPreview is keyed on component instance, not on the body prop. Today response bodies are written once via updateResponseBody (core/mcp/fetchTracking.ts:178) and never replaced, so there's no observable bug. If a future change ever streams partial bodies into the same entry, a previously-revealed view would stay revealed across content swaps. A useEffect(() => setRevealed(false), [body]) (or a key={body} on BodyPreview) would future-proof this — but optional.

4. Minor a11yNetworkEntry.tsx:159-171

The masked ↔ revealed transition swaps a large block of monospace text but isn't announced. For screen-reader users, the "Secrets revealed" label updates but the body change is silent. An aria-live="polite" on the body container, or an aria-label on the Reveal button that says "Reveal secrets in response body", would help. Low priority.

5. Pretty-print asymmetrymaskSecrets.ts:72

Masked view is always pretty-printed; revealed view shows the original raw body (often minified on the wire). Toggling between them therefore reflows the layout. Not wrong, just a slight UX jolt. If you want symmetry, pretty-print the revealed body too (it's already passing through ContentViewer, which has its own JSON detection at ContentViewer.tsx:18-24 — so this may already happen for the non-secret JSON path; worth a quick visual check).

6. Nit: documentation tripwire

The old comment in inspectorClient.ts was explicit about why updateResponseBody was skipped for auth. The new comment explains the new design well. Consider also dropping a one-liner in core/mcp/state/fetchRequestLogState.ts near saveSession reminding future-you that auth response bodies in the log may contain unmasked secrets (since masking is UI-only) — so any new persistence path needs to redact before write.

Test coverage

Unit tests look complete for the happy paths. One suggestion:

  • maskSecrets.test.ts doesn't test the "valid JSON but not an object" branch (e.g., JSON.parse('"abc"'), JSON.parse('42'), JSON.parse('null')). These should fall through with hasSecrets: false and be re-serialized untouched — verifying that explicitly would lock in the contract.

Otherwise the test set (incl. the new NetworkEntry.test.tsx reveal/hide assertions and the Storybook play function) gives me good confidence.

Verdict

LGTM with the small caveat that the security guarantee is currently a function of saveSession's call-site discipline rather than a code-enforced redaction. As long as the team treats "captured auth bodies are sensitive" as an invariant that any new persistence path must respect, this PR is in good shape.
• Branch: v2/auth-response-bodies

cliffhall and others added 7 commits May 30, 2026 20:05
… 401 match (#1379)

Code-review feedback on the OAuth-wiring PR:

- Callback effect: split completeOAuthFlow vs connect() into separate
  try/catch blocks. A token-exchange failure now reads "OAuth token exchange
  failed … Please try connecting again." (the single-use code is spent and the
  URL was cleared, so a reload can't retry); a post-OAuth connect failure reads
  "Failed to connect" since OAuth actually succeeded and re-clicking Connect
  reuses the persisted tokens.
- isUnauthorizedError: anchor the message fallback on the transport's
  `failed …(401)` wording instead of a bare `(401)`, so an unrelated `(401)`
  spliced into an error message can't trip the OAuth flow. Added a test.
- Documented that clearing the pending id + URL before the server lookup is
  intentional (deleted/renamed server mid-flow → require a fresh Connect).

Also merges the squash-merged #1382 base from v2/main.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts:
#	clients/web/src/App.tsx
cliffhall and others added 2 commits May 30, 2026 20:30
…p + test fetch default (#1384)

Code-review feedback on the OAuth Network-log persistence PR:

- Documented the double-save: `FetchRequestLogState`'s `saveSession` listener
  is the backstop; `BrowserNavigation`'s `beforeNavigate` hook is the primary
  flush for the redirect case. Notes the listener may lose the navigation race
  and is harmless when it duplicates (last-writer-wins, identical payload).
- Reworded the keepalive comment in `RemoteInspectorClientStorage.saveSession`:
  the 64KB cap is general (the method is also reachable from the listener with
  the full session log), so a long session could exceed it and drop silently —
  acceptable since the persisted log is best-effort, not load-bearing.
- Added a regression test that constructs `RemoteInspectorClientStorage`
  without a `fetchFn`, stubs `globalThis.fetch`, and asserts the default
  wrapper calls it (locks in the "Illegal invocation" fix, which callers
  otherwise swallow).

Optional items (logger.warn on swallowed save errors; setupClientForServer dep
churn) acknowledged on the PR, not changed.

Also merges the squash-merged #1383 base from v2/main.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 31, 2026

Claude finished @cliffhall's task in 2m 30s —— View job


Claude Code Review (second pass)

Reviewed against origin/v2/oauth-network-persist. The prior Claude review at 2026-05-31T00:02:58Z already covered the main angles (form-encoded gap, double-stringify fragility, reveal-state reset, a11y, pretty-print asymmetry, scalar-JSON test gap, tripwire comment). This pass focuses on what was not called out there.

  • Gather PR context
  • Read changed files
  • Analyze for correctness, security, tests
  • Post final review

Fresh observations

1. Masking is broader than the PR title implies — and that's good, but worth pinning down

BodyPreview is rendered for both entry.requestBody (NetworkEntry.tsx:228) and entry.responseBody (NetworkEntry.tsx:245). So any JSON request body containing a sensitive key (e.g. a confidential-client client_secret sent as JSON, or a refresh exchange whose request happens to be JSON) is also masked, not just responses. The PR title and description scope this to "auth response bodies" but the implementation is symmetric. Suggest updating the description and a code comment so a future contributor doesn't tighten the scope back to responses and accidentally weaken coverage. (Form-encoded /token request bodies still bypass — see prior review's point 1.)

2. maskValue short-circuits only for string values; objects under a sensitive key recurse

maskSecrets.ts:28-33:

function maskValue(key: string, value: unknown): unknown {
  if (isSensitiveKey(key) && typeof value === "string" && value.length > 0) {
    return MASK_PLACEHOLDER;
  }
  return maskNode(value);  // recurses
}

If a server ever returned a sensitive key whose value is not a string (e.g. access_token: { value: "..." } — non-standard, but technically possible), maskNode would walk into it and only mask leaves whose own keys are sensitive — the wrapping object falls through as-is. Per OAuth specs these are always strings, so this is defensible, but it's a silent edge. Cheapest fix: if (isSensitiveKey(key)) return MASK_PLACEHOLDER (drop the type check entirely; non-string sensitive values are always weird and a fixed-shape placeholder is a safer default than recursion).

3. Clipboard exfiltration is intentional but undocumented

ContentViewer block={{ type: "text", text: shown }} copyable (NetworkEntry.tsx:169) means Copy while revealed lands the raw secret on the system clipboard. The PR description says "Copy honors the shown view" — which is correct — but a screen-share viewer who watches a reveal-then-copy can't see the clipboard, so this is a real escalation path beyond "saw it for a second." Not asking to change the behavior (the user explicitly revealed; copying is their call), but please drop a one-liner near the copyable prop saying so, since the safer assumption when scanning the file is "copy reflects the masked view."

4. MAX_INLINE_BODY_CHARS interacts cleanly with masking — confirmed, not a bug

NetworkEntry.tsx:140-147 runs the tooLarge check before maskSecretsInBody. A pathological case (a body that's both > 100k chars and contains access_token) shows the "too large" placeholder with no parse and no leak. Good — wanted to flag this because the order matters and it's not obvious from the diff that it was deliberate.

5. Mantine compliance check (per AGENTS.md)

All passes: RevealButton uses .withProps() (NetworkEntry.tsx:130-133); BodyPreview uses Stack/Group/Text/Button only; no inline styles, no raw hex; hook order is stable (useState before the early return, with an explanatory comment). Nothing to fix here, calling it out as confirmation.

6. Test gap: request-body masking branch

The new masks token-response secrets until revealed test (NetworkEntry.test.tsx:200-230) exercises only the response side. Given observation 1 — that the same code path handles request bodies — one test asserting that a JSON request body with access_token also gets masked would lock in the symmetric behavior. Otherwise a future refactor could split the code path and we'd lose half the coverage without a red test.

Security claim — verified end-to-end

Traced saveSession callers across core/ and clients/web/. There are exactly two flush points and both run pre-redirect:

  • inspectorClient.ts:241this.saveSession() from onBeforeOAuthRedirect, before window.location.href assignment.
  • App.tsx:487-510onBeforeOAuthRedirect synchronously calls sessionStorageAdapter.saveSession(...) via BrowserNavigation's beforeNavigate hook (core/auth/browser/providers.ts:34), again before navigation commits.

Both capture the log at the moment of redirect — the /token exchange happens after navigation, so its body is never in scope of either save path. The prior Claude review's tripwire suggestion (a comment near saveSession() warning future contributors not to add a post-redirect flush) is sound — please consider it.

Verdict

LGTM. The implementation is solid; the security guarantee currently relies on saveSession() call-site discipline rather than code-enforced redaction, and that discipline should be made explicit in a comment.
• Branch: v2/auth-response-bodies

Base automatically changed from v2/oauth-network-persist to v2/main May 31, 2026 00:39
cliffhall and others added 2 commits May 30, 2026 20:39
…lag, a11y (#1386)

Code-review feedback on the auth-response-body masking PR:

- Extended masking to form-urlencoded bodies (finding #1): the token *request*
  is `application/x-www-form-urlencoded` and carries `code` / `code_verifier` /
  `client_secret` / `refresh_token`. `maskSecretsInBody` now masks those in
  form bodies too (preserving formatting; placeholder not percent-encoded).
  `code`/`code_verifier`/`client_assertion` are form-only sensitive keys —
  deliberately NOT masked in JSON, where `code` is usually an error/status code.
- Replaced the double-stringify `hasSecrets` heuristic with an explicit
  masked-flag propagated out of `maskNode` (finding #2) — robust if the
  transform ever grows non-identity behavior, and one fewer serialization.
- Reset reveal state on body change via `key={body}` remount instead of a
  setState-in-effect (finding #3; avoids the cascading-render lint rule).
- a11y (finding #4): `aria-label` on the Reveal/Hide button and `aria-live`
  on the hidden/revealed status text.
- Security tripwire comment near the `saveSession` listener (finding #6):
  captured auth bodies are unmasked at source (masking is UI-only), so any new
  post-token-exchange persistence path must redact first.

Tests: form-encoded masking (token + refresh requests), JSON `code` NOT masked,
non-object JSON pass-through, default-fetch wrapper. Story play + NetworkEntry
tests updated for the new aria-labels and the now-masked form request body.
Finding #5 (pretty-print asymmetry) is already handled by ContentViewer's JSON
formatting; #4-aria and #6 are the doc/a11y bits.

Also merges the squash-merged #1385 base from v2/main.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

Thanks for the thorough review — addressed the substantive items in 88aa583.

1. Form-encoded bodies bypassed masking — Implemented (didn't just document). maskSecretsInBody now masks form-urlencoded bodies too, so a confidential-client client_secret=… or a refresh_token=…/code=…/code_verifier=… in a token/refresh request body is masked with the same reveal toggle. Formatting is preserved and the placeholder isn't percent-encoded (manual split rather than URLSearchParams.toString()). code/code_verifier/client_assertion are form-only sensitive keys — deliberately kept out of the JSON set so a JSON-RPC error code isn't masked. Kept the name maskSecretsInBody since it now handles both.

2. hasSecrets double-stringify fragility — Fixed your way: maskNode now returns { node, masked } and propagates the flag up; no more comparing serializations (and one fewer stringify). Robust if the transform ever grows non-identity behavior.

3. Reveal state doesn't reset on body change — Done via key={body} remount on <BodyPreview> (your alternative) rather than a setState-in-effect, which trips our cascading-render lint rule.

4. a11y — Added an aria-label on the Reveal/Hide button ("Reveal/Hide secrets in body") and aria-live="polite" on the hidden/revealed status text.

5. Pretty-print asymmetry — No change needed: ContentViewer.formatJson already pretty-prints any JSON text it renders ({/[-leading), so both the masked and revealed JSON views are formatted consistently. Form bodies share the same shape masked vs revealed, so no reflow there either.

6. Tripwire comment — Added a SECURITY TRIPWIRE note on the saveSession listener: captured auth bodies are stored unmasked (masking is UI-only), today's only saveSession trigger is the pre-redirect flush (before /token), and any new post-token-exchange persistence path must redact response bodies first.

Tests — Added: form-encoded masking (token + refresh requests, formatting preserved), JSON code NOT masked, valid-but-non-object JSON ("abc"/42/null/true) pass-through, and the default-fetchFn-wrapper guard (carried from #1385). Updated the NetworkEntry tests + Storybook AuthSuccess play for the new aria-labels and the now-masked form request body (reveals both bodies).

On the security verdict: agreed it's call-site discipline, not code-enforced redaction — the new tripwire comment makes that invariant explicit at the persistence path. validate (1874), test:integration (495), test:storybook (322) all green. Base is now v2/main (#1385 merged); this is the last PR in the stack.

@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 31, 2026

Claude finished @cliffhall's task in 2m 53s —— View job


Claude Code Review (third pass)

Reviewed against origin/v2/main. The author addressed the substantive items from the prior two passes (form-encoded masking is now implemented, the {node, masked} flag is propagated, key={body} remounts on body change, a11y labels added, security tripwire comment in place). Re-verified each of those and they look right. This pass focuses on what's still surfaceable.

  • Gather PR context
  • Read changed files
  • Analyze for correctness, security, tests
  • Post review

Fresh observations

1. The "non-JSON ⇒ treat as form" fallback is broader than the name suggestsmaskSecrets.ts:124-133

export function maskSecretsInBody(body: string): MaskResult {
  let parsed: unknown;
  try {
    parsed = JSON.parse(body);
  } catch {
    return maskFormBody(body);
  }
  ...
}

Any body that isn't valid JSON — HTML error pages, plaintext stack traces, XML, etc. — runs through maskFormBody. Today this is harmless because non-form bodies have no key=value&… shape with a sensitive key, so they fall through with hasSecrets:false. But the implicit contract is "anything not JSON is treated as form-encoded", which is a heuristic. Two consequences worth pinning down:

  • A plaintext server error like Failed: access_token=abcdef expired would be detected as form-encoded and masked. False positive, but conservative (no info leaked).
  • BodyPreview could pass a content-type hint from the entry headers (application/json vs application/x-www-form-urlencoded) instead of guessing per body — would also make the contract explicit and let unknown content-types skip masking entirely. The plumbing is one extra prop.

Either way, a one-line block-doc on maskSecretsInBody saying "JSON parses first; everything else is treated as form-encoded" would prevent a future reader from being surprised.

2. Sensitive-key-with-non-string-value still recurses (carried from prior pass, not addressed)maskSecrets.ts:63-74

if (
  JSON_SENSITIVE_KEYS.has(key.toLowerCase()) &&
  typeof value === "string" &&
  value.length > 0
) {
  out[key] = MASK_PLACEHOLDER;
  masked = true;
} else {
  const r = maskNode(value);   // recurses into objects/arrays under a sensitive key
  ...
}

A non-standard server returning access_token: { value: "secret", expires: ... } would have value left untouched (the wrapping object's key value isn't sensitive). OAuth specs always say strings, so this is academic, but the safer default is "any value under a sensitive key is replaced wholesale":

if (JSON_SENSITIVE_KEYS.has(key.toLowerCase())) {
  out[key] = MASK_PLACEHOLDER;
  masked = true;
}

That also covers the related case where the value is a stringified JSON blob ({"raw":"{\"access_token\":\"abc\"}"} — the outer raw isn't sensitive and the inner JSON string is never re-parsed, so it leaks today). Neither is a real-world concern; both go away with the simpler check.

3. Inline isSensitiveKey checks duplicated between pathsmaskSecrets.ts:63-67 and 100

The JSON and form paths each have their own copy of the key.toLowerCase() ∈ SET && value.length > 0 predicate. A small isSensitive(set, key, value): boolean helper would deduplicate and keep the two paths in lockstep if the rule ever grows (e.g. min-length threshold).

4. Form-body test coverage gaps

The form-side tests cover happy paths but skip:

  • Repeated sensitive key (code=A&code=B) — current code masks both, but no regression test will catch a future "first-occurrence-wins" refactor.
  • Empty sensitive value (client_secret=) — current code correctly does NOT flag; symmetry with the JSON empty-string test would be nice.

These would add maybe four lines to maskSecrets.test.ts.

5. Storybook play function reveals but doesn't re-hide

NetworkEntry.stories.tsx:122-131 clicks every Reveal button and asserts the raw token appears, but never toggles back. The unit test (NetworkEntry.test.tsx:230-233) does cover re-mask, so the missing flow in Storybook is only a documentation gap, not a coverage gap. Skip if you'd rather keep the play function lean.

6. Doc-string on MaskResult.masked is now narrow

maskSecrets.ts:111: "The body with sensitive values replaced; pretty-printed when JSON." — true for the JSON branch, but for form bodies the masked field is the same single-line form-encoded shape with •••••••• substituted (no pretty-print). One word: drop "; pretty-printed when JSON" or expand to "; pretty-printed for JSON, otherwise preserves the original shape".

Re-verification of the security claim

Re-traced saveSession callers in core/ and clients/web/:

  • inspectorClient.ts:241 — pre-redirect, in onBeforeOAuthRedirect.
  • App.tsx:500 — pre-redirect, synchronously via BrowserNavigation.beforeNavigate.
  • fetchRequestLogState.ts:124-137 — sole listener that writes fetchRequests to storage.

No post-redirect persistence path exists. The new SECURITY TRIPWIRE comment in fetchRequestLogState.ts:116-123 correctly captures the invariant. The masking-is-UI-only design is sound as long as that comment is respected — agree with prior reviews that this is call-site discipline rather than code-enforced redaction, and the tripwire is the right way to make that explicit.

Verdict

LGTM. None of the above is blocking; items 1, 2, and 6 are the highest-value to tighten up before this lands. Items 3-5 are nice-to-haves.
• Branch: v2/auth-response-bodies

…king, wholesale mask, dedup (#1386)

Third-pass code-review feedback on the auth-body masking PR:

- maskSecretsInBody now takes the body's content-type (finding #1): `*json*`
  → JSON masking, form-urlencoded → form masking, any other known type
  (HTML/plaintext/XML) → no masking; absent/unknown → sniff as before. Removes
  the implicit "non-JSON ⇒ form" guess for error pages etc. NetworkEntry passes
  the request/response `content-type` header through to BodyPreview.
- Mask any non-empty value under a sensitive key wholesale (finding #2):
  a non-standard object/array value under e.g. `access_token` is replaced
  rather than recursed into, so it can't leak. Empty-string still not flagged.
- Extracted `isSensitiveKey(set, key)` to dedupe the JSON/form key checks
  (finding #3).
- Reworded `MaskResult.masked` doc to cover the form (non-pretty-printed) case
  (finding #6).

Tests: non-string-value-under-sensitive-key wholesale mask; repeated form
param; empty form value not flagged; content-type honored (JSON type skips
form masking, text/plain skips masking, explicit form type masks).

Finding #5 (story re-hide) skipped per reviewer — unit test already covers
re-mask. Finding #4 (form edge cases) added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cliffhall
Copy link
Copy Markdown
Member Author

Thanks for the third pass — addressed 1, 2, 3, 4, 6 in e499830; skipped 5 per your offer.

1. "non-JSON ⇒ form" was too broad — Made it explicit via content-type instead of a block-doc. maskSecretsInBody(body, contentType?) now branches: *json* → JSON masking, x-www-form-urlencoded → form masking, any other known type (HTML/plaintext/XML/…) → no masking, and absent/unknown → the previous sniff (JSON first, else form). NetworkEntry threads the request/response content-type header into BodyPreview. So a plaintext error body containing access_token=… is no longer guessed at as form-encoded. Block-doc added too.

2. Non-string value under a sensitive key recursed — Fixed: any non-empty value under a sensitive key is now masked wholesale (replaced, not recursed), so a non-standard access_token: { value: "…" } can't leak. Empty-string/absent still aren't flagged (kept that test). Added a test for the object-value case.

3. Duplicated predicate — Extracted isSensitiveKey(set, key), shared by the JSON and form paths. (The value check stays inline since JSON now masks non-string values wholesale while form values are always strings.)

4. Form test gaps — Added: repeated sensitive param (code=A&code=B → both masked) and empty form value (client_secret= → not flagged), mirroring the JSON empty-string test.

6. MaskResult.masked doc was JSON-narrow — Reworded to "pretty-printed for JSON, otherwise the original shape with values substituted."

5. Story doesn't re-hide — Skipped per your note; the NetworkEntry unit test covers the re-mask toggle, so this is only a play-function doc gap.

Re the security claim: agreed it remains call-site discipline; the tripwire comment is the enforcement mechanism. validate, test:integration (495), test:storybook (322) green — one unrelated flake in the SSE OAuth-state-file e2e (waitForStateFile timeout) that passes on its own and in the dedicated integration run; nothing in this PR touches OAuth state persistence.

@cliffhall
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 31, 2026

Claude finished @cliffhall's task in 2m 46s —— View job


Claude Code Review (fourth pass)

Reviewed against origin/v2/main (no new commits since the third-pass review's e499830). Re-verified each item the author addressed from the third pass — content-type branching, wholesale-mask under sensitive keys, isSensitiveKey dedup, MaskResult.masked doc — and they're correctly implemented. Three prior passes (mine and prior Claude's) have already covered the meaningful design surface, so this pass is short on purpose.

  • Gather PR context (diff vs origin/v2/main)
  • Read changed files
  • Verify third-pass items 1, 2, 3, 6 are addressed
  • Analyze for fresh correctness, security, test gaps

Re-verifications

  • Content-type branchingmaskSecretsInBody(body, contentType?) at maskSecrets.ts:153-171 now branches: *json* → JSON masking, x-www-form-urlencoded → form masking, any other present type → no masking, absent → sniff. NetworkEntry threads requestHeaders["content-type"] and responseHeaders?.["content-type"] through (NetworkEntry.tsx:247,268). Headers captured via Headers.forEach in fetchTracking.ts:60-71,126-129 are lowercased by the platform, so the substring-match casing works in practice.
  • Wholesale mask under sensitive keymaskSecrets.ts:79-81 replaces any non-empty value under a JSON sensitive key with the placeholder rather than recursing. isMaskableValue (maskSecrets.ts:48-52) correctly filters null/undefined/"" and masks the rest wholesale. Covered by maskSecrets.test.ts:53-64.
  • isSensitiveKey extractedmaskSecrets.ts:40-42 shared by both paths (:79, :130).
  • Form-side test gaps — repeated param (:140-144) and empty form value (:146-152) both covered.
  • Security tripwirefetchRequestLogState.ts:116-123 puts the invariant where future-you will hit it. Re-traced saveSession callers: only inspectorClient.ts:241 (pre-redirect) and the synchronous BrowserNavigation.beforeNavigate flush — no post-token-exchange persistence path. The story holds.

Fresh observations (all non-blocking; ignorable)

1. Content-type matching is substring-basedmaskSecrets.ts:159-160

ct.includes("json") / ct.includes("x-www-form-urlencoded") is fine in practice (application/json; charset=utf-8, application/vnd.api+json, application/jose+json all match correctly). Two edge cases worth knowing rather than fixing:

  • A server that mislabels the response (e.g. JSON body sent as text/html) would fall into the "known type → no masking" branch and render the body raw including any access_token. Adversary-controlled labels aren't an OAuth threat model — calling it out so the contract ("we trust the wire's content-type label") is on the record.
  • A weird type like application/x-json-but-not-really still takes the JSON path; JSON.parse then fails and maskJsonBody falls through with hasSecrets:false. Safe by construction.

A one-line note on maskSecretsInBody saying "matches *json* / *x-www-form-urlencoded* by substring; we trust the wire content-type" would lock the contract in. Optional.

2. Form-side decodeURIComponent fallback is silent on malformed keysmaskSecrets.ts:125-129

A malformed percent-encoding (client%ZZsecret=…) falls back to matching against the raw key, so the human-readable client_secret isn't matched. This is the safer-default direction (rather than throwing) and you'll never see this in real OAuth traffic, but it does mean a hypothetical attacker who controls the request body could craft a key that looks sensitive to a reader but won't get masked. Threat model is "screen-share viewer," not "byzantine request crafter," so this is fine — flagging it as a known quirk, not a defect.

3. contentType change without body change doesn't reset revealNetworkEntry.tsx:245-247,265-268

BodyPreview is keyed by body only. If a future code path replaces the entry's content-type header without changing the body bytes, a previously-revealed view would persist with a different mask. Not reachable today (body and headers are written once together at the end of fetchTracking), so this is a "future-proof if the lifecycle changes" note, not a current bug.

4. Storybook AuthSuccess reveal loop assertion order is brittleNetworkEntry.stories.tsx:122-131

canvas.getAllByText("Secrets revealed").length === revealButtons.length works today (two BodyPreviews, both reveal), but if a future fixture adds a non-masked body to the same story the math drifts silently. Cheap fix: assert >= revealButtons.length to mirror the >= 2 hidden check on :115. Optional.

Test coverage

The four new test cases in maskSecrets.test.ts (third pass) and the existing reveal/hide test in NetworkEntry.test.tsx:200-234 cover the meaningful branches. The only coverage gap I'd consider adding is a unit-test counterpart for form-encoded request-body masking inside NetworkEntry (currently exercised only via the Storybook play function on AuthSuccess). One test that mounts a NetworkEntry with a form requestBody containing code=AAA and asserts the masked render + reveal would lock in the symmetry. Not blocking.

Verdict

LGTM. Items 1–4 above are nits; the substantive design has been worked through across three rounds and the code reflects that. The security guarantee remains call-site discipline around saveSession, which the tripwire comment now makes explicit at the persistence path.
• Branch: v2/auth-response-bodies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Issues and PRs related to authorization v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show OAuth auth response bodies in Network tab, with sensitive-field masking + reveal toggle

1 participant