Skip to content
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.

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

feat: move to whitelist database table from growthbook #864

Merged
merged 4 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
8 changes: 8 additions & 0 deletions apps/studio/prisma/generated/generatedTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ export interface Version {
publishedBy: string
updatedAt: Generated<Timestamp>
}
export interface Whitelist {
id: GeneratedAlways<number>
email: string
expiry: Timestamp | null
createdAt: Generated<Timestamp>
updatedAt: Generated<Timestamp>
}
export interface DB {
Blob: Blob
Footer: Footer
Expand All @@ -126,4 +133,5 @@ export interface DB {
User: User
VerificationToken: VerificationToken
Version: Version
Whitelist: Whitelist
}
1 change: 1 addition & 0 deletions apps/studio/prisma/generated/selectableTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ export type SiteMember = Selectable<T.SiteMember>
export type User = Selectable<T.User>
export type VerificationToken = Selectable<T.VerificationToken>
export type Version = Selectable<T.Version>
export type Whitelist = Selectable<T.Whitelist>
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- CreateTable
CREATE TABLE "Whitelist" (
"id" SERIAL NOT NULL,
"email" TEXT NOT NULL,
"expiry" TIMESTAMP(3),
"createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,

CONSTRAINT "Whitelist_pkey" PRIMARY KEY ("id")
);

-- CreateIndex
CREATE UNIQUE INDEX "Whitelist_email_key" ON "Whitelist"("email");

-- CreateIndex
CREATE INDEX "Whitelist_email_idx" ON "Whitelist"("email");

-- AlterTable
CREATE TRIGGER update_timestamp BEFORE UPDATE ON "Whitelist" FOR EACH ROW EXECUTE PROCEDURE moddatetime("updatedAt");
10 changes: 10 additions & 0 deletions apps/studio/prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -212,3 +212,13 @@ model RateLimiterFlexible {
points Int
expire DateTime?
}

model Whitelist {
id Int @id @default(autoincrement())
email String @unique
expiry DateTime?
createdAt DateTime @default(now())
updatedAt DateTime @default(now()) @updatedAt

@@index([email])
}
12 changes: 12 additions & 0 deletions apps/studio/prisma/seed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,18 @@ async function main() {
)
.executeTakeFirstOrThrow()

await db
.insertInto("Whitelist")
.values({
email: "@open.gov.sg",
})
.onConflict((oc) =>
oc
.column("email")
.doUpdateSet((eb) => ({ email: eb.ref("excluded.email") })),
)
.executeTakeFirstOrThrow()

await Promise.all(
[...ISOMER_ADMINS, ...ISOMER_MIGRATORS, EDITOR_USER, PUBLISHER_USER].map(
async (name) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
applySession,
createMockRequest,
} from "tests/integration/helpers/iron-session"
import { setUpWhitelist } from "tests/integration/helpers/seed"
import { describe, expect, it } from "vitest"

import { env } from "~/env.mjs"
Expand All @@ -15,16 +16,17 @@ import { getIpFingerprint, LOCALHOST } from "../utils"
describe("auth.email", () => {
let caller: Awaited<ReturnType<typeof emailSessionRouter.createCaller>>
let session: ReturnType<typeof applySession>
const TEST_VALID_EMAIL = "[email protected]"

beforeEach(async () => {
await resetTables("User", "VerificationToken")
await resetTables("User", "VerificationToken", "Whitelist")
await setUpWhitelist({ email: TEST_VALID_EMAIL })
session = applySession()
const ctx = createMockRequest(session)
caller = emailSessionRouter.createCaller(ctx)
})

describe("login", () => {
const TEST_VALID_EMAIL = "[email protected]"
it("should throw if email is not provided", async () => {
// Act
const result = caller.login({ email: "" })
Expand Down Expand Up @@ -72,7 +74,6 @@ describe("auth.email", () => {
})

describe("verifyOtp", () => {
const TEST_VALID_EMAIL = "[email protected]"
const VALID_OTP = "123456"
const VALID_TOKEN_HASH = createTokenHash(VALID_OTP, TEST_VALID_EMAIL)
const INVALID_OTP = "987643"
Expand Down
19 changes: 7 additions & 12 deletions apps/studio/src/server/modules/auth/email/email.router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import { publicProcedure, router } from "~/server/trpc"
import { getBaseUrl } from "~/utils/getBaseUrl"
import { defaultMeSelect } from "../../me/me.select"
import { isEmailWhitelisted } from "../../whitelist/whitelist.service"
import { VerificationError } from "../auth.error"
import { verifyToken } from "../auth.service"
import { createTokenHash, createVfnPrefix, createVfnToken } from "../auth.util"
Expand All @@ -21,19 +22,13 @@ export const emailSessionRouter = router({
.input(emailSignInSchema)
.meta({ rateLimitOptions: {} })
.mutation(async ({ ctx, input: { email } }) => {
if (env.NODE_ENV === "production") {
// check if whitelisted email on Growthbook
const defaultWhitelist: string[] = []
const whitelistedUsers = ctx.gb.getFeatureValue("whitelisted_users", {
whitelist: defaultWhitelist,
})
const isWhitelisted = await isEmailWhitelisted(email)

if (!whitelistedUsers.whitelist.includes(email)) {
throw new TRPCError({
code: "UNAUTHORIZED",
message: "Unauthorized. Contact Isomer support.",
})
}
if (!isWhitelisted) {
throw new TRPCError({
code: "UNAUTHORIZED",
message: "Unauthorized. Contact Isomer support.",
})
}

// TODO: instead of storing expires, store issuedAt to calculate when the next otp can be re-issued
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import { resetTables } from "tests/integration/helpers/db"
import { setUpWhitelist } from "tests/integration/helpers/seed"

import { isEmailWhitelisted } from "../whitelist.service"

describe("whitelist.service", () => {
beforeAll(async () => {
const oneYearFromNow = new Date()
oneYearFromNow.setFullYear(oneYearFromNow.getFullYear() + 1)
const oneYearAgo = new Date()
oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1)

await resetTables("Whitelist")
await setUpWhitelist({ email: "[email protected]" })
await setUpWhitelist({
email: "[email protected]",
expiry: oneYearFromNow,
})
await setUpWhitelist({
email: "[email protected]",
expiry: oneYearAgo,
})
await setUpWhitelist({ email: ".gov.sg" })
await setUpWhitelist({
email: "@vendor.com.sg",
})
await setUpWhitelist({
email: "@whitelisted.com.sg",
expiry: oneYearFromNow,
})
await setUpWhitelist({ email: "@expired.sg", expiry: oneYearAgo })
await setUpWhitelist({
email: "[email protected]",
expiry: oneYearAgo,
})
})

it("should show email as whitelisted if the exact email address is whitelisted and expiry is NULL", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})

it("should show email as whitelisted if the exact email address is whitelisted and expiry is in the future", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})

it("should show email as not whitelisted if the exact email address is whitelisted and expiry is in the past", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(false)
})

it("should show email as whitelisted if the exact email domain is whitelisted and expiry is NULL", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})

it("should show email as whitelisted if the exact email domain is whitelisted and expiry is in the future", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})

it("should show email as not whitelisted if the exact email domain is whitelisted and expiry is in the past", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(false)
})

it("should show email as whitelisted if the suffix of the email domain is whitelisted and expiry is NULL", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})

it("should show email as whitelisted if the exact email address is expired, but the domain's expiry is in the future", async () => {
// Arrange
const email = "[email protected]"

// Act
const result = await isEmailWhitelisted(email)

// Assert
expect(result).toBe(true)
})
})
66 changes: 66 additions & 0 deletions apps/studio/src/server/modules/whitelist/whitelist.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { TRPCError } from "@trpc/server"

import { db } from "../database"

export const isEmailWhitelisted = async (email: string) => {
const lowercaseEmail = email.toLowerCase()

// Step 1: Check if the exact email address is whitelisted
const exactMatch = await db
.selectFrom("Whitelist")
.where("email", "=", lowercaseEmail)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this userinput sanitised?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is sanitised via the zod schema of the caller procedure, but sure added an additional guard here in 805eebf.

.where(({ eb }) =>
eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]),
)
.select(["id"])
.executeTakeFirst()

if (exactMatch) {
return true
}

// Step 2: Check if the exact email domain is whitelisted
const emailParts = lowercaseEmail.split("@")
if (emailParts.length !== 2 && !emailParts[1]) {

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

throw new TRPCError({
code: "BAD_REQUEST",
message: "An invalid email was provided",
})
}

const emailDomain = `@${emailParts[1]}`
const domainMatch = await db
.selectFrom("Whitelist")
.where("email", "=", emailDomain)
.where(({ eb }) =>
eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]),
)
.select(["id"])
.executeTakeFirst()

if (domainMatch) {
return true
}

// Step 3: Check if the suffix of the email domain is whitelisted
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the use case for suffix match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to whitelist .gov.sg, so we can whitelist all domains that end with a .gov.sg in the domain name.

Copy link

@spaceraccoon spaceraccoon Nov 8, 2024

Choose a reason for hiding this comment

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

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.

const domainParts = emailDomain.split(".")
for (let i = 1; i < domainParts.length; i++) {
// Suffices should start with a dot (e.g. ".gov.sg")
const suffix = `.${domainParts.slice(i).join(".")}`

const suffixMatch = await db
.selectFrom("Whitelist")
.where("email", "=", suffix)
.where(({ eb }) =>
eb.or([eb("expiry", "is", null), eb("expiry", ">", new Date())]),
)
.select(["id"])
.executeTakeFirst()

if (suffixMatch) {
return true
}
}

return false
}
17 changes: 5 additions & 12 deletions apps/studio/src/server/trpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import getIP from "~/utils/getClientIp"
import { type Context } from "./context"
import { defaultMeSelect } from "./modules/me/me.select"
import { checkRateLimit } from "./modules/rate-limit/rate-limit.service"
import { isEmailWhitelisted } from "./modules/whitelist/whitelist.service"
import { prisma } from "./prisma"

interface Meta {
Expand Down Expand Up @@ -124,13 +125,6 @@ const baseMiddleware = t.middleware(async ({ ctx, next }) => {
})

const authMiddleware = t.middleware(async ({ next, ctx }) => {
const defaultWhitelist: string[] = []
const whitelistedUsers = ctx.gb
.getFeatureValue("whitelisted_users", {
whitelist: defaultWhitelist,
})
.whitelist.map((email) => email.toLowerCase())

if (!ctx.session?.userId) {
throw new TRPCError({ code: "UNAUTHORIZED" })
}
Expand All @@ -145,11 +139,10 @@ const authMiddleware = t.middleware(async ({ next, ctx }) => {
throw new TRPCError({ code: "UNAUTHORIZED" })
}

// check against Growthbook if user is whitelisted for prod/stg
if (env.NODE_ENV === "production") {
if (!whitelistedUsers.includes(user.email.toLowerCase())) {
throw new TRPCError({ code: "UNAUTHORIZED" })
}
// Ensure that the user is whitelisted to use the app
const isWhitelisted = await isEmailWhitelisted(user.email)
if (!isWhitelisted) {
throw new TRPCError({ code: "UNAUTHORIZED" })
}

return next({
Expand Down
3 changes: 3 additions & 0 deletions apps/studio/tests/integration/helpers/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ import cuid2 from "@paralleldrive/cuid2"
import { db } from "~server/db"

import type { User } from "~server/db"
import { setUpWhitelist } from "./seed"

export const auth = async ({ id, ...user }: SetOptional<User, "id">) => {
await setUpWhitelist({ email: user.email })

if (id !== undefined) {
return db
.updateTable("User")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
const mockFeatureFlags = new Map<string, unknown>()
mockFeatureFlags.set("whitelisted_users", {
whitelist: ["[email protected]"],
})

export { mockFeatureFlags }
Loading
Loading