Merge pull request #2036 from modelcontextprotocol/v2/fix/2006-crlf-sse-frames

fix(web): accept CRLF-delimited SSE frames on the servers events stream

clients/web/src/test/core/react/useServers.test.tsx

@@ -969,6 +969,146 @@ describe("useServers", () => {
     );
   });
 
+  it("parses CRLF-delimited SSE frames as change notifications (#2006)", async () => {
+    // SSE permits CRLF line endings, and this hook is a hand-rolled parser
+    // (EventSource can't send `x-mcp-remote-auth`). Matching only `\n\n` left
+    // a CRLF frame buffered forever, so an external mcp.json edit never
+    // reached the list.
+    writeFileSync(
+      h.configPath,
+      JSON.stringify({
+        mcpServers: { seed: { type: "stdio", command: "s" } },
+      }),
+    );
+
+    // The frame is held back until the test has mutated the file, so the
+    // resulting list can only come from a refresh this frame triggered.
+    let releaseFrame: (() => void) | undefined;
+    const framed = new Promise<void>((r) => {
+      releaseFrame = r;
+    });
+    const encoder = new TextEncoder();
+    // Referentially stable: an inline arrow re-subscribes on every render, and
+    // the re-subscription's own mount refresh would pick the edit up on its
+    // own — passing whether or not the frame was ever parsed.
+    const fetchFn: typeof fetch = async (input, init) => {
+      const url = input instanceof Request ? input.url : String(input);
+      if (url.endsWith("/api/servers/events")) {
+        // A real ReadableStream in a real Response — `pull` is only invoked
+        // when the hook's reader asks for a chunk, so awaiting inside it
+        // gates delivery exactly the way a hand-rolled reader double would,
+        // while keeping the mock type-checked against the Response API.
+        const stream = new ReadableStream<Uint8Array>({
+          async pull(controller) {
+            await framed;
+            controller.enqueue(encoder.encode("event: change\r\n\r\n"));
+            controller.close();
+          },
+        });
+        return new Response(stream, { status: 200 });
+      }
+      return h.fetchFn(url, init);
+    };
+    const { result } = renderHook(() =>
+      useServers({ baseUrl: "http://test.local", fetchFn }),
+    );
+    await waitFor(() => expect(result.current.loading).toBe(false));
+    expect(result.current.servers.map((s) => s.id)).toEqual(["seed"]);
+
+    writeFileSync(
+      h.configPath,
+      JSON.stringify({
+        mcpServers: { aftercrlf: { type: "stdio", command: "x" } },
+      }),
+    );
+    releaseFrame?.();
+
+    await waitFor(
+      () => {
+        expect(result.current.servers.map((s) => s.id)).toEqual(["aftercrlf"]);
+      },
+      { timeout: 3000 },
+    );
+  });
+
+  it("does not split a CRLF frame separator straddling two chunks (#2006)", async () => {
+    // The bare-CR terminator is deliberately not accepted: a chunk ending in
+    // `…\r\n\r` would otherwise read as a completed frame, and the `\n`
+    // arriving next would start a bogus one. Deliver exactly that split and
+    // assert the frame is recognized once — only after both halves land.
+    writeFileSync(
+      h.configPath,
+      JSON.stringify({
+        mcpServers: { seed: { type: "stdio", command: "s" } },
+      }),
+    );
+
+    let releaseFirstHalf: (() => void) | undefined;
+    const firstHalf = new Promise<void>((r) => {
+      releaseFirstHalf = r;
+    });
+    let releaseSecondHalf: (() => void) | undefined;
+    const secondHalf = new Promise<void>((r) => {
+      releaseSecondHalf = r;
+    });
+    let listGets = 0;
+    let chunks = 0;
+    const encoder = new TextEncoder();
+    const fetchFn: typeof fetch = async (input, init) => {
+      const url = input instanceof Request ? input.url : String(input);
+      if (url.endsWith("/api/servers/events")) {
+        // `pull` runs once per chunk the hook's reader consumes, so `chunks`
+        // reaching 2 proves the half-separator chunk was read and parsed.
+        const stream = new ReadableStream<Uint8Array>({
+          async pull(controller) {
+            chunks += 1;
+            if (chunks === 1) {
+              await firstHalf;
+              controller.enqueue(encoder.encode("event: change\r\n\r"));
+              return;
+            }
+            if (chunks === 2) {
+              await secondHalf;
+              controller.enqueue(encoder.encode("\n"));
+              return;
+            }
+            controller.close();
+          },
+        });
+        return new Response(stream, { status: 200 });
+      }
+      if (url.endsWith("/api/servers")) listGets += 1;
+      return h.fetchFn(url, init);
+    };
+    const { result } = renderHook(() =>
+      useServers({ baseUrl: "http://test.local", fetchFn }),
+    );
+    await waitFor(() => expect(result.current.loading).toBe(false));
+    expect(listGets).toBe(1);
+
+    // Half a separator is not a frame: no refresh yet.
+    releaseFirstHalf?.();
+    await waitFor(() => expect(chunks).toBeGreaterThanOrEqual(2));
+    await new Promise((r) => setTimeout(r, 100));
+    expect(listGets).toBe(1);
+
+    writeFileSync(
+      h.configPath,
+      JSON.stringify({
+        mcpServers: { aftersplit: { type: "stdio", command: "x" } },
+      }),
+    );
+    releaseSecondHalf?.();
+
+    await waitFor(
+      () => {
+        expect(result.current.servers.map((s) => s.id)).toEqual(["aftersplit"]);
+      },
+      { timeout: 3000 },
+    );
+    expect(listGets).toBe(2);
+  });
+
   it("ignores the backend's inert priming comment frame (#1858)", async () => {
     // The backend opens every SSE stream with a `:` comment so a streaming
     // fetch() resolves on Firefox at all. That frame carries no event/data

core/react/useServers.ts

@@ -90,10 +90,38 @@ async function readErrorMessage(res: Response): Promise<string> {
  */
 function isSseDataFrame(frame: string): boolean {
   return frame
-    .split("\n")
+    .split(SSE_LINE_SEPARATOR)
     .some((line) => line.startsWith("event:") || line.startsWith("data:"));
 }
 
+/**
+ * SSE line terminators. The spec permits CRLF, LF, and a bare CR; we accept
+ * the first two. A bare CR is deliberately excluded because it can't be told
+ * apart from the first half of a CRLF that straddles a chunk boundary — a
+ * buffer ending in `…\r\n\r` would look like a completed frame and the `\n`
+ * arriving in the next chunk would be mis-read as the start of another. No
+ * SSE producer in the wild emits bare-CR frames, so the ambiguity buys
+ * nothing.
+ *
+ * A frame ends at a blank line, i.e. two terminators back to back.
+ */
+const SSE_LINE_SEPARATOR = /\r\n|\n/;
+const SSE_FRAME_SEPARATOR = /(?:\r\n|\n){2}/;
+
+/**
+ * Locates the next frame boundary in `buffer`. Returns the frame's end offset
+ * and the length of the blank-line separator that terminated it (2–4 chars,
+ * depending on which terminators the server used), or `null` when the buffer
+ * holds no complete frame yet.
+ */
+function findSseFrameEnd(
+  buffer: string,
+): { end: number; separatorLength: number } | null {
+  const match = SSE_FRAME_SEPARATOR.exec(buffer);
+  if (!match) return null;
+  return { end: match.index, separatorLength: match[0].length };
+}
+
 export function useServers(opts: UseServersOptions): UseServersResult {
   const { baseUrl, authToken, fetchFn } = opts;
   const doFetch = fetchFn ?? globalThis.fetch;
@@ -163,7 +191,9 @@ export function useServers(opts: UseServersOptions): UseServersResult {
         const reader = res.body.getReader();
         const decoder = new TextDecoder();
         let buffer = "";
-        // SSE frames are separated by a blank line (`\n\n`). We don't parse
+        // SSE frames are separated by a blank line — LF- or CRLF-terminated,
+        // both of which the spec permits and `findSseFrameEnd` accepts (#2006).
+        // We don't parse
         // the event type or data — `refreshInternal()` re-fetches the
         // canonical state regardless — so we just count frames and fire a
         // single background refresh per decode chunk. Two `change`
@@ -183,12 +213,12 @@ export function useServers(opts: UseServersOptions): UseServersResult {
           if (done) break;
           buffer += decoder.decode(value, { stream: true });
           let sawFrame = false;
-          let frameEnd = buffer.indexOf("\n\n");
-          while (frameEnd !== -1) {
-            const frame = buffer.slice(0, frameEnd);
-            buffer = buffer.slice(frameEnd + 2);
+          let boundary = findSseFrameEnd(buffer);
+          while (boundary !== null) {
+            const frame = buffer.slice(0, boundary.end);
+            buffer = buffer.slice(boundary.end + boundary.separatorLength);
             if (isSseDataFrame(frame)) sawFrame = true;
-            frameEnd = buffer.indexOf("\n\n");
+            boundary = findSseFrameEnd(buffer);
           }
           if (sawFrame) void refreshInternal(true);
         }