From 095b1ca6d9ad2cd333cc7dcfff87116852ba325b Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Tue, 11 Aug 2026 22:00:43 -0400 Subject: [PATCH 1/3] feat(expenses): admin_notes column and lambda S3 object permissions Groundwork for the expense review flow, split out so it lands before the lambda code that depends on it. - Adds a nullable `admin_notes TEXT` to `expenditures` for the reviewing admin's note. Additive and forward-only. - Grants the shared lambda role `s3:PutObject`/`s3:GetObject` on the reports bucket. The role had no S3 permissions at all, so presigning a receipt upload would have failed AccessDenied -- a presigned URL carries the signer's permissions. Co-Authored-By: Claude Opus 5 (1M context) --- ...0812012951_add_expenditure_admin_notes.sql | 16 ++++++++++++++ infrastructure/aws/lambda.tf | 21 +++++++++++++++++++ shared/types/db-types.d.ts | 1 + 3 files changed, 38 insertions(+) create mode 100644 apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql diff --git a/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql b/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql new file mode 100644 index 00000000..8cf39f6a --- /dev/null +++ b/apps/backend/db/migrations/20260812012951_add_expenditure_admin_notes.sql @@ -0,0 +1,16 @@ +-- add_expenditure_admin_notes +-- +-- Every pending migration runs inside a SINGLE transaction, with +-- search_path = branch, public -- so table names can be unqualified, and +-- CREATE INDEX CONCURRENTLY / VACUUM will not work here. +-- +-- This migration is applied to PRODUCTION automatically when the PR merges, +-- BEFORE the new lambda code is deployed. It must be safe for the code that is +-- live right now: additive changes only. See apps/backend/db/README.md for the +-- expand/contract rules that destructive changes need. +-- +-- Forward-only: there is no rollback. Fix a mistake with a new migration, and +-- never edit a migration that has been merged -- someone has already run it. +-- Do not use IF NOT EXISTS: you want a failure, not silent drift. + +ALTER TABLE expenditures ADD COLUMN admin_notes TEXT; diff --git a/infrastructure/aws/lambda.tf b/infrastructure/aws/lambda.tf index 8bddf3d3..f5111f5a 100644 --- a/infrastructure/aws/lambda.tf +++ b/infrastructure/aws/lambda.tf @@ -46,6 +46,27 @@ resource "aws_iam_role_policy" "lambda_cognito_admin" { }) } +# The expenditures lambda presigns receipt uploads and downloads. A presigned +# URL carries the signer's permissions, so the role needs both PutObject and +# GetObject or the browser's PUT/GET fails AccessDenied. +resource "aws_iam_role_policy" "lambda_s3_objects" { + name = "branch-lambda-s3-objects" + role = aws_iam_role.lambda_role.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "LambdaReportsBucketObjects" + Effect = "Allow" + Action = [ + "s3:PutObject", + "s3:GetObject", + ] + Resource = "${aws_s3_bucket.reports_bucket.arn}/*" + }] + }) +} + # Get AWS account ID for unique bucket naming data "aws_caller_identity" "current" {} diff --git a/shared/types/db-types.d.ts b/shared/types/db-types.d.ts index a1ffe4ed..00f906ae 100644 --- a/shared/types/db-types.d.ts +++ b/shared/types/db-types.d.ts @@ -30,6 +30,7 @@ export interface BranchDonors { } export interface BranchExpenditures { + admin_notes: string | null; amount: Numeric; category: string | null; created_at: Generated; From 3d00fb3b7ee28638c98aa2f9176c69d2e85a282e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Wed, 12 Aug 2026 02:03:41 +0000 Subject: [PATCH 2/3] chore: auto-format terraform and update documentation - Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah --- infrastructure/aws/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/infrastructure/aws/README.md b/infrastructure/aws/README.md index 890a502f..d83d4dc2 100644 --- a/infrastructure/aws/README.md +++ b/infrastructure/aws/README.md @@ -49,6 +49,7 @@ No modules. | [aws_iam_role_policy.ci_plan_state_lock](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy.ci_preview](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy.lambda_cognito_admin](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | +| [aws_iam_role_policy.lambda_s3_objects](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy) | resource | | [aws_iam_role_policy_attachment.ci_apply_admin](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | | [aws_iam_role_policy_attachment.ci_plan_readonly](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | | [aws_iam_role_policy_attachment.lambda_basic](https://registry.terraform.io/providers/hashicorp/aws/6.14.1/docs/resources/iam_role_policy_attachment) | resource | From cab3d95a0bd520869d89ee877f582fefdf34a1d3 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Tue, 11 Aug 2026 22:09:18 -0400 Subject: [PATCH 3/3] fix(ci): scan migration SQL, not comments, in the destructive-SQL guard `make new-migration` scaffolds a header that reads "CREATE INDEX CONCURRENTLY / VACUUM will not work here". The guard grepped the raw file, so that sentence matched and every scaffolded migration failed before it contained any SQL at all. This is the first new migration since the guard landed -- the only other file, the baseline, predates the template -- so nothing had tripped it yet. Comments are now blanked before the destructive/CONCURRENTLY/user-insert scans. Blanking rather than deleting the lines keeps grep -n line numbers pointing at the real line. The allow-destructive opt-out still reads the raw file, since that marker is itself a comment. Verified the guard still fails a real CREATE INDEX CONCURRENTLY and a real ALTER TABLE ... DROP COLUMN, and still passes the baseline. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/lambda-tests.yml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/lambda-tests.yml b/.github/workflows/lambda-tests.yml index 92676d9e..fac7ff00 100644 --- a/.github/workflows/lambda-tests.yml +++ b/.github/workflows/lambda-tests.yml @@ -213,19 +213,25 @@ jobs: for f in $files; do [ -f "$f" ] || continue # '-- allow-destructive: ' opts a file out, deliberately loudly. + # Read from the raw file: this opt-out is itself a comment. if grep -qi '^-- allow-destructive:' "$f"; then echo "::warning file=$f::opted out of the destructive-SQL check" continue fi - if grep -inE '\b(DROP[[:space:]]+SCHEMA|DROP[[:space:]]+DATABASE|TRUNCATE|DROP[[:space:]]+TABLE|DROP[[:space:]]+COLUMN)\b' "$f"; then + # Scan SQL, not prose. `make new-migration` scaffolds a header that + # says "CREATE INDEX CONCURRENTLY / VACUUM will not work here", and + # matching that failed every generated migration. Blanking comments + # rather than dropping the lines keeps grep -n line numbers honest. + sql=$(sed 's/--.*//' "$f") + if printf '%s\n' "$sql" | grep -inE '\b(DROP[[:space:]]+SCHEMA|DROP[[:space:]]+DATABASE|TRUNCATE|DROP[[:space:]]+TABLE|DROP[[:space:]]+COLUMN)\b'; then echo "::error file=$f::destructive statement. Migrations run against production BEFORE the new lambda code deploys, so the currently deployed code would hit the changed schema. See the expand/contract rules in apps/backend/db/README.md. Add '-- allow-destructive: ' only for a contract-phase migration whose expand phase is already live." fail=1 fi - if grep -inE 'CONCURRENTLY' "$f"; then + if printf '%s\n' "$sql" | grep -inE 'CONCURRENTLY'; then echo "::error file=$f::CONCURRENTLY cannot run inside a transaction, and the migrator wraps the run in one. This database is tiny -- use a plain CREATE INDEX." fail=1 fi - if grep -inE '^[[:space:]]*INSERT[[:space:]]+INTO[[:space:]]+(branch\.)?users\b' "$f"; then + if printf '%s\n' "$sql" | grep -inE '^[[:space:]]*INSERT[[:space:]]+INTO[[:space:]]+(branch\.)?users\b'; then echo "::warning file=$f::inserting users in a migration puts rows in PRODUCTION. Seeded users with a NULL cognito_sub are claimable by POST /auth/register, so this can hand someone an account. Dev seed rows belong in apps/backend/db/seed.sql." fi done