feat(auth): Add email verify regex bypass#20273
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for bypassing sign-in email confirmation based on a configurable regex, while keeping the existing allowlist for backward compatibility.
Changes:
- Introduced
skipForEmailRegexconfiguration option (convict) with an env var. - Updated login flow to bypass confirmation when the email matches either an allowlist entry or the configured regex.
- Added unit tests covering regex-based bypass behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/routes/account.ts | Adds regex-based bypass check in the login confirmation logic. |
| packages/fxa-auth-server/lib/routes/account.spec.ts | Adds test coverage for regex-based bypass behavior. |
| packages/fxa-auth-server/config/index.ts | Introduces new convict config entry/env var for regex bypass. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.skipConfirmationForEmailAddresses?.includes(lowerCaseEmail) || | ||
| // use both as a backward compatability and eventually remove | ||
| // the array of emails in favor of just a regex which is more flexible | ||
| this.skipConfirmationForEmailRegex?.test(lowerCaseEmail); |
There was a problem hiding this comment.
Calling .test() on a shared RegExp instance can be stateful if the configured regex includes the g or y flags (because lastIndex is mutated), leading to incorrect allow/deny behavior across requests. To avoid this, either (a) disallow g/y in validation, (b) reset lastIndex = 0 before testing, or (c) test against a cloned RegExp instance without g/y.
| it('should skip sign-in confirmation for email matching regex', () => { | ||
| setupSkipForEmailRegex('qa-test@example.com', /.+@example\.com$/); | ||
|
|
||
| return runTest(route, mockRequest, (response: any) => { | ||
| expect(mockDB.createSessionToken.callCount).toBe(1); | ||
| const tokenData = mockDB.createSessionToken.getCall(0).args[0]; | ||
| expect(tokenData.tokenVerificationId).toBeFalsy(); | ||
| expect(response.emailVerified).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new behavior depends on a configured RegExp instance, but tests currently don't cover the stateful-regex edge case (e.g., a configured /.+@example\\.com$/g), which can produce flaky bypass decisions due to lastIndex mutation. Add a test that sets a g (and/or y) regex and asserts consistent behavior across multiple login attempts, or enforce validation that rejects these flags and test that validation path.
| mockConfig.oauth = {}; | ||
| mockConfig.signinConfirmation = {}; | ||
| mockConfig.signinConfirmation.skipForEmailAddresses = []; | ||
| mockConfig.signinConfirmation.skipForEmailRegex = /^$/; |
There was a problem hiding this comment.
Is it redundant to have both skipForEmailAddresses and skipForEmailRegex?
There was a problem hiding this comment.
Probably! My only thinking is that this wouldn't break anything and have weird timing with updates to webservices-infra to deploy updates to both. This way we can deploy the new code, and add the regex to webservices, test to make sure it works, then go back and rip out the old version
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
Makes sense to me. Thanks for the update!
Because: - We currently use a list of exact matching emails to allow bypassing verification emails This Commit: - Adds a regex variable option to make bypassing a bit more flexible - Leaves old variable in place so we don't break existing functionality until code is live and webservices-infra is updated - Adds tests
nshirley
force-pushed
the
nshirley/email-skip-verify-regex
branch
from
630bec3 to
104f397
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-13352
Sister webservices-infra pr: https://github.com/mozilla/webservices-infra/pull/10328/changes
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.