Skip to content

Commit a4e3ced

Browse files
anagnorisis2peripeteiaanagnorisis2peripeteia
committed
fix(gateway): validate Origin before auth on GET/DELETE; merge-safe test token
Addresses review: - Reorder the new GET and DELETE branches so rejectsBrowserLoopbackRequest() runs BEFORE bearer auth, matching the POST path — a browser-Origin loopback request is now rejected (403) before auth, preserving the loopback Origin boundary even for unauthenticated browser requests. Added focused tests: browser-Origin GET and DELETE with no bearer return 403 (before auth). - The new transport tests now read the loopback owner token from getActiveMcpLoopbackRuntime().ownerToken instead of resolveMcpLoopbackBearerToken, so they don't depend on that helper's import (which current main's test file no longer carries).
1 parent c60aed6 commit a4e3ced

2 files changed

Lines changed: 37 additions & 18 deletions

File tree

‎src/gateway/mcp-http.request.ts‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,14 @@ export function validateMcpLoopbackRequest(params: {
9494
}
9595

9696
if (params.req.method === "GET") {
97+
// Origin validation first (matches the POST path): a browser loopback request is
98+
// rejected before bearer auth, so the local-loopback Origin boundary holds even for
99+
// unauthenticated browser requests.
100+
if (rejectsBrowserLoopbackRequest(params.req)) {
101+
params.res.writeHead(403, { "Content-Type": "application/json" });
102+
params.res.end(JSON.stringify({ error: "forbidden" }));
103+
return null;
104+
}
97105
const authHeader = getHeader(params.req, "authorization") ?? "";
98106
const ownerTokenMatched = safeEqualSecret(authHeader, `Bearer ${params.ownerToken}`);
99107
const nonOwnerTokenMatched = safeEqualSecret(authHeader, `Bearer ${params.nonOwnerToken}`);
@@ -102,11 +110,6 @@ export function validateMcpLoopbackRequest(params: {
102110
params.res.end(JSON.stringify({ error: "unauthorized" }));
103111
return null;
104112
}
105-
if (rejectsBrowserLoopbackRequest(params.req)) {
106-
params.res.writeHead(403, { "Content-Type": "application/json" });
107-
params.res.end(JSON.stringify({ error: "forbidden" }));
108-
return null;
109-
}
110113
logMcpLoopbackHttp("sse-open", { method: "GET", path: url.pathname });
111114
params.res.writeHead(200, {
112115
"Content-Type": "text/event-stream",
@@ -127,6 +130,12 @@ export function validateMcpLoopbackRequest(params: {
127130
// Streamable HTTP session teardown. The loopback server is stateless — it owns no
128131
// session lifecycle — so this is an auth-gated no-op acknowledgement: clients that
129132
// send DELETE when closing the transport get a clean 200 rather than a 405.
133+
// Origin validation first (matches the POST/GET paths), before bearer auth.
134+
if (rejectsBrowserLoopbackRequest(params.req)) {
135+
params.res.writeHead(403, { "Content-Type": "application/json" });
136+
params.res.end(JSON.stringify({ error: "forbidden" }));
137+
return null;
138+
}
130139
const authHeader = getHeader(params.req, "authorization") ?? "";
131140
const ownerTokenMatched = safeEqualSecret(authHeader, `Bearer ${params.ownerToken}`);
132141
const nonOwnerTokenMatched = safeEqualSecret(authHeader, `Bearer ${params.nonOwnerToken}`);
@@ -135,11 +144,6 @@ export function validateMcpLoopbackRequest(params: {
135144
params.res.end(JSON.stringify({ error: "unauthorized" }));
136145
return null;
137146
}
138-
if (rejectsBrowserLoopbackRequest(params.req)) {
139-
params.res.writeHead(403, { "Content-Type": "application/json" });
140-
params.res.end(JSON.stringify({ error: "forbidden" }));
141-
return null;
142-
}
143147
logMcpLoopbackHttp("session-delete", { method: "DELETE", path: url.pathname });
144148
params.res.writeHead(200, { "Content-Type": "application/json" });
145149
params.res.end(JSON.stringify({ ok: true }));

‎src/gateway/mcp-http.test.ts‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -657,8 +657,7 @@ describe("createMcpLoopbackServerConfig", () => {
657657

658658
it("opens an auth-gated SSE stream on GET (Streamable HTTP notification channel)", async () => {
659659
server = await startMcpLoopbackServer(0);
660-
const runtime = getActiveMcpLoopbackRuntime();
661-
const token = runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined;
660+
const token = getActiveMcpLoopbackRuntime()?.ownerToken;
662661
const res = await fetch(`http://127.0.0.1:${server.port}/mcp`, {
663662
method: "GET",
664663
headers: token ? { authorization: `Bearer ${token}` } : {},
@@ -677,8 +676,7 @@ describe("createMcpLoopbackServerConfig", () => {
677676

678677
it("rejects a GET notification channel from a browser Origin (403)", async () => {
679678
server = await startMcpLoopbackServer(0);
680-
const runtime = getActiveMcpLoopbackRuntime();
681-
const token = runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined;
679+
const token = getActiveMcpLoopbackRuntime()?.ownerToken;
682680
const res = await fetch(`http://127.0.0.1:${server.port}/mcp`, {
683681
method: "GET",
684682
headers: {
@@ -692,8 +690,7 @@ describe("createMcpLoopbackServerConfig", () => {
692690

693691
it("acknowledges DELETE session teardown with 200 (stateless no-op)", async () => {
694692
server = await startMcpLoopbackServer(0);
695-
const runtime = getActiveMcpLoopbackRuntime();
696-
const token = runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined;
693+
const token = getActiveMcpLoopbackRuntime()?.ownerToken;
697694
const res = await fetch(`http://127.0.0.1:${server.port}/mcp`, {
698695
method: "DELETE",
699696
headers: token ? { authorization: `Bearer ${token}` } : {},
@@ -716,14 +713,32 @@ describe("createMcpLoopbackServerConfig", () => {
716713

717714
it("stays stateless: POST responses advertise no Mcp-Session-Id", async () => {
718715
server = await startMcpLoopbackServer(0);
719-
const runtime = getActiveMcpLoopbackRuntime();
720716
const res = await sendRaw({
721717
port: server.port,
722-
token: runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined,
718+
token: getActiveMcpLoopbackRuntime()?.ownerToken,
723719
headers: { "content-type": "application/json", "x-session-key": "agent:main:main" },
724720
body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }),
725721
});
726722
expect(res.status).toBe(200);
727723
expect(res.headers.get("mcp-session-id")).toBeNull();
728724
});
725+
726+
it("rejects a browser-Origin GET before auth (403, no bearer)", async () => {
727+
server = await startMcpLoopbackServer(0);
728+
const res = await fetch(`http://127.0.0.1:${server.port}/mcp`, {
729+
method: "GET",
730+
headers: { origin: "https://evil.example" },
731+
});
732+
expect(res.status).toBe(403);
733+
await res.body?.cancel();
734+
});
735+
736+
it("rejects a browser-Origin DELETE before auth (403, no bearer)", async () => {
737+
server = await startMcpLoopbackServer(0);
738+
const res = await fetch(`http://127.0.0.1:${server.port}/mcp`, {
739+
method: "DELETE",
740+
headers: { origin: "https://evil.example" },
741+
});
742+
expect(res.status).toBe(403);
743+
});
729744
});

0 commit comments

Comments
 (0)