feat: mcp health checker tool#2190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Deploying openstatus-docs with
|
| Latest commit: |
8eaacd6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cb1ec22e.openstatus-1sy.pages.dev |
| Branch Preview URL: | https://feat-mcp-health.openstatus-1sy.pages.dev |
|
@claude review |
|
Claude finished @mxkaske's task in 2m 12s —— View job Code Review: feat/mcp-healthTodo:
Overall this is a well-structured feature. The core library is high quality: Edge-safe, good SSE parsing, proper SSRF protection, validated Redis persistence. A few things worth attention below. Security1. User headers can override In const RESERVED = new Set(["content-type", "accept", "mcp-session-id", "mcp-protocol-version"]);
for (const { key, value } of extra ?? []) {
if (!key || RESERVED.has(key.toLowerCase())) continue;
headers.set(key, value);
}2. No max-length validation on custom header values (
z.array(z.object({ key: z.string().max(256), value: z.string().max(2048) })).max(20)3. Self-request guard is too narrow ( urlObject.hostname.toLowerCase().endsWith("openstatus.dev") &&
urlObject.pathname.startsWith("/play/mcp-health/api")This only blocks self-calls to the MCP API path. Any other path on Logic4. Latency timestamps drift in parallel steps ( const pingStarted = Date.now();
const toolsStarted = Date.now();Both timestamps are captured synchronously before the parallel requests start, so they measure effectively the same start point. This is fine functionally — the latencies will both be measured from launch time. A minor note: if one wanted the latency to reflect time-to-first-byte of each request independently, you'd need to instrument inside 5. Both call sites pass UX / Minor6. const input = document.querySelector<HTMLInputElement>("input[name='url']");
if (input) input.value = url;Querying by 7. Rate limit is quite tight ( 3 requests / 60 seconds per IP is very restrictive — a developer who checks their own MCP server twice and then immediately tests a second one is already rate-limited. Worth considering a slightly wider window (e.g. 5–10 per minute) for a public dev tool. 8. No function StatusDot({ input }: { input: boolean | null | StepResult }) {
return <div className={cn("size-4", statusDotClass(input))} />;
}Color-only indicators have no accessible label. Screen readers won't know the step's status. Adding What's solid
|
![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 24 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="apps/web/src/app/(landing)/play/mcp-health/api/route.ts">
<violation number="1" location="apps/web/src/app/(landing)/play/mcp-health/api/route.ts:79">
P1: The SSRF check only validates the hostname string and can be bypassed with domains that resolve to private/internal IPs.</violation>
</file>
<file name="apps/web/src/app/(landing)/play/mcp-health/[id]/page.tsx">
<violation number="1" location="apps/web/src/app/(landing)/play/mcp-health/[id]/page.tsx:69">
P2: Use `notFound()` instead of redirecting when the health report does not exist.
(Based on your team's feedback about Use framework-provided notFound handling in Next.js.) [FEEDBACK_USED]</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
|
||
| try { | ||
| // protocol check + private/loopback/metadata-host block. Throws on bad input. | ||
| assertSafeUrlSync(parsed.url); |
There was a problem hiding this comment.
P1: The SSRF check only validates the hostname string and can be bypassed with domains that resolve to private/internal IPs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/app/(landing)/play/mcp-health/api/route.ts, line 79:
<comment>The SSRF check only validates the hostname string and can be bypassed with domains that resolve to private/internal IPs.</comment>
<file context>
@@ -0,0 +1,193 @@
+
+ try {
+ // protocol check + private/loopback/metadata-host block. Throws on bad input.
+ assertSafeUrlSync(parsed.url);
+ } catch (err) {
+ return errorResponse(
</file context>
| const page = getToolsPage("mcp-health-slug"); | ||
|
|
||
| const data = await getHealthReportById(id); | ||
| if (!data) redirect("/play/mcp-health"); |
There was a problem hiding this comment.
P2: Use notFound() instead of redirecting when the health report does not exist.
(Based on your team's feedback about Use framework-provided notFound handling in Next.js.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/app/(landing)/play/mcp-health/[id]/page.tsx, line 69:
<comment>Use `notFound()` instead of redirecting when the health report does not exist.
(Based on your team's feedback about Use framework-provided notFound handling in Next.js.) </comment>
<file context>
@@ -0,0 +1,97 @@
+ const page = getToolsPage("mcp-health-slug");
+
+ const data = await getHealthReportById(id);
+ if (!data) redirect("/play/mcp-health");
+
+ const jsonLDGraph = createJsonLDGraph([
</file context>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="apps/web/src/lib/mcp/health-check.ts">
<violation number="1" location="apps/web/src/lib/mcp/health-check.ts:257">
P1: Manual redirect following forwards the same headers to redirected hosts, which can leak sensitive user-supplied headers (such as Authorization) across origins.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| target = new URL(location, target).toString(); | ||
| assertSafeUrlSync(target); |
There was a problem hiding this comment.
P1: Manual redirect following forwards the same headers to redirected hosts, which can leak sensitive user-supplied headers (such as Authorization) across origins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/lib/mcp/health-check.ts, line 257:
<comment>Manual redirect following forwards the same headers to redirected hosts, which can leak sensitive user-supplied headers (such as Authorization) across origins.</comment>
<file context>
@@ -223,6 +234,32 @@ async function readSseUntilId(
+ if (res.status < 300 || res.status >= 400) return res;
+ const location = res.headers.get("location");
+ if (!location) return res;
+ target = new URL(location, target).toString();
+ assertSafeUrlSync(target);
+ }
</file context>
| target = new URL(location, target).toString(); | |
| assertSafeUrlSync(target); | |
| const next = new URL(location, target); | |
| if (next.origin !== new URL(target).origin) { | |
| throw new Error("refusing cross-origin redirect"); | |
| } | |
| target = next.toString(); | |
| assertSafeUrlSync(target); |
1 participant
No description provided.