refactor(oauth): wire one redaction owner into runtime execution - #3884
Conversation
GitHub Actions
Inactive
dosubot
Bot
added
the
size:XXL
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3517cf23-4755-497a-b438-9cbe795f5f50) |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-3884.up.railway.app |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5340c5512e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .replace( | ||
| new RegExp(`\\b(${CREDENTIAL_PARAM_NAMES})=[^&\\s"'<>]+`, "gi"), | ||
| "$1=[redacted]", | ||
| ) |
There was a problem hiding this comment.
Restore redaction for free-form forms such as access_token: secret, client_secret = secret, and clientSecret: secret. The previous SDK and client sanitizers accepted both : and = (including quoted values), but this replacement only handles an adjacent = or double-quoted JSON. If an authorization server echoes one of the common colon-delimited forms in an error description, the raw credential now survives into sanitized OAuth snapshots, copied error details, and hosted error-boundary telemetry.
Useful? React with 👍 / 👎.
| .replace( | ||
| /\b(bearer|basic)\s+(?:[\w-]*[._~+/=][\w\-._~+/=]*|\w{20,})/gi, | ||
| "$1 [redacted]", |
There was a problem hiding this comment.
Do not require a bare Bearer or Basic value to contain punctuation or be at least 20 characters. Both schemes permit shorter opaque/base64 values—for example, Basic dXNlcjpwYXNz is a valid credential shape—and the previous client sanitizer redacted these unconditionally. When such a value is echoed without the literal Authorization: prefix, this regex leaves it intact in the trace or OAuth debugger telemetry.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/src/conformance-reporting.ts">
<violation number="1" location="sdk/src/conformance-reporting.ts:574">
P1: OAuth JSON/JUnit reports leak the generated CSRF `state` from authorization request URLs because `redactForTelemetry` does not redact `state`; extend this redaction boundary to scrub `state` query values before rendering.</violation>
</file>
<file name="mcpjam-inspector/client/src/App.tsx">
<violation number="1" location="mcpjam-inspector/client/src/App.tsx:368">
P1: OAuth boundary errors can expose a client secret in the fallback, copied details, and `oauth_debugger_error_boundary` analytics when the error uses camelCase or colon-delimited credentials. Extending `sanitizeTraceErrorMessage` and its tests to preserve the old `clientSecret`/`:` matches would keep this redaction-owner migration from weakening coverage.</violation>
</file>
<file name="mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts">
<violation number="1" location="mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts:83">
P3: The full recursive source scan + regex read is computed twice: the first test ('offenders') and the 'has no stale allowlist entries' test ('live') each walk all 1300+ files under client/src and readFileSync them independently. Compute the matched-file set once (e.g. a module-level helper or beforeAll) and reuse it across both assertions, so adding a redaction identifier doesn't double the CI cost and the two lists can't drift.</violation>
</file>
<file name="sdk/src/oauth/state-machines/trace-redaction.ts">
<violation number="1" location="sdk/src/oauth/state-machines/trace-redaction.ts:328">
P1: Sanitized traces still publish URL userinfo credentials because `sanitizeOAuthUrl` never removes the URL username/password; clearing both fields before serialization preserves the host/path while preventing this leak.</violation>
<violation number="2" location="sdk/src/oauth/state-machines/trace-redaction.ts:439">
P2: State diagnostics misclassify an explicitly returned empty `state` as absent; treating any string, including `""`, as present reports the callback accurately and still yields a mismatch when an issued non-empty state exists.</violation>
</file>
<file name="mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts">
<violation number="1" location="mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts:2437">
P3: The updated comment on this factory is now inaccurate: it says there are only two flow entry points and "both call sites," but `createMCPOAuthProvider` is still called from three places — `initiateOAuth` (line ~2626), `handleOAuthCallback` (line ~3424), and `refreshOAuthTokens` (line ~3953), which was removed from the comment but still constructs the provider. A future contributor adding a constructor argument will trust the comment and may miss the `refreshOAuthTokens` call site. Please either keep the original three-entry-point wording or drop the call-site count so the comment can't mislead.</violation>
</file>
<file name="sdk/tests/xaa/diagnostic-redaction-keys.test.ts">
<violation number="1" location="sdk/tests/xaa/diagnostic-redaction-keys.test.ts:27">
P3: The test's `CONSUMED_CREDENTIAL_FIELDS` comment says "Fields the XAA flow CONSUMES" but three of the entries — `refreshToken`, `codeVerifier`, and `authorizationCode` — are OAuth state-machine fields rather than `XAAFlowState` fields. The XAA flow never reads them (verified: no references exist in the XAA source). Consider either removing the three fields that don't belong to the XAA state, or updating the docstring to clarify they are OAuth protocol fields included as a cross-cutting safety check.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| report: ConformanceReport, | ||
| ): ConformanceReport { | ||
| return redactSensitiveValue(report) as ConformanceReport; | ||
| return redactForTelemetry(report) as ConformanceReport; |
There was a problem hiding this comment.
P1: OAuth JSON/JUnit reports leak the generated CSRF state from authorization request URLs because redactForTelemetry does not redact state; extend this redaction boundary to scrub state query values before rendering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/src/conformance-reporting.ts, line 574:
<comment>OAuth JSON/JUnit reports leak the generated CSRF `state` from authorization request URLs because `redactForTelemetry` does not redact `state`; extend this redaction boundary to scrub `state` query values before rendering.</comment>
<file context>
@@ -571,7 +571,7 @@ export function toConformanceReport(
report: ConformanceReport,
): ConformanceReport {
- return redactSensitiveValue(report) as ConformanceReport;
+ return redactForTelemetry(report) as ConformanceReport;
}
</file context>
| message: sanitizeOAuthDebuggerText(error?.message ?? "Unknown error"), | ||
| stack: sanitizeOAuthDebuggerText(error?.stack), | ||
| name: sanitizeTraceErrorMessage(error?.name ?? "Error"), | ||
| message: sanitizeTraceErrorMessage(error?.message ?? "Unknown error"), |
There was a problem hiding this comment.
P1: OAuth boundary errors can expose a client secret in the fallback, copied details, and oauth_debugger_error_boundary analytics when the error uses camelCase or colon-delimited credentials. Extending sanitizeTraceErrorMessage and its tests to preserve the old clientSecret/: matches would keep this redaction-owner migration from weakening coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/App.tsx, line 368:
<comment>OAuth boundary errors can expose a client secret in the fallback, copied details, and `oauth_debugger_error_boundary` analytics when the error uses camelCase or colon-delimited credentials. Extending `sanitizeTraceErrorMessage` and its tests to preserve the old `clientSecret`/`:` matches would keep this redaction-owner migration from weakening coverage.</comment>
<file context>
@@ -351,38 +352,28 @@ function clearHostedCallbackRetryState() {
- message: sanitizeOAuthDebuggerText(error?.message ?? "Unknown error"),
- stack: sanitizeOAuthDebuggerText(error?.stack),
+ name: sanitizeTraceErrorMessage(error?.name ?? "Error"),
+ message: sanitizeTraceErrorMessage(error?.message ?? "Unknown error"),
+ stack: stack
+ ? stack.split("\n").map(sanitizeTraceErrorMessage).join("\n")
</file context>
|
|
||
| export function sanitizeOAuthUrl(rawUrl: string): string { | ||
| try { | ||
| const url = new URL(rawUrl); |
There was a problem hiding this comment.
P1: Sanitized traces still publish URL userinfo credentials because sanitizeOAuthUrl never removes the URL username/password; clearing both fields before serialization preserves the host/path while preventing this leak.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/src/oauth/state-machines/trace-redaction.ts, line 328:
<comment>Sanitized traces still publish URL userinfo credentials because `sanitizeOAuthUrl` never removes the URL username/password; clearing both fields before serialization preserves the host/path while preventing this leak.</comment>
<file context>
@@ -0,0 +1,449 @@
+
+export function sanitizeOAuthUrl(rawUrl: string): string {
+ try {
+ const url = new URL(rawUrl);
+ for (const key of [...url.searchParams.keys()]) {
+ if (isSensitiveQueryParamName(key)) {
</file context>
| const url = new URL(rawUrl); | |
| const url = new URL(rawUrl); | |
| url.username = ""; | |
| url.password = ""; |
| callbackState?: string | null; | ||
| }): OAuthStateMatchDiagnostics { | ||
| const statePresent = | ||
| typeof input.callbackState === "string" && input.callbackState.length > 0; |
There was a problem hiding this comment.
P2: State diagnostics misclassify an explicitly returned empty state as absent; treating any string, including "", as present reports the callback accurately and still yields a mismatch when an issued non-empty state exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/src/oauth/state-machines/trace-redaction.ts, line 439:
<comment>State diagnostics misclassify an explicitly returned empty `state` as absent; treating any string, including `""`, as present reports the callback accurately and still yields a mismatch when an issued non-empty state exists.</comment>
<file context>
@@ -0,0 +1,449 @@
+ callbackState?: string | null;
+}): OAuthStateMatchDiagnostics {
+ const statePresent =
+ typeof input.callbackState === "string" && input.callbackState.length > 0;
+
+ if (!input.issuedState) {
</file context>
| typeof input.callbackState === "string" && input.callbackState.length > 0; | |
| typeof input.callbackState === "string"; |
| }); | ||
|
|
||
| it("has no stale allowlist entries", () => { | ||
| const live = new Set( |
There was a problem hiding this comment.
P3: The full recursive source scan + regex read is computed twice: the first test ('offenders') and the 'has no stale allowlist entries' test ('live') each walk all 1300+ files under client/src and readFileSync them independently. Compute the matched-file set once (e.g. a module-level helper or beforeAll) and reuse it across both assertions, so adding a redaction identifier doesn't double the CI cost and the two lists can't drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts, line 83:
<comment>The full recursive source scan + regex read is computed twice: the first test ('offenders') and the 'has no stale allowlist entries' test ('live') each walk all 1300+ files under client/src and readFileSync them independently. Compute the matched-file set once (e.g. a module-level helper or beforeAll) and reuse it across both assertions, so adding a redaction identifier doesn't double the CI cost and the two lists can't drift.</comment>
<file context>
@@ -0,0 +1,93 @@
+ });
+
+ it("has no stale allowlist entries", () => {
+ const live = new Set(
+ sourceFiles(CLIENT_SRC)
+ .filter((file) =>
</file context>
| * Both OAuth flow entry points (`initiateOAuth`, `handleOAuthCallback`) need an | ||
| * identical instance shape; this factory keeps that wiring in one place so | ||
| * adding a constructor argument doesn't require touching both call sites. |
There was a problem hiding this comment.
P3: The updated comment on this factory is now inaccurate: it says there are only two flow entry points and "both call sites," but createMCPOAuthProvider is still called from three places — initiateOAuth (line ~2626), handleOAuthCallback (line ~3424), and refreshOAuthTokens (line ~3953), which was removed from the comment but still constructs the provider. A future contributor adding a constructor argument will trust the comment and may miss the refreshOAuthTokens call site. Please either keep the original three-entry-point wording or drop the call-site count so the comment can't mislead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts, line 2437:
<comment>The updated comment on this factory is now inaccurate: it says there are only two flow entry points and "both call sites," but `createMCPOAuthProvider` is still called from three places — `initiateOAuth` (line ~2626), `handleOAuthCallback` (line ~3424), and `refreshOAuthTokens` (line ~3953), which was removed from the comment but still constructs the provider. A future contributor adding a constructor argument will trust the comment and may miss the `refreshOAuthTokens` call site. Please either keep the original three-entry-point wording or drop the call-site count so the comment can't mislead.</comment>
<file context>
@@ -2670,10 +2434,9 @@ function buildConvexBindingForServer(input: {
- * `refreshOAuthTokens`) all need an identical instance shape; this factory
- * keeps that wiring in one place so adding a constructor argument doesn't
- * require touching three call sites.
+ * Both OAuth flow entry points (`initiateOAuth`, `handleOAuthCallback`) need an
+ * identical instance shape; this factory keeps that wiring in one place so
+ * adding a constructor argument doesn't require touching both call sites.
</file context>
| * Both OAuth flow entry points (`initiateOAuth`, `handleOAuthCallback`) need an | |
| * identical instance shape; this factory keeps that wiring in one place so | |
| * adding a constructor argument doesn't require touching both call sites. | |
| * The three OAuth flow entry points (`initiateOAuth`, `handleOAuthCallback`, | |
| * `refreshOAuthTokens`) all construct a provider; this factory keeps that wiring | |
| * in one place so adding a constructor argument doesn't require touching three | |
| * call sites. |
| "identityAssertion", | ||
| "idJag", | ||
| "accessToken", | ||
| "refreshToken", |
There was a problem hiding this comment.
P3: The test's CONSUMED_CREDENTIAL_FIELDS comment says "Fields the XAA flow CONSUMES" but three of the entries — refreshToken, codeVerifier, and authorizationCode — are OAuth state-machine fields rather than XAAFlowState fields. The XAA flow never reads them (verified: no references exist in the XAA source). Consider either removing the three fields that don't belong to the XAA state, or updating the docstring to clarify they are OAuth protocol fields included as a cross-cutting safety check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/tests/xaa/diagnostic-redaction-keys.test.ts, line 27:
<comment>The test's `CONSUMED_CREDENTIAL_FIELDS` comment says "Fields the XAA flow CONSUMES" but three of the entries — `refreshToken`, `codeVerifier`, and `authorizationCode` — are OAuth state-machine fields rather than `XAAFlowState` fields. The XAA flow never reads them (verified: no references exist in the XAA source). Consider either removing the three fields that don't belong to the XAA state, or updating the docstring to clarify they are OAuth protocol fields included as a cross-cutting safety check.</comment>
<file context>
@@ -0,0 +1,52 @@
+ "identityAssertion",
+ "idJag",
+ "accessToken",
+ "refreshToken",
+ "codeVerifier",
+ "clientId",
</file context>
5340c55 to
9afd2fe
Compare
GitHub Actions
Inactive
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_68926f04-46b8-4e1b-9905-10dd1851c461) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9afd2fec2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .replace( | ||
| new RegExp( | ||
| `\\b((?:[\\w-]*[_-])?(?:${UNAMBIGUOUS_CREDENTIAL_NAMES}))(\\s*[:=]\\s*)("(?:\\\\.|[^"\\\\])*"|[^&\\s"'<>,;]+)`, | ||
| "gi", |
There was a problem hiding this comment.
When an upstream error includes a Python/Ruby-style mapping such as {'access_token': 'SUPERSECRET'}, this rule skips the quoted key and rule 4 only accepts double-quoted JSON, so sanitizeTraceErrorMessage returns the raw credential to hosted persisted traces and OAuth error-boundary telemetry. Unlike the previously flagged bare-colon case, the new rule still leaves this quoted-key form uncovered; accept quoted keys and single-quoted values or extend the JSON-like matcher accordingly.
Useful? React with 👍 / 👎.
| .replace(/\b(bearer|basic)(\s+)(\S+)/gi, (match, scheme, gap, value) => { | ||
| // Trailing sentence punctuation belongs to the prose, not the value. | ||
| const core = (value as string).replace(/[.,;:!?)\]}]+$/, ""); | ||
| return /^[a-z]{1,20}$/.test(core) ? match : `${scheme}${gap}[redacted]`; |
There was a problem hiding this comment.
When an authorization server capitalizes the wording, for example BEARER TOKEN is expired, the outer regex matches case-insensitively but the callback's ^[a-z]{1,20}$ test is case-sensitive, so TOKEN is misclassified as a credential and the useful diagnostic becomes BEARER [redacted] is expired. Normalize core before the vocabulary check or make that check case-insensitive.
Useful? React with 👍 / 👎.
f2efcf0 to
13c3378
Compare
dosubot
Bot
added
size:XL
Redaction policy lived in six places and had drifted: the SDK's sensitive-field set omitted `state` while the client's included it, and the SDK's error redactor mangled "Bearer token is expired". One source of truth now lives in the SDK (oauth/state-machines/trace-redaction.ts); the client re-exports it and adds only the SANITIZE_OAUTH_TRACES gate. Three leaks closed along the way: - `state` is now redacted as a field, in URL queries, in bodies, and in error strings; `describeOAuthStateMatch` reports presence/match instead of the nonce - sanitizeHttpHistoryEntry did not redact `request.url`, so recorded authorization requests kept their `state` - URLSearchParams never fails, so prose containing `=` was reshaped into a field whose key is not in the sensitive set, losing redaction Telemetry redaction is renamed `redactForTelemetry` and stays separate; `redactSensitiveValue` remains as a deprecated alias. A ratchet test keeps redaction identifiers inside the trace modules. Adds the real executor -> real state machine integration test against the fake OAuth MCP fixture, mocking only `@/lib/config` and `authFetch`, with named assertions for the MCP OAuth wire invariants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVEJChsUHNKf16PJ2kkbpf
One exhaustive test asks the whole question at once: a sanitized projection carrying an access token, refresh token, ID token, client secret, authorization code, PKCE verifier, cookie, and OAuth state — across request URLs, headers, bodies, transport errors, info logs, and the flow error — contains none of them, still says enough to debug with, and stays fully raw when sanitize is off. Also fences XAA's redact-into-live-state instance: extracts REDACTED_DIAGNOSTIC_KEYS, documents why it is inert (every key is a display surface; the one value read back is a number), and pins the list so widening it to a credential field cannot be a one-word edit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVEJChsUHNKf16PJ2kkbpf
Consolidating the two error-string redactors onto the client's implementation kept the client's coverage but silently dropped two cases the SDK redactor in `trace.ts` had. Both are reachable from `error_description`, which is prose chosen by the server under test. Colon-delimited fields. The SDK matched `(\s*[:=]\s*)`; the client — and so the merged module — matched `=` only, so `access_token: SECRET` survived into persisted traces and copied error details. Restored as its own rule over the names that can only ever be a credential. It deliberately does NOT reuse the full `CREDENTIAL_PARAM_NAMES` list, because `code`, `token` and `state` are ordinary English after a colon and "status code: 401" has to stay readable — the same split the old SDK rule drew. `[-_]?` in the names picks up the camelCase spellings (`clientSecret:`) a JSON API actually emits. Short bearer/basic values. The SDK redacted `Bearer <anything>`; the merged rule requires base64url punctuation or 20+ characters, which misses valid short credentials such as `Basic dXNlcjpwYXNz`. The length/punctuation test existed to protect "Bearer token is expired" from a naive `\w+` rule, so the test is now inverted rather than removed: redact unless the value is a plain lowercase word. Mixed case, a digit or punctuation all read as credential shape, and the diagnostic vocabulary the module promises to keep is still pinned by its existing cases. Also re-adds the two trace-redaction integration tests from the metadata PR below this one. They assert the unified `matchesSensitiveName` policy that this PR introduces — a nested `error_description` body key, and `state` as a sensitive name — so this is where they pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0145V8yfgirroxFKNaZNQ5dS
9afd2fe to
db64ab9
Compare
dosubot
Bot
added
size:XXL
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d97fa2aa-84f2-4ff7-9f36-5453246d34a0) |
WalkthroughThe SDK adds centralized OAuth trace sanitization for fields, headers, URLs, bodies, errors, and callback state diagnostics. The inspector uses these helpers for OAuth history, debugger errors, telemetry, and integration tests. Telemetry redaction is renamed to Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTwo redaction tests can pass without a trace. Both assert only the absence of a secret in
JSON.stringify(oauthTrace ?? {}). WhenoauthTraceisundefined, the serialized value is"{}", the assertion succeeds, and the redactor is never exercised. A regression that drops or empties the trace turns both tests into no-ops. Give each negative assertion a positive anchor.
mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts#L334-345: assertcallback.oauthTraceis defined and that itshttpHistoryorstepsrecords the failed token exchange, then assert the credential is absent.mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts#L440-450: assertflow.callback.oauthTraceis defined and non-empty, and assert the state is represented as a match — the comment on line 440 promises exactly that positive half, which the body does not yet check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts` at line 1, Strengthen the two redaction tests in the OAuth refresh integration suite by adding positive trace assertions before checking secret absence. In the first test, require callback.oauthTrace and verify its httpHistory or steps records the failed token exchange; in the second, require flow.callback.oauthTrace to be defined and non-empty and verify the state is represented as a match, then retain the credential-redaction assertions.
🧹 Nitpick comments (7)
sdk/tests/worker-entry.test.ts (1)
6-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve compatibility coverage for both entrypoints.
Both changes replace the deprecated-alias assertion with the new-name assertion. Add assertions for both exports.
sdk/tests/worker-entry.test.ts#L6-L6: asserttypeof worker.redactSensitiveValueis"function"alongsideworker.redactForTelemetry.sdk/tests/browser-entry.test.ts#L75-L75: asserttypeof browser.redactSensitiveValueis"function"alongsidebrowser.redactForTelemetry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/tests/worker-entry.test.ts` at line 6, Preserve compatibility coverage by keeping the existing worker.redactForTelemetry assertion and adding a worker.redactSensitiveValue function assertion in sdk/tests/worker-entry.test.ts:6-6. Likewise, keep browser.redactForTelemetry and add a browser.redactSensitiveValue function assertion in sdk/tests/browser-entry.test.ts:75-75.sdk/tests/oauth/sanitized-trace-contains-no-secrets.test.ts (1)
18-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the sweep with a vendor-prefixed credential key.
The fixture exercises canonical names only. A body field such as
user_access_tokenpasses this suite today and still leaks through the structured path, as described in the comment onsdk/src/oauth/state-machines/trace-redaction.tsLines 78-99. Add the case so the sweep pins the widened policy.🧪 Proposed fixture addition
cookie: "session=LEAKED_COOKIE_VALUE_0000000007", state: "st_LEAKED_CSRF_STATE_00000000000008", + vendorToken: "vt_LEAKED_VENDOR_ACCESS_TOKEN_009", };body: { access_token: SECRETS.accessToken, refresh_token: SECRETS.refreshToken, id_token: SECRETS.idToken, + user_access_token: SECRETS.vendorToken, error_description: `rejected access_token=${SECRETS.accessToken}`, },Also applies to: 63-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/tests/oauth/sanitized-trace-contains-no-secrets.test.ts` around lines 18 - 27, Add a vendor-prefixed credential entry to the SECRETS fixture, such as a user_access_token body field, and include it in the sanitized-trace sweep assertions covering the structured path. Ensure the test verifies this widened credential-key policy alongside the existing canonical OAuth secret cases.mcpjam-inspector/client/src/App.tsx (2)
1832-1832: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fallback sanitizes the whole stack to render one line.
redactOAuthDebuggerErrorwalks and sanitizes every stack line, and this call site reads only.message. The fallback renders rarely, so the cost is immaterial; the clarity is not. Call the sanitizer for the message alone.♻️ Proposed change
- {redactOAuthDebuggerError(error).message} + {sanitizeTraceErrorMessage(error?.message ?? "Unknown error")}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/App.tsx` at line 1832, Update the fallback rendering near redactOAuthDebuggerError so it sanitizes only the error message rather than the entire error object and stack. Preserve the existing displayed message while avoiding the full stack-processing path.
359-377: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
redactOAuthDebuggerErrorhas no test.This function decides what a crash report sends to PostHog. It merits direct coverage: a null
error, anErrorwith nostack, an empty-string stack, a multi-line stack whose lines each carry a token, and a name or message containing a credential. The guideline asks for it, and the blast radius of a silent regression here is a leaked secret in telemetry.As per coding guidelines: "All changes should include tests, covering happy paths, validation errors, error handling, and edge cases such as null and empty values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/App.tsx` around lines 359 - 377, Add direct tests for redactOAuthDebuggerError covering null errors, missing and empty stacks, multiline stacks with token-bearing lines, and credentials in the error name or message; assert each returned field is sanitized and that the expected empty/default values are preserved.Source: Coding guidelines
mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts (1)
59-64: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBoth tests walk and read the whole client tree.
sourceFiles(CLIENT_SRC)recurses over every.ts/.tsxfile underclient/srcandreadFileSyncreads each one — twice, once per test. Compute the matching set once at module scope and let both tests read it.♻️ Proposed refactor
+const MATCHING_FILES = sourceFiles(CLIENT_SRC) + .filter((file) => REDACTION_IDENTIFIER_PATTERN.test(readFileSync(file, "utf8"))) + .map((file) => relative(CLIENT_SRC, file).split(sep).join("/")); + describe("OAuth redaction ratchet", () => { it("keeps redaction identifiers inside the trace modules", () => { - const offenders = sourceFiles(CLIENT_SRC) - .filter((file) => - REDACTION_IDENTIFIER_PATTERN.test(readFileSync(file, "utf8")), - ) - .map((file) => relative(CLIENT_SRC, file).split(sep).join("/")) - .filter((file) => !ALLOWED_FILES.has(file)); - - expect(offenders).toEqual([]); + expect(MATCHING_FILES.filter((file) => !ALLOWED_FILES.has(file))).toEqual([]); });and correspondingly:
it("has no stale allowlist entries", () => { - const live = new Set( - sourceFiles(CLIENT_SRC) - .filter((file) => - REDACTION_IDENTIFIER_PATTERN.test(readFileSync(file, "utf8")), - ) - .map((file) => relative(CLIENT_SRC, file).split(sep).join("/")), - ); - + const live = new Set(MATCHING_FILES); expect([...ALLOWED_FILES].filter((file) => !live.has(file))).toEqual([]); });Note that
sourceFilesmust be declared before the module-scope call, which it is.Also applies to: 82-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts` around lines 59 - 64, Compute the redaction-matching client file set once at module scope after the `sourceFiles` helper is declared, then reuse that shared result in both tests instead of recursively walking and reading `CLIENT_SRC` separately. Preserve the existing relative-path normalization and `ALLOWED_FILES` filtering behavior.mcpjam-inspector/client/src/lib/oauth/trace-redaction.ts (1)
49-77: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd unit tests for both branches of the gate.
These four helpers are the entire contribution of this module; the policy itself is borrowed from the SDK. Their one behaviour worth proving is the branch:
SANITIZE_OAUTH_TRACES === truemust redact, andfalsemust return the value untouched. No supplied test toggles that flag. The integration test exercises only one side, and the ratchet test inspects text, not behaviour.Please cover, with
SANITIZE_OAUTH_TRACESmocked in both states:
traceOAuthUrl,traceOAuthValue,traceOAuthErrorMessage— redacted versus identical output.traceOAuthHeaders— the local branch returns a copy, not the same reference.- Empty and null-ish inputs:
traceOAuthHeaders({}),traceOAuthValue(undefined),traceOAuthValue(null),traceOAuthUrl("").As per coding guidelines: "All changes should include tests, covering happy paths, validation errors, error handling, and edge cases such as null and empty values."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/oauth/trace-redaction.ts` around lines 49 - 77, Add unit tests for traceOAuthUrl, traceOAuthHeaders, traceOAuthValue, and traceOAuthErrorMessage with SANITIZE_OAUTH_TRACES mocked to both true and false, asserting redacted output versus unchanged output. Cover empty and null-ish inputs, including traceOAuthHeaders({}), traceOAuthValue(undefined), traceOAuthValue(null), and traceOAuthUrl(""), and verify the disabled traceOAuthHeaders branch returns an equivalent copy with a different reference.Source: Coding guidelines
sdk/src/browser.ts (1)
29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose
redactSensitiveValueas a documented local alias. TypeScript 5.9 does not expose the@deprecatedtag from this re-export to consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/browser.ts` around lines 29 - 40, Update the browser entrypoint exports so redactSensitiveValue is declared as a local documented alias rather than only a renamed re-export, preserving its `@deprecated` annotation for TypeScript consumers while continuing to delegate to redactForTelemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts`:
- Around line 32-41: Expand REDACTION_IDENTIFIER_PATTERN to match
sanitizeTraceErrorMessage, sanitizeStepError, and redactSensitiveTraceValue,
then add App.tsx to ALLOWED_FILES as an explicit approved consumer. Preserve the
existing trace-module allowlist and ensure the ratchet flags any other files
using these redaction identifiers.
In
`@mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts`:
- Around line 334-345: Strengthen both OAuth trace redaction tests around the
callback.oauthTrace assertions: require the trace to exist and verify it
contains the failed token-exchange step before checking that the echoed
credential is absent. Apply this to the test at “keeps a credential echoed in
error_description out of the published trace” and the corresponding test near
the second site, preserving the existing failure and redaction assertions.
In `@sdk/src/oauth/state-machines/trace-redaction.ts`:
- Around line 362-377: Update sanitizeOAuthUrl to clear url.username and
url.password before calling url.toString(), ensuring embedded credentials are
removed from sanitized request URLs. Use a literal redaction value without
brackets if assigning a replacement, since URL serialization percent-encodes
bracket characters; preserve the existing query, fragment, and fallback
sanitization behavior.
- Around line 78-99: Update isSensitiveTraceFieldName to reuse the same
credential-name heuristics as isSensitiveHeaderName and
isSensitiveQueryParamName, including token, secret, password, credential,
cookie, auth, and api_key patterns after normalization. Preserve exact
sensitive-name checks and ensure vendor-prefixed or reshaped keys such as
user_access_token and rejected:access_token are detected; add a narrow allowlist
only if needed to preserve non-secret token_type diagnostics.
---
Outside diff comments:
In
`@mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts`:
- Line 1: Strengthen the two redaction tests in the OAuth refresh integration
suite by adding positive trace assertions before checking secret absence. In the
first test, require callback.oauthTrace and verify its httpHistory or steps
records the failed token exchange; in the second, require
flow.callback.oauthTrace to be defined and non-empty and verify the state is
represented as a match, then retain the credential-redaction assertions.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Line 1832: Update the fallback rendering near redactOAuthDebuggerError so it
sanitizes only the error message rather than the entire error object and stack.
Preserve the existing displayed message while avoiding the full stack-processing
path.
- Around line 359-377: Add direct tests for redactOAuthDebuggerError covering
null errors, missing and empty stacks, multiline stacks with token-bearing
lines, and credentials in the error name or message; assert each returned field
is sanitized and that the expected empty/default values are preserved.
In `@mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts`:
- Around line 59-64: Compute the redaction-matching client file set once at
module scope after the `sourceFiles` helper is declared, then reuse that shared
result in both tests instead of recursively walking and reading `CLIENT_SRC`
separately. Preserve the existing relative-path normalization and
`ALLOWED_FILES` filtering behavior.
In `@mcpjam-inspector/client/src/lib/oauth/trace-redaction.ts`:
- Around line 49-77: Add unit tests for traceOAuthUrl, traceOAuthHeaders,
traceOAuthValue, and traceOAuthErrorMessage with SANITIZE_OAUTH_TRACES mocked to
both true and false, asserting redacted output versus unchanged output. Cover
empty and null-ish inputs, including traceOAuthHeaders({}),
traceOAuthValue(undefined), traceOAuthValue(null), and traceOAuthUrl(""), and
verify the disabled traceOAuthHeaders branch returns an equivalent copy with a
different reference.
In `@sdk/src/browser.ts`:
- Around line 29-40: Update the browser entrypoint exports so
redactSensitiveValue is declared as a local documented alias rather than only a
renamed re-export, preserving its `@deprecated` annotation for TypeScript
consumers while continuing to delegate to redactForTelemetry.
In `@sdk/tests/oauth/sanitized-trace-contains-no-secrets.test.ts`:
- Around line 18-27: Add a vendor-prefixed credential entry to the SECRETS
fixture, such as a user_access_token body field, and include it in the
sanitized-trace sweep assertions covering the structured path. Ensure the test
verifies this widened credential-key policy alongside the existing canonical
OAuth secret cases.
In `@sdk/tests/worker-entry.test.ts`:
- Line 6: Preserve compatibility coverage by keeping the existing
worker.redactForTelemetry assertion and adding a worker.redactSensitiveValue
function assertion in sdk/tests/worker-entry.test.ts:6-6. Likewise, keep
browser.redactForTelemetry and add a browser.redactSensitiveValue function
assertion in sdk/tests/browser-entry.test.ts:75-75.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f7d860d-4787-4b17-a5ba-7589120ac30e
📒 Files selected for processing (39)
.changeset/oauth-executor-machine-integration.md.changeset/oauth-one-redaction-owner.md.changeset/oauth-secret-sweep-and-xaa-pin.mdcli/src/commands/server.tscli/src/commands/xaa.tscli/src/lib/credentials-file.tscli/src/lib/debug-artifact.tscli/src/lib/mcp-server.tscli/src/lib/oauth-output.tscli/src/lib/redaction.tscli/src/lib/rpc-logs.tscli/tests/server-doctor.test.tsmcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/oauth/trace-redaction.tsmcpjam-inspector/client/src/state/oauth-orchestrator.tssdk/src/browser.tssdk/src/conformance-reporting.tssdk/src/error-describer/describe.tssdk/src/index.tssdk/src/oauth/emulation/preflight.tssdk/src/oauth/state-machines/factory.tssdk/src/oauth/state-machines/trace-redaction.tssdk/src/oauth/state-machines/trace.tssdk/src/response-validation.tssdk/src/structured-reporting.tssdk/src/telemetry-redaction.tssdk/src/worker.tssdk/src/xaa/state-machines/state-machine.tssdk/tests/browser-entry.test.tssdk/tests/oauth/sanitized-trace-contains-no-secrets.test.tssdk/tests/oauth/trace-error-redaction.test.tssdk/tests/oauth/trace-state-policy.test.tssdk/tests/telemetry-redaction.test.tssdk/tests/worker-entry.test.tssdk/tests/xaa/diagnostic-redaction-keys.test.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/lib/oauth/tests/debug-state-machine-step-reporting.test.ts
| const REDACTION_IDENTIFIER_PATTERN = | ||
| /\b(sanitizeOAuth[A-Za-z]*|redactSensitiveValue[A-Za-z]*|traceOAuth[A-Za-z]*)\b/; | ||
|
|
||
| const ALLOWED_FILES = new Set([ | ||
| // The gate + re-export. The policy itself lives in the SDK. | ||
| "lib/oauth/trace-redaction.ts", | ||
| // Builds the trace entries; calls the gated helpers, defines none. | ||
| "lib/oauth/mcp-oauth.ts", | ||
| "lib/oauth/oauth-trace.ts", | ||
| ]); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The fence leaves the most-used redactor outside it.
REDACTION_IDENTIFIER_PATTERN catches sanitizeOAuth*, traceOAuth*, and redactSensitiveValue*. It does not catch three names this PR re-exports and uses:
sanitizeTraceErrorMessage— imported and applied inApp.tsxtoday.sanitizeStepError— the historical alias for the same function.redactSensitiveTraceValue—redactSensitiveValue[A-Za-z]*does not match it, because the literal prefix diverges atTrace.
The stated invariant is that redaction identifiers stay inside the trace modules. As written, a fourth private copy built on sanitizeTraceErrorMessage would pass this ratchet unnoticed — the very drift the module header describes.
Extending the pattern makes App.tsx an offender, which is the correct outcome: allowlist it deliberately, so the next such addition is a review decision rather than a silent one.
🛡️ Proposed fix
const REDACTION_IDENTIFIER_PATTERN =
- /\b(sanitizeOAuth[A-Za-z]*|redactSensitiveValue[A-Za-z]*|traceOAuth[A-Za-z]*)\b/;
+ /\b(sanitizeOAuth[A-Za-z]*|sanitizeTraceErrorMessage|sanitizeStepError|redactSensitive[A-Za-z]*|traceOAuth[A-Za-z]*)\b/;
const ALLOWED_FILES = new Set([
// The gate + re-export. The policy itself lives in the SDK.
"lib/oauth/trace-redaction.ts",
// Builds the trace entries; calls the gated helpers, defines none.
"lib/oauth/mcp-oauth.ts",
"lib/oauth/oauth-trace.ts",
+ // Redacts the OAuth debugger error boundary before it reaches telemetry.
+ "App.tsx",
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const REDACTION_IDENTIFIER_PATTERN = | |
| /\b(sanitizeOAuth[A-Za-z]*|redactSensitiveValue[A-Za-z]*|traceOAuth[A-Za-z]*)\b/; | |
| const ALLOWED_FILES = new Set([ | |
| // The gate + re-export. The policy itself lives in the SDK. | |
| "lib/oauth/trace-redaction.ts", | |
| // Builds the trace entries; calls the gated helpers, defines none. | |
| "lib/oauth/mcp-oauth.ts", | |
| "lib/oauth/oauth-trace.ts", | |
| ]); | |
| const REDACTION_IDENTIFIER_PATTERN = | |
| /\b(sanitizeOAuth[A-Za-z]*|sanitizeTraceErrorMessage|sanitizeStepError|redactSensitive[A-Za-z]*|traceOAuth[A-Za-z]*)\b/; | |
| const ALLOWED_FILES = new Set([ | |
| // The gate + re-export. The policy itself lives in the SDK. | |
| "lib/oauth/trace-redaction.ts", | |
| // Builds the trace entries; calls the gated helpers, defines none. | |
| "lib/oauth/mcp-oauth.ts", | |
| "lib/oauth/oauth-trace.ts", | |
| // Redacts the OAuth debugger error boundary before it reaches telemetry. | |
| "App.tsx", | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/lib/__tests__/oauth-redaction-ratchet.test.ts`
around lines 32 - 41, Expand REDACTION_IDENTIFIER_PATTERN to match
sanitizeTraceErrorMessage, sanitizeStepError, and redactSensitiveTraceValue,
then add App.tsx to ALLOWED_FILES as an explicit approved consumer. Preserve the
existing trace-module allowlist and ensure the ratchet flags any other files
using these redaction identifiers.
| it("keeps a credential echoed in error_description out of the published trace", async () => { | ||
| const echoed = FAKE_OAUTH_ACCESS_TOKEN; | ||
| const { callback } = await track( | ||
| runFullFlow("integration-token-failure", { | ||
| tokenFailure: { echoInErrorDescription: `access_token=${echoed}` }, | ||
| }), | ||
| ); | ||
|
|
||
| expect(callback.success).toBe(false); | ||
| const serialized = JSON.stringify(callback.oauthTrace ?? {}); | ||
| expect(serialized).not.toContain(echoed); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The negative assertion can pass on an empty trace.
If callback.oauthTrace is undefined, JSON.stringify({}) yields "{}", and not.toContain(echoed) succeeds without the redactor ever running. A future change that drops the trace entirely, or short-circuits before the token exchange is recorded, would leave this test green. Anchor it: assert the trace exists and contains the failed token step before asserting the absence of the credential.
The same shape appears at lines 444-450. See the consolidated comment for both sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@mcpjam-inspector/client/src/lib/oauth/__tests__/oauth-refresh-integration.test.ts`
around lines 334 - 345, Strengthen both OAuth trace redaction tests around the
callback.oauthTrace assertions: require the trace to exist and verify it
contains the failed token-exchange step before checking that the echoed
credential is absent. Apply this to the test at “keeps a credential echoed in
error_description out of the published trace” and the corresponding test near
the second site, preserving the existing failure and redaction assertions.
| export function isSensitiveTraceFieldName(key: string): boolean { | ||
| return OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalizeSensitiveKey(key)); | ||
| } | ||
|
|
||
| export function isSensitiveHeaderName(key: string): boolean { | ||
| const normalized = normalizeSensitiveKey(key); | ||
| return ( | ||
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | ||
| SENSITIVE_HEADER_PATTERNS.some((pattern) => pattern.test(key)) || | ||
| /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) || | ||
| /(^|_)api_?key(_|$)/.test(normalized) | ||
| ); | ||
| } | ||
|
|
||
| export function isSensitiveQueryParamName(key: string): boolean { | ||
| const normalized = normalizeSensitiveKey(key); | ||
| return ( | ||
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | ||
| /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) || | ||
| /(^|_)api_?key(_|$)/.test(normalized) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
isSensitiveTraceFieldName matches exact names only, so vendor-prefixed credential fields survive in structured traces.
isSensitiveHeaderName and isSensitiveQueryParamName apply the token|secret|password|credential|cookie|auth and api_key heuristics. isSensitiveTraceFieldName does not. The JSON error redactor is wider still: CREDENTIAL_JSON_FIELD_NAME (Line 148) accepts a vendor prefix, and sdk/tests/oauth/trace-error-redaction.test.ts Lines 65-72 pin user_access_token as a credential in free-form text.
Trace a body field through the structured path:
{ "user_access_token": "vendor-secret" } → sanitizeOAuthTraceValue → key not in OAUTH_TRACE_SENSITIVE_FIELD_NAMES → value is a plain string → sanitizeOAuthTraceString → no scheme, no =, no braces → sanitizeTraceErrorMessage("vendor-secret") → returned verbatim.
The same value is therefore redacted in an error string and published in a response body. nango_secret_key, x_api_key, and similar vendor spellings leak the same way.
The identical root cause reaches the reshaped-prose case: looksLikeRequestFields (Line 356) permits : in a key, so "rejected:access_token=<value>" becomes the field rejected:access_token, which the exact-match test also misses. Sharing the heuristics fixes both, because the normalized key ends in _token.
One caution on the fix: token_type then normalizes into the (^|_)token(_|$) prefix branch and its Bearer value is redacted. Consider a short non-secret allowlist if that diagnostic matters.
🔒️ Proposed fix: share one name policy across fields, headers, and query params
+const SENSITIVE_NAME_PATTERNS = [
+ /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/,
+ /(^|_)api_?key(_|$)/,
+];
+
+function matchesSensitiveNamePattern(normalized: string): boolean {
+ return SENSITIVE_NAME_PATTERNS.some((pattern) => pattern.test(normalized));
+}
+
export function isSensitiveTraceFieldName(key: string): boolean {
- return OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalizeSensitiveKey(key));
+ const normalized = normalizeSensitiveKey(key);
+ return (
+ OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) ||
+ matchesSensitiveNamePattern(normalized)
+ );
}
export function isSensitiveHeaderName(key: string): boolean {
const normalized = normalizeSensitiveKey(key);
return (
OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) ||
SENSITIVE_HEADER_PATTERNS.some((pattern) => pattern.test(key)) ||
- /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) ||
- /(^|_)api_?key(_|$)/.test(normalized)
+ matchesSensitiveNamePattern(normalized)
);
}
export function isSensitiveQueryParamName(key: string): boolean {
const normalized = normalizeSensitiveKey(key);
return (
OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) ||
- /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) ||
- /(^|_)api_?key(_|$)/.test(normalized)
+ matchesSensitiveNamePattern(normalized)
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isSensitiveTraceFieldName(key: string): boolean { | |
| return OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalizeSensitiveKey(key)); | |
| } | |
| export function isSensitiveHeaderName(key: string): boolean { | |
| const normalized = normalizeSensitiveKey(key); | |
| return ( | |
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | |
| SENSITIVE_HEADER_PATTERNS.some((pattern) => pattern.test(key)) || | |
| /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) || | |
| /(^|_)api_?key(_|$)/.test(normalized) | |
| ); | |
| } | |
| export function isSensitiveQueryParamName(key: string): boolean { | |
| const normalized = normalizeSensitiveKey(key); | |
| return ( | |
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | |
| /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/.test(normalized) || | |
| /(^|_)api_?key(_|$)/.test(normalized) | |
| ); | |
| } | |
| const SENSITIVE_NAME_PATTERNS = [ | |
| /(^|_)(token|secret|password|credential|cookie|auth)(_|$)/, | |
| /(^|_)api_?key(_|$)/, | |
| ]; | |
| function matchesSensitiveNamePattern(normalized: string): boolean { | |
| return SENSITIVE_NAME_PATTERNS.some((pattern) => pattern.test(normalized)); | |
| } | |
| export function isSensitiveTraceFieldName(key: string): boolean { | |
| const normalized = normalizeSensitiveKey(key); | |
| return ( | |
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | |
| matchesSensitiveNamePattern(normalized) | |
| ); | |
| } | |
| export function isSensitiveHeaderName(key: string): boolean { | |
| const normalized = normalizeSensitiveKey(key); | |
| return ( | |
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | |
| SENSITIVE_HEADER_PATTERNS.some((pattern) => pattern.test(key)) || | |
| matchesSensitiveNamePattern(normalized) | |
| ); | |
| } | |
| export function isSensitiveQueryParamName(key: string): boolean { | |
| const normalized = normalizeSensitiveKey(key); | |
| return ( | |
| OAUTH_TRACE_SENSITIVE_FIELD_NAMES.has(normalized) || | |
| matchesSensitiveNamePattern(normalized) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/oauth/state-machines/trace-redaction.ts` around lines 78 - 99, Update
isSensitiveTraceFieldName to reuse the same credential-name heuristics as
isSensitiveHeaderName and isSensitiveQueryParamName, including token, secret,
password, credential, cookie, auth, and api_key patterns after normalization.
Preserve exact sensitive-name checks and ensure vendor-prefixed or reshaped keys
such as user_access_token and rejected:access_token are detected; add a narrow
allowlist only if needed to preserve non-secret token_type diagnostics.
| export function sanitizeOAuthUrl(rawUrl: string): string { | ||
| try { | ||
| const url = new URL(rawUrl); | ||
| for (const key of [...url.searchParams.keys()]) { | ||
| if (isSensitiveQueryParamName(key)) { | ||
| url.searchParams.set(key, "[redacted]"); | ||
| } | ||
| } | ||
| if (url.hash) { | ||
| url.hash = "#[redacted]"; | ||
| } | ||
| return url.toString(); | ||
| } catch { | ||
| return sanitizeTraceErrorMessage(rawUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
sanitizeOAuthUrl keeps URL userinfo, so request.url can retain embedded credentials.
The function redacts query parameters and the fragment. It leaves url.username and url.password intact, and URL.toString() re-emits them. sanitizeTraceErrorMessage rules 1a and 1b redact exactly this shape, so the same URL is redacted in an error string and published in an HTTP history entry after sdk/src/oauth/state-machines/trace.ts Line 70 started routing request.url here.
Clear the userinfo before serialization. Use a literal without brackets, because URL percent-encodes [ and ] in the credential components.
🔒️ Proposed fix for URL userinfo
export function sanitizeOAuthUrl(rawUrl: string): string {
try {
const url = new URL(rawUrl);
+ if (url.username || url.password) {
+ url.username = "redacted";
+ url.password = "";
+ }
for (const key of [...url.searchParams.keys()]) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function sanitizeOAuthUrl(rawUrl: string): string { | |
| try { | |
| const url = new URL(rawUrl); | |
| for (const key of [...url.searchParams.keys()]) { | |
| if (isSensitiveQueryParamName(key)) { | |
| url.searchParams.set(key, "[redacted]"); | |
| } | |
| } | |
| if (url.hash) { | |
| url.hash = "#[redacted]"; | |
| } | |
| return url.toString(); | |
| } catch { | |
| return sanitizeTraceErrorMessage(rawUrl); | |
| } | |
| } | |
| export function sanitizeOAuthUrl(rawUrl: string): string { | |
| try { | |
| const url = new URL(rawUrl); | |
| if (url.username || url.password) { | |
| url.username = "redacted"; | |
| url.password = ""; | |
| } | |
| for (const key of [...url.searchParams.keys()]) { | |
| if (isSensitiveQueryParamName(key)) { | |
| url.searchParams.set(key, "[redacted]"); | |
| } | |
| } | |
| if (url.hash) { | |
| url.hash = "#[redacted]"; | |
| } | |
| return url.toString(); | |
| } catch { | |
| return sanitizeTraceErrorMessage(rawUrl); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/oauth/state-machines/trace-redaction.ts` around lines 362 - 377,
Update sanitizeOAuthUrl to clear url.username and url.password before calling
url.toString(), ensuring embedded credentials are removed from sanitized request
URLs. Use a literal redaction value without brackets if assigning a replacement,
since URL serialization percent-encodes bracket characters; preserve the
existing query, fragment, and fallback sanitization behavior.
2 participants
Part 3 of the OAuth maintainability stack; stacked on #3883.
This isolates the state-machine executor wiring, the SDK-owned trace redaction boundary, real integration coverage, and the secret sweep. Vendor-prefixed JSON credential redaction is covered here as a review fix.
Note
High Risk
Touches OAuth credential redaction and display paths across SDK and inspector — incorrect changes can leak secrets in traces or break live token handling. New tests mitigate, but this remains a security-critical surface.
Overview
Unifies OAuth trace redaction under a single SDK module (
trace-redaction.ts) and wires the inspector through it, so client and SDK can no longer drift on what counts as a secret.Closes several display leaks along the way:
stateis now redacted everywhere (fields, URLs, bodies, error text) withdescribeOAuthStateMatchfor diagnostics; request URLs in HTTP history are sanitized; colon-delimited and short Bearer/Basic credentials are covered; prose with=is no longer reshaped into unredacted fields.Telemetry scrubbing is renamed to
redactForTelemetry(deprecated alias kept) and kept deliberately separate from display redaction. Adds a real executor↔state-machine integration test, an exhaustive “no secrets in sanitized traces” sweep, a client redaction ratchet, and a pin on XAA’sREDACTED_DIAGNOSTIC_KEYSso that redact-into-live-state path cannot widen silently.Reviewed by Cursor Bugbot for commit db64ab9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Unifies OAuth trace redaction under one SDK-owned module and wires it into runtime, closing remaining leaks (including
state, colon‑delimited fields, short Bearer/Basic values, and vendor‑prefixed JSON credentials) and keeping client and SDK in sync. Telemetry scrubbing is nowredactForTelemetry, and new integration and sweep tests pin the behavior.Bug Fixes
stateeverywhere (fields, URLs, bodies, error text) and reports presence/match without exposing the nonce; fixeshttpHistory.request.url.=from becoming unredacted fields.Refactors
sdk/src/oauth/state-machines/trace-redaction.tsthe single redaction owner, exported by@mcpjam/sdk/browser;@mcpjam/inspectorre‑exports it behindSANITIZE_OAUTH_TRACESand removes local copies.redactForTelemetryacross CLI/SDK; keepsredactSensitiveValueas a deprecated alias.Written for commit db64ab9. Summary will update on new commits.