Log request paths with the query string filtered - #484
Conversation
Request logging records `request.url` verbatim, and the logger's redaction only matches object keys, so a URL held as a string passes through untouched. Routes that accept credentials in the query string (the MCP endpoint takes `?token=`) therefore wrote those values into log lines. Filter sensitive parameters out of the URL at the three places a raw URL is logged, and apply the same filtering inside the logger's own redaction so future callers are covered without having to remember the helper. The sensitive-parameter list stays a single definition in the logger package. Docs and the MCP settings page now lead with the `Authorization: Bearer` form and present the query parameter as the fallback for clients that cannot set headers. Query-parameter auth itself is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds shared URL query sanitization, applies it to API request and error logging, adds focused tests, and updates MCP documentation and client examples to prefer bearer-header authentication. ChangesCredential redaction and MCP authentication
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to MCP setup now prioritizes bearer authentication, but self-hosted HTTP endpoints may expose copied bearer credentials in transit because the UI does not establish or warn about an HTTPS requirement. Resolve the endpoint scheme contract or add an HTTP safeguard before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 13 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
1 participant
What changed and why
Request logging recorded
request.urlverbatim. The logger's redaction only matches object keys, so a URL held as a plain string went into the log line untouched. Any route that accepts something sensitive in the query string ended up with that value in the logs; the MCP endpoint accepts?token=, so its tokens were the concrete case.This adds
sanitizeUrl, which keeps the path and every ordinary parameter and replaces the value of any parameter whose lowercased name matches the sensitive-key list the logger already uses. Matching is substring and case-insensitive, the same ruleredactSensitiveapplies to object keys. A URL with no query string, or an empty one, comes back unchanged, and a malformed query string does not throw. The query is rebuilt from the raw text rather than throughURLSearchParamsso untouched values keep their original encoding.The filter is applied at the three places a raw URL is logged, and inside
redactSensitiveitself for string values under a key containingurl. The second one is the backstop: code that logs a URL later is covered without having to remember the helper.SENSITIVE_KEY_PATTERNSis now exported frompackages/loggerso there is one list, not two copies.apps/api/src/utils/sanitize-url.tsre-exports the implementation under the name the API side uses.Docs and the MCP settings page led with the query-parameter form of the endpoint. They now lead with
Authorization: Bearerand show the query form as the fallback for clients that cannot set headers. Query-parameter auth still works exactly as before;packages/mcp/src/auth.tsis untouched.Where the raw URL was logged
apps/api/src/hooks/request-logging.hook.ts:49—url: request.urlin the non-tRPC branch. The tRPC branch at line 31 already dropped the query and is unchanged.apps/api/src/app.ts:417—url: request.urlin the error handler's request context. That context is now built bybuildErrorRequestContextinapps/api/src/utils/errors.tsso it can be tested without standing up the whole app; the fields are the same. Itsqueryandheadersare objects, so the logger already redacted those by key.apps/api/src/utils/rate-limiter.ts:52—url: req.urlin the rate-limit warning.packages/logger/index.ts:25— theSENSITIVE_KEY_PATTERNSlist, andredactSensitiveat lines 49-82, which only ever looked at keys.Docs and UI:
apps/public/content/docs/mcp/index.mdx(authentication, token format, Claude Desktop and Claude Code sections),apps/public/content/features/mcp.json(setup step 3 and the authentication FAQ answer),apps/start/src/routes/_app.$organizationId.$projectId.settings._tabs.mcp.tsx:35and:177.Tests
apps/api/src/utils/sanitize-url.test.ts— replacement and path preservation, other parameters kept,TOKEN/Token/accessToken, repeated parameters, several sensitive parameters in one URL, no query string, empty query string, malformed query string, encoding preserved.apps/api/src/hooks/request-logging.hook.test.ts— the logged object for a non-tRPC request contains no substring of the token value; the tRPC branch still logs the bare path;/trackbodies are still attached now that the URL is filtered.apps/api/src/utils/errors.test.tsandapps/api/src/utils/rate-limiter.test.ts— the other two payloads, so all three sites are pinned.packages/logger/index.test.ts—redactSensitive({ url: '/x?token=abc&foo=1' })replaces the token and keepsfoo=1, a non-stringurlvalue still goes through the existing recursion, key-based redaction is unchanged. Needed avitest.config.tsin that package, matching the other packages.Ran:
vitest runinpackages/logger(6 tests) and inapps/apiforsrc/utils,src/hooks,src/bots(11 files, 81 tests) — all pass.tsc --noEmitis clean for@openpanel/logger,@openpanel/apiandstartafterpnpm db:codegen. The API integration suites that need Postgres, ClickHouse and Redis were not run; nothing was available to connect to in this environment.Left out on purpose
headersblock.SENSITIVE_KEY_PATTERNSincludes short entries likeip, so a parameter named e.g.recipientwill be replaced too. That is the existing behaviour for object keys and now applies to query parameters as well. Narrowing the list is a change with its own blast radius and is not part of this.biome checkalready reports on the files touched here were left alone.Summary by CodeRabbit
Security
Authentication
Authorization: Bearerheaders instead of embedding tokens in URLs.Documentation