Conversation
There was a problem hiding this comment.
Pull request overview
Updates email validation to reject non-ASCII characters in the email local part to avoid creating accounts that can’t receive mail via AWS SES (no SMTPUTF8 support), and updates test expectations accordingly.
Changes:
- Stop using
punycode.toASCII()for validating the local part; validate local part directly againstEMAIL_USER. - Update/add Jest tests to expect unicode local parts to be invalid while keeping unicode domains valid.
- Add a mail-sending-time guard that skips sending when an address has a non-ASCII local part, with logging/metrics to reduce Sentry noise from existing accounts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/senders/email.js | Adds a runtime skip for non-ASCII local parts (logging + statsd metric) before attempting SES send/bounce checks. |
| packages/fxa-auth-server/lib/routes/validators.spec.ts | Updates existing unicode-local-part expectations and adds explicit coverage for additional non-ASCII local-part examples. |
| packages/fxa-auth-server/lib/routes/validators.js | Removes punycode conversion from local-part validation; keeps punycode conversion for the domain part. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip non-ASCII local parts. SES doesn't support SMTPUTF8, so these | ||
| // always fail at RCPT TO. Silently return (like bounce check below) | ||
| // to avoid Sentry noise from pre-existing accounts. | ||
| const [localPart] = to.split('@'); | ||
| if (/[^\x20-\x7E]/.test(localPart)) { | ||
| statsd.increment('email.nonAsciiLocalPart', { template }); | ||
| log.warn('mailer.send.nonAsciiLocalPart', { to, template }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The check for “non-ASCII local parts” uses /[^\x20-\x7E]/, which flags any non-printable ASCII control characters as well (e.g. \n, \t) and would count/log them under the nonAsciiLocalPart metric even though they’re still ASCII. Consider changing the regex to test strictly for non-ASCII bytes (e.g. [^\x00-\x7F]) or update the comment/metric name to match what’s actually being detected.
| // Skip non-ASCII local parts. SES doesn't support SMTPUTF8, so these | ||
| // always fail at RCPT TO. Silently return (like bounce check below) | ||
| // to avoid Sentry noise from pre-existing accounts. | ||
| const [localPart] = to.split('@'); | ||
| if (/[^\x20-\x7E]/.test(localPart)) { | ||
| statsd.increment('email.nonAsciiLocalPart', { template }); | ||
| log.warn('mailer.send.nonAsciiLocalPart', { to, template }); | ||
| return; |
There was a problem hiding this comment.
The PR description lists only validation/test updates, but this change also alters runtime behavior by silently skipping sends for some existing accounts (and adds new logging/metrics). Please update the PR description (or add a note) to reflect this additional behavior change so reviewers/operators don’t miss it.
| it('isValidEmailAddress rejects non-ASCII local parts', () => { | ||
| // Accented letters | ||
| expect(validators.isValidEmailAddress('andrée.torta@free.fr')).toBe(false); | ||
| expect(validators.isValidEmailAddress('bendegúz@gmail.com')).toBe(false); |
There was a problem hiding this comment.
I'm sure these aren't real but let's use restmail or example here please
|
|
||
| it('isValidEmailAddress rejects non-ASCII local parts', () => { | ||
| // Accented letters | ||
| expect(validators.isValidEmailAddress('andrée.torta@free.fr')).toBe(false); |
vbudhram
force-pushed
the
fxa-12710
branch
2 times, most recently
from
4c609c8 to
38a6398
Compare
| // Note: punycode.toASCII() is intentionally NOT used here because | ||
| // it would encode non-ASCII chars (e.g. 'andrée' → 'xn--andre-9ua'), | ||
| // masking the problem. | ||
| if (!EMAIL_USER.test(parts[0])) { |
Add validation to reject email addresses with non-ASCII characters in the local part (before the @). Updates validators, email sender, and related test files to handle non-ASCII rejection consistently.
Because
punycode.toASCII()was silently encoding non-ASCII local parts (e.g.andrée→xn--andre-9ua), masking the problem at validation timeThis pull request
punycode.toASCII()from the local part check invalidators.jssoEMAIL_USERrejects non-ASCII characters directlyvalidators.spec.tsfor unicode local parts (Δ٢,🦀🧙) fromtruetofalseIssue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-12710
Checklist
Notes
I guess this error has been around for a long time, perhaps masked by Sentry noise or issue linking. With the updated Sentry it turns out that emails local part was using unicode with AWS does not support. The frontend would create these users but they could never get verified.