fix: Handle WorkOS email verification during managed sign-in#5814
fix: Handle WorkOS email verification during managed sign-in#5814marcindobry merged 9 commits intomasterfrom
Conversation
marcindobry
changed the title
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature implementation, but two important fixes are needed for secure error handling and resilient session-save failure behavior.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Prevent upstream error-message leakage; return only sanitized fallback text.
packages/server/lib/controllers/v1/account/managed/postVerification.ts - Handle saveSession failures locally; redirect with friendly signin error.
packages/server/lib/controllers/v1/account/managed/getCallback.ts
Review Details
📁 13 files reviewed | 💬 2 comments
Instruction Files
└── .claude/
├── agents/
│ └── nango-docs-migrator.md
└── skills
👍 / 👎 individual comments to help improve reviews for you
| if ( | ||
| workosErr.rawData?.code === 'email_verification_required' && | ||
| workosErr.rawData.pending_authentication_token && | ||
| workosErr.rawData.email && | ||
| workosErr.rawData.email_verification_id | ||
| ) { | ||
| req.session.managedAuthEmailVerification = { | ||
| email: workosErr.rawData.email, | ||
| pendingAuthenticationToken: workosErr.rawData.pending_authentication_token, | ||
| emailVerificationId: workosErr.rawData.email_verification_id, | ||
| state: query.state | ||
| }; | ||
| await saveSession(req); | ||
| res.redirect(`${basePublicUrl}/signin/verify`); |
There was a problem hiding this comment.
[Reliability] If saveSession rejects (e.g., session store is unavailable), the rejection escapes the catch block and asyncWrapper forwards it to Express's global error handler via next(err), producing a 500 instead of redirecting to /signin with a meaningful error.
Wrap saveSession in its own try/catch here to ensure a user-friendly redirect on failure:
| if ( | |
| workosErr.rawData?.code === 'email_verification_required' && | |
| workosErr.rawData.pending_authentication_token && | |
| workosErr.rawData.email && | |
| workosErr.rawData.email_verification_id | |
| ) { | |
| req.session.managedAuthEmailVerification = { | |
| email: workosErr.rawData.email, | |
| pendingAuthenticationToken: workosErr.rawData.pending_authentication_token, | |
| emailVerificationId: workosErr.rawData.email_verification_id, | |
| state: query.state | |
| }; | |
| await saveSession(req); | |
| res.redirect(`${basePublicUrl}/signin/verify`); | |
| if ( | |
| workosErr.rawData?.code === 'email_verification_required' && | |
| workosErr.rawData.pending_authentication_token && | |
| workosErr.rawData.email && | |
| workosErr.rawData.email_verification_id | |
| ) { | |
| req.session.managedAuthEmailVerification = { | |
| email: workosErr.rawData.email, | |
| pendingAuthenticationToken: workosErr.rawData.pending_authentication_token, | |
| emailVerificationId: workosErr.rawData.email_verification_id, | |
| state: query.state | |
| }; | |
| try { | |
| await saveSession(req); | |
| res.redirect(`${basePublicUrl}/signin/verify`); | |
| } catch { | |
| res.redirect(`${basePublicUrl}/signin?error=sso_session_expired`); | |
| } | |
| return; | |
| } |
Context for Agents
If `saveSession` rejects (e.g., session store is unavailable), the rejection escapes the `catch` block and `asyncWrapper` forwards it to Express's global error handler via `next(err)`, producing a 500 instead of redirecting to `/signin` with a meaningful error.
Wrap `saveSession` in its own try/catch here to ensure a user-friendly redirect on failure:
```suggestion
if (
workosErr.rawData?.code === 'email_verification_required' &&
workosErr.rawData.pending_authentication_token &&
workosErr.rawData.email &&
workosErr.rawData.email_verification_id
) {
req.session.managedAuthEmailVerification = {
email: workosErr.rawData.email,
pendingAuthenticationToken: workosErr.rawData.pending_authentication_token,
emailVerificationId: workosErr.rawData.email_verification_id,
state: query.state
};
try {
await saveSession(req);
res.redirect(`${basePublicUrl}/signin/verify`);
} catch {
res.redirect(`${basePublicUrl}/signin?error=sso_session_expired`);
}
return;
}
```
File: packages/server/lib/controllers/v1/account/managed/getCallback.ts
Line: 84![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc89ed4017
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await finalizeManagedAuthentication({ | ||
| req, | ||
| res, | ||
| authorizedUser: authResponse.user, | ||
| organizationId: authResponse.organizationId, |
There was a problem hiding this comment.
postManagedEmailVerification wraps both authenticateWithEmailVerification and finalizeManagedAuthentication in one try/catch, so any local failure after a valid code (e.g., DB/user creation/login errors inside finalization) is returned as 400 invalid_verification_code. In that case users are told their code is wrong even when WorkOS verification succeeded, and operational failures are masked as client errors; this should only map WorkOS verification failures to 400 and rethrow/propagate non-WorkOS errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good feature implementation, but one important error-handling path and minor maintainability issues should be addressed before merge.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Prevent swallowing unexpected WorkOS errors; rethrow non-verification codes:
packages/server/lib/controllers/v1/account/managed/postVerification.ts - Trim constructed display names to avoid trailing whitespace artifacts:
packages/server/lib/controllers/v1/account/managed/utils.ts - Avoid tagging expected verification flow as error-like telemetry noise:
packages/server/lib/controllers/v1/account/managed/getCallback.ts
Review Details
📁 13 files reviewed | 💬 3 comments
Instruction Files
└── .claude/
├── agents/
│ └── nango-docs-migrator.md
└── skills
👍 / 👎 individual comments to help improve reviews for you
| const updatedVerification = getManagedAuthEmailVerificationFromError(err); | ||
|
|
||
| if (updatedVerification) { | ||
| await setManagedAuthEmailVerification(req, updatedVerification, verification.state); | ||
| } else if (!workosErr.rawData) { |
There was a problem hiding this comment.
[Logic] The error handling logic has an ambiguous silent-swallow path. When getManagedAuthEmailVerificationFromError(err) returns null AND workosErr.rawData is truthy (i.e. WorkOS returned a structured error that is NOT email_verification_required), the code falls through to return 400 invalid_verification_code without rethrowing. This means genuine WorkOS API errors — like rate_limit_exceeded, client_id_invalid, or other structured WorkOS errors — are silently swallowed and presented to the user as an invalid verification code, making them impossible to debug.
The condition should be inverted: rethrow if the error is unexpected, return 400 only for known invalid-code errors:
| const updatedVerification = getManagedAuthEmailVerificationFromError(err); | |
| if (updatedVerification) { | |
| await setManagedAuthEmailVerification(req, updatedVerification, verification.state); | |
| } else if (!workosErr.rawData) { | |
| if (updatedVerification) { | |
| await setManagedAuthEmailVerification(req, updatedVerification, verification.state); | |
| } else if (workosErr.rawData?.code !== 'email_verification_required' && workosErr.rawData?.code !== 'verification_code_expired' && workosErr.rawData?.code !== 'verification_code_invalid') { | |
| throw err; | |
| } |
Alternatively, explicitly enumerate the codes that should return 400 rather than relying on the presence of rawData.
Context for Agents
The error handling logic has an ambiguous silent-swallow path. When `getManagedAuthEmailVerificationFromError(err)` returns `null` AND `workosErr.rawData` is truthy (i.e. WorkOS returned a structured error that is NOT `email_verification_required`), the code falls through to return `400 invalid_verification_code` without rethrowing. This means genuine WorkOS API errors — like `rate_limit_exceeded`, `client_id_invalid`, or other structured WorkOS errors — are silently swallowed and presented to the user as an invalid verification code, making them impossible to debug.
The condition should be inverted: rethrow if the error is unexpected, return 400 only for known invalid-code errors:
```suggestion
if (updatedVerification) {
await setManagedAuthEmailVerification(req, updatedVerification, verification.state);
} else if (workosErr.rawData?.code !== 'email_verification_required' && workosErr.rawData?.code !== 'verification_code_expired' && workosErr.rawData?.code !== 'verification_code_invalid') {
throw err;
}
```
Alternatively, explicitly enumerate the codes that should return 400 rather than relying on the presence of `rawData`.
File: packages/server/lib/controllers/v1/account/managed/postVerification.ts
Line: 70| const verification = getManagedAuthEmailVerificationFromError(err); | ||
| if (verification) { | ||
| await setManagedAuthEmailVerification(req, verification, query.state); |
There was a problem hiding this comment.
[Maintainability] The span tags for workos.error_code and workos.error_message are set unconditionally for all errors (lines 60-68), including email_verification_required, before the verification check on line 71. Since this PR introduces email_verification_required as an expected, user-facing flow (not a server error), the verification-required case will create trace spans tagged with error-like metadata. While these custom tags don't inherently mark the span as errored in dd-trace (that requires span.setTag('error', true)), they could create noise in any dashboards or queries filtering on workos.error_code presence. Consider moving the getManagedAuthEmailVerificationFromError(err) check before the span tag logic, or setting a different tag (e.g. workos.flow = 'email_verification') for the expected verification case.
Context for Agents
The span tags for `workos.error_code` and `workos.error_message` are set unconditionally for all errors (lines 60-68), including `email_verification_required`, before the verification check on line 71. Since this PR introduces `email_verification_required` as an expected, user-facing flow (not a server error), the verification-required case will create trace spans tagged with error-like metadata. While these custom tags don't inherently mark the span as errored in dd-trace (that requires `span.setTag('error', true)`), they could create noise in any dashboards or queries filtering on `workos.error_code` presence. Consider moving the `getManagedAuthEmailVerificationFromError(err)` check before the span tag logic, or setting a different tag (e.g. `workos.flow = 'email_verification'`) for the expected verification case.
File: packages/server/lib/controllers/v1/account/managed/getCallback.ts
Line: 73
| return metadata; | ||
| } | ||
|
|
||
| export async function finalizeManagedAuthentication({ |
There was a problem hiding this comment.
This is mostly the code from getCallback so it can be reused
| declare module 'express-session' { | ||
| interface SessionData { | ||
| debugMode?: boolean; | ||
| managedAuthEmailVerification?: { |
There was a problem hiding this comment.
I couldn't figure out a different way of storing this data securely across requests. Any patterns we use already?
| } | ||
|
|
||
| if (invitation) { | ||
| await acceptInvitation(invitation.token); |
There was a problem hiding this comment.
can acceptInvitation throw? if yes what's responded?
|
|
||
| const validation = z | ||
| .object({ | ||
| code: z.string().trim().min(6).max(12) |
There was a problem hiding this comment.
is utils the new misc? 😉
what about auth.ts or evenmanaged.ts
There was a problem hiding this comment.
I like auth.ts :)
And yep, seems like "if in doubt, util.ts out"
|
|
||
| interface WorkOSOrganizationClient { | ||
| getOrganization(organizationId: string): Promise<{ name: string }>; | ||
| } |
There was a problem hiding this comment.
is it not exported from WorkOS sdk?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 573e790ce2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }); | ||
| } | ||
|
|
||
| if (globalEnv.features.auth) { |
There was a problem hiding this comment.
This change makes /signup available only when globalEnv.features.auth is true, but getManagedCallback still redirects invalid WorkOS callback payloads to /signup in its validation-failure branch (packages/server/lib/controllers/v1/account/managed/getCallback.ts). In managed-auth-only deployments (features.auth=false, features.managedAuth=true), a callback without a code (for example after an OAuth/provider error) now routes users to a non-existent page instead of a recoverable sign-in screen.
Useful? React with 👍 / 👎.
2 participants
WorkOS-managed Google sign-in could fail with
email_verification_required, leaving users stuck on the callback and never creating the local Nango user. This adds a managed-auth verification step that stores the pending WorkOS verification state in session, redirects users to/signin/verify, submits the verification code back to WorkOS, and then reuses the existing managed-auth finalization flow.https://linear.app/nango/issue/NAN-5232/user-cant-sign-in-email-verification-flow-from-workos-not-handled
npm run test:integration -- packages/server/lib/controllers/v1/account/managed/verification.integration.test.tsnpm run ts-buildIn addition to handling the verification challenge path, this change also broadens managed-auth support by formalizing the verification lifecycle across backend and frontend boundaries, including reusable managed-auth flow structure and end-to-end coverage to ensure the callback, verification, and completion paths behave consistently across different auth configurations.
This summary was automatically generated by @propel-code-bot