diff --git a/.changeset/hono-toresponse-preserve-response.md b/.changeset/hono-toresponse-preserve-response.md new file mode 100644 index 0000000000..9e4b913b48 --- /dev/null +++ b/.changeset/hono-toresponse-preserve-response.md @@ -0,0 +1,19 @@ +--- +"@objectstack/hono": patch +--- + +`createHonoApp` no longer discards the status and body of a dispatcher result that is already a `Response` — it hands the object on unchanged. + +`HttpDispatcherResult.result` is declared for direct response objects ("For flexible return types or direct response objects (Response/NextResponse)"), and the runtime really puts one there: the `/auth` domain returns whatever the auth service answered as `{ handled: true, result: response }`. The adapter's `toResponse` had no arm for that. It tested `result.type` for the `redirect` and `stream` descriptors, a `Response` spells neither, and the fall-through was `c.json(res, 200)` — so the real status was replaced by a literal `200` and the real body by `JSON.stringify` of a `Response`, which is `{}` because a `Response` has no own enumerable properties. + +Measured on a real boot through this adapter (a real kernel, the real dispatcher, `prefix: '/api/v1'`), an auth service answering an honest 404 on a path it does not serve: + +``` +GET /api/v1/auth/me/permissions + the door answered : 404 {"message":"Not found","code":"NOT_FOUND"} + the caller read : 200 {} +``` + +A discarded status is not a missing answer, it is a wrong one that reads as success: `res.ok`, `status === 200` and "nothing threw" all report a refusal, a 404 or a 500 as a completed operation, and a fail-closed guard written as `if (!data) return false` does not fire on `{}` because `{}` is truthy. Callers embedding this adapter now see the status and the body the door actually produced, along with its headers, and a non-JSON body arrives byte-identical instead of being re-serialized. + +The check is `instanceof Response` and nothing else: the `redirect` and `stream` descriptor arms, the plain-object rendering after them, and the separate `response` arm all behave exactly as before. diff --git a/packages/adapters/hono/src/hono-result-response-passthrough.test.ts b/packages/adapters/hono/src/hono-result-response-passthrough.test.ts new file mode 100644 index 0000000000..b27a1b1db5 --- /dev/null +++ b/packages/adapters/hono/src/hono-result-response-passthrough.test.ts @@ -0,0 +1,214 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16383] `toResponse` returns a `HttpDispatcherResult.result` that IS a + * `Response` unchanged — its real status, its real body, its real headers. + * + * ## The defect + * + * `HttpDispatcherResult.result` is DECLARED for direct response objects + * (`packages/runtime/src/http-dispatcher.ts`: "For flexible return types or + * direct response objects (Response/NextResponse)"), and the runtime really + * puts one there — `runtime/src/domains/auth.ts` hands back whatever the auth + * service answered as `{ handled: true, result: response }`. + * + * `toResponse` had no arm for that. It tested `result.type === 'redirect'` and + * `result.type === 'stream'`, and everything else fell into `c.json(res, 200)`. + * A Fetch `Response` has no own enumerable properties, so `JSON.stringify` of + * one is `{}`, and the `200` was a literal: + * + * door answers 404 {"message":"Not found","code":"NOT_FOUND"} + * caller reads 200 {} + * + * ⭐ The failure direction is what makes this a p1 rather than a cosmetic loss. + * A discarded status is not a missing answer, it is a WRONG answer that reads + * as success — `res.ok`, `status === 200` and "nothing threw" all report a + * refusal as a completed operation — and it DEFEATS fail-closed guards instead + * of merely missing them: objectui's `MePermissionsProvider.tsx` refuses on + * `if (!data) return false`, and `{}` is truthy. + * + * ⇒ Every case below asserts the real status AND the real body. A pin that + * asserted only "not 200" would stay green on a repair that answered some other + * wrong status with the body still destroyed. + * + * ## What this file is, and what its sibling is + * + * This package's vitest config aliases `@objectstack/runtime` to a stub, so the + * dispatcher here is a fixture — which is exactly what lets these cases drive + * `toResponse`'s `result` arm over statuses and body shapes the real + * composition cannot reach on demand. The other half is a REAL boot, in + * `packages/qa/http-conformance/src/hono-dispatcher-result-response.conformance.test.ts`: + * a real `LiteKernel`, the real `HttpDispatcher`, the real `/auth` domain, one + * wire reading. `@objectstack/hono` has no in-repo consumer (#4117), so that + * boot is the only thing there is to observe this through; neither file + * replaces the other. + * + * ⛔ Not this card, deliberately untouched: which paths the dispatcher CLAIMS + * (#16026), WHERE auth is mounted (#16025), and the escaped ADR-0112 envelope + * on the same function's error exit (#16545). + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { Hono } from 'hono'; + +const mockDispatcher = { + getDiscoveryInfo: vi.fn().mockReturnValue({ version: '1.0', routes: {} }), + handleAuth: vi.fn(), + dispatch: vi.fn(), +}; + +vi.mock('@objectstack/runtime', () => ({ + HttpDispatcher: function HttpDispatcher() { return mockDispatcher; }, +})); + +import { createHonoApp } from './index'; + +const PREFIX = '/api/v1'; +/** A path no explicit mount claims, so it lands on the `${prefix}/*` catch-all. */ +const PATH = `${PREFIX}/data/thing`; + +const kernel = { name: 'test-kernel' } as any; +const bootApp = (): Hono => createHonoApp({ kernel, prefix: PREFIX }); + +const jsonResponse = (status: number, body: unknown, headers: Record = {}) => + new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json', ...headers }, + }); + +describe('#16383: toResponse passes a `result` that is already a Response through', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockDispatcher.handleAuth.mockResolvedValue({ handled: false }); + }); + + // The statuses a door really produces. 200 is carried too: a repair that + // special-cased "non-200" would leave the success path rebuilt and its body + // re-serialized, which is the same defect wearing the other sign. + it.each([200, 201, 302, 400, 401, 403, 404, 409, 422, 500, 503])( + 'a %i Response reaches the caller with that status and its own body', + async (status) => { + const body = { message: `answer-${status}`, code: 'DOOR_SAID_SO' }; + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + result: jsonResponse(status, body, { 'X-Door': 'dispatcher' }), + }); + + const res = await bootApp().request(`http://localhost${PATH}`, { redirect: 'manual' }); + + expect(res.status).toBe(status); + // ⭐ The body half. `{}` is what the defect produced, and it is TRUTHY — + // asserting the status alone would pass on a door that still destroys it. + await expect(res.clone().json()).resolves.toEqual(body); + await expect(res.clone().text()).resolves.not.toBe('{}'); + expect(res.headers.get('x-door')).toBe('dispatcher'); + }, + ); + + it('does not re-serialize — a non-JSON body arrives byte-identical', async () => { + // `c.json(res, 200)` could not have produced this at all: the body is not + // JSON and its content-type is not `application/json`. A repair that + // rebuilt the Response from a parsed body would corrupt both. + const payload = 'id,name\n1,ada\n'; + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + result: new Response(payload, { + status: 418, + headers: { 'Content-Type': 'text/csv; charset=utf-8' }, + }), + }); + + const res = await bootApp().request(`http://localhost${PATH}`); + + expect(res.status).toBe(418); + expect(res.headers.get('content-type')).toBe('text/csv; charset=utf-8'); + await expect(res.text()).resolves.toBe(payload); + }); + + it('a bodyless refusal stays bodyless — no `{}` is invented for it', async () => { + // better-call answers an unrouted path exactly this way, and it is the + // shape `hono-auth-owned-404.test.ts` calls `unrouted404`. + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + result: new Response(null, { status: 404, statusText: 'Not Found' }), + }); + + const res = await bootApp().request(`http://localhost${PATH}`); + + expect(res.status).toBe(404); + await expect(res.text()).resolves.toBe(''); + }); + + it('the auth mount\'s dispatcher fallback passes one through too', async () => { + // The second door into `toResponse`: `${prefix}/auth/*` with no auth + // service on the kernel falls back to `dispatcher.handleAuth`, and + // `runtime/src/domains/auth.ts` is the very producer that puts a `Response` + // in `result`. Both callers must render it the same way. + mockDispatcher.handleAuth.mockResolvedValue({ + handled: true, + result: jsonResponse(401, { message: 'Unauthorized', code: 'UNAUTHENTICATED' }), + }); + + const res = await bootApp().request(`http://localhost${PREFIX}/auth/get-session`); + + expect(res.status).toBe(401); + await expect(res.json()).resolves.toEqual({ message: 'Unauthorized', code: 'UNAUTHENTICATED' }); + }); + + describe('⛔ the arms either side of it are untouched', () => { + it('a plain object result is still rendered as JSON with 200', async () => { + // The narrowness control. This is `hono.test.ts`'s "generic result + // objects with 200 status" case, restated here so a future widening of + // the passthrough (`typeof res === 'object'`, say) fails in THIS file, + // next to the reason it must not. + mockDispatcher.dispatch.mockResolvedValue({ handled: true, result: { foo: 'bar' } }); + + const res = await bootApp().request(`http://localhost${PATH}`); + + expect(res.status).toBe(200); + await expect(res.json()).resolves.toEqual({ foo: 'bar' }); + }); + + it('a redirect descriptor still redirects', async () => { + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + result: { type: 'redirect', url: 'https://example.com' }, + }); + + const res = await bootApp().request(`http://localhost${PATH}`, { redirect: 'manual' }); + + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('https://example.com'); + }); + + it('a stream descriptor still streams', async () => { + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + result: { + type: 'stream', + events: (async function* () { yield { tick: 1 }; })(), + contentType: 'text/event-stream', + }, + }); + + const res = await bootApp().request(`http://localhost${PATH}`); + + expect(res.status).toBe(200); + expect(res.headers.get('content-type')).toContain('text/event-stream'); + await expect(res.text()).resolves.toContain('data: {"tick":1}'); + }); + + it('the `response` arm — status + body + headers — is unchanged', async () => { + mockDispatcher.dispatch.mockResolvedValue({ + handled: true, + response: { status: 201, body: { id: 1 }, headers: { 'X-Custom': 'yes' } }, + }); + + const res = await bootApp().request(`http://localhost${PATH}`); + + expect(res.status).toBe(201); + expect(res.headers.get('x-custom')).toBe('yes'); + await expect(res.json()).resolves.toEqual({ id: 1 }); + }); + }); +}); diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index ba7ea91924..558a001549 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -258,6 +258,46 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { } if (result.result) { const res = result.result; + /** + * [#16383] A `result` that IS a `Response` is the answer — hand it on. + * + * `HttpDispatcherResult.result` is DECLARED for exactly this ("For + * flexible return types or direct response objects + * (Response/NextResponse)"), and the runtime really puts one there: + * `runtime/src/domains/auth.ts` returns `{ handled: true, result: + * response }` with whatever the auth service answered. + * + * This function had no arm for it. The two below test `res.type`, a + * `Response` never spells `'redirect'` or `'stream'` there, and the + * fall-through was `c.json(res, 200)` — so the real status was replaced + * by a literal `200` and the real body by `JSON.stringify` of a + * `Response`, which is `{}` because it has no own enumerable + * properties. Measured on a real boot through this adapter (a real + * kernel, the real dispatcher, `prefix: '/api/v1'`), an auth service + * answering an honest 404: + * + * GET /api/v1/auth/me/permissions + * the door answered : 404 {"message":"Not found","code":"NOT_FOUND"} + * the caller read : 200 {} <- manufactured here + * + * ⭐ That is not a missing answer, it is a WRONG one that reads as + * success, and it defeats fail-closed guards rather than missing them: + * objectui's `MePermissionsProvider.tsx` refuses on `if (!data) return + * false`, and `{}` is truthy. `res.ok`, `status === 200` and "nothing + * threw" all report a refusal as a completed operation. + * + * ⛔ Narrow on purpose — `instanceof Response`, not "looks like one". + * The arms below and the plain-object rendering after them are other + * producers' contracts and are unchanged; `hono.test.ts` and + * `hono-result-response-passthrough.test.ts` pin both sides of that + * line. Returning the object itself rather than rebuilding it is what + * keeps the body byte-identical (a CSV, an empty 404) and the + * producer's headers attached; the `stream` arms below already return a + * `Response` this way. + */ + if (res instanceof Response) { + return res; + } if (res.type === 'redirect' && res.url) { return c.redirect(res.url); } diff --git a/packages/qa/http-conformance/package.json b/packages/qa/http-conformance/package.json index 6646106b27..15c8e98b84 100644 --- a/packages/qa/http-conformance/package.json +++ b/packages/qa/http-conformance/package.json @@ -14,6 +14,7 @@ }, "devDependencies": { "@objectstack/driver-sqlite-wasm": "workspace:*", + "@objectstack/hono": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/plugin-hono-server": "workspace:*", "@objectstack/runtime": "workspace:*", diff --git a/packages/qa/http-conformance/src/hono-dispatcher-result-response.conformance.test.ts b/packages/qa/http-conformance/src/hono-dispatcher-result-response.conformance.test.ts new file mode 100644 index 0000000000..2f9765861c --- /dev/null +++ b/packages/qa/http-conformance/src/hono-dispatcher-result-response.conformance.test.ts @@ -0,0 +1,148 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16383] `@objectstack/hono` must hand a dispatcher result that IS a + * `Response` to the caller AS ITSELF — real status, real body, real headers. + * + * ## The defect this file was written against + * + * `HttpDispatcherResult.result` is DECLARED for direct response objects + * (`packages/runtime/src/http-dispatcher.ts`: "For flexible return types or + * direct response objects (Response/NextResponse)"), and the runtime really + * puts one there: `packages/runtime/src/domains/auth.ts` forwards whatever the + * auth service answered as `{ handled: true, result: response }`. That producer + * half is pinned on its own side by + * `runtime/src/domains/auth-claim-segment-boundary.test.ts` and + * `runtime/src/auth-forward-fault-sanitization.test.ts`; this file is the + * consumer half. + * + * The adapter's `toResponse` fell past its `redirect` and `stream` arms into + * `c.json(res, 200)`. A Fetch `Response` has no own enumerable properties, so + * `JSON.stringify` of one is `{}`, and the `200` was a literal — so every + * status a door produced reached the caller as `200 {}` with the producer's + * headers gone. + * + * ⭐ That is not a missing answer, it is a WRONG answer that reads as success, + * and it defeats fail-closed guards rather than merely missing them: objectui's + * `MePermissionsProvider.tsx` refuses on `if (!data) return false`, and `{}` is + * truthy. + * + * ## ⚠️ Why this lives HERE and not in the adapter's own suite + * + * `@objectstack/hono` has NO in-repo consumer (#4117), so there is nothing to + * observe this through except a CONSTRUCTED BOOT — and the adapter's own suite + * cannot be that boot for this question: `packages/adapters/hono/vitest.config.ts` + * aliases `@objectstack/runtime` to a hand-written stub, so no spelling of that + * specifier reaches the real `HttpDispatcher` from inside that package. The + * adapter-local pin + * (`packages/adapters/hono/src/hono-result-response-passthrough.test.ts`) pins + * the RENDERING against that stub, over every status and every `result` shape; + * this file pins that the two packages still meet — a real `LiteKernel`, the + * real `HttpDispatcher` the adapter constructs for itself, the real `/auth` + * domain, requests injected through the returned Hono app and read off the + * wire. Neither half is redundant: the local one fails fast on an adapter edit, + * this one fails when the contract between the packages moves. + * + * ## ⭐ The route these rows drive, and why it is the ONLY one that reaches the arm + * + * Measured on this boot, all four statuses over `/auth`, `/auth/`, + * `/auth/me/permissions` and `/auth/whatever`: the adapter's + * `${prefix}/auth/*` mount answers a 200 / 403 / 500 ITSELF, from its own + * `forwarded()` — those never reach `toResponse` at all. Only a **404** is + * disclaimed (#4088 / #15928: the mount yields a 404 the auth service does not + * own), and only then does the `${prefix}/*` catch-all reach `dispatch()`, the + * `/auth` domain claim the path, and the auth service's `Response` arrive in + * `HttpDispatcherResult.result`. + * + * That is why `handleRequest` call COUNT is asserted beside every status here + * and is not decoration: **2 means the arm under test ran** (the mount called + * the service, disclaimed, yielded; the domain called it again), **1 means the + * mount answered and `toResponse` was never consulted**. Without it a row could + * go green through a path that has nothing to do with this card. The status + * matrix over arbitrary `result` values belongs to the adapter-local pin, which + * can drive the arm directly. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import { createHonoApp } from '@objectstack/hono'; + +const PREFIX = '/api/v1'; + +/** A refusal body a real door writes — a payload, not an empty envelope. */ +const REFUSAL_BODY = { message: 'Not found', code: 'NOT_FOUND', hint: 'no such endpoint' }; + +/** + * Boot the real stack: a real `LiteKernel` carrying an `auth` service in the + * shape the kernel really registers, and the adapter's own real + * `HttpDispatcher` behind `createHonoApp`. + * + * ⛔ No `ownsRoute` on the service: these are DISCLAIMED paths, which is what + * makes the mount yield instead of answering — the only way a `Response` gets + * into `HttpDispatcherResult.result` on this composition. + */ +async function bootApp(status: number, body: unknown) { + const seen: string[] = []; + const kernel = new LiteKernel(); + kernel.use({ + metadata: { name: 'test-auth-door', version: '1.0.0' }, + init: (c: any) => c.registerService('auth', { + handleRequest: async (req: Request) => { + seen.push(`${req.method} ${new URL(req.url).pathname}`); + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json', 'X-Door': 'auth' }, + }); + }, + }), + } as any); + await kernel.bootstrap(); + return { app: createHonoApp({ kernel: kernel as any, prefix: PREFIX }), seen }; +} + +describe('#16383: a dispatcher result that IS a Response survives the hono adapter intact', () => { + it('answers the door\'s REAL status and REAL body, not a hard-coded 200 with `{}`', async () => { + const { app, seen } = await bootApp(404, REFUSAL_BODY); + const res = await app.request(`http://localhost${PREFIX}/auth/me/permissions`); + + // The arm under test really ran — see the header. Asserted FIRST so a + // routing change that stops reaching `toResponse` reads as this row's + // failure and not as a passing status assertion. + expect(seen).toHaveLength(2); + + expect(res.status).toBe(404); + // ⭐ Both halves are load-bearing and the second is the one a weaker pin + // drops: a repair that answered SOME non-200 with a destroyed body would + // satisfy the status assertion alone while still puncturing every caller + // that reads the payload — `{}` is truthy. + await expect(res.clone().json()).resolves.toEqual(REFUSAL_BODY); + await expect(res.clone().text()).resolves.not.toBe('{}'); + // The producer's own headers are part of "the real Response", and + // rebuilding the body would drop them silently. + expect(res.headers.get('x-door')).toBe('auth'); + expect(res.headers.get('content-type')).toContain('application/json'); + }, 30_000); + + it('is the same answer on every disclaimed path under the mount', async () => { + for (const path of ['/auth', '/auth/', '/auth/whatever']) { + const { app, seen } = await bootApp(404, REFUSAL_BODY); + const res = await app.request(`http://localhost${PREFIX}${path}`); + expect(seen, path).toHaveLength(2); + expect(res.status, path).toBe(404); + await expect(res.json(), path).resolves.toEqual(REFUSAL_BODY); + } + }, 30_000); + + it('CONTROL — a 403 the mount answers ITSELF is untouched, and says so with one call', async () => { + // Anti-vacuity, two ways. It proves this harness can tell the two arms + // apart (one call, not two), so the rows above are not green by way of + // some path that never consults `toResponse`; and it proves the fix did + // not disturb the mount's own direct answer, which #15928 owns. + const { app, seen } = await bootApp(403, { message: 'nope', code: 'FORBIDDEN' }); + const res = await app.request(`http://localhost${PREFIX}/auth/me/permissions`); + + expect(seen).toHaveLength(1); + expect(res.status).toBe(403); + await expect(res.json()).resolves.toEqual({ message: 'nope', code: 'FORBIDDEN' }); + }, 30_000); +}); diff --git a/packages/qa/http-conformance/vitest.config.ts b/packages/qa/http-conformance/vitest.config.ts index 3afab41a0c..5cdd71c95f 100644 --- a/packages/qa/http-conformance/vitest.config.ts +++ b/packages/qa/http-conformance/vitest.config.ts @@ -1,8 +1,31 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { defineConfig } from 'vitest/config'; +import path from 'node:path'; export default defineConfig({ + resolve: { + // ARRAY form with an ANCHORED pattern, deliberately: a bare-string `find` + // matches by PREFIX, so a key whose replacement is a FILE also swallows + // that package's subpaths and resolves them to `…/index.ts/` — + // ENOTDIR at run time, in a config that reads as correct. + // `scripts/check-test-source-alias.mjs` is the authority on the rule. + alias: [ + // The SUBJECT of `hono-dispatcher-result-response.conformance.test.ts` is + // `createHonoApp` itself, so its verdict has to be about this checkout's + // adapter source and not about the last `pnpm build`. Through the + // package `exports` this specifier resolves to `packages/adapters/hono/dist`, + // and a stale `dist` there would leave that file green against the very + // rendering it exists to pin. Everything else it loads — + // `@objectstack/runtime`'s real `HttpDispatcher` above all — is + // deliberately left resolving normally: those are the REAL boot, not the + // subject. + { + find: /^@objectstack\/hono$/, + replacement: path.resolve(__dirname, '../../adapters/hono/src/index.ts'), + }, + ], + }, test: { // A late console.* must not redden a green suite (#10374): vitest's worker // forwards console output over RPC and discards the promise, and a write diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 81670053fb..b858513b14 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2107,6 +2107,9 @@ importers: '@objectstack/driver-sqlite-wasm': specifier: workspace:* version: link:../../drivers/driver-sqlite-wasm + '@objectstack/hono': + specifier: workspace:* + version: link:../../adapters/hono '@objectstack/objectql': specifier: workspace:* version: link:../../objectql