modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 2 on #2319

Browse source 
Both findings are on the caller-signal handling added in round 1:

- `"signal" in init` treated a member present as `undefined` as an
  explicit override. `RequestInit` is a WebIDL dictionary, where such a
  member converts as absent — so `{ ...base, signal: undefined }`, which
  is what a spread over an options object with no signal produces, was
  clearing a Request's embedded signal instead of inheriting it. Only a
  non-`undefined` value overrides now; an explicit `null` is still the
  genuine no-signal case.

- An already-aborted caller signal rejected the race, but
  `Promise.race` still evaluated `fetchFn(...)`, so on the
  signal-dropping proxy path the OAuth request went out anyway. It now
  short-circuits before anything is constructed or sent, and the
  now-unreachable aborted branch inside the race is gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-11 02:44:43 -04:00
commit 2710c98681
1 changed files with 40 additions and 12 deletions
Download patch file Download diff file Expand all files Collapse all files

27
core/auth/requestTimeout.ts
View file

@ -93,15 +93,19 @@ function requestUrlOf(input: RequestInfo | URL): string {
* cancellation semantics for `withOAuthRequestTimeout(new Request(url, { signal }))`
* (Copilot).
*
* `init.signal` still wins whenever the key is *present*, per the fetch spec:
* an explicit `null` there means "no signal" even when `input` is a `Request`
* that has one.
* An `init.signal` still wins, but only when it is not `undefined`. `RequestInit`
* is a WebIDL dictionary, where a member present with the value `undefined` is
* converted as *absent* — so `{ ...base, signal: undefined }`, which is what a
* spread over an options object that had no signal produces, must inherit the
* `Request`'s signal rather than clear it (Copilot). An explicit `null` is the
* genuine "no signal" override and is honoured as one.
*/
function callerSignalOf(
input: RequestInfo | URL,
init: RequestInit | undefined,
): AbortSignal | undefined {
if (init && "signal" in init) return init.signal ?? undefined;
const explicit = init?.signal;
if (explicit !== undefined) return explicit ?? undefined;
if (typeof input !== "string" && !(input instanceof URL)) return input.signal;
return undefined;
}
@ -176,6 +180,12 @@ export function withOAuthRequestTimeout(
const url = requestUrlOf(input);
const controller = new AbortController();
const callerSignal = callerSignalOf(input, init);
// Before anything is constructed or sent. Racing an already-aborted signal
// would still evaluate `fetchFn(...)`, and on the signal-dropping
// `createRemoteFetch` path that means the OAuth request goes out even
// though the caller cancelled before the call (Copilot).
if (callerSignal?.aborted) throw callerSignal.reason;
const signal = callerSignal
? AbortSignal.any([controller.signal, callerSignal])
: controller.signal;
@ -203,13 +213,10 @@ export function withOAuthRequestTimeout(
// whole budget instead of the outer cancellation winning as documented
// (Copilot). The caller's own `reason` is preserved, so it still sees its
// abort rather than a substituted error.
// Not aborted — the short-circuit above returned for that case.
if (callerSignal) {
if (callerSignal.aborted) {
reject(callerSignal.reason);
} else {
onCallerAbort = () => reject(callerSignal.reason);
callerSignal.addEventListener("abort", onCallerAbort, { once: true });
}
onCallerAbort = () => reject(callerSignal.reason);
callerSignal.addEventListener("abort", onCallerAbort, { once: true });
}
});