fix(harness): sweep remaining PR #2996 review findings (COMP-10)#3041
fix(harness): sweep remaining PR #2996 review findings (COMP-10)#3041ignaciojimenezr wants to merge 1 commit into
Conversation
Dispositions for all 22 cubic findings on #2996, verified against current main (4 were already fixed by #2998; Electron upload 404 is COMP-8 and the ?token= fallback removal is COMP-2): - harness-mcp + adapter-http (streamable POST): malformed JSON-RPC now gets -32700/-32600 instead of a 202 that fakes delivery (SSE /messages keeps 202 per SSE transport semantics) - use-chat-session: preflight-resolved server ids now ride with the names snapshot they were resolved from (mid-preflight selection change race); submit no longer deadlocks for ad-hoc/App servers when a send-time resolver exists - run-harness-turn: fail closed on tool-approval-request without a continuity lane (was an unresumable pause) - localhost-check: fc/fd ULA prefixes only for IPv6 literals; IPv4-mapped addresses judged by the embedded IPv4 (dotted + URL-normalized hex forms) - harness-proxy-token-client: tokens payload validated as a string record; endpoint-config errors stay on the result contract; bearer scheme normalized case-insensitively - computer-upload: mid-batch write failure reports written files + failedAt - ComputerTerminal: quote the upload dir in the auto-typed ls - shared/harness-session: workdir must be a trimmed absolute path; reset-reason set typed with the union - ensure-server-tunnel: one shared scoped ensure flow for both planes - mcpjam-stream-handler: drop dead `skills` option; mcp-config: drop dead HarnessMcpConfigError; proxy-token test: env swap in try/finally Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
size:L
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-3041.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/server/routes/mcp/http-adapters.ts">
<violation number="1" location="mcpjam-inspector/server/routes/mcp/http-adapters.ts:343">
P2: Valid JSON-RPC batch requests to adapter-http/manager-http now fail with `-32600` because the new validation rejects every top-level array. If these routes need JSON-RPC 2.0 compatibility beyond single requests, this should distinguish malformed arrays from batches and process each request element.</violation>
</file>
<file name="mcpjam-inspector/server/routes/web/harness-mcp.ts">
<violation number="1" location="mcpjam-inspector/server/routes/web/harness-mcp.ts:151">
P2: Malformed JSON-RPC without `jsonrpc: "2.0"` can still be accepted or forwarded. Since this gate is meant to stop invalid JSON-RPC before the notification `202` path, it should also require the JSON-RPC version field.</violation>
<violation number="2" location="mcpjam-inspector/server/routes/web/harness-mcp.ts:154">
P2: Invalid-request responses can echo an invalid `id` value such as an object or array. Normalizing non-string/non-number/non-null ids to `null` keeps the error response valid JSON-RPC and easier for clients to parse.</violation>
</file>
<file name="mcpjam-inspector/server/utils/localhost-check.ts">
<violation number="1" location="mcpjam-inspector/server/utils/localhost-check.ts:46">
P2: Link-local IPv6 URLs such as `http://[fe90::1]` are still treated as publicly reachable, so hosted URL strategy can choose direct access for a non-routable address. The link-local check should cover the full `fe80::/10` range, not only addresses whose first hextet is exactly `fe80`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if ( | ||
| !body || | ||
| typeof body !== "object" || | ||
| Array.isArray(body) || |
There was a problem hiding this comment.
P2: Valid JSON-RPC batch requests to adapter-http/manager-http now fail with -32600 because the new validation rejects every top-level array. If these routes need JSON-RPC 2.0 compatibility beyond single requests, this should distinguish malformed arrays from batches and process each request element.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/mcp/http-adapters.ts, line 343:
<comment>Valid JSON-RPC batch requests to adapter-http/manager-http now fail with `-32600` because the new validation rejects every top-level array. If these routes need JSON-RPC 2.0 compatibility beyond single requests, this should distinguish malformed arrays from batches and process each request element.</comment>
<file context>
@@ -321,11 +321,42 @@ function createHttpHandler(mode: BridgeMode, routePrefix: string) {
+ if (
+ !body ||
+ typeof body !== "object" ||
+ Array.isArray(body) ||
+ typeof body.method !== "string" ||
+ body.method.length === 0
</file context>
| const id = | ||
| body && typeof body === "object" && !Array.isArray(body) | ||
| ? (body.id ?? null) | ||
| : null; |
There was a problem hiding this comment.
P2: Invalid-request responses can echo an invalid id value such as an object or array. Normalizing non-string/non-number/non-null ids to null keeps the error response valid JSON-RPC and easier for clients to parse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/web/harness-mcp.ts, line 154:
<comment>Invalid-request responses can echo an invalid `id` value such as an object or array. Normalizing non-string/non-number/non-null ids to `null` keeps the error response valid JSON-RPC and easier for clients to parse.</comment>
<file context>
@@ -128,11 +128,41 @@ async function handle(c: any) {
+ typeof body.method !== "string" ||
+ body.method.length === 0
+ ) {
+ const id =
+ body && typeof body === "object" && !Array.isArray(body)
+ ? (body.id ?? null)
</file context>
| const id = | |
| body && typeof body === "object" && !Array.isArray(body) | |
| ? (body.id ?? null) | |
| : null; | |
| const rawId = | |
| body && typeof body === "object" && !Array.isArray(body) | |
| ? (body.id ?? null) | |
| : null; | |
| const id = | |
| typeof rawId === "string" || typeof rawId === "number" || rawId === null | |
| ? rawId | |
| : null; |
| typeof body.method !== "string" || | ||
| body.method.length === 0 |
There was a problem hiding this comment.
P2: Malformed JSON-RPC without jsonrpc: "2.0" can still be accepted or forwarded. Since this gate is meant to stop invalid JSON-RPC before the notification 202 path, it should also require the JSON-RPC version field.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/routes/web/harness-mcp.ts, line 151:
<comment>Malformed JSON-RPC without `jsonrpc: "2.0"` can still be accepted or forwarded. Since this gate is meant to stop invalid JSON-RPC before the notification `202` path, it should also require the JSON-RPC version field.</comment>
<file context>
@@ -128,11 +128,41 @@ async function handle(c: any) {
+ !body ||
+ typeof body !== "object" ||
+ Array.isArray(body) ||
+ typeof body.method !== "string" ||
+ body.method.length === 0
+ ) {
</file context>
| typeof body.method !== "string" || | |
| body.method.length === 0 | |
| body.jsonrpc !== "2.0" || | |
| typeof body.method !== "string" || | |
| body.method.length === 0 |
| if (h.includes(":")) { | ||
| // IPv6 literal (a hostname like "fc.example.com" must NOT hit these). | ||
| if (h === "::1" || h === "::") return false; | ||
| if (h.startsWith("fe80:") || h.startsWith("fc") || h.startsWith("fd")) { |
There was a problem hiding this comment.
P2: Link-local IPv6 URLs such as http://[fe90::1] are still treated as publicly reachable, so hosted URL strategy can choose direct access for a non-routable address. The link-local check should cover the full fe80::/10 range, not only addresses whose first hextet is exactly fe80.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/utils/localhost-check.ts, line 46:
<comment>Link-local IPv6 URLs such as `http://[fe90::1]` are still treated as publicly reachable, so hosted URL strategy can choose direct access for a non-routable address. The link-local check should cover the full `fe80::/10` range, not only addresses whose first hextet is exactly `fe80`.</comment>
<file context>
@@ -39,10 +39,38 @@ export function isPubliclyReachableUrl(raw: string): boolean {
+ if (h.includes(":")) {
+ // IPv6 literal (a hostname like "fc.example.com" must NOT hit these).
+ if (h === "::1" || h === "::") return false;
+ if (h.startsWith("fe80:") || h.startsWith("fc") || h.startsWith("fd")) {
+ return false; // link-local / unique-local
+ }
</file context>
| if (h.startsWith("fe80:") || h.startsWith("fc") || h.startsWith("fd")) { | |
| if (/^fe[89ab][0-9a-f]:/.test(h) || h.startsWith("fc") || h.startsWith("fd")) { |
WalkthroughChangesThis PR tightens JSON-RPC request validation on both the adapter-http and harness-mcp routes, returning proper -32700/-32600 error responses instead of treating malformed bodies as notifications. It refines hosted chat session logic to pair resolved server IDs with the exact server names used during resolution, preventing stale mismatches during async preflight races. Network security is hardened via stricter IPv6/IPv4-mapped address filtering, a unified scoped tunnel-provisioning helper, and stronger harness proxy token payload/URL validation. Additional changes include quoting terminal directory paths against shell injection, a fail-closed guard for unresumable tool-approval requests, stricter harness session data-part validation, richer computer-upload failure responses, and removal of unused exports ( Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpAdapterRoute
participant HarnessMcpRoute
Client->>HttpAdapterRoute: POST JSON-RPC body
HttpAdapterRoute->>HttpAdapterRoute: parse and validate
alt malformed JSON
HttpAdapterRoute-->>Client: 400, error -32700
else invalid structure
HttpAdapterRoute-->>Client: 400, error -32600
else notification
HttpAdapterRoute-->>Client: 202 Accepted
end
Client->>HarnessMcpRoute: POST JSON-RPC body
HarnessMcpRoute->>HarnessMcpRoute: parse and validate
alt malformed JSON
HarnessMcpRoute-->>Client: 400, error -32700
else invalid structure
HarnessMcpRoute-->>Client: 400, error -32600
else notification
HarnessMcpRoute-->>Client: 202 Accepted
end
Compact metadata Related issues: None found. Suggested labels: security, bug, tests, server, client Suggested reviewers: None specified. Poem Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
mcpjam-inspector/client/src/components/computer/ComputerTerminal.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
mcpjam-inspector/client/src/hooks/use-chat-session.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
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: 1
🤖 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 `@mcpjam-inspector/server/routes/mcp/http-adapters.ts`:
- Around line 324-359: The JSON-RPC parse/validation block in the HTTP route is
duplicated and should be centralized. Move the shared `c.req.json()` parsing
plus `-32700`/`-32600` response construction into a helper in
`services/mcp-http-bridge` such as `parseAndValidateJsonRpc(rawRequest)`, and
have both `http-adapters.ts` and `harness-mcp.ts` call it and short-circuit on
an error result. Keep the helper responsible for returning either the validated
body or a ready-made response tuple so the `handleMcp`/route code stays
consistent and correctness-critical error handling does not drift.
🪄 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: 48834ffe-7a6e-41c7-bad9-80c2631d1a10
📒 Files selected for processing (18)
mcpjam-inspector/client/src/components/computer/ComputerTerminal.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/server/routes/mcp/__tests__/http-adapters.streamable-http.test.tsmcpjam-inspector/server/routes/mcp/http-adapters.tsmcpjam-inspector/server/routes/web/__tests__/harness-mcp.test.tsmcpjam-inspector/server/routes/web/computer-upload.tsmcpjam-inspector/server/routes/web/harness-mcp.tsmcpjam-inspector/server/services/ensure-server-tunnel.tsmcpjam-inspector/server/utils/harness/__tests__/harness-proxy-strategy.test.tsmcpjam-inspector/server/utils/harness/__tests__/harness-proxy-token.test.tsmcpjam-inspector/server/utils/harness/harness-proxy-token-client.tsmcpjam-inspector/server/utils/harness/mcp-config.tsmcpjam-inspector/server/utils/harness/run-harness-turn.tsmcpjam-inspector/server/utils/localhost-check.tsmcpjam-inspector/server/utils/mcpjam-stream-handler.tsmcpjam-inspector/shared/__tests__/harness-session.test.tsmcpjam-inspector/shared/harness-session.ts
💤 Files with no reviewable changes (2)
- mcpjam-inspector/server/utils/harness/mcp-config.ts
- mcpjam-inspector/server/utils/mcpjam-stream-handler.ts
| // Malformed payloads must NOT fall through to the notification → 202 path | ||
| // (the bridge treats a missing method as a notification): a garbage body | ||
| // acknowledged as "Accepted" looks like a delivered message to the client. | ||
| let body: any = undefined; | ||
| try { | ||
| body = await c.req.json(); | ||
| } catch {} | ||
| } catch { | ||
| return c.json( | ||
| { | ||
| jsonrpc: "2.0", | ||
| id: null, | ||
| error: { code: -32700, message: "Parse error" }, | ||
| }, | ||
| 400 | ||
| ); | ||
| } | ||
| if ( | ||
| !body || | ||
| typeof body !== "object" || | ||
| Array.isArray(body) || | ||
| typeof body.method !== "string" || | ||
| body.method.length === 0 | ||
| ) { | ||
| const id = | ||
| body && typeof body === "object" && !Array.isArray(body) | ||
| ? (body.id ?? null) | ||
| : null; | ||
| return c.json( | ||
| { | ||
| jsonrpc: "2.0", | ||
| id, | ||
| error: { code: -32600, message: "Invalid Request" }, | ||
| }, | ||
| 400 | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract shared JSON-RPC parse/validate logic.
This block is duplicated verbatim in harness-mcp.ts (lines 131-166). Both files already import from ../../services/mcp-http-bridge, which is a natural home for a parseAndValidateJsonRpc(rawRequest) helper returning either a parsed body or a ready-made {status, response} error tuple. Keeping this correctness-critical error-code logic (-32700/-32600) in one place avoids future drift between the two routes.
♻️ Sketch of a shared helper
+// services/mcp-http-bridge.ts
+export async function parseAndValidateJsonRpcBody(
+ req: { json(): Promise<any> }
+): Promise<{ ok: true; body: any } | { ok: false; status: number; response: any }> {
+ let body: any;
+ try {
+ body = await req.json();
+ } catch {
+ return {
+ ok: false,
+ status: 400,
+ response: { jsonrpc: "2.0", id: null, error: { code: -32700, message: "Parse error" } },
+ };
+ }
+ if (
+ !body ||
+ typeof body !== "object" ||
+ Array.isArray(body) ||
+ typeof body.method !== "string" ||
+ body.method.length === 0
+ ) {
+ const id = body && typeof body === "object" && !Array.isArray(body) ? (body.id ?? null) : null;
+ return {
+ ok: false,
+ status: 400,
+ response: { jsonrpc: "2.0", id, error: { code: -32600, message: "Invalid Request" } },
+ };
+ }
+ return { ok: true, body };
+}Then both routes call this helper and short-circuit on !result.ok.
🤖 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 `@mcpjam-inspector/server/routes/mcp/http-adapters.ts` around lines 324 - 359,
The JSON-RPC parse/validation block in the HTTP route is duplicated and should
be centralized. Move the shared `c.req.json()` parsing plus `-32700`/`-32600`
response construction into a helper in `services/mcp-http-bridge` such as
`parseAndValidateJsonRpc(rawRequest)`, and have both `http-adapters.ts` and
`harness-mcp.ts` call it and short-circuit on an error result. Keep the helper
responsible for returning either the validated body or a ready-made response
tuple so the `handleMcp`/route code stays consistent and correctness-critical
error handling does not drift.
2 participants
harness-mcpand localadapter-http) now gets a proper JSON-RPC error (-32700 / -32600) instead of a 202 that made dropped messages look delivered. Covered by tests on both routes; real notifications still 202.isPubliclyReachableUrlno longer treatsfc*/fd*hostnames as private and judges IPv4-mapped IPv6 by the embedded IPv4; the proxy-token client validates Convex payloads strictly, keeps config errors on its result contract, and accepts any bearer-scheme casing; upload failures mid-batch report which files landed; the auto-typedlsafter upload quotes the path; the harness workdir validator requires a trimmed absolute path.skillsoption and deadHarnessMcpConfigErrorremoved, reset-reason set typed with its union, test env-var swap wrapped in try/finally.Disposition of all 22 cubic findings on #2996 (verified against current main):
>vs>=?token=auth/messageskeeps 202 per SSE transport semantics)lspathstringskillsoptionHarnessMcpConfigErrorVerification: full
vitest rungreen (7647 passed, 0 failed),typecheck:clientgreen, servertscerrors unchanged from main's baseline.🤖 Generated with Claude Code
Summary by cubic
Fixes the remaining findings from PR #2996 for COMP-10, improving reliability of harness JSON-RPC handling, hosted preflight selection, and upload reporting.
Bug Fixes
harness-mcpandadapter-http: malformed JSON-RPC now returns -32700/-32600 (not 202); real notifications still 202.use-chat-session: preflight-resolved server ids are tied to the names snapshot; submit isn’t blocked whenensureServerIdsis provided.writtenfiles andfailedAt;ComputerTerminalauto-typedlsnow quotes the path.isPubliclyReachableUrl: treatfc*/fd*only for IPv6 literals; judge IPv4-mapped IPv6 by the embedded IPv4.harness-proxy-token-client: strict Convex payload validation; endpoint-config errors returned on the result; case-insensitive bearer handling.shared/harness-session: require trimmed absoluteworkdir;reset-reasonuses its union type.Refactors
ensureServerTunnel,ensureHarnessWebTunnel); removed deadskillsoption andHarnessMcpConfigError; minor test cleanup.Written for commit 2e392cc. Summary will update on new commits.