From 123a83b5050fec61c32fd184ec64459e163a0073 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 27 Jul 2026 21:47:01 -0400 Subject: [PATCH 1/2] fix(auth): make account provisioning invitation-only POST /auth/register is public and unauthenticated, and when no branch.users row matched the email it inserted one. Anyone on the internet could therefore create a working account. That predates this branch, but claim-on-register made the invitation path work without closing the open one. It mattered more than `is_admin: false` suggests. Four list endpoints authorize on `isAuthenticated` alone with no project scoping -- GET /donors, /donations, /expenditures and /reports -- so a self-registered stranger could read the full donor list and financial history. Registration can no longer create a branch.users row, only claim one an admin already approved via the ADMIN-gated POST /users. An email with no pending invitation gets 403 INVITATION_REQUIRED, and the insert path is gone. The row is the real control rather than the pool configuration: authenticateRequest rejects any Cognito identity whose sub has no row, so an identity created out of band stays inert. The 403 is deliberately identical whether or not the address exists, so the endpoint cannot be used to enumerate staff emails. Also update the e2e tests, whose premises this invalidated: two inserted rows without a cognito_sub, which now reads as a pending invitation rather than a conflict, and one created an account from nothing. Read scoping on those four endpoints is deliberately left to a separate PR. Co-Authored-By: Claude Opus 5 --- apps/backend/AGENTS.md | 6 +- apps/backend/db/db_setup.sql | 3 + apps/backend/lambdas/auth/handler.ts | 62 ++++++++------ apps/backend/lambdas/auth/openapi.yaml | 27 ++++++- .../lambdas/auth/test/auth.e2e.test.ts | 80 ++++++++++++------- .../lambdas/auth/test/auth.login.unit.test.ts | 39 +++++++-- 6 files changed, 148 insertions(+), 69 deletions(-) diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index e4158926..d9be875a 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -74,7 +74,11 @@ Automatic on push to `main` touching `apps/backend/lambdas/**` or `shared/types/ **`branch.users.is_admin` is the single source of truth for admin.** There is no promotion from a Cognito group, and no pre-token-generation trigger, so `is_admin` is not a JWT claim — `GET /auth/me` is the only way a client can learn it. -**A `branch.users` row with `cognito_sub IS NULL` is a pending invitation**, created by the `db_setup.sql` seeds or by admin `POST /users`. `POST /auth/register` claims such a row (setting `cognito_sub`, never touching `is_admin`) instead of returning 409. Registration only 409s when the row is already claimed. +**Account provisioning is invitation-only.** A `branch.users` row with `cognito_sub IS NULL` is a pending invitation, created by the `db_setup.sql` seeds or by admin `POST /users` (ADMIN-gated). `POST /auth/register` is public, so it deliberately **cannot create a row** — it only claims an existing invitation, setting `cognito_sub` and never touching `is_admin`. An email with no pending invitation gets 403 `INVITATION_REQUIRED`; an already-claimed one gets 409. + +The 403 is intentionally identical whether or not the address exists, so registration cannot be used to enumerate staff emails. + +This is the real control, not the Cognito pool config: `authenticateRequest` rejects any Cognito identity whose `sub` has no `branch.users` row, so a Cognito user created out of band is inert. Full flow: admin `POST /users` → invitee `POST /auth/register` with that email → `POST /auth/verify-email` with the emailed code → `POST /auth/login`. **Bootstrapping the first admin** is a manual SQL statement in every environment, because `is_admin` can only be set by an existing admin: `make grant-admin EMAIL=…` locally, or the equivalent `UPDATE` against RDS in production. diff --git a/apps/backend/db/db_setup.sql b/apps/backend/db/db_setup.sql index 16163e8c..c6e9ecd4 100644 --- a/apps/backend/db/db_setup.sql +++ b/apps/backend/db/db_setup.sql @@ -78,6 +78,9 @@ CREATE TABLE reports ( -- preserves user_id and is_admin. The same mechanism backs admin-created users -- (POST /users), which also insert without a cognito_sub. -- +-- Registration is invitation-only: /auth/register cannot create a row, only +-- claim one, so an email with no row here (or created by an admin) gets 403. +-- -- To sign in as one of these locally you must control the mailbox to receive the -- Cognito verification code. Otherwise register your own email and run -- `make grant-admin EMAIL=you@example.com`. diff --git a/apps/backend/lambdas/auth/handler.ts b/apps/backend/lambdas/auth/handler.ts index 120b2425..3a325de5 100644 --- a/apps/backend/lambdas/auth/handler.ts +++ b/apps/backend/lambdas/auth/handler.ts @@ -666,7 +666,27 @@ async function handleRegister(event: any): Promise { return json(409, { message: 'User with this email already exists' }); } - const claimingUserId: number | null = existingUser ? existingUser.user_id : null; + // REGISTRATION IS INVITATION-ONLY. This endpoint is public and + // unauthenticated, so without this gate anyone could create a working + // account for themselves. An account is only meaningful once a branch.users + // row exists -- authenticateRequest rejects any Cognito identity whose sub + // has no row -- so refusing to create that row here is the control. + // + // The invitation must be created first by an admin via the ADMIN-gated + // POST /users, which inserts a row with a NULL cognito_sub. + // + // 403 rather than 404: this endpoint must not become an oracle for which + // email addresses have been invited, so the response is deliberately the + // same whether or not the address is known. + if (!existingUser) { + return json(403, { + message: + 'Registration is by invitation only. Ask an administrator to create your account.', + code: 'INVITATION_REQUIRED', + }); + } + + const claimingUserId: number = existingUser.user_id; // Prepare Cognito SignUp parameters const signUpParams: SignUpCommandInput = { @@ -700,7 +720,7 @@ async function handleRegister(event: any): Promise { // SignUp can never hand us a sub. Happens routinely in local dev: `make // down-v` wipes Postgres while the shared dev pool keeps the user. Link // the existing Cognito identity instead of dead-ending on a 409. - if (claimingUserId !== null) { + { try { // AdminGetUser is SigV4-signed and needs cognito-idp:AdminGetUser // (granted in infrastructure/aws/lambda.tf). With no AWS credentials @@ -746,29 +766,19 @@ async function handleRegister(event: any): Promise { // Create user in database, or claim the pending invitation try { - if (claimingUserId !== null) { - // is_admin is deliberately NOT touched: it was set by whoever created the - // invitation (a seed, or an admin via POST /users) and must not be - // settable from a public, unauthenticated endpoint. The cognito_sub IS - // NULL predicate makes a concurrent claim a no-op rather than an - // overwrite; UNIQUE(cognito_sub) is the backstop. - await db - .updateTable('branch.users') - .set({ cognito_sub: cognitoUserSub, name: name.trim() }) - .where('user_id', '=', claimingUserId) - .where('cognito_sub', 'is', null) - .execute(); - } else { - await db - .insertInto('branch.users') - .values({ - cognito_sub: cognitoUserSub, - email: email.toLowerCase(), - name: name.trim(), - is_admin: false, - }) - .execute(); - } + // Claim the invitation. is_admin is deliberately NOT touched: it was set + // by whoever created the invitation (a seed, or an admin via POST /users) + // and must never be settable from a public, unauthenticated endpoint. + // There is no insert path here -- registration cannot mint a new row, only + // claim one an admin already approved. The cognito_sub IS NULL predicate + // makes a concurrent claim a no-op rather than an overwrite; + // UNIQUE(cognito_sub) is the backstop. + await db + .updateTable('branch.users') + .set({ cognito_sub: cognitoUserSub, name: name.trim() }) + .where('user_id', '=', claimingUserId) + .where('cognito_sub', 'is', null) + .execute(); } catch (dbError: any) { console.error('Database insert error:', dbError); @@ -795,7 +805,7 @@ async function handleRegister(event: any): Promise { name: name.trim(), emailVerificationRequired: true, details: 'Please check your email for verification code', - ...(claimingUserId !== null ? { claimed: true } : {}), + claimed: true, }); } catch (error: any) { console.error('Registration error:', error); diff --git a/apps/backend/lambdas/auth/openapi.yaml b/apps/backend/lambdas/auth/openapi.yaml index f95251a0..87cd84b1 100644 --- a/apps/backend/lambdas/auth/openapi.yaml +++ b/apps/backend/lambdas/auth/openapi.yaml @@ -21,8 +21,13 @@ paths: /register: post: - summary: Register a new user - description: Creates a new user account in Cognito and the database + summary: Claim an invitation and activate an account + description: > + INVITATION-ONLY. This endpoint is public and unauthenticated, so it + cannot mint new accounts: it only activates a branch.users row that an + admin already created via the ADMIN-gated POST /users (such a row has a + NULL cognito_sub). An email with no pending invitation gets 403. + is_admin is never written here. requestBody: required: true content: @@ -87,8 +92,24 @@ paths: message: type: string example: Invalid email format + '403': + description: > + No pending invitation for this email. Deliberately indistinguishable + from the response for an address that is not in the system at all, + so this endpoint cannot be used to enumerate staff emails. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: Registration is by invitation only. Ask an administrator to create your account. + code: + type: string + example: INVITATION_REQUIRED '409': - description: User already exists + description: The invitation has already been claimed content: application/json: schema: diff --git a/apps/backend/lambdas/auth/test/auth.e2e.test.ts b/apps/backend/lambdas/auth/test/auth.e2e.test.ts index d2d6e391..b897a83d 100644 --- a/apps/backend/lambdas/auth/test/auth.e2e.test.ts +++ b/apps/backend/lambdas/auth/test/auth.e2e.test.ts @@ -16,7 +16,7 @@ const seedSql = fs.readFileSync(seedSqlPath, 'utf8'); beforeEach(async () => { const testName = expect.getState().currentTestName; - if (testName && (testName.includes('duplicate') || testName.includes('normalization') || testName.includes('register with valid'))) { + if (testName && (testName.includes('duplicate') || testName.includes('normalization') || testName.includes('uninvited') || testName.includes('seeded admins'))) { const client = await pool.connect(); try { await client.query(seedSql); @@ -31,12 +31,13 @@ afterAll(async () => { }); test("duplicate email returns 409", async () => { - // Insert a user with lowercase email directly in DB + // A CLAIMED row (cognito_sub set) is a genuine conflict. A row without one is + // a pending invitation and would be claimed instead -- see the test below. const client = await pool.connect(); try { await client.query( - 'INSERT INTO branch.users (email, name, is_admin) VALUES ($1, $2, $3)', - ['existing@example.com', 'Existing User', false] + 'INSERT INTO branch.users (email, name, is_admin, cognito_sub) VALUES ($1, $2, $3, $4)', + ['existing@example.com', 'Existing User', false, 'existing-sub-123'] ); } finally { client.release(); @@ -62,8 +63,8 @@ test("email normalization uppercase matches lowercase in DB", async () => { const client = await pool.connect(); try { await client.query( - 'INSERT INTO branch.users (email, name, is_admin) VALUES ($1, $2, $3)', - ['lowercase@example.com', 'Existing User', false] + 'INSERT INTO branch.users (email, name, is_admin, cognito_sub) VALUES ($1, $2, $3, $4)', + ['lowercase@example.com', 'Existing User', false, 'lowercase-sub-123'] ); } finally { client.release(); @@ -84,9 +85,12 @@ test("email normalization uppercase matches lowercase in DB", async () => { expect(body.message).toContain("already exists"); }); -test("register with valid data (requires Cognito)", async () => { +test("uninvited email is refused before Cognito is ever called", async () => { + // Registration is invitation-only: with no branch.users row for this address + // the request must be rejected outright. Previously this created a working + // account for any caller. const uniqueEmail = `test${Date.now()}${Math.random().toString(36).substring(7)}@example.com`; - + const res = await fetch("http://localhost:3000/auth/register", { method: "POST", body: JSON.stringify({ @@ -96,30 +100,44 @@ test("register with valid data (requires Cognito)", async () => { }) }); - if (res.status === 201) { - const body = await res.json(); - expect(body.message).toBe("User registered successfully"); - expect(body.userId).toBeDefined(); - expect(body.email).toBe(uniqueEmail); - expect(body.name).toBe("Test User"); - expect(body.emailVerificationRequired).toBe(true); + expect(res.status).toBe(403); + const body = await res.json(); + expect(body.code).toBe("INVITATION_REQUIRED"); - const client = await pool.connect(); - try { - const result = await client.query( - 'SELECT * FROM branch.users WHERE email = $1', - [uniqueEmail] - ); - expect(result.rows.length).toBe(1); - expect(result.rows[0].cognito_sub).toBe(body.userId); - expect(result.rows[0].is_admin).toBe(false); - } finally { - client.release(); + // And nothing was written. + const client = await pool.connect(); + try { + const { rows } = await client.query( + 'SELECT 1 FROM branch.users WHERE email = $1', + [uniqueEmail] + ); + expect(rows).toHaveLength(0); + } finally { + client.release(); + } +}); + +test("seeded admins are pending invitations, not claimed accounts", async () => { + // The claim path depends on this contract: a seeded admin has a row but no + // Cognito identity, so /auth/register activates it instead of 409ing. Before + // claim-on-register these three could never sign in at all. + const client = await pool.connect(); + try { + const { rows } = await client.query( + 'SELECT email, user_id, is_admin, cognito_sub FROM branch.users ORDER BY user_id' + ); + + expect(rows).toHaveLength(3); + for (const row of rows) { + expect(row.cognito_sub).toBeNull(); + expect(row.is_admin).toBe(true); } - } else if (res.status === 500) { - console.log('Skipping Cognito test - Cognito not configured'); - expect(res.status).toBe(500); - } else { - throw new Error(`Unexpected status code: ${res.status}`); + expect(rows.map((r: { email: string }) => r.email)).toEqual([ + 'ashley@branch.org', + 'renee@branch.org', + 'nour@branch.org', + ]); + } finally { + client.release(); } }); diff --git a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts index 9392b91a..357f1075 100644 --- a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts +++ b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts @@ -474,6 +474,14 @@ describe('GET /me', () => { describe('POST /register — claim-on-register', () => { const validBody = { email: 'Ashley@branch.org', password: 'Password123', name: 'Ashley' }; + /** An admin-created invitation: the row exists, but no Cognito identity yet. */ + const invitation = { + user_id: 1, + email: 'ashley@branch.org', + cognito_sub: null, + is_admin: true, + }; + it('claims a pending invitation (cognito_sub IS NULL) instead of returning 409', async () => { mockExecuteTakeFirst.mockResolvedValue({ user_id: 1, @@ -521,22 +529,37 @@ describe('POST /register — claim-on-register', () => { expect(mockSend).not.toHaveBeenCalled(); }); - it('inserts a new row when no invitation exists', async () => { + it('refuses to create an account for an uninvited email', async () => { + // Registration is invitation-only. This endpoint is public, so without the + // gate anyone on the internet could mint themselves a working account -- + // and several list endpoints authorize on `isAuthenticated` alone. mockExecuteTakeFirst.mockResolvedValue(undefined); - mockSend.mockResolvedValue({ UserSub: 'new-sub' }); - mockExecute.mockResolvedValue(undefined); const res = await handler(event('/register', 'POST', validBody)); - expect(res.statusCode).toBe(201); - expect(JSON.parse(res.body).claimed).toBeUndefined(); - expect(mockValues).toHaveBeenCalledWith( - expect.objectContaining({ cognito_sub: 'new-sub', is_admin: false }), + expect(res.statusCode).toBe(403); + expect(JSON.parse(res.body).code).toBe('INVITATION_REQUIRED'); + // No Cognito user and no DB row: nothing is created at all. + expect(mockSend).not.toHaveBeenCalled(); + expect(mockValues).not.toHaveBeenCalled(); + expect(mockExecute).not.toHaveBeenCalled(); + }); + + it('does not reveal whether an uninvited email is known', async () => { + // Same response shape for an unknown address as for a known-but-uninvited + // one, so /register cannot be used to enumerate staff email addresses. + mockExecuteTakeFirst.mockResolvedValue(undefined); + + const unknown = await handler( + event('/register', 'POST', { ...validBody, email: 'stranger@example.com' }), ); + + expect(unknown.statusCode).toBe(403); + expect(JSON.parse(unknown.body).message).not.toMatch(/not found|no such|unknown/i); }); it('rolls back the Cognito user when the database write fails', async () => { - mockExecuteTakeFirst.mockResolvedValue(undefined); + mockExecuteTakeFirst.mockResolvedValue(invitation); mockSend .mockResolvedValueOnce({ UserSub: 'new-sub' }) // SignUp .mockResolvedValueOnce({}); // AdminDeleteUser From 5333df9659ff86eb56b2573b7259cf7a1e41b70c Mon Sep 17 00:00:00 2001 From: Nour Shoreibah <168875317+nourshoreibah@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:40:34 -0400 Subject: [PATCH 2/2] Update db_setup.sql --- apps/backend/db/db_setup.sql | 3 --- 1 file changed, 3 deletions(-) diff --git a/apps/backend/db/db_setup.sql b/apps/backend/db/db_setup.sql index c6e9ecd4..16163e8c 100644 --- a/apps/backend/db/db_setup.sql +++ b/apps/backend/db/db_setup.sql @@ -78,9 +78,6 @@ CREATE TABLE reports ( -- preserves user_id and is_admin. The same mechanism backs admin-created users -- (POST /users), which also insert without a cognito_sub. -- --- Registration is invitation-only: /auth/register cannot create a row, only --- claim one, so an email with no row here (or created by an admin) gets 403. --- -- To sign in as one of these locally you must control the mailbox to receive the -- Cognito verification code. Otherwise register your own email and run -- `make grant-admin EMAIL=you@example.com`.