fix: address audit findings across lambdas, frontend and infra

.gitignore

@@ -4,6 +4,7 @@
 dist
 tmp
 /out-tsc
+lambda.zip
 
 # dependencies
 node_modules

AGENTS.md

@@ -30,7 +30,7 @@ BRANCH is a non-profit accounting platform (projects, donors, donations, expendi
 
 Two `file:`-linked packages dedupe code across lambdas:
 
-- **`@branch/types`** (`shared/types/`) — types only, no runtime. Exports DB row types (`DB`, `BranchUsers`, ...) + auth DTOs (`AuthContext`, `AuthenticatedUser`, `AccessLevel`, `AuthorizationCheck`). `db-types.d.ts` is **generated** from `apps/backend/db/migrations/**` by the `Schema Change Checks` workflow (or locally by `make types`) — never hand-edit it.
+- **`@branch/types`** (`shared/types/`) — types only, no runtime. Exports DB row types (`DB`, `BranchUsers`, ...) + auth DTOs (`AuthContext`, `AuthenticatedUser`, `AccessLevel`, `AuthorizationCheck`). It is the **single declaration** of those DTOs: `@branch/lambda-auth` depends on this package and re-exports them, so never add a second copy anywhere. `db-types.d.ts` is **generated** from `apps/backend/db/migrations/**` by the `Schema Change Checks` workflow (or locally by `make types`) — never hand-edit it.
 - **`@branch/lambda-auth`** (`shared/lambda-auth/`) — runtime auth: `authenticateRequest(db, event)`, `extractToken(event)`, `checkAuthorization(ctx, level, resourceUserId?)`. Lambdas wrap it in their local `auth.ts`.
 
 ## Root commands

apps/backend/AGENTS.md

@@ -50,8 +50,8 @@ http://localhost:3000/<service>/health
 ## Shared packages
 
 Both linked via `file:` deps in each lambda's `package.json`:
-- `@branch/types` (`../../../../shared/types`) — devDependency, types only.
-- `@branch/lambda-auth` (`../../../../shared/lambda-auth`) — dependency, runtime auth. Build it (`npm run build` in `shared/lambda-auth`) when its source changes; lambdas consume `dist/`.
+- `@branch/types` (`../../../../shared/types`) — devDependency, types only. Must stay a dependency-free leaf: `@branch/lambda-auth` depends on it.
+- `@branch/lambda-auth` (`../../../../shared/lambda-auth`) — dependency, runtime auth. Build it (`npm run build` in `shared/lambda-auth`) when its source changes; lambdas consume `dist/`. It depends on `@branch/types` and re-exports the auth DTOs from there, so those types have exactly one declaration; changing `shared/lambda-auth/package.json` deps invalidates every lambda's `package-lock.json`, so regenerate all six.
 
 ## Deploy
 

apps/backend/lambdas/auth/handler.ts

@@ -733,17 +733,21 @@ async function handleRegister(event: any): Promise<APIGatewayProxyResult> {
             );
             const sub = cognitoUser.UserAttributes?.find((a) => a.Name === 'sub')?.Value;
             if (sub && cognitoUser.UserStatus === 'CONFIRMED') {
-              await db
+              const linkResult = await db
                 .updateTable('branch.users')
                 .set({ cognito_sub: sub })
                 .where('user_id', '=', claimingUserId)
                 .where('cognito_sub', 'is', null)
-                .execute();
-              return json(200, {
-                message: 'Existing account linked',
-                claimed: true,
-                email: email.toLowerCase(),
-              });
+                .executeTakeFirst();
+              // A concurrent claim already took this row; do not delete the
+              // pre-existing Cognito user, it may back a working account.
+              if (linkResult.numUpdatedRows > 0n) {
+                return json(200, {
+                  message: 'Existing account linked',
+                  claimed: true,
+                  email: email.toLowerCase(),
+                });
+              }
             }
           } catch (linkError) {
             console.warn('Could not auto-link existing Cognito user:', linkError);
@@ -764,6 +768,20 @@ async function handleRegister(event: any): Promise<APIGatewayProxyResult> {
       return json(500, { message: 'Failed to register user in authentication service' });
     }
 
+    const rollbackCognitoUser = async () => {
+      try {
+        await cognitoClient.send(
+          new AdminDeleteUserCommand({
+            UserPoolId: USER_POOL_ID,
+            Username: email.toLowerCase(),
+          })
+        );
+        console.log('Rolled back Cognito user after database failure');
+      } catch (rollbackError) {
+        console.error('Failed to rollback Cognito user:', rollbackError);
+      }
+    };
+
     // Create user in database, or claim the pending invitation
     try {
       // Claim the invitation. is_admin is deliberately NOT touched: it was set
@@ -773,27 +791,28 @@ async function handleRegister(event: any): Promise<APIGatewayProxyResult> {
       // 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
+      const claimResult = await db
         .updateTable('branch.users')
         .set({ cognito_sub: cognitoUserSub, name: name.trim() })
         .where('user_id', '=', claimingUserId)
         .where('cognito_sub', 'is', null)
-        .execute();
+        .executeTakeFirst();
+
+      // No-op claim: the Cognito sub we just created would reference no row, so
+      // every later login would fail. Undo the Cognito user instead.
+      if (claimResult.numUpdatedRows === 0n) {
+        console.error('Invitation already claimed for user_id:', claimingUserId);
+        await rollbackCognitoUser();
+        return json(409, {
+          message: 'User with this email already exists',
+          code: 'ALREADY_CLAIMED',
+        });
+      }
     } catch (dbError: any) {
       console.error('Database insert error:', dbError);
 
       // Rollback: Delete user from Cognito if database insert fails
-      try {
-        await cognitoClient.send(
-          new AdminDeleteUserCommand({
-            UserPoolId: USER_POOL_ID,
-            Username: email.toLowerCase(),
-          })
-        );
-        console.log('Rolled back Cognito user after database failure');
-      } catch (rollbackError) {
-        console.error('Failed to rollback Cognito user:', rollbackError);
-      }
+      await rollbackCognitoUser();
 
       return json(500, { message: 'Failed to create user account' });
     }

apps/backend/lambdas/auth/test/auth.login.unit.test.ts

@@ -23,6 +23,7 @@ jest.mock('../auth', () => ({
 
 const mockExecuteTakeFirst = jest.fn();
 const mockExecute = jest.fn();
+const mockUpdateResult = jest.fn();
 const mockSet = jest.fn();
 const mockValues = jest.fn();
 
@@ -40,6 +41,7 @@ jest.mock('../db', () => {
     },
     where: () => updateChain,
     execute: (...a: unknown[]) => mockExecute(...a),
+    executeTakeFirst: (...a: unknown[]) => mockUpdateResult(...a),
   };
   const insertChain: any = {
     values: (...a: unknown[]) => {
@@ -84,6 +86,7 @@ const TOKENS = {
 
 beforeEach(() => {
   jest.clearAllMocks();
+  mockUpdateResult.mockResolvedValue({ numUpdatedRows: 1n });
   jest.spyOn(console, 'error').mockImplementation(() => undefined);
   jest.spyOn(console, 'warn').mockImplementation(() => undefined);
   jest.spyOn(console, 'log').mockImplementation(() => undefined);
@@ -563,7 +566,7 @@ describe('POST /register — claim-on-register', () => {
     mockSend
       .mockResolvedValueOnce({ UserSub: 'new-sub' }) // SignUp
       .mockResolvedValueOnce({}); // AdminDeleteUser
-    mockExecute.mockRejectedValue(new Error('db down'));
+    mockUpdateResult.mockRejectedValue(new Error('db down'));
 
     const res = await handler(event('/register', 'POST', validBody));
 

apps/backend/lambdas/donors/handler.ts

@@ -1,7 +1,7 @@
 import { APIGatewayProxyResult } from 'aws-lambda';
 import db from './db';
 import { authenticateRequest } from './auth';
-import { DonorValidationUtils, DonationValidationUtils } from './validation-utils';
+import { DonorValidationUtils } from './validation-utils';
 
 export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
   try {
@@ -144,48 +144,79 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
       if (donor_id === undefined || project_id === undefined || amount === undefined) {
         return json(400, { message: 'donor_id, project_id, and amount are required' });
       }
-      if (!Number.isInteger(donor_id) || (donor_id as number) < 1) {
+      // Numeric fields arrive as strings from form posts; amount is NUMERIC(12,2)
+      const num = (value: unknown) =>
+        typeof value === 'number' || (typeof value === 'string' && value.trim() !== '') ? Number(value) : NaN;
+      const donorId = num(donor_id);
+      const projectId = num(project_id);
+      const donationAmount = num(amount);
+
+      if (!Number.isInteger(donorId) || donorId < 1) {
         return json(400, { message: 'donor_id must be a positive integer' });
       }
-      if (!Number.isInteger(project_id) || (project_id as number) < 1) {
+      if (!Number.isInteger(projectId) || projectId < 1) {
         return json(400, { message: 'project_id must be a positive integer' });
       }
-      if (typeof amount !== 'number' || amount <= 0 || !isFinite(amount)) {
+      if (!isFinite(donationAmount) || donationAmount <= 0) {
         return json(400, { message: 'amount must be a positive number' });
       }
       // Check user is admin or a member of the project
       if (!authContext.user?.isAdmin) {
-      const userId = authContext.user!.userId as number;
-      const membership = await db
-        .selectFrom('branch.project_memberships')
-        .select('membership_id')
-        .where('project_id', '=', project_id as number)
-        .where('user_id', '=', userId)
+        const userId = authContext.user!.userId as number;
+        const membership = await db
+          .selectFrom('branch.project_memberships')
+          .select('membership_id')
+          .where('project_id', '=', projectId)
+          .where('user_id', '=', userId)
+          .executeTakeFirst();
+
+        if (!membership) {
+          return json(403, { message: 'You must be a member' });
+        }
+      }
+
+      // Checked after the membership check so project existence isn't leaked to non-members
+      const donor = await db
+        .selectFrom('branch.donors')
+        .select('donor_id')
+        .where('donor_id', '=', donorId)
         .executeTakeFirst();
 
-      if (!membership) {
-        return json(403, { message: 'You must be a member' });
+      if (!donor) {
+        return json(404, { message: 'Donor not found' });
+      }
+
+      const project = await db
+        .selectFrom('branch.projects')
+        .select('project_id')
+        .where('project_id', '=', projectId)
+        .executeTakeFirst();
+
+      if (!project) {
+        return json(404, { message: 'Project not found' });
       }
-    }
 
       try {
-      const donation = await db
-        .insertInto('branch.project_donations')
-        .values({
-          donor_id: donor_id as number,
-          project_id: project_id as number,
-          amount: amount as number,
-        })
-        .returningAll()
-        .executeTakeFirstOrThrow();
-
-      return json(201, { data: donation });
-    } catch (err: any) {
-      if (err?.code === '23505') {
-        return json(409, { message: 'A donation from this donor to this project already exists' });
+        const donation = await db
+          .insertInto('branch.project_donations')
+          .values({
+            donor_id: donorId,
+            project_id: projectId,
+            amount: donationAmount,
+          })
+          .returningAll()
+          .executeTakeFirstOrThrow();
+
+        return json(201, { data: donation });
+      } catch (err: any) {
+        if (err?.code === '23505') {
+          return json(409, { message: 'A donation from this donor to this project already exists' });
+        }
+        if (err?.code === '23503') {
+          return json(404, { message: 'Donor or project not found' });
+        }
+        throw err;
       }
-      throw err;
-    }
     }
 
     // POST /donors

apps/backend/lambdas/projects/handler.ts

@@ -143,6 +143,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
       // handles both /projects/1/members and /1/members
       const id = parts.length === 3 ? parts[1] : parts[0];
       if (!id) return json(400, { message: 'id is required' });
+      if (!/^\d+$/.test(id)) return json(400, { message: 'Project id must be a valid number' });
       if (!(await canAccessProject(user.userId!, Number(id)))) {
         return json(403, { message: 'You do not have access to this project' });
       }
@@ -155,7 +156,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
           'u.email',
           'pm.role'
         ])
-        .where('pm.project_id', '=', id)
+        .where('pm.project_id', '=', Number(id))
         .execute();
       return json(200, {
         ok: true, route: 'GET /projects/{id}/members', pathParams: { id }, body: {
@@ -213,14 +214,15 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
         "branch.donors as bd",
         "bd.donor_id",
         "bpd.donor_id"
-      ).selectAll().execute();
+      ).select(['bd.donor_id', 'bd.organization', 'bd.contact_name', 'bd.contact_email', 'bpd.donation_id', 'bpd.amount', 'bpd.donated_at']).execute();
       return json(200, { donors });
     }
 
     // GET /projects/{id}
     if (rawPath.startsWith('/') && rawPath.split('/').length === 2 && method === 'GET') {
       const id = rawPath.split('/')[1];
       if (!id) return json(400, { message: 'id is required' });
+      if (!/^\d+$/.test(id)) return json(400, { message: 'Project id must be a valid number' });
       if (!(await canAccessProject(user.userId!, Number(id)))) {
         return json(403, { message: 'You do not have access to this project' });
       }
@@ -234,6 +236,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
     if (rawPath.startsWith('/') && rawPath.split('/').length === 2 && method === 'PUT') {
       const id = rawPath.split('/')[1];
       if (!id) return json(400, { message: 'id is required' });
+      if (!/^\d+$/.test(id)) return json(400, { message: 'Project id must be a valid number' });
       if (!(await canEditProject(user.userId!, Number(id)))) {
         return json(403, { message: 'You do not have access to edit this project' });
       }
@@ -347,6 +350,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
         id = pathParts[0];
       }
       if (!id) return json(400, { message: 'id is required' });
+      if (!/^\d+$/.test(id)) return json(400, { message: 'Project id must be a valid number' });
 
       if (!(await canAccessProject(user.userId!, Number(id)))) {
         return json(403, { message: 'You do not have access to this project' });
@@ -356,7 +360,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
 
         const project = await db
           .selectFrom('branch.projects')
-          .where('project_id', '=', parseInt(id))
+          .where('project_id', '=', Number(id))
           .selectAll()
           .executeTakeFirst();
 
@@ -367,7 +371,7 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => {
 
         const expenditures = await db
           .selectFrom('branch.expenditures')
-          .where('project_id', '=', parseInt(id))
+          .where('project_id', '=', Number(id))
           .selectAll()
           .orderBy('spent_on', 'desc')
           .execute();