Add hosted Streamable HTTP MCP service - #1
Conversation
📝 WalkthroughWalkthroughThe project adds a hosted MCP HTTP server with health checks, rate limiting, origin rejection, hosted credential handling, SSRF protections, redirect controls, Docker packaging, Railway deployment configuration, CI image validation, and expanded setup and security documentation. ChangesHosted MCP service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant HostedHTTPServer
participant loadHostedConfig
participant StreamableHTTPServerTransport
participant McpServer
MCPClient->>HostedHTTPServer: POST /mcp
HostedHTTPServer->>HostedHTTPServer: Check Origin and rate limit
HostedHTTPServer->>loadHostedConfig: Read x-umami-* headers
HostedHTTPServer->>StreamableHTTPServerTransport: Handle MCP request
StreamableHTTPServerTransport->>McpServer: Dispatch request
McpServer-->>MCPClient: Return MCP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
30-30: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable checkout credential persistence.
This job only builds the image, so keeping
GITHUB_TOKENin.git/configis unnecessary. Setpersist-credentials: falseonactions/checkout.Proposed change
- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/ci.yml at line 30, Update the actions/checkout@v4 step in the CI workflow to set persist-credentials to false, ensuring the GitHub token is not retained in the repository’s Git configuration while preserving the existing checkout behavior.Source: Linters/SAST tools
test/package.test.ts (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the production install mode too.
This regex only matches the builder-stage install and would not catch removal of
npm ci --omit=dev --ignore-scriptsfrom the runtime image. Add a separate assertion for the production-stage command.🤖 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 `@test/package.test.ts` at line 29, Extend the test around the existing dockerfile assertion to separately verify the production-stage install command includes both --omit=dev and --ignore-scripts. Keep the current builder-stage assertion and add a distinct match targeting the runtime npm ci command.src/config.ts (1)
171-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "publicly routable host" message into a constant.
The literal
'Hosted Umami URLs must use a publicly routable host'is duplicated at Line 151, Line 218, and Line 224, and this block re-derives coupling to it viacause?.message.includes('publicly routable host')(Line 195). If the wording ever changes in one spot, the substring match silently stops unwrapping the real cause, and callers get a generic "fetch failed"TypeErrorinstead of the actionable message — degrading error clarity for a security-relevant failure path. Consider a shared constant, or better, matching onerror.code === 'ENOTFOUND'(already set at Line 152) instead of message text.♻️ Proposed refactor
+const UNSAFE_HOST_CODE = 'ENOTFOUND'; + function createSafeLookup(lookup: HostedLookup): LookupFunction { return (hostname, options, callback) => { void lookup(hostname).then( (addresses) => { if (...) { const error = Object.assign( new Error('Hosted Umami URLs must use a publicly routable host'), - { code: 'ENOTFOUND' }, + { code: UNSAFE_HOST_CODE }, );} catch (error) { - const cause = - error instanceof Error ? (error.cause as Error | undefined) : undefined; - if (cause?.message.includes('publicly routable host')) { + const cause = + error instanceof Error + ? (error.cause as NodeJS.ErrnoException | undefined) + : undefined; + if (cause?.code === UNSAFE_HOST_CODE) { throw cause; } throw error; }🤖 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.ts` around lines 171 - 201, Update createHostedFetch and the related hosted-URL validation code to use a shared constant for the “publicly routable host” message, or preferably identify the wrapped ENOTFOUND error via its existing error.code instead of matching message text. Ensure the security-related cause is still unwrapped and propagated while keeping the existing behavior for unrelated fetch errors.test/hosted-config.test.ts (1)
68-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing happy-path coverage for
assertHostedUrlIsSafe.All tests here exercise rejection paths (non-HTTPS, blocked literals, unsafe DNS results, embedded credentials, unsafe connect-time DNS). There's no test asserting
assertHostedUrlIsSaferesolves successfully for a genuinely public host/IP, so a regression that made the function always throw (or always pass a safe check incorrectly) on the success path wouldn't be caught.🤖 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 `@test/hosted-config.test.ts` around lines 68 - 92, Add a happy-path test for assertHostedUrlIsSafe that awaits a genuinely public HTTPS host or IP and verifies it resolves without throwing. Keep the existing rejection coverage unchanged and use a deterministic publicly routable target so the test does not depend on unsafe or variable DNS behavior.
🤖 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/app.ts`:
- Around line 164-167: Update the close function in src/app.ts (lines 164-167)
to call httpServer.closeAllConnections?.() before httpServer.close(). In
src/http.ts (lines 15-25), add a 5-second hard timeout around the shutdown
handler’s await server.close() that exits the process if closing stalls.
- Around line 37-59: Ensure handleHostedMcpRequest cannot hang indefinitely by
adding a request-level timeout that covers transport.handleRequest and the
underlying hostedFetch/UmamiClient calls. Prefer propagating an
AbortSignal.timeout through UmamiClient into hostedFetch; otherwise enforce
equivalent timeout behavior around the handler while preserving cleanup in the
existing finally block.
- Around line 89-114: Update the rate limiting flow around the rateLimits Map
and clientAddress derivation to periodically remove entries whose resetAt
timestamps have expired, preventing unbounded growth. Do not unconditionally
trust x-forwarded-for: only use it when the request is verified as coming
through a configured trusted proxy; otherwise rate-limit by
request.socket.remoteAddress. Document the trusted-proxy deployment requirement
or reuse an existing trusted-proxy configuration.
In `@src/config.ts`:
- Around line 203-227: Update assertHostedUrlIsSafe so a hostname lookup
returning an empty addresses array is rejected as unsafe, matching
createSafeLookup. In the !family branch, validate addresses.length before or
alongside the blocked-address check and throw the existing publicly routable
host error for empty results; preserve the current behavior for non-empty safe
results.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 30: Update the actions/checkout@v4 step in the CI workflow to set
persist-credentials to false, ensuring the GitHub token is not retained in the
repository’s Git configuration while preserving the existing checkout behavior.
In `@src/config.ts`:
- Around line 171-201: Update createHostedFetch and the related hosted-URL
validation code to use a shared constant for the “publicly routable host”
message, or preferably identify the wrapped ENOTFOUND error via its existing
error.code instead of matching message text. Ensure the security-related cause
is still unwrapped and propagated while keeping the existing behavior for
unrelated fetch errors.
In `@test/hosted-config.test.ts`:
- Around line 68-92: Add a happy-path test for assertHostedUrlIsSafe that awaits
a genuinely public HTTPS host or IP and verifies it resolves without throwing.
Keep the existing rejection coverage unchanged and use a deterministic publicly
routable target so the test does not depend on unsafe or variable DNS behavior.
In `@test/package.test.ts`:
- Line 29: Extend the test around the existing dockerfile assertion to
separately verify the production-stage install command includes both --omit=dev
and --ignore-scripts. Keep the current builder-stage assertion and add a
distinct match targeting the runtime npm ci command.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80099476-1303-4e3b-a7b8-7871c9a85190
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.dockerignore.github/workflows/ci.ymlDockerfileREADME.mdpackage.jsonrailway.tomlsrc/app.tssrc/config.tssrc/http.tssrc/umami-client.tstest/app.test.tstest/hosted-config.test.tstest/http.test.tstest/package.test.tstest/umami-client.test.ts
| async function handleHostedMcpRequest( | ||
| request: Parameters<StreamableHTTPServerTransport['handleRequest']>[0], | ||
| response: Parameters<StreamableHTTPServerTransport['handleRequest']>[1], | ||
| ): Promise<void> { | ||
| const config = loadHostedConfig(request.headers); | ||
| await assertHostedUrlIsSafe(config.apiUrl); | ||
|
|
||
| const server = createUmamiMcpServer( | ||
| new UmamiClient(config, { fetch: hostedFetch, redirect: 'error' }), | ||
| ); | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, | ||
| enableJsonResponse: true, | ||
| }); | ||
|
|
||
| try { | ||
| await server.connect(transport); | ||
| await transport.handleRequest(request, response); | ||
| } finally { | ||
| await transport.close(); | ||
| await server.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on handleHostedMcpRequest — a slow downstream Umami API hangs the request indefinitely.
If the Umami backend is slow or unresponsive, transport.handleRequest (and the underlying hostedFetch) will block forever, tying up the Node.js request handler and socket. Consider adding an AbortSignal.timeout() to the outbound fetch or a request-level timeout.
⏱️ Proposed fix: add a request-level timeout
async function handleHostedMcpRequest(
request: Parameters<StreamableHTTPServerTransport['handleRequest']>[0],
response: Parameters<StreamableHTTPServerTransport['handleRequest']>[1],
): Promise<void> {
+ const timeout = AbortSignal.timeout(30_000);
const config = loadHostedConfig(request.headers);
await assertHostedUrlIsSafe(config.apiUrl);
const server = createUmamiMcpServer(
- new UmamiClient(config, { fetch: hostedFetch, redirect: 'error' }),
+ new UmamiClient(config, { fetch: hostedFetch, redirect: 'error', signal: timeout }),
);This requires UmamiClient to accept and forward an AbortSignal to its fetch calls. Alternatively, wrap the entire handler in a Promise.race with a timeout.
🤖 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/app.ts` around lines 37 - 59, Ensure handleHostedMcpRequest cannot hang
indefinitely by adding a request-level timeout that covers
transport.handleRequest and the underlying hostedFetch/UmamiClient calls. Prefer
propagating an AbortSignal.timeout through UmamiClient into hostedFetch;
otherwise enforce equivalent timeout behavior around the handler while
preserving cleanup in the existing finally block.
| const rateLimits = new Map<string, { count: number; resetAt: number }>(); | ||
|
|
||
| const httpServer: Server = createServer((request, response) => { | ||
| if (request.url === '/health') { | ||
| response.statusCode = 200; | ||
| response.setHeader('content-type', 'application/json'); | ||
| response.end(JSON.stringify({ status: 'ok' })); | ||
| return; | ||
| } | ||
|
|
||
| if (request.url === '/mcp' && request.method === 'POST') { | ||
| const forwardedFor = request.headers['x-forwarded-for']; | ||
| const forwardedValue = Array.isArray(forwardedFor) | ||
| ? forwardedFor.at(-1) | ||
| : forwardedFor; | ||
| const clientAddress = | ||
| forwardedValue?.split(',').at(-1)?.trim() || | ||
| request.socket.remoteAddress || | ||
| 'unknown'; | ||
| const now = Date.now(); | ||
| const current = rateLimits.get(clientAddress); | ||
| const rateLimit = | ||
| !current || current.resetAt <= now | ||
| ? { count: 1, resetAt: now + 60_000 } | ||
| : { count: current.count + 1, resetAt: current.resetAt }; | ||
| rateLimits.set(clientAddress, rateLimit); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Rate limit map grows unboundedly and trusts x-forwarded-for unconditionally.
The rateLimits Map is never cleaned up — expired entries persist forever. Each unique clientAddress adds a permanent entry. Since x-forwarded-for is trusted without verifying the request came through a trusted proxy, an attacker not behind Railway's proxy can spoof a unique XFF value per request to both bypass rate limiting and exhaust memory.
🔒 Proposed fix: periodic cleanup + document proxy requirement
const maxRequestsPerMinute = options.maxRequestsPerMinute ?? 120;
const rateLimits = new Map<string, { count: number; resetAt: number }>();
+
+ // Periodically evict expired entries to prevent unbounded memory growth
+ const cleanup = setInterval(() => {
+ const now = Date.now();
+ for (const [key, value] of rateLimits) {
+ if (value.resetAt <= now) {
+ rateLimits.delete(key);
+ }
+ }
+ }, 60_000).unref();Additionally, consider documenting that the service must be deployed behind a trusted reverse proxy (e.g., Railway) that overwrites x-forwarded-for, or adding a configurable trusted-proxy check.
📝 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 rateLimits = new Map<string, { count: number; resetAt: number }>(); | |
| const httpServer: Server = createServer((request, response) => { | |
| if (request.url === '/health') { | |
| response.statusCode = 200; | |
| response.setHeader('content-type', 'application/json'); | |
| response.end(JSON.stringify({ status: 'ok' })); | |
| return; | |
| } | |
| if (request.url === '/mcp' && request.method === 'POST') { | |
| const forwardedFor = request.headers['x-forwarded-for']; | |
| const forwardedValue = Array.isArray(forwardedFor) | |
| ? forwardedFor.at(-1) | |
| : forwardedFor; | |
| const clientAddress = | |
| forwardedValue?.split(',').at(-1)?.trim() || | |
| request.socket.remoteAddress || | |
| 'unknown'; | |
| const now = Date.now(); | |
| const current = rateLimits.get(clientAddress); | |
| const rateLimit = | |
| !current || current.resetAt <= now | |
| ? { count: 1, resetAt: now + 60_000 } | |
| : { count: current.count + 1, resetAt: current.resetAt }; | |
| rateLimits.set(clientAddress, rateLimit); | |
| const rateLimits = new Map<string, { count: number; resetAt: number }>(); | |
| // Periodically evict expired entries to prevent unbounded memory growth | |
| const cleanup = setInterval(() => { | |
| const now = Date.now(); | |
| for (const [key, value] of rateLimits) { | |
| if (value.resetAt <= now) { | |
| rateLimits.delete(key); | |
| } | |
| } | |
| }, 60_000).unref(); | |
| const httpServer: Server = createServer((request, response) => { | |
| if (request.url === '/health') { | |
| response.statusCode = 200; | |
| response.setHeader('content-type', 'application/json'); | |
| response.end(JSON.stringify({ status: 'ok' })); | |
| return; | |
| } | |
| if (request.url === '/mcp' && request.method === 'POST') { | |
| const forwardedFor = request.headers['x-forwarded-for']; | |
| const forwardedValue = Array.isArray(forwardedFor) | |
| ? forwardedFor.at(-1) | |
| : forwardedFor; | |
| const clientAddress = | |
| forwardedValue?.split(',').at(-1)?.trim() || | |
| request.socket.remoteAddress || | |
| 'unknown'; | |
| const now = Date.now(); | |
| const current = rateLimits.get(clientAddress); | |
| const rateLimit = | |
| !current || current.resetAt <= now | |
| ? { count: 1, resetAt: now + 60_000 } | |
| : { count: current.count + 1, resetAt: current.resetAt }; | |
| rateLimits.set(clientAddress, rateLimit); |
🤖 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/app.ts` around lines 89 - 114, Update the rate limiting flow around the
rateLimits Map and clientAddress derivation to periodically remove entries whose
resetAt timestamps have expired, preventing unbounded growth. Do not
unconditionally trust x-forwarded-for: only use it when the request is verified
as coming through a configured trusted proxy; otherwise rate-limit by
request.socket.remoteAddress. Document the trusted-proxy deployment requirement
or reuse an existing trusted-proxy configuration.
| close: () => | ||
| new Promise<void>((resolve, reject) => { | ||
| httpServer.close((error) => (error ? reject(error) : resolve())); | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Graceful shutdown can hang on lingering keep-alive connections. httpServer.close() waits for all existing connections to finish, but HTTP keep-alive connections can keep it pending indefinitely. The shutdown handler in http.ts awaits close() with no force-exit timeout, so the process hangs on SIGINT/SIGTERM.
src/app.ts#L164-L167: callhttpServer.closeAllConnections?.()beforehttpServer.close()to force-close idle keep-alive connections.src/http.ts#L15-L25: add a hard timeout (e.g.,setTimeout(() => process.exit(1), 5000)) aroundawait server.close()so the process exits even ifclose()stalls.
📍 Affects 2 files
src/app.ts#L164-L167(this comment)src/http.ts#L15-L25
🤖 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/app.ts` around lines 164 - 167, Update the close function in src/app.ts
(lines 164-167) to call httpServer.closeAllConnections?.() before
httpServer.close(). In src/http.ts (lines 15-25), add a 5-second hard timeout
around the shutdown handler’s await server.close() that exits the process if
closing stalls.
| export async function assertHostedUrlIsSafe( | ||
| apiUrl: string, | ||
| lookup: HostedLookup = lookupAll, | ||
| ): Promise<void> { | ||
| const url = new URL(apiUrl); | ||
| if (url.protocol !== 'https:') { | ||
| throw new Error('Hosted Umami URLs must use HTTPS'); | ||
| } | ||
| if (url.username || url.password) { | ||
| throw new Error('Hosted Umami URLs must not contain embedded credentials'); | ||
| } | ||
|
|
||
| const hostname = url.hostname.replace(/^\[|\]$/g, ''); | ||
| const family = isIP(hostname); | ||
| if (family && isBlockedAddress(hostname, family)) { | ||
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | ||
| } | ||
|
|
||
| if (!family) { | ||
| const addresses = await lookup(hostname); | ||
| if (addresses.some(({ address, family }) => isBlockedAddress(address, family))) { | ||
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
assertHostedUrlIsSafe doesn't reject an empty DNS-resolution result, unlike createSafeLookup.
createSafeLookup (Line 145) explicitly treats addresses.length === 0 as unsafe. Here, addresses.some(...) on an empty array is false, so a lookup that resolves to zero addresses would pass validation. The default lookupAll won't hit this (Node's dns.lookup rejects on no records), but HostedLookup is a public, injectable type/parameter, so any future custom lookup implementation returning [] would silently bypass this safety check.
🛡️ Proposed fix
if (!family) {
const addresses = await lookup(hostname);
- if (addresses.some(({ address, family }) => isBlockedAddress(address, family))) {
+ if (
+ addresses.length === 0 ||
+ addresses.some(({ address, family }) => isBlockedAddress(address, family))
+ ) {
throw new Error('Hosted Umami URLs must use a publicly routable host');
}
}📝 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 async function assertHostedUrlIsSafe( | |
| apiUrl: string, | |
| lookup: HostedLookup = lookupAll, | |
| ): Promise<void> { | |
| const url = new URL(apiUrl); | |
| if (url.protocol !== 'https:') { | |
| throw new Error('Hosted Umami URLs must use HTTPS'); | |
| } | |
| if (url.username || url.password) { | |
| throw new Error('Hosted Umami URLs must not contain embedded credentials'); | |
| } | |
| const hostname = url.hostname.replace(/^\[|\]$/g, ''); | |
| const family = isIP(hostname); | |
| if (family && isBlockedAddress(hostname, family)) { | |
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | |
| } | |
| if (!family) { | |
| const addresses = await lookup(hostname); | |
| if (addresses.some(({ address, family }) => isBlockedAddress(address, family))) { | |
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | |
| } | |
| } | |
| } | |
| export async function assertHostedUrlIsSafe( | |
| apiUrl: string, | |
| lookup: HostedLookup = lookupAll, | |
| ): Promise<void> { | |
| const url = new URL(apiUrl); | |
| if (url.protocol !== 'https:') { | |
| throw new Error('Hosted Umami URLs must use HTTPS'); | |
| } | |
| if (url.username || url.password) { | |
| throw new Error('Hosted Umami URLs must not contain embedded credentials'); | |
| } | |
| const hostname = url.hostname.replace(/^\[|\]$/g, ''); | |
| const family = isIP(hostname); | |
| if (family && isBlockedAddress(hostname, family)) { | |
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | |
| } | |
| if (!family) { | |
| const addresses = await lookup(hostname); | |
| if ( | |
| addresses.length === 0 || | |
| addresses.some(({ address, family }) => isBlockedAddress(address, family)) | |
| ) { | |
| throw new Error('Hosted Umami URLs must use a publicly routable host'); | |
| } | |
| } | |
| } |
🤖 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.ts` around lines 203 - 227, Update assertHostedUrlIsSafe so a
hostname lookup returning an empty addresses array is rejected as unsafe,
matching createSafeLookup. In the !family branch, validate addresses.length
before or alongside the blocked-address check and throw the existing publicly
routable host error for empty results; preserve the current behavior for
non-empty safe results.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests