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

[cliffhall]

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.ts — maskSecretsInBody() 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]

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

V2 Just Masks the Actual Secret

Secrets Hidden

Secrets Revealed

[claude]

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:241 — saveSession() 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 entirely — clients/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 fragile — maskSecrets.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 a11y — NetworkEntry.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 asymmetry — maskSecrets.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

[claude]

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:241 — this.saveSession() from onBeforeOAuthRedirect, before window.location.href assignment.
• App.tsx:487-510 — onBeforeOAuthRedirect 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

[cliffhall]

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]

@claude review

[claude]

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 suggests — maskSecrets.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 paths — maskSecrets.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

[cliffhall]

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]

@claude review

[claude]

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 branching — maskSecretsInBody(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 key — maskSecrets.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 extracted — maskSecrets.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 tripwire — fetchRequestLogState.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-based — maskSecrets.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 keys — maskSecrets.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 reveal — NetworkEntry.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 brittle — NetworkEntry.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

[cliffhall]

Thanks for the fourth pass — took the cheap ones in af7ad0d.

1. Content-type matching contract — Added a block-doc note on maskSecretsInBody: matched by substring (*json* / *x-www-form-urlencoded*), and we trust the wire's own label — a server-mislabeled body (JSON sent as text/html) takes the no-masking branch and renders raw, which is acceptable since the threat model is a screen-share viewer, not someone controlling the response's content-type.

3. Header-only change doesn't reset reveal — Now keying <BodyPreview> by content-type|body instead of body alone, so a content-type swap (even with identical bytes) remounts and re-hides. Not reachable today (body + headers are written together once), but cheap to future-proof.

4. Storybook assertion brittleness — Changed === revealButtons.length to >= revealButtons.length, mirroring the hidden >= 2 check, so a later non-masked body in the fixture can't silently drift it.

Test gap (form masking in NetworkEntry) — Added a unit test that mounts NetworkEntry with a form requestBody (code=…&code_verifier=…, content-type x-www-form-urlencoded) and asserts masked-by-default → reveal shows the raw values, with the non-secret grant_type staying visible. Locks in form masking at the component level rather than only via the Storybook play.

2. Silent decodeURIComponent fallback — Acknowledged as a known quirk; a byzantine request-body crafter is outside the screen-share threat model, and falling back to the raw key is the safer (non-throwing) direction. No change.

validate (1881), test:integration (495), test:storybook (322) all green — the SSE OAuth-state flake from the previous pass passed this run too.

[cliffhall]

@claude review

[claude]

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

---

Claude Code Review (fifth pass)

Reviewed against origin/v2/main. Verified each of the fourth-pass items from af7ad0d: content-type contract block-doc on maskSecretsInBody (maskSecrets.ts:153-157), <BodyPreview> keying on ${contentType}|${body} (NetworkEntry.tsx:245,266), Storybook revealed.length >= revealButtons.length (NetworkEntry.stories.tsx:130), and the new form-encoded request-body unit test (NetworkEntry.test.tsx:236-265) — all in place and correctly implemented. Four prior passes have worked through the design surface; this one is short by intent.

• Gather PR context (diff vs origin/v2/main)
• Read changed files
• Verify fourth-pass items 1, 3, 4 + form NetworkEntry test
• Analyze for fresh correctness, security, test gaps
• Post review

Fresh observations (all non-blocking)

1. maskSecretsInBody recomputes on every BodyPreview render, including reveal toggles — NetworkEntry.tsx:164

const { masked, hasSecrets } = maskSecretsInBody(body, contentType);

This runs on every render of BodyPreview, which means every Reveal/Hide click re-parses the body and walks the tree. For a 99,999-char body just under MAX_INLINE_BODY_CHARS, that's a per-click JSON parse + recursion. Trivially fixed:

const { masked, hasSecrets } = useMemo(
  () => maskSecretsInBody(body, contentType),
  [body, contentType],
);

Same hook-ordering rules — useMemo already runs before the early tooLarge return today since the useState does. Cheap and pins the cost at once-per-mount (which the key={…} remount already scopes correctly).

2. Transport bodies now pay a JSON-parse cost they didn't before — NetworkEntry.tsx:164

Pre-PR, transport BodyPreview rendered the body directly through ContentViewer. Post-PR, every transport body runs maskSecretsInBody → JSON.parse → recursive walk just to determine hasSecrets: false. For a typical tools/list response of a few KB this is invisible; for a large structured response it's a small new cost. ContentViewer then parses again for pretty-printing (per ContentViewer.tsx:18-24). The memoization in (1) caps this at one extra parse per mount, so it's mostly academic — flagging it because the masking-by-default symmetry is the right call, just noting the tradeoff exists.

3. isMaskableValue masks true/false/0 under a sensitive key but not null / "" — maskSecrets.ts:48-52

function isMaskableValue(value: unknown): boolean {
  if (value === null || value === undefined) return false;
  if (typeof value === "string") return value.length > 0;
  return true;
}

Contract: "non-null, non-empty-string value under a sensitive key gets masked wholesale, regardless of type." So access_token: 0 or access_token: false would render as •••••••• — pathological but harmless (no real OAuth server can do this), and the safer default given the third-pass fix for object-wrapped tokens. Worth one inline word in the function body comment ("non-null, non-empty-string" rather than just "non-empty") to lock the contract in. Optional.

4. MASK_PLACEHOLDER is exported only for tests — maskSecrets.ts:38

Not referenced outside maskSecrets.ts and maskSecrets.test.ts. Could be internal with the test importing it through a barrel or just hard-coding "••••••••". Nit; the current export is harmless.

5. Test coverage: full DCR-response shape isn't asserted as one fixture — maskSecrets.test.ts

client_secret masking is covered (Test #2, case-insensitive). A confidential-client /register response in the wild is { client_id, client_secret, registration_access_token, … }; an explicit fixture mirroring that shape would document the OAuth-DCR happy path alongside the token-exchange one. Optional; code path is already covered piecewise.

Re-verification

• Backend wiring — inspectorClient.ts:325-330 now passes updateResponseBody: (id, body) => this.dispatchFetchRequestBodyUpdate(id, body) to the auth fetcher, mirroring dispatchFetchRequestBodyUpdate use elsewhere in the file. fetchTracking.ts:172-187 is the body-capture path; the auth path now reuses the same fire-and-forget tee'd-stream read as the transport path. Clean, symmetric.
• Security tripwire — fetchRequestLogState.ts:116-123 is exactly where future-you will hit it when adding a new saveSession trigger. Re-traced callers: still only the pre-redirect path in inspectorClient.ts:241 and the synchronous BrowserNavigation.beforeNavigate flush. No post-redirect persistence path exists.
• Mantine compliance — RevealButton uses .withProps() (NetworkEntry.tsx:130-133); BodyPreview uses Stack/Group/Text/Button only; no inline styles or raw hex; hook order stable. Compliant with AGENTS.md.

Verdict

LGTM. The PR has been through four substantive review rounds, each addressed thoughtfully; what remains are micro-perf and contract-documentation nits. Worth taking item 1 (the useMemo) since it's a four-line change that improves the per-click cost on near-cap bodies. The rest are ignorable.
• Branch: v2/auth-response-bodies

[cliffhall]

Thanks for the fifth pass — took 1, 3, 5 in 2237d2d.

1. Re-masking on every render — Memoized: maskSecretsInBody is now wrapped in useMemo([tooLarge, body, contentType]), so a Reveal/Hide click no longer re-parses and re-walks the body. The key={contentType|body} remount re-runs it on a real content change. I kept the too-large bodies out of the parse path with an in-memo guard (tooLarge ? {masked: body, hasSecrets: false} : maskSecretsInBody(…)) rather than an early return above the hook, so we still never parse something we won't display.

3. isMaskableValue contract wording — Tightened the comment to state it exactly: non-null, non-empty-string values under a sensitive key are masked wholesale (so access_token: 0/false would render masked — pathological but the safe default).

5. DCR happy-path fixture — Added a confidential-client /register response fixture test. While writing it I noticed a real gap: a realistic DCR response carries registration_access_token (RFC 7592 management credential, same bearer class as access_token) which our exact-key set didn't mask. Added it to the sensitive set; the new test asserts both client_secret and registration_access_token are masked while client_id/registration_client_uri stay visible.

2. Transport parse cost — Left as-is; the memo from #1 caps it at one parse per mount, and the masking-by-default symmetry is the right call.

4. MASK_PLACEHOLDER test-only export — Left as-is (harmless).

validate (1882), test:storybook (322) green. This was a genuinely useful round — the registration_access_token masking is a real (if rare) leak closed.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 38s —— View job

---

Claude Code Review (sixth pass)

Reviewed against origin/v2/main. Verified each of the fifth-pass items addressed in 2237d2d:

• useMemo on maskSecretsInBody — NetworkEntry.tsx:163-169 correctly memoizes on [tooLarge, body, contentType]. The in-memo tooLarge guard keeps the hook unconditional while skipping the parse for over-cap bodies. Hook order remains stable.

• isMaskableValue contract wording — maskSecrets.ts:47-56 now spells out "non-null, non-empty-string" with the rationale.

• registration_access_token added to JSON_SENSITIVE_KEYS — maskSecrets.ts:26. Good catch: this is the RFC 7592 management credential, bearer-class. The new DCR fixture test (maskSecrets.test.ts:66-88) locks both client_secret and registration_access_token masking while leaving client_id/registration_client_uri visible. This is the most material change since the last pass — a real (if rare) leak path closed.

• Gather PR context (diff vs origin/v2/main)

• Read changed files

• Verify fifth-pass items (memoization, contract wording, DCR fixture)

• Analyze for fresh correctness, security, test gaps

• Post review

Fresh observations (all non-blocking; ignorable)

1. BodyPreview key uses the full body string — NetworkEntry.tsx:259,280

key={`${entry.requestHeaders["content-type"] ?? ""}|${entry.requestBody}`}

For a near-cap body (~100 KiB), this is a 100k-char React key. React stores keys as strings and compares them on every reconciliation pass — for one entry this is invisible; for a Network log with many large entries it adds up. A length+prefix hash (${ct}|${len}|${body.slice(0, 64)}) would be functionally equivalent for the remount-on-change goal. Micro-perf; only worth doing if profiling ever flags it.

2. onFetchRequestBodyUpdate re-emits only the list event — fetchRequestLogState.ts:82-100

The handler patches the matching entry and dispatches only fetchRequestsChange, not the per-entry fetchRequest. Subscribers that listen to fetchRequest for incremental UI updates won't see response bodies fill in — only list-event subscribers will. There are no such per-entry subscribers in the tree today (useFetchRequests reads the full list), so this is correct as-is, but a comment near dispatchTypedEvent saying "intentionally list-only; per-entry subscribers see the body via the next list re-render" would prevent a future regression.

3. isMaskableValue masks NaN — maskSecrets.ts:52-56

typeof NaN === "number" returns true from isMaskableValue, so access_token: NaN (pathological — JSON can't even serialize NaN) would render as the placeholder. The contract is "any non-null, non-empty-string value is masked," which is exactly the safe-by-default choice, so this is consistent. Calling it out as a confirmed edge, not a defect.

4. Form-encoded separator — maskSecrets.ts:122

maskFormBody splits on & only. RFC 3986 / WHATWG URLSearchParams use & exclusively; the legacy ; separator from RFC 1866 is dead in OAuth. Mentioning so a future "let me add ; support" thought has prior art: don't, OAuth clients don't emit it.

Re-verification of the security claim

Re-traced saveSession callers across core/ and clients/web/. Still only two flush points, both pre-redirect:

• inspectorClient.ts:241 — this.saveSession() from onBeforeOAuthRedirect, before window.location.href assignment.
• App.tsx:500-510 — synchronous BrowserNavigation.beforeNavigate flush, before navigation commits.

fetchRequestLogState.ts:116-123 SECURITY TRIPWIRE comment captures the invariant where future-you will encounter it when adding a new saveSession trigger. The masking-is-UI-only design holds.

Mantine compliance

Spot-checked against AGENTS.md:

• RevealButton uses .withProps() (NetworkEntry.tsx:130-133).
• BodyPreview uses Stack/Group/Text/Button only — no Box, no inline styles, no raw hex.
• aria-label on the toggle and aria-live="polite" on the status text are still in place.

Compliant.

Verdict

LGTM. The PR has been through five substantive review rounds, each addressed with care, and the implementation reflects that. Items 1-4 above are nits/observations; none warrant another round. The registration_access_token masking added in this round is a real defensive improvement worth highlighting.

• • Branch: v2/auth-response-bodies

[cliffhall]

Thanks for the sixth pass. All four are ignorable nits as you noted; took the one zero-risk doc improvement in 06fcae0.

2. Body-update emits list event only — Added a comment on onFetchRequestBodyUpdate documenting that body fill-in re-emits fetchRequestsChange only (not the per-entry fetchRequest, which fires once on append); list-reading consumers (useFetchRequests) pick the body up on the next render, and a future per-entry subscriber would need its own event. Prevents the regression you flagged.

Left as-is, intentionally:

• 1. Full-string BodyPreview key — keeping it; it's exact for the remount-on-change goal, and a length+prefix hash trades that exactness for a theoretical collision (same length + same first 64 chars, different suffix). Per your note, only worth revisiting if profiling ever flags large Network logs.
• 3. NaN under a sensitive key — consistent with the documented contract (any non-null, non-empty-string value is masked wholesale); pathological and JSON can't serialize it anyway.
• 4. &-only form separator — deliberate; OAuth clients don't emit the legacy ; separator.

validate (1882) / test:storybook (322) green. Appreciate the six rounds — the cumulative result (form-request masking + registration_access_token) is meaningfully tighter than where it started.

[cliffhall]

@claude review

[claude]

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

---

Claude Code Review (seventh pass)

Reviewed against origin/v2/main. The only change since the sixth pass (06fcae0) is the two doc comments — the body-update list-event clarification (fetchRequestLogState.ts:95-99) and the SECURITY TRIPWIRE block (:121-128). Both are in place and worded clearly enough that a future contributor adding a per-entry body subscriber or a new saveSession trigger will hit the right tripwire.

• Gather PR context (diff vs origin/v2/main)
• Read changed files
• Verify sixth-pass items (body-update list-event comment)
• Analyze for fresh correctness, security, test gaps
• Post review

Re-verifications

• Sixth-pass comment placements — onFetchRequestBodyUpdate doc lands directly on the dispatch site; the SECURITY TRIPWIRE block lands on the onSaveSession listener (the only path that writes fetchRequests to disk). Both are positioned where a future change would force a reader to acknowledge the invariant.
• saveSession callers — Re-traced: only inspectorClient.ts:241 (pre-redirect in onBeforeOAuthRedirect) and App.tsx's synchronous BrowserNavigation.beforeNavigate flush. No post-token-exchange flush path exists. The masking-is-UI-only design holds.
• Mantine compliance — RevealButton uses .withProps(); BodyPreview uses Stack/Group/Text/Button only; aria-label on the toggle and aria-live="polite" on the status text remain in place.

Fresh observations (all non-blocking; ignorable)

1. fetchRequestBodyUpdate silently no-ops when the entry has rotated out — fetchRequestLogState.ts:88-101

const idx = this.fetchRequests.findIndex((e) => e.id === id);
if (idx === -1) return;

If the matching entry has been evicted by maxFetchRequests rotation (default 1000) between fetchRequest and the deferred fetchRequestBodyUpdate, the body is silently dropped — no log, no warning. For auth traffic this is irrelevant (sparse). For a chatty transport log where a slow body read could be displaced by 1000 newer requests, the user would just see (empty) in the response body section with no way to tell whether the body never came back vs. was dropped. Not a defect, not in the PR's scope, but the seventh round is the right time to surface it since this is the diff that introduced the auth body update path. Worth a logger.debug at the early-return site if/when a logger is plumbed into the state managers.

2. Pretty-printed JSON can push a near-cap body over the cap downstream — NetworkEntry.tsx:149 + maskSecrets.ts:112

tooLarge is checked against the raw body.length. For a body just under 100k chars, masked JSON is pretty-printed (whitespace added) and could be ~30% larger when rendered. The cap is "what gets through the JSON parse + walk + render" — it's measured at the input, not the output. ContentViewer presumably handles 130K fine, so this is mostly academic; the cap is doing its job (keeping the DOM under control), and a future tightening of the cap should remember to measure post-masking.

3. Storybook AuthSuccess for…of reveal loop is correct, not parallel — NetworkEntry.stories.tsx:125-127

Confirming for the record: serial await userEvent.click(button) is the right shape — userEvent simulates input events that need to settle before the next click, and Promise.all would interleave them. Calling out only because future contributors looking at "two clicks in a loop, looks like a parallelizable thing" may be tempted to "optimize" it.

Verdict

LGTM. After seven passes the PR has converged: the implementation is correct, the security guarantee has a tripwire where it needs one, the test set covers the meaningful branches (JSON masking, form masking, content-type branching, repeated/empty params, scalar-JSON pass-through, DCR management token, NetworkEntry reveal/hide for both body sides, Storybook play function), and the documentation reflects the design. Item 1 is the only fresh observation worth a follow-up issue if you want one; items 2-3 are FYI.
• Branch: v2/auth-response-bodies

[cliffhall]

Thanks — agreed it's converged after seven passes. No code change this round; the three items are FYI/out-of-scope:

1. Body silently dropped when the entry rotates out before fetchRequestBodyUpdate — Real but not actionable here: the fix you suggest (logger.debug at the idx === -1 early return) needs a logger plumbed into the state managers, which they don't have today. Filed as a follow-up to track both the drop and the logger dependency: #1390 (on the v2 board, Backlog). Out of scope for this PR as you noted.

2. Pretty-printing can push a near-cap body over the cap downstream — Noted; the cap is intentionally measured at the raw input (its job is bounding the parse/walk/DOM), and a future cap tightening should measure post-masking.

3. for…of reveal loop is serial on purpose — Confirmed; userEvent clicks must settle sequentially, so it's not a Promise.all candidate. (Worth the call-out for future contributors.)

Nothing further to change on my end — ready to merge whenever you are.