feat(server): validate the webjs config block at boot - #1355
Conversation
…he server readAppBasePath in packages/cli/lib/doctor.js is a hand-maintained port of normalizeBasePath, kept as a port because doctor must run when the framework does not resolve from the app dir at all (#954). Nothing tested that the two agree, and base-path.js has already changed twice since the port landed. One input table now runs through both and asserts three-way against the expected value, so drift is a red test instead of a silent disagreement between what the UNMARKED_ASSET_LINKS check assumes and what the server serves.
The published JSON Schema reached users through exactly one wire, the scaffold's .vscode $ref, so a typo'd key was caught only for a VS Code user with package.json open. Everywhere else the key was dropped, the feature stayed at its default, and nothing said so, which is the exact gap the WebjsConfig docblock claims to close. The validator that already existed, trapped in a test file, is now packages/server/src/webjs-config-validate.js, and createRequestHandler runs it once per boot, so dev, prod, and an embedded host all get it from one call site. Every problem rides one aggregated warning. It warns and never throws. A typo costs one feature its setting; refusing to boot over a schema quibble costs the whole app, usually mid-deploy. The CLI's doctor.gate validation stays separate on purpose: it fails closed, runs where the server may not resolve, and checks DOCTOR_CODES membership the schema cannot express.
|
Decision: parts 1 and 2 shipped as one PR, not the two the issue sequenced #1300 planned three PRs. Part 3 landed on its own in #1351, which was right: it was the priority slice and it stood alone. Splitting the remaining two buys nothing I can see. They touch disjoint files (a test file plus two docblocks on one side, a new module plus a boot call site on the other), neither depends on the other, and the risk ordering the issue used, cheap wins first and the risky slice last, only matters when a later slice can invalidate an earlier one. Nothing here can. What it costs is a second review round on a diff that is small either way, which is the thing worth economising on. If part 2 turns out to be the wrong call, reverting this commit leaves the parity test in place, since they are separate commits. Why the validator warns instead of throwing Worth writing down, because it looks like the weaker choice. The typo it catches costs exactly one feature its setting. Throwing costs the whole app its boot, and it would do so at the worst moment, which is a deploy of a config change. Next made the same call for the same reason: unknown and invalid options warn and the boot continues, and only required or migrated options are fatal. WebJs has no required keys at all, since every one is optional with a default, so nothing in this block is even a candidate for the fatal bucket. The |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff. The mechanism is right and the two halves are cleanly separated, and I like that the port is kept and TESTED rather than deduped, which is the call the issue argued for and the one I would have made anyway.
The problem is what we tell people the boot check does. validateWebjsBlock type-checks enum, boolean, and integer leaves and nothing else, so of the 18 top-level schema properties, 8 get no value check at all: headers, redirects, allowedOrigins, basePath, csp, dev, start, doctor. An "allowedOrigins": "https://x.com" (a string where the schema wants an array) is dropped by readAllowedOrigins with no diagnostic and the new check stays quiet about it, which is the exact silent-default shape the feature exists to close. Three user-facing surfaces promise otherwise, and each of them carves out ONLY the nested-key limit, which reads as a guarantee that top-level values are fully checked. packages/server/AGENTS.md states the real scope correctly, so the internal and external surfaces now contradict each other.
I am not widening the validator for it. Depth was settled in the issue and the free-form headers / redirects / csp shapes are where a naive type check starts producing false positives. The docs are what should move.
The three user-facing surfaces promised a warning for "every value of the wrong type", which is more than the validator does. It checks enum membership and boolean / integer leaves, so 8 of the 18 top-level keys (headers, redirects, allowedOrigins, basePath, csp, dev, start, doctor) get no value check at all, and each of the three then carved out only the nested-key limit, which read as a promise that top-level values were covered. A wrong-shaped allowedOrigins is still dropped silently, which is the same failure the feature exists to close. Depth is not widening: those are the free-form shapes where a blunt check starts refusing configs that work. The docs move instead, and the promoted function's docblock drops a type check it never performed and a test-sufficiency argument that no longer describes its job.
Four things the previous commit got wrong, each checkable and each now checked against the schema and the readers. The schema has 17 top-level properties, not 18: 9 are checked (3 boolean, 5 integer, 1 enum) and 8 are not. The website list of what IS checked omitted maxMultipartBytes while reading as exhaustive. The bigger error was calling all 8 unchecked keys a silent drop. Only the KEY-level type is silent. A malformed entry inside a well-shaped headers or redirects array already warns from the reader, and a wrong-shaped doctor is never dropped at all, since the server does not read that key and webjs doctor exits 1 on it. Two of the three surfaces then contradicted themselves a sentence or two later, where they describe that same doctor exception.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the docs fix on its own and checked every claim it adds against the schema and the readers, since the whole point of that commit was to stop the docs saying more than the code does. It traded one overclaim for four smaller wrong ones.
The count is off by one: the schema has 17 top-level properties, not 18, and 9 of them are checked (3 boolean, 5 integer, 1 enum). More importantly, calling all 8 unchecked keys a silent drop is wrong in two places, and both of them contradict text sitting a sentence or two away. A malformed ENTRY inside a well-shaped headers or redirects array already warns from the reader, which is what the opening clause of that same built-ins paragraph says. And doctor is not dropped at all: the server never reads it, and webjs doctor exits 1 on a bad shape, which is the exception the very next sentence describes. What is genuinely unreported is only the KEY-level type, "headers": "x" and friends.
Fixed in 0690bfa7: correct count, maxMultipartBytes restored to the caught list, and all three surfaces now scope the gap to the key-level type and reconcile with the entry-level and doctor behaviour instead of talking past it.
Two rounds in a row went to claims about whether a given reader warns when it rejects a value, and both were wrong somewhere. It varies per reader and per branch inside one reader: compileHeaderRules warns only when Headers.set rejects a key, and drops a typo'd `source`, a bad URLPattern, and a non-object entry in silence; readAllowedOrigins filters bad entries with no warning at all; compileRedirectRules warns on every branch but one. So the docs stop asserting it. Each surface now states what the validator checks and what it does not, and leaves downstream behaviour to the reader that owns it. The sweeping "that key-level case is the whole of the gap" line is gone too, since it contradicted the nested-key sentence beside it. The built-ins lead sentence claiming every malformed entry warns was pre-existing and is corrected in passing, since this paragraph leaned on it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Checked the correction commit's own claims against the readers, and it is wrong in the same way the one before it was, just about different keys. That is the signal worth acting on: the problem is not the individual sentences, it is that I keep describing what OTHER modules do when this check stays quiet, and that claim needs exhaustive per-branch verification every time it is written.
Measured: compileHeaderRules warns on exactly one branch, when Headers.set rejects a key, and silently drops a typo'd source, an unparseable URLPattern, a non-object entry, and a rule whose directives all drop. readAllowedOrigins is a bare filter with no warning at all. compileRedirectRules warns on every branch except a non-object array element. So "a malformed entry already warns from the reader" is false for headers and allowedOrigins, and "that key-level case is the whole of what goes unreported" is false generally, and also contradicted the nested-key sentence sitting next to it.
Fixed in 934c680c by removing the class of claim rather than repairing the instances. Each surface now states what the validator checks and what it does not, and stops there. The built-ins.md lead sentence that all this leaned on was pre-existing and also untrue, so it is corrected in passing.
Four problems, all in prose added by this PR, all from explaining more than the check does. The cli AGENTS.md paragraph implied the boot check has an opinion about webjs.doctor. It has none: it validates top-level keys only and doctor's schema type is object, so nothing under it is ever inspected. The reasons the two validations coexist stand, but they are reasons the boot check would be wrong for the job even if it did descend. The d.ts called all 8 unchecked keys array, string, or object typed; csp is a oneOf with no type at all, which is the actual reason it escapes. The built-ins connective still read "the one exception" after the sentence it excepted was replaced. The validator docblock stated a rule and broke it in the next sentence, and carried a count of review rounds, which is session detail in a file that ships in the tarball.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth read, and every finding was again in prose this PR adds rather than in the code, which by now is the finding. The check itself has not moved since the first round.
Real ones, all fixed in 6a80c5e0. The cli AGENTS.md paragraph implied the boot check has an opinion about webjs.doctor; it has none, since it validates top-level keys only and doctor's schema type is object. The d.ts called all eight unchecked keys array, string, or object typed, but csp is a oneOf with no type, which is the actual reason it escapes. The built-ins.md connective still read "the one exception" after the sentence it excepted was replaced. And the validator docblock stated a rule then broke it in the next sentence, and carried a count of review rounds, which is session detail in a file that ships in the tarball.
One finding I am NOT fixing here. Three places state that a malformed webjs.redirects entry is dropped with a warning, and redirects.js:243 drops a non-object element silently, so the claim is wrong at website/app/docs/configuration/page.ts:108, webjs-config.schema.json's redirects description, and redirects.js:227. All three predate this PR and none is in a paragraph it touches. Deferred: it is a real bug in the docs and worth its own change, either fixing the prose or making that branch warn like its five siblings do, and folding a reader fix into a config-validation PR is how this diff grows a third concern.
Deferral ledger: 1. redirects non-object array element drops silently while three surfaces say it warns. Pre-existing, out of scope, awaiting a call on whether to file it.
The previous commit fixed the "array, string, and object shapes" claim in the d.ts and left it standing in built-ins.md and the docs page, so one commit shipped two answers for why csp escapes the boolean check. It has no schema type at all, being a oneOf, so no shape list can be right; all four surfaces now name the eight keys and say nothing about their types. Two leftover framings go with it. built-ins.md gave a false cause for the boot check ignoring doctor.gate (who reads the key, rather than the real reason, which is that the check never descends into any key). The docs page still called doctor "the one part of the block that is never merely dropped", written before this check existed and untrue now that a bad enum or boolean warns and is dropped too.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth read. Every finding was again in prose, and this time the substance of three of them was that the PREVIOUS round's fix was applied to two of the three surfaces carrying the same sentence. That is the useful signal: the claims were being repaired one file at a time by reading, which is how one gets missed.
Fixed in d72d8ad1, and I changed how it is checked rather than just fixing the two files. The four surfaces are now verified against the schema programmatically: the eight key names in each are diffed against the set the schema actually produces, and the array, string, and object claim is grepped for repo-wide. csp has no schema type at all, being a oneOf, so no shape list could ever be right, and none of the four makes one now.
Also gone: the false cause in built-ins.md for why the boot check ignores doctor.gate (who reads the key, rather than the real reason, that the check never descends into any key), and the docs page calling doctor "the one part of the block that is never merely dropped", which was written before this check existed and stopped being true when a bad enum started warning.
Deferral ledger, unchanged and still awaiting a call: 1. A non-object element in webjs.redirects is dropped silently while website/app/docs/configuration/page.ts:108, the schema's redirects description, and redirects.js:227 all say a malformed entry warns. Pre-existing, in paragraphs this PR does not touch.
Both surfaces listed the unknown-key report and the value checks as one list and then said "which is 9 of the 17 keys", which reads as though an unknown key is only caught for those 9. It is caught whatever it is called, since that check is membership against the schema rather than a per-key rule. The value checks are the ones bounded at 9, being trailingSlash's enum and the eight boolean / integer leaves.
Five of the six drop branches in compileRedirectRules call warnDrop. The non-object one did not, so `"redirects": ["/old"]`, which is what you write when you forget the object wrapper, vanished with nothing said. That contradicted four places at once: this function's own docblock, the schema's redirects description, the configuration docs page, and the built-ins reference, all of which state that a malformed entry is dropped with a one-line warning. Found while auditing those exact claims for the boot-validation docs in this PR.
compileHeaderRules had one warning path, the Headers.set probe, and five silent ones. A `source` misspelled as `sources`, a non-object rule, a non-array headers list, an unparseable pattern, and a malformed directive all produced no rule and no diagnostic, which is precisely the typo class this PR's boot check exists to end, in the reader sitting next to it. Now every drop goes through a warnDrop shaped like the one in redirects.js, and the docblock records it. The posture is unchanged: a bad entry is still dropped rather than thrown, so the valid rules around it keep working.
…ctives Two defects from the warning pass that landed in #1355, both in headers.js. `{ source: "/x", headers: [] }` is schema-valid (no minItems) and the boot config check does not descend into headers, so the rule was dropped with nothing anywhere saying so. That is the silent-config failure the warnings were added to end, four lines under a docblock claiming it could not happen. A rule whose every directive was dropped hit the same branch: the per-directive warnings named the directives and never said the rule went with them. The warnDrop helper also sat between compileHeaderRules' JSDoc and its declaration, so the JSDoc bound to the helper and the reader lost its @param, its @returns, the shape example, and the value:null contract. redirects.js puts its helper after the function, which is why it was fine; this now matches, with a comment recording why the order matters.
…ctives (#1362) * fix(server): report the webjs.headers rule dropped for having no directives Two defects from the warning pass that landed in #1355, both in headers.js. `{ source: "/x", headers: [] }` is schema-valid (no minItems) and the boot config check does not descend into headers, so the rule was dropped with nothing anywhere saying so. That is the silent-config failure the warnings were added to end, four lines under a docblock claiming it could not happen. A rule whose every directive was dropped hit the same branch: the per-directive warnings named the directives and never said the rule went with them. The warnDrop helper also sat between compileHeaderRules' JSDoc and its declaration, so the JSDoc bound to the helper and the reader lost its @param, its @returns, the shape example, and the value:null contract. redirects.js puts its helper after the function, which is why it was fine; this now matches, with a comment recording why the order matters. * fix(server): warn when webjs.headers or webjs.redirects is not an array Both readers discarded the ENTIRE config in silence when the key was present but not an array. The schema types both keys `array` and the boot config check inspects only boolean / integer / enum leaves, so a `"headers": {}` was reported by nothing anywhere, which is the same silent-config failure the rule-level and directive-level warnings just fixed one level down. An absent key stays silent, since that is the default rather than a mistake, and it is asserted so an app without either key never warns on boot. The warnDrop docblock claimed every drop branch went through it while this one did not. It now names all three levels and the absent-key exception. * fix(server): decide config-key presence from the key, not an && chain The presence test read `raw` off the `&&` chain that walks `pkg.webjs.<key>`. A chain short-circuits to whatever link FAILED, so for a non-object `pkg` `raw` was the boolean `false` from `typeof pkg === 'object'`, and for a falsy config block it was that block. Six shapes therefore warned about a `headers` or `redirects` key nobody wrote, and printed a value not in the config, in a change whose whole point is diagnostic accuracy. It also contradicted `webjs-config-validate.js`, which returns `[]` for a non-object config block on the stated grounds that the readers treat it as unconfigured. The block is now read explicitly and presence tested with `in`, so an explicit null still warns (the author wrote the key) while every absent shape stays silent. The old tests covered only null, the one value the chain passes through unchanged, which is why this sat under a green suite. Also renames the config-level message from "entry" to "config", since an entry is an array element, and documents the behaviour on the three doc surfaces that describe these keys. * docs: correct the drop-level counts for redirects Two doc claims overstated the symmetry between the readers. A redirect entry carries no directives, so redirects has TWO drop levels (the key and the entry) where headers has three (the key, the rule, the directive). The built-ins reference said "all three levels" of both, and the server module table said headers mirrors redirects exactly when only the presence test does.
1 participant
Closes #1300.
The last two slices of that issue, shipped together rather than as two sequenced PRs, because they touch disjoint files and each is small enough that a second review round would cost more than it caught. Part 3 already landed in #1351.
Part 1. Base-path normalizer parity
readAppBasePath(packages/cli/lib/doctor.js) is a hand-maintained port of the server'snormalizeBasePath(packages/server/src/base-path.js). The port is deliberate and stays: doctor must run when@webjsdev/serverdoes not resolve from the app dir at all, which is the #954 fresh-worktree case doctor exists to diagnose, and that helper is not on the server's public surface either way.What was missing is any signal when the two drift.
base-path.jshas changed twice since the port landed, and a divergence would mean theUNMARKED_ASSET_LINKScheck silently assumes a different base path than the server serves.test/cli/base-path-parity.test.mjsruns one input table through both and asserts three-way: CLI port equals server reader equals the expected value. Equality alone would pass if both drifted the same way, and the expected column alone would not prove they agree.Part 2. The config block is validated at boot
webjs-config.schema.jsonreached users through exactly one wire, the scaffold's.vscode/settings.json$ref, so a typo'd key was caught only for a VS Code user withpackage.jsonopen. Everywhere else the key was dropped, the feature stayed at its default, and nothing said so. That is the exact gap theWebjsConfigdocblock claims to close.The validator that already existed (trapped inside
packages/server/test/config/webjs-config-schema.test.js) is nowpackages/server/src/webjs-config-validate.js, andcreateRequestHandlerruns it once per boot. One aggregated warning names every unknown key and bad value.It warns and never throws. A typo costs one feature its setting; a hard boot failure over a schema quibble costs the whole app, on a deploy. That is Next's posture too (unknown and invalid options warn, only required or migrated ones are fatal), and WebJs has no required keys at all.
readDoctorPolicy's hand-writtendoctor.gatevalidation stays and is untouched. It runs in the CLI where the server may not resolve, checks gate keys against the realDOCTOR_CODESset (membership the schema cannot express), and fails CLOSED, because a silently-ignored gate leaves CI un-gated while looking gated. One function cannot honour both failure modes, and the new module's docblock says so, so the next reader does not clean up the apparent duplication.Part 3. Two readers made to say when they drop something
Auditing the docs claims above turned up two readers contradicting what four surfaces say about them, so they are fixed here rather than deferred.
compileRedirectRuleswarned on five of its six drop branches. The non-object one, which is what"redirects": ["/old"]hits when you forget the object wrapper, dropped in silence, contradicting its own docblock, the schema'sredirectsdescription, the configuration docs page, and the built-ins reference, all of which promise a one-line warning.compileHeaderRuleswas worse: one warning path and five silent ones, so asourcemisspelled assourcesproduced no rule and no diagnostic at all. That is the same typo class the boot check in part 2 exists to end, in the reader sitting right next to it.Both now route every drop through a
warnDropof the same shape. The posture does not change: a bad entry is still dropped rather than thrown, so the valid rules around it keep working.One correction to the issue's plan
The issue's parity table expects
"///app"to normalize to/appunder a "leading slashes collapsed" branch. It does not, on either side: the//hostguard fires on the//prefix before the collapse can run, so the value fails safe to''like any other network-path reference. Both implementations agree, which is what the test asserts. The table row was wrong, and the collapse is in fact unreachable for more than one leading slash. Noted in a comment on that row.Deliberately not here
Everything the issue's
## Out of scopelists. In particular: no dedupe of the base-path port, no descent into nested config objects (webjs.dev.beforeestays uncaught, since the free-formheaders/redirects/cspshapes make that a behaviour change of its own), no throw and nowebjs checkrule, no new config key, and no validation dependency.Test plan
test/cli/base-path-parity.test.mjs(new): 29 pass. 27 table rows plus the two file-level branches only the CLI port has (a missing and an unparseablepackage.json).packages/server/test/config/webjs-config-validate.test.js(new): 10 pass. Six unit cases overvalidateAppWebjsConfig, four boot cases throughcreateRequestHandlerwith an injected logger.packages/server/test/config/webjs-config-schema.test.js: 10 pass, assertions byte-identical against the imported validator, which is the proof the move was faithful.1185a596: move the//hostguard below the leading-slash collapse inpackages/server/src/base-path.jsand 3 parity rows red; do the same inpackages/cli/lib/doctor.jsand the same 3 red; drop thetrim()from the CLI port and the whitespace-padded row reds.test/bun/webjs-config-validate.mjs(new) asserts the warn-once and boot-completes behaviour, green undernode(26.7.0) andbun(1.3.14), with its own CI step alongside the other listener proofs. Owed because the check sits insidecreateRequestHandler, the boot path both the node:http andBun.serveshells go through.packages/server/test/redirects/redirects.test.jsandpackages/server/test/headers/headers.test.js: one new case each, asserting every bad entry warns AND that the valid sibling still compiles. Counterfactual: revert theredirects.jsguard to its parent state and the new case reds.npm test: exits 0. Two assertions intest/scaffolds/gallery-coverage.test.jsfail, and they fail identically onmainin the primary checkout, so they are not from this diff.webjs checkpasses andwebjs doctorexits 0 in bothexamples/blogandwebsite.webjs typecheckpasses inwebsite, which is the app whose docs page this edits.websiteboots in prod mode through the BRANCH's ownpackages/server/index.jsand serves 200 on/,/docs/configuration,/ui, and/ui/button, with 7, 12, 12, and 50 modulepreload hints and none broken. The blog is not honestly checkable that way from a linked worktree: its.server.tsfiles resolve@webjsdev/serverthrough the linked root to the PRIMARY checkout, so loading the branch copy in the same process gives two module graphs and twoAsyncLocalStorageinstances, andcookies()throws "called outside a request scope" before any of this diff runs. Through one consistent graph it serves 200. Instead, the validator was run directly against both apps' real package.json blocks and returns[]for each, so neither in-repo app gains a warning.How the docs claims are checked
The review found the same class of defect five rounds running, always in prose rather than in code: a sentence asserting something about the schema or about what a reader does with a value the check passes over. Each one needed per-branch verification, and reading was not producing it.
So the surviving claims are now the ones a script can check, and they are checked that way. The eight unchecked key names in each of the four surfaces are diffed against the set the schema actually produces (
9 of 17checked, the other 8 named), rather than transcribed by hand. Nothing in any surface characterises what a reader does downstream, which is where every wrong sentence came from: whether a reader warns on a value it rejects varies per reader and per branch inside one reader, so any summary of it is wrong somewhere almost at once.Docs
packages/core/src/webjs-config.d.ts, the "close that gap" claim, now accurate about which of the three mechanisms fires when..agents/skills/webjs/references/built-ins.md, the config-block section.website/app/docs/configuration/page.ts, a new section on boot-time reporting.packages/server/AGENTS.md, the module table.packages/cli/AGENTS.md, the "editor-only, so it can never be the enforcement" sentence in the doctor severity gate paragraph, and whyreadDoctorPolicystill coexists.