ReviewOS

stacks/stx

Route guards fail open on an unknown name, and mode: 'client' can never run

#1891
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

Split out of #1849, which fixed the non-breaking half in 998e02645c. These two are behaviour changes to published packages (@stacksjs/stx / stx-router 0.2.155) and were deliberately deferred so they can land in a deliberate release with a deprecation window.

1. A guard that cannot be found lets the request through

packages/router/src/middleware.ts:297-300:

const middleware = getMiddleware(name)
if (!middleware) {
  console.warn(`Middleware '${name}' not found`)
  continue          // <- chain continues, result is { passed: true }
}

Verified:

result.passed for a typo'd guard name: true

So definePageMeta({ middleware: ['ath'] }) — one transposed character — renders the page to anyone. The only signal is a console.warn on the server. For a mechanism whose entire job is to deny access, fail-open is the wrong default: a typo silently removes the protection rather than breaking loudly.

Note this is quieter than a missing guard, because the page still looks protected in the source.

Ask: an unresolved middleware name aborts the chain (500) instead of passing. Deprecation path: warn loudly for one minor, then flip.

Known breakage: an app that references a conditionally-registered middleware name currently gets a silent pass and would start getting a 500. That is arguably the bug being fixed, but it is a real behaviour change.

2. mode: 'client' is registered, typed, documented, and can never run

defineMiddleware(handler, { mode: 'client' }) round-trips the field faithfully, and middleware.ts:303 skips it whenever isServer. Every caller of runMiddleware is a server process (ssg.ts:1101, dev-server/serve-app.ts:714), and packages/router/src/client.ts contains zero references to middleware — it is an emitted template-literal string with no imports and no access to the registry. The registry itself holds live closures in a module-level Map and is loaded via fs.readdirSync + await import(), so it cannot cross into a browser.

So the API cannot fire, in any configuration.

examples/drivly/middleware/auth.ts is someone reaching for it and getting nothing — its docstring says "This is a client-side check — server middleware runs at build time with no request context." (That file has a second, separate bug: it calls defineMiddleware('auth', fn), passing a string where the handler goes, so handler is 'auth'. Worth fixing regardless.)

Ask: decide and act, rather than leaving it half-true. A three-agent design pass split evenly, which is itself the finding — the answer depends on the axis:

  • remove — smallest change that removes the lie; MiddlewareMode narrows to 'universal' | 'server'
  • implement — feasible: composable-loader.ts and store-loader.ts already ship live functions to the browser by shipping source text and re-creating closures there, so a middleware/ bundle at /_stx/middleware.js is not novel
  • fail loudly — registering client-mode middleware becomes a startup error

My recommendation is not to implement. A guard that runs in the browser is not a security boundary, so implementing it would replace an API that visibly does nothing with one that looks like protection — a worse failure than the current one. If client-side guards are wanted, they should be framed as UX (avoiding a flash of protected content) and named so nobody mistakes them for enforcement.

Whichever way it goes, routeMiddleware.defaultMode (config-types.ts:287) is declared and never read, and should go with it.

Test-environment caveat for whoever picks this up

Fixed in 998e02645c, but worth knowing: bunfig.toml preloads happy-dom, so typeof window !== 'undefined' in every test run. Before that commit the mode filters ran inverted under test — mode: 'client' executed and mode: 'server' did not. Any test written here must state isServer explicitly via createMiddlewareContext(to, from, request, { isServer: true }) rather than relying on the sniff, or it will assert the mirror image of production. packages/stx/test/route-middleware/server-detection.test.ts pins this.

Related: #1849

14 days ago

Both parts shipped in a79aac7585 — closing.

1. Guards fail closed. runMiddleware did if (!middleware) { console.warn(...); continue }, so definePageMeta({ middleware: ['ath'] }) — one transposed character — passed the chain and rendered the page to everyone. An unresolved name now aborts with 500 (the same shape a throwing handler already produced), so a typo breaks loudly instead of silently dropping the protection. Two tests that pinned the old fail-open (result.passed → true for a missing guard) are flipped, and a chain test proves one bad name fails the whole chain even after an earlier guard ran.

2. mode: 'client' removed. MiddlewareMode narrows to 'universal' | 'server'; the dead client-skip branch in runMiddleware and the never-read routeMiddleware.defaultMode config go with it. This follows the issue's own recommendation — remove, not implement: the SPA router is a stringified runtime with no access to the registry and every runMiddleware caller is a server process, so a client-mode guard could never fire, and shipping one would be an API that looks like a browser-side security boundary while doing nothing. Browser-side UX guards, if wanted, belong in a separately named non-enforcement API.

Also fixed examples/drivly/middleware/auth.ts — it called defineMiddleware('auth', fn), a string where the handler goes — and reframed its docstring toward the server cookie path.

Per the test-environment caveat: the new/updated tests state isServer explicitly via createMiddlewareContext(to, from, request, { isServer: true }) rather than relying on the sniff. 64 middleware tests pass; router dist rebuilt (gitignored, CI builds it).

Note this is a breaking change (fix(router)!): a typo'd guard now 500s instead of passing, and the mode: 'client' type is gone — both intended.

Sign in to comment on this issue.