diff --git a/.changeset/dashboard-service-gate-default-path.md b/.changeset/dashboard-service-gate-default-path.md new file mode 100644 index 0000000000..8227700f7e --- /dev/null +++ b/.changeset/dashboard-service-gate-default-path.md @@ -0,0 +1,30 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): dashboard 组件门禁在默认配置下真正执行 (#5881) + +ADR-0057 D10 的 `requiresService` 组件门禁 —— 剔除指向未注册可选服务的 dashboard +磁贴 —— 在默认部署里一次都没跑过。`GET /meta/:type/:name` 的单条读取有一条缓存分支, +它排除了 `app`(per-user RBAC 过滤)与 `doc` / `book`(per-caller audience),唯独没有 +排除 `dashboard`;而 `enableCache` 默认为 `true`。门禁写在非缓存分支里,于是只有显式 +关掉缓存的部署才会执行到它。 + +后果正是该 ADR 点名要防的那一幕:在没有某个可选服务的部署里(比如单租户运行时里的 +Organizations KPI,其 `org-scoping` 服务不存在),console 会渲染一块绑定到缺失服务的 +死磁贴 —— 尽管服务端的门禁代码在、测试也在。 + +**修复**:`dashboard` 与 `app` 同款,从缓存分支排除,两种拼写(`/meta/dashboard/x` +与规范复数 `/meta/dashboards/x`)都覆盖。其它元数据类型的 ETag 快路径不受影响。 + +**为什么不是"把门禁提到分支之外、两条路径共用"** —— 那读起来更整齐,但 ETag 无法承载 +门禁结论:validator 是**未过滤文档**的哈希,而 `notModified` 在 protocol 内部就已判定, +REST 层没有机会重判。共用之后送出的就是"过滤过的正文 + 指向未过滤正文的 validator"。 +一次 boot 之内这没有危害(已注册服务集在 bootstrap 之后不可变),但 `Cache-Control: +private, no-cache` 意味着客户端**存下正文**、之后只做重验证,而存下的正文比进程活得久: +一次关掉该可选服务的重新部署并不改变文档,ETag 不变 ⇒ 每次重验证都回 304 ⇒ 那块死磁贴 +恰好在移除其服务的那次部署之后被永久缓存下来。放弃快路径的代价则接近于零: +`getMetaItemCached` 本就委托给 `getMetaItem`,服务端两条路做的是同样的工作,失去的只是 +304 省下的正文字节。 + +对调用方的可见变化:dashboard 的单条读取不再返回 ETag / 304,每次都是完整的 200。 diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 095868c815..bc07aaf9f2 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -4246,7 +4246,48 @@ export class RestServer { // `doc` and `book` bypass the shared cache: their §6.7 // audience gate is per-caller, and a shared ETag would // leak gated content across viewers. - if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDraftRead && !previewDrafts && !packageScoped && req.params.type !== 'doc' && req.params.type !== 'book') { + // + // [#5881] `dashboard` bypasses it too, and the reason is + // NOT the one above — worth writing down, because the + // obvious reading says a dashboard needn't bypass at all. + // Its ADR-0057 D10 widget gate (`filterDashboardForUser`, + // below) is per-DEPLOYMENT — it asks which optional kernel + // services are registered — never per-caller, so there is + // no cross-viewer leak to avoid. What rules out sharing + // the cached path is the validator itself: the ETag is + // `simpleHash(locale + JSON.stringify(item))` over the + // UNFILTERED document (metadata-protocol `getMetaItemCached`), + // so it cannot express the gate dimension at all, and + // `notModified` is decided inside the protocol before this + // layer could re-judge it. Gating the cached body would + // therefore ship a filtered body under a validator that + // identifies the unfiltered one. + // + // That mismatch is not academic, because the two have + // different lifetimes. Within one boot the registered-service + // set is fixed (`Kernel.use()` throws once bootstrap has + // started, and no deregistration API exists), so the gate + // verdict is stable per process — but `Cache-Control: + // private, no-cache` means the client STORES the body and + // revalidates, and that stored body outlives the process. + // A redeploy that turns the optional service off does not + // change the document, so the ETag is unchanged, every + // revalidation answers 304, and the stale unfiltered body + // stands: the dead tile D10 exists to prevent, now cached + // indefinitely. Bypassing costs nothing to weigh against + // that — `getMetaItemCached` delegates to `getMetaItem`, + // so the server does identical work either way and only + // the 304's saved body bytes are given up. + // + // Compared on the NORMALIZED type, like `isAppType` and + // unlike the two literals at the end of this condition + // (`/meta/dashboards/x` is the canonical plural spelling + // under Prime Directive #3, and an exclusion it could be + // spelled around would not be an exclusion). The `doc` / + // `book` literals have exactly that hole — measured and + // filed as #6241, deliberately not fixed here. + const isDashboardType = RestServer.metaTypeSingular(req.params.type) === 'dashboard'; + if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDashboardType && !isDraftRead && !previewDrafts && !packageScoped && req.params.type !== 'doc' && req.params.type !== 'book') { const cacheRequest = { ifNoneMatch: req.headers['if-none-match'] as string, ifModifiedSince: req.headers['if-modified-since'] as string, @@ -4367,7 +4408,18 @@ export class RestServer { // ADR-0057 D10: gate dashboard widgets by `requiresService` // (mirrors the app-nav gate above) so the console never // renders a tile bound to an absent optional service. - if (RestServer.metaTypeSingular(req.params.type) === 'dashboard' && visible) { + // + // [#5881] This is now on the DEFAULT path. It reads as + // ordinary code either way, which is exactly why the + // defect was invisible: `enableCache` defaults to true + // and `dashboard` was not excluded above, so every + // default deployment took the cached branch and this + // gate ran only where an operator had turned the cache + // off. Declared, tested, and never executed in + // production — the exclusion above is what makes the + // ADR's "the server is the authoritative gate" true + // rather than merely written down. + if (isDashboardType && visible) { const ctx = await this.resolveExecCtx(environmentId, req).catch(() => undefined); const registered = await this.resolveRegisteredServices((ctx as any)?.__kernel, [visible]); const serviceGate = registered ? (n: string) => registered.has(n) : undefined; diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 5fc929a410..5a6114d727 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -3375,9 +3375,12 @@ describe('filterDashboardForUser — ADR-0057 D10 widget requiresService gate', const rest: any = new RestServer( createMockServer() as any, protocol, - // The widget gate lives on the uncached read; the cached branch — which - // is the DEFAULT — does not run it at all (pre-existing and unrelated to - // #5563, filed as #5881), so this pins the gate where it exists. + // [#5881] `enableCache: false` is no longer what makes this reachable — + // dashboard reads bypass the cache unconditionally now, and the DEFAULT + // configuration is pinned separately below. Kept explicit because an + // operator who disables the cache must get the same answer, and because + // this case is what the fix had to leave untouched: it was the only + // green proof the gate worked at all while the default path skipped it. { api: { requireAuth: false }, metadata: { enableCache: false } } as any, ); rest.resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: [] }); @@ -3395,6 +3398,114 @@ describe('filterDashboardForUser — ADR-0057 D10 widget requiresService gate', expect(ids(body.item)).toContain('widget_total_users'); }); + // ------------------------------------------------------------------------- + // [#5881] The gate on the DEFAULT path. + // + // `enableCache` defaults to true, and the single-item read had a cached + // branch that excluded `app` (per-user RBAC) and `doc`/`book` (per-caller + // audience) but NOT `dashboard` — so the widget gate above, which lives in + // the uncached branch, never ran in a default deployment. Measured on + // `origin/main` @ 8e2bbba24 before the fix, against the real RestServer: + // + // cachedCalls: 1 | uncachedCalls: 0 | widgetsServed: ["w_users","w_orgs"] + // + // The fix excludes `dashboard` from the cached branch, as `app` already was. + // Why not instead hoist the gate so both branches run it once — which reads + // like the tidier shape? Because the ETag cannot carry the gate's verdict: + // it is a hash of the UNFILTERED document, and `notModified` is decided + // inside the protocol before this layer sees it. Measured, same baseline: + // + // etag(unfiltered): 2504e71e | etag(gated body): 75ca17c1 | same? false + // revalidate-with-unfiltered-etag -> notModified: true + // cacheControl: {"directives":["private","no-cache"]} + // + // So a hoisted gate would ship a filtered body under a validator naming the + // unfiltered one. Within one boot that is harmless (the registered-service + // set cannot change: `Kernel.use()` throws after bootstrap and nothing + // deregisters), but `private, no-cache` means the client stores the body and + // only revalidates — and the stored body outlives the process. Turn the + // optional service off in a redeploy and the document is unchanged, so every + // revalidation answers 304 and the dead tile survives the very deploy that + // removed its service. The full-read cost of not caching is nil: + // `getMetaItemCached` delegates to `getMetaItem`, so the server does the same + // work either way and only the 304's saved bytes are given up. + // ------------------------------------------------------------------------- + /** A protocol offering BOTH reads, so the branch choice is the thing tested. */ + const bothReads = () => { + const protocol: any = createMockProtocol(); + protocol.getMetaItem = vi.fn().mockResolvedValue({ + type: 'dashboard', name: 'system_overview', item: dash(), + }); + protocol.getMetaItemCached = vi.fn().mockResolvedValue({ + data: dash(), + etag: { value: 'etag-unfiltered', weak: false }, + lastModified: new Date().toISOString(), + cacheControl: { directives: ['private', 'no-cache'] }, + notModified: false, + }); + return protocol; + }; + /** Default config — no `metadata` block at all, so `enableCache` is its default. */ + const defaultServer = (protocol: any) => { + const rest: any = new RestServer(createMockServer() as any, protocol, ANON_API as any); + rest.resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: [] }); + rest.serviceExistsProvider = (n: string) => n !== 'org-scoping'; + rest.registerRoutes(); + return rest; + }; + const readMeta = async (rest: any, type: string, name: string) => { + const route = rest.getRoutes().find( + (r: any) => r.method === 'GET' && r.path === '/api/v1/meta/:type/:name', + ); + const res = { json: vi.fn(), status: vi.fn().mockReturnThis(), header: vi.fn(), send: vi.fn() }; + await route.handler({ params: { type, name }, query: {}, headers: {} }, res); + return res; + }; + + it('runs the gate under the DEFAULT configuration, not only when the cache is off', async () => { + const protocol = bothReads(); + const res = await readMeta(defaultServer(protocol), 'dashboard', 'system_overview'); + + const body = res.json.mock.calls.at(-1)![0]; + expect(ids(body.item)).not.toContain('widget_organizations'); + expect(ids(body.item)).toContain('widget_total_users'); + // The envelope the route owes its caller is unchanged by the branch switch. + expect(body).toMatchObject({ type: 'dashboard', name: 'system_overview' }); + }); + + it('a dashboard read takes the uncached branch — while other types still cache', async () => { + // The positive control matters as much as the assertion above it: excluding + // one type from the cached branch is only correct if it excludes exactly + // that type. A fix that disabled the cache wholesale would satisfy every + // gate assertion here and quietly cost every other metadata read its ETag. + const protocol = bothReads(); + const rest = defaultServer(protocol); + + const dashRes = await readMeta(rest, 'dashboard', 'system_overview'); + expect(protocol.getMetaItemCached).not.toHaveBeenCalled(); + expect(protocol.getMetaItem).toHaveBeenCalledTimes(1); + // The observable price of the bypass, pinned rather than hidden: a + // dashboard read carries no ETag validator now. + expect(dashRes.header.mock.calls.map((c: any[]) => c[0])).not.toContain('ETag'); + + const viewRes = await readMeta(rest, 'view', 'account_list'); + expect(protocol.getMetaItemCached).toHaveBeenCalledTimes(1); + expect(viewRes.header.mock.calls.map((c: any[]) => c[0])).toContain('ETag'); + }); + + it('the plural spelling cannot spell its way around the exclusion', async () => { + // `/meta/dashboards/x` is the CANONICAL REST spelling (Prime Directive #3) + // and the route serves both, so an exclusion compared against the literal + // `'dashboard'` would leave the default path ungated under the spelling + // most callers use — the #3984 defect class, which `metaTypeSingular`'s own + // docstring exists to remember. + const protocol = bothReads(); + const res = await readMeta(defaultServer(protocol), 'dashboards', 'system_overview'); + + expect(protocol.getMetaItemCached).not.toHaveBeenCalled(); + expect(ids(res.json.mock.calls.at(-1)![0].item)).not.toContain('widget_organizations'); + }); + it('resolveRegisteredServices discovers requiresService declared on widgets', async () => { const rest: any = make(); const kernel = { getServiceAsync: async (n: string) => { if (n === 'org-scoping') return {}; throw new Error('absent'); } };