fix: address audit findings across lambdas, frontend and infra - #310
Merged
Conversation
Fixes the actionable findings from a full-repo bug scan. Findings already covered by open PRs (#301-#307) are deliberately untouched. Security / data exposure: - Lock down the generated-reports S3 bucket. All four block_public_* were false and a bucket policy granted s3:GetObject to Principal "*", so reports (member emails, donor contacts, expenditure amounts) were world-readable at predictable keys. Reports are now served only through a presigned GET. - Grant the lambda role s3:PutObject/s3:GetObject on the reports bucket. It had no S3 permissions at all, so POST /reports/generate was failing AccessDenied; GetObject is additionally required because a presigned URL carries the signer's permissions. - Validate objectUrl on POST /reports against the bucket's own host, so an arbitrary (e.g. javascript:) URL can no longer be stored and rendered. - Sanitize fileName before interpolating it into an S3 key in GET /reports/upload-url. - Stop logging every user row (emails, admin flags) to CloudWatch. Correctness: - Lowercase emails in UserValidationUtils.validateEmail. POST /users stored them verbatim while POST /auth/register looks up email.toLowerCase(), so any invite containing uppercase was permanently unclaimable (403 INVITATION_REQUIRED). - POST /auth/register now checks numUpdatedRows on the invitation claim. A no-op claim previously still returned 201, leaving a Cognito user whose sub referenced no row, which broke every later login. Same check on the auto-link path. - DELETE /users/{userId} deletes the Cognito user too, so the address can be re-invited. - PATCH /users/{userId} rejects email changes. Email is the Cognito username and nothing synced it, so a change silently broke sign-in and reset. - POST /donations returns 404 for a missing donor/project instead of 500, and accepts numeric strings for amount and ids. - GET /projects/{id}/donors selects explicit columns; selectAll() over a 3-table join collided project_id and leaked the whole project row. - Reject non-numeric path ids on the users and projects {id} routes, which reached Postgres as NaN and surfaced as 500s. Features that were half-built: - Add GET /reports/{id}/download returning a presigned URL. Reports could be generated but never retrieved; the frontend used object_url only for a format label. - Wire up bulk delete on the reports page. It was stubbed behind a stale comment claiming no DELETE endpoint existed, though DELETE /reports/{id} has been there all along. - Switch the reports page to server-side pagination and surface transient delete/download failures in a non-blocking banner. - Pass report_type through POST /reports/generate and return file_type. Cleanup: - Single canonical region-qualified S3 URL helper; the two call sites disagreed and the region-less form only resolved via a redirect. - Drop the unused DonationValidationUtils import. - Reconcile the duplicated auth DTOs (isAdmin is now required in both). Full dedup needs a packaging change, since neither shared package can resolve the other without a new cross-dependency. - Gitignore lambda.zip. Users-lambda tests now mock the Cognito SDK: CI injects a real user pool id, and the DELETE tests target seeded ashley@branch.org, so they would otherwise have issued live AdminDeleteUser calls against production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
nourshoreibah
added
the
test-environment
Contributor
|
🌿 ⏳ Creating preview environment… (logs) |
Contributor
🌿 Preview environment — ready ✅Open: https://d3nmtjoh6ir9ym.cloudfront.net/pr-310/ Shared RDS + Cognito (prod data); DB migrations are not applied here — if this PR adds a migration, endpoints using the new columns will fail until it merges. New commits update this environment in place — a note is posted here on each update. Remove the |
There was a problem hiding this comment.
Pull request overview
Addresses repo-wide audit findings spanning infrastructure, backend lambdas, shared types, and the frontend—primarily tightening S3 access for generated reports, fixing Cognito/user lifecycle edge cases, and completing report download + pagination flows.
Changes:
- Infrastructure: make the reports S3 bucket fully private and grant the shared Lambda role scoped
s3:GetObject/s3:PutObjectpermissions for reports. - Backend: add presigned download support for reports, normalize email validation, harden auth registration invitation-claim/linking correctness, delete Cognito users on user deletion, and improve ID/DB error handling across routes.
- Frontend: move reports to server-side pagination, add download + working bulk delete UX, and update tests accordingly.
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/types/auth-types.d.ts | Makes isAdmin required on the shared AuthenticatedUser DTO. |
| infrastructure/aws/s3.tf | Locks down reports bucket public access (removes public policy + blocks public settings). |
| infrastructure/aws/README.md | Updates generated Terraform resource inventory to reflect new IAM policy and removed bucket policy. |
| infrastructure/aws/lambda.tf | Adds IAM policy for reports bucket read/write; updates Cognito admin-policy comment. |
| infrastructure/AGENTS.md | Updates infra docs to reflect reports bucket privacy and new IAM permissions. |
| apps/frontend/test/components/ReportsPage.test.tsx | Updates mocks/assertions to the new paginated /reports?page=&limit= fetch contract. |
| apps/frontend/src/app/reports/page.tsx | Implements server-side pagination, bulk delete via per-report DELETE, and presigned download UX. |
| apps/backend/lambdas/users/validation-utils.ts | Normalizes emails (trim + lowercase) at validation choke point. |
| apps/backend/lambdas/users/test/users.test.ts | Mocks Cognito SDK to prevent destructive live calls in CI; updates PATCH expectations. |
| apps/backend/lambdas/users/test/user.unit.test.ts | Mocks Cognito SDK in unit tests; updates PATCH expectations around immutable email. |
| apps/backend/lambdas/users/package.json | Adds Cognito Identity Provider AWS SDK dependency for AdminDeleteUser. |
| apps/backend/lambdas/users/package-lock.json | Locks new Cognito client dependency (and associated lockfile updates). |
| apps/backend/lambdas/users/handler.ts | Adds Cognito delete-on-user-delete, rejects non-numeric IDs, and makes email immutable on PATCH. |
| apps/backend/lambdas/reports/test/reports.unit.test.ts | Extends report-service mock with objectUrlFor/keyFromObjectUrl helpers. |
| apps/backend/lambdas/reports/test/reports.e2e.test.ts | Sets REPORTS_BUCKET_NAME env for URL helper tests. |
| apps/backend/lambdas/reports/report-service.ts | Introduces canonical region-qualified S3 URL helper + key extraction/validation. |
| apps/backend/lambdas/reports/README.md | Documents new GET /reports/{id}/download endpoint. |
| apps/backend/lambdas/reports/openapi.yaml | Adds OpenAPI spec for presigned report download endpoint. |
| apps/backend/lambdas/reports/handler.ts | Adds GET /reports/{id}/download, filename sanitization, report_type support, URL helper usage. |
| apps/backend/lambdas/projects/test/projects.unit.test.ts | Updates invalid-id expectation from 500 to 400. |
| apps/backend/lambdas/projects/test/example.test.ts | Removes assertions that depended on leaking project columns in donors list responses. |
| apps/backend/lambdas/projects/handler.ts | Rejects non-numeric IDs and replaces broad selectAll() in donors join with explicit columns. |
| apps/backend/lambdas/donors/handler.ts | Accepts numeric strings for IDs/amount, adds FK-not-found handling, removes unused import. |
| apps/backend/lambdas/auth/test/auth.login.unit.test.ts | Adjusts mocks to assert/update on executeTakeFirst result for invitation claims. |
| apps/backend/lambdas/auth/handler.ts | Checks numUpdatedRows for invitation claim/link paths; refactors Cognito rollback helper. |
| .gitignore | Ignores lambda.zip build artifacts. |
Files not reviewed (1)
- apps/backend/lambdas/users/package-lock.json: Generated file
Suppressed comments (2)
apps/backend/lambdas/reports/handler.ts:306
- GET /reports/{id}/download signs whatever key is embedded in report.object_url as long as it is in the reports bucket. If a report row were ever created with a cross-project key, this endpoint would leak the other object's contents. Enforce that the key is under the expected
reports/${report.project_id}/prefix before minting a presigned URL.
const key = keyFromObjectUrl(report.object_url);
if (!key) {
return json(409, { message: 'Report is not stored in the reports bucket' });
}
const downloadUrl = await getSignedUrl(s3, new GetObjectCommand({
Bucket: BUCKET,
Key: key,
}), { expiresIn: DOWNLOAD_URL_TTL_SECONDS });
apps/backend/lambdas/donors/handler.ts:220
- The POST /donations try/catch block is also mis-indented, which obscures which statements are inside the try vs after it. Re-indent this section to keep the error handling and return paths clear.
try {
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;
}
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+256
to
+258
| if (!keyFromObjectUrl(objectUrl)) { | ||
| return json(400, { message: 'objectUrl must point at the reports bucket' }); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Confirmed and fixed. The path was real: a member of project A could POST a row with project_id: A and an objectUrl under reports/B/ (host check passes, checkProjectAccess(A) passes), then read project B's object back via the download route, which authorizes off report.project_id.
Keys are now bound to their project through a single reportKeyPrefix(projectId) helper used for both construction and validation, so the two can't drift:
POST /reportsrejects a key outside the project's prefix.GET /reports/{id}/downloadre-checks the stored key againstreport.project_idbefore presigning, which also covers any row written before this validation existed.
Two notes on the details. I placed the prefix check after the project-existence and checkProjectAccess calls rather than with the other body validation, so 404/403 still take precedence and an unauthorized caller doesn't learn anything about key layout. And all existing rows satisfy the prefix — both key-construction paths (uploadToS3 and /reports/upload-url) already used reports/{projectId}/ — so this is not a breaking change for stored data. Added one regression test for the cross-project rejection.
Comment on lines
149
to
+152
| useEffect(() => { | ||
| fetchReports(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [currentPage]); |
Collaborator
Author
There was a problem hiding this comment.
Fixed. selectedIds is now cleared in the [currentPage] effect.
Verified the failure mode before changing it: selection genuinely survived a page change, and handleDeleteSelected iterates all of selectedIds, so selecting a row on page 1 and hitting Delete from page 2 removed the page-1 row with nothing on screen indicating it. That's a regression I introduced in this PR — under the previous client-side pagination reports held every row, so a cross-page selection was at least coherent.
Comment on lines
+206
to
+214
| let cognitoDeleted = true; | ||
| try { | ||
| await cognitoClient.send(new AdminDeleteUserCommand({ UserPoolId: USER_POOL_ID, Username: user.email })); | ||
| } catch (err: any) { | ||
| if (err?.name !== 'UserNotFoundException') { | ||
| console.error('Cognito delete error:', err); | ||
| cognitoDeleted = false; | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Partly. The remedy you describe was already in place, so I implemented the narrower improvement.
An empty UserPoolId makes AdminDeleteUser throw InvalidParameterException, which is not UserNotFoundException, so it already fell into the catch that sets cognitoDeleted = false. The response therefore already reported cognitoDeleted: false rather than a silent success — I saw exactly this locally, where the pool id is unset.
What was genuinely worth fixing is the part about attempting the call: it can never succeed, and the resulting error is an opaque SDK exception rather than a clear signal. It now short-circuits when USER_POOL_ID is empty, logs it as a configuration error, and sets cognitoDeleted = false without the round trip.
I did not take the "fail the request" option. By that point the branch.users row is already deleted, so a 4xx/5xx would misreport what happened; 200 plus cognitoDeleted: false is the accurate contract for a partial success. Worth noting the deployed lambdas always have COGNITO_USER_POOL_ID set — infrastructure/aws/lambda.tf declares it in an authoritative environment block — so this is a local-dev and misconfiguration guard rather than a production path.
Comment on lines
164
to
176
| @@ -168,13 +175,34 @@ export const handler = async (event: any): Promise<APIGatewayProxyResult> => { | |||
| } | |||
| } | |||
Collaborator
Author
There was a problem hiding this comment.
Fixed. Re-indented both the membership block and the try/catch in POST /donations.
The concern was accurate: the membership if body sat at the same level as the if itself and its closing brace was outdented two spaces, so it read as though it closed the route block rather than the conditional — which is exactly the kind of thing that gets mis-edited. Same shape on the try.
Whitespace only, no behavioural change; donors tests are unchanged at 50 passing (the one failure is the pre-existing health test, which fetches localhost:3000 and needs the lambda running). Note this file's GET routes are being rewritten by #301, so I kept the reformatting strictly inside POST /donations to avoid widening that merge.
The auth DTOs were declared twice, in shared/types/auth-types.d.ts and shared/lambda-auth/src/types.ts, and had already drifted (isAdmin was optional in one and required in the other). The previous commit only reconciled the two copies and left a "keep these in sync" comment, which is just documented duplication. shared/lambda-auth now takes a file: dependency on @branch/types and re-exports the DTOs from it, so there is exactly one declaration. @branch/types stays a dependency-free leaf, which is what keeps the edge acyclic; lambdas are unaffected because they already depend on both packages, and the types are erased at compile time so nothing reaches the bundle. Adding a dependency to shared/lambda-auth changes the resolved tree for every lambda, so all six package-lock.json files are regenerated. They were already stale: none recorded lambda-auth's devDependencies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Addresses the four review comments on #310. The significant one: POST /reports only checked that objectUrl pointed at the reports bucket, not that the key belonged to the project being written. A caller with access to project A could register a row with project_id A and an objectUrl under reports/B/, then read project B's report back through GET /reports/{id}/download, which authorizes off report.project_id. That defeats the access control this PR set out to add. Keys are now bound to their project via a single reportKeyPrefix() helper used for both construction and validation: POST /reports rejects a key outside the project's prefix, and the download route re-checks the stored key against report.project_id before presigning. The prefix check runs after the access check so 403/404 still take precedence. All existing rows match the prefix, since both key-construction paths already used it. Also: - Clear the reports-page selection on page change. With server-side pagination selectedIds could retain rows from a previous page and bulk delete would remove them unseen. - Skip the Cognito delete when COGNITO_USER_POOL_ID is unset instead of making a call that cannot succeed, and log it as a configuration error. cognitoDeleted already reported false in this case via the InvalidParameterException path. - Re-indent the POST /donations membership and try/catch blocks, whose closing braces read as though they closed the route. One regression test covers the cross-project key rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
This was referenced Aug 12, 2026
Conflicts in infrastructure/aws/lambda.tf and the generated infrastructure/aws/README.md. main's #314 added aws_iam_role_policy.lambda_s3_objects with the same actions (s3:PutObject, s3:GetObject) on the same resource (reports_bucket) as the lambda_reports_bucket policy added here, so the two were functionally identical. Kept main's resource and dropped the duplicate, folding this branch's rationale (report generation was failing AccessDenied, and a presigned GET needs the signer to hold GetObject) into its comment. README.md is terraform-docs output: took main's copy and dropped the aws_s3_bucket_policy.reports_bucket_policy row, since this branch deletes that resource. CI regenerates this file regardless.
…o worktree-fix-audit-findings
Contributor
Terraform Plan 📖
|
Contributor
Contributor
|
🌿 Preview environment torn down 🧹 — the stack for this PR has been destroyed. |
nourshoreibah
added a commit
that referenced
this pull request
Aug 12, 2026
Resolves conflicts between the admin-only dashboard and main's expense approval flow (#315), project role rename (#311) and audit fixes (#310): - routes: /dashboard is admin-gated, /expenses is not. Main opened /expenses to non-admins because they submit and read their own expenses there; only the review modal's approve/deny is admin-gated. - accounts: both sides moved the staff roster out of page.tsx to satisfy the Next.js page-export rule. Kept main's mockUsers.ts and dropped the duplicate staff.ts. - Navbar/routes tests follow the same split. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the actionable findings from a full-repo bug scan. Findings already covered by open PRs (#301–#307) are deliberately untouched — see the bottom section.
Critical
The generated-reports S3 bucket was world-readable. All four
block_public_*werefalseplus a bucket policy grantings3:GetObjecttoPrincipal: "*". Reports embed member names and emails, donor contacts, and every expenditure amount, at predictable keys (reports/{projectId}/{ISO-timestamp}.pdf). Bucket is now fully private and the policy is gone.The lambda role had no S3 permissions at all — only
AWSLambdaBasicExecutionRole+ Cognito. SoPOST /reports/generatehas been failingAccessDeniedin production. Addeds3:PutObject/s3:GetObjecton the reports bucket;GetObjectis also required because a presigned URL carries the signer's permissions.Any invite containing an uppercase letter created a permanently unclaimable account.
validateEmailreturned the string verbatim, soPOST /usersstoredAlice@Branch.org, whilePOST /auth/registerlooks upemail.toLowerCase()— it missed and returned403 INVITATION_REQUIRED, indistinguishable from "never invited". Emails are now normalized at the single validation choke point.Correctness
POST /auth/registernow checksnumUpdatedRowson the invitation claim. A no-op claim still returned 201, leaving a Cognito user whose sub referenced no row — every later login failed, unfixable without manual SQL. Rolls back the Cognito user and returns 409ALREADY_CLAIMED; the auto-link path got the same check.DELETE /users/{userId}deletes the Cognito user too, so the address can be re-invited. Without this, once feat(auth): #243 Admin Invite User #302 lands, delete-then-reinvite 409s forever.PATCH /users/{userId}rejects email changes — email is the Cognito username and nothing synced it, so a self-service change silently broke sign-in and password reset.POST /donationsreturns 404 for a missing donor/project instead of 500 (23503was unhandled), and accepts numeric strings foramount/ids since the column isNUMERIC(12,2)and forms post strings.GET /projects/{id}/donorsselects explicit columns.selectAll()over a 3-table join emittedp.*, bpd.*, bd.*, collided onproject_id, and leaked the whole project row into a donors list.{id}routes; they previously reached Postgres asNaNand surfaced as 500s.GET /users.Half-built features finished
GET /reports/{id}/download→{ downloadUrl, expiresIn: 900 }. Reports could be generated but never retrieved: nothing presigned a GET, and the frontend usedobject_urlonly to compute a format label. Same auth +checkProjectAccessasGET /reports/{id}; 409 if the stored URL isn't ours.DELETE /reports/{id}has been there all along.POST /reports/generateacceptsreport_typeand returnsfile_type; it previously hardcoded'technical'even for a.docx.Cleanup
DonationValidationUtilsimport (its camelCase keys andamount: 0tolerance conflict with this route's contract and tests).isAdminis now required in both. Full dedup is blocked by packaging — neithershared/typesnorshared/lambda-authcan resolve the other without a new cross-dependency, andshared/typesguarantees zero dependencies. Proper fix is a third types-only package.lambda.zip(~5 MB each, previously onegit add -Afrom being committed).Test-safety fix worth reviewing
The users-lambda tests now mock
@aws-sdk/client-cognito-identity-provider. CI injects a realCOGNITO_USER_POOL_ID, and the DELETE tests targetpath: '/1'— seededashley@branch.org. Without the mock, every CI run would issue a liveAdminDeleteUseragainst the production pool, which becomes destructive the moment that address has a Cognito user. Verified zero live calls, including under a real-looking pool id.Verification
tsc --noEmit: clean across all six lambdas + frontend.fetcheshttp://localhost:3000with no dev server running; confirmed unrelated to these changes.terraform fmt -check -recursive infrastructure/clean.prettier --checkfails on 30 files, including ones untouched here — pre-existing repo-wide, so not reformatted.Merge ordering
Additive-only overlaps with open PRs, but worth sequencing deliberately:
receipts_bucketto the end ofs3.tfand an IAM policy at the same anchor inlambda.tf; this PR edits the top ofs3.tfand inserts at that anchor. Both merge, but expect a triviallambda.tfconflict.GETlist routes indonors/reportshandlers. Those routes were left alone here on purpose. One review note for scope read endpoints to project membership/admins #301:.where('project_id','in',projectIds)with an empty array rendersin (), a Postgres syntax error — a member of zero projects would get a 500.users/package.json; this PR adds the same dependency, so expect a lockfile conflict. feat(auth): #243 Admin Invite User #302 also passesemailunchanged toAdminCreateUserwhile/auth/loginlowercases it — same root cause as the invite bug fixed here, worth applying there too.Not addressed
Three local
.envfiles hold long-livedAKIA…IAM keys (apps/backend/.env,apps/backend/lambdas/{auth,reports}/.env). All are gitignored and absent fromgit ls-files, so nothing leaked — but static keys for a shared account are the wrong shape. Recommend rotating to short-lived credentials and confirming they aren't the deploy keys. Not a code change, so out of scope here.🤖 Generated with Claude Code