Repository navigation
Add a friendly browser landing page for the MCP endpoint - #1185
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe MCP endpoint now detects unauthenticated browser navigation requests and returns an onboarding page. Protocol requests continue through authentication and return consistent JSON ChangesMCP browser guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
kody-bot
marked this pull request as ready for review
August 3, 2026 22:33
![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
🧹 Nitpick comments (1)
packages/worker/src/mcp-auth.workers.test.ts (1)
372-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the no-store response contract.
packages/worker/src/mcp-auth.tssetsCache-Control: no-store, but this test does not verify it. Add a header assertion so a future change cannot make the browser landing page cacheable.Proposed fix
expect(browserResponse.headers.get('Content-Type')).toBe( 'text/html; charset=utf-8', ) + expect(browserResponse.headers.get('Cache-Control')).toBe('no-store') expect(await browserResponse.text()).toContain(🤖 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 `@packages/worker/src/mcp-auth.workers.test.ts` around lines 372 - 380, Add an assertion in the browser landing-page response test alongside the existing header checks to verify the Cache-Control header is exactly no-store, preserving the contract established by the MCP authentication response.
🤖 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 `@packages/worker/src/mcp-auth.ts`:
- Around line 88-110: Update acceptsMediaType and isBrowserMcpNavigation to
parse and compare quality values for text/html, application/json, and
text/event-stream rather than rejecting based on mere presence. Return true only
when HTML has a positive quality greater than both protocol media types,
treating omitted quality as 1 and q=0 as unacceptable; add regression coverage
for weighted preferences and q=0 JSON/SSE values.
---
Nitpick comments:
In `@packages/worker/src/mcp-auth.workers.test.ts`:
- Around line 372-380: Add an assertion in the browser landing-page response
test alongside the existing header checks to verify the Cache-Control header is
exactly no-store, preserving the contract established by the MCP authentication
response.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d72f51c-9a2e-4d9a-8777-174ad9578ef2
📒 Files selected for processing (2)
packages/worker/src/mcp-auth.tspackages/worker/src/mcp-auth.workers.test.ts
Comment on lines
+88
to
+110
| function acceptsMediaType(accept: string, mediaType: string) { | ||
| return accept.split(',').some((range) => { | ||
| const [type, ...parameters] = range.trim().split(';') | ||
| if (type?.trim() !== mediaType) return false | ||
| const quality = parameters | ||
| .map((parameter) => parameter.trim()) | ||
| .find((parameter) => parameter.startsWith('q=')) | ||
| return quality ? Number(quality.slice(2)) > 0 : true | ||
| }) | ||
| } | ||
|
|
||
| function isBrowserMcpNavigation(request: Request) { | ||
| if (request.method !== 'GET' || request.headers.has('Authorization')) { | ||
| return false | ||
| } | ||
| const accept = request.headers.get('Accept')?.toLowerCase() | ||
| if (!accept) return false | ||
| return ( | ||
| acceptsMediaType(accept, 'text/html') && | ||
| !accept.includes('text/event-stream') && | ||
| !accept.includes('application/json') | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare Accept quality values before rejecting browser navigation.
Lines 107-108 reject any request that includes JSON or SSE. A request with text/html;q=1, application/json;q=0.1 prefers HTML but receives a JSON 401. This conflicts with the browser-navigation contract.
Parse the quality value for each media type. Return the landing page only when the HTML quality is greater than the JSON and SSE qualities. Add regression cases for weighted and q=0 protocol media types.
Proposed fix
-function acceptsMediaType(accept: string, mediaType: string) {
- return accept.split(',').some((range) => {
+function mediaTypeQuality(accept: string, mediaType: string) {
+ return accept.split(',').reduce((highest, range) => {
const [type, ...parameters] = range.trim().split(';')
- if (type?.trim() !== mediaType) return false
- const quality = parameters
+ const qualityParameter = parameters
.map((parameter) => parameter.trim())
.find((parameter) => parameter.startsWith('q='))
- return quality ? Number(quality.slice(2)) > 0 : true
- })
+ const quality = qualityParameter
+ ? Number(qualityParameter.slice(2))
+ : 1
+ if (
+ type?.trim() !== mediaType ||
+ !Number.isFinite(quality) ||
+ quality < 0 ||
+ quality > 1
+ ) {
+ return highest
+ }
+ return Math.max(highest, quality)
+ }, 0)
}
function isBrowserMcpNavigation(request: Request) {
+ // ...
+ const htmlQuality = mediaTypeQuality(accept, 'text/html')
return (
- acceptsMediaType(accept, 'text/html') &&
- !accept.includes('text/event-stream') &&
- !accept.includes('application/json')
+ htmlQuality > 0 &&
+ htmlQuality > mediaTypeQuality(accept, 'text/event-stream') &&
+ htmlQuality > mediaTypeQuality(accept, 'application/json')
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function acceptsMediaType(accept: string, mediaType: string) { | |
| return accept.split(',').some((range) => { | |
| const [type, ...parameters] = range.trim().split(';') | |
| if (type?.trim() !== mediaType) return false | |
| const quality = parameters | |
| .map((parameter) => parameter.trim()) | |
| .find((parameter) => parameter.startsWith('q=')) | |
| return quality ? Number(quality.slice(2)) > 0 : true | |
| }) | |
| } | |
| function isBrowserMcpNavigation(request: Request) { | |
| if (request.method !== 'GET' || request.headers.has('Authorization')) { | |
| return false | |
| } | |
| const accept = request.headers.get('Accept')?.toLowerCase() | |
| if (!accept) return false | |
| return ( | |
| acceptsMediaType(accept, 'text/html') && | |
| !accept.includes('text/event-stream') && | |
| !accept.includes('application/json') | |
| ) | |
| } | |
| function mediaTypeQuality(accept: string, mediaType: string) { | |
| return accept.split(',').reduce((highest, range) => { | |
| const [type, ...parameters] = range.trim().split(';') | |
| const qualityParameter = parameters | |
| .map((parameter) => parameter.trim()) | |
| .find((parameter) => parameter.startsWith('q=')) | |
| const quality = qualityParameter | |
| ? Number(qualityParameter.slice(2)) | |
| : 1 | |
| if ( | |
| type?.trim() !== mediaType || | |
| !Number.isFinite(quality) || | |
| quality < 0 || | |
| quality > 1 | |
| ) { | |
| return highest | |
| } | |
| return Math.max(highest, quality) | |
| }, 0) | |
| } | |
| function isBrowserMcpNavigation(request: Request) { | |
| if (request.method !== 'GET' || request.headers.has('Authorization')) { | |
| return false | |
| } | |
| const accept = request.headers.get('Accept')?.toLowerCase() | |
| if (!accept) return false | |
| const htmlQuality = mediaTypeQuality(accept, 'text/html') | |
| return ( | |
| htmlQuality > 0 && | |
| htmlQuality > mediaTypeQuality(accept, 'text/event-stream') && | |
| htmlQuality > mediaTypeQuality(accept, 'application/json') | |
| ) | |
| } |
🤖 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 `@packages/worker/src/mcp-auth.ts` around lines 88 - 110, Update
acceptsMediaType and isBrowserMcpNavigation to parse and compare quality values
for text/html, application/json, and text/event-stream rather than rejecting
based on mere presence. Return true only when HTML has a positive quality
greater than both protocol media types, treating omitted quality as 1 and q=0 as
unacceptable; add regression coverage for weighted preferences and q=0 JSON/SSE
values.
Contributor
|
🔎 Preview deployed: https://kody-pr-1185.kody-a99.workers.dev Worker: Mocks:
|
This was referenced Aug 3, 2026
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
3 participants
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.
Intent
Prevent non-technical users from seeing a raw OAuth error when they paste Kody's MCP URL into a browser, without changing MCP protocol behavior.
Summary
/mcp./onboardingguide.Testing
npm run validate— passed (1,920 unit/Workers tests, 3 MCP tests, E2E, lint, format, types, build, and repository checks).npm run test:workers -- packages/worker/src/mcp-auth.workers.test.ts— 5 tests passed.System changes
System recap — extends an existing primitive (medium risk)
Mode: recap · Base:
main@a4c25471· Head:84f2baf8Classification: extends — this changes the unauthenticated browser response at the OAuth-protected MCP entrypoint while preserving protocol challenges.
Primitives touched
mcp-oauthSystem map
Browser GETs receive guidance while MCP transport requests continue through the existing OAuth challenge path.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
401JSON OAuth challenge200HTML guidanceConductor report
Status: merged and verified, but production rollout is blocked by a pre-existing cross-track migration gate.
Evidence: PR #1185 squash-merged as
89ac5513; local authoritative gate and all ready-for-review/post-merge CI passed; curl and browser walkthrough prove the HTML/protocol split.What remains: production deploy run 30859923182 failed before Worker upload because migration
0135-drop-legacy-email-graph.sqlfrom PR #1174 correctly rejected missing/mismatched destructive-drop approval data (CHECK constraint failed: value = 1). Production remains on the previous build until Track Email's fresh backup/approval procedure succeeds and deploy is rerun.Summary by CodeRabbit
New Features
/onboardingwithout displaying an authentication challenge.Bug Fixes