diff --git a/.changeset/adapter-hono-auth-wildcard-yields.md b/.changeset/adapter-hono-auth-wildcard-yields.md new file mode 100644 index 0000000000..58157ad6e6 --- /dev/null +++ b/.changeset/adapter-hono-auth-wildcard-yields.md @@ -0,0 +1,28 @@ +--- +"@objectstack/adapter-hono": patch +--- + +fix(adapters/hono): the auth wildcard yields paths the auth service does not own (#4117) + +`app.all('${prefix}/auth/*')` claimed a whole namespace and was **terminal**: it +returned the auth service's response unconditionally, including better-auth's 404 +for a path it does not implement, and the legacy `handleAuth` bridge's own +`handled: false` 404. That is the #4088 shape, found by #4116's enumeration after +manual greps had missed it. + +A 404 from better-auth, or `handled: false` from the dispatcher, now means "not +this mount's path" and the handler yields. The predicate is the dispatcher's own +`handled` flag wherever one exists — an explicit ownership answer beats inferring +one from a status; only the better-auth hand-off lacks such a flag, and there the +404 is the signal, as in #4092. + +**What changes on the wire.** An unowned path under `${prefix}/auth/*` used to get +a 404 built by this mount. It now continues to the `${prefix}/*` dispatcher +catch-all and gets a real, gate-carrying `dispatch()` attempt, so a domain handler +registered for such a path becomes reachable — this adapter's actual extension +mechanism. When nothing anywhere claims the path the reply is still the same +enveloped `{ success: false, error: { message: 'Not Found', code: 404 } }`. Paths +the auth service does own are untouched, and a 401/403 from it is never treated as +a disclaimer of ownership. + +No configuration changes and no new routes. diff --git a/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts b/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts new file mode 100644 index 0000000000..43364309cc --- /dev/null +++ b/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts @@ -0,0 +1,145 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4117 — the `/auth/*` mount must yield paths it does not own. + * + * It claims a whole namespace and used to be TERMINAL: it answered 404 for a path + * its auth service does not implement. Found by #4116's scan, which no manual grep + * had managed. Its `/storage/*` twin was ratcheted alongside it and is now gone + * entirely — #4087/#4112 deleted that bridge after reaching the same conclusion + * from the other direction ("the wildcard was wider than the two routes it + * served"). + * + * ## What yielding buys HERE, precisely + * + * Not what it bought in #4092. There, yielding made another plugin's route + * reachable. In this adapter it cannot: the `${prefix}/*` dispatcher catch-all + * is registered right after these two and is DELIBERATELY terminal (ADR-0076 + * OQ#9, #3576/#3608 — the gate stages live inside `dispatch()`, so splitting it + * into per-prefix Hono mounts would bypass them). So a route registered later + * under `/api/auth/*` is swallowed by the catch-all whether or not these two + * yield, and Hono-route mounting is not this adapter's extension path at all — + * registering a domain handler is. + * + * What yielding buys is that an unowned `/auth/*` or `/storage/*` path now + * reaches that gated `dispatch()` instead of dead-ending in a 404 built two + * mounts earlier. A domain handler registered for such a path becomes + * reachable, which is exactly the adapter's intended mechanism. These tests pin + * that, and pin that the owners still win everything they own. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { Hono } from 'hono'; + +const mockDispatcher = { + getDiscoveryInfo: vi.fn().mockReturnValue({ version: '1.0', endpoints: [] }), + handleAuth: vi.fn(), + handleGraphQL: vi.fn(), + dispatch: vi.fn(), +}; + +vi.mock('@objectstack/runtime', () => ({ + HttpDispatcher: function HttpDispatcher() { return mockDispatcher; }, +})); + +import { createHonoApp } from './index'; + +/** No domain claimed the path — the dispatcher's own ownership signal. */ +const UNHANDLED = { handled: false }; +const HANDLED = (body: unknown, status = 200) => ({ handled: true, response: { body, status } }); + +/** A kernel whose `auth` service is (or is not) present. */ +const kernelWith = (authService?: unknown) => ({ + name: 'test-kernel', + getService: (n: string) => (n === 'auth' && authService ? authService : undefined), +}) as any; + +const authServiceReturning = (status: number, body: unknown = { from: 'better-auth' }) => ({ + handleRequest: vi.fn(async () => new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + })), +}); + +describe('auth mount yields paths better-auth does not own (#4117)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockDispatcher.dispatch.mockResolvedValue(UNHANDLED); + mockDispatcher.handleAuth.mockResolvedValue(UNHANDLED); + }); + + it('reaches the gated dispatch when better-auth 404s', async () => { + // `/auth/me/permissions` is the canonical unowned path: nothing in + // better-auth serves it. Before #4117 this 404'd here; now the request gets + // a real dispatch attempt, so a domain handler for it is reachable. + mockDispatcher.dispatch.mockResolvedValue(HANDLED({ authenticated: true, from: 'dispatch' })); + const app: Hono = createHonoApp({ kernel: kernelWith(authServiceReturning(404)) }); + + const res = await app.request('/api/auth/me/permissions'); + + expect(mockDispatcher.dispatch).toHaveBeenCalled(); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ authenticated: true, from: 'dispatch' }); + }); + + it('keeps better-auth winning the paths it DOES own', async () => { + const app: Hono = createHonoApp({ kernel: kernelWith(authServiceReturning(200, { user: null })) }); + + const res = await app.request('/api/auth/get-session'); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ user: null }); + expect(mockDispatcher.dispatch).not.toHaveBeenCalled(); + }); + + it('does NOT yield on 401 — a real answer, not a disclaimer of ownership', async () => { + const app: Hono = createHonoApp({ kernel: kernelWith(authServiceReturning(401, { error: 'nope' })) }); + + const res = await app.request('/api/auth/protected'); + + expect(res.status).toBe(401); + expect(await res.json()).toEqual({ error: 'nope' }); + expect(mockDispatcher.dispatch).not.toHaveBeenCalled(); + }); + + it('yields on the dispatcher path too, keyed on `handled: false`', async () => { + // No auth service at all, so the mount falls back to `dispatcher.handleAuth` + // — whose explicit `handled` flag beats inferring ownership from a status. + mockDispatcher.dispatch.mockResolvedValue(HANDLED({ currency: 'USD' })); + const app: Hono = createHonoApp({ kernel: kernelWith() }); + + const res = await app.request('/api/auth/me/localization'); + + expect(mockDispatcher.handleAuth).toHaveBeenCalled(); + expect(mockDispatcher.dispatch).toHaveBeenCalled(); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ currency: 'USD' }); + }); + + it('still lets handleAuth answer what it DOES handle', async () => { + mockDispatcher.handleAuth.mockResolvedValue(HANDLED({ ok: true })); + const app: Hono = createHonoApp({ kernel: kernelWith() }); + + const res = await app.request('/api/auth/login', { method: 'POST' }); + + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ ok: true }); + expect(mockDispatcher.dispatch).not.toHaveBeenCalled(); + }); +}); + +describe('the enveloped 404 survives when nothing anywhere claims the path', () => { + beforeEach(() => vi.clearAllMocks()); + + it('still ends in the enveloped 404 when nothing anywhere claims it', async () => { + mockDispatcher.handleAuth.mockResolvedValue(UNHANDLED); + mockDispatcher.dispatch.mockResolvedValue(UNHANDLED); + const app: Hono = createHonoApp({ kernel: kernelWith() }); + + const res = await app.request('/api/auth/nope'); + + expect(res.status).toBe(404); + // Not Hono's bare "404 Not Found" text — the platform envelope is preserved. + expect(await res.json()).toEqual({ success: false, error: { message: 'Not Found', code: 404 } }); + }); +}); diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index 6db020306a..c83049fcdd 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -275,8 +275,39 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { return c.redirect(prefix); }); + /** + * Hand a path THIS mount does not own to whatever else matched (#4117). + * + * The `${prefix}/auth/*` mount below claims a whole namespace and used to be + * TERMINAL — it answered 404 for a path its auth service does not implement. + * That is #4088's shape, which cost four fixes before #4116's scan started + * enumerating it, and it is what #4087/#4112 had already concluded about the + * `/storage` bridge it deleted: "the wildcard was wider than the two routes it + * served". + * + * What yielding buys HERE is narrower than in #4092, and worth stating so + * nobody over-reads it. It does NOT make a later-registered Hono route + * reachable: the `${prefix}/*` dispatcher catch-all below is DELIBERATELY + * terminal (ADR-0076 OQ#9, #3576/#3608 — the gate stages live inside + * `dispatch()`), so it would swallow such a route either way, and mounting Hono + * routes is not this adapter's extension path anyway. It means an unowned + * `/auth/*` path now reaches that gated `dispatch()` instead of dead-ending in + * a 404 built one mount earlier, so a domain handler registered for it becomes + * reachable — which IS the adapter's mechanism. + * + * `c.res = …` rather than `return …`: `app.use('*', cors(…))` above puts a + * middleware in this chain, and Hono's compose only assigns a handler's + * RETURNED Response while `c.finalized` is false. Reaching the end of the chain + * runs notFound, which sets a response and flips that flag, so a `return` here + * is silently dropped (learned the hard way in #4092). + */ + const yieldUnowned = async (c: any, next: any, fallback: () => Response) => { + await next(); + if (!c.res) c.res = fallback(); + }; + // --- Auth (needs auth service integration) --- - app.all(`${prefix}/auth/*`, async (c) => { + app.all(`${prefix}/auth/*`, async (c, next) => { try { const path = c.req.path.substring(`${prefix}/auth/`.length); const method = c.req.method; @@ -330,10 +361,15 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { if (authService && typeof authService.handleRequest === 'function') { const response = await authService.handleRequest(c.req.raw); - return new Response(response.body, { + const forwarded = () => new Response(response.body, { status: response.status, headers: response.headers, }); + // 404 from better-auth means "not one of my endpoints" — the #4092 + // signal. `/auth/me/permissions` is the canonical example: nothing in + // better-auth serves it, `plugin-hono-server` does. + if (response.status === 404) return yieldUnowned(c, next, forwarded); + return forwarded(); } // Fallback to legacy dispatcher @@ -341,6 +377,9 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { ? {} : await c.req.json().catch(() => ({})); const result = await dispatcher.handleAuth(path, method, body, { request: c.req.raw }); + // `handled: false` is the dispatcher saying no auth domain claimed it — + // an explicit ownership signal, better than inferring one from a status. + if (!result.handled) return yieldUnowned(c, next, () => toResponse(c, result)); return toResponse(c, result); } catch (err: any) { return errorJson(c, err.message || 'Internal Server Error', err.statusCode || 500); diff --git a/scripts/check-wildcard-fallthrough.mjs b/scripts/check-wildcard-fallthrough.mjs index 0a37024dfd..95428ea746 100644 --- a/scripts/check-wildcard-fallthrough.mjs +++ b/scripts/check-wildcard-fallthrough.mjs @@ -107,6 +107,21 @@ const MOUNTS = { // adapter's and plugin's own `*` middlewares). Terminal here would take down // the entire surface, not one namespace. 'packages/cli/src/commands/serve.ts:use *': { yields: true }, + // Fixed by #4117, found BY this scan (manual greps had missed it). A + // pre-processing shim that hands off to the env's auth service, not an owner: + // a path that service does not claim now continues to the `${prefix}/*` + // dispatcher below. Narrower than #4092 — it does not make a later Hono route + // reachable (the dispatcher catch-all is deliberately terminal and would + // swallow it anyway); it means an unowned path gets a real gated `dispatch()` + // attempt. Keyed on the dispatcher's own `handled` flag where one exists. + // + // Its `${prefix}/storage/*` twin was ratcheted here alongside it and is now + // gone entirely: #4087/#4112 deleted that bridge, having reached the same + // conclusion from the other direction — "the wildcard was wider than the two + // routes it served". Two independent reads landing on the same defect is the + // argument for enumerating the shape rather than finding it by eye each time. + "packages/adapters/hono/src/index.ts:all `${prefix}/auth/*`": { yields: true }, + 'packages/plugins/plugin-hono-server/src/adapter.ts:use *': { yields: true }, 'packages/plugins/plugin-hono-server/src/hono-plugin.ts:use *': { yields: true }, @@ -147,17 +162,9 @@ const MOUNTS = { // ── Ratchet: real, tracked, NOT blessed ───────────────────────────────── // - // Both are the #4088 shape in an adapter that no in-repo package depends on, - // so nothing is broken today — but it ships, and ADR-0076's own trigger is an - // out-of-tree embedder (`../objectbase`'s gateway). An embedder mounting a - // route under either prefix hits exactly #4088. Found BY this scan; manual - // greps for the pattern had missed both. Tracked by #4117. - 'packages/adapters/hono/src/index.ts:all `${prefix}/auth/*`': { - ratchet: '#4117 — terminal, same shape as #4088; adapter has no in-repo consumer', - }, - 'packages/adapters/hono/src/index.ts:all `${prefix}/storage/*`': { - ratchet: '#4117 — terminal, same shape as #4088; adapter has no in-repo consumer', - }, + // Empty as of #4117. The mechanism stays — it is how the next terminal wildcard + // gets recorded honestly instead of being either fixed on the spot or quietly + // skipped. Declare the current state plus a `ratchet` naming the issue. }; /** HTTP-verb registrars plus `use`; anything that can claim a path pattern. */ @@ -229,9 +236,23 @@ function callsContinuation(fn, src) { // a call inside it is not this handler yielding. if (rebinds(node)) return; // `next()` — or `return next()` / `await next()`, same shape. - if (ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === name) { - found = true; - return; + if (ts.isCallExpression(node)) { + // `next()` — or `return next()` / `await next()`, same shape. + if (ts.isIdentifier(node.expression) && node.expression.text === name) { + found = true; + return; + } + // …or HANDED to a helper that awaits it: `yieldUnowned(c, next, …)`. + // Counting this is deliberate (#4117). `adapters/hono` factors the yield + // into one helper precisely because the compose subtlety it encodes should + // be written once, and a checker that only recognised a direct call would + // push code toward duplicating it. The trade-off is a handler that passes + // the continuation somewhere that never awaits it — narrower than the false + // NEGATIVE it replaces, and the ledger still requires a human to classify. + if (node.arguments.some((a) => ts.isIdentifier(a) && a.text === name)) { + found = true; + return; + } } ts.forEachChild(node, visit); }; @@ -463,6 +484,14 @@ function selfTest() { !yieldsIn('app.all("/a/*", async (c, next) => { items.forEach((next) => next()); return r; });'), 'an INNER binding shadowing the name must not count as yielding', ); + assert( + yieldsIn('app.all("/a/*", async (c, next) => yieldUnowned(c, next, fb));'), + 'handing the continuation to a helper that awaits it DOES count (#4117)', + ); + assert( + !yieldsIn('app.all("/a/*", async (c, next) => { log("no next here"); return r; });'), + 'a call that neither invokes nor receives the continuation must not count', + ); // `resolveHandler` — a handler passed by name must still be analysed, or the // scan reports a yielding mount as terminal (cloud#923 mounts it that way). @@ -486,7 +515,7 @@ function selfTest() { 'method is part of the key', ); - console.log('✓ self-test: 15 cases'); + console.log('✓ self-test: 17 cases'); } if (process.argv.includes('--self-test')) selfTest();