Fix connection hanging#1966
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
dosubot
Bot
added
the
size:XL
✅ 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. |
![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: df98a027be
ℹ️ 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".
| const { done, value } = await Promise.race([ | ||
| reader.read(), | ||
| new Promise<{ done: boolean; value: undefined }>((_, reject) => | ||
| setTimeout(() => reject(new Error("Read timeout")), 1000) | ||
| ), |
There was a problem hiding this comment.
parseSseResponseBody races each reader.read() against a hard-coded 1s timeout and then swallows the timeout error, so any server that takes more than 1 second to emit its first SSE chunk will return events: [] even though the configured OAuth step timeout is much higher (now 120s). In the OAuth login flow this can misclassify legacy HTTP+SSE responses and fail authenticated probe/login paths on slower networks or loaded servers. The per-read timeout should be aligned with the configured step timeout (or at least maxReadTime) instead of a fixed 1000ms.
Useful? React with 👍 / 👎.
Internal previewPreview URL: https://mcp-inspector-pr-1966.up.railway.app |
WalkthroughThis pull request increases the default OAuth step timeout from 30,000ms to 120,000ms across CLI commands and SDK, implements a 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: 5
🧹 Nitpick comments (1)
sdk/tests/oauth-conformance/interactive.test.ts (1)
54-54: Increase the timeout margin to reduce test flakiness.Line 54 uses
timeoutMs: 10, which is brittle under CI scheduling jitter. A slightly larger value (or fake timers) would make this test reliably deterministic.Suggested tweak
- timeoutMs: 10, + timeoutMs: 100,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth-conformance/interactive.test.ts` at line 54, The test is using an overly small timeout (timeoutMs: 10) which causes flakiness under CI; update the call site that sets timeoutMs to a larger, conservative value (e.g., 50–200 ms) or replace the real-timer dependency with deterministic fake timers (Jest.useFakeTimers()/advanceTimersByTime) for the test in interactive.test.ts where timeoutMs is passed; ensure the updated value or fake-timer approach is applied to the same invocation that currently sets timeoutMs so the test becomes stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/oauth.ts`:
- Around line 90-101: The onProgress handler (and other stderr writes around the
file such as the blocks that output url and non-tty messages) currently writes
untrusted strings directly to process.stderr, enabling terminal injection;
before calling process.stderr.write (both the TTY inline path that uses
wroteInlineProgress and the non‑TTY path) sanitize the dynamic values (message,
url) by stripping or escaping control characters and ANSI escape sequences
(e.g., remove bytes < 0x20 except newline/tab, strip \x1b[...] CSI sequences, or
replace them with safe visible tokens). Implement and reuse a helper like
sanitizeTerminalString(s) and call it for message/url wherever
process.stderr.write is used, leaving the TTY logic (isTTY, \r\x1b[K) intact.
In `@cli/src/lib/debug-artifact.ts`:
- Around line 149-157: Add a short clarifying comment above the file-permission
code around writeFile/chmod explaining that the 0o600 mode is a best-effort on
non-POSIX platforms (Windows maps to ACLs and won’t enforce strict owner-only
perms) and that the implementation intentionally degrades gracefully rather than
hard-failing; reference the resolvedPath, redactedPayload, writeFile and chmod
calls so future reviewers know this is a platform-dependent behavior and whether
strict owner-only enforcement on Windows is required or not.
In `@sdk/src/oauth-conformance/validation.ts`:
- Around line 114-115: The current assignment of listTools can be false when
callTool is true, causing runVerification() to skip verification; change the
logic that sets listTools in the config to ensure callTool forces listTools to
true (e.g., compute listTools as a boolean OR of config.verification?.listTools
and config.verification?.callTool) so that when callTool is enabled verification
runs; update the assignment where listTools is set (referencing
config.verification, listTools and callTool) to reflect this boolean OR
behavior.
In `@sdk/src/oauth-login.ts`:
- Around line 140-179: The code breaks out of the reader loop after collecting a
single event and overwrites repeated data lines, losing later SSE events and
multi-line data. Remove the early exit (the if (events.length >= 1) break;) so
the loop reads the full stream; change parsing so repeated "data:" lines are
concatenated into a buffer on currentEvent (e.g., accumulate into
currentEvent._dataText or append to currentEvent.data when line starts with
"data:"), then JSON.parse that full text once on blank-line boundary (fallback
to the raw concatenated string on parse error). Finally, compute isOldTransport,
endpoint, and mcpResponse from the collected events array (e.g., find the first
event with event === "endpoint" and the first/last "message" event) rather than
assuming events[0].
- Around line 127-131: The Promise.race timeout created alongside reader.read()
is never cleared, so capture the timer id when calling setTimeout inside the new
Promise used in Promise.race(), and when reader.read() resolves (i.e., when
done/value are returned from reader.read()), call clearTimeout(timerId) to
cancel the pending timer; update the Promise.race logic around reader.read(),
the new Promise, and the setTimeout to expose the timer id so you can
clearTimeout it on successful reads (reference reader.read(), Promise.race(),
setTimeout/new Promise).
---
Nitpick comments:
In `@sdk/tests/oauth-conformance/interactive.test.ts`:
- Line 54: The test is using an overly small timeout (timeoutMs: 10) which
causes flakiness under CI; update the call site that sets timeoutMs to a larger,
conservative value (e.g., 50–200 ms) or replace the real-timer dependency with
deterministic fake timers (Jest.useFakeTimers()/advanceTimersByTime) for the
test in interactive.test.ts where timeoutMs is passed; ensure the updated value
or fake-timer approach is applied to the same invocation that currently sets
timeoutMs so the test becomes stable.
🪄 Autofix (Beta)
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
Run ID: a30899bf-c827-43b9-bb08-a5c81c1e100b
📒 Files selected for processing (13)
cli/src/commands/oauth.tscli/src/lib/debug-artifact.tscli/tests/debug-artifact.test.tscli/tests/oauth.test.tsdocs/cli/oauth-conformance.mdxdocs/cli/oauth-login.mdxdocs/cli/reference.mdxdocs/sdk/reference/oauth-conformance.mdxsdk/src/oauth-conformance/auth-strategies/interactive.tssdk/src/oauth-conformance/validation.tssdk/src/oauth-login.tssdk/tests/oauth-conformance/interactive.test.tssdk/tests/oauth-login.probe-timeout.test.ts
| onProgress(message: string) { | ||
| if (quiet) { | ||
| return; | ||
| } | ||
|
|
||
| if (process.stderr.isTTY) { | ||
| wroteInlineProgress = true; | ||
| process.stderr.write(`\r\x1b[K${message}`); | ||
| return; | ||
| } | ||
|
|
||
| process.stderr.write(`${message}\n`); |
There was a problem hiding this comment.
Sanitize terminal-bound text before writing to stderr.
These writes output dynamic values (message, url) directly to terminal control streams. If those values contain control characters, this enables terminal injection/spoofing (e.g., ANSI escapes or forged lines). Please sanitize before output.
Proposed hardening
+function sanitizeTerminalText(value: string): string {
+ return value.replace(/[\u0000-\u001F\u007F]/g, " ");
+}
+
function createOAuthProgressReporter(quiet?: boolean): {
onProgress: (message: string) => void;
clear: () => void;
} {
@@
onProgress(message: string) {
+ const safeMessage = sanitizeTerminalText(message);
if (quiet) {
return;
}
if (process.stderr.isTTY) {
wroteInlineProgress = true;
- process.stderr.write(`\r\x1b[K${message}`);
+ process.stderr.write(`\r\x1b[K${safeMessage}`);
return;
}
- process.stderr.write(`${message}\n`);
+ process.stderr.write(`${safeMessage}\n`);
},
@@
- process.stderr.write(`OAUTH_CONSENT_URL: ${url}\n`);
+ process.stderr.write(
+ `OAUTH_CONSENT_URL: ${sanitizeTerminalText(url)}\n`
+ );
@@
- process.stderr.write(`OAUTH_CONSENT_URL: ${url}\n`);
+ process.stderr.write(
+ `OAUTH_CONSENT_URL: ${sanitizeTerminalText(url)}\n`
+ );Also applies to: 670-674, 801-805
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/oauth.ts` around lines 90 - 101, The onProgress handler (and
other stderr writes around the file such as the blocks that output url and
non-tty messages) currently writes untrusted strings directly to process.stderr,
enabling terminal injection; before calling process.stderr.write (both the TTY
inline path that uses wroteInlineProgress and the non‑TTY path) sanitize the
dynamic values (message, url) by stripping or escaping control characters and
ANSI escape sequences (e.g., remove bytes < 0x20 except newline/tab, strip
\x1b[...] CSI sequences, or replace them with safe visible tokens). Implement
and reuse a helper like sanitizeTerminalString(s) and call it for message/url
wherever process.stderr.write is used, leaving the TTY logic (isTTY, \r\x1b[K)
intact.
| await writeFile( | ||
| resolvedPath, | ||
| `${JSON.stringify(redactedPayload, null, 2)}\n`, | ||
| "utf8", | ||
| { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| } | ||
| ); | ||
| await chmod(resolvedPath, 0o600); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CI runner OS matrix =="
rg -n --iglob '*.yml' --iglob '*.yaml' 'runs-on|matrix|windows|ubuntu|macos' .github/workflows || true
echo
echo "== Permission-related implementation and tests =="
rg -n --type=ts 'chmod\(|0o600|mode\s*&\s*0o777|writeDebugArtifact|writeCommandDebugArtifact'
echo
echo "== Node fs permission caveats currently documented in repo (if any) =="
rg -n --iglob '*.md' 'chmod|permissions|Windows|0o600|debug artifact' .Repository: MCPJam/inspector
Length of output: 10739
🏁 Script executed:
# View the full context around the write/chmod implementation including error handling
sed -n '140,165p' cli/src/lib/debug-artifact.tsRepository: MCPJam/inspector
Length of output: 773
🏁 Script executed:
# Check if there's error handling wrapping the write and chmod operations
rg -A 10 -B 5 'await chmod\(resolvedPath, 0o600\)' cli/src/lib/debug-artifact.tsRepository: MCPJam/inspector
Length of output: 401
🏁 Script executed:
# Verify test coverage - which platforms run the permission validation test?
grep -A 5 -B 5 'mode.*0o777.*0o600' cli/tests/debug-artifact.test.tsRepository: MCPJam/inspector
Length of output: 514
Clarify Windows permission enforcement for 0o600 mode.
Windows is a supported platform (see .github/workflows/windows-release.yml), but chmod() on Windows silently maps 0o600 to Windows ACL semantics rather than enforcing strict owner-only permissions. The code doesn't hard-fail; it degrades gracefully. However, tests only validate POSIX behavior on ubuntu-latest.
Consider adding a brief comment noting that mode enforcement is platform-dependent, and/or document whether strict owner-only permissions are a hard requirement on Windows or acceptable as best-effort.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/lib/debug-artifact.ts` around lines 149 - 157, Add a short clarifying
comment above the file-permission code around writeFile/chmod explaining that
the 0o600 mode is a best-effort on non-POSIX platforms (Windows maps to ACLs and
won’t enforce strict owner-only perms) and that the implementation intentionally
degrades gracefully rather than hard-failing; reference the resolvedPath,
redactedPayload, writeFile and chmod calls so future reviewers know this is a
platform-dependent behavior and whether strict owner-only enforcement on Windows
is required or not.
| listTools: | ||
| config.verification?.listTools ?? !!config.verification?.callTool, |
There was a problem hiding this comment.
Make callTool override listTools: false.
runVerification() bails out when listTools is false, so { callTool, listTools: false } silently skips all verification. That contradicts the documented behavior that callTool enables listTools.
🛠️ Suggested fix
- listTools:
- config.verification?.listTools ?? !!config.verification?.callTool,
+ listTools:
+ config.verification?.callTool !== undefined
+ ? true
+ : (config.verification?.listTools ?? false),📝 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.
| listTools: | |
| config.verification?.listTools ?? !!config.verification?.callTool, | |
| listTools: | |
| config.verification?.callTool !== undefined | |
| ? true | |
| : (config.verification?.listTools ?? false), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/oauth-conformance/validation.ts` around lines 114 - 115, The current
assignment of listTools can be false when callTool is true, causing
runVerification() to skip verification; change the logic that sets listTools in
the config to ensure callTool forces listTools to true (e.g., compute listTools
as a boolean OR of config.verification?.listTools and
config.verification?.callTool) so that when callTool is enabled verification
runs; update the assignment where listTools is set (referencing
config.verification, listTools and callTool) to reflect this boolean OR
behavior.
| const { done, value } = await Promise.race([ | ||
| reader.read(), | ||
| new Promise<{ done: boolean; value: undefined }>((_, reject) => | ||
| setTimeout(() => reject(new Error("Read timeout")), 1000) | ||
| ), |
There was a problem hiding this comment.
Clear the Promise.race() timeout on successful reads.
When reader.read() wins, the 1s timer keeps running. Those ref’ed timers can hold the CLI open briefly even after parsing succeeds, which undercuts the hanging fix.
🛠️ Suggested fix
- const { done, value } = await Promise.race([
- reader.read(),
- new Promise<{ done: boolean; value: undefined }>((_, reject) =>
- setTimeout(() => reject(new Error("Read timeout")), 1000)
- ),
- ]);
+ let readTimeout: ReturnType<typeof setTimeout> | undefined;
+ const { done, value } = await Promise.race([
+ reader.read(),
+ new Promise<{ done: boolean; value: undefined }>((_, reject) => {
+ readTimeout = setTimeout(
+ () => reject(new Error("Read timeout")),
+ 1000
+ );
+ }),
+ ]).finally(() => {
+ if (readTimeout) {
+ clearTimeout(readTimeout);
+ }
+ });📝 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 { done, value } = await Promise.race([ | |
| reader.read(), | |
| new Promise<{ done: boolean; value: undefined }>((_, reject) => | |
| setTimeout(() => reject(new Error("Read timeout")), 1000) | |
| ), | |
| let readTimeout: ReturnType<typeof setTimeout> | undefined; | |
| const { done, value } = await Promise.race([ | |
| reader.read(), | |
| new Promise<{ done: boolean; value: undefined }>((_, reject) => { | |
| readTimeout = setTimeout( | |
| () => reject(new Error("Read timeout")), | |
| 1000 | |
| ); | |
| }), | |
| ]).finally(() => { | |
| if (readTimeout) { | |
| clearTimeout(readTimeout); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/oauth-login.ts` around lines 127 - 131, The Promise.race timeout
created alongside reader.read() is never cleared, so capture the timer id when
calling setTimeout inside the new Promise used in Promise.race(), and when
reader.read() resolves (i.e., when done/value are returned from reader.read()),
call clearTimeout(timerId) to cancel the pending timer; update the Promise.race
logic around reader.read(), the new Promise, and the setTimeout to expose the
timer id so you can clearTimeout it on successful reads (reference
reader.read(), Promise.race(), setTimeout/new Promise).
| for (const line of lines) { | ||
| if (line.startsWith("event:")) { | ||
| currentEvent.event = line.substring(6).trim(); | ||
| } else if (line.startsWith("data:")) { | ||
| const data = line.substring(5).trim(); | ||
| try { | ||
| currentEvent.data = JSON.parse(data); | ||
| } catch { | ||
| currentEvent.data = data; | ||
| } | ||
| } else if (line.startsWith("id:")) { | ||
| currentEvent.id = line.substring(3).trim(); | ||
| } else if (line === "") { | ||
| if (Object.keys(currentEvent).length > 0) { | ||
| events.push({ ...currentEvent }); | ||
| currentEvent = {}; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (events.length >= 1) break; | ||
| } | ||
| } catch { | ||
| // Read timeout or stream error — return whatever we collected | ||
| } finally { | ||
| try { | ||
| await reader.cancel(); | ||
| } catch { | ||
| // ignore cancel errors | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| transport: "sse", | ||
| events, | ||
| isOldTransport: events[0]?.event === "endpoint", | ||
| endpoint: events[0]?.event === "endpoint" ? events[0].data : null, | ||
| mcpResponse: | ||
| events.find((event) => event.event === "message" || !event.event) | ||
| ?.data ?? null, |
There was a problem hiding this comment.
Don’t truncate the SSE stream after the first event.
The return shape tries to surface both endpoint and message, but Line 160 guarantees events never has more than one entry. A valid stream that emits event: endpoint first and event: message second will lose the MCP payload, and repeated data: lines are overwritten instead of concatenated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/oauth-login.ts` around lines 140 - 179, The code breaks out of the
reader loop after collecting a single event and overwrites repeated data lines,
losing later SSE events and multi-line data. Remove the early exit (the if
(events.length >= 1) break;) so the loop reads the full stream; change parsing
so repeated "data:" lines are concatenated into a buffer on currentEvent (e.g.,
accumulate into currentEvent._dataText or append to currentEvent.data when line
starts with "data:"), then JSON.parse that full text once on blank-line boundary
(fallback to the raw concatenated string on parse error). Finally, compute
isOldTransport, endpoint, and mcpResponse from the collected events array (e.g.,
find the first event with event === "endpoint" and the first/last "message"
event) rather than assuming events[0].
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit df98a02. Configure here.
| currentEvent.data = JSON.parse(data); | ||
| } catch { | ||
| currentEvent.data = data; | ||
| } |
There was a problem hiding this comment.
SSE parser overwrites multi-line data instead of concatenating
Low Severity
The new parseSseResponseBody function overwrites currentEvent.data on each data: line. Per the SSE specification, multiple consecutive data: lines in a single event must be concatenated with \n between them. If an MCP server sends a JSON payload split across multiple data: lines (valid per spec), only the last line is preserved, causing JSON.parse to fail and the mcpResponse field to contain a partial string instead of a parsed object. The state machine consumers (e.g., debug-oauth-2025-03-26.ts) then see a null mcpResponse, degrading diagnostic output.
Reviewed by Cursor Bugbot for commit df98a02. Configure here.
1 participant
Note
Medium Risk
Touches OAuth login/conformance flow control and HTTP response parsing (including interactive callback handling), which can affect authentication reliability and timeouts. Changes are well-scoped with added tests/docs but impact a critical path (auth/login).
Overview
OAuth CLI login/conformance usability improvements. Increases the default per-step OAuth timeout from
30_000to120_000, adds--print-urltooauth login(printsOAUTH_CONSENT_URLinstead of opening a browser), and refactors progress reporting to work cleanly on both TTY and non-TTY stderr while emitting status during credential file saves.Fixes for hanging/expired interactive callbacks in the SDK. The interactive callback server now returns a clear
410"session expired" page if the browser redirect arrives after the CLI has timed out, andstop()force-closes lingering keep-alive connections to avoid the process hanging on shutdown.Better diagnostics and security hardening.
runOAuthLoginnow parsestext/event-stream(SSE) response bodies for debug visibility, and debug artifact files are written with0600permissions (with tests updated to assert mode) alongside updated docs reflecting the new timeout default and guidance for stale callbacks.Reviewed by Cursor Bugbot for commit df98a02. Bugbot is set up for automated code reviews on this repo. Configure here.