diff --git a/.changeset/auth-catchall-yields-unowned-paths.md b/.changeset/auth-catchall-yields-unowned-paths.md new file mode 100644 index 0000000000..4baacff006 --- /dev/null +++ b/.changeset/auth-catchall-yields-unowned-paths.md @@ -0,0 +1,36 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): the auth catch-all yields paths better-auth does not own (#4088) + +`registerAuthRoutes` mounts `rawApp.all('${basePath}/*')` over the whole auth +namespace (`/api/v1/auth` by default), and that handler was **terminal**: it +returned better-auth's response unconditionally, including the 404 better-auth +produces for a path it does not implement. Any other plugin's route under that +prefix was therefore reachable only if it happened to register **first** — Hono +runs handlers matching a path in registration order and the first to return a +Response wins. + +That put a load-bearing surface at the mercy of `kernel.use()` order. +`@objectstack/plugin-hono-server` mounts `/auth/me/permissions` and +`/auth/me/localization` from its own `kernel:ready` hook; objectui's entire +permission layer reads the former and `core`'s auth gate allow-lists the latter +as an endpoint a gated user must still reach. Register `AuthPlugin` before +`HonoServerPlugin` and all of it silently 404s. + +A 404 from better-auth now means "this path is not mine" and the catch-all yields +to whatever else matched, in either registration order. Deliberately narrow: + +- **Only 404 falls through.** 401/403 are real better-auth answers, not + disclaimers of ownership. +- **Precedence still favours the namespace owner.** better-auth wins every path + it implements; only its leftovers are up for grabs. +- **The unclaimed-path wire shape is unchanged.** When nothing downstream + answers, better-auth's own 404 is returned verbatim rather than Hono's + `404 Not Found`. + +No configuration changes and no new routes. The only behavioural difference for +an existing deployment is that a route another plugin mounts under +`/api/v1/auth/*` now answers regardless of plugin order — previously it answered +only in the lucky order. diff --git a/packages/plugins/plugin-auth/package.json b/packages/plugins/plugin-auth/package.json index c3b07eabc6..b68552ee00 100644 --- a/packages/plugins/plugin-auth/package.json +++ b/packages/plugins/plugin-auth/package.json @@ -33,6 +33,7 @@ }, "devDependencies": { "@types/node": "^26.1.1", + "hono": "^4.12.32", "typescript": "^6.0.3", "vitest": "^4.1.10" }, diff --git a/packages/plugins/plugin-auth/src/auth-catchall-fallthrough.test.ts b/packages/plugins/plugin-auth/src/auth-catchall-fallthrough.test.ts new file mode 100644 index 0000000000..503f64b230 --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-catchall-fallthrough.test.ts @@ -0,0 +1,140 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4088 — the auth catch-all must not swallow other plugins' routes. + * + * `registerAuthRoutes` mounts `rawApp.all('${basePath}/*')` over the whole auth + * namespace. It used to be TERMINAL — better-auth's response was returned + * unconditionally, including the 404 better-auth produces for a path it does + * not implement. Any other plugin's route under that prefix was therefore + * reachable only if it happened to register FIRST (Hono runs handlers matching + * a path in registration order; first to return a Response wins). + * + * That made a load-bearing surface depend on `kernel.use()` order: + * `plugin-hono-server` mounts `/auth/me/permissions` + `/auth/me/localization` + * from its own `kernel:ready` hook, objectui's whole permission layer reads the + * former and `core`'s auth gate allow-lists the latter — and all of it silently + * 404s if AuthPlugin is registered first. + * + * These tests register the catch-all FIRST and the specific route SECOND, i.e. + * exactly the order that used to fail, and drive a real Hono app so the + * assertions are about the shipped handler rather than a stand-in. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { Hono } from 'hono'; +import { AuthPlugin } from './auth-plugin'; +import type { PluginContext } from '@objectstack/core'; + +const BASE = '/api/v1/auth'; + +/** better-auth's own 404 for a path it does not implement. */ +const BETTER_AUTH_404 = { message: 'Not Found', code: 'NOT_FOUND' }; + +/** + * Mount the plugin's real route registration on a real Hono app, with + * better-auth stubbed by a path → Response table so we control exactly which + * paths the namespace owner claims. + */ +async function mountCatchAll(owned: Record Response>) { + const app = new Hono(); + const ctx: PluginContext = { + registerService: vi.fn(), + getService: vi.fn((name: string) => (name === 'manifest' ? { register: vi.fn() } : undefined)), + getServices: vi.fn(() => new Map()), + hook: vi.fn(), + trigger: vi.fn(), + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }, + getKernel: vi.fn(), + } as any; + + const plugin = new AuthPlugin({ secret: 'test-secret-at-least-32-chars-long!!' }); + await plugin.init(ctx); + + const handleRequest = vi.fn(async (req: Request) => { + const path = new URL(req.url).pathname; + const make = owned[path]; + if (make) return make(); + return new Response(JSON.stringify(BETTER_AUTH_404), { + status: 404, + headers: { 'Content-Type': 'application/json' }, + }); + }); + (plugin as any).authManager = { handleRequest }; + + const httpServer: any = { getRawApp: () => app, getPort: () => 0 }; + (plugin as any).registerAuthRoutes(httpServer, ctx); + + return { app, handleRequest }; +} + +describe('auth catch-all yields paths better-auth does not own (#4088)', () => { + let mounted: Awaited>; + + beforeEach(async () => { + mounted = await mountCatchAll({ + // A path better-auth really implements, plus one that answers 401. + [`${BASE}/get-session`]: () => new Response(JSON.stringify({ user: null }), { status: 200 }), + [`${BASE}/protected`]: () => new Response(JSON.stringify({ error: 'nope' }), { status: 401 }), + }); + }); + + it('lets a LATER-registered route answer — the order that used to 404', async () => { + // The real collision: plugin-hono-server mounts this from its own + // kernel:ready hook, which may run after AuthPlugin's. + mounted.app.get(`${BASE}/me/permissions`, (c) => c.json({ authenticated: true, from: 'hono-plugin' })); + + const res = await mounted.app.request(`http://localhost${BASE}/me/permissions`); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ authenticated: true, from: 'hono-plugin' }); + }); + + it('does the same for /auth/me/localization', async () => { + mounted.app.get(`${BASE}/me/localization`, (c) => c.json({ authenticated: true, currency: 'USD' })); + + const res = await mounted.app.request(`http://localhost${BASE}/me/localization`); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ authenticated: true, currency: 'USD' }); + }); + + it('keeps better-auth winning every path it DOES own', async () => { + // Precedence must still favour the namespace owner: a later route must + // not be able to hijack a real better-auth endpoint. + mounted.app.get(`${BASE}/get-session`, (c) => c.json({ hijacked: true })); + + const res = await mounted.app.request(`http://localhost${BASE}/get-session`); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ user: null }); + }); + + it('does NOT fall through on 401 — that is a real answer, not a disclaimer', async () => { + mounted.app.get(`${BASE}/protected`, (c) => c.json({ leaked: true })); + + const res = await mounted.app.request(`http://localhost${BASE}/protected`); + + expect(res.status).toBe(401); + expect(await res.json()).toEqual({ error: 'nope' }); + }); + + it('returns better-auth’s own 404 verbatim when nothing else matches', async () => { + // The wire shape for a genuinely unclaimed auth path must not change: + // no route is registered for this path, so the fall-through finds + // nothing and better-auth's 404 stands. + const res = await mounted.app.request(`http://localhost${BASE}/no-such-endpoint`); + + expect(res.status).toBe(404); + expect(await res.json()).toEqual(BETTER_AUTH_404); + }); + + it('still forwards to better-auth exactly once per request', async () => { + mounted.app.get(`${BASE}/me/permissions`, (c) => c.json({ ok: true })); + mounted.handleRequest.mockClear(); + + await mounted.app.request(`http://localhost${BASE}/me/permissions`); + + expect(mounted.handleRequest).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index b8dfc7abd8..933852bead 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -1814,11 +1814,56 @@ export class AuthPlugin implements Plugin { // Register wildcard route to forward all auth requests to better-auth. // better-auth is configured with basePath matching our route prefix, so we // forward the original request directly — no path rewriting needed. - rawApp.all(`${basePath}/*`, async (c: any) => { + // + // [#4088] This catch-all owns the whole auth namespace, and it used to be + // TERMINAL: it returned better-auth's response unconditionally, including + // the 404 better-auth produces for a path it does not implement. Any OTHER + // plugin's route under `${basePath}/*` was therefore reachable only if it + // happened to register FIRST — Hono runs handlers matching a path in + // registration order and the first to return a Response wins. That made a + // load-bearing surface depend on `kernel.use()` order: + // `plugin-hono-server` mounts `/auth/me/permissions` and + // `/auth/me/localization` from its own `kernel:ready` hook, the console's + // entire permission layer reads the former, and `core`'s auth gate + // allow-lists the latter — all of it silently 404s if AuthPlugin is used + // before HonoServerPlugin. Same class as #2567 and #4018: an invariant held + // by ordering luck rather than enforced. + // + // So a 404 now means "better-auth does not own this path" and we yield to + // whatever else matched, in either registration order. Deliberately narrow: + // 401/403 are real better-auth answers, not disclaimers of ownership, and + // when nothing downstream answers we return better-auth's own 404 verbatim + // so the wire shape for a genuinely unclaimed auth path is unchanged. + // Precedence still favours the namespace owner — better-auth wins every + // path it implements, and only its leftovers are up for grabs. + rawApp.all(`${basePath}/*`, async (c: any, next: any) => { try { // Forward the original request to better-auth handler const response = await this.authManager!.handleRequest(c.req.raw); + if (response.status === 404) { + await next(); + // A non-404 downstream means something else answered — hand that back. + // NOT `c.finalized`: reaching the end of the chain with nothing + // matched runs Hono's notFound handler, which sets a response and + // flips `finalized` to true, so it cannot tell "someone answered" + // from "nobody did". Status can. The one thing this trades away is a + // downstream route's own 404 BODY (better-auth's 404 is returned in + // its place); the status is identical either way, and no route under + // this prefix answers 404 today — `/auth/me/*` answer 200 with an + // `authenticated: false` payload for an anonymous caller. + if (c.res && c.res.status !== 404) return; + // Assign, don't `return`: the IP-gate `rawApp.use()` above puts a + // middleware in this chain, and Hono's compose only assigns a + // handler's returned Response while `c.finalized` is false. The + // notFound above already flipped it, so a `return response` here is + // silently dropped and the caller gets Hono's "404 Not Found" text + // instead of better-auth's JSON. Setting `c.res` is not subject to + // that condition. + c.res = response; + return; + } + // better-auth catches internal errors and returns error Responses // without throwing, so the catch block below would never trigger. // We proactively log server errors here for observability. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1e60504dd9..4fcd35d620 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1345,6 +1345,9 @@ importers: '@types/node': specifier: ^26.1.1 version: 26.1.1 + hono: + specifier: ^4.12.32 + version: 4.12.32 typescript: specifier: ^6.0.3 version: 6.0.3 @@ -6283,10 +6286,6 @@ packages: headers-polyfill@5.0.1: resolution: {integrity: sha512-1TJ6Fih/b8h5TIcv+1+Hw0PDQWJTKDKzFZzcKOiW1wJza3XoAQlkCuXLbymPYB8+ZQyw8mHvdw560e8zVFIWyA==} - hono@4.12.31: - resolution: {integrity: sha512-zJIHFrl6bq3RDd2YusFNCDlM8qUprxKswyi/OPzPyzKDdyBXDqWx8bZlZ7R+saTdSTatUmb3O7K4SspGPaEOQg==} - engines: {node: '>=16.9.0'} - hono@4.12.32: resolution: {integrity: sha512-XcuyW9qE2kJn07PkecMOBd5Vq/hMy7mmGw+idz1yblbg9N17ijJODrvPkn7/dwL3Kulj8LcRJ69DLOWf91dRUg==} engines: {node: '>=16.9.0'} @@ -9727,10 +9726,6 @@ snapshots: optionalDependencies: tailwindcss: 4.3.3 - '@hono/node-server@2.0.12(hono@4.12.31)': - dependencies: - hono: 4.12.31 - '@hono/node-server@2.0.12(hono@4.12.32)': dependencies: hono: 4.12.32 @@ -10008,7 +10003,7 @@ snapshots: '@modelcontextprotocol/sdk@1.29.0(zod@4.4.3)': dependencies: - '@hono/node-server': 2.0.12(hono@4.12.31) + '@hono/node-server': 2.0.12(hono@4.12.32) ajv: 8.20.0 ajv-formats: 3.0.1(ajv@8.20.0) content-type: 1.0.5 @@ -10018,7 +10013,7 @@ snapshots: eventsource-parser: 3.1.0 express: 5.2.1 express-rate-limit: 8.6.0(express@5.2.1) - hono: 4.12.31 + hono: 4.12.32 jose: 6.2.3 json-schema-typed: 8.0.2 pkce-challenge: 5.0.1 @@ -13095,8 +13090,6 @@ snapshots: set-cookie-parser: 3.1.2 optional: true - hono@4.12.31: {} - hono@4.12.32: {} hosted-git-info@4.1.0: