From 5889189e94b647f4f600e44ef70fbce72df0f33a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 02:33:28 +0000 Subject: [PATCH] =?UTF-8?q?fix(hono,plugin-hono-server,runtime):=20one=20C?= =?UTF-8?q?ORS=20source=20and=20one=20registry=20key=20=E2=80=94=20the=20l?= =?UTF-8?q?ast=20derivable=20copies=20from=20the=20#3786=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-ran the sweep across all 72 packages. The earlier pass globbed `packages/*/src`, one level deep, so it missed everything under `packages/plugins/` and `packages/adapters/` — "the sweep is basically clean" was based on an incomplete scan. A STALE CORS DEFAULT, on the one description callers actually read. `HonoCorsOptions.allowHeaders`' TSDoc promised `['Content-Type', 'Authorization', 'X-Requested-With']` "sufficient for cookie and bearer-token auth". The real default carries three more: `X-Tenant-ID`, `X-Environment-Id` (multi-tenant routing) and `If-Match` (OCC token on record PATCHes, objectui#2572). Sizing a custom allowHeaders against that sentence drops all three and breaks every cross-origin save. Three Hono CORS sites each kept their own copy under "keep in sync" comments. The copies agreed; the DOC — the only description with no counterpart to be diffed against, and the only one a caller reads — is what drifted. Both defaults are now single constants exported from plugin-hono-server and imported by the adapter (which already depends on it). The TSDoc links them instead of restating, and documents the asymmetry it never mentioned: allowHeaders REPLACES the default, exposeHeaders MERGES. hono-plugin.test.ts stopped stubbing ./adapter wholesale and keeps the real constants via importOriginal — it asserts exact header lists, so a mocked copy would make the test agree with itself rather than with what ships. Verified: removing `If-Match` fails `should allow If-Match by default`, by name. A HAND-COPIED REGISTRY KEY. runtime's share-links domain resolved 'shareLinks' as a literal, copied from SHARE_LINK_SERVICE — whose own comment says "keep in sync with the SharingPlugin registration". Now imports the constant. A drifted copy resolves nothing, so every share link 501s "Sharing is not configured for this environment" on an environment where it is. PLUS A DUPLICATE LEDGER ENTRY, the same defect one level up: check-generated.ts carried two NO_GENERATOR entries for check:strictness-ledger, because #4203 and #4252 each added one without seeing the other. Harmless (the ledger is read into a Set) but two comments told overlapping versions of one story. #4203's is kept — the fuller account, from the PR that fixed the underlying problem. NOT included, because main already has it: wiring the reconciliation into CI. #4203 landed it as `check:generated --reconcile-only` in lint.yml's unfiltered job, which is strictly better than what this branch first proposed — it runs only the reconciliation rather than re-running all eight artifact gates that already have their own steps, and it sits in an unfiltered required job so it cannot go dormant behind a paths filter. Checked and left alone: ApprovalStatus (5 values) and ApprovalActionKind (12 values) vs their plugin-approvals selects — diffed verbatim, no drift today, still hand-copied across a package boundary. Verified: 28/28 turbo test tasks green (plugin-hono-server 149, hono 73, runtime 67 files), affected builds clean, check:generated --reconcile-only PASS. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01UXGj3Z5TmwSV6RK2oGc3cb --- .changeset/sweep-close-out.md | 67 +++++++++++++++++++ .../docs/protocol/kernel/http-protocol.mdx | 24 ++++++- packages/adapters/hono/src/index.ts | 33 +++++---- .../plugins/plugin-hono-server/src/adapter.ts | 54 ++++++++++++--- .../src/hono-plugin.test.ts | 8 ++- .../plugin-hono-server/src/hono-plugin.ts | 30 ++++----- packages/runtime/src/domains/share-links.ts | 9 ++- packages/spec/scripts/check-generated.ts | 8 --- 8 files changed, 178 insertions(+), 55 deletions(-) create mode 100644 .changeset/sweep-close-out.md diff --git a/.changeset/sweep-close-out.md b/.changeset/sweep-close-out.md new file mode 100644 index 0000000000..85f96db1e4 --- /dev/null +++ b/.changeset/sweep-close-out.md @@ -0,0 +1,67 @@ +--- +"@objectstack/plugin-hono-server": patch +"@objectstack/hono": patch +"@objectstack/runtime": patch +--- + +fix(hono,plugin-hono-server,runtime): one CORS source and one registry key — the last derivable copies from the #3786 sweep + +Re-ran the sweep across all 72 packages. The earlier pass globbed `packages/*/src`, +which is one level deep, so it missed everything under `packages/plugins/` and +`packages/adapters/` — the "sweep is basically clean" report was based on an +incomplete scan. + +**A stale CORS default, on the one description callers actually read.** +`HonoCorsOptions.allowHeaders`' TSDoc promised +`['Content-Type', 'Authorization', 'X-Requested-With']` "which is sufficient for +cookie and bearer-token auth". The real default carries three more: +`X-Tenant-ID` and `X-Environment-Id` (multi-tenant routing) and `If-Match` (the +OCC token on record PATCHes, objectui#2572). Sizing a custom `allowHeaders` +against that sentence drops all three and every cross-origin save fails with +"Failed to fetch". + +The instructive part: **three** Hono CORS sites each carried their own copy of +the defaults under "keep in sync" comments, and the copies all agreed. What +drifted was the *doc* — the only description with no counterpart to be diffed +against, and the only one a caller reads. + +Both defaults are now single constants, `DEFAULT_CORS_ALLOW_HEADERS` and +`DEFAULT_CORS_EXPOSE_HEADERS`, exported from `@objectstack/plugin-hono-server` +and imported by the adapter (which already depends on it — no new edge). The +TSDoc links them rather than restating, and documents an asymmetry it never +mentioned: `allowHeaders` REPLACES the default, `exposeHeaders` MERGES with it. + +`hono-plugin.test.ts` stopped stubbing `./adapter` wholesale and keeps the real +constants via `importOriginal` — it asserts exact header lists, so a mocked copy +would make the test agree with itself rather than with what ships. Verified: +removing `If-Match` from the constant fails `should allow If-Match by default`, +by name. + +**A third copy, in the public protocol docs.** `content/docs/protocol/kernel/ +http-protocol.mdx` advertised `Access-Control-Allow-Headers: Authorization, +Content-Type` — two of the six — and methods missing `PUT` and `HEAD`, with no +mention of the exposed headers at all. That is the copy an integrator builds a +client against: reading it, you would not know `If-Match` is permitted (so you +would not attempt OCC) or that `set-auth-token` is readable (so a rotated +session would look like a bug). Corrected, with the three non-obvious allowed +headers and the two exposed ones explained, and a pointer to the constants as +the source of truth. + +**A hand-copied service-registry key.** `runtime`'s share-links domain resolved +`'shareLinks'` as a string literal, copied from `SHARE_LINK_SERVICE` — whose own +doc-comment says "keep in sync with the SharingPlugin registration". It now +imports the constant. A drifted copy resolves nothing, so every share link +answers 501 "Sharing is not configured for this environment" on an environment +where it is configured perfectly well. + +**Plus a duplicate ledger entry**, which is the same defect one level up: +`check-generated.ts` carried two `NO_GENERATOR` entries for +`check:strictness-ledger`, because #4203 and #4252 each added one without seeing +the other. Functionally harmless (the ledger is read into a `Set`) but it leaves +two comments telling overlapping versions of the same story. #4203's is kept — +it is the more complete account and it is the PR that fixed the underlying +problem. + +Checked and deliberately left alone: `ApprovalStatus` (5 values) and +`ApprovalActionKind` (12 values) versus their `plugin-approvals` selects — diffed +verbatim, no drift today, still hand-copied across a package boundary. diff --git a/content/docs/protocol/kernel/http-protocol.mdx b/content/docs/protocol/kernel/http-protocol.mdx index 7627861d59..291cbc8567 100644 --- a/content/docs/protocol/kernel/http-protocol.mdx +++ b/content/docs/protocol/kernel/http-protocol.mdx @@ -791,11 +791,31 @@ ObjectStack sends CORS headers automatically: ```http Access-Control-Allow-Origin: https://app.acme.com -Access-Control-Allow-Methods: GET, POST, PATCH, DELETE, OPTIONS -Access-Control-Allow-Headers: Authorization, Content-Type +Access-Control-Allow-Methods: GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS +Access-Control-Allow-Headers: Content-Type, Authorization, X-Requested-With, X-Tenant-ID, X-Environment-Id, If-Match +Access-Control-Expose-Headers: set-auth-token, x-objectstack-dropped-fields Access-Control-Max-Age: 86400 ``` +Three of the allowed request headers are easy to overlook, and each one disables +a feature if an intermediate proxy strips it: + +| Header | Why it is allowed | +| --- | --- | +| `X-Tenant-ID` / `X-Environment-Id` | Route the request to its environment on a multi-tenant host. | +| `If-Match` | Carries the OCC token on record `PATCH`es. Without it, a cross-origin save fails in the browser with "Failed to fetch". | + +The two **exposed** response headers matter to browser clients specifically: +`set-auth-token` delivers a rotated session token (without it a cross-origin +session silently breaks even though every request succeeds), and +`x-objectstack-dropped-fields` warns that a write dropped undeclared keys — the +response body's `droppedFields` stays the primary channel for that. + +These are the defaults exported as `DEFAULT_CORS_ALLOW_HEADERS` and +`DEFAULT_CORS_EXPOSE_HEADERS` from `@objectstack/plugin-hono-server`. Supplying +`allowHeaders` **replaces** the default; supplying `exposeHeaders` **merges** +with it. + **Preflight request:** ```http OPTIONS /api/v1/data/account diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index c83049fcdd..a59cfcd71b 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -39,7 +39,12 @@ export type KernelManager = any; */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export type EnvironmentDriverRegistry = any; -import { createOriginMatcher, hasWildcardPattern } from '@objectstack/plugin-hono-server'; +import { + createOriginMatcher, + hasWildcardPattern, + DEFAULT_CORS_ALLOW_HEADERS, + DEFAULT_CORS_EXPOSE_HEADERS, +} from '@objectstack/plugin-hono-server'; export interface ObjectStackHonoCorsOptions { /** Enable or disable CORS. Defaults to true. */ @@ -172,30 +177,24 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { origin = configuredOrigin; } - // Always include `set-auth-token` in exposed headers so that the - // better-auth `bearer()` plugin (registered by plugin-auth) can - // deliver rotated session tokens to cross-origin clients. Without - // this, browsers strip the header from every response, the client - // never sees the new token, and cross-origin sessions silently - // break even when preflight and the actual request both succeed. + // Both CORS defaults are imported from `@objectstack/plugin-hono-server`, + // which this package already depends on (#3786). The three Hono-based CORS + // sites used to carry their own copies under "keep in sync" comments — this + // one included, right down to a duplicate of the rationale below. // - // This mirrors `plugin-hono-server`'s CORS wiring — all three - // Hono-based CORS sites must stay in lockstep on this default. - // `x-objectstack-dropped-fields` (#3455) lets a cross-origin browser read - // the single-write drop header (#3431); the body `droppedFields` channel is - // the primary, cross-origin-safe surface, so this is a convenience mirror. - const defaultExposeHeaders = ['set-auth-token', 'x-objectstack-dropped-fields']; + // `set-auth-token` is the load-bearing one to understand: without it in the + // exposed set, browsers strip the header from every response, the client + // never sees its rotated session token, and cross-origin sessions silently + // break even though preflight and the request both succeed. const exposeHeaders = Array.from(new Set([ - ...defaultExposeHeaders, + ...DEFAULT_CORS_EXPOSE_HEADERS, ...(corsOpts.exposeHeaders ?? []), ])); app.use('*', cors({ origin: origin as any, allowMethods: corsOpts.methods || ['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'HEAD', 'OPTIONS'], - // Keep in sync with plugin-hono-server's defaultAllowHeaders — `If-Match` - // carries the OCC token on cross-origin record PATCHes (objectui#2572). - allowHeaders: corsOpts.allowHeaders || ['Content-Type', 'Authorization', 'X-Requested-With', 'X-Tenant-ID', 'X-Environment-Id', 'If-Match'], + allowHeaders: corsOpts.allowHeaders || [...DEFAULT_CORS_ALLOW_HEADERS], exposeHeaders, credentials, maxAge, diff --git a/packages/plugins/plugin-hono-server/src/adapter.ts b/packages/plugins/plugin-hono-server/src/adapter.ts index ef0892f52f..20c3be769c 100644 --- a/packages/plugins/plugin-hono-server/src/adapter.ts +++ b/packages/plugins/plugin-hono-server/src/adapter.ts @@ -14,6 +14,45 @@ import { serve } from '@hono/node-server'; import { serveStatic } from '@hono/node-server/serve-static'; import { matchesRoutePattern } from './route-pattern'; +/** + * Request headers allowed on preflight, by default. + * + * **The** default — three Hono-based CORS sites apply it (this package's + * `hono-plugin.ts` and the `@objectstack/hono` adapter, which depends on this + * package), and they used to each carry their own copy under "keep in sync" + * comments. The copies happened to agree; the TSDoc on {@link + * HonoCorsOptions.allowHeaders} did not — it had been three headers behind for + * long enough to predate multi-tenant routing, so the one description a caller + * actually reads was the one that drifted (#3786). + * + * `X-Tenant-ID` / `X-Environment-Id` route a request to its environment. + * `If-Match` carries the OCC token on record PATCHes (objectui's inline edit, + * REST `update` with `ifMatch`) — without it in the preflight allow-list every + * cross-origin save fails in the browser with "Failed to fetch" (objectui#2572). + */ +export const DEFAULT_CORS_ALLOW_HEADERS: readonly string[] = Object.freeze([ + 'Content-Type', + 'Authorization', + 'X-Requested-With', + 'X-Tenant-ID', + 'X-Environment-Id', + 'If-Match', +]); + +/** + * Response headers exposed to cross-origin JS, by default. Same three sites, + * same reason as {@link DEFAULT_CORS_ALLOW_HEADERS}. + * + * `set-auth-token` lets better-auth's `bearer()` plugin hand rotated session + * tokens to cross-origin clients (see plugin-auth). `x-objectstack-dropped-fields` + * (#3455) exposes the single-write drop warning (#3431); the body `droppedFields` + * channel remains the primary, cross-origin-safe surface. + */ +export const DEFAULT_CORS_EXPOSE_HEADERS: readonly string[] = Object.freeze([ + 'set-auth-token', + 'x-objectstack-dropped-fields', +]); + export interface HonoCorsOptions { enabled?: boolean; origins?: string | string[]; @@ -21,19 +60,18 @@ export interface HonoCorsOptions { /** * Request headers allowed on preflight (`Access-Control-Allow-Headers`). * - * Defaults to `['Content-Type', 'Authorization', 'X-Requested-With']`, - * which is sufficient for cookie and bearer-token auth. + * Defaults to {@link DEFAULT_CORS_ALLOW_HEADERS} — deliberately a link and + * not a restatement. Supplying this REPLACES the default rather than + * extending it, so spread the constant if you only mean to add: + * `allowHeaders: [...DEFAULT_CORS_ALLOW_HEADERS, 'X-My-Header']`. */ allowHeaders?: string[]; /** * Response headers exposed to JS (`Access-Control-Expose-Headers`). * - * Defaults to `['set-auth-token', 'x-objectstack-dropped-fields']` so that - * better-auth's `bearer()` plugin can hand rotated session tokens to - * cross-origin clients, and a browser can read the single-write - * `X-ObjectStack-Dropped-Fields` warning header (#3431/#3455). User-supplied - * values are merged with these defaults — they are always exposed unless CORS - * is disabled entirely. + * Defaults to {@link DEFAULT_CORS_EXPOSE_HEADERS}. Unlike `allowHeaders`, + * user-supplied values are MERGED with the default — those are always + * exposed unless CORS is disabled entirely. */ exposeHeaders?: string[]; credentials?: boolean; diff --git a/packages/plugins/plugin-hono-server/src/hono-plugin.test.ts b/packages/plugins/plugin-hono-server/src/hono-plugin.test.ts index 47fa61a7f3..56cc4d78e1 100644 --- a/packages/plugins/plugin-hono-server/src/hono-plugin.test.ts +++ b/packages/plugins/plugin-hono-server/src/hono-plugin.test.ts @@ -15,7 +15,13 @@ vi.mock('@hono/node-server/serve-static', () => ({ serveStatic: vi.fn(() => (c: any, next: any) => next()) })); -vi.mock('./adapter', () => ({ +// PARTIAL mock: only `HonoHttpServer` is replaced. The CORS default constants +// are deliberately kept REAL via `importOriginal` — they are the single source +// this plugin and the `@objectstack/hono` adapter both read (#3786), and the +// assertions below check exact header lists, so stubbing them would make this +// file agree with itself instead of with the shipped defaults. +vi.mock('./adapter', async (importOriginal) => ({ + ...(await importOriginal()), HonoHttpServer: vi.fn(function() { return { mount: vi.fn(), diff --git a/packages/plugins/plugin-hono-server/src/hono-plugin.ts b/packages/plugins/plugin-hono-server/src/hono-plugin.ts index 718af23a43..46021ec460 100644 --- a/packages/plugins/plugin-hono-server/src/hono-plugin.ts +++ b/packages/plugins/plugin-hono-server/src/hono-plugin.ts @@ -12,7 +12,12 @@ import { makeExecutionContextResolver, registerCurrentUserEndpoints, } from './current-user-endpoints'; -import { HonoHttpServer, HonoCorsOptions } from './adapter'; +import { + HonoHttpServer, + HonoCorsOptions, + DEFAULT_CORS_ALLOW_HEADERS, + DEFAULT_CORS_EXPOSE_HEADERS, +} from './adapter'; import { cors } from 'hono/cors'; import { serveStatic } from '@hono/node-server/serve-static'; import * as fs from 'fs'; @@ -420,24 +425,13 @@ export class HonoServerPlugin implements Plugin { } const rawApp = this.server.getRawApp(); - // Always include `set-auth-token` in exposed headers so that - // the better-auth `bearer()` plugin can deliver rotated - // session tokens to cross-origin clients (see plugin-auth). - // User-supplied exposeHeaders are merged with this default. - // `If-Match` carries the OCC token on record PATCHes (objectui's - // record-level inline edit, REST `update` with `ifMatch`) — without - // it in the preflight allow-list, every cross-origin save fails in - // the browser with "Failed to fetch" (objectui#2572 dogfood find; - // same split-origin class as the #2548 Bearer fixes). - const defaultAllowHeaders = ['Content-Type', 'Authorization', 'X-Requested-With', 'X-Tenant-ID', 'X-Environment-Id', 'If-Match']; - // `x-objectstack-dropped-fields` (#3455): expose the single-write - // drop header (#3431) to cross-origin JS. Kept in lockstep with the - // `@objectstack/hono` adapter's default. The body `droppedFields` - // channel remains the primary, cross-origin-safe surface. - const defaultExposeHeaders = ['set-auth-token', 'x-objectstack-dropped-fields']; - const allowHeaders = corsOpts.allowHeaders ?? defaultAllowHeaders; + // Both defaults come from `adapter.ts` (#3786). They used to be + // spelled out here AND in the `@objectstack/hono` adapter, each + // under a "keep in sync" comment; the per-entry rationale now + // lives on the constants themselves. + const allowHeaders = corsOpts.allowHeaders ?? [...DEFAULT_CORS_ALLOW_HEADERS]; const exposeHeaders = Array.from(new Set([ - ...defaultExposeHeaders, + ...DEFAULT_CORS_EXPOSE_HEADERS, ...(corsOpts.exposeHeaders ?? []), ])); diff --git a/packages/runtime/src/domains/share-links.ts b/packages/runtime/src/domains/share-links.ts index 0c1ebd344f..b72c87da09 100644 --- a/packages/runtime/src/domains/share-links.ts +++ b/packages/runtime/src/domains/share-links.ts @@ -35,6 +35,8 @@ * value whichever one served the request. */ +import { SHARE_LINK_SERVICE } from '@objectstack/spec/contracts'; + import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -59,7 +61,12 @@ export async function handleShareLinksRequest( // [#4127 batch 3] `plugin-sharing` registers `ShareLinkService`, which // declares `implements IShareLinkService`; the four methods called below // were all already on that contract. Only the ledger entry was missing. - const svc = await deps.resolveService('shareLinks', context.environmentId); + // The registry key comes from the contract that DEFINES it (#3786). This was + // a second hand-written `'shareLinks'`, copied from a constant whose own + // doc-comment says "keep in sync with the SharingPlugin registration" — and a + // drifted copy here resolves nothing, so every share link 501s with "Sharing + // is not configured for this environment" on an environment where it is. + const svc = await deps.resolveService(SHARE_LINK_SERVICE, context.environmentId); if (!svc) { return { handled: true, response: deps.error('Sharing is not configured for this environment', 501) }; } diff --git a/packages/spec/scripts/check-generated.ts b/packages/spec/scripts/check-generated.ts index 2dc7c0e3db..0302cd58ce 100644 --- a/packages/spec/scripts/check-generated.ts +++ b/packages/spec/scripts/check-generated.ts @@ -85,14 +85,6 @@ const NO_GENERATOR: ReadonlyArray<{ check: string; why: string }> = [ check: 'check:exported-any', why: 'audits the built .d.ts for exported types/schemas that resolve to `any` — no artifact (needs a fresh `pnpm build`)', }, - // Same cross-PR race as `check:variant-docs` above, now for the second time: - // landed in #4232 with no entry here, so `main` again carries an unclassified - // script and this reconciliation fails on `main` itself. The ledger it audits - // is hand-written prose — there is no generator to name. - { - check: 'check:strictness-ledger', - why: 'audits the hand-written #4001 strictness ledger against the z.object sites in src — no artifact', - }, ]; /**