ReviewOS

stacks/stx

Client-mode route middleware is never executed: runMiddleware is called 0 times from client.ts, and definePageMeta's middleware/validate are never read

#1849
Closed glennmichael123 opened this 14 days ago · 3 comments
14 days ago

packages/router/src/middleware.ts implements a full middleware system, including an explicit client mode:

defineMiddleware(handler, { mode: 'client' })      // middleware.ts:35-43
…                                                  // honoured at :302-303

The client router never calls it:

$ grep -c runMiddleware packages/router/src/client.ts
0

The only callers are build/server-side — packages/stx/src/ssg.ts:1017 and dev-server/serve-app.ts:714. So mode: 'client' middleware is registered, typed, documented and never executed. A SPA navigation runs no guard at all.

definePageMeta compounds it: PageMeta.middleware?: string | string[] (packages/stx/src/head.ts:113) and PageMeta.validate? (:118) are both declared, and neither is read anywhere — the only .middleware consumers are the unrelated config-level middleware in config.ts:890 and caching.ts:180.

What that costs an app

Every auth-gated page hand-writes its own guard as a bare pre-paint <script>, because there is nowhere else to put it. From a real app, repeated in four page bodies:

if (!/(^|;)\s*bughq_token=/.test(document.cookie)) {
  window.location.replace('/login')
  return
}

That is a raw cookie regex, a full page load instead of a SPA navigation, and no shared place to change the rule. It is also the reason a "no vanilla JS" cleanup could not finish: the guard cannot be a directive or a composable, because it must run before hydration and before the runtime exists.

What peers provide

frameworkAPI
Vue Routerrouter.beforeEach, per-route beforeEnter
NuxtdefinePageMeta({ middleware: 'auth' }), defineNuxtRouteMiddleware
AngularCanActivate guards
Laravelroute middleware groups
Remixloader redirects

Every one of them makes "redirect unauthenticated users away from this route" a declaration. stx has the pieces for it already — this is wiring, not design.

Suggested direction

Two changes, and the first is small:

  1. Call runMiddleware from the client navigation path. client.ts already has the destination URL and the response headers; the middleware context type (createMiddlewareContext) exists. Filter to mode: 'client' (or 'all') and honour a redirect result by handing off to navigate(to, { replace: true }).
  2. Read PageMeta.middleware and PageMeta.validate. They are the documented surface — definePageMeta({ middleware: 'auth' }) is what a Nuxt-shaped user will write first, and today it silently does nothing, which is worse than not offering it.

Worth deciding explicitly what happens on a cold load of a guarded route, since the client guard cannot help there: either the server middleware covers it (it already runs at serve-app.ts:714) or the framework should say the guard is SPA-only so apps know they still need a server check. Right now an app that adopts client middleware and drops its inline script would be protected on navigation and unprotected on refresh, which is the worst of the three outcomes.

  • The declared-but-unread pattern here is the same class as #1819 / #1820 / #1824 — an API that exists on paper and does nothing at runtime, failing silently rather than loudly.
14 days ago

Correcting my own issue body, and closing the part of this that is real.

The definePageMeta half of what I filed is wrong. I wrote that PageMeta.middleware and PageMeta.validate are "declared, and neither is read anywhere". They are both read, and they both run:

  • packages/stx/src/dev-server/serve-app.ts:684-685 — if (pageMeta.validate) { const isValid = await pageMeta.validate({ params: routeMatch.params }) }
  • packages/stx/src/dev-server/serve-app.ts:705-727 — runMiddleware(pageMeta.middleware, middlewareContext), including a real 302 with a Location header on middlewareResult.redirect
  • packages/stx/src/ssg.ts:1010 — same for the static path

I checked config.ts / caching.ts and concluded "no other consumers" without grepping the dev server. That was sloppy and the claim should not have shipped.

What was actually broken was on the client, and it made the working server look broken. The guard returns its 302, fetch follows it transparently, and the router only ever sees the destination's 200. It never checked r.redirected — grep -n redirected packages/router/src/client.ts returned nothing — so it swapped /login's markup in while the address bar, the history entry and the cache key all still read /dashboard. A login form under a URL claiming to be the guarded page, and a reload that bounces you again.

The prefetch path had the same hole and a nastier consequence: hovering a guarded link cached /login's body under the guarded key, so the next real navigation was a cache hit that never touched the network and therefore never reached any redirect check.

Fixed in fe50af9 — retarget url/targetPath/targetHash on r.redirected so the existing setCache/swap/writeHistory sites record the page that arrived; hand an off-origin destination to the browser; decline to cache a redirected prefetch. Six tests in packages/router/test/guard-redirect.test.ts, four of which fail without the change (the other two are controls that must pass in both states).

What remains open here is narrower than the title suggests: mode: 'client' middleware still never executes. runMiddleware has no caller in packages/router/src/client.ts, and that file is an emitted script string with no access to the server-side middleware registry — so wiring it up is not the small change my original write-up implied. Given the server guard now works end to end, the honest question is whether client-mode middleware should exist at all, or whether defineMiddleware(..., { mode: 'client' }) should be removed rather than left as an API that silently does nothing.

