Skip to content

Commit 4834e4f

Browse files
committed
fix(rest): run the ADR-0057 D10 dashboard widget gate on the default read path (#5881)
The `requiresService` widget gate — which strips dashboard tiles bound to an optional kernel service that is not registered — never ran in a default deployment. `GET /meta/:type/:name` has a cached branch that excludes `app` (per-user RBAC) and `doc`/`book` (per-caller audience) but not `dashboard`, and `enableCache` defaults to true; the gate lives in the uncached branch, so only a deployment that had explicitly disabled the cache reached it. Measured on origin/main @ 8e2bbba, against the real RestServer: cachedCalls: 1 | uncachedCalls: 0 | widgetsServed: ["w_users","w_orgs"] `dashboard` now bypasses the cached branch as `app` already did, compared on the normalized type so the canonical plural spelling cannot route around it. Hoisting the gate to a shared exit instead was measured and rejected: the ETag is a hash of the UNFILTERED document and `notModified` is decided inside the protocol, so a shared exit would ship a filtered body under a validator naming the unfiltered one. etag(unfiltered): 2504e71e | etag(gated body): 75ca17c1 | same? false revalidate-with-unfiltered-etag -> notModified: true cacheControl: {"directives":["private","no-cache"]} Within one boot that is harmless (the registered-service set cannot change: `Kernel.use()` throws after bootstrap and nothing deregisters), but the client stores the body and only revalidates, and the stored body outlives the process — so a redeploy that turns the service off leaves the document unchanged, every revalidation answers 304, and the dead tile survives the deploy that removed its service. Giving up the fast path costs nothing measurable: `getMetaItemCached` delegates to `getMetaItem`, so the server does identical work either way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wbxm29qPKnLf44AbSxizqW
1 parent 8e2bbba commit 4834e4f

3 files changed

Lines changed: 198 additions & 5 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
"@objectstack/rest": patch
3+
---
4+
5+
fix(rest): dashboard 组件门禁在默认配置下真正执行 (#5881)
6+
7+
ADR-0057 D10 的 `requiresService` 组件门禁 —— 剔除指向未注册可选服务的 dashboard
8+
磁贴 —— 在默认部署里一次都没跑过。`GET /meta/:type/:name` 的单条读取有一条缓存分支,
9+
它排除了 `app`(per-user RBAC 过滤)与 `doc` / `book`(per-caller audience),唯独没有
10+
排除 `dashboard`;而 `enableCache` 默认为 `true`。门禁写在非缓存分支里,于是只有显式
11+
关掉缓存的部署才会执行到它。
12+
13+
后果正是该 ADR 点名要防的那一幕:在没有某个可选服务的部署里(比如单租户运行时里的
14+
Organizations KPI,其 `org-scoping` 服务不存在),console 会渲染一块绑定到缺失服务的
15+
死磁贴 —— 尽管服务端的门禁代码在、测试也在。
16+
17+
**修复**:`dashboard``app` 同款,从缓存分支排除,两种拼写(`/meta/dashboard/x`
18+
与规范复数 `/meta/dashboards/x`)都覆盖。其它元数据类型的 ETag 快路径不受影响。
19+
20+
**为什么不是"把门禁提到分支之外、两条路径共用"** —— 那读起来更整齐,但 ETag 无法承载
21+
门禁结论:validator**未过滤文档**的哈希,而 `notModified` 在 protocol 内部就已判定,
22+
REST 层没有机会重判。共用之后送出的就是"过滤过的正文 + 指向未过滤正文的 validator"。
23+
一次 boot 之内这没有危害(已注册服务集在 bootstrap 之后不可变),但 `Cache-Control:
24+
private, no-cache` 意味着客户端**存下正文**、之后只做重验证,而存下的正文比进程活得久:
25+
一次关掉该可选服务的重新部署并不改变文档,ETag 不变 ⇒ 每次重验证都回 304 ⇒ 那块死磁贴
26+
恰好在移除其服务的那次部署之后被永久缓存下来。放弃快路径的代价则接近于零:
27+
`getMetaItemCached` 本就委托给 `getMetaItem`,服务端两条路做的是同样的工作,失去的只是
28+
304 省下的正文字节。
29+
30+
对调用方的可见变化:dashboard 的单条读取不再返回 ETag / 304,每次都是完整的 200。

packages/rest/src/rest-server.ts

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4246,7 +4246,48 @@ export class RestServer {
42464246
// `doc` and `book` bypass the shared cache: their §6.7
42474247
// audience gate is per-caller, and a shared ETag would
42484248
// leak gated content across viewers.
4249-
if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDraftRead && !previewDrafts && !packageScoped && req.params.type !== 'doc' && req.params.type !== 'book') {
4249+
//
4250+
// [#5881] `dashboard` bypasses it too, and the reason is
4251+
// NOT the one above — worth writing down, because the
4252+
// obvious reading says a dashboard needn't bypass at all.
4253+
// Its ADR-0057 D10 widget gate (`filterDashboardForUser`,
4254+
// below) is per-DEPLOYMENT — it asks which optional kernel
4255+
// services are registered — never per-caller, so there is
4256+
// no cross-viewer leak to avoid. What rules out sharing
4257+
// the cached path is the validator itself: the ETag is
4258+
// `simpleHash(locale + JSON.stringify(item))` over the
4259+
// UNFILTERED document (metadata-protocol `getMetaItemCached`),
4260+
// so it cannot express the gate dimension at all, and
4261+
// `notModified` is decided inside the protocol before this
4262+
// layer could re-judge it. Gating the cached body would
4263+
// therefore ship a filtered body under a validator that
4264+
// identifies the unfiltered one.
4265+
//
4266+
// That mismatch is not academic, because the two have
4267+
// different lifetimes. Within one boot the registered-service
4268+
// set is fixed (`Kernel.use()` throws once bootstrap has
4269+
// started, and no deregistration API exists), so the gate
4270+
// verdict is stable per process — but `Cache-Control:
4271+
// private, no-cache` means the client STORES the body and
4272+
// revalidates, and that stored body outlives the process.
4273+
// A redeploy that turns the optional service off does not
4274+
// change the document, so the ETag is unchanged, every
4275+
// revalidation answers 304, and the stale unfiltered body
4276+
// stands: the dead tile D10 exists to prevent, now cached
4277+
// indefinitely. Bypassing costs nothing to weigh against
4278+
// that — `getMetaItemCached` delegates to `getMetaItem`,
4279+
// so the server does identical work either way and only
4280+
// the 304's saved body bytes are given up.
4281+
//
4282+
// Compared on the NORMALIZED type, like `isAppType` and
4283+
// unlike the two literals at the end of this condition
4284+
// (`/meta/dashboards/x` is the canonical plural spelling
4285+
// under Prime Directive #3, and an exclusion it could be
4286+
// spelled around would not be an exclusion). The `doc` /
4287+
// `book` literals have exactly that hole — measured and
4288+
// filed as #6241, deliberately not fixed here.
4289+
const isDashboardType = RestServer.metaTypeSingular(req.params.type) === 'dashboard';
4290+
if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDashboardType && !isDraftRead && !previewDrafts && !packageScoped && req.params.type !== 'doc' && req.params.type !== 'book') {
42504291
const cacheRequest = {
42514292
ifNoneMatch: req.headers['if-none-match'] as string,
42524293
ifModifiedSince: req.headers['if-modified-since'] as string,
@@ -4367,7 +4408,18 @@ export class RestServer {
43674408
// ADR-0057 D10: gate dashboard widgets by `requiresService`
43684409
// (mirrors the app-nav gate above) so the console never
43694410
// renders a tile bound to an absent optional service.
4370-
if (RestServer.metaTypeSingular(req.params.type) === 'dashboard' && visible) {
4411+
//
4412+
// [#5881] This is now on the DEFAULT path. It reads as
4413+
// ordinary code either way, which is exactly why the
4414+
// defect was invisible: `enableCache` defaults to true
4415+
// and `dashboard` was not excluded above, so every
4416+
// default deployment took the cached branch and this
4417+
// gate ran only where an operator had turned the cache
4418+
// off. Declared, tested, and never executed in
4419+
// production — the exclusion above is what makes the
4420+
// ADR's "the server is the authoritative gate" true
4421+
// rather than merely written down.
4422+
if (isDashboardType && visible) {
43714423
const ctx = await this.resolveExecCtx(environmentId, req).catch(() => undefined);
43724424
const registered = await this.resolveRegisteredServices((ctx as any)?.__kernel, [visible]);
43734425
const serviceGate = registered ? (n: string) => registered.has(n) : undefined;

packages/rest/src/rest.test.ts

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3375,9 +3375,12 @@ describe('filterDashboardForUser — ADR-0057 D10 widget requiresService gate',
33753375
const rest: any = new RestServer(
33763376
createMockServer() as any,
33773377
protocol,
3378-
// The widget gate lives on the uncached read; the cached branch — which
3379-
// is the DEFAULT — does not run it at all (pre-existing and unrelated to
3380-
// #5563, filed as #5881), so this pins the gate where it exists.
3378+
// [#5881] `enableCache: false` is no longer what makes this reachable —
3379+
// dashboard reads bypass the cache unconditionally now, and the DEFAULT
3380+
// configuration is pinned separately below. Kept explicit because an
3381+
// operator who disables the cache must get the same answer, and because
3382+
// this case is what the fix had to leave untouched: it was the only
3383+
// green proof the gate worked at all while the default path skipped it.
33813384
{ api: { requireAuth: false }, metadata: { enableCache: false } } as any,
33823385
);
33833386
rest.resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: [] });
@@ -3395,6 +3398,114 @@ describe('filterDashboardForUser — ADR-0057 D10 widget requiresService gate',
33953398
expect(ids(body.item)).toContain('widget_total_users');
33963399
});
33973400

3401+
// -------------------------------------------------------------------------
3402+
// [#5881] The gate on the DEFAULT path.
3403+
//
3404+
// `enableCache` defaults to true, and the single-item read had a cached
3405+
// branch that excluded `app` (per-user RBAC) and `doc`/`book` (per-caller
3406+
// audience) but NOT `dashboard` — so the widget gate above, which lives in
3407+
// the uncached branch, never ran in a default deployment. Measured on
3408+
// `origin/main` @ 8e2bbba24 before the fix, against the real RestServer:
3409+
//
3410+
// cachedCalls: 1 | uncachedCalls: 0 | widgetsServed: ["w_users","w_orgs"]
3411+
//
3412+
// The fix excludes `dashboard` from the cached branch, as `app` already was.
3413+
// Why not instead hoist the gate so both branches run it once — which reads
3414+
// like the tidier shape? Because the ETag cannot carry the gate's verdict:
3415+
// it is a hash of the UNFILTERED document, and `notModified` is decided
3416+
// inside the protocol before this layer sees it. Measured, same baseline:
3417+
//
3418+
// etag(unfiltered): 2504e71e | etag(gated body): 75ca17c1 | same? false
3419+
// revalidate-with-unfiltered-etag -> notModified: true
3420+
// cacheControl: {"directives":["private","no-cache"]}
3421+
//
3422+
// So a hoisted gate would ship a filtered body under a validator naming the
3423+
// unfiltered one. Within one boot that is harmless (the registered-service
3424+
// set cannot change: `Kernel.use()` throws after bootstrap and nothing
3425+
// deregisters), but `private, no-cache` means the client stores the body and
3426+
// only revalidates — and the stored body outlives the process. Turn the
3427+
// optional service off in a redeploy and the document is unchanged, so every
3428+
// revalidation answers 304 and the dead tile survives the very deploy that
3429+
// removed its service. The full-read cost of not caching is nil:
3430+
// `getMetaItemCached` delegates to `getMetaItem`, so the server does the same
3431+
// work either way and only the 304's saved bytes are given up.
3432+
// -------------------------------------------------------------------------
3433+
/** A protocol offering BOTH reads, so the branch choice is the thing tested. */
3434+
const bothReads = () => {
3435+
const protocol: any = createMockProtocol();
3436+
protocol.getMetaItem = vi.fn().mockResolvedValue({
3437+
type: 'dashboard', name: 'system_overview', item: dash(),
3438+
});
3439+
protocol.getMetaItemCached = vi.fn().mockResolvedValue({
3440+
data: dash(),
3441+
etag: { value: 'etag-unfiltered', weak: false },
3442+
lastModified: new Date().toISOString(),
3443+
cacheControl: { directives: ['private', 'no-cache'] },
3444+
notModified: false,
3445+
});
3446+
return protocol;
3447+
};
3448+
/** Default config — no `metadata` block at all, so `enableCache` is its default. */
3449+
const defaultServer = (protocol: any) => {
3450+
const rest: any = new RestServer(createMockServer() as any, protocol, ANON_API as any);
3451+
rest.resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: [] });
3452+
rest.serviceExistsProvider = (n: string) => n !== 'org-scoping';
3453+
rest.registerRoutes();
3454+
return rest;
3455+
};
3456+
const readMeta = async (rest: any, type: string, name: string) => {
3457+
const route = rest.getRoutes().find(
3458+
(r: any) => r.method === 'GET' && r.path === '/api/v1/meta/:type/:name',
3459+
);
3460+
const res = { json: vi.fn(), status: vi.fn().mockReturnThis(), header: vi.fn(), send: vi.fn() };
3461+
await route.handler({ params: { type, name }, query: {}, headers: {} }, res);
3462+
return res;
3463+
};
3464+
3465+
it('runs the gate under the DEFAULT configuration, not only when the cache is off', async () => {
3466+
const protocol = bothReads();
3467+
const res = await readMeta(defaultServer(protocol), 'dashboard', 'system_overview');
3468+
3469+
const body = res.json.mock.calls.at(-1)![0];
3470+
expect(ids(body.item)).not.toContain('widget_organizations');
3471+
expect(ids(body.item)).toContain('widget_total_users');
3472+
// The envelope the route owes its caller is unchanged by the branch switch.
3473+
expect(body).toMatchObject({ type: 'dashboard', name: 'system_overview' });
3474+
});
3475+
3476+
it('a dashboard read takes the uncached branch — while other types still cache', async () => {
3477+
// The positive control matters as much as the assertion above it: excluding
3478+
// one type from the cached branch is only correct if it excludes exactly
3479+
// that type. A fix that disabled the cache wholesale would satisfy every
3480+
// gate assertion here and quietly cost every other metadata read its ETag.
3481+
const protocol = bothReads();
3482+
const rest = defaultServer(protocol);
3483+
3484+
const dashRes = await readMeta(rest, 'dashboard', 'system_overview');
3485+
expect(protocol.getMetaItemCached).not.toHaveBeenCalled();
3486+
expect(protocol.getMetaItem).toHaveBeenCalledTimes(1);
3487+
// The observable price of the bypass, pinned rather than hidden: a
3488+
// dashboard read carries no ETag validator now.
3489+
expect(dashRes.header.mock.calls.map((c: any[]) => c[0])).not.toContain('ETag');
3490+
3491+
const viewRes = await readMeta(rest, 'view', 'account_list');
3492+
expect(protocol.getMetaItemCached).toHaveBeenCalledTimes(1);
3493+
expect(viewRes.header.mock.calls.map((c: any[]) => c[0])).toContain('ETag');
3494+
});
3495+
3496+
it('the plural spelling cannot spell its way around the exclusion', async () => {
3497+
// `/meta/dashboards/x` is the CANONICAL REST spelling (Prime Directive #3)
3498+
// and the route serves both, so an exclusion compared against the literal
3499+
// `'dashboard'` would leave the default path ungated under the spelling
3500+
// most callers use — the #3984 defect class, which `metaTypeSingular`'s own
3501+
// docstring exists to remember.
3502+
const protocol = bothReads();
3503+
const res = await readMeta(defaultServer(protocol), 'dashboards', 'system_overview');
3504+
3505+
expect(protocol.getMetaItemCached).not.toHaveBeenCalled();
3506+
expect(ids(res.json.mock.calls.at(-1)![0].item)).not.toContain('widget_organizations');
3507+
});
3508+
33983509
it('resolveRegisteredServices discovers requiresService declared on widgets', async () => {
33993510
const rest: any = make();
34003511
const kernel = { getServiceAsync: async (n: string) => { if (n === 'org-scoping') return {}; throw new Error('absent'); } };

0 commit comments

Comments
 (0)