feat: move to whitelist database table from growthbook #864
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Datadog ReportBranch report: ✅ 0 Failed, 175 Passed, 34 Skipped, 36.75s Total Time |
dcshzj
force-pushed
the
feat/whitelist-db-table
branch
from
0593cc1 to
4ee237c
Compare
| // Step 1: Check if the exact email address is whitelisted | ||
| const exactMatch = await db | ||
| .selectFrom("Whitelist") | ||
| .where("email", "=", lowercaseEmail) |
There was a problem hiding this comment.
is this userinput sanitised?
There was a problem hiding this comment.
Yes it is sanitised via the zod schema of the caller procedure, but sure added an additional guard here in 805eebf.
| return true | ||
| } | ||
|
|
||
| // Step 3: Check if the suffix of the email domain is whitelisted |
There was a problem hiding this comment.
what is the use case for suffix match?
There was a problem hiding this comment.
This is to whitelist .gov.sg, so we can whitelist all domains that end with a .gov.sg in the domain name.
There was a problem hiding this comment.
Bypassed with this POC:
var validator = require('validator');
const email = '"[email protected]@"@example.org'
const lowercaseEmail = email.toLowerCase()
console.log(validator.isEmail(email))
const emailParts = lowercaseEmail.split("@")
console.log(emailParts)
console.log(emailParts.length)
if (emailParts.length !== 2 && !emailParts[1]) {
console.log('fail')
} else {
const emailDomain = `@${emailParts[1]}`
console.log(emailDomain)
}
node index.js
true
[ '"john..d', 'open.gov.sg', '"', 'example.org' ]
4
@open.gov.sg
This is because RFC-compliant emails that pass validators allow a lot of sus stuff.
This is a common issue and we wrote a specific validator in starter-kitty for this, but I'd like to stress-test this a bit more: https://github.com/opengovsg/starter-kitty/tree/develop/packages/validators/src/email.
There was a problem hiding this comment.
Let's get a quick review from @spaceraccoon since this touches parts of auth
There was a problem hiding this comment.
lgtm, pending approval from @spaceraccoon on security side of things
| return true | ||
| } | ||
|
|
||
| // Step 3: Check if the suffix of the email domain is whitelisted |
There was a problem hiding this comment.
Bypassed with this POC:
var validator = require('validator');
const email = '"[email protected]@"@example.org'
const lowercaseEmail = email.toLowerCase()
console.log(validator.isEmail(email))
const emailParts = lowercaseEmail.split("@")
console.log(emailParts)
console.log(emailParts.length)
if (emailParts.length !== 2 && !emailParts[1]) {
console.log('fail')
} else {
const emailDomain = `@${emailParts[1]}`
console.log(emailDomain)
}
node index.js
true
[ '"john..d', 'open.gov.sg', '"', 'example.org' ]
4
@open.gov.sg
This is because RFC-compliant emails that pass validators allow a lot of sus stuff.
This is a common issue and we wrote a specific validator in starter-kitty for this, but I'd like to stress-test this a bit more: https://github.com/opengovsg/starter-kitty/tree/develop/packages/validators/src/email.
|
|
||
| // Step 2: Check if the exact email domain is whitelisted | ||
| const emailParts = lowercaseEmail.split("@") | ||
| if (emailParts.length !== 2 && !emailParts[1]) { |
There was a problem hiding this comment.
| if (emailParts.length !== 2 && !emailParts[1]) { | |
| if (emailParts.length !== 2 || !emailParts[1]) { |
This is the mistake that allowed my bypass, but overall is a bit sus
There was a problem hiding this comment.
Adjusted in b102054! I think doing a .pop() will enforce that we always take the last part, and it's something I see FormSG doing too: https://github.com/opengovsg/FormSG/blob/49e88ce138bdcb36b7ff15ce10332ea60ba5f0d3/src/app/modules/auth/auth.service.ts#L65
Problem
Our email whitelist lives in Growthbook, which makes adding new users a little difficult as it involves both updating the database and updating the Growthbook feature flag.
Fixes ISOM-1668.
Solution
Breaking Changes
Features: