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', + ); }); });