feat(admin): add email regex blocklist page#20420
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an operator-managed email regex blocklist feature (DB + shared model + admin API/UI) and enforces it during account registration to block disposable/spam domains without a deploy.
Changes:
- Introduces
emailBlocklistDB table via patch level 188 → 189 (plus rollback patch). - Adds
EmailBlocklistshared model and wires it intofxa-auth-serverregistration to reject matching emails and emit logs/metrics. - Adds admin-server REST endpoints and an admin-panel page (bulk add, file upload, per-row delete, delete-all) gated by a new
AdminPanelFeature.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-shared/db/models/auth/index.ts | Exposes the new EmailBlocklist model via shared auth models export. |
| packages/fxa-shared/db/models/auth/email-blocklist.ts | Implements CRUD and “find first matching regex” logic over the new table. |
| packages/fxa-auth-server/lib/routes/account.ts | Enforces blocklist during accountCreate, logs match details, increments statsd counter. |
| packages/fxa-admin-server/src/types.ts | Adds EmailBlocklistEntry API type. |
| packages/fxa-admin-server/src/rest/rest.module.ts | Registers the new EmailBlocklist controller. |
| packages/fxa-admin-server/src/rest/email-blocklist/email-blocklist.controller.ts | Adds list/add/delete/delete-all endpoints with feature gating and auditing. |
| packages/fxa-admin-panel/src/lib/api.ts | Adds admin API client methods for the blocklist endpoints. |
| packages/fxa-admin-panel/src/images/icon-email-blocklist.svg | Adds nav icon for the new page. |
| packages/fxa-admin-panel/src/components/PageEmailBlocklist/index.tsx | New UI page to manage patterns (bulk add + upload + delete). |
| packages/fxa-admin-panel/src/components/Nav/index.tsx | Adds nav entry for the Email Blocklist page behind feature guard. |
| packages/fxa-admin-panel/src/App.tsx | Adds routing for /email-blocklist behind feature guard. |
| packages/db-migrations/databases/fxa/target-patch.json | Bumps schema patch target to 189. |
| packages/db-migrations/databases/fxa/patches/patch-188-189.sql | Creates emailBlocklist table and updates patch level to 189. |
| packages/db-migrations/databases/fxa/patches/patch-189-188.sql | Rollback: drops table and reverts patch level to 188. |
| libs/shared/guards/src/lib/admin-panel-guard.ts | Adds AdminPanelFeature.EmailBlocklist and default permission metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
402f5e6 to
fec34d2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
17c33d8 to
c8be583
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1d6bb7b to
377fb61
Compare
There was a problem hiding this comment.
There are no tests for this controller and there is non-trivial logic in here, probably worth adding a spec file along side it
There was a problem hiding this comment.
Thanks for the review @nshirley! Agreed. I spoke with Wil yesterday and there will be multiple iterations/tickets of this first pass. In effort to get this in before sprint close and available for next incident, I figured we could land this and fast follow with spec files (also given it's internal-only admin panel). Does that sound reasonable or should we hold this to get spec files before merging?
| errno: error.ERRNO.REQUEST_BLOCKED, | ||
| }); | ||
|
|
||
| sinon.assert.calledWith(statsd.increment, 'account.create.blocked'); |
There was a problem hiding this comment.
The sinon style asserts went away with a PR from Vijay, can you update these to use just Jest style assertions and mocks? It'll probably have conflicts at this point too
There was a problem hiding this comment.
Similar here, there are no unit tests for this new page and we should probably have some
377fb61 to
d6e7d38
Compare
| <li>Blocked attempts are logged and counted in statsd.</li> | ||
| <li> | ||
| Changes propagate to auth-server within 5 minutes. Keep the list small | ||
| (hundreds of entries, not thousands) to avoid slowing registration. |
There was a problem hiding this comment.
It might be worth calling out in here too if we don't include a code fix to block it - but there's a risk of ReDoS with current setup. Obviously we have regex matching we do in other places, but those are config driven and require code review. Since this is an admin panel it is restricted but it's manual entry so there's a chance to accidentally enter a regex pattern with nested quantifiers and cause exponentially long matching when checking emails
There was a problem hiding this comment.
Yep, GitHub security flagged this above as well. The rationale is that this is admin-only and the risk was low.
Regardless, I'll add a new fast-follow ticket to restrict arbitrary regex power -- limiting only to the case of adding domain names.
|
|
||
| const CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes | ||
|
|
||
| let cachedEntries: { regex: string; createdAt: number }[] | null = null; |
There was a problem hiding this comment.
Since these are module level mutable objects, there's a chance of them colliding between near simultaneous requests could cause them to stampede eachother when updating these objects.
It's not super likely to effect real world, but it would be especially prevalent for unit tests and cause state leakage between tests. If we have a follow up to add tests then we should probably address this in that issue and include tests for it
| const normalizedEmail = normalizeEmail(email); | ||
| const blockedRegex = | ||
| await EmailBlocklist.findMatchingRegex(normalizedEmail); | ||
| if (blockedRegex !== null) { |
There was a problem hiding this comment.
Additional though, a fast kill-switch here would be nice. Auth-server config so we can quickly disable checking the block list if we find something is going sideways... Though, I do suppose we could also just delete entries but the toggle be prevent us from having to play wack-a-mole
d6e7d38 to
017d0c8
Compare
nshirley
left a comment
There was a problem hiding this comment.
r+wc, from slack conversation there will be some fast follows to add tests, address the ReDoS potential, and other flagged notes
I have tested locally and the logic works. I can add individual records, delete individual, bulk delete and import from file. Once a pattern is added I was blocked from sign up as expected too, and removing the block let me continue with sign up
Because
This pull request
emailBlocklistMySQL table (patch 188→189) storing regex patterns with a unique constraintEmailBlocklistshared model with methods for CRUD and registration-time matchingEmailBlocklistadmin panel feature with textarea bulk-add, CSV/TXT file upload, per-row delete and delete-allfxa-auth-serverwhen the email matches any blocklist pattern; logs the matched domain + regex and increments a statsd counter (account.create.blocked)Issue that this pull request solves
Closes: FXA-13431
Checklist
Put an
xin the boxes that applyHow to review (Optional)
email-blocklist.ts(model),email-blocklist.controller.ts(API),PageEmailBlocklist/index.tsx(UI),account.ts(enforcement)http://localhost:8091/email-blocklist, add a pattern like@evildoge\.example\.com$, then attempt to registertest@evildoge.example.com— should fail.Other information (Optional)
nx build fxa-shared && pm2 restart admin-servermay be required after pulling