Skip to content

Open CORS for any origin to allow direct browser connection #2725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
domdomegg merged 8 commits into from
Sep 19, 2025
Merged

Open CORS for any origin to allow direct browser connection #2725

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1979e68
* In src/everything/sse.ts
cliffhall Sep 16, 2025
921e2f7
* In package.json and package-lock.json
cliffhall Sep 16, 2025
0ea3756
Add caution note for CORS origin wildcard usage
evalstate Sep 17, 2025
bf68938
* In streamableHttp.ts
cliffhall Sep 18, 2025
215e0e4
* In streamableHttp.ts
cliffhall Sep 18, 2025
972bb43
* In sse.ts
cliffhall Sep 18, 2025
635db36
* In sse.ts
cliffhall Sep 18, 2025
1366abd
Merge branch 'main' into open-cors-for-direct-connect
cliffhall Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/everything/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@
},
"dependencies": {
"@modelcontextprotocol/sdk": "^1.18.0",
"cors": "^2.8.5",
"express": "^4.21.1",
"zod": "^3.23.8",
"zod-to-json-schema": "^3.23.5"
},
"devDependencies": {
"@types/cors": "^2.8.19",
"@types/express": "^5.0.0",
"shx": "^0.3.4",
"typescript": "^5.6.2"
Expand Down
8 changes: 7 additions & 1 deletion src/everything/sse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import { SSEServerTransport } from "@modelcontextprotocol/sdk/server/sse.js";
import express from "express";
import { createServer } from "./everything.js";
import cors from 'cors';

console.error('Starting SSE server...');

const app = express();

app.use(cors({
"origin": "*",
"methods": "GET,POST",
"preflightContinue": false,
"optionsSuccessStatus": 204,
})); // Enable CORS for all routes
const transports: Map<string, SSEServerTransport> = new Map<string, SSEServerTransport>();

app.get("/sse", async (req, res) => {
Expand Down
13 changes: 13 additions & 0 deletions src/everything/streamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,22 @@ import { InMemoryEventStore } from '@modelcontextprotocol/sdk/examples/shared/in
import express, { Request, Response } from "express";
import { createServer } from "./everything.js";
import { randomUUID } from 'node:crypto';
import cors from 'cors';

console.error('Starting Streamable HTTP server...');

const app = express();
app.use(cors({
"origin": "*", // use "*" with caution in production
Copy link
Contributor

@felixweinberger felixweinberger Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we restrict this at least a little to localhost:6274?

The reason we need this is only to allow the inspector to work right? I think it's a little too easy for folks to copy paste this server as a starting point and end up with a too permissive CORS policy without ever seeing this comment.

Maybe a comment // enables inspector to connect instead? Would that work for your demo use case?

Copy link
Member Author

@cliffhall cliffhall Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but is there an actual reason for this? In the auth-wg-devx meeting earlier we discussed this and no one could think of a reason why it shouldn't be fully open. If you curl this or use a desktop client, you don't have the cors blocking. There's no obvious XSS vector. And also there was discussion about hosting a version of the inspector. If that happened, then it wouldn't be from that URL. Further, if someone is running the inspector in a container and has chosen to expose 6274 on a different host port, you have a problem.

Finally, in the hosted version of this server that the core team forked to work on an auth example, they have it fully open:

https://github.com/modelcontextprotocol/example-remote-server/blob/main/src/index.ts#L101

Copy link
Member Author

@cliffhall cliffhall Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added // Enable CORS for all routes so Inspector can connect as a comment in both sse.ts and streamableHttp.ts and also // use "*" with caution in production

Copy link
Contributor

@felixweinberger felixweinberger Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not enough of a security expert to push back hard here, but I'm just reminded of the vulnerability we had originally addressed with modelcontextprotocol/inspector@50df0e1 hence asking the question here to be sure.

I believe if you visit evil-website.com while you have your local unauthed dev server running, it could theoretically make requests to it and execute tools with this approach (assuming it knows the port - it might try common ones like 8000 and 8080).

If you've discussed this in auth-wg and we're also giving this as an example remote server, those are convincing points to not block here for me.

Copy link
Member Author

@cliffhall cliffhall Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixweinberger The vulnerability you mention was with the proxy and had two prongs: 0.0.0.0 DNS rebinding that allowed evil-website.com to momentarily change its DNS to point to 0.0.0.0 causing the proxy to think it was actually our inspector, AND being able to tell the proxy to run an arbitrary shell command as an stdio server with the query string.

<script>
fetch("http://0.0.0.0:6277/sse?transportType=stdio&command=touch&args=%2Ftmp%2Fexploited-from-the-browser", {
    "headers": {
        "accept": "*/*",
        "accept-language": "en-US,en;q=0.9", 
        "cache-control": "no-cache",
        "pragma": "no-cache"
    },
    "referrer": "http://127.0.0.1:6274/",
    "referrerPolicy": "strict-origin-when-cross-origin",
    "body": null,
    "method": "GET",
    "mode": "no-cors",
    "credentials": "omit"
})
</script>

Having cors('*') in this example server doesn't equate to that possibility. Something in the browser could connect and execute tools if it knew you had a server there (i.e., the inspector), or it could probe as you suggested. But you wouldn't leave an http mcp server up and running locally for anything but testing purposes. STDIO servers, you'd run locally, but those would not be accessible in this way but rather run within local processes, such as the proxy, which is now locked down on that front.

I do think that the comments about using cors('*') with caution in production should be enough for this example server.

Copy link
Contributor

@felixweinberger felixweinberger Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

"methods": "GET,POST,DELETE",
"preflightContinue": false,
"optionsSuccessStatus": 204,
"exposedHeaders": [
'mcp-session-id',
'last-event-id',
'mcp-protocol-version'
]
})); // Enable CORS for all routes

const transports: Map<string, StreamableHTTPServerTransport> = new Map<string, StreamableHTTPServerTransport>();

Expand All @@ -15,6 +27,7 @@ app.post('/mcp', async (req: Request, res: Response) => {
try {
// Check for existing session ID
const sessionId = req.headers['mcp-session-id'] as string | undefined;
console.log(`Session id: ${sessionId}`);
let transport: StreamableHTTPServerTransport;

if (sessionId && transports.has(sessionId)) {
Expand Down