feat: add an MCP service that supports converting Swagger documents i…#175
Open
gargameljyh wants to merge 11 commits into
Open
feat: add an MCP service that supports converting Swagger documents i…#175gargameljyh wants to merge 11 commits into
gargameljyh wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a playground Swagger→MCP service: project scaffolding, env/config loader and bootstrap, OpenAPI input security and parser, operation extraction and schema→Zod conversion, API executor with timeout/cookie handling, MCP HTTP/stdio transports with per-session registry and assets API, meta-tools and dynamic registry, AI-SDK adapters, and playground integration. ChangesSwagger MCP Server
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant SessionMgr
participant McpServer
participant SwaggerRegistry
Client->>ExpressApp: POST /mcp (init or tool call)
ExpressApp->>SessionMgr: lookup or create session (mcp-session-id)
SessionMgr->>McpServer: forward request via transport
McpServer->>SwaggerRegistry: parse_swagger / list_tools / tool handler
SwaggerRegistry->>SwaggerRegistry: parse spec, extract ops, register tools
SwaggerRegistry-->>McpServer: registration result or tool output
McpServer-->>SessionMgr: execution result
SessionMgr-->>ExpressApp: response
ExpressApp-->>Client: final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 12
🤖 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 `@sites/playground/swagger-mcp-server/.env.example`:
- Line 12: Fix the typo in the .env.example comment: change the misspelled
"assests" to "assets" in the comment that starts with "# Assets API（读取 assests
目录）" so it matches routes and API naming; ensure the updated comment reads "#
Assets API（读取 assets 目录）" to avoid confusion.
In `@sites/playground/swagger-mcp-server/src/config.ts`:
- Around line 11-13: The config currently sets port via Number(process.env.PORT
?? 3100) which can yield NaN or out-of-range values; update the port logic in
config.ts (the port export) to parse and validate process.env.PORT (e.g., use
parseInt, check isFinite and integer, and clamp/validate to the TCP port range
1–65535) and fall back to the default 3100 when invalid; ensure the resulting
port is a safe integer before returning it in the config so listeners never
receive NaN or an invalid port.
In `@sites/playground/swagger-mcp-server/src/server/assets-api.ts`:
- Around line 44-48: The API is leaking server internals by returning ASSETS_DIR
and raw error messages; update the response in the handler that currently does
res.json({ directory: ASSETS_DIR, files }) and the catch blocks (where
res.status(500).json({ error: message }) is used) to avoid exposing absolute
paths or full error details: return a sanitized directory identifier (e.g.,
path.basename(ASSETS_DIR) or a fixed logical name like "assets") and only return
a generic error message to clients (e.g., { error: "internal server error" });
log the full error server-side using the existing logger/console (preserve error
via console.error or processLogger.error) so debugging info is retained but not
sent to clients; make the same change for the other occurrence that uses the
same catch pattern.
- Line 6: ASSETS_DIR currently exposes an absolute path and raw error messages;
change the code so the server still resolves the assets folder (keep the
existing resolve(dirname(fileURLToPath(import.meta.url)), '../../assests')
logic) but never return that value to clients (remove any response field like
directory: ASSETS_DIR). In handlers that stat/read files (e.g., the routes that
use fs.stat/fs.readFile), validate and sanitize requested paths to prevent path
traversal (use path.join with the user-supplied name and assert
path.relative(ASSETS_DIR, joined) does not start with '..'), catch filesystem
errors and map them to generic HTTP responses (404 for not found, 400 for
invalid request, 500 for other errors) and return non-sensitive messages (e.g.,
"file not found" / "internal server error") instead of error.message. Also keep
the folder name exactly as "assests" to match disk and ensure any logging of
errors stays server-side (log full error with processLogger or console) without
sending details to the client.
In `@sites/playground/swagger-mcp-server/src/server/http-transport.ts`:
- Around line 14-37: transports is unbounded; add a max session limit and
TTL-based expiry to avoid memory growth: introduce a MaxSessions constant and a
per-session metadata map (e.g., lastAccessed/created timestamps) and enforce
when creating a new StreamableHTTPServerTransport in mcpPostHandler — if
transports.size >= MaxSessions prune expired entries first and then the
oldest/least-recently-used sessions before inserting; also set a per-transport
inactivity timeout (or reuse transport.onclose) to delete its entry when TTL
elapses; apply the same pruning/expiry logic where transports is
accessed/created (including the code paths around the existing transport.onclose
and the referenced lines 81-83).
In `@sites/playground/swagger-mcp-server/src/server/register-meta-tools.ts`:
- Around line 89-93: Existing dynamic tools are being cleared before the new
Swagger spec is validated, so if parseSwaggerInput(swagger) fails the server
ends up with no dynamic tools; change the flow to call
parseSwaggerInput(swagger) first, verify it succeeds, and only then call
registry.clearDynamicTools() when replaceExisting is true and proceed to
register the new tools (e.g., in the same block that currently follows
parseSwaggerInput); ensure errors from parseSwaggerInput are caught and
returned/propagated without mutating registry state so existing tools remain
intact.
In `@sites/playground/swagger-mcp-server/src/swagger/extract-operations.ts`:
- Around line 48-58: The current logic picks jsonContent by searching content
entries but then picks contentType by blindly taking the first content key,
which can mismatch; change the selection to capture the matched content key when
finding the JSON entry (e.g., store the result of Object.entries(...).find(...)
as jsonEntry and set jsonContent = jsonEntry?.[1]) and then compute contentType
from that same key (jsonEntry?.[0]) falling back to application/json or the
first key only if no json entry was found; update references to jsonContent and
contentType accordingly so they always correspond to the same content entry from
requestBody.
In `@sites/playground/swagger-mcp-server/src/swagger/http-executor.ts`:
- Line 86: The fetch call using "url" and "init" (result assigned to "response")
needs timeout protection to avoid indefinite hangs—wrap the request with an
AbortController: create a controller, attach controller.signal to the fetch init
(or to a shallow-cloned init), start a timer (e.g., HTTP_TIMEOUT_MS) that calls
controller.abort() on expiry, then clear the timer after the await fetch
completes; ensure you catch AbortError and throw or convert to a useful timeout
error. Add a named timeout constant (e.g., HTTP_TIMEOUT_MS) and update the fetch
invocation to use the controller.signal so the request is canceled on timeout.
- Around line 49-61: The switch currently ignores cookie params; add
cookieArgs[param.name] = value in the 'cookie' case and then, in the
request-building code (where pathArgs, queryArgs and headerArgs are used to
create the outgoing HTTP request), serialize cookieArgs into a single "Cookie"
header (e.g., join name=value pairs with "; ") and merge it into headerArgs
(preserving any existing Cookie header). Use proper URI-encoding for cookie
values and ensure headerArgs['Cookie'] is set before the HTTP client is invoked.
In `@sites/playground/swagger-mcp-server/src/swagger/parse-swagger-input.ts`:
- Around line 31-46: The readSpecContent function presently allows arbitrary
HTTP(S) fetches and filesystem reads, enabling SSRF and local file access; fix
this by validating and restricting sources: for URL sources (detected by the
/^https?:\/\// check and fetch usage) enforce an allowlist of trusted
hostnames/origins and perform IP resolution to block private/reserved ranges
before calling fetch, and for file sources (code paths that call isAbsolute,
resolve and readFile) disallow absolute paths and paths that escape a safe base
directory (use a configured safeDocsDir and reject paths with ../ after path
normalization); also keep isInlineSwaggerContent handling as-is and return clear
errors when a source is rejected.
- Around line 36-41: The remote fetch can hang; wrap fetch(source) with an
AbortController and a timeout: create an AbortController, start a timer (e.g.
configurable constant like SWAGGER_FETCH_TIMEOUT_MS) that calls
controller.abort() after the timeout, pass controller.signal into fetch(source,
{ signal }), then clear the timer after fetch completes; catch the abort error
and throw a clear timeout/abort Error. Update the call site where fetch(source)
is used in parse-swagger-input (the fetch(...) block) to use this
AbortController pattern so remote Swagger fetches are aborted after the timeout.
In `@sites/playground/swagger-mcp-server/src/swagger/schema-to-zod.ts`:
- Around line 64-73: The code currently returns a z.record when
schema.additionalProperties is present, dropping explicit properties in shape;
instead, keep the base object (built from shape or a record when there are no
properties) and apply a catchall for additionalProperties: when
additionalProperties === true use .catchall(z.unknown()), and when it is an
object use .catchall(schemaToZod(schema.additionalProperties, true)); ensure you
still wrap the final result with withDescription and reference the symbols
schemaToZod, withDescription, shape, and schema.additionalProperties when making
the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0272c1ed-9cd2-48c2-8bb0-cc924dc3ea71
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
sites/playground/swagger-mcp-server/.env.examplesites/playground/swagger-mcp-server/assests/aad.jsonsites/playground/swagger-mcp-server/package.jsonsites/playground/swagger-mcp-server/src/config.tssites/playground/swagger-mcp-server/src/index.tssites/playground/swagger-mcp-server/src/main.tssites/playground/swagger-mcp-server/src/server/assets-api.tssites/playground/swagger-mcp-server/src/server/create-mcp-server.tssites/playground/swagger-mcp-server/src/server/http-transport.tssites/playground/swagger-mcp-server/src/server/index.tssites/playground/swagger-mcp-server/src/server/register-meta-tools.tssites/playground/swagger-mcp-server/src/server/register-swagger-tools.tssites/playground/swagger-mcp-server/src/server/swagger-tool-registry.tssites/playground/swagger-mcp-server/src/swagger/extract-operations.tssites/playground/swagger-mcp-server/src/swagger/http-executor.tssites/playground/swagger-mcp-server/src/swagger/index.tssites/playground/swagger-mcp-server/src/swagger/parse-swagger-input.tssites/playground/swagger-mcp-server/src/swagger/schema-to-zod.tssites/playground/swagger-mcp-server/src/types.tssites/playground/swagger-mcp-server/tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sites/playground/swagger-mcp-server/src/server/http-transport.ts (1)
74-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden GET/DELETE handlers with explicit error handling.
handleRequesterrors in these handlers are not wrapped, so failures can bypass consistent JSON-RPC error responses.Proposed patch
const mcpGetHandler = async (req: Request, res: Response) => { - const sessionId = req.headers['mcp-session-id'] as string | undefined; - if (!sessionId || !transports[sessionId]) { - res.status(400).send('Invalid or missing session ID'); - return; - } - await transports[sessionId].handleRequest(req, res); + try { + const sessionId = req.headers['mcp-session-id'] as string | undefined; + if (!sessionId || !transports[sessionId]) { + res.status(400).send('Invalid or missing session ID'); + return; + } + await transports[sessionId].handleRequest(req, res); + } catch (error) { + console.error('MCP GET request error:', error); + if (!res.headersSent) { + res.status(500).send('Internal server error'); + } + } }; @@ const mcpDeleteHandler = async (req: Request, res: Response) => { - const sessionId = req.headers['mcp-session-id'] as string | undefined; - if (!sessionId || !transports[sessionId]) { - res.status(400).send('Invalid or missing session ID'); - return; - } - await transports[sessionId].handleRequest(req, res); + try { + const sessionId = req.headers['mcp-session-id'] as string | undefined; + if (!sessionId || !transports[sessionId]) { + res.status(400).send('Invalid or missing session ID'); + return; + } + await transports[sessionId].handleRequest(req, res); + } catch (error) { + console.error('MCP DELETE request error:', error); + if (!res.headersSent) { + res.status(500).send('Internal server error'); + } + } };Also applies to: 83-90
🤖 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 `@sites/playground/swagger-mcp-server/src/server/http-transport.ts` around lines 74 - 81, mcpGetHandler (and the analogous DELETE handler) calls transports[sessionId].handleRequest without protection; wrap the await transport call in a try/catch, log the caught error, and send a consistent JSON-RPC error response (e.g., status 500 with { jsonrpc: "2.0", error: { code: -32000, message: "Internal error", data: error.message }, id: null }) instead of letting the exception propagate; ensure you still validate sessionId/transports[sessionId] first and reference transports[sessionId].handleRequest in your fix so the handler names are updated consistently.
♻️ Duplicate comments (2)
sites/playground/swagger-mcp-server/src/server/http-transport.ts (1)
25-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a session cap to avoid unbounded in-memory growth.
transportsremains unbounded; repeated session initialization can grow memory indefinitely.Proposed patch
type TransportMap = Record<string, StreamableHTTPServerTransport>; +const MAX_SESSIONS = 1000; @@ } else if (!sessionId && isInitializeRequest(req.body)) { + if (Object.keys(transports).length >= MAX_SESSIONS) { + res.status(503).json({ + jsonrpc: '2.0', + error: { code: -32001, message: 'Server busy: session limit reached' }, + id: null, + }); + return; + } + transport = new StreamableHTTPServerTransport({Also applies to: 35-46
🤖 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 `@sites/playground/swagger-mcp-server/src/server/http-transport.ts` at line 25, The global transports map (transports: TransportMap) is unbounded and can grow indefinitely; add a session cap by replacing or wrapping transports with a bounded structure (e.g., an LRU or capped Map) and a MAX_SESSIONS constant, ensure every site where transports is mutated (places that create or set entries into transports) performs eviction when the cap is exceeded (evict oldest/least-recently-used entry and close/cleanup its transport), and update access paths so reads update recency if using LRU—adjust logic around the existing transports/TransportMap usage (including the blocks around lines 35-46 that create sessions) to use the capped container and perform proper cleanup on removal.sites/playground/swagger-mcp-server/src/server/assets-api.ts (1)
52-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop exposing filesystem paths and internal error details in responses.
The API still returns
assetsDirand raw exception messages, which leaks server internals.Proposed hardening patch
- res.json({ directory: assetsDir, files }); + res.json({ files }); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - res.status(500).json({ error: message }); + console.error('Failed to list assets:', error); + res.status(500).json({ error: 'Internal server error' }); } @@ - const message = error instanceof Error ? error.message : String(error); - res.status(500).json({ error: message }); + console.error('Failed to read asset file:', error); + res.status(500).json({ error: 'Internal server error' }); }Also applies to: 75-77
🤖 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 `@sites/playground/swagger-mcp-server/src/server/assets-api.ts` around lines 52 - 56, Remove exposure of internal filesystem paths and raw exceptions: stop returning the assetsDir value in the JSON response and replace it with a non-sensitive placeholder (e.g., omit directory or return a boolean/summary), and change the catch block to send a generic error message like "Internal server error" to clients while logging the full error server-side via console.error or processLogger; specifically update the res.json({ directory: assetsDir, files })/res.json({ files }) usage and the catch handling that builds message from error (the assetsDir variable and the catch block in assets-api.ts) and apply the same change to the similar response/catch at the other occurrence referenced (lines 75-77).
🤖 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 `@sites/playground/server/src/chat-genui.ts`:
- Around line 295-299: The code currently only warns on duplicateToolNames and
then merges swaggerTools, mcpTools, agentTools, and skillTools into tools,
causing silent shadowing; change this to fail fast by checking
duplicateToolNames.size and throwing an Error (including the duplicate names)
instead of console.warn before the merge of swaggerTools, mcpTools, agentTools,
and skillTools so callers cannot proceed with ambiguous tool resolution.
- Around line 274-276: The filter treating MCP server entries as disabled when
`enabled` is omitted should be adjusted so unspecified `enabled` defaults to
true; update the predicate used to compute `externalMcpServers` (and the
identical predicate in the other usage in chat-template) from `s.enabled &&
!isBuiltinSwaggerMcpUrl(s.url)` to a check that treats undefined as true (e.g.,
use nullish-coalescing or explicit default logic for `s.enabled`) while
preserving the `!isBuiltinSwaggerMcpUrl(s.url)` part; apply this change to the
code that computes `externalMcpServers` in chat-genui and the corresponding
filter in chat-template.
In `@sites/playground/server/src/swagger-mcp/build-tools.ts`:
- Around line 27-29: The port matching logic incorrectly treats a URL with no
explicit port (target.port empty) as port 80 only when PORT=3008, causing
localhost without a port to be treated as builtin; fix by computing an effective
port for the parsed URL and comparing that to the configured port: derive
effectiveTargetPort = target.port || (target.protocol === 'https:' ? '443' :
'80') and then set portMatches = effectiveTargetPort === port (remove the
special-case clause that checks targetPort === '80' && port === '3008' &&
!target.port). Keep the rest of the builtin detection (isLocalHost and pathname
comparison using target.pathname and normalizedPath) unchanged so variables
targetPort, port, target.port, target.protocol, isLocalHost, portMatches,
target.pathname, and normalizedPath remain the reference points.
In `@sites/playground/server/vite.config.ts`:
- Around line 30-33: The static-copy config in vite.config.ts contains a typo:
both src and dest strings use "assests" which will break asset copying; update
the values referenced in the static-copy object (the src string
"../swagger-mcp-server/assests" and the dest string "./assests") to use the
correct folder name "assets" so the src becomes "../swagger-mcp-server/assets"
and the dest becomes "./assets".
In `@sites/playground/swagger-mcp-server/src/instance.ts`:
- Around line 10-33: The current getSwaggerMcpServer function returns
process-wide singletons (swaggerMcpServer, swaggerToolRegistry) causing
cross-user tool registration bleed; replace this pattern by removing the
module-level singletons and exposing a factory that creates a fresh McpServer
and SwaggerToolRegistry per call (e.g., createSwaggerMcpServer or change
getSwaggerMcpServer to accept a per-request context and always instantiate new
McpServer/SwaggerToolRegistry), instantiate McpServer (using the same
constructor args), new SwaggerToolRegistry(server), call
registerMetaTools(server, registry), and return the newly created server (and
make registry available to the caller if needed) so that parse_swagger
registrations are scoped to the caller instead of global state.
- Around line 47-51: listSwaggerMcpToolEntries currently reads the private
McpServer._registeredTools; change this to use an explicit registry you maintain
when calling server.registerTool (or intercept/override the protocol response
for "tools/list") instead of accessing _registeredTools directly: update
getSwaggerMcpServer usage to consult your own Map/array of registered tool
definitions (the source-of-truth you populate at register time) and have
listSwaggerMcpToolEntries return entries from that registry; alternatively
implement a wrapper around server.registerTool that records each tool and expose
a public getter used by listSwaggerMcpToolEntries rather than touching
_registeredTools.
In `@sites/playground/swagger-mcp-server/src/server/assets-api.ts`:
- Around line 59-63: The call to resolveSafeAssetPath is currently outside the
route's try/catch so any thrown error can bypass your 400/404/500 mapping; wrap
the path resolution and the subsequent file access logic inside the same
try/catch within the app.get handler (the route using resolveSafeAssetPath,
assetsDir and req.params.filename) and map errors consistently—return 400 for
invalid filename results, 404 when file not found, and 500 for unexpected
exceptions—so all thrown errors from resolveSafeAssetPath are caught and handled
by the existing error-response flow.
---
Outside diff comments:
In `@sites/playground/swagger-mcp-server/src/server/http-transport.ts`:
- Around line 74-81: mcpGetHandler (and the analogous DELETE handler) calls
transports[sessionId].handleRequest without protection; wrap the await transport
call in a try/catch, log the caught error, and send a consistent JSON-RPC error
response (e.g., status 500 with { jsonrpc: "2.0", error: { code: -32000,
message: "Internal error", data: error.message }, id: null }) instead of letting
the exception propagate; ensure you still validate
sessionId/transports[sessionId] first and reference
transports[sessionId].handleRequest in your fix so the handler names are updated
consistently.
---
Duplicate comments:
In `@sites/playground/swagger-mcp-server/src/server/assets-api.ts`:
- Around line 52-56: Remove exposure of internal filesystem paths and raw
exceptions: stop returning the assetsDir value in the JSON response and replace
it with a non-sensitive placeholder (e.g., omit directory or return a
boolean/summary), and change the catch block to send a generic error message
like "Internal server error" to clients while logging the full error server-side
via console.error or processLogger; specifically update the res.json({
directory: assetsDir, files })/res.json({ files }) usage and the catch handling
that builds message from error (the assetsDir variable and the catch block in
assets-api.ts) and apply the same change to the similar response/catch at the
other occurrence referenced (lines 75-77).
In `@sites/playground/swagger-mcp-server/src/server/http-transport.ts`:
- Line 25: The global transports map (transports: TransportMap) is unbounded and
can grow indefinitely; add a session cap by replacing or wrapping transports
with a bounded structure (e.g., an LRU or capped Map) and a MAX_SESSIONS
constant, ensure every site where transports is mutated (places that create or
set entries into transports) performs eviction when the cap is exceeded (evict
oldest/least-recently-used entry and close/cleanup its transport), and update
access paths so reads update recency if using LRU—adjust logic around the
existing transports/TransportMap usage (including the blocks around lines 35-46
that create sessions) to use the capped container and perform proper cleanup on
removal.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 324571f3-91a7-4604-b6ff-bb1b5b28916f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
sites/playground/server/.env.examplesites/playground/server/index.tssites/playground/server/package.jsonsites/playground/server/src/chat-genui.tssites/playground/server/src/chat-template.tssites/playground/server/src/swagger-mcp/build-tools.tssites/playground/server/src/swagger-mcp/index.tssites/playground/server/vite.config.tssites/playground/swagger-mcp-server/.env.examplesites/playground/swagger-mcp-server/package.jsonsites/playground/swagger-mcp-server/src/build-ai-sdk-tools.tssites/playground/swagger-mcp-server/src/index.tssites/playground/swagger-mcp-server/src/instance.tssites/playground/swagger-mcp-server/src/server/assets-api.tssites/playground/swagger-mcp-server/src/server/create-mcp-server.tssites/playground/swagger-mcp-server/src/server/http-transport.tssites/playground/swagger-mcp-server/src/server/index.ts
✅ Files skipped from review due to trivial changes (2)
- sites/playground/server/.env.example
- sites/playground/swagger-mcp-server/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- sites/playground/swagger-mcp-server/.env.example
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sites/playground/swagger-mcp-server/src/server/assets-api.ts (1)
52-56:⚠️ Potential issue | 🟠 MajorInformation disclosure issues remain unaddressed.
The security concerns flagged in previous reviews persist:
- Line 52:
directory: assetsDirexposes the absolute filesystem path to clients.- Lines 54-55, 75-76: Raw
error.messagevalues leak internal details.These should be remediated as outlined in the earlier review comments (remove
directoryfield, return generic error messages, log full errors server-side only).Also applies to: 75-76
🤖 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 `@sites/playground/swagger-mcp-server/src/server/assets-api.ts` around lines 52 - 56, Remove the sensitive absolute path and replace detailed error messages with generic responses: stop including the assetsDir field in the successful response (only return files), and in the catch blocks that currently build message from error (the catch that calls res.status(500).json and the similar handler around lines 75-76) send a generic error payload like { error: "Internal server error" } to clients while logging the full error server-side (use console.error or the existing logger) so the original error.message/stack is not leaked. Target the res.json call that currently returns { directory: assetsDir, files } and the catch blocks that use error instanceof Error ? error.message : String(error) to implement these changes.
🤖 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.
Duplicate comments:
In `@sites/playground/swagger-mcp-server/src/server/assets-api.ts`:
- Around line 52-56: Remove the sensitive absolute path and replace detailed
error messages with generic responses: stop including the assetsDir field in the
successful response (only return files), and in the catch blocks that currently
build message from error (the catch that calls res.status(500).json and the
similar handler around lines 75-76) send a generic error payload like { error:
"Internal server error" } to clients while logging the full error server-side
(use console.error or the existing logger) so the original error.message/stack
is not leaked. Target the res.json call that currently returns { directory:
assetsDir, files } and the catch blocks that use error instanceof Error ?
error.message : String(error) to implement these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07b68ba1-2dc4-4b75-9f12-b14d36371cb4
📒 Files selected for processing (6)
sites/playground/server/src/swagger-mcp/index.tssites/playground/server/vite.config.tssites/playground/swagger-mcp-server/.env.examplesites/playground/swagger-mcp-server/assets/aad.jsonsites/playground/swagger-mcp-server/src/config.tssites/playground/swagger-mcp-server/src/server/assets-api.ts
✅ Files skipped from review due to trivial changes (1)
- sites/playground/swagger-mcp-server/.env.example
🚧 Files skipped from review as they are similar to previous changes (3)
- sites/playground/server/vite.config.ts
- sites/playground/swagger-mcp-server/src/config.ts
- sites/playground/server/src/swagger-mcp/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
sites/playground/swagger-mcp-server/src/swagger/http-executor.ts (2)
120-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeout/abort protection to outbound fetch.
Line 120can block indefinitely if the upstream hangs, which can stall MCP tool execution under load.Proposed fix
+const HTTP_TIMEOUT_MS = 15_000; @@ - const response = await fetch(url, init); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), HTTP_TIMEOUT_MS); + let response: Response; + try { + response = await fetch(url, { ...init, signal: controller.signal }); + } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + throw new Error(`Request timed out after ${HTTP_TIMEOUT_MS}ms`); + } + throw error; + } finally { + clearTimeout(timer); + }🤖 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 `@sites/playground/swagger-mcp-server/src/swagger/http-executor.ts` at line 120, The outbound fetch at "response = await fetch(url, init);" needs abort/timeout protection: create an AbortController, set a timeout (e.g. configurable TIMEOUT_MS) that calls controller.abort(), merge controller.signal into the request init (respecting any existing init.signal by using controller.signal as the effective signal or linking signals), call fetch with that signal, clear the timeout (clearTimeout) after fetch completes, and ensure downstream callers handle AbortError/DOMException appropriately; update the function (where fetch is invoked in http-executor.ts) to implement this pattern so hung upstreams cannot stall execution.
44-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncode cookie names before composing the
Cookieheader.
Line 44inserts raw parameter names into the cookie header. If a Swagger param name contains cookie delimiters (;,=), this can alter cookie semantics and send unintended cookie pairs.Proposed fix
- .map(([name, value]) => `${name}=${encodeCookieValue(value)}`); + .map(([name, value]) => `${encodeURIComponent(name)}=${encodeCookieValue(value)}`);🤖 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 `@sites/playground/swagger-mcp-server/src/swagger/http-executor.ts` at line 44, The cookie header builder currently inserts raw cookie names in the map callback (.map(([name, value]) => `${name}=${encodeCookieValue(value)}`), which can allow delimiters in names to break semantics; update this expression to encode the cookie name as well (e.g., use encodeCookieValue(name) or a dedicated encodeCookieName(name)) so the composed string becomes `${encodeCookieName(name)}=${encodeCookieValue(value)}` and ensure any helper you add/reuse follows the same encoding rules as encodeCookieValue.
🤖 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 `@sites/playground/swagger-mcp-server/src/server/http-transport.ts`:
- Around line 46-63: The current capacity check
(sessionRegistry.canAcceptSession()) is racy because registration happens later
in StreamableHTTPServerTransport. Fix by making session creation atomic:
generate a session id up front (using the same randomUUID() logic), then attempt
to reserve/register that id in sessionRegistry immediately (add or use a
register/reserve method that returns success/failure) before constructing or
returning the transport; if the immediate register/reserve fails respond with
503, otherwise create the StreamableHTTPServerTransport with that id and ensure
onsessioninitialized either confirms or finalizes the registration (or becomes a
no-op) so no two concurrent initializations can exceed maxSessions. Reference
the functions/objects: sessionRegistry.canAcceptSession(),
sessionRegistry.register(), StreamableHTTPServerTransport, sessionIdGenerator,
onsessioninitialized.
- Around line 130-133: The code currently disposes a caller-provided
sessionRegistry on SIGINT/SIGTERM; change this so only registries created
locally by this module are disposed on process signals: introduce a boolean like
createdLocalSessionRegistry that you set to true when you instantiate
sessionRegistry locally (where sessionRegistry is constructed), leave it false
when sessionRegistry is injected via options, and guard the
process.once('SIGINT'...) / process.once('SIGTERM'...) registration (and the
dispose callback) behind that flag so you do not attach signal handlers or call
sessionRegistry.dispose() for caller-owned registries.
---
Duplicate comments:
In `@sites/playground/swagger-mcp-server/src/swagger/http-executor.ts`:
- Line 120: The outbound fetch at "response = await fetch(url, init);" needs
abort/timeout protection: create an AbortController, set a timeout (e.g.
configurable TIMEOUT_MS) that calls controller.abort(), merge controller.signal
into the request init (respecting any existing init.signal by using
controller.signal as the effective signal or linking signals), call fetch with
that signal, clear the timeout (clearTimeout) after fetch completes, and ensure
downstream callers handle AbortError/DOMException appropriately; update the
function (where fetch is invoked in http-executor.ts) to implement this pattern
so hung upstreams cannot stall execution.
- Line 44: The cookie header builder currently inserts raw cookie names in the
map callback (.map(([name, value]) => `${name}=${encodeCookieValue(value)}`),
which can allow delimiters in names to break semantics; update this expression
to encode the cookie name as well (e.g., use encodeCookieValue(name) or a
dedicated encodeCookieName(name)) so the composed string becomes
`${encodeCookieName(name)}=${encodeCookieValue(value)}` and ensure any helper
you add/reuse follows the same encoding rules as encodeCookieValue.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36b012ab-ba45-473f-b8a9-5daca65c10fc
📒 Files selected for processing (7)
sites/playground/server/src/swagger-mcp/index.tssites/playground/swagger-mcp-server/.env.examplesites/playground/swagger-mcp-server/src/server/assets-api.tssites/playground/swagger-mcp-server/src/server/http-transport.tssites/playground/swagger-mcp-server/src/server/index.tssites/playground/swagger-mcp-server/src/server/mcp-session-registry.tssites/playground/swagger-mcp-server/src/swagger/http-executor.ts
✅ Files skipped from review due to trivial changes (1)
- sites/playground/swagger-mcp-server/.env.example
🚧 Files skipped from review as they are similar to previous changes (3)
- sites/playground/swagger-mcp-server/src/server/index.ts
- sites/playground/server/src/swagger-mcp/index.ts
- sites/playground/swagger-mcp-server/src/server/assets-api.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@sites/playground/swagger-mcp-server/src/swagger/parse-swagger-input.ts`:
- Around line 60-61: The call to SwaggerParser.validate(raw) can resolve
external $ref pointers; update the call in parse-swagger-input.ts so
SwaggerParser.validate(raw, { resolve: { external: false } }) is used to disable
external resolution. Locate the parseRawSpec(...) -> raw variable and replace
the existing SwaggerParser.validate(raw) invocation with
SwaggerParser.validate(raw, { resolve: { external: false } }) to ensure external
HTTP/file $refs are not fetched.
In `@sites/playground/swagger-mcp-server/src/swagger/swagger-input-security.ts`:
- Around line 85-105: isBlockedHostname currently only rejects literal IP hosts,
allowing hostnames that resolve to loopback/RFC1918/link-local addresses to
bypass SSRF checks; update isBlockedHostname (and the similar logic around lines
142-149) to perform a DNS resolution of the input hostname first (use
dns.promises.resolve or resolve4/resolve6 to get all addresses), normalize the
original hostname as you already do, then for each resolved IP run the existing
IP logic (isIP, isPrivateIpv4, isPrivateIpv6, and the metadata.google.internal
check) and return blocked if any resolved address is
private/loopback/link-local; keep the urlHostnameAllowlist check but still
resolve and validate the resolved IPs even when the hostname matches the
allowlist to avoid allowlist-based bypass. Ensure the function remains
synchronous/async consistent with callers (make isBlockedHostname async if
necessary) and update all call sites accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac3132a9-06a8-4ba1-93c4-e924296762ff
📒 Files selected for processing (9)
sites/playground/server/.env.examplesites/playground/swagger-mcp-server/.env.examplesites/playground/swagger-mcp-server/src/server/register-meta-tools.tssites/playground/swagger-mcp-server/src/server/register-swagger-tools.tssites/playground/swagger-mcp-server/src/swagger/http-executor.tssites/playground/swagger-mcp-server/src/swagger/index.tssites/playground/swagger-mcp-server/src/swagger/parse-swagger-input.tssites/playground/swagger-mcp-server/src/swagger/swagger-input-security.tssites/playground/swagger-mcp-server/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- sites/playground/swagger-mcp-server/.env.example
🚧 Files skipped from review as they are similar to previous changes (5)
- sites/playground/server/.env.example
- sites/playground/swagger-mcp-server/src/swagger/index.ts
- sites/playground/swagger-mcp-server/src/types.ts
- sites/playground/swagger-mcp-server/src/server/register-swagger-tools.ts
- sites/playground/swagger-mcp-server/src/server/register-meta-tools.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add an MCP service that supports converting Swagger documents into MCP tools
Summary by CodeRabbit
New Features
Chores