Skip to content

[SES-PHASE-1-6] - PLU-744: add admin whitelist endpoint #1632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m0nggh wants to merge 1 commit into
base: feat/ses/trunk
Choose a base branch
from
+60 −0
Open

[SES-PHASE-1-6] - PLU-744: add admin whitelist endpoint #1632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions packages/backend/src/routes/api/admin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { Router } from 'express'

import logger from '@/helpers/logger'
import EmailSuppressionEntry from '@/models/email-suppression-entry'

const router = Router()

/**
* Middleware to ensure only plumber admins can access admin routes.
*/
router.use((req, res, next) => {
if (!req.context?.isAdminOperation) {
res.status(403).json({ error: 'Admin access required' })
return
}
next()
})

/**
* GET /api/admin/email-suppression/whitelist?emails=a@x.com,b+1@x.com
*
* Whitelists one or more emails from the suppression list.
* Requires x-plumber-admin-token header.
*/
router.get('/email-suppression/whitelist', async (req, res) => {
Copy link
Copy Markdown
Contributor

@ogp-weeloong ogp-weeloong Jun 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@ogp-weeloong ogp-weeloong Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Copy Markdown
Contributor

@ogp-weeloong ogp-weeloong Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: zod this?

.split(',')
.map((e) => e.trim().replace(/ /g, '+'))
Comment on lines +26 to +37
Copy link
Copy Markdown

@graphite-app graphite-app Bot May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Suggested change
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

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

.filter(Boolean)
if (emails.length === 0) {
res.status(400).json({ error: 'emails must not be empty' })
return
}

const whitelisted = await EmailSuppressionEntry.whitelistEmails(emails)

logger.info('Admin whitelisted emails from suppression list', {
event: 'admin-email-suppression-whitelist',
requested: emails,
whitelisted,
})

res.json({
whitelisted,
count: whitelisted.length,
})
})

export default router
2 changes: 2 additions & 0 deletions packages/backend/src/routes/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
requireAuthentication,
setCurrentUserContext,
} from './middleware/authentication'
import adminRouter from './admin'
import appsRouter from './apps'
import chatRouter from './chat'

Expand All @@ -17,6 +18,7 @@ router.use(requireAuthentication)

// Mount routes that admins can access before blockAdminOperations
router.use('/apps', appsRouter)
router.use('/admin', adminRouter)
Copy link
Copy Markdown
Contributor

@ogp-weeloong ogp-weeloong Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be extra safe, we should also add guardrails for this route to our WAF


// Block admin mutations for all subsequent routes
router.use(blockAdminOperations)
Expand Down
Loading