[SES-PHASE-1-6] - PLU-744: add admin whitelist endpoint#1632
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
9c04394 to
d5647a6
Compare
6a8790b to
38bd775
Compare
d5647a6 to
19d6d37
Compare
This comment has been minimized.
This comment has been minimized.
38bd775 to
0e74293
Compare
19d6d37 to
e533f62
Compare
| const emailsParam = req.query.emails as string | ||
| if (!emailsParam) { | ||
| res.status(400).json({ error: 'emails query parameter is required' }) | ||
| return | ||
| } | ||
|
|
||
| // `+` in query strings is decoded to a space; restore it since spaces | ||
| // are not valid in email addresses. This lets callers paste URLs with | ||
| // unencoded `+` characters directly. | ||
| const emails = emailsParam | ||
| .split(',') | ||
| .map((e) => e.trim().replace(/ /g, '+')) |
There was a problem hiding this comment.
Type assertion bug: req.query.emails can be a string array if the parameter is repeated in the URL (e.g., ?emails=a@x.com&emails=b@x.com). The code assumes it's always a string and calls .split(',') on it, which will throw TypeError: split is not a function if an array is passed.
const emailsParam = req.query.emails
if (!emailsParam || Array.isArray(emailsParam)) {
res.status(400).json({ error: 'emails query parameter is required and must be a single value' })
return
}
const emails = (emailsParam as string)
.split(',')
.map((e) => e.trim().replace(/ /g, '+'))
.filter(Boolean)| const emailsParam = req.query.emails as string | |
| if (!emailsParam) { | |
| res.status(400).json({ error: 'emails query parameter is required' }) | |
| return | |
| } | |
| // `+` in query strings is decoded to a space; restore it since spaces | |
| // are not valid in email addresses. This lets callers paste URLs with | |
| // unencoded `+` characters directly. | |
| const emails = emailsParam | |
| .split(',') | |
| .map((e) => e.trim().replace(/ /g, '+')) | |
| const emailsParam = req.query.emails | |
| if (!emailsParam || Array.isArray(emailsParam)) { | |
| res.status(400).json({ error: 'emails query parameter is required and must be a single value' }) | |
| return | |
| } | |
| const emails = (emailsParam as string) | |
| .split(',') | |
| .map((e) => e.trim().replace(/ /g, '+')) | |
| .filter(Boolean) | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
e533f62 to
badc3fe
Compare
0e74293 to
143cf2f
Compare
| // `+` in query strings is decoded to a space; restore it since spaces | ||
| // are not valid in email addresses. This lets callers paste URLs with | ||
| // unencoded `+` characters directly. | ||
| const emails = emailsParam |
|
|
||
| // Mount routes that admins can access before blockAdminOperations | ||
| router.use('/apps', appsRouter) | ||
| router.use('/admin', adminRouter) |
There was a problem hiding this comment.
to be extra safe, we should also add guardrails for this route to our WAF
| * Whitelists one or more emails from the suppression list. | ||
| * Requires x-plumber-admin-token header. | ||
| */ | ||
| router.get('/email-suppression/whitelist', async (req, res) => { |
There was a problem hiding this comment.
eh, why use a GET here?
For writes, it's more idiomatic to use POST / PUT / PATCH / DELETE. Primarily we're concerned about accidentlly exposing ourselves to CSRF attacks.
The more important part is actually for POST requests, browsers put up the form resubmission warning.
For this case, POST is fine, but tbh we don't need to enforce resubmission warning; PUT or PATCH also OK. DELETE feels weird
There was a problem hiding this comment.
Ah ok as discussed - we're safe behind the plumber admin header but still kinda dangerous to propagate this in the code. would like to avoid if possible unless we have a really really strong use case.
badc3fe to
a291c21
Compare
1b169a3 to
efe4153
Compare
983424e to
68d56fe
Compare
efe4153 to
5b575a1
Compare
68d56fe to
08ba585
Compare
1a2f127 to
87de761
Compare
08ba585 to
067c7be
Compare
87de761 to
80cc550
Compare
067c7be to
8dbd477
Compare

TL;DR
Adds an admin-only API endpoint to remove emails from the suppression list.
What changed?
A new
/api/admin/email-suppression/whitelistGET endpoint has been added, protected by anisAdminOperationmiddleware check that returns a 403 if the request is not from a Plumber admin. The endpoint accepts a comma-separatedemailsquery parameter, handles+characters that get decoded as spaces in query strings, and callsEmailSuppressionEntry.whitelistEmailsto remove the specified emails from the suppression list. The operation is logged with the requested and successfully whitelisted emails.How to test?
Note that this is to be done on Plumber admin: will release a PR on plumber admin to test on staging first
Refer to PR for the change
whitelistedarray andcount.+characters are handled correctly (e.g.,b+1@example.comshould not be mangled).403is returned (make the http call on Plumber app)emailsparameter or pass an empty value and confirm a400is returned.Why make this change?
Emails can end up on the suppression list after a bounce or complaint, preventing legitimate users from receiving emails. This endpoint gives Plumber admins a way to manually remove specific emails from the suppression list without requiring direct database access.