-
Notifications
You must be signed in to change notification settings - Fork 703
chore: secure http transports with secret #433
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds secret-based authentication to both HTTP and SSE transports by introducing a configurable --secret
flag, enforcing it on incoming requests, and providing a default secret persisted on disk. It also refactors cache/user-data directory logic and updates tests to cover secret scenarios.
- Introduce
--secret
CLI option and propagate through config and merge logic - Enforce secret validation in
startHttpTransport
, generate default secret viagetDefaultSecret
- Update tests in
tests/sse.spec.ts
to include secret in URLs and test failure when missing
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/sse.spec.ts | Update tests to use URL type, include secret param, and add missing-secret cases |
src/transport.ts | Enforce secret on requests, generate default secret, adjust SSE endpoint URL |
src/tools/utils.ts | Add getMSPlaywrightDir() helper for locating cache directory |
src/program.ts | Add --secret CLI option |
src/context.ts | Refactor user-data-dir creation to use getMSPlaywrightDir |
src/config.ts | Add secret to CLI options, config schema, and merge logic |
README.md | Document new --secret flag and URL usage |
Comments suppressed due to low confidence (3)
src/transport.ts:116
- [nitpick] The variable name
url
shadows the imported and globalURL
context; consider renaming it toparsedUrl
orreqUrl
for better clarity.
const url = new URL(`http://localhost${req.url}`);
src/transport.ts:117
- [nitpick] Currently responding with a 401 status code and no body; adding a descriptive message (e.g., 'Unauthorized') in
res.end()
would help clients understand the failure.
if (url.searchParams.get('secret') !== secret) {
src/transport.ts:157
- [nitpick] Logging an array directly may not produce clean multi-line output; use
console.error(...message)
orconsole.error(message.join('\n'))
for more readable logs.
console.error(message);
@@ -49,7 +52,7 @@ async function handleSSE(config: FullConfig, req: http.IncomingMessage, res: htt | |||
|
|||
return await transport.handlePostMessage(req, res); | |||
} else if (req.method === 'GET') { | |||
const transport = new SSEServerTransport('/sse', res); | |||
const transport = new SSEServerTransport('/sse?' + new URLSearchParams({ secret: url.searchParams.get('secret')! }), res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Building the SSE path via string concatenation with URLSearchParams
can be fragile; consider constructing a URL
object or using a helper to generate the full endpoint.
const transport = new SSEServerTransport('/sse?' + new URLSearchParams({ secret: url.searchParams.get('secret')! }), res); | |
const sseUrl = new URL('/sse', 'http://localhost'); | |
sseUrl.searchParams.set('secret', url.searchParams.get('secret')!); | |
const transport = new SSEServerTransport(sseUrl.toString(), res); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to provide an origin, so this is the best I can do.
VS Code implements MCP auth now - let me implement that instead. |
I gave that a test drive, and then realised that with OAuth, we can only authenticate based on the user approving a UI prompt. SSE mode is mostly useful for running the MCP server remotely though, where we don't have a UI the user can approve in. So I'm reviving this config-based one instead. |
No description provided.