Require email verification before MCP setup and preserve media results - #734
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR adds pending email verification with preserved redirects, gates onboarding and MCP OAuth access until verification, centralizes verification prompts, and introduces bounded passthrough handling for non-text MCP content across execution and persistence. ChangesEmail verification flow
MCP content passthrough
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
July 12, 2026 18:42
Contributor
|
🔎 Preview deployed: https://kody-pr-734.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/client/routes/login.tsx (1)
354-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify passkey redirect to a single
resolvePasswordAuthRedirectcall.The two-branch pattern with an early
returnis unnecessary —resolvePasswordAuthRedirectalready handlesrequiresTwoFactorinternally. A single call mirrors the email/password flow on lines 269-277 and eliminates the conditional.♻️ Proposed refactor
if ( !verificationResponse.ok || verificationPayload?.ok !== true ) { const errorMessage = typeof verificationPayload?.error === 'string' ? verificationPayload.error : 'Passkey sign-in failed.' setState('error', errorMessage) return } - if (verificationPayload.requiresTwoFactor === true) { - window.location.assign( - resolvePasswordAuthRedirect({ - mode: 'login', - requiresTwoFactor: true, - redirectTo: getCurrentRedirectTo(handle), - }), - ) - return - } window.location.assign( resolvePasswordAuthRedirect({ mode: 'login', + requiresTwoFactor: verificationPayload.requiresTwoFactor === true, redirectTo: getCurrentRedirectTo(handle), }), )🤖 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/client/routes/login.tsx` around lines 354 - 369, Simplify the passkey redirect flow by removing the requiresTwoFactor conditional and early return, then invoke resolvePasswordAuthRedirect once with mode, requiresTwoFactor, and redirectTo. Preserve the existing redirect target and let resolvePasswordAuthRedirect handle the two-factor value internally, matching the email/password flow.
🤖 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/client/routes/oauth-authorize.tsx`:
- Around line 289-292: Update the email-verification handling in the
authorization flow so the refreshed result from fetchSessionInfo() is
authoritative after an email_verification_required response. In the logic around
the approval and verification prompt checks, including the paths corresponding
to info.emailVerified and related session-state handling, derive verification
status from the refreshed session rather than stale authorization metadata,
ensuring the prompt appears and approval is blocked when required.
In `@packages/worker/client/routes/pending-verification.tsx`:
- Around line 138-150: Update handleContinue to wrap fetchSessionInfo and the
subsequent verification flow in the same error-handling pattern used by
loadPending and handleResend. On failure, set the appropriate error tone and
user-facing message, then call handle.update() so rejected requests do not leave
the button without feedback.
In `@packages/worker/src/app/handlers/verify-email.ts`:
- Around line 73-76: Update the email verification flow around the verification
transaction/link and its success response to preserve the normalized OAuth
resume target from the original request. Carry that target through verification,
then derive the success CTA’s href from it instead of always using the literal
/onboarding path, while retaining the existing onboarding fallback and covering
the verify-to-CTA OAuth resume path.
---
Nitpick comments:
In `@packages/worker/client/routes/login.tsx`:
- Around line 354-369: Simplify the passkey redirect flow by removing the
requiresTwoFactor conditional and early return, then invoke
resolvePasswordAuthRedirect once with mode, requiresTwoFactor, and redirectTo.
Preserve the existing redirect target and let resolvePasswordAuthRedirect handle
the two-factor value internally, matching the email/password flow.
🪄 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: 343c99e0-3bf6-4f88-b732-6ebfbc470540
📒 Files selected for processing (51)
docs/contributing/architecture/authentication.mddocs/contributing/architecture/mcp-client-servers.mddocs/contributing/architecture/primitives.yamldocs/contributing/architecture/remote-connectors.mddocs/use/connect-your-agent.mddocs/use/execute.mddocs/use/raw-content-blocks.mddocs/use/troubleshooting.mde2e/invite-signup-verification.spec.tspackages/worker/client/routes/account.tsxpackages/worker/client/routes/email-verification-prompt.tsxpackages/worker/client/routes/home.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/login.tsxpackages/worker/client/routes/oauth-authorize.tsxpackages/worker/client/routes/onboarding-banner.tsxpackages/worker/client/routes/onboarding.tsxpackages/worker/client/routes/pending-verification-path.node.test.tspackages/worker/client/routes/pending-verification-path.tspackages/worker/client/routes/pending-verification.tsxpackages/worker/client/routes/resolve-password-auth-redirect.node.test.tspackages/worker/client/routes/resolve-password-auth-redirect.tspackages/worker/client/routes/verify-email.tsxpackages/worker/src/app/auth-redirect.tspackages/worker/src/app/email-verification.tspackages/worker/src/app/handlers/account.tspackages/worker/src/app/handlers/home.tspackages/worker/src/app/handlers/onboarding.node.test.tspackages/worker/src/app/handlers/onboarding.tspackages/worker/src/app/handlers/pending-verification.node.test.tspackages/worker/src/app/handlers/pending-verification.tspackages/worker/src/app/handlers/verify-email.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/onboarding-data.node.test.tspackages/worker/src/app/onboarding-data.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/app/safe-redirect.tspackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/mcp/capabilities/mcp-server/index.tspackages/worker/src/mcp/capabilities/remote-connector/index.tspackages/worker/src/mcp/downstream-mcp-result.node.test.tspackages/worker/src/mcp/downstream-mcp-result.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/oauth-handlers.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/package-invocations/service.tspackages/worker/tsconfig-client.json
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d85b503. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/client/routes/onboarding-redirect.node.test.ts (1)
13-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest expectations use
encodeURIComponentbut implementation usesURLSearchParams.The implementations of
buildAuthLinkandbuildPendingVerificationPath(fromsafe-redirect.ts/pending-verification-path.ts) serialize query parameters withnew URLSearchParams(...).toString(), which usesapplication/x-www-form-urlencodedencoding. The test computes expected values withencodeURIComponent. These two encoders differ for space (%20vs+),!,',(,), and~. The current test data (/oauth/authorize?client_id=demo&state=abc) contains none of these characters, so the tests pass — but if a future test case adds any of them, the expectations will silently break.Consider building expected values with
URLSearchParamsto match the implementation exactly:♻️ Optional refactor for encoding consistency
test('onboarding redirect helpers preserve safe redirectTo and reject open redirects', () => { const oauthResume = '/oauth/authorize?client_id=demo&state=abc' + const encode = (value: string) => new URLSearchParams({ redirectTo: value }).toString().slice('redirectTo='.length) - expect(buildOnboardingPath(null)).toBe(onboardingPath) - expect(buildOnboardingPath(oauthResume)).toBe( - `/onboarding?redirectTo=${encodeURIComponent(oauthResume)}`, - ) + expect(buildOnboardingPath(null)).toBe(onboardingPath) + expect(buildOnboardingPath(oauthResume)).toBe( + `/onboarding?redirectTo=${encode(oauthResume)}`, + ) expect(buildOnboardingPath('https://evil.example')).toBe(onboardingPath) expect(buildOnboardingPath('/\\evil.example')).toBe(onboardingPath) - expect(resolveOnboardingPendingVerificationPath(null)).toBe( - '/pending-verification', - ) - expect(resolveOnboardingPendingVerificationPath(oauthResume)).toBe( - `/pending-verification?redirectTo=${encodeURIComponent(oauthResume)}`, - ) + expect(resolveOnboardingPendingVerificationPath(null)).toBe( + '/pending-verification', + ) + expect(resolveOnboardingPendingVerificationPath(oauthResume)).toBe( + `/pending-verification?redirectTo=${encode(oauthResume)}`, + ) expect(resolveOnboardingPendingVerificationPath('https://evil.example')).toBe( '/pending-verification', ) expect(resolveOnboardingPendingVerificationPath('/\\evil.example')).toBe( '/pending-verification', ) - expect(resolveOnboardingLoginPath(null)).toBe( - '/login?redirectTo=%2Fonboarding', - ) - expect(resolveOnboardingLoginPath(oauthResume)).toBe( - `/login?redirectTo=${encodeURIComponent(buildOnboardingPath(oauthResume))}`, - ) + expect(resolveOnboardingLoginPath(null)).toBe( + '/login?redirectTo=%2Fonboarding', + ) + expect(resolveOnboardingLoginPath(oauthResume)).toBe( + `/login?redirectTo=${encode(buildOnboardingPath(oauthResume))}`, + ) expect(resolveOnboardingLoginPath('https://evil.example')).toBe( '/login?redirectTo=%2Fonboarding', ) })🤖 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/client/routes/onboarding-redirect.node.test.ts` around lines 13 - 37, Update the expectations in the onboarding redirect tests to serialize redirectTo values with URLSearchParams, matching buildOnboardingPath, resolveOnboardingPendingVerificationPath, and resolveOnboardingLoginPath. Replace encodeURIComponent-based expected query strings while preserving the existing redirect values and unsafe-redirect assertions.
🤖 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.
Nitpick comments:
In `@packages/worker/client/routes/onboarding-redirect.node.test.ts`:
- Around line 13-37: Update the expectations in the onboarding redirect tests to
serialize redirectTo values with URLSearchParams, matching buildOnboardingPath,
resolveOnboardingPendingVerificationPath, and resolveOnboardingLoginPath.
Replace encodeURIComponent-based expected query strings while preserving the
existing redirect values and unsafe-redirect assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: afb6d0ad-f84c-4be9-8d1d-b4a2c6788825
📒 Files selected for processing (4)
e2e/invite-signup-verification.spec.tspackages/worker/client/routes/onboarding-redirect.node.test.tspackages/worker/client/routes/onboarding-redirect.tspackages/worker/client/routes/onboarding.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/invite-signup-verification.spec.ts
- packages/worker/client/routes/onboarding.tsx
This was referenced Jul 13, 2026
This was referenced Jul 16, 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.
Summary
executewith bounded media-specific limits.Validation
npm run validatepasses: format, lint, typecheck, 825 unit/worker tests, 14 Playwright tests, and 2 MCP E2E tests.602eb2af./onboardingredirects to verification.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@025ea00a· Head:602eb2afClassification: extends — changes email-verification gates, MCP OAuth authorization, and downstream MCP result transport without adding a primitive.
Primitives touched
app-uiapp-sessionsmcp-oauthmcp-servercapability-registrymcp-client-serversremote-connectorspackage-runtimeSystem map
Email verification gates onboarding and OAuth grants while downstream media flows through trusted markers into MCP responses and bounded persistence.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
userId.Summary by CodeRabbit