refactor(env): validate every environment variable at boot - #77
Open
sorfeb wants to merge 1 commit into
Open
Conversation
Twelve variables were read across ten files with no single declaration of what the application needs. A missing one surfaced as a TRPCError inside a serverless function in front of a visitor, and only for whichever feature happened to be opened first. `src/env.ts` now parses them all once at startup and names everything that is wrong in one message. Zod rather than @t3-oss/env-nextjs. t3-env's hard parts are all about the client/server boundary: the schema split by isServer, the access Proxy, runtimeEnv mapping, and the non-obvious ignoreProp exemption for __esModule and $$typeof that stops React and bundlers tripping the guard. None of that is load-bearing here, because no NEXT_PUBLIC_ variable is read by our own code. What does apply is empty-string handling and a build-time bypass, which is four lines, and zod is already a dependency. Blank has to become absent before validation rather than be rejected during it. Vercel's UI accepts an empty value and z.string() treats '' as a valid string, so a blank secret would pass straight through. Adding .min(1) catches that, but then a blank optional variable fails too: '' is present-but-invalid rather than missing, so leaving OWNER_USER_ID empty would stop the whole application booting over a variable that is allowed to be unset. Required versus optional is deliberate. Required means the site cannot function at all: DATABASE_URL, both GitHub OAuth credentials, and BETTER_AUTH_SECRET. Optional means one feature stops: Spotify, Last.fm, Cloudinary, OWNER_USER_ID. Promoting those would turn a missing Last.fm key into a total outage, trading a small failure for a large one, so they keep throwing at the point of use and only gain a typed accessor. BETTER_AUTH_SECRET and NEXT_PUBLIC_CLOUDINARY_CLOUD_NAME are declared even though no code reads them: Better Auth and next-cloudinary read them internally. A variable consumed inside a dependency is invisible to a grep of src/, which is exactly the kind that goes missing unnoticed. db.ts loses 102 lines and gains 30: the hand-rolled DATABASE_URL validation, the exported testDatabaseConnection and disconnectDatabase that nothing called, and a catch that flattened the real error into "Database connection failed: ..." and discarded the stack. auth.ts loses its two process.env.X! non-null assertions, which told the compiler a value existed without checking. NODE_ENV stays on process.env because Next inlines it and layout.tsx reads it client-side. TRPCProvider.tsx keeps reading VERCEL_URL and PORT directly because it carries 'use client'; importing the contract there would trip its server-only guard. Verified with eight behaviour cases, each in its own process because env.ts validates at import and a query string does not bust Node's ESM cache: missing required names the variable, a non-postgres URL is rejected, a blank optional is absent rather than invalid, a blank required still fails, the skip flag bypasses, and an empty environment reports all four required variables at once rather than one per run. Then built and served against the production database: chat transcript, rooms.list, get-session, /media, /photos and /chatroom all 200. Refs SOR-165 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Why
Twelve variables read across ten files, with no single declaration of what the app needs. A missing one surfaced as a
TRPCErrorinside a serverless function in front of a visitor, and only for whichever feature happened to be opened first.Concretely, before this:
process.env.GITHUB_CLIENT_ID!andGITHUB_CLIENT_SECRET!inauth.ts— non-null assertions telling the compiler a value exists without checkingdb.tsfor one variabletestDatabaseConnectionanddisconnectDatabase— exported, called from nowherecatchthat flattened the real error intoDatabase connection failed: ...and discarded the stackZod, not @t3-oss/env-nextjs
t3-env's hard parts are all about the client/server boundary — the schema split by
isServer, the accessProxy,runtimeEnvmapping, and this, which you would never predict:React probes
$$typeof, bundlers probe__esModule; without the exemption a hand-rolled guard throws on internal machinery. That is a real argument for the library — and it is entirely inside the half we are not building, because noNEXT_PUBLIC_variable is read by our own code.What does apply is empty-string handling and a build bypass: four lines.
zodis already a direct dependency.The subtlety worth reviewing
Blank must become absent before validation, not be rejected during it.
withoutBlanks()deletes empty keys first, so blank and unset mean the same thing.Required vs optional is deliberate
Required = the site cannot function:
DATABASE_URL, both GitHub credentials,BETTER_AUTH_SECRET.Optional = one feature stops: Spotify, Last.fm, Cloudinary,
OWNER_USER_ID.Promoting the optional ones would turn a missing Last.fm key into a total outage — trading a small failure for a large one. They keep throwing at the point of use and only gain a typed accessor.
Two variables declared that no code reads
BETTER_AUTH_SECRETandNEXT_PUBLIC_CLOUDINARY_CLOUD_NAMEare consumed inside Better Auth and next-cloudinary. A variable read inside a dependency is invisible to a grep ofsrc/— I got this wrong earlier in review, calling the Cloudinary one an unused leftover when/mediaand/photosdepend on it in production. Declaring them makes them visible.Deliberately untouched
NODE_ENV— Next inlines it andlayout.tsxreads it client-sideTRPCProvider.tsx— carries'use client', so importing the contract would trip its server-only guard. Keeps readingVERCEL_URL/PORTdirectly, already guarded bytypeof window !== 'undefined'Evidence
Eight behaviour cases, each in its own process —
env.tsvalidates at import, and a?queryon afile://URL does not bust Node's ESM cache. My first harness got this wrong and reported five false failures.Then built and served against the production database, reads only:
messages.listByRoomrooms.list/api/auth/get-session!assertions)/media,/photos/chatroomnpm run compileclean ·npm run lint0 errors ·lint:useeffect0 unapproved ·lint:cssunchanged at 1136 warnings, no CSS touched.Net:
db.ts−102/+30.Closes SOR-165
🤖 Generated with Claude Code