ReviewOS

stacks/stx

fix(router): `interceptAllLinks` claims every same-origin anchor, including paths the router does not own — an OAuth redirect gets hit twice

#1864
Closed glennmichael123 opened this 14 days ago · 2 comments
14 days ago

Summary

With interceptAllLinks: true — which bun-plugin-stx's serve() hardcodes as its base default — the SPA router intercepts every same-origin relative <a href>, whether or not that path is a page the router can render. shouldIntercept() has no notion of a route table: it filters on protocol, target and a handful of opt-out attributes only. Clicking a link to a non-page endpoint (an OAuth redirect, a file download handler, a proxied docs site) makes the router fetch() it, fail to swap it, and then fall back to a real navigation — so the endpoint is hit twice. If the endpoint instead returns a successful cross-origin redirect, fetch follows it and the router swaps a third-party document into the container.

The only opt-out, data-no-router, removes the anchor from routing entirely, so it cannot be used to say "this is a page, just do not treat it as a fragment".

Impact (real app)

In analyticshq the four OAuth sign-in anchors carry data-no-router (resources/views/login.stx:105, :109; resources/views/register.stx:124, :128). The network trace recorded before that fix:

Fetch     503  /api/auth/github/redirect   ← router intercepted and fetched it
Document  503  /api/auth/github/redirect   ← fallback real navigation

Two hits on an endpoint whose entire job is to mint OAuth state.

Separately, resources/partials/site-nav.stx:53-55 keeps the opt-out on a path the router genuinely cannot render, and says so:

/docs is served by the BunPress proxy, not by stx. A fragment fetch would get a document the router cannot swap, so this one keeps the opt-out.

That is the same missing concept twice: the router has no manifest of which same-origin paths it actually owns. There are 17 data-no-router attributes in resources/ today, and every new link has to remember whether it needs one.

Evidence in stx

packages/router/src/client.ts:1211-1221

function shouldIntercept(link){
  if(!link)return false;
  var href=link.getAttribute('href');
  if(!href)return false;
  if(href.startsWith('http')||href.startsWith('#')||href.startsWith('mailto:')||href.startsWith('tel:')||href.startsWith('javascript:'))return false;
  if(link.target==='_blank')return false;
  if(link.hasAttribute('data-stx-no-router')||link.hasAttribute('data-no-router')||link.hasAttribute('download'))return false;
  if(href.startsWith('?'))return true;
  if(href===location.pathname)return false;
  if(!getContainer())return false;
  return true;
}

No route-table consultation, no method/idempotency notion, no /api exemption.

  • client.ts:1229-1244 — capture-phase click handler; with o.interceptAllLinks it upgrades any a[href] that passes shouldIntercept.
  • client.ts:554-555 — fetch(url, …).then(r => { if(!r.ok) throw new Error(r.status) … }); the rejection path ends in location.href = url, producing the second request.
  • client.ts:495 — a cross-origin target is handled correctly when it is the anchor's own href, but a same-origin URL that 302s off-site is followed by fetch and the resulting body is swapped in.
  • packages/bun-plugin/src/serve.ts:2302 — interceptAllLinks: true is the base of the merged router config on the serve path.

(For the record: hover prefetch is not implicated — client.ts:1254-1258 only prefetches [data-stx-link], so a plain anchor is never speculatively fetched.)

Prior art

  • Nuxt: <NuxtLink> only routes paths that resolve in the route manifest; anything else falls through to a native navigation. prefetch="false" is separate from external.
  • Next.js: <Link> routes against the app router's known segments; an unknown path is a plain anchor.
  • Turbo: data-turbo="false" (routing) and data-turbo-prefetch="false" (prefetch) are independent attributes, and Turbo Drive only claims same-origin URLs within its configured root.

