feat: allow securing SSE transport with secret #323
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an optional --secret parameter for securing the SSE transport. Key changes include updating tests to validate authentication behavior, modifying the transport handling to incorporate the secret, and updating the CLI and documentation accordingly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/sse.spec.ts | Updated tests to use the new URL type for serverEndpoint, spawn the process with --secret, and add tests for auth scenarios. |
| src/transport.ts | Modified transport handling functions to accommodate an optional secret for both SSE and HTTP endpoints. |
| src/program.ts | Added the --secret CLI option and passed it to the HTTP transport startup. |
| README.md | Updated documentation to include the new --secret command-line option. |
Comments suppressed due to low confidence (1)
src/transport.ts:47
- [nitpick] Consider renaming this local variable (e.g., to 'endpoint') to avoid shadowing the parameter 'url' and improving readability.
let url = '/sse';
| serverEndpoint.searchParams.delete('secret'); | ||
| const transport = new SSEClientTransport(serverEndpoint); | ||
| const client = new Client({ name: 'test', version: '1.0.0' }); | ||
| await expect(() => client.connect(transport)).rejects.toThrow(/403/); | ||
| }); | ||
|
|
||
| test('streamable http transport', async ({ serverEndpoint }) => { | ||
| // need dynamic import b/c of some ESM nonsense | ||
| const { StreamableHTTPClientTransport } = await import('@modelcontextprotocol/sdk/client/streamableHttp.js'); | ||
| const { Client } = await import('@modelcontextprotocol/sdk/client/index.js'); | ||
| const transport = new StreamableHTTPClientTransport(new URL('/mcp', serverEndpoint)); | ||
| serverEndpoint.pathname = '/mcp'; | ||
| const transport = new StreamableHTTPClientTransport(serverEndpoint); |
There was a problem hiding this comment.
[nitpick] Modifying the 'serverEndpoint' URL directly may cause unintended side-effects in subsequent tests if the same instance is shared; consider cloning the URL instance to isolate changes.
Copilot uses AI. Check for mistakes.
|
Is there an issue for this? |
| @@ -76,6 +76,7 @@ The Playwright MCP server supports the following command-line options: | |||
| - `--user-data-dir <path>`: Path to the user data directory | |||
| - `--port <port>`: Port to listen on for SSE transport | |||
| - `--host <host>`: Host to bind server to. Default is localhost. Use 0.0.0.0 to bind to all interfaces. | |||
| - `--secret`: Secret used to secure the SSE transport. | |||
There was a problem hiding this comment.
Users are bad at managing secrets. I think we should generate endpoint based on crypto-secure guid by default. And allow user to override it with full --endpoint. So they'll say npx @playwright/mcp --server http://localhost:0/my-secret for manual control, wdyt? We'll add path to certs later for tls.
There was a problem hiding this comment.
I think we should generate endpoint based on crypto-secure guid by default.
Absolutely, but let's do that in a follow-up.
allow user to override it with full
--endpoint
Full endpoint control will bring us trouble down the road when adopting MCP OAuth, which has some well-known paths. --secret is enough to solve the problem at hand.
We'll add path to certs later for tls.
I don't think TLS is our business, would prefer the user to setup a reverse proxy for this. Imagine we implement TLS, and the next thing people ask for is automated Let's Encrypt provisioning.
There was a problem hiding this comment.
In that case we can just wait - we can't expect users to opt into the --secret.
Adds an optional
--secretparameter that can be used to secure the HTTP transport. In a follow-up i'll look into a secure default for this.