Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates rate-limit email exclusions to consider alias-preserving input while retaining normalized keys and telemetry.
Changes:
- Passes the original email into
RateLimit.skip(). - Matches exclusions against original and normalized emails.
- Adds coverage for aliases, fallback matching, and BigQuery output.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/fxa-auth-server/lib/customs.js |
Forwards original emails to rate-limit checks. |
packages/fxa-auth-server/lib/customs.spec.ts |
Tests original-email forwarding. |
libs/accounts/rate-limit/src/lib/rate-limit.ts |
Extends ignored-email matching. |
libs/accounts/rate-limit/src/lib/rate-limit.spec.ts |
Tests matching and telemetry behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+123
to
+127
| const emails = [nonNormalizedEmail, opts.email].filter( | ||
| (x): x is string => x != null | ||
| ); | ||
| const ignoredEmail = this.config.ignoreEmails?.some((x) => | ||
| emails.some((email) => email.match(x)) |
## Because
- A stage bypass pattern like `^.*\+srl\d{0,4}@mozilla\.com$` never matched, so rate limits could not be skipped for those test accounts.
- `customs.js` normalizes the email before it reaches the rate-limit library. Normalization strips the `+suffix` that the pattern needs.
## This pull request
- Adds an optional `nonNormalizedEmail` parameter to `RateLimit.skip()`. It matches `config.ignoreEmails` against the raw email and the normalized one.
- Adds the lowercased raw email as a third match candidate. Normalization lowercases before it strips the `+suffix`, so a mixed-case input like `User+srl1@Mozilla.com` matched neither of the first two forms.
- Passes the raw email through `CustomsClient.checkV2()` from `check()` and `checkAuthenticated()`.
- Keeps `RateLimit.check()` on the normalized email, so a `+suffix` address cannot evade rate limits.
- Keeps the normalized email on the BigQuery row that a skipped check writes.
- Adds specs for the `+suffix` match and for the normalized fallback.
## Issue that this pull request solves
Closes: FXA-13194
5 tasks
nshirley
approved these changes
Aug 20, 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
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.
Because
^.*\+srl\d{0,4}@mozilla\.com$never matched, so rate limits could not be skipped for those test accounts.customs.jsnormalizes the email before it reaches the rate-limit library. Normalization strips the+suffixthat the pattern needs.This pull request
nonNormalizedEmailparameter toRateLimit.skip(). It matchesconfig.ignoreEmailsagainst the raw email and the normalized one.+suffix, so a mixed-case input likeUser+srl1@Mozilla.commatched neither of the first two forms.CustomsClient.checkV2()fromcheck()andcheckAuthenticated().RateLimit.check()on the normalized email, so a+suffixaddress cannot evade rate limits.+suffixmatch and for the normalized fallback.Issue that this pull request solves
Closes: FXA-13194
Checklist
Put an
xin the boxes that applyHow to review (Optional)
skip()inlibs/accounts/rate-limit/src/lib/rate-limit.ts, and thecheckV2()call site inpackages/fxa-auth-server/lib/customs.js.rate-limit.ts, thencustoms.js, then the two spec files.skip()matches on the raw email and the normalized one. A match on the raw email only would drop the trim and the lowercase that an existing pattern can depend on. Read the deployment note below.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Warning: audit the deployed
ignoreEmailsvalues before you merge this.The bypass is config-gated, not code-gated.
config.ignoreEmailsis a per-environment list of regexes. This fix makes those patterns match the raw address as well as the normalized one. Any pattern already in a production config becomes more permissive, because it then also matchesuser+anything@.... The owner of the prod config must make that call.BlockOnandBlockOnOptsdo not change. A raw email is not a new blocking dimension, so it stays out of the mapped type.