feat: remote multi-user MCP server with YNAB OAuth (v0.3.0) - #8
Conversation
Groundwork for a multi-tenant remote MCP server. Adds an HTTP transport alongside stdio and the per-user isolation seams needed for later OAuth. - MCP_TRANSPORT=http runs an Express app exposing the MCP Streamable HTTP transport at POST /mcp (per-session Server) plus a plain GET /health. - Per-session isolation: generalize createServer into buildYnabClient/ createServerForUser so each session gets its own YnabClient, cache, rate limiter, and audit log. - De-singletonize the audit log: inject an AuditLog into YnabClient (defaults to the process singleton for stdio); the ynab_audit_log tool reads the per-request client's instance. Prevents cross-user leakage. - Interim auth: HTTP mode binds the YNAB token per session via the X-YNAB-Token header (falls back to YNAB_ACCESS_TOKEN); TLS required. Replaced by YNAB OAuth in a later phase. - Config: loadHttpConfig() (PORT, PUBLIC_URL, ALLOWED_HOSTS/ORIGINS, DNS- rebinding protection); loadConfig() (stdio) unchanged. - Docs: README HTTP section, .env.example, Dockerfile EXPOSE 3000. - Tests: HTTP wiring (health, auth gate, session lifecycle) + per-user isolation; full suite 627 passing, 80% coverage gate green. stdio path unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PgWfscEzb25EXrgJg1Mt5
WalkthroughThis PR adds an experimental HTTP/remote transport mode for the MCP server using Express, alongside the existing stdio transport. It refactors server construction into per-user context isolation with injected audit logging, adds HTTP configuration loading, interim token-based authentication, and supporting documentation and tests. ChangesHTTP transport and multi-tenant server isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant SessionMap
participant createServerForUser
participant Transport
Client->>ExpressApp: POST /mcp (initialize, X-YNAB-Token)
ExpressApp->>ExpressApp: check session id header
ExpressApp->>ExpressApp: validate token / fallback token
ExpressApp->>createServerForUser: build per-user server
createServerForUser-->>ExpressApp: Server
ExpressApp->>Transport: create StreamableHTTPServerTransport
Transport->>SessionMap: store sessionId -> transport
ExpressApp->>Transport: connect + handleRequest
Transport-->>Client: response + Mcp-Session-Id header
Client->>ExpressApp: DELETE /mcp (sessionId)
ExpressApp->>SessionMap: lookup transport
ExpressApp->>Transport: close
Transport->>SessionMap: delete sessionId
Security note: This PR introduces an interim authentication scheme relying on a plaintext Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/environment.ts`:
- Around line 98-110: Validate the HTTP config numeric env vars in
environment.ts before returning the config object. The current parseInteger
calls for port, cacheTtlMs, and rateLimitPerHour allow zero or other invalid
values that only fail later in HTTP/session setup, so add range-specific
validation at the config parsing layer and fail fast for out-of-range inputs.
Update the existing environment parsing logic around parseInteger, parseBoolean,
and the config return path so PORT, CACHE_TTL_MS, and RATE_LIMIT_PER_HOUR are
guaranteed to be within valid bounds before use.
- Around line 64-70: parseList currently claims to handle comma/space-separated
values, but it only splits on commas, so space-delimited env vars are parsed as
a single entry. Update parseList in environment.ts to split on both commas and
whitespace, then trim and filter empties, keeping the documented contract
aligned with the behavior.
- Line 106: The current environment config in environment.ts always sets
fallbackAccessToken from YNAB_ACCESS_TOKEN, which lets downstream HTTP setup
silently use the owner’s token when X-YNAB-Token is absent. Update the
environment loading logic to require an explicit opt-in flag that defaults to
off, and only populate fallbackAccessToken when that single-user flag is
enabled. Make the change in the config path that defines fallbackAccessToken so
the HTTP initialization code will no longer accept the env token by default.
In `@src/http.ts`:
- Around line 34-36: The active-session Map in http.ts can grow without bound
because transports and their per-user resources are only removed on close. Add
explicit session lifecycle controls around the transport handling in the
initialize flow and cleanup path: enforce an idle TTL for inactive sessions
and/or a maximum session cap, and evict old entries from transports before
creating new ones. Make sure the cleanup logic updates the associated per-user
server/client/cache/rate-limiter/audit-log resources consistently when a session
expires or is rejected.
- Around line 122-133: startHttpServer currently only logs that TLS is required
while still starting the HTTP listener and accepting X-YNAB-Token auth. Add a
startup guard in startHttpServer (using HttpConfig and the app.listen path) that
fails closed unless the deployment is explicitly local/single-user or the
request is guaranteed to arrive over HTTPS/proxy-terminated TLS. Ensure interim
token auth is rejected at runtime when TLS/proxy protocol is not configured,
rather than relying on the console warning.
- Around line 57-63: The session auth in src/http.ts should not fall back to
config.fallbackAccessToken for remote callers by default. Update the token
lookup in the request handling path (around the req.header('x-ynab-token')
logic) so X-YNAB-Token is required unless an explicit single-user/local-only
mode flag is enabled. Keep the unauthorized response path in the same handler,
and make the env-token fallback reachable only through that deliberate mode in
the session setup flow.
- Around line 31-32: The Express app initialization in the http setup is missing
baseline security headers. Update the app configuration around the express() and
app.use(...) setup to add helmet() before registering routes or other middleware
that expose the HTTP surface, so the default browser-facing security headers are
applied. Use the existing app initialization in the HTTP entrypoint to place
Helmet early in the middleware chain.
- Around line 93-97: The async `/mcp` request path in `http.ts` is letting
rejections from `server.connect()` and `transport.handleRequest()` bubble into
Express’ default error handler. Update the handler around `handleRequest` (and
the `server.connect` call site if it is in the same async route) to catch
failures explicitly, log only sanitized request context, and return a generic
`jsonRpcError(...)` response instead of a raw Express error page. Use the
existing `transport.handleRequest` and `server.connect` flow as the place to add
this guarded error handling.
In `@src/index.ts`:
- Around line 48-50: The stdin EOF shutdown hook is being registered too
broadly, so HTTP mode still exits when stdin closes. Update the startup flow in
the main entrypoint around the transportMode check so `process.stdin.on('close',
...)` is only attached for stdio mode, and keep the HTTP branch
(`startHttpServer`) free of that handler. Make the transport-specific setup
explicit so the stdio shutdown logic cannot run after HTTP startup.
- Around line 33-35: The shutdown path in the httpServer close block can hang
forever because httpServer.close() waits for open MCP GET streams. Update the
shutdown logic in index.ts to use a bounded timeout and forcibly close lingering
sockets, or add a transport/session close hook from http.ts and call it before
awaiting server close. Use the httpServer handling in index.ts and the HTTP
transport/session code in src/http.ts to locate the shutdown flow.
In `@tests/unit/http.test.ts`:
- Around line 85-96: The DELETE /mcp test in http.test.ts only verifies that
teardown succeeds, but it does not confirm the session was actually removed by
createHttpApp/transports.delete. Update the test around createHttpApp and the
existing sessionId flow to send a follow-up request after the DELETE and assert
it is rejected (for example with a 400), using the same session id so the
teardown behavior is verified end to end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88f5b2c2-ddfe-4297-911b-c41a1c222b32
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.env.exampleDockerfileREADME.mdpackage.jsonsrc/config/environment.tssrc/http.tssrc/index.tssrc/server.tssrc/services/ynab-client.tssrc/tools/system/audit-log.tstests/unit/http.test.tstests/unit/server-context.test.ts
| /** Split a comma/space-separated env list into a trimmed string array (or undefined). */ | ||
| function parseList(value: string | undefined): string[] | undefined { | ||
| if (value === undefined || value.trim() === '') return undefined; | ||
| const items = value | ||
| .split(',') | ||
| .map((s) => s.trim()) | ||
| .filter((s) => s.length > 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make parseList match its documented separator contract.
Line 64 promises comma/space-separated values, but Line 68 only splits on commas, so ALLOWED_HOSTS="api.example.com app.example.com" becomes one unusable allowlist entry.
Proposed fix
function parseList(value: string | undefined): string[] | undefined {
if (value === undefined || value.trim() === '') return undefined;
const items = value
- .split(',')
+ .split(/[,\s]+/)
.map((s) => s.trim())
.filter((s) => s.length > 0);
return items.length > 0 ? items : undefined;
}As per path instructions, “Environment variables must be validated.”
📝 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.
| /** Split a comma/space-separated env list into a trimmed string array (or undefined). */ | |
| function parseList(value: string | undefined): string[] | undefined { | |
| if (value === undefined || value.trim() === '') return undefined; | |
| const items = value | |
| .split(',') | |
| .map((s) => s.trim()) | |
| .filter((s) => s.length > 0); | |
| /** Split a comma/space-separated env list into a trimmed string array (or undefined). */ | |
| function parseList(value: string | undefined): string[] | undefined { | |
| if (value === undefined || value.trim() === '') return undefined; | |
| const items = value | |
| .split(/[,\s]+/) | |
| .map((s) => s.trim()) | |
| .filter((s) => s.length > 0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/environment.ts` around lines 64 - 70, parseList currently claims
to handle comma/space-separated values, but it only splits on commas, so
space-delimited env vars are parsed as a single entry. Update parseList in
environment.ts to split on both commas and whitespace, then trim and filter
empties, keeping the documented contract aligned with the behavior.
Source: Path instructions
| port: parseInteger(process.env['PORT'], 3000, 'PORT'), | ||
| publicUrl: process.env['PUBLIC_URL'] || undefined, | ||
| allowedHosts, | ||
| allowedOrigins, | ||
| // Only meaningful when a host/origin allowlist is configured. | ||
| enableDnsRebindingProtection: | ||
| parseBoolean(process.env['ENABLE_DNS_REBINDING_PROTECTION'], false, 'ENABLE_DNS_REBINDING_PROTECTION') && | ||
| (allowedHosts !== undefined || allowedOrigins !== undefined), | ||
| fallbackAccessToken: process.env['YNAB_ACCESS_TOKEN'] || undefined, | ||
| defaultBudgetId: process.env['YNAB_BUDGET_ID'] || undefined, | ||
| readOnly: parseBoolean(process.env['YNAB_READ_ONLY'], true, 'YNAB_READ_ONLY'), | ||
| cacheTtlMs: parseInteger(process.env['CACHE_TTL_MS'], 300000, 'CACHE_TTL_MS'), | ||
| rateLimitPerHour: parseInteger(process.env['RATE_LIMIT_PER_HOUR'], 180, 'RATE_LIMIT_PER_HOUR'), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate numeric env ranges before returning HTTP config.
parseInteger() accepts 0 and unbounded digit strings; CACHE_TTL_MS=0 or RATE_LIMIT_PER_HOUR=0 pass here but fail later when Cache/RateLimiter are constructed during HTTP session setup. Validate PORT, CACHE_TTL_MS, and RATE_LIMIT_PER_HOUR with range-specific checks so bad config fails fast.
As per path instructions, “Environment variables must be validated.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/environment.ts` around lines 98 - 110, Validate the HTTP config
numeric env vars in environment.ts before returning the config object. The
current parseInteger calls for port, cacheTtlMs, and rateLimitPerHour allow zero
or other invalid values that only fail later in HTTP/session setup, so add
range-specific validation at the config parsing layer and fail fast for
out-of-range inputs. Update the existing environment parsing logic around
parseInteger, parseBoolean, and the config return path so PORT, CACHE_TTL_MS,
and RATE_LIMIT_PER_HOUR are guaranteed to be within valid bounds before use.
Source: Path instructions
| enableDnsRebindingProtection: | ||
| parseBoolean(process.env['ENABLE_DNS_REBINDING_PROTECTION'], false, 'ENABLE_DNS_REBINDING_PROTECTION') && | ||
| (allowedHosts !== undefined || allowedOrigins !== undefined), | ||
| fallbackAccessToken: process.env['YNAB_ACCESS_TOKEN'] || undefined, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require explicit opt-in before using the env token as HTTP fallback.
fallbackAccessToken is populated whenever YNAB_ACCESS_TOKEN exists; downstream HTTP initialization uses it when X-YNAB-Token is missing. If this server is reachable, unauthenticated callers can operate with the server owner’s token. Gate this behind an explicit single-user flag that defaults off.
Proposed fix
export function loadHttpConfig(): HttpConfig {
const allowedHosts = parseList(process.env['ALLOWED_HOSTS']);
const allowedOrigins = parseList(process.env['ALLOWED_ORIGINS']);
+ const allowEnvTokenFallback = parseBoolean(
+ process.env['HTTP_ALLOW_ENV_TOKEN_FALLBACK'],
+ false,
+ 'HTTP_ALLOW_ENV_TOKEN_FALLBACK'
+ );
return {
@@
- fallbackAccessToken: process.env['YNAB_ACCESS_TOKEN'] || undefined,
+ fallbackAccessToken: allowEnvTokenFallback ? process.env['YNAB_ACCESS_TOKEN'] || undefined : undefined,📝 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.
| fallbackAccessToken: process.env['YNAB_ACCESS_TOKEN'] || undefined, | |
| export function loadHttpConfig(): HttpConfig { | |
| const allowedHosts = parseList(process.env['ALLOWED_HOSTS']); | |
| const allowedOrigins = parseList(process.env['ALLOWED_ORIGINS']); | |
| const allowEnvTokenFallback = parseBoolean( | |
| process.env['HTTP_ALLOW_ENV_TOKEN_FALLBACK'], | |
| false, | |
| 'HTTP_ALLOW_ENV_TOKEN_FALLBACK' | |
| ); | |
| return { | |
| fallbackAccessToken: allowEnvTokenFallback ? process.env['YNAB_ACCESS_TOKEN'] || undefined : undefined, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/environment.ts` at line 106, The current environment config in
environment.ts always sets fallbackAccessToken from YNAB_ACCESS_TOKEN, which
lets downstream HTTP setup silently use the owner’s token when X-YNAB-Token is
absent. Update the environment loading logic to require an explicit opt-in flag
that defaults to off, and only populate fallbackAccessToken when that
single-user flag is enabled. Make the change in the config path that defines
fallbackAccessToken so the HTTP initialization code will no longer accept the
env token by default.
| const app = express(); | ||
| app.use(express.json({ limit: '4mb' })); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add baseline security headers before exposing the Express app.
This is a new remote HTTP surface; add helmet() before the routes so default browser-facing headers are not omitted. Static analysis also flagged this Express app as missing Helmet.
Suggested hardening
+import helmet from 'helmet';
import express, { type Request, type Response } from 'express'; const app = express();
+ app.use(helmet());
app.use(express.json({ limit: '4mb' }));📝 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.
| const app = express(); | |
| app.use(express.json({ limit: '4mb' })); | |
| import helmet from 'helmet'; | |
| import express, { type Request, type Response } from 'express'; | |
| const app = express(); | |
| app.use(helmet()); | |
| app.use(express.json({ limit: '4mb' })); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http.ts` around lines 31 - 32, The Express app initialization in the http
setup is missing baseline security headers. Update the app configuration around
the express() and app.use(...) setup to add helmet() before registering routes
or other middleware that expose the HTTP surface, so the default browser-facing
security headers are applied. Use the existing app initialization in the HTTP
entrypoint to place Helmet early in the middleware chain.
Source: Linters/SAST tools
| // Active sessions: session id -> transport. Each transport is wired to its own | ||
| // per-user MCP Server instance. | ||
| const transports = new Map<string, StreamableHTTPServerTransport>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound session lifetime and capacity.
Every initialize creates a transport plus per-user server/client/cache/rate-limiter/audit-log, and the Map is only cleaned when the transport closes. A client can create sessions and never send DELETE, causing unbounded memory/resource growth. Add idle TTL cleanup and/or a max-session cap.
Also applies to: 71-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http.ts` around lines 34 - 36, The active-session Map in http.ts can grow
without bound because transports and their per-user resources are only removed
on close. Add explicit session lifecycle controls around the transport handling
in the initialize flow and cleanup path: enforce an idle TTL for inactive
sessions and/or a maximum session cap, and evict old entries from transports
before creating new ones. Make sure the cleanup logic updates the associated
per-user server/client/cache/rate-limiter/audit-log resources consistently when
a session expires or is rejected.
| await transport.handleRequest( | ||
| req as unknown as IncomingMessage, | ||
| res as unknown as ServerResponse, | ||
| req.body | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/http.tsRepository: auzroz/ynab-mcp
Length of output: 5446
🏁 Script executed:
rg -n "handleRequest\(|app\.use\(|error middleware|jsonRpcError|jsonrpc|express\(\)" src . --glob '!node_modules'Repository: auzroz/ynab-mcp
Length of output: 1709
🏁 Script executed:
cat -n package.json
printf '\n---\n'
sed -n '1,220p' tests/unit/http.test.tsRepository: auzroz/ynab-mcp
Length of output: 5629
🌐 Web query:
Express 5 async route handler rejected promise default error middleware stack trace production JSON-RPC transport handleRequest StreamableHTTPServerTransport error handling
💡 Result:
In Express 5, support for asynchronous route handlers and middleware is native [1][2]. If an async function throws an error or returns a rejected promise, Express 5 automatically catches the error and forwards it to your error handling middleware by calling next(err) [1][3][4]. This eliminates the need for manual try/catch blocks or helper libraries like express-async-handler [2][5]. Regarding the NodeStreamableHTTPServerTransport (used in the Model Context Protocol SDK): 1. Error Handling and Observability: The transport includes an onerror callback property [6][7]. Recent updates ensure that all transport-level errors—such as JSON-RPC parse errors, session validation failures, or protocol mismatches—trigger this callback, preventing them from being silently swallowed [8][9]. 2. Integration with Express: When using NodeStreamableHTTPServerTransport within an Express route, you delegate request handling via transport.handleRequest(req, res, req.body) [6][7]. Because Express 5 natively handles rejected promises, if an error occurs within the transport or the handler chain, it will be forwarded to your Express error middleware [1][4]. 3. Production Considerations: In production, Express error middleware typically logs the error (including the stack trace if desired) and sends a structured JSON response [2][5]. To avoid exposing sensitive stack traces to clients, ensure your error middleware explicitly checks the environment (e.g., if (process.env.NODE_ENV!== 'production')) before including stack details in the response [2]. For the MCP transport, the onerror callback provides a dedicated hook to log internal transport errors for observability without necessarily exposing them to the end user via the JSON-RPC response [6][8][9].
Citations:
- 1: https://expressjs.com/en/5x/guide/error-handling/
- 2: https://dev.to/mahmud007/goodbye-asynchandler-native-async-support-in-express-5-2o9p
- 3: https://blog.logrocket.com/express-js-5-migration-guide/
- 4: https://expressjs.com/en/guide/migrating-5/
- 5: https://nodewire.net/express-async-error-handling/
- 6: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_node.streamableHttp.NodeStreamableHTTPServerTransport.html
- 7: https://github.com/modelcontextprotocol/typescript-sdk/blob/2c0c481c/packages/middleware/node/src/streamableHttp.ts
- 8: fix: call onerror callback for all error responses in StreamableHTTPServerTransport modelcontextprotocol/typescript-sdk#1687
- 9: fix(server): call onerror callback for all transport errors modelcontextprotocol/typescript-sdk#1433
🌐 Web query:
ModelContextProtocol NodeStreamableHTTPServerTransport handleRequest throws rejects onerror JSON-RPC error response
💡 Result:
In the Model Context Protocol (MCP) TypeScript SDK, the NodeStreamableHTTPServerTransport manages JSON-RPC error responses by centralizing them through the underlying WebStandardStreamableHTTPServerTransport. Key mechanisms regarding error handling and the onerror callback include: 1. Centralized Error Responses: Transport-level errors (such as invalid JSON-RPC messages, unsupported content types, or validation failures) are handled by a private createJsonErrorResponse method [1]. This method generates a standardized JSON-RPC error object with the appropriate HTTP status code [1]. 2. Integration with onerror: To prevent transport errors from being silently swallowed, recent updates (specifically those tracking towards V2 and relevant PRs) have ensured that the onerror callback is triggered whenever an error response is generated [2][3]. The onerror callback is intended for observing out-of-band errors for logging or observability without altering the HTTP response sent to the client [4]. 3. Handling Mechanism in handleRequest: The NodeStreamableHTTPServerTransport.handleRequest method acts as a bridge, converting Node.js IncomingMessage and ServerResponse objects into Web Standard Request/Response objects before delegating to the web-standard transport [5]. If an error occurs during this process, the underlying transport's error-handling logic ensures that the onerror handler is invoked (if defined by the user) before the JSON-RPC error response is returned to the client [2][3]. 4. Error Fidelity: Recent improvements have enhanced error reporting by including an optional cause parameter, allowing the onerror callback to receive the original exception (if available) rather than just a generic error message, improving the utility of error logs [3]. In summary, when handleRequest encounters a condition requiring a JSON-RPC error response, it uses the transport's internal error-handling logic to both send the error to the client and notify the registered onerror listener [2][3]. It does not typically throw or reject at the transport level to report these; rather, it performs these asynchronous operations and signals exceptional conditions via the callback [6][4].
Citations:
- 1: https://github.com/modelcontextprotocol/typescript-sdk/blob/327243ce/packages/server/src/server/streamableHttp.ts
- 2: fix(server): call onerror callback for all transport errors modelcontextprotocol/typescript-sdk#1433
- 3: fix: call onerror callback for all error responses in StreamableHTTPServerTransport modelcontextprotocol/typescript-sdk#1687
- 4: https://ts.sdk.modelcontextprotocol.io/v2/documents/Documents.Server_Guide.html
- 5: https://github.com/modelcontextprotocol/typescript-sdk/blob/327243ce/packages/middleware/node/src/streamableHttp.ts
- 6: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_node.streamableHttp.NodeStreamableHTTPServerTransport.html
Handle /mcp async failures explicitly. server.connect() and transport.handleRequest() run inside async Express handlers here, so unexpected rejections will hit Express’ default error handler and can return a non-JSON-RPC error page instead of a sanitized protocol error. Wrap these paths (or add error middleware) and log only sanitized context before returning a generic jsonRpcError(...).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http.ts` around lines 93 - 97, The async `/mcp` request path in `http.ts`
is letting rejections from `server.connect()` and `transport.handleRequest()`
bubble into Express’ default error handler. Update the handler around
`handleRequest` (and the `server.connect` call site if it is in the same async
route) to catch failures explicitly, log only sanitized request context, and
return a generic `jsonRpcError(...)` response instead of a raw Express error
page. Use the existing `transport.handleRequest` and `server.connect` flow as
the place to add this guarded error handling.
Sources: Coding guidelines, Path instructions
| export function startHttpServer(config: HttpConfig): ReturnType<express.Express['listen']> { | ||
| const app = createHttpApp(config); | ||
| const httpServer = app.listen(config.port, () => { | ||
| console.error(`YNAB MCP Server (HTTP) listening on port ${config.port}`); | ||
| console.error( | ||
| config.readOnly | ||
| ? 'READ-ONLY mode (write operations disabled)' | ||
| : 'WRITE operations ENABLED' | ||
| ); | ||
| console.error( | ||
| 'Interim auth: YNAB token via X-YNAB-Token header (TLS required; replaced by OAuth in a later phase)' | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Fail closed when TLS is not guaranteed.
The server only logs “TLS required” while accepting bearer tokens over the HTTP listener. Add runtime enforcement for HTTPS/proxy protocol, or reject startup unless the deployment is explicitly configured as localhost/single-user. Based on PR objectives, interim X-YNAB-Token auth must only be used over TLS.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http.ts` around lines 122 - 133, startHttpServer currently only logs that
TLS is required while still starting the HTTP listener and accepting
X-YNAB-Token auth. Add a startup guard in startHttpServer (using HttpConfig and
the app.listen path) that fails closed unless the deployment is explicitly
local/single-user or the request is guaranteed to arrive over
HTTPS/proxy-terminated TLS. Ensure interim token auth is rejected at runtime
when TLS/proxy protocol is not configured, rather than relying on the console
warning.
| if (httpServer) { | ||
| try { | ||
| await new Promise<void>((resolve) => httpServer?.close(() => resolve())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'src/index.ts' 'src/http.ts' 'src/**/*.ts' | sed -n '1,120p'
printf '\n== Outline: src/index.ts ==\n'
ast-grep outline src/index.ts --view expanded || true
printf '\n== Outline: src/http.ts ==\n'
ast-grep outline src/http.ts --view expanded || true
printf '\n== Relevant search ==\n'
rg -n "httpServer|closeAllConnections|closeIdleConnections|/mcp|SSE|stream|transport|session|shutdown|close\\(" src/index.ts src/http.ts src -g'*.ts'Repository: auzroz/ynab-mcp
Length of output: 11679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/index.ts ==\n'
sed -n '1,120p' src/index.ts | cat -n
printf '\n== src/http.ts ==\n'
sed -n '1,220p' src/http.ts | cat -nRepository: auzroz/ynab-mcp
Length of output: 9476
Do not let HTTP shutdown wait forever on active MCP streams. httpServer.close() will sit on open connections, and /mcp GET streams can remain alive indefinitely. Add a shutdown timeout and force-close lingering sockets, or expose a transport/session close hook from src/http.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 33 - 35, The shutdown path in the httpServer close
block can hang forever because httpServer.close() waits for open MCP GET
streams. Update the shutdown logic in index.ts to use a bounded timeout and
forcibly close lingering sockets, or add a transport/session close hook from
http.ts and call it before awaiting server close. Use the httpServer handling in
index.ts and the HTTP transport/session code in src/http.ts to locate the
shutdown flow.
| if (transportMode === 'http') { | ||
| httpServer = startHttpServer(loadHttpConfig()); | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the stdio EOF shutdown hook out of HTTP mode.
The global process.stdin.on('close', ...) still applies after HTTP startup. In Docker/systemd-style service runs, stdin may be closed, causing the HTTP server to shut down right after it starts. Register the stdin-close handler only for stdio mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 48 - 50, The stdin EOF shutdown hook is being
registered too broadly, so HTTP mode still exits when stdin closes. Update the
startup flow in the main entrypoint around the transportMode check so
`process.stdin.on('close', ...)` is only attached for stdio mode, and keep the
HTTP branch (`startHttpServer`) free of that handler. Make the
transport-specific setup explicit so the stdio shutdown logic cannot run after
HTTP startup.
| it('DELETE /mcp with a valid session id tears the session down', async () => { | ||
| const app = createHttpApp(makeConfig({ fallbackAccessToken: 'env-token' })); | ||
| const init = await request(app) | ||
| .post('/mcp') | ||
| .set('Accept', 'application/json, text/event-stream') | ||
| .send(initializeBody); | ||
| const sessionId = init.headers['mcp-session-id'] as string; | ||
| expect(sessionId).toBeDefined(); | ||
|
|
||
| const del = await request(app).delete('/mcp').set('mcp-session-id', sessionId); | ||
| expect(del.status).toBeLessThan(400); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider asserting the session is actually inaccessible post-teardown.
The DELETE test only checks that the teardown request itself succeeds; it doesn't confirm a follow-up request with the same session id is rejected (e.g., 400). This would strengthen confidence that transports.delete(sid) in createHttpApp actually removes the session.
♻️ Suggested addition
const del = await request(app).delete('/mcp').set('mcp-session-id', sessionId);
expect(del.status).toBeLessThan(400);
+
+ // Session should no longer be usable after teardown.
+ const after = await request(app).get('/mcp').set('mcp-session-id', sessionId);
+ expect(after.status).toBe(400);
});📝 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.
| it('DELETE /mcp with a valid session id tears the session down', async () => { | |
| const app = createHttpApp(makeConfig({ fallbackAccessToken: 'env-token' })); | |
| const init = await request(app) | |
| .post('/mcp') | |
| .set('Accept', 'application/json, text/event-stream') | |
| .send(initializeBody); | |
| const sessionId = init.headers['mcp-session-id'] as string; | |
| expect(sessionId).toBeDefined(); | |
| const del = await request(app).delete('/mcp').set('mcp-session-id', sessionId); | |
| expect(del.status).toBeLessThan(400); | |
| }); | |
| it('DELETE /mcp with a valid session id tears the session down', async () => { | |
| const app = createHttpApp(makeConfig({ fallbackAccessToken: 'env-token' })); | |
| const init = await request(app) | |
| .post('/mcp') | |
| .set('Accept', 'application/json, text/event-stream') | |
| .send(initializeBody); | |
| const sessionId = init.headers['mcp-session-id'] as string; | |
| expect(sessionId).toBeDefined(); | |
| const del = await request(app).delete('/mcp').set('mcp-session-id', sessionId); | |
| expect(del.status).toBeLessThan(400); | |
| // Session should no longer be usable after teardown. | |
| const after = await request(app).get('/mcp').set('mcp-session-id', sessionId); | |
| expect(after.status).toBe(400); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/http.test.ts` around lines 85 - 96, The DELETE /mcp test in
http.test.ts only verifies that teardown succeeds, but it does not confirm the
session was actually removed by createHttpApp/transports.delete. Update the test
around createHttpApp and the existing sessionId flow to send a follow-up request
after the DELETE and assert it is rejected (for example with a 400), using the
same session id so the teardown behavior is verified end to end.
Complete the remote MCP server: HTTP mode can now act as an OAuth 2.1 Authorization Server federated to YNAB, so many users connect their own YNAB accounts to one self-hosted instance with per-user isolation. - MCP Authorization Server (src/auth/mcp-provider.ts): OAuthServerProvider with Dynamic Client Registration + PKCE, a read-only/read-write consent screen, federated authorize/callback to YNAB, and token issuance/refresh/ revoke. Identity is the user's YNAB user id. - YNAB OAuth client (src/auth/ynab-oauth.ts): authorize URL, code exchange, refresh, and /user identity lookup; errors never leak tokens. - Per-user token resolution (src/auth/user-session.ts) with an in-memory access-token cache and transparent refresh + rotation. - Pluggable storage (src/storage): Storage interface with memory (default), sqlite (better-sqlite3), and postgres (pg) adapters; durable drivers are optional deps loaded on demand. - At-rest encryption (src/crypto.ts): AES-256-GCM for YNAB refresh tokens. - HTTP app (src/http.ts) wires mcpAuthRouter, the YNAB callback routes, and requireBearerAuth on /mcp; resolves the authenticated user to a per-user YnabClient. Interim header auth remains when OAuth vars are unset. - Per-user isolation: audit log de-singletonized and injected into YnabClient; per-user Cache/RateLimiter/AuditLog via createServerForUser. - docs/REMOTE_HOSTING.md deployer guide; .env.example, README, CHANGELOG updated; Dockerfile builds native deps and EXPOSEs the port. stdio single-user mode and the MCP tool surface are unchanged. Minor version bump to 0.3.0 (additive). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016PgWfscEzb25EXrgJg1Mt5
auzroz
changed the title
Documentation-only follow-up to #8: update the README Remote/HTTP section to reflect shipped multi-user YNAB OAuth (linking docs/REMOTE_HOSTING.md), refresh the architecture tree, and correct stale .env.example notes. No code change.
2 participants
Summary
Turns
ynab-mcpinto a self-hostable, multi-user remote MCP server. A deployer registers their own YNAB OAuth application and runs one instance for N users; each user connects their own YNAB account via OAuth, with per-user isolation. The stdio (local, single-user) path and the MCP tool surface are unchanged. Ships as a minor bump to v0.3.0 (additive).How it works — two OAuth layers
auth/module). Clients (claude.ai, Claude Desktop viamcp-remote) authenticate and receive an MCP access token.What's in this PR
MCP_TRANSPORT=httpruns an Express app exposing the MCP Streamable HTTP transport atPOST /mcp(withGET/DELETEfor streaming/teardown) plus a plainGET /health. Per-sessionServerinstances (the SDK's cross-client-leak advisory requires this).src/auth/mcp-provider.ts) —OAuthServerProviderwith DCR + PKCE, a read-only/read-write consent screen, federated authorize/callback to YNAB, and token issuance/refresh/revoke.src/auth/ynab-oauth.ts) — authorize URL, code exchange, refresh,/useridentity lookup; errors never leak tokens.src/auth/user-session.ts) — in-memory access-token cache with transparent refresh + refresh-token rotation.src/storage/) —Storageinterface withmemory(default),sqlite(better-sqlite3), andpostgres(pg) adapters; durable drivers are optional deps loaded on demand.src/crypto.ts) — AES-256-GCM for YNAB refresh tokens (ENCRYPTION_KEY).YnabClient; each user gets their ownYnabClient, cache, rate limiter, and audit log viacreateServerForUser.X-YNAB-Tokenfor a single-user remote instance.docs/REMOTE_HOSTING.mddeployer guide (register a YNAB OAuth app, redirect<PUBLIC_URL>/oauth/ynab/callback, env table, TLS/Caddy compose, client connection);.env.example,README,CHANGELOGupdated;Dockerfilebuilds native deps andEXPOSEs the port.New dependencies
express(dep);better-sqlite3andpgas optional dependencies (durable storage drivers, lazy-loaded).Testing
/health, auth gate → 401, session validation,initializehandshake, teardown); per-user isolation; crypto round-trip; all three storage adapters; the MCP OAuth provider end-to-end (DCR, consent HTML, federated authorize/callback, token issuance/refresh/revoke — YNAB network mocked); token resolver refresh + rotation; and the OAuth HTTP surface (AS metadata, bearer gate).npm run lint,npm run typecheck,npm run buildclean. stdio single-user mode unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_016PgWfscEzb25EXrgJg1Mt5