Ask

  1. Give the router a manifest of the paths it can render (the server already has one — it is what discoverFiles() builds) and have shouldIntercept consult it, so an unmatched same-origin path falls through to a native navigation instead of being fetched and then re-navigated.
  2. Failing (1), exempt paths the app declares as non-page — e.g. a router.ignore: ['/api/**', '/docs/**'] config array merged into window.__stxRouterConfig.
  3. Treat a fetch whose response.redirected is true and whose final URL is cross-origin as "not ours": abort the swap and do a real navigation, rather than injecting a third-party document into the container.
  4. Split the opt-outs: keep data-no-router for "never route this", and add a separate per-link prefetch control so the two decisions are independent.

Related: #1719, #1777

14 days ago

Fixed in a760f9b.

The click path is what the issue describes, and the cause is ordering: the handler matched [data-stx-link] first and only consulted shouldIntercept() on the interceptAllLinks fallback, so a marked link never saw the exclusion list. data-no-router on a [data-stx-link] anchor did nothing at all.

There is a third instance the issue doesn't mention, and it is the worst one. The hover-prefetch handler also matches [data-stx-link] and applied no exclusions whatsoever — not even a protocol check:

var link=e.target.closest('[data-stx-link]');
if(!link)return;
var href=withCurrentLocale(link.getAttribute('href'));
fetch(href, ...)

So merely hovering a link the router was told to leave alone fired a real GET at it. A cross-origin OAuth URL was requested from a mouseover, and hovering "Sign out" logged the user out. That is covered by a test now.

The exclusions are split in two, because they answer different questions. isRouterExcluded() is the opt-out — no href, foreign scheme, target=_blank, download, data-no-router — and gates both the click and the prefetch path however the link was found. shouldIntercept() keeps only the eligibility rules that make sense when auto-claiming an unmarked anchor (same-pathname, container presence, ?-relative).

Same-pathname behaviour for marked links is deliberately unchanged, so clicking a link to the current page still re-navigates rather than becoming a full reload. The two inline re-checks in the click handler are gone — they were a second, incomplete copy of the same list, which is how the two drifted apart to begin with.

ROUTER_REV 3 -> 4. 11 tests, 6 of which fail without the change.

14 days ago

Fixed in be29fef.

The missing concept — "which same-origin paths does the router own" — already existed as the file router's route table; it just never reached the client. getOwnedRouteMatchers() now ships Route.regex, the same matcher the server routes with, in window.__stxRouterConfig.ownedRoutes, and shouldIntercept() consults it.

Regex sources rather than patterns, deliberately: the client runtime is a template literal (CLAUDE.md item 41), so re-implementing the pattern compiler in there would mean maintaining a second matcher — in a file where every backslash has to be doubled — that must agree with this one forever. Shipping the compiled regex makes disagreement structurally impossible.

Verified against your shape:

/                          OWNED (SPA)
/login                     OWNED (SPA)
/cars/42                   OWNED (SPA)     ← dynamic, via the server's own matcher
/api/auth/github/redirect  not owned -> browser
/docs/guide                not owned -> browser

So the four OAuth anchors and the /docs proxy link no longer need data-no-router, and neither does the next one anyone adds. I'd re-run before deleting all 17, but they should be redundant now.

The safety property is the part I'd want reviewed. Absence of the table means UNKNOWN, not "owns nothing" — a client that never received it keeps the old behaviour, and a matcher that fails to compile is skipped rather than taking the gate down. Reading an empty list as authoritative would disable SPA navigation for an entire site, which is a much worse failure than the double-fetch. There are explicit tests for all three cases.

One thing found while wiring it: the router config script was only emitted when stx.config.ts declared a router block, so for most apps the table would have shipped nowhere. It's now emitted whenever there's something to say.

Scope note: this gates the interceptAllLinks auto-claim path only. An explicit <StxLink to> / [data-stx-link] is an author saying "treat this as a router link", and I left that authoritative — pointing one at a non-page is an authoring error that a build-time attribute check should catch (part of #1852), not something to silently reinterpret at runtime.

Covered by packages/router/test/client-owned-routes.test.ts (10 tests, driving the real router in a DOM) and packages/stx/test/owned-routes.test.ts (8 tests). Verified non-vacuous — removing the gate fails 3. Full suite: 11285 pass, 1 pre-existing unrelated failure.

Sign in to comment on this issue.