fix(api): fail closed on missing upstash env in production - #506
Conversation
|
@subheeksh5599 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
Redoing this as inline file comments instead of a single review body.
|
|
||
| // production rate-limit guard ------------------------------------------------- | ||
|
|
||
| describe("production rate-limit guard", () => { |
There was a problem hiding this comment.
This new describe block runs inside a test file that imports middleware.js statically at the top of the file (line 7). The fix under review moves the production/missing-env check to module load time, so if this test file ever runs in a shell with VERCEL_ENV=production set but the Upstash credentials absent (vercel env pull, vercel build locally, or a future CI job mirroring the Vercel build environment), that top-level import throws and crashes the entire file, taking down all the pre-existing applyCors/rate-limit tests along with these new production-guard ones, not just failing one assertion.
Guard against the ambient environment instead of relying on it never having VERCEL_ENV=production set during a test run. Either unset VERCEL_ENV explicitly in a top-level beforeAll, or move the production-throw test into its own file that imports middleware.js dynamically inside the test body.
| process.env.VERCEL_ENV = savedEnv.VERCEL_ENV; | ||
| process.env.UPSTASH_REDIS_REST_URL = savedEnv.UPSTASH_REDIS_REST_URL; | ||
| process.env.UPSTASH_REDIS_REST_TOKEN = savedEnv.UPSTASH_REDIS_REST_TOKEN; | ||
| delete process.env.VERCEL_ENV; | ||
| delete process.env.UPSTASH_REDIS_REST_URL; | ||
| delete process.env.UPSTASH_REDIS_REST_TOKEN; |
There was a problem hiding this comment.
These lines assign each saved env var back and then immediately delete it, before the Object.entries(savedEnv).forEach on lines 72-75 does the real restoration. Nothing else executes in between, so the delete calls unconditionally wipe out whatever the assignments just set, and the forEach overwrites the result again right after. They have no effect on the final state and only obscure that the forEach is the actual restoration logic. Delete them.
Move the production-guard tests into their own file that imports middleware.js only dynamically, and add a vitest setup file that clears VERCEL_ENV/Upstash vars before test files load. Without this, any test file importing middleware.ts crashes at load when the shell has VERCEL_ENV=production and no Upstash credentials (e.g. after vercel env pull), taking down the pre-existing applyCors/rate-limit tests.
|
Addressed both review points in 74bccfb:
Verified: |
collinsezedike
left a comment
There was a problem hiding this comment.
Thank you for the fix, and for addressing both review comments cleanly, moving the production-guard tests into their own file with dynamic imports is a better solution than either fix I suggested. Approved.
Feel free to pick up another open issue whenever you're ready.
Summary
api/_lib/middleware.tsfalls back to a per-process in-memoryMaplimiter whenever the Upstash env vars are missing, with no signal. Across concurrent Vercel workers that fallback is effectively no limit at all, so a production deploy missingUPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKENsilently serves traffic without distributed rate limiting.This makes the module fail closed: at load time, if
VERCEL_ENV === "production"and the Upstash vars are absent, it throws instead of constructingratelimitasnull. Local dev (VERCEL_ENVunset) and preview deploys (VERCEL_ENV="preview") keep the in-memory fallback unchanged.Test plan
pnpm --filter @meridian/api test— 26 passed (4 new tests cover the production throw, production-with-vars load, and fallback kept for dev + preview)pnpm typecheck:apipassespnpm exec eslinton changed files — cleanpnpm exec prettier --check— cleanCloses #483