-
Notifications
You must be signed in to change notification settings - Fork 30
feat(api): replace default logger with pretty colored request logging #2166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🧪 BenchmarkShould we run the MCP Gateway benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/mesh/src/api/app.ts">
<violation number="1" location="apps/mesh/src/api/app.ts:218">
P2: If downstream throws, the completion (status/duration) log won’t run. Wrap `await next()` in `try/finally` so the outgoing log is always emitted while still propagating errors.</violation>
<violation number="2" location="apps/mesh/src/api/app.ts:237">
P1: Untrusted MCP request fields (toolName/eventType/arg keys) are interpolated into console logs without sanitizing control characters. This enables log forging / terminal escape injection. Strip `\r`, `\n`, and ANSI `\x1b` (or escape/control-character filter) before building `mcpInfo` (and consider doing the same for `path`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/mesh/src/api/app.ts
Outdated
| }; | ||
| }; | ||
| if (body.method === "tools/call" && body.params?.name) { | ||
| const toolName = body.params.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Untrusted MCP request fields (toolName/eventType/arg keys) are interpolated into console logs without sanitizing control characters. This enables log forging / terminal escape injection. Strip \r, \n, and ANSI \x1b (or escape/control-character filter) before building mcpInfo (and consider doing the same for path).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/app.ts, line 237:
<comment>Untrusted MCP request fields (toolName/eventType/arg keys) are interpolated into console logs without sanitizing control characters. This enables log forging / terminal escape injection. Strip `\r`, `\n`, and ANSI `\x1b` (or escape/control-character filter) before building `mcpInfo` (and consider doing the same for `path`).</comment>
<file context>
@@ -176,8 +175,124 @@ export function createApp(options: CreateAppOptions = {}) {
+ };
+ };
+ if (body.method === "tools/call" && body.params?.name) {
+ const toolName = body.params.name;
+ const args = body.params.arguments || {};
+
</file context>
✅ Addressed in 952a1ef
apps/mesh/src/api/app.ts
Outdated
|
|
||
| // Skip noisy paths | ||
| if (path === "/api/auth/get-session" || path.includes("favicon")) { | ||
| await next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: If downstream throws, the completion (status/duration) log won’t run. Wrap await next() in try/finally so the outgoing log is always emitted while still propagating errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/app.ts, line 218:
<comment>If downstream throws, the completion (status/duration) log won’t run. Wrap `await next()` in `try/finally` so the outgoing log is always emitted while still propagating errors.</comment>
<file context>
@@ -176,8 +175,124 @@ export function createApp(options: CreateAppOptions = {}) {
+
+ // Skip noisy paths
+ if (path === "/api/auth/get-session" || path.includes("favicon")) {
+ await next();
+ return;
+ }
</file context>
✅ Addressed in 952a1ef
Replace hono/logger with custom colored logging middleware that: - Uses ANSI colors for methods, status codes, and MCP-specific info - Shows tool names and arguments for MCP calls - Highlights event bus operations (PUBLISH, SUBSCRIBE, UNSUBSCRIBE) - Shortens long connection IDs in paths - Uses directional arrows (◀/▶) for MCP calls vs regular arrows (←/→) - Skips noisy paths like /api/auth/get-session This provides better visibility into MCP operations during development.
146724d to
076fa71
Compare
- Skip body cloning in production for performance improvements. - Maintain detailed parsing in the call stack for better visibility. - Ensure error handling for body parsing failures without breaking requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 22
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| mcpInfo = `${colors.tool}EVENT_SUBSCRIBE${colors.reset} ${colors.bold}← ${eventType}${colors.reset}`; | ||
| } else if (toolName === "EVENT_UNSUBSCRIBE" && args.eventType) { | ||
| const eventType = sanitizeForLog(String(args.eventType)); | ||
| mcpInfo = `${colors.tool}EVENT_UNSUBSCRIBE${colors.reset} ${colors.dim}✕ ${eventType}${colors.reset}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EVENT_UNSUBSCRIBE special log formatting never triggers
Low Severity
The condition args.eventType for EVENT_UNSUBSCRIBE will never be truthy because this tool's input schema uses subscriptionId, not eventType. The spec and implementation confirm that EVENT_UNSUBSCRIBE takes { subscriptionId: string }. As a result, the special "✕ eventType" formatting will never appear for unsubscribe operations - they will fall through to the default formatting instead.
Replace hono/logger with custom colored logging middleware that:
This provides better visibility into MCP operations during development.
What is this contribution about?
Screenshots/Demonstration
Review Checklist
Summary by cubic
Replaced the default logger in development with a custom, colorized request logger to make MCP activity easier to see and debug. Production keeps hono/logger.
New Features
Refactors
Written for commit f7e71bc. Summary will update on new commits.
Note
Introduces a custom, colorized dev logging middleware and wires it into the app for non-production environments.
utils/dev-logger.tsmiddleware: ANSI-colored methods/status, request duration, MCP-aware parsing (tool names, arg keys), event bus highlights (PUBLISH/SUBSCRIBE/UNSUBSCRIBE), shortened MCP connection IDs, directional arrows, and sanitized log output; skips noisy paths like/api/auth/get-sessionand faviconapi/app.ts, conditionally usesdevLogger()in non-production andhono/loggerin productionWritten by Cursor Bugbot for commit f7e71bc. This will update automatically on new commits. Configure here.