From a7c95137bcf4088c67e807b4d44ed084f4c385fc Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Tue, 11 Aug 2026 22:15:54 -0400 Subject: [PATCH] fix(preview): point preview lambdas at the branch RDS, not DBInstances[0] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preview environments 401'd on every authenticated request while prod was fine. preview-env.yml resolved the preview lambdas' DB_HOST with `aws rds describe-db-instances --query "DBInstances[0].Endpoint.Address"`. This account hosts several unrelated C4C databases, and the branch instance is not first, so every preview lambda was pointed at `bhchp-postgres` — another project's DB, whose security group blackholes our traffic. Each query hit the 5s `connectionTimeoutMillis` in db.ts and threw. The prod lambda config the workflow already reads carries the correct DB_HOST, so stop re-deriving it and inherit it like every other DB credential. Added a guard that fails the job if any required key is missing, rather than shipping a preview that 401s on every call. Second half of the bug: authenticateRequest wrapped both the JWT check and the branch.users lookup in one try/catch that returned `isAuthenticated: false`, so an unreachable database surfaced as 401 "Authentication required". That hid the real fault and, because the frontend treats 401 as an expired session, cleared the user's tokens and logged them out on a DB blip. Only token verification is caught now; DB errors and a missing COGNITO_USER_POOL_ID propagate to the handlers' existing 500 mapping. Existing preview stacks were repaired out of band — the workflow only resolves lambda env on label-add, so already-created stacks kept the bad DB_HOST. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/preview-env.yml | 22 +++++-- infrastructure/preview/variables.tf | 6 +- shared/lambda-auth/src/authenticate.ts | 67 +++++++++++--------- shared/lambda-auth/test/authenticate.test.ts | 24 +++---- 4 files changed, 69 insertions(+), 50 deletions(-) diff --git a/.github/workflows/preview-env.yml b/.github/workflows/preview-env.yml index 38fe31d3..62958368 100644 --- a/.github/workflows/preview-env.yml +++ b/.github/workflows/preview-env.yml @@ -115,19 +115,29 @@ jobs: exit 1 fi - # Build the preview lambdas' env from the ACTUAL prod config (single source - # of truth) + the live RDS endpoint. Reserved / credential keys are dropped; - # the module adds NODE_ENV. Only needed when creating the stack. + # Prod config is the single source of truth, DB_HOST included. Never re-derive + # it from `DBInstances[0]` -- this account hosts other C4C databases and that + # picked an unreachable one. Reserved / credential keys are dropped; the module + # adds NODE_ENV. Only needed when creating the stack. - name: Resolve preview lambda env if: steps.mode.outputs.create == 'true' run: | - DB_HOST=$(aws rds describe-db-instances --query "DBInstances[0].Endpoint.Address" --output text) + set -euo pipefail AUTH_ENV=$(aws lambda get-function-configuration --function-name branch-auth --query 'Environment.Variables' --output json) REPORTS_ENV=$(aws lambda get-function-configuration --function-name branch-reports --query 'Environment.Variables' --output json) - ENV_JSON=$(jq -n --argjson a "$AUTH_ENV" --argjson r "$REPORTS_ENV" --arg dbh "$DB_HOST" ' - (($a // {}) + ($r // {}) + { "DB_HOST": $dbh }) + ENV_JSON=$(jq -n --argjson a "$AUTH_ENV" --argjson r "$REPORTS_ENV" ' + (($a // {}) + ($r // {})) | del(.NODE_ENV, .AWS_REGION, .AWS_DEFAULT_REGION, .AWS_ACCESS_KEY_ID, .AWS_SECRET_ACCESS_KEY, .AWS_SESSION_TOKEN) ') + # Fail loudly rather than shipping a preview that 401s on every DB call. + for key in DB_HOST DB_NAME DB_USER DB_PASSWORD COGNITO_USER_POOL_ID COGNITO_CLIENT_ID; do + value=$(jq -r --arg k "$key" '.[$k] // ""' <<<"$ENV_JSON") + if [ -z "$value" ]; then + echo "::error::branch-auth/branch-reports did not provide $key; preview lambdas would be misconfigured." + exit 1 + fi + done + echo "Preview DB_HOST: $(jq -r '.DB_HOST' <<<"$ENV_JSON")" # Stash for the terraform step (multiline-safe). printf 'LAMBDA_ENV<> "$GITHUB_ENV" diff --git a/infrastructure/preview/variables.tf b/infrastructure/preview/variables.tf index 3a0e3db1..5e0f900a 100644 --- a/infrastructure/preview/variables.tf +++ b/infrastructure/preview/variables.tf @@ -4,9 +4,9 @@ variable "pr_number" { } # Full runtime environment for the preview lambdas, resolved by the workflow -# from the ACTUAL prod lambda config (branch-auth + branch-reports) plus the -# shared RDS endpoint. Passing it in keeps prod as the single source of truth -# for DB creds / Cognito ids / reports bucket rather than duplicating them here. +# from the ACTUAL prod lambda config (branch-auth + branch-reports). Passing it in +# keeps prod as the single source of truth for DB_HOST / DB creds / Cognito ids / +# reports bucket rather than duplicating -- or re-deriving -- them here. # Preview envs deliberately reuse the shared RDS + Cognito pool (data risk is # accepted); migrations are NEVER run from this module -- the generated DB types # hardcode the `branch.` schema prefix, so a per-PR schema would require per-PR diff --git a/shared/lambda-auth/src/authenticate.ts b/shared/lambda-auth/src/authenticate.ts index f832ee8e..d0941119 100644 --- a/shared/lambda-auth/src/authenticate.ts +++ b/shared/lambda-auth/src/authenticate.ts @@ -49,39 +49,46 @@ export async function authenticateRequest( const token = extractToken(event); if (!token) return { isAuthenticated: false }; - try { - const payload = await getVerifier().verify(token); - - const dbUser = await db - .selectFrom('branch.users') - .where('cognito_sub', '=', payload.sub) - .selectAll() - .executeTakeFirst(); - - if (!dbUser) { - console.warn( - 'User authenticated with Cognito but not found in database:', - payload.sub, - ); - return { isAuthenticated: false }; - } - - const user: AuthenticatedUser = { - cognitoSub: payload.sub, - userId: dbUser.user_id, - email: payload.email as string | undefined, - isAdmin: dbUser.is_admin === true, - // Informational only. We deliberately do NOT promote on a Cognito - // "Admins" group: branch.users.is_admin is the single source of truth. - // A second source would make demotion via PATCH /users/{userId} silently - // ineffective, nothing in this codebase writes group membership, and no - // aws_cognito_user_group is defined in infrastructure/aws/cognito.tf. - cognitoGroups: payload['cognito:groups'] as string[] | undefined, - }; + // Outside the try: missing config is a broken deployment, not a bad token. + const jwtVerifier = getVerifier(); - return { user, isAuthenticated: true }; + let payload: any; + try { + payload = await jwtVerifier.verify(token); } catch (error) { + // Only an unverifiable token is genuinely unauthenticated. console.error('Token verification failed:', error); return { isAuthenticated: false }; } + + // Uncaught on purpose: a DB outage is not a 401. Catching it hid an unreachable + // RDS behind "Authentication required" and logged users out. Handlers map to 500. + const dbUser = await db + .selectFrom('branch.users') + .where('cognito_sub', '=', payload.sub) + .selectAll() + .executeTakeFirst(); + + if (!dbUser) { + console.warn( + 'User authenticated with Cognito but not found in database:', + payload.sub, + ); + return { isAuthenticated: false }; + } + + const user: AuthenticatedUser = { + cognitoSub: payload.sub, + userId: dbUser.user_id, + email: payload.email as string | undefined, + isAdmin: dbUser.is_admin === true, + // Informational only. We deliberately do NOT promote on a Cognito + // "Admins" group: branch.users.is_admin is the single source of truth. + // A second source would make demotion via PATCH /users/{userId} silently + // ineffective, nothing in this codebase writes group membership, and no + // aws_cognito_user_group is defined in infrastructure/aws/cognito.tf. + cognitoGroups: payload['cognito:groups'] as string[] | undefined, + }; + + return { user, isAuthenticated: true }; } diff --git a/shared/lambda-auth/test/authenticate.test.ts b/shared/lambda-auth/test/authenticate.test.ts index 2b9aa821..5de4b350 100644 --- a/shared/lambda-auth/test/authenticate.test.ts +++ b/shared/lambda-auth/test/authenticate.test.ts @@ -197,34 +197,36 @@ describe('authenticateRequest', () => { expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ clientId: null })); }); - it('degrades to unauthenticated (not a throw) when COGNITO_USER_POOL_ID is unset', async () => { - // This is why a missing env var manifests as blanket silent 401s across all - // six lambdas rather than a loud 500: getVerifier() throws inside the try. + it('throws (not a silent 401) when COGNITO_USER_POOL_ID is unset', async () => { + // Swallowing this gave blanket silent 401s across all six lambdas. delete process.env.COGNITO_USER_POOL_ID; const { authenticateRequest } = await loadModule(); const { db } = makeDb({ user_id: 7, is_admin: true }); - await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({ - isAuthenticated: false, - }); + await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow( + 'COGNITO_USER_POOL_ID', + ); expect(mockCreate).not.toHaveBeenCalled(); }); - it('returns unauthenticated when the database query throws', async () => { + it('propagates a database failure instead of reporting it as unauthenticated', async () => { + // Regression guard for the preview-env outage (PR #316). mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); const db = { selectFrom: () => ({ where: () => ({ selectAll: () => ({ - executeTakeFirst: jest.fn().mockRejectedValue(new Error('db down')), + executeTakeFirst: jest + .fn() + .mockRejectedValue(new Error('timeout exceeded when trying to connect')), }), }), }), }; - await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({ - isAuthenticated: false, - }); + await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow( + 'timeout exceeded when trying to connect', + ); }); });