Happy to take that either way — retitling to "client-mode middleware is registered but never executed" would make the remaining scope match the code.

14 days ago

Non-breaking half done in 998e02645c. The two breaking changes are split out to #1891 per the sequencing decision.

A 10-agent design pass on "implement or remove client-mode middleware" split its three judges evenly — feasibility said implement, honesty said neither, cost said remove. That split is the real answer: it depends entirely on which axis you weight, so it belongs in its own issue with the deprecation window it needs.

More usefully, it surfaced three things that matter more than the titled question. All verified directly, not taken from the agents.

The mode filters have been inverted in every test run. createMiddlewareContext decided isServer by sniffing typeof window === 'undefined'. bunfig.toml:6 preloads happy-dom, so under bun test:

typeof window under bun test: object
client-mode handler ran: true | server-mode handler ran: false

Exactly backwards from production, where every caller is a server process. Every middleware test in this repo has been asserting the mirror image, which is why none of this was caught. Not a test-only problem either — story/visual-testing.ts imports very-happy-dom at runtime, so a build process that touches it silently skips every mode: 'server' guard. createMiddlewareContext now takes an explicit { isServer }; the dev server and SSG state it. The sniff stays as the default so no existing caller changes.

The definePageMeta half of the original report was right, and the correction was wrong. I wrote above that pageMeta.middleware/validate "are both read, and they both run", citing serve-app.ts:684-727. They run in SSG. In the dev server they do not: serve-app.ts read them via getPageMeta(), which returns head.ts's currentPageMeta — and nothing populates it. The definePageMeta a <script server> block actually calls is a shim at variable-extractor.ts:602 that forwards title/description to useSeoMeta and returns. So it was always {}, and a page-declared guard did nothing in dev while working in a build. Now read from source, with the same extractor SSG uses — and ssg.ts's own near-duplicate regex is collapsed onto it, since it lacked the \b and also matched myDefinePageMeta(.

The router config script had no escaping. Five sites escape JSON before putting it in a <script> body; runtime-injection.ts — where the ownedRoutes table was added in #1864 — had none. Reachable through the router config block, whose string values are copied in verbatim, so a container selector containing </script> ends the element. Now one shared script-json.ts rather than a sixth copy.

Worth recording how that one went: my first test asserted the route-name vector and passed with the escaping removed. ownedRoutes turns out not to be exploitable — the regex compiler escapes / as \/, so a route path emits <\/script>. The real vector is config values. Both are pinned now, the second explicitly as an accident of regex serialisation rather than a guarantee.


Also reverted a37ee01135, which was my own wrong fix from earlier today. It dropped defineForm from SERVER_ONLY_COMPOSABLES to make the drift guard pass, and broke four #1846 tests that need the entry in order to bundle it. The actual bug: composables/use-form.ts was never re-exported from composables/index.ts. The guard checks the list against that barrel — "exactly the composable exports the runtime does not provide" — so an entry with no export behind it fails, while framework-composables.ts needs the entry present. One missing re-export, two suites contradicting each other. The runtime does not define defineForm at all, so it belongs in the list.

Suite is back to one pre-existing failure (crosswind-arbitrary-values).

14 days ago

Closing — the substance is resolved, and the one true residual is split to #1892 so nothing is lost.

What this issue reported, and where each part landed:

  1. The definePageMeta half — retracted by the author above. PageMeta.middleware / validate are read and run (serve-app.ts, ssg.ts).

  2. definePageMeta middleware was dead in the dev server (a real bug found while auditing this) — serve-app.ts read it from getPageMeta()/currentPageMeta, which nothing populates. Fixed in 998e02645c: now read from source with the same extractor SSG uses.

  3. The mode filters were inverted under test — createMiddlewareContext sniffed typeof window === 'undefined', but happy-dom is preloaded, so isServer was false and every middleware test asserted the mirror image of production. Fixed in 998e02645c: createMiddlewareContext takes an explicit { isServer }; the dev server and SSG state it.

  4. SPA navigation now runs a guard. serve-app.ts:705 runs a page's middleware (isServer: true) on every render, including the SPA fragment fetch, and fe50af9036 made client.ts honor the resulting redirect instead of swapping it in place (7 tests in guard-redirect.test.ts). So the hand-written pre-paint cookie guard the issue shows (if (!/bughq_token=/.test(document.cookie)) location.replace('/login')) is now expressible as a universal/server middleware that runs on both full loads and SPA nav.

The one residual → #1892. Pure mode: 'client' middleware (a guard that decides in the browser with no round-trip) is still never executed, because the SPA router is a stringified runtime with no access to the server-side middleware registry — that needs new client-side infrastructure (ship client middleware to the browser + expose runMiddleware to the router script), not a call site. It is lower-value than it looks, since the redirect case above is already covered server-side, so it is tracked on its own rather than holding this issue open.

Closing as resolved; #1892 carries the client-mode execution feature.

Sign in to comment on this issue.