Implement OAuth authentication and authorization / custom handlers fo… #13
Conversation
…r tools to format them properly
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new MCP OAuth/auth module and integrates it into HTTP and stdio servers; centralizes well-known endpoints and unauthorized responses; introduces many MCP tool handlers with shared types/utilities; rewires tools to use custom handlers; adds a SetLevel MCP request handler and startup port logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as HTTP Client
participant Express as Express App
participant Auth as MCP Auth Router
participant Provider as ProxyOAuthServerProvider
participant MCP as MCP Server
Client->>Express: GET /.well-known/oauth-protected-resource
Express->>Auth: route
Auth-->>Client: { resource, authorization_servers }
Client->>Express: POST / (JSON-RPC)
Express->>Express: parse options
alt Missing token
Express->>Client: 401 WWW-Authenticate + JSON-RPC error
else Token present
Express->>Provider: verifyAccessToken(token)
alt valid
Express->>MCP: forward JSON-RPC
MCP-->>Express: response
Express-->>Client: 200 response
else invalid
Express->>Client: 401 WWW-Authenticate + JSON-RPC error
end
end
sequenceDiagram
autonumber
participant StdioLauncher as STDIO Launcher
participant AuthModule as Auth Module
participant OAuthServer as Desktop OAuth Server
participant MCPServer as MCP Server
participant StdioIO as STDIO Transport
StdioLauncher->>AuthModule: getTokenForStdio()
AuthModule->>OAuthServer: POST /oauth/token (client_credentials)
OAuthServer-->>AuthModule: { access_token }
AuthModule-->>StdioLauncher: token
StdioLauncher->>MCPServer: initMcpServer({ Authorization: Bearer ... })
StdioLauncher->>StdioIO: connect stdio transport
StdioIO-->>StdioLauncher: connected
StdioLauncher-->>StdioLauncher: "MCP server running on stdio"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mcp-server/src/server.ts (1)
78-91: Gate logs by current level — the SDK doesn’t drop messages based on the client’s set level; per MCP spec, servers must respect the configured minimum and only emit logs at or above that level.【1】【2】File: packages/mcp-server/src/server.ts (lines 78–91)
- const logAtLevel = - (level: 'debug' | 'info' | 'warning' | 'error') => - (message: string, ...rest: unknown[]) => { - void server.sendLoggingMessage({ - level, - data: { message, rest }, - }); - }; + const order: LoggingLevel[] = [ + 'debug','info','notice','warning','error','critical','alert','emergency' + ]; + let currentLevel: LoggingLevel = 'info'; + const shouldSend = (level: LoggingLevel) => + order.indexOf(level) >= order.indexOf(currentLevel); + const logAtLevel = + (level: LoggingLevel) => + (message: string, ...rest: unknown[]) => { + if (!shouldSend(level)) return; + void server.sendLoggingMessage({ level, data: { message, rest } }); + }; const logger = { debug: logAtLevel('debug'), info: logAtLevel('info'), warn: logAtLevel('warning'), error: logAtLevel('error'), };packages/mcp-server/src/http.ts (1)
40-49: Authorization header is dropped by overriding defaultHeaders
...authOptionsis spread first, thendefaultHeadersis replaced with onlyX-Stainless-MCP, discarding anyAuthorizationfromauthOptions(or the parsedaccessToken). This will cause downstream 401s despite passing the access token check.initMcpServer({ server: server, clientOptions: { - ...authOptions, - defaultHeaders: { - 'X-Stainless-MCP': 'true', - }, + ...authOptions, + defaultHeaders: { + // preserve any headers provided by parseAuthHeaders + ...((authOptions as any)?.defaultHeaders ?? {}), + // or construct from the parsed access token + ...((authOptions as any)?.accessToken ? { Authorization: `Bearer ${(authOptions as any).accessToken}` } : {}), + 'X-Stainless-MCP': 'true', + }, }, mcpOptions, });
🧹 Nitpick comments (24)
packages/mcp-server/src/auth.ts (2)
119-131: Add a timeout/abort to token retrieval.External calls can hang; add AbortController with a short timeout and surface failures cleanly.
export const getTokenForStdio = async (): Promise<string> => { - const response = await fetch(`${baseURL}/oauth/token`, { + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 10_000); + const response = await fetch(`${baseURL}/oauth/token`, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, body: new URLSearchParams({ grant_type: 'client_credentials', client_id: 'stdio-mcp-client', // client_secret: process.env.MCP_CLIENT_SECRET || '', scope: 'read write', }), - }); + signal: ac.signal, + }).finally(() => clearTimeout(t));Also applies to: 133-139
38-39: Trim noisy console logs or downgrade to debug.Console noise can leak into tests/CI; route through a logger or guard by env.
- console.log('Token validation response status:', response.status); + if (process.env.DEBUG?.includes('mcp-auth')) { + console.debug('Token validation response status:', response.status); + } @@ - console.error('Token validation failed:', error); + console.error('Token validation failed:', error);Also applies to: 56-57
packages/mcp-server/src/stdio.ts (2)
7-27: Split concerns: improve error messages and avoid hard-exiting the host processSeparate token acquisition from server startup so failures are logged accurately, and prefer throwing over
process.exit(1)to avoid taking down an embedding process.export const launchStdioServer = async (options: McpOptions) => { - try { - const token = await getTokenForStdio(); - const server = newMcpServer(); + let token: string; + try { + token = await getTokenForStdio(); + } catch (error) { + console.error('Failed to obtain access token for stdio MCP:', error); + throw error; + } + const server = newMcpServer(); initMcpServer({ server, clientOptions: { defaultHeaders: { Authorization: `Bearer ${token}`, }, }, mcpOptions: options, }); - const transport = new StdioServerTransport(); - await server.connect(transport); - console.error('MCP Server running on stdio'); - } catch (error) { - console.error('Failed to obtain access token:', error); - process.exit(1); - } + try { + const transport = new StdioServerTransport(); + await server.connect(transport); + console.log('MCP Server running on stdio'); + } catch (error) { + console.error('Failed to start MCP server over stdio:', error); + throw error; + } };
23-23: Use info-level for success logSuccess path logs to stderr. Prefer
console.log(as in the diff above) or the MCP logger for consistency.packages/mcp-server/src/http.ts (3)
50-52: Propagate error details in 401 helperPass the caught error so the body includes the cause via
fromError.- } catch (error) { - sendUnauthorizedResponse(res, port); + } catch (error) { + sendUnauthorizedResponse(res, port, error); return null; }
65-68: Omit no-op option
sessionIdGenerator: undefinedis equivalent to not setting it.- const transport = new StreamableHTTPServerTransport({ - // Stateless server - sessionIdGenerator: undefined, - }); + const transport = new StreamableHTTPServerTransport({ + // Stateless server + });
119-125: Info logs to stderrStartup info logs use
console.error. Considerconsole.logfor normal operation.packages/mcp-server/src/tool-handlers/chats/reminders/clear-chat-reminder-handler.ts (2)
3-6: Handle API errors and avoid untyped argsWrap the delete call to return a user-friendly failure instead of throwing, and avoid
anyby minimally guarding expected fields.-export const clearChatReminderHandler: CustomHandlerFunction = async (client, args) => { - const currArgs = args as any; - const output = await client.chats.reminders.delete(currArgs); +export const clearChatReminderHandler: CustomHandlerFunction = async (client, args) => { + const currArgs = (args ?? {}) as Record<string, unknown>; + let output: any; + try { + output = await client.chats.reminders.delete(currArgs); + } catch (err) { + return asFormattedMCPContentResult( + ['# Clear Reminder', `Failed to clear reminder: ${err instanceof Error ? err.message : String(err)}`].join('\n'), + ); + }
14-16: Tidy formattingAvoid embedding newline markers; push a blank line and a proper subheader for consistent rendering.
- lines.push('\n# Using this information\n'); - lines.push('- You can set another reminder with set_chat_reminder.'); + lines.push(''); + lines.push('## Using this information'); + lines.push('- You can set another reminder with set_chat_reminder.');packages/mcp-server/src/tool-handlers/chats/archive-chat-handler.ts (1)
10-11: Prefer reflecting the final state from server response (if available)If the API returns the resulting archive state (e.g., output.archived), use that instead of echoing the requested state to avoid drift on toggle endpoints.
packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts (2)
10-15: Consider a try/catch to surface HTTP or transport errors cleanlyWrap the call and return an error payload via asTextContentResult so callers get actionable feedback instead of an unhandled rejection.
1-8: Remove commented-out blocks or gate them behind a feature flagThe file is ~150 lines of commented code. Either delete or move to a helper with TODOs to keep the handler lean and maintainable.
packages/mcp-server/src/tool-handlers/app/open-in-app-handler.ts (1)
5-6: Catch errors from client.app.open and tidy headingsHandle exceptions and avoid embedding newline characters inside headings.
export const openInAppHandler: CustomHandlerFunction = async (client, args) => { const currArgs = args as any; - const output = await client.app.open(currArgs); + let output; + try { + output = await client.app.open(currArgs); + } catch (err: any) { + return asFormattedMCPContentResult(`# App\nFailed to open Beeper: ${err?.message || String(err)}`); + } @@ - lines.push('\n# Using this information\n'); + lines.push(''); + lines.push('# Using this information'); lines.push('- Use search_chats or get_chat to retrieve chat context.');Also applies to: 21-22
packages/mcp-server/src/tool-handlers/accounts/get-accounts-handler.ts (2)
6-7: Avoid unused parameter lint and catch list() errorsRename the unused args parameter to _args and surface list() failures gracefully.
-export const getAccountsHandler: CustomHandlerFunction = async (client, args) => { - const output = await client.accounts.list(); +export const getAccountsHandler: CustomHandlerFunction = async (client, _args) => { + let output; + try { + output = await client.accounts.list(); + } catch (err: any) { + return asFormattedMCPContentResult( + `Failed to load accounts. ${CONTACT_SUPPORT}\n\nError: ${err?.message || String(err)}`, + ); + }
15-29: Optional: group accounts by network for readabilityIf users often have multiple accounts per network, grouping them reduces header repetition.
packages/mcp-server/src/tool-handlers/types.ts (1)
8-11: Consider threading context through the handler signatureIf HandlerContext is intended to be used, add it as a 3rd param now to avoid future breaking changes.
-export type CustomHandlerFunction = ( +export type CustomHandlerFunction = ( client: BeeperDesktop, - args: Record<string, unknown> | undefined, -) => Promise<ToolCallResult>; + args: Record<string, unknown> | undefined, + context?: HandlerContext, +) => Promise<ToolCallResult>;packages/mcp-server/src/tool-handlers/messages/attachments/download-attachment-handler.ts (1)
9-16: Optionally emit a file content block for better UXIf ContentBlock supports files, return a file entry alongside the markdown so clients can offer direct open/download.
Example (verify exact shape in tools/types.ts):
return { content: [ { type: 'file', path: output.filePath /*, mimeType: 'application/pdf' */ }, { type: 'text', text: lines.join('\n') }, ], };packages/mcp-server/src/tools.ts (2)
16-27: Type-safe handler registry and key drift detectionUse a typed, immutable registry to avoid silent key drift and to get a literal union of tool names.
-const customHandlers: Record<string, CustomHandlerFunction> = { - get_accounts: getAccountsHandler, - open_in_app: openInAppHandler, - get_chat: getChatHandler, - search_chats: searchChatsHandler, - archive_chat: archiveChatHandler, - set_chat_reminder: setChatReminderHandler, - clear_chat_reminder: clearChatReminderHandler, - search_messages: searchMessagesHandler, - send_message: sendMessageHandler, - download_attachment: downloadAttachmentHandler, -}; +const customHandlers = { + get_accounts: getAccountsHandler, + open_in_app: openInAppHandler, + get_chat: getChatHandler, + search_chats: searchChatsHandler, + archive_chat: archiveChatHandler, + set_chat_reminder: setChatReminderHandler, + clear_chat_reminder: clearChatReminderHandler, + search_messages: searchMessagesHandler, + send_message: sendMessageHandler, + download_attachment: downloadAttachmentHandler, +} as const satisfies Record<string, CustomHandlerFunction>; +type CustomToolName = keyof typeof customHandlers;
29-40: Inline override and keep original handlers when no override existsSimplify and reduce branches; keep original handler by default.
-const endpoints: Endpoint[] = originalEndpoints.map((endpoint) => { - const customHandler = customHandlers[endpoint.tool.name]; - - if (customHandler) { - return { - ...endpoint, - handler: customHandler, - }; - } - - return endpoint; -}); +const endpoints: Endpoint[] = originalEndpoints.map((endpoint) => ({ + ...endpoint, + handler: customHandlers[endpoint.tool.name as CustomToolName] ?? endpoint.handler, +}));packages/mcp-server/src/tool-handlers/utils.ts (5)
29-31: Normalize baseURL to avoid double slashesIf
baseURLends with/, links become//open/.... Trim trailing slashes.-export const createOpenLink = (baseURL: string, localChatIDOrChatID: string, messageKey?: string) => - `${baseURL}/open/${encodeURIComponent(localChatIDOrChatID)}${messageKey ? `/${messageKey}` : ''}`; +export const createOpenLink = (baseURL: string, localChatIDOrChatID: string, messageKey?: string) => { + const base = baseURL.replace(/\/+$/, ''); + return `${base}/open/${encodeURIComponent(localChatIDOrChatID)}${messageKey ? `/${messageKey}` : ''}`; +};
59-67: Narrow types after filter to keep TS happyMake the type guard explicit so
reactorNamesisstring[].- .filter(Boolean); + .filter((n): n is string => Boolean(n));
85-99: UsegetParticipantNameand ensure string coercionAvoid duplicating name-picking logic and ensure string output.
-export const formatParticipantsToMarkdown = (participants: User[] | undefined, limit = 3): string => { +export const formatParticipantsToMarkdown = (participants: User[] | undefined, limit = 3): string => { if (!participants || participants.length === 0) return ''; - const names = participants - .slice(0, limit) - .map((p) => p.fullName || p.username || p.id) - .filter(Boolean); + const names = participants + .slice(0, limit) + .map((p) => getParticipantName(p) ?? (p.id != null ? String(p.id) : null)) + .filter((n): n is string => Boolean(n)); if (participants.length > limit) { const othersCount = participants.length - limit; names.push(`& ${othersCount} other${othersCount === 1 ? '' : 's'}`); } return names.join(', '); };
15-17: Optional: handle VS16 to normalize emoji glyph variantsSome emoji include U+FE0F; stripping it keeps reaction keys consistent across platforms.
-const skinToneRegex = /\uD83C[\uDFFB-\uDFFF]/g; +const skinToneRegex = /\uD83C[\uDFFB-\uDFFF]|\uFE0F/g;
4-4: Resolve the TODO with a concreteParticipanttypeDefine a minimal interface (subset of
User) and use it ingetParticipantNameto dropany.I can add a
ParticipantLiketype and update usages—want me to push a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/mcp-server/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
packages/mcp-server/src/auth.ts(1 hunks)packages/mcp-server/src/http.ts(4 hunks)packages/mcp-server/src/server.ts(2 hunks)packages/mcp-server/src/stdio.ts(1 hunks)packages/mcp-server/src/tool-handlers/accounts/get-accounts-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/app/open-in-app-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/chats/archive-chat-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/chats/get-chat-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/chats/reminders/clear-chat-reminder-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/chats/reminders/set-chat-reminder-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/chats/search-chats-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/messages/attachments/download-attachment-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/messages/send-message-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/types.ts(1 hunks)packages/mcp-server/src/tool-handlers/utils.ts(1 hunks)packages/mcp-server/src/tools.ts(1 hunks)packages/mcp-server/src/tools/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (16)
packages/mcp-server/src/tool-handlers/chats/reminders/clear-chat-reminder-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/tool-handlers/accounts/get-accounts-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/tool-handlers/chats/reminders/set-chat-reminder-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts (2)
packages/mcp-server/src/tool-handlers/types.ts (1)
CustomHandlerFunction(8-11)packages/mcp-server/src/tools/types.ts (1)
asTextContentResult(50-59)
packages/mcp-server/src/tool-handlers/chats/archive-chat-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/tool-handlers/messages/attachments/download-attachment-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/auth.ts (1)
packages/mcp-server/src/server.ts (1)
readEnv(176-183)
packages/mcp-server/src/tool-handlers/messages/send-message-handler.ts (2)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)packages/mcp-server/src/tool-handlers/utils.ts (1)
createOpenLink(29-30)
packages/mcp-server/src/tool-handlers/types.ts (1)
packages/mcp-server/src/tools/types.ts (1)
ToolCallResult(40-43)
packages/mcp-server/src/stdio.ts (3)
packages/mcp-server/src/server.ts (4)
McpOptions(26-26)server(46-46)newMcpServer(32-43)initMcpServer(52-129)packages/mcp-server/src/options.ts (1)
McpOptions(15-22)packages/mcp-server/src/auth.ts (1)
getTokenForStdio(119-139)
packages/mcp-server/src/tool-handlers/app/open-in-app-handler.ts (1)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)
packages/mcp-server/src/tool-handlers/chats/search-chats-handler.ts (2)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)packages/mcp-server/src/tool-handlers/utils.ts (1)
formatChatToMarkdown(101-122)
packages/mcp-server/src/tools.ts (14)
packages/mcp-server/src/tool-handlers/types.ts (1)
CustomHandlerFunction(8-11)packages/mcp-server/src/tool-handlers/accounts/get-accounts-handler.ts (1)
getAccountsHandler(6-34)packages/mcp-server/src/tool-handlers/app/open-in-app-handler.ts (1)
openInAppHandler(3-25)packages/mcp-server/src/tool-handlers/chats/get-chat-handler.ts (1)
getChatHandler(4-23)packages/mcp-server/src/tool-handlers/chats/search-chats-handler.ts (1)
searchChatsHandler(4-39)packages/mcp-server/src/tool-handlers/chats/archive-chat-handler.ts (1)
archiveChatHandler(3-18)packages/mcp-server/src/tool-handlers/chats/reminders/set-chat-reminder-handler.ts (1)
setChatReminderHandler(3-18)packages/mcp-server/src/tool-handlers/chats/reminders/clear-chat-reminder-handler.ts (1)
clearChatReminderHandler(3-18)packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts (1)
searchMessagesHandler(10-158)packages/mcp-server/src/tool-handlers/messages/send-message-handler.ts (1)
sendMessageHandler(4-22)packages/mcp-server/src/tool-handlers/messages/attachments/download-attachment-handler.ts (1)
downloadAttachmentHandler(3-25)packages/mcp-server/src/server.ts (1)
endpoints(30-30)packages/mcp-server/src/tools/index.ts (2)
endpoints(18-18)Endpoint(5-5)packages/mcp-server/src/tools/types.ts (1)
Endpoint(100-104)
packages/mcp-server/src/tool-handlers/chats/get-chat-handler.ts (2)
packages/mcp-server/src/tool-handlers/types.ts (2)
CustomHandlerFunction(8-11)asFormattedMCPContentResult(13-22)packages/mcp-server/src/tool-handlers/utils.ts (1)
formatChatToMarkdown(101-122)
packages/mcp-server/src/tool-handlers/utils.ts (1)
packages/mcp-server/src/auth.ts (1)
baseURL(7-7)
packages/mcp-server/src/http.ts (3)
packages/mcp-server/src/server.ts (3)
McpOptions(26-26)server(46-46)newMcpServer(32-43)packages/mcp-server/src/options.ts (1)
McpOptions(15-22)packages/mcp-server/src/auth.ts (3)
sendUnauthorizedResponse(101-117)createMCPAuthRouter(77-86)customWellKnownEndpoint(88-99)
🪛 GitHub Check: lint
packages/mcp-server/src/tools/types.ts
[failure] 61-61:
Delete ⏎
packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts
[failure] 7-7:
'createOpenLink' is defined but never used
[failure] 6-6:
'formatRelativeDate' is defined but never used
[failure] 5-5:
'formatReactionsToMarkdown' is defined but never used
[failure] 4-4:
'formatParticipantsToMarkdown' is defined but never used
[failure] 1-1:
'asFormattedMCPContentResult' is defined but never used
🪛 ESLint
packages/mcp-server/src/tools/types.ts
[error] 61-62: Delete ⏎
(prettier/prettier)
packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts
[error] 1-1: 'asFormattedMCPContentResult' is defined but never used.
(unused-imports/no-unused-imports)
[error] 4-4: 'formatParticipantsToMarkdown' is defined but never used.
(unused-imports/no-unused-imports)
[error] 5-5: 'formatReactionsToMarkdown' is defined but never used.
(unused-imports/no-unused-imports)
[error] 6-6: 'formatRelativeDate' is defined but never used.
(unused-imports/no-unused-imports)
[error] 7-7: 'createOpenLink' is defined but never used.
(unused-imports/no-unused-imports)
🔇 Additional comments (7)
packages/mcp-server/src/tools/types.ts (2)
61-62: Fix Prettier failure (extra blank line).Remove the stray newline to satisfy lint.
[ suggest_nitpick ]- export async function asBinaryContentResult(response: Response): Promise<ToolCallResult> {
62-89: Harden binary result handling and verify TS lib/type settings
- Add upstream‐error handling and MIME fallback:
export async function asBinaryContentResult(response: Response): Promise<ToolCallResult> { - const blob = await response.blob(); - const mimeType = blob.type; + if (!response.ok) { + return { content: [{ type: 'text', text: `HTTP ${response.status} from upstream` }], isError: true }; + } + const blob = await response.blob(); + const mimeType = blob.type || 'application/octet-stream';- Confirm your TypeScript setup includes
"dom"incompilerOptions.lib(for Response/Blob) and that@types/nodeis installed (for Buffer) so all types resolve correctly.packages/mcp-server/src/auth.ts (1)
9-16: Validate unix-socket URL usage for resource identifiers.RFC 9728 defines the resource identifier as an HTTPS URL; using “http://unix:…” (or plain http) may break compliant clients and metadata validation. If unix sockets are needed for transport, consider advertising an https loopback origin for metadata while internally binding to the socket. (datatracker.ietf.org)
Would you like a small helper to derive an https-origin resource URL for metadata while keeping the socket transport?
packages/mcp-server/src/http.ts (1)
103-106: Good routing orderWell-known endpoint before auth middleware and then mounting the auth router is correct.
packages/mcp-server/src/tool-handlers/chats/reminders/clear-chat-reminder-handler.ts (1)
10-11: I’m sorry, but I couldn’t locate the file at that path. Please verify the exact location ofclear-chat-reminder.tsor provide its relative path so I can inspect its metadata.packages/mcp-server/src/tools.ts (2)
42-44: Re-export check passed:queryandFilterare confirmed exported frompackages/mcp-server/src/tools/index.ts.
29-44: Warn on unused custom handler overrides
Add diagnostics after mappingendpointsto catch typos or mismatches betweencustomHandlerskeys and actualendpoint.tool.nameat startup:const endpoints: Endpoint[] = originalEndpoints.map((endpoint) => { const customHandler = customHandlers[endpoint.tool.name]; if (customHandler) { return { ...endpoint, handler: customHandler, }; } return endpoint; }); +// Diagnostics: surface custom handler mismatches at startup +{ + const available = new Set(endpoints.map((e) => e.tool.name)); + const unused = Object.keys(customHandlers).filter((k) => !available.has(k)); + if (unused.length) { + // eslint-disable-next-line no-console + console.warn( + '[mcp-server] Unused custom handlers (no matching endpoint):', + unused.join(', ') + ); + } +}
| SetLevelRequestSchema, | ||
| Tool, | ||
| } from '@modelcontextprotocol/sdk/types.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also import LoggingLevel to validate and handle the full set of MCP log levels.
You’re adding a SetLevel handler; pull in LoggingLevel so we can type-check and support all levels (debug, info, notice, warning, error, critical, alert, emergency).
CallToolRequestSchema,
Implementation,
ListToolsRequestSchema,
- SetLevelRequestSchema,
+ SetLevelRequestSchema,
+ LoggingLevel,
Tool,
} from '@modelcontextprotocol/sdk/types.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SetLevelRequestSchema, | |
| Tool, | |
| } from '@modelcontextprotocol/sdk/types.js'; | |
| SetLevelRequestSchema, | |
| LoggingLevel, | |
| Tool, | |
| } from '@modelcontextprotocol/sdk/types.js'; |
🤖 Prompt for AI Agents
In packages/mcp-server/src/server.ts around lines 10 to 12, the new SetLevel
handler is missing the LoggingLevel import and therefore doesn’t type-check or
validate all MCP log levels; add LoggingLevel to the existing import list from
'@modelcontextprotocol/sdk/types.js' and update the SetLevel handler to
accept/validate the incoming level against the LoggingLevel enum (or set) —
reject or map unsupported values and handle each valid level (debug, info,
notice, warning, error, critical, alert, emergency) explicitly so the handler is
fully typed and covers the full set of MCP log levels.
| server.setRequestHandler(SetLevelRequestSchema, async (request) => { | ||
| const { level } = request.params; | ||
| logger.info(`Log level set to: ${level}`); | ||
| return {}; | ||
| }); |
There was a problem hiding this comment.
SetLevel handler is a no-op; it should update the server’s minimum log level or defer to the SDK’s default.
At present it only logs; it doesn’t change behavior. Implement level validation and update the gate. Per spec, reject invalid params with an error. (spec.modelcontextprotocol.io)
- server.setRequestHandler(SetLevelRequestSchema, async (request) => {
- const { level } = request.params;
- logger.info(`Log level set to: ${level}`);
- return {};
- });
+ server.setRequestHandler(SetLevelRequestSchema, async (request) => {
+ const { level } = request.params;
+ const valid: LoggingLevel[] = [
+ 'debug','info','notice','warning','error','critical','alert','emergency'
+ ];
+ if (!valid.includes(level)) {
+ throw new Error(`Invalid params: unsupported level "${level}"`);
+ }
+ currentLevel = level;
+ // Optionally acknowledge at info (will be gated appropriately)
+ logger.info(`Log level set to: ${level}`);
+ return {};
+ });Committable suggestion skipped: line range outside the PR's diff.
| export const sendMessageHandler: CustomHandlerFunction = async (client, args) => { | ||
| const currArgs = args as any; | ||
| const output = await client.messages.send(currArgs); | ||
|
|
||
| const lines: string[] = []; | ||
| lines.push('# Message Sent'); | ||
| if (output.success) { | ||
| lines.push(`**Message ID**: ${output.messageID}`); | ||
| const deeplink = | ||
| client.baseURL ? createOpenLink(client.baseURL, currArgs?.chatID ?? '', output.messageID) : undefined; | ||
| if (deeplink) lines.push(`**Open in Beeper**: ${deeplink}`); | ||
| } else { | ||
| lines.push('Failed to send message.'); | ||
| } | ||
| lines.push('\n# Using this information\n'); | ||
| lines.push('- Use get_chat to view the conversation, or search_messages for context.'); | ||
|
|
||
| return asFormattedMCPContentResult(lines.join('\n')); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate failures with isError; avoid empty deeplinks; catch thrown errors
Only include an “Open in Beeper” link when chatID is present. Mark failures with isError and handle exceptions.
-export const sendMessageHandler: CustomHandlerFunction = async (client, args) => {
- const currArgs = args as any;
- const output = await client.messages.send(currArgs);
-
- const lines: string[] = [];
- lines.push('# Message Sent');
- if (output.success) {
- lines.push(`**Message ID**: ${output.messageID}`);
- const deeplink =
- client.baseURL ? createOpenLink(client.baseURL, currArgs?.chatID ?? '', output.messageID) : undefined;
- if (deeplink) lines.push(`**Open in Beeper**: ${deeplink}`);
- } else {
- lines.push('Failed to send message.');
- }
- lines.push('\n# Using this information\n');
- lines.push('- Use get_chat to view the conversation, or search_messages for context.');
-
- return asFormattedMCPContentResult(lines.join('\n'));
-};
+export const sendMessageHandler: CustomHandlerFunction = async (client, args) => {
+ const currArgs = args as any;
+ const lines: string[] = [];
+ lines.push('# Message Sent');
+ try {
+ const output = await client.messages.send(currArgs);
+ if (output.success) {
+ lines.push(`**Message ID**: ${output.messageID}`);
+ const deeplink =
+ client.baseURL && currArgs?.chatID
+ ? createOpenLink(client.baseURL, String(currArgs.chatID), output.messageID)
+ : undefined;
+ if (deeplink) lines.push(`**Open in Beeper**: ${deeplink}`);
+ } else {
+ lines.push('Failed to send message.');
+ return asFormattedMCPContentResult(lines.join('\n'), { isError: true });
+ }
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ lines.push(`Failed to send message: ${msg}.`);
+ return asFormattedMCPContentResult(lines.join('\n'), { isError: true });
+ }
+ lines.push('\n# Using this information\n');
+ lines.push('- Use get_chat to view the conversation, or search_messages for context.');
+ return asFormattedMCPContentResult(lines.join('\n'));
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const sendMessageHandler: CustomHandlerFunction = async (client, args) => { | |
| const currArgs = args as any; | |
| const output = await client.messages.send(currArgs); | |
| const lines: string[] = []; | |
| lines.push('# Message Sent'); | |
| if (output.success) { | |
| lines.push(`**Message ID**: ${output.messageID}`); | |
| const deeplink = | |
| client.baseURL ? createOpenLink(client.baseURL, currArgs?.chatID ?? '', output.messageID) : undefined; | |
| if (deeplink) lines.push(`**Open in Beeper**: ${deeplink}`); | |
| } else { | |
| lines.push('Failed to send message.'); | |
| } | |
| lines.push('\n# Using this information\n'); | |
| lines.push('- Use get_chat to view the conversation, or search_messages for context.'); | |
| return asFormattedMCPContentResult(lines.join('\n')); | |
| export const sendMessageHandler: CustomHandlerFunction = async (client, args) => { | |
| const currArgs = args as any; | |
| const lines: string[] = []; | |
| lines.push('# Message Sent'); | |
| try { | |
| const output = await client.messages.send(currArgs); | |
| if (output.success) { | |
| lines.push(`**Message ID**: ${output.messageID}`); | |
| const deeplink = | |
| client.baseURL && currArgs?.chatID | |
| ? createOpenLink(client.baseURL, String(currArgs.chatID), output.messageID) | |
| : undefined; | |
| if (deeplink) lines.push(`**Open in Beeper**: ${deeplink}`); | |
| } else { | |
| lines.push('Failed to send message.'); | |
| return asFormattedMCPContentResult(lines.join('\n'), { isError: true }); | |
| } | |
| } catch (err) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| lines.push(`Failed to send message: ${msg}.`); | |
| return asFormattedMCPContentResult(lines.join('\n'), { isError: true }); | |
| } | |
| lines.push('\n# Using this information\n'); | |
| lines.push('- Use get_chat to view the conversation, or search_messages for context.'); | |
| return asFormattedMCPContentResult(lines.join('\n')); | |
| }; |
🤖 Prompt for AI Agents
In packages/mcp-server/src/tool-handlers/messages/send-message-handler.ts around
lines 4 to 21, the handler must (1) only construct and include the "Open in
Beeper" deeplink when currArgs.chatID is a non-empty value (avoid empty/dead
links), (2) set the result to indicate an error when output.success is false by
returning isError=true (and include the failure message), and (3) wrap the send
call in a try/catch to catch thrown exceptions and return a formatted result
with isError=true and the caught error message; update the deeplink creation to
use currArgs.chatID explicitly and only push the link when present, and ensure
all error paths return asFormattedMCPContentResult with isError set.
| @@ -0,0 +1,22 @@ | |||
| import BeeperDesktop from '@beeper/desktop-api'; | |||
| import { ToolCallResult } from '@beeper/desktop-mcp/tools/types'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix import path for local type to avoid resolution/build failures
Use the local tools/types instead of the external alias.
-import { ToolCallResult } from '@beeper/desktop-mcp/tools/types';
+import { ToolCallResult } from '../tools/types';Run to check for other stray imports:
🏁 Script executed:
#!/bin/bash
rg -n "@beeper/desktop-mcp/tools/types" -g "packages/**"Length of output: 1727
Fix import path for local types across mcp-server
Replace all imports from @beeper/desktop-mcp/tools/types with the correct relative path to packages/mcp-server/src/tools/types.ts. For example:
-import { ToolCallResult } from '@beeper/desktop-mcp/tools/types';
+import { ToolCallResult } from '../tools/types';Occurrences include:
- packages/mcp-server/src/tool-handlers/types.ts
- packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts (use
../../tools/types) - packages/mcp-server/src/tools/messages/send-message.ts
- packages/mcp-server/src/tools/messages/search-messages.ts
- packages/mcp-server/src/tools/messages/attachments/download-attachment.ts
- packages/mcp-server/src/tools/chats/archive-chat.ts
- packages/mcp-server/src/tools/app/open-in-app.ts
- packages/mcp-server/src/tools/chats/search-chats.ts
- packages/mcp-server/src/tools/accounts/get-accounts.ts
- packages/mcp-server/src/tools/chats/get-chat.ts
- packages/mcp-server/src/tools/chats/reminders/set-chat-reminder.ts
- packages/mcp-server/src/tools/chats/reminders/clear-chat-reminder.ts
Locate any remaining with:
rg -n "@beeper/desktop-mcp/tools/types" -g "packages/mcp-server/src/**"🤖 Prompt for AI Agents
In packages/mcp-server/src/tool-handlers/types.ts around line 2, the import
currently references the external package path
"@beeper/desktop-mcp/tools/types"; replace it with the correct local relative
path to packages/mcp-server/src/tools/types.ts (use ../tools/types from this
file). Update other listed files similarly (use ../../tools/types for files
nested one extra folder like tool-handlers/messages/search-messages-handler.ts)
and run the provided ripgrep command to find any remaining occurrences.
| export const formatChatToMarkdown = (chat: Chat, baseURL: string | undefined) => { | ||
| const openURL = baseURL ? createOpenLink(baseURL, chat.localChatID ?? chat.id) : undefined; | ||
| const title = openURL ? `[${chat.title}](${openURL})` : chat.title; | ||
| const participantList = | ||
| chat.participants?.items ? formatParticipantsToMarkdown(chat.participants.items, 3) : ''; | ||
| const participantInfo = participantList ? ` with ${participantList}` : ''; | ||
| const lines: string[] = []; | ||
| lines.push(`\n## ${title} (chatID: ${chat.localChatID})`); | ||
| let chatLine = `Chat on ${chat.network}${participantInfo}.`; | ||
| if (typeof chat.unreadCount === 'number' && chat.unreadCount > 0) { | ||
| chatLine += ` It has ${chat.unreadCount} unread message${chat.unreadCount === 1 ? '' : 's'}.`; | ||
| } | ||
| lines.push(chatLine); | ||
| lines.push(`**Type**: ${chat.type}`); | ||
| if (chat.lastActivity) lines.push(`**Last Activity**: ${chat.lastActivity}`); | ||
| const status: string[] = []; |
There was a problem hiding this comment.
Avoid “undefined” in output and format last activity
Fallback to chat.id when localChatID is absent. Use formatRelativeDate to improve readability of lastActivity.
-export const formatChatToMarkdown = (chat: Chat, baseURL: string | undefined) => {
- const openURL = baseURL ? createOpenLink(baseURL, chat.localChatID ?? chat.id) : undefined;
- const title = openURL ? `[${chat.title}](${openURL})` : chat.title;
+export const formatChatToMarkdown = (chat: Chat, baseURL: string | undefined) => {
+ const chatRef = chat.localChatID ?? chat.id;
+ const openURL = baseURL ? createOpenLink(baseURL, chatRef) : undefined;
+ const title = openURL ? `[${chat.title ?? chatRef}](${openURL})` : (chat.title ?? String(chatRef));
const participantList =
chat.participants?.items ? formatParticipantsToMarkdown(chat.participants.items, 3) : '';
const participantInfo = participantList ? ` with ${participantList}` : '';
const lines: string[] = [];
- lines.push(`\n## ${title} (chatID: ${chat.localChatID})`);
+ lines.push(`\n## ${title} (chatID: ${chatRef})`);
let chatLine = `Chat on ${chat.network}${participantInfo}.`;
if (typeof chat.unreadCount === 'number' && chat.unreadCount > 0) {
chatLine += ` It has ${chat.unreadCount} unread message${chat.unreadCount === 1 ? '' : 's'}.`;
}
lines.push(chatLine);
lines.push(`**Type**: ${chat.type}`);
- if (chat.lastActivity) lines.push(`**Last Activity**: ${chat.lastActivity}`);
+ if (chat.lastActivity) {
+ const d = new Date(chat.lastActivity as any);
+ lines.push(`**Last Activity**: ${isNaN(+d) ? String(chat.lastActivity) : formatRelativeDate(d)}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const formatChatToMarkdown = (chat: Chat, baseURL: string | undefined) => { | |
| const openURL = baseURL ? createOpenLink(baseURL, chat.localChatID ?? chat.id) : undefined; | |
| const title = openURL ? `[${chat.title}](${openURL})` : chat.title; | |
| const participantList = | |
| chat.participants?.items ? formatParticipantsToMarkdown(chat.participants.items, 3) : ''; | |
| const participantInfo = participantList ? ` with ${participantList}` : ''; | |
| const lines: string[] = []; | |
| lines.push(`\n## ${title} (chatID: ${chat.localChatID})`); | |
| let chatLine = `Chat on ${chat.network}${participantInfo}.`; | |
| if (typeof chat.unreadCount === 'number' && chat.unreadCount > 0) { | |
| chatLine += ` It has ${chat.unreadCount} unread message${chat.unreadCount === 1 ? '' : 's'}.`; | |
| } | |
| lines.push(chatLine); | |
| lines.push(`**Type**: ${chat.type}`); | |
| if (chat.lastActivity) lines.push(`**Last Activity**: ${chat.lastActivity}`); | |
| const status: string[] = []; | |
| export const formatChatToMarkdown = (chat: Chat, baseURL: string | undefined) => { | |
| const chatRef = chat.localChatID ?? chat.id; | |
| const openURL = baseURL ? createOpenLink(baseURL, chatRef) : undefined; | |
| const title = openURL | |
| ? `[${chat.title ?? chatRef}](${openURL})` | |
| : (chat.title ?? String(chatRef)); | |
| const participantList = | |
| chat.participants?.items | |
| ? formatParticipantsToMarkdown(chat.participants.items, 3) | |
| : ''; | |
| const participantInfo = participantList ? ` with ${participantList}` : ''; | |
| const lines: string[] = []; | |
| lines.push(`\n## ${title} (chatID: ${chatRef})`); | |
| let chatLine = `Chat on ${chat.network}${participantInfo}.`; | |
| if (typeof chat.unreadCount === 'number' && chat.unreadCount > 0) { | |
| chatLine += ` It has ${chat.unreadCount} unread message${chat.unreadCount === 1 ? '' : 's'}.`; | |
| } | |
| lines.push(chatLine); | |
| lines.push(`**Type**: ${chat.type}`); | |
| if (chat.lastActivity) { | |
| const d = new Date(chat.lastActivity as any); | |
| lines.push( | |
| `**Last Activity**: ${ | |
| isNaN(+d) ? String(chat.lastActivity) : formatRelativeDate(d) | |
| }` | |
| ); | |
| } | |
| const status: string[] = []; | |
| // …rest of function… | |
| } |
🤖 Prompt for AI Agents
In packages/mcp-server/src/tool-handlers/utils.ts around lines 101 to 116, avoid
rendering "undefined" for the chat ID and make lastActivity human-friendly: use
chat.localChatID ?? chat.id wherever the code currently interpolates
chat.localChatID (e.g., in the title and the "(chatID: ...)" text) so the
fallback to chat.id is explicit, and replace plain lastActivity output with a
call to formatRelativeDate(chat.lastActivity) (import formatRelativeDate if not
already imported) before pushing the Last Activity line.
…nses refactor(http): remove unused port parameter and simplify server creation refactor(messages): clean up searchMessagesHandler by removing unused imports fix(types): update asFormattedMCPContentResult to accept optional error flag fix(utils): enhance getParticipantName to handle null participant and ensure ID is a string
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/mcp-server/src/auth.ts (1)
30-44: Resolved: now failing closed on invalid tokensNon-OK responses and errors now throw, avoiding treating bad tokens as authenticated. Thanks for addressing the earlier concern.
🧹 Nitpick comments (4)
packages/mcp-server/src/auth.ts (2)
28-29: Gate validation status logs behind DEBUGReduce noisy logs in prod; print when DEBUG or NODE_ENV!=='production'.
- console.log('Token validation response status:', response.status); + if (process.env.DEBUG) { + console.log('Token validation response status:', response.status); + }
92-112: Surface error details on token acquisition failuresInclude response body snippet for easier debugging and verify access_token presence.
if (!response.ok) { - throw new Error(`Failed to get token: ${response.status}`); + const text = await response.text().catch(() => ''); + throw new Error(`Failed to get token: ${response.status} ${text.slice(0, 200)}`); } const data = await response.json(); - return (data as any).access_token; + const token = (data as any).access_token; + if (!token) throw new Error('Token response missing access_token'); + return token;packages/mcp-server/src/http.ts (2)
91-99: Trust proxy to compute correct scheme/host behind load balancersRequired for accurate resource URLs when deriving from X-Forwarded-* headers.
export const streamableHTTPApp = (options: McpOptions): express.Express => { const app = express(); + app.set('trust proxy', true); app.set('query parser', 'extended'); app.use(express.json());
113-119: Use console.log for startup info (not error channel)These are informational logs.
- console.error(`MCP Server running on streamable HTTP at ${address}`); + console.log(`MCP Server running on streamable HTTP at ${address}`); } else if (address !== null) { - console.error(`MCP Server running on streamable HTTP on port ${address.port}`); + console.log(`MCP Server running on streamable HTTP on port ${address.port}`); } else { - console.error(`MCP Server running on streamable HTTP on port ${port}`); + console.log(`MCP Server running on streamable HTTP on port ${port}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/mcp-server/src/auth.ts(1 hunks)packages/mcp-server/src/http.ts(5 hunks)packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts(1 hunks)packages/mcp-server/src/tool-handlers/types.ts(1 hunks)packages/mcp-server/src/tool-handlers/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mcp-server/src/tool-handlers/messages/search-messages-handler.ts
- packages/mcp-server/src/tool-handlers/utils.ts
- packages/mcp-server/src/tool-handlers/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/mcp-server/src/http.ts (3)
packages/mcp-server/src/server.ts (3)
McpOptions(26-26)server(46-46)newMcpServer(32-43)packages/mcp-server/src/options.ts (1)
McpOptions(15-22)packages/mcp-server/src/auth.ts (3)
sendUnauthorizedResponse(79-90)createMCPAuthRouter(62-70)customWellKnownEndpoint(72-77)
packages/mcp-server/src/auth.ts (1)
packages/mcp-server/src/server.ts (1)
readEnv(176-183)
🔇 Additional comments (1)
packages/mcp-server/src/http.ts (1)
97-100: LGTM: well-known endpoint wiringRouting /.well-known/oauth-protected-resource via the custom handler is correct and complements the OAuth router.
| try { | ||
| const authOptions = parseAuthHeaders(req); | ||
|
|
||
| if (!authOptions.accessToken) { | ||
| sendUnauthorizedResponse(res); | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate the bearer token before initializing the MCP server
Presence-only check accepts any token string. Validate it and propagate the specific error in the 401 challenge.
- const authOptions = parseAuthHeaders(req);
-
- if (!authOptions.accessToken) {
- sendUnauthorizedResponse(res);
- return null;
- }
+ const authOptions = parseAuthHeaders(req);
+ if (!authOptions.accessToken) {
+ sendUnauthorizedResponse(req, res);
+ return null;
+ }
+ try {
+ await validateAccessToken(authOptions.accessToken);
+ } catch (error) {
+ sendUnauthorizedResponse(req, res, error);
+ return null;
+ }Note: import validateAccessToken and updated sendUnauthorizedResponse.
-import { createMCPAuthRouter, customWellKnownEndpoint, sendUnauthorizedResponse } from './auth';
+import {
+ createMCPAuthRouter,
+ customWellKnownEndpoint,
+ sendUnauthorizedResponse,
+ validateAccessToken,
+} from './auth';🤖 Prompt for AI Agents
In packages/mcp-server/src/http.ts around lines 31 to 38, the code currently
only checks presence of authOptions.accessToken and returns 401 for any
non-empty value; update this to call
validateAccessToken(authOptions.accessToken) and handle its result/errors before
proceeding to initialize the MCP server. If validation fails, call
sendUnauthorizedResponse(res, { error: specificError }) (or update
sendUnauthorizedResponse to accept and include the specific error in the
WWW-Authenticate challenge) so the 401 response contains the precise validation
failure; ensure successful validation lets execution continue.
| } catch (error) { | ||
| sendUnauthorizedResponse(res); | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass req into sendUnauthorizedResponse to build correct resource_metadata
Align with the new signature so the 401 header uses the actual host/port.
- } catch (error) {
- sendUnauthorizedResponse(res);
+ } catch (error) {
+ sendUnauthorizedResponse(req, res, error);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| sendUnauthorizedResponse(res); | |
| return null; | |
| } catch (error) { | |
| sendUnauthorizedResponse(req, res, error); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In packages/mcp-server/src/http.ts around lines 49 to 51, the catch block
currently calls sendUnauthorizedResponse(res) but the function now expects the
request to build correct resource_metadata; update the call to pass the request
as the first argument (e.g., sendUnauthorizedResponse(req, res)) and keep the
existing return null; so the 401 header will include the actual host/port.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/mcp-server/src/http.ts (1)
8-8: Presence-only bearer check permits arbitrary tokens; validate before init (async)Currently any non-empty string passes. Validate the token (JWT verify or introspection) and fail fast with a 401. This requires making newServer async and awaiting it in post.
Apply:
-import { createMCPAuthRouter, customWellKnownEndpoint, sendUnauthorizedResponse } from './auth'; +import { createMCPAuthRouter, customWellKnownEndpoint, sendUnauthorizedResponse, validateAccessToken } from './auth';-const newServer = ( +const newServer = async ( defaultMcpOptions: McpOptions, req: express.Request, res: express.Response, -): McpServer | null => { +): Promise<McpServer | null> => {- if (!authOptions.accessToken) { + if (!authOptions.accessToken) { sendUnauthorizedResponse(res); return null; } + await validateAccessToken(authOptions.accessToken);- const server = newServer(defaultOptions, req, res); + const server = await newServer(defaultOptions, req, res);If validateAccessToken isn’t implemented yet, expose it from auth.ts (JWT audience/issuer check or OAuth2 introspection) and reuse here.
Also applies to: 10-15, 31-37, 57-61
🧹 Nitpick comments (3)
packages/mcp-server/src/http.ts (3)
49-51: Avoid leaking internals in 401 body/headersDon’t echo raw errors to clients. Log server-side, return a generic 401.
- } catch (error) { - sendUnauthorizedResponse(res, error); + } catch (error) { + console.warn('Unauthorized request', error); + sendUnauthorizedResponse(res);
97-100: Scope the auth router to its prefix to avoid intercepting '/' and duplicate .well-knownMounting at root may intercept unrelated routes; also ensure the router doesn’t already serve the well-known endpoint.
- const beeperProxyRouter = createMCPAuthRouter(); - app.get('/.well-known/oauth-protected-resource', (req, res) => customWellKnownEndpoint(req, res)); - app.use(beeperProxyRouter); + const beeperProxyRouter = createMCPAuthRouter(); + app.get('/.well-known/oauth-protected-resource', (req, res) => customWellKnownEndpoint(req, res)); + app.use('/oauth', beeperProxyRouter);Confirm the router’s internal paths before scoping to '/oauth'.
108-120: Harden server startup: handle undefined port, log on 'listening', avoid console.error for normal logsBind to 0 when port is undefined, add error handler, and log after the socket is actually bound.
export const launchStreamableHTTPServer = async (options: McpOptions, port: number | string | undefined) => { const app = streamableHTTPApp(options); - const server = app.listen(port); - const address = server.address(); - - if (typeof address === 'string') { - console.error(`MCP Server running on streamable HTTP at ${address}`); - } else if (address !== null) { - console.error(`MCP Server running on streamable HTTP on port ${address.port}`); - } else { - console.error(`MCP Server running on streamable HTTP on port ${port}`); - } + const server = app.listen(port ?? 0); + server.on('error', (err) => { + console.error('MCP HTTP server error:', err); + }); + server.on('listening', () => { + const address = server.address(); + if (typeof address === 'string') { + console.log(`MCP Server running on streamable HTTP at ${address}`); + } else if (address !== null) { + console.log(`MCP Server running on streamable HTTP on port ${address.port}`); + } else { + console.log(`MCP Server running on streamable HTTP on port ${port ?? 0}`); + } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/mcp-server/src/auth.ts(1 hunks)packages/mcp-server/src/http.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-server/src/auth.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/mcp-server/src/http.ts (3)
packages/mcp-server/src/server.ts (3)
McpOptions(26-26)server(46-46)newMcpServer(32-43)packages/mcp-server/src/options.ts (1)
McpOptions(15-22)packages/mcp-server/src/auth.ts (3)
sendUnauthorizedResponse(79-90)createMCPAuthRouter(62-70)customWellKnownEndpoint(72-77)
🔇 Additional comments (1)
packages/mcp-server/src/http.ts (1)
8-8: Auth imports align with centralized OAuth wiringImport set looks right and is used below.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client.ts (1)
132-137: ClarifyskipAccessTokensemantics (docs).Doc is vague and scoped to “stdio MCP server”. Clarify intent and consequences.
Apply:
/** - * Skip access token check for stdio MCP server + * Skip access-token validation at construction time. + * Intended for stdio MCP server flows where an upstream proxy injects Authorization. + * When true, the client may be created without an accessToken. */ skipAccessToken?: boolean | undefined;Additionally (outside this hunk), align the
accessTokendoc so it’s not “always required”:@@ /** - * Bearer access token obtained via OAuth2 PKCE flow or created in-app. Required for all API operations. + * Bearer access token obtained via OAuth2 PKCE flow or created in-app. + * Required for API operations unless `skipAccessToken` is true and an upstream proxy supplies Authorization. */ accessToken?: string | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/mcp-server/src/server.ts(3 hunks)src/client.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-server/src/server.ts
| baseURL = readEnv('BEEPER_DESKTOP_BASE_URL'), | ||
| accessToken = readEnv('BEEPER_ACCESS_TOKEN'), | ||
| skipAccessToken, | ||
| ...opts | ||
| }: ClientOptions = {}) { | ||
| if (accessToken === undefined) { | ||
| if (accessToken === undefined && !skipAccessToken) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Treat empty strings as missing; persist skipAccessToken in options.
Empty string currently bypasses the check and later yields an empty Bearer header. Also, skipAccessToken is dropped from options.
Apply:
constructor({
baseURL = readEnv('BEEPER_DESKTOP_BASE_URL'),
accessToken = readEnv('BEEPER_ACCESS_TOKEN'),
- skipAccessToken,
+ skipAccessToken = false,
...opts
}: ClientOptions = {}) {
- if (accessToken === undefined && !skipAccessToken) {
+ if ((!accessToken || accessToken.trim() === '') && !skipAccessToken) {
throw new Errors.BeeperDesktopError(
"The BEEPER_ACCESS_TOKEN environment variable is missing or empty; either provide it, or instantiate the BeeperDesktop client with an accessToken option, like new BeeperDesktop({ accessToken: 'My Access Token' }).",
);
}Also (outside this hunk), keep skipAccessToken on _options:
@@
- const options: ClientOptions = {
- accessToken,
- ...opts,
- baseURL: baseURL || `http://localhost:23373`,
- };
+ const options: ClientOptions = {
+ accessToken,
+ skipAccessToken,
+ ...opts,
+ baseURL: baseURL || `http://localhost:23373`,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| baseURL = readEnv('BEEPER_DESKTOP_BASE_URL'), | |
| accessToken = readEnv('BEEPER_ACCESS_TOKEN'), | |
| skipAccessToken, | |
| ...opts | |
| }: ClientOptions = {}) { | |
| if (accessToken === undefined) { | |
| if (accessToken === undefined && !skipAccessToken) { | |
| // … earlier in src/client.ts … | |
| constructor({ | |
| baseURL = readEnv('BEEPER_DESKTOP_BASE_URL'), | |
| accessToken = readEnv('BEEPER_ACCESS_TOKEN'), | |
| skipAccessToken = false, | |
| ...opts | |
| }: ClientOptions = {}) { | |
| if ((!accessToken || accessToken.trim() === '') && !skipAccessToken) { | |
| throw new Errors.BeeperDesktopError( | |
| "The BEEPER_ACCESS_TOKEN environment variable is missing or empty; either provide it, or instantiate the BeeperDesktop client with an accessToken option, like new BeeperDesktop({ accessToken: 'My Access Token' }).", | |
| ); | |
| } | |
| const options: ClientOptions = { | |
| accessToken, | |
| skipAccessToken, | |
| ...opts, | |
| baseURL: baseURL || `http://localhost:23373`, | |
| }; | |
| // … rest of constructor … | |
| } |
🤖 Prompt for AI Agents
In src/client.ts around lines 171 to 176, the constructor treats an empty
accessToken string as present (resulting in an empty Bearer header) and drops
skipAccessToken from the stored options; change the missing-token check to treat
empty string as missing (e.g., if ((accessToken === undefined || accessToken ===
'') && !skipAccessToken) { ... }) and when assigning/storing options ensure
skipAccessToken is preserved (include skipAccessToken in the object you store
under _options or include it in the spread: _options = { baseURL, accessToken,
skipAccessToken, ...opts }).
| this._options = options; | ||
|
|
||
| this.accessToken = accessToken; | ||
| this.accessToken = accessToken || ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t emit Authorization when token is absent; normalize token.
Normalize to trimmed string and omit the header entirely when empty to avoid sending Authorization: Bearer .
Apply:
- this.accessToken = accessToken || '';
+ this.accessToken = (accessToken ?? '').trim();Additionally (outside this hunk), guard bearerAuth:
@@
protected async bearerAuth(opts: FinalRequestOptions): Promise<NullableHeaders | undefined> {
- return buildHeaders([{ Authorization: `Bearer ${this.accessToken}` }]);
+ if (!this.accessToken) return undefined;
+ return buildHeaders([{ Authorization: `Bearer ${this.accessToken}` }]);
}🤖 Prompt for AI Agents
In src/client.ts around line 211, normalize the incoming accessToken by trimming
it and storing an empty string when absent, and ensure the Authorization header
is omitted entirely when the token is empty; update this.assignment to set
this.accessToken = (accessToken || '').trim() and adjust header construction
elsewhere to only add Authorization: Bearer <token> when this.accessToken is
non-empty. Also add a guard around bearerAuth usage (outside this hunk) so any
code that reads or uses bearerAuth checks it exists/non-empty before including
or sending the header.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/mcp-server/src/auth.ts (3)
19-46: Extract token validation into a shared helper; reuse across call sites.Prevents duplication and keeps error handling uniform. This was suggested earlier.
Apply:
+export const validateAccessToken = async (token: string) => { + const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { + method: 'POST', + headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json' }, + // Optional timeout added below in verifyAccessToken for reuse + }); + if (!response.ok) throw new Error(`invalid_token (status ${response.status})`); + const tokenData: any = await response.json(); + const scopes = Array.isArray(tokenData.scopes) + ? tokenData.scopes + : typeof tokenData.scopes === 'string' + ? tokenData.scopes.split(/\s+/).filter(Boolean) + : ['read']; + return { token, clientId: tokenData.clientInfo?.name || 'unknown', scopes }; +}; @@ - verifyAccessToken: async (token: string) => { - try { - const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { - method: 'POST', - headers: { - Authorization: `Bearer ${token}`, - 'Content-Type': 'application/json', - }, - }); - - console.log('Token validation response status:', response.status); - - if (!response.ok) { - throw new Error(`invalid_token (status ${response.status})`); - } - - const tokenData: any = await response.json(); - - return { - token, - clientId: tokenData.clientInfo?.name || 'unknown', - scopes: tokenData.scopes || ['read'], - }; - } catch (error) { - console.error('Token validation failed:', error); - throw error; - } - }, + verifyAccessToken: async (token: string) => { + try { + return await validateAccessToken(token); + } catch (error) { + console.error('Token validation failed:', error); + throw error; + } + },
73-78: Build resource metadata from the incoming request; avoid hardcoded host/port.Ensures correct discovery behind proxies and non-default ports. This was suggested earlier.
Apply:
-export const customWellKnownEndpoint = (req: express.Request, res: express.Response) => { - res.json({ - resource: BEEPER_MCP_BASE_URL, - authorization_servers: [BEEPER_DESKTOP_BASE_URL], - }); -}; +export const customWellKnownEndpoint = (req: express.Request, res: express.Response) => { + const proto = + (req.headers['x-forwarded-proto'] as string)?.split(',')[0]?.trim() || req.protocol; + const host = + (req.headers['x-forwarded-host'] as string)?.split(',')[0]?.trim() || req.get('host'); + const resource = `${proto}://${host}`; + res.json({ + resource, + authorization_servers: [BEEPER_DESKTOP_BASE_URL], + }); +};
80-91: 401 should include RFC 6750 fields and correct resource_metadata; avoid leaking internal errors.Also add no-store and vary headers. Prior feedback noted this.
Apply:
-export const sendUnauthorizedResponse = (res: express.Response, error?: any) => { - const wwwAuth = `Bearer resource_metadata="${BEEPER_MCP_BASE_URL}/.well-known/oauth-protected-resource"`; - - res.set('WWW-Authenticate', wwwAuth); - res.status(401).json({ - jsonrpc: '2.0', - error: { - code: -32000, - message: `Invalid request: ${fromError(error)}`, - }, - }); -}; +export const sendUnauthorizedResponse = (req: express.Request, res: express.Response, error?: unknown) => { + const proto = + (req.headers['x-forwarded-proto'] as string)?.split(',')[0]?.trim() || req.protocol; + const host = + (req.headers['x-forwarded-host'] as string)?.split(',')[0]?.trim() || req.get('host'); + const resource = `${proto}://${host}`; + const errCode = error ? 'invalid_token' : 'invalid_request'; + const errDescRaw = error ? String(fromError(error as any)) : 'Missing access token'; + const errDesc = errDescRaw.replace(/"/g, '\\"'); + const wwwAuth = `Bearer error="${errCode}", error_description="${errDesc}", resource_metadata="${resource}/.well-known/oauth-protected-resource"`; + res.set('WWW-Authenticate', wwwAuth); + res.set('Cache-Control', 'no-store'); + res.set('Pragma', 'no-cache'); + res.set('Vary', 'Authorization'); + res.status(401).json({ + jsonrpc: '2.0', + error: { code: -32000, message: `Unauthorized: ${errDescRaw}` }, + }); +};Follow-up: update call sites to pass req.
🧹 Nitpick comments (3)
packages/mcp-server/src/auth.ts (3)
29-29: Reduce noisy console logging.Downgrade to debug or remove in production paths.
- console.log('Token validation response status:', response.status); + // console.debug('Token validation response status:', response.status);
47-59: Validate default redirect_uris; include explicit callback path.BEEPER_MCP_BASE_URL alone is unlikely to be a valid OAuth redirect URI. Ensure a callback path or drop it.
- redirect_uris: - redirect_uris ? redirect_uris : ( - [ - BEEPER_MCP_BASE_URL, - 'http://localhost:6274/oauth/callback/debug', - 'http://localhost:6274/oauth/callback', - ] - ), + redirect_uris: redirect_uris ?? [ + `${BEEPER_MCP_BASE_URL.replace(/\/$/, '')}/oauth/callback`, + 'http://localhost:6274/oauth/callback/debug', + 'http://localhost:6274/oauth/callback', + ],
63-71: Consider deriving baseUrl/issuerUrl from config once and validating.If these are misconfigured, the router will emit links that don’t match the host. Validate format on startup and log a single warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/mcp-server/src/auth.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/mcp-server/src/auth.ts (1)
packages/mcp-server/src/server.ts (1)
readEnv(177-184)
packages/mcp-server/src/auth.ts
Outdated
| export const BEEPER_DESKTOP_BASE_URL = readEnv('BEEPER_DESKTOP_BASE_URL') || 'http://localhost:23373'; | ||
| export const BEEPER_MCP_BASE_URL = readEnv('BEEPER_MCP_BASE_URL') || 'http://localhost:3000'; | ||
| export const BEEPER_AUTH_TOKEN = readEnv('AUTH_TOKEN') || ''; |
There was a problem hiding this comment.
Env var name mismatch for auth token; read both to avoid silent fallback.
Constant is BEEPER_AUTH_TOKEN, but you read AUTH_TOKEN. This will ignore BEEPER_AUTH_TOKEN if set.
Apply:
-export const BEEPER_AUTH_TOKEN = readEnv('AUTH_TOKEN') || '';
+export const BEEPER_AUTH_TOKEN =
+ readEnv('BEEPER_AUTH_TOKEN') ??
+ readEnv('AUTH_TOKEN') ?? // backward-compat
+ '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const BEEPER_DESKTOP_BASE_URL = readEnv('BEEPER_DESKTOP_BASE_URL') || 'http://localhost:23373'; | |
| export const BEEPER_MCP_BASE_URL = readEnv('BEEPER_MCP_BASE_URL') || 'http://localhost:3000'; | |
| export const BEEPER_AUTH_TOKEN = readEnv('AUTH_TOKEN') || ''; | |
| export const BEEPER_DESKTOP_BASE_URL = | |
| readEnv('BEEPER_DESKTOP_BASE_URL') || | |
| 'http://localhost:23373'; | |
| export const BEEPER_MCP_BASE_URL = | |
| readEnv('BEEPER_MCP_BASE_URL') || | |
| 'http://localhost:3000'; | |
| export const BEEPER_AUTH_TOKEN = | |
| readEnv('BEEPER_AUTH_TOKEN') ?? | |
| readEnv('AUTH_TOKEN') ?? // backward-compat | |
| ''; |
🤖 Prompt for AI Agents
In packages/mcp-server/src/auth.ts around lines 7 to 9, the code reads process
env 'AUTH_TOKEN' into BEEPER_AUTH_TOKEN so an explicitly set BEEPER_AUTH_TOKEN
would be ignored; change the lookup to prefer BEEPER_AUTH_TOKEN then fall back
to AUTH_TOKEN and finally to empty string (i.e., check
readEnv('BEEPER_AUTH_TOKEN') || readEnv('AUTH_TOKEN') || '') so both env names
are supported and no silent fallback occurs.
| verifyAccessToken: async (token: string) => { | ||
| try { | ||
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
||
| console.log('Token validation response status:', response.status); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`invalid_token (status ${response.status})`); | ||
| } | ||
|
|
||
| const tokenData: any = await response.json(); | ||
|
|
||
| return { | ||
| token, | ||
| clientId: tokenData.clientInfo?.name || 'unknown', | ||
| scopes: tokenData.scopes || ['read'], | ||
| }; | ||
| } catch (error) { | ||
| console.error('Token validation failed:', error); | ||
| throw error; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add network timeouts to external fetch calls.
Avoids hanging on network stalls.
Apply:
- const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, {
method: 'POST',
headers: {
Authorization: `Bearer ${token}`,
'Content-Type': 'application/json',
},
- });
+ signal: controller.signal,
+ });
+ clearTimeout(timeout);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| verifyAccessToken: async (token: string) => { | |
| try { | |
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { | |
| method: 'POST', | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| console.log('Token validation response status:', response.status); | |
| if (!response.ok) { | |
| throw new Error(`invalid_token (status ${response.status})`); | |
| } | |
| const tokenData: any = await response.json(); | |
| return { | |
| token, | |
| clientId: tokenData.clientInfo?.name || 'unknown', | |
| scopes: tokenData.scopes || ['read'], | |
| }; | |
| } catch (error) { | |
| console.error('Token validation failed:', error); | |
| throw error; | |
| } | |
| }, | |
| verifyAccessToken: async (token: string) => { | |
| try { | |
| // abort after 10 seconds to avoid hanging on network stalls | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10_000); | |
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { | |
| method: 'POST', | |
| headers: { | |
| Authorization: `Bearer ${token}`, | |
| 'Content-Type': 'application/json', | |
| }, | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| console.log('Token validation response status:', response.status); | |
| if (!response.ok) { | |
| throw new Error(`invalid_token (status ${response.status})`); | |
| } | |
| const tokenData: any = await response.json(); | |
| return { | |
| token, | |
| clientId: tokenData.clientInfo?.name || 'unknown', | |
| scopes: tokenData.scopes || ['read'], | |
| }; | |
| } catch (error) { | |
| console.error('Token validation failed:', error); | |
| throw error; | |
| } | |
| }, |
🤖 Prompt for AI Agents
In packages/mcp-server/src/auth.ts around lines 19 to 46, the external fetch to
validate tokens has no network timeout and can hang; add an AbortController with
a reasonable timeout (e.g., 5s) that aborts the fetch if exceeded, clear the
timeout on success, and convert an AbortError into a clear timeout error before
rethrowing; ensure the controller.signal is passed into fetch and the timeout is
cleaned up in both success and error paths.
| export const getTokenForStdio = async (): Promise<string> => { | ||
| if (BEEPER_AUTH_TOKEN) return BEEPER_AUTH_TOKEN; | ||
|
|
||
| // Needs to be implemented | ||
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| body: new URLSearchParams({ | ||
| grant_type: 'client_credentials', | ||
| client_id: 'stdio-mcp-client', | ||
| // client_secret: process.env.MCP_CLIENT_SECRET || '', | ||
| scope: 'read write', | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to get token: ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return (data as any).access_token; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden token acquisition; add timeout and clearer error messages.
Prevents hangs and improves diagnostics.
export const getTokenForStdio = async (): Promise<string> => {
if (BEEPER_AUTH_TOKEN) return BEEPER_AUTH_TOKEN;
// Needs to be implemented
- const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, {
method: 'POST',
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
+ 'Accept': 'application/json',
},
body: new URLSearchParams({
grant_type: 'client_credentials',
- client_id: 'stdio-mcp-client',
+ client_id: readEnv('MCP_CLIENT_ID') ?? 'stdio-mcp-client',
// client_secret: process.env.MCP_CLIENT_SECRET || '',
scope: 'read write',
}),
- });
+ signal: controller.signal,
+ }).catch((e) => {
+ throw new Error(`Token request failed: ${e?.name === 'AbortError' ? 'timeout' : String(e)}`);
+ });
+ clearTimeout(timeout);
if (!response.ok) {
- throw new Error(`Failed to get token: ${response.status}`);
+ let details = '';
+ try { details = JSON.stringify(await response.json()); } catch {}
+ throw new Error(`Failed to get token: HTTP ${response.status}${details ? ` ${details}` : ''}`);
}
const data = await response.json();
return (data as any).access_token;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getTokenForStdio = async (): Promise<string> => { | |
| if (BEEPER_AUTH_TOKEN) return BEEPER_AUTH_TOKEN; | |
| // Needs to be implemented | |
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/x-www-form-urlencoded', | |
| }, | |
| body: new URLSearchParams({ | |
| grant_type: 'client_credentials', | |
| client_id: 'stdio-mcp-client', | |
| // client_secret: process.env.MCP_CLIENT_SECRET || '', | |
| scope: 'read write', | |
| }), | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`Failed to get token: ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| return (data as any).access_token; | |
| }; | |
| export const getTokenForStdio = async (): Promise<string> => { | |
| if (BEEPER_AUTH_TOKEN) return BEEPER_AUTH_TOKEN; | |
| // Needs to be implemented | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10_000); | |
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/x-www-form-urlencoded', | |
| 'Accept': 'application/json', | |
| }, | |
| body: new URLSearchParams({ | |
| grant_type: 'client_credentials', | |
| client_id: readEnv('MCP_CLIENT_ID') ?? 'stdio-mcp-client', | |
| // client_secret: process.env.MCP_CLIENT_SECRET || '', | |
| scope: 'read write', | |
| }), | |
| signal: controller.signal, | |
| }).catch((e) => { | |
| throw new Error(`Token request failed: ${e?.name === 'AbortError' ? 'timeout' : String(e)}`); | |
| }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| let details = ''; | |
| try { | |
| details = JSON.stringify(await response.json()); | |
| } catch {} | |
| throw new Error( | |
| `Failed to get token: HTTP ${response.status}${details ? ` ${details}` : ''}` | |
| ); | |
| } | |
| const data = await response.json(); | |
| return (data as any).access_token; | |
| }; |
🤖 Prompt for AI Agents
In packages/mcp-server/src/auth.ts around lines 93 to 116, the token fetch can
hang and returns opaque errors; wrap the fetch in an AbortController with a
configurable timeout (e.g., 5s) so the request is aborted on timeout, catch
fetch/abort errors and throw a descriptive Error including whether it was a
timeout, and when the response is non-ok, include the response.status and
response text/body in the thrown error to improve diagnostics; ensure the
AbortController signal is passed to fetch and the timer is cleared after
completion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/mcp-server/src/auth.ts (3)
73-78: Build well-known resource URL from the request; avoid hardcoded host/port.Keeps discovery correct behind proxies and non-default ports. This aligns with previous feedback.
+const baseUrlFromRequest = (req: express.Request): string => { + const proto = + (req.headers['x-forwarded-proto'] as string)?.split(',')[0]?.trim() || req.protocol; + const host = + (req.headers['x-forwarded-host'] as string)?.split(',')[0]?.trim() || req.get('host'); + return `${proto}://${host}`; +}; + export const customWellKnownEndpoint = (req: express.Request, res: express.Response) => { - res.json({ - resource: BEEPER_MCP_BASE_URL, - authorization_servers: [BEEPER_DESKTOP_BASE_URL], - }); + const resource = baseUrlFromRequest(req); + res.json({ + resource, + authorization_servers: [BEEPER_DESKTOP_BASE_URL], + }); };
80-91: 401 response: derive resource from request; add RFC 6750 fields; harden error handling.Include error/error_description, Cache-Control no-store, and escape quotes. Also change signature to accept req. Matches prior review notes.
-export const sendUnauthorizedResponse = (res: express.Response, error?: any) => { - const wwwAuth = `Bearer resource_metadata="${BEEPER_MCP_BASE_URL}/.well-known/oauth-protected-resource"`; - - res.set('WWW-Authenticate', wwwAuth); - res.status(401).json({ +export const sendUnauthorizedResponse = ( + req: express.Request, + res: express.Response, + error?: unknown, +) => { + const resource = baseUrlFromRequest(req); + let errDesc = 'Missing access token'; + try { + if (error) errDesc = String(fromError(error as any)); + } catch { + errDesc = String(error); + } + const errCode = error ? 'invalid_token' : 'invalid_request'; + const wwwAuth = `Bearer error="${errCode}", error_description="${errDesc.replace(/"/g, '\\"')}", resource_metadata="${resource}/.well-known/oauth-protected-resource"`; + + res.set('WWW-Authenticate', wwwAuth); + res.set('Cache-Control', 'no-store'); + res.status(401).json({ jsonrpc: '2.0', error: { code: -32000, - message: `Invalid request: ${fromError(error)}`, + message: `Unauthorized: ${errDesc}`, }, }); };Update call sites to pass
req. To find them:#!/bin/bash rg -nP -C2 '\bsendUnauthorizedResponse\s*\('
93-116: Token fetch (stdio): add timeout, clearer errors, Accept header, and env-driven client_id.Prevents hangs and improves diagnostics. Mirrors earlier suggestion.
export const getTokenForStdio = async (): Promise<string> => { if (BEEPER_ACCESS_TOKEN) return BEEPER_ACCESS_TOKEN; // Needs to be implemented - const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/oauth/token`, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json', }, body: new URLSearchParams({ grant_type: 'client_credentials', - client_id: 'stdio-mcp-client', + client_id: readEnv('MCP_CLIENT_ID') ?? 'stdio-mcp-client', // client_secret: process.env.MCP_CLIENT_SECRET || '', scope: 'read write', }), - }); + signal: controller.signal, + }).catch((e) => { + throw new Error(`Token request failed: ${e?.name === 'AbortError' ? 'timeout' : String(e)}`); + }); + clearTimeout(timeout); if (!response.ok) { - throw new Error(`Failed to get token: ${response.status}`); + let details = ''; + try { + details = JSON.stringify(await response.json()); + } catch {} + throw new Error(`Failed to get token: HTTP ${response.status}${details ? ` ${details}` : ''}`); } const data = await response.json(); return (data as any).access_token; };
🧹 Nitpick comments (3)
packages/mcp-server/src/auth.ts (3)
35-41: Type the token validation response.Avoid
any; define a minimal interface fortokenDatato catch shape drift at compile time.Example:
interface ValidateResp { clientInfo?: { name?: string }; scopes?: string[]; } const tokenData: ValidateResp = await response.json();
96-101: Nit: remove stale comment.“Needs to be implemented” is no longer accurate.
7-10: Normalize access-token env name; add backward-compat fallbacks in auth.ts-export const BEEPER_ACCESS_TOKEN = readEnv('BEEPER_ACCESS_TOKEN') || ''; +export const BEEPER_ACCESS_TOKEN = + readEnv('BEEPER_ACCESS_TOKEN') ?? + readEnv('BEEPER_AUTH_TOKEN') ?? + readEnv('AUTH_TOKEN') ?? + '';Update README.md and any tests in packages/mcp-server to mention and cover the legacy BEEPER_AUTH_TOKEN and AUTH_TOKEN variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/mcp-server/src/auth.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/mcp-server/src/auth.ts (1)
packages/mcp-server/src/server.ts (1)
readEnv(177-184)
🔇 Additional comments (1)
packages/mcp-server/src/auth.ts (1)
63-71: LGTM: Router wiring looks correct.
| verifyAccessToken: async (token: string) => { | ||
| try { | ||
| const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, { | ||
| method: 'POST', | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add network timeout and fix header/body mismatch on token validation.
Prevent hangs; avoid sending Content-Type without a body.
- verifyAccessToken: async (token: string) => {
- try {
- const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, {
+ verifyAccessToken: async (token: string) => {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ try {
+ const response = await fetch(`${BEEPER_DESKTOP_BASE_URL}/v0/mcp/validate`, {
method: 'POST',
headers: {
Authorization: `Bearer ${token}`,
- 'Content-Type': 'application/json',
+ Accept: 'application/json',
},
- });
+ // no body; endpoint validates via Authorization header
+ signal: controller.signal,
+ });
console.log('Token validation response status:', response.status);
if (!response.ok) {
throw new Error(`invalid_token (status ${response.status})`);
}
const tokenData: any = await response.json();
return {
token,
clientId: tokenData.clientInfo?.name || 'unknown',
- scopes: tokenData.scopes || ['read'],
+ scopes: tokenData.scopes || [],
};
} catch (error) {
console.error('Token validation failed:', error);
throw error;
+ } finally {
+ clearTimeout(timeout);
}
},Note: defaulting scopes to [] avoids implicitly granting “read”. If “read” is required as a fallback, ignore this part.
Also applies to: 31-46
🏁 Script executed:
#!/bin/bash
# Show implementation of verifyAccessToken with surrounding context
rg -nC5 "verifyAccessToken" packages/mcp-server/src/auth.ts
# Show file imports to verify how fetch is being provided
sed -n '1,30p' packages/mcp-server/src/auth.tsLength of output: 2035
🌐 Web query:
Does Node.js global fetch implementation have a default timeout? What is the recommended way to add a timeout to fetch in Node.js?
💡 Result:
Short answer
- No reliable per-request default you should rely on: Node's global fetch uses undici's timeouts (historically 300s by default; there was a temporary change to 30s and it was reverted). There is no higher-level “fetch timeout” option you can pass other than using Abort signals. [1][2]
Recommended way to add a timeout
- Use an AbortSignal. In modern Node.js you can use AbortSignal.timeout(ms) or create an AbortController and call controller.abort() from setTimeout. Pass the signal to fetch: fetch(url, { signal }). Handle AbortError/TimeoutError. Example patterns: AbortSignal.timeout(5000) or a small helper that creates an AbortController + timer and clears it on completion. [3][4]
Sources
[1] Node.js/undici timeout discussion & defaults (issue history).
[2] BetterStack explanation showing Node follows Chromium/undici defaults and example using AbortSignal.timeout().
[3] MDN: AbortSignal.timeout() and examples.
[4] Common patterns (AbortController + setTimeout) from community docs/StackOverflow.
🏁 Script executed:
#!/bin/bash
rg -nC3 "scopes" packages/mcp-server/src/auth.tsLength of output: 305
Add request timeout, remove mismatched Content-Type header, and clear timer in verifyAccessToken
packages/mcp-server/src/auth.ts:19-28,31-46
- Wrap the fetch call in an AbortController (e.g.
const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 10_000);) and passsignalto avoid indefinite hangs. - Remove the
'Content-Type': 'application/json'header (no request body) and replace withAccept: 'application/json'. - Clear the timeout in a
finallyblock (clearTimeout(timeout)). - Change the fallback scopes from
['read']to[]to avoid implicitly granting read access—if you still need a default “read” scope, you can omit this change.
🤖 Prompt for AI Agents
In packages/mcp-server/src/auth.ts around lines 19 to 28 and 31 to 46, the fetch
in verifyAccessToken lacks a timeout and uses a mismatched Content-Type header
and an unsafe default scope; wrap the fetch with an AbortController (create
controller and set a timeout e.g. 10_000 ms that calls controller.abort()), pass
controller.signal into fetch, remove the 'Content-Type' header and use 'Accept:
application/json' instead, ensure you clearTimeout(timeout) in a finally block
so the timer is cleaned up, and change the fallback scopes from ['read'] to []
(or remove the implicit read permission) to avoid granting unintended access.
2 participants
…r tools to format them properly