diff --git a/.changeset/domain-claim-segment-boundary.md b/.changeset/domain-claim-segment-boundary.md new file mode 100644 index 0000000000..693240aaa1 --- /dev/null +++ b/.changeset/domain-claim-segment-boundary.md @@ -0,0 +1,13 @@ +--- +"@objectstack/runtime": minor +--- + +Dispatcher domain routes stop claiming their lexical neighbours: `DomainRoute.match` now defaults to `'segment'`, and the project-membership skip list gained the same boundary. + +Ten shipped routes — `/actions`, `/ai`, `/analytics`, `/automation`, `/data`, `/i18n`, `/meta`, `/notifications`, `/packages`, `/ui` — carried the implicit `'prefix'` default, a bare `path.startsWith(prefix)` with no segment boundary. So `/datax`, `/metaxyz`, `/uifoo`, `/aixx` and `/packagesomething` were each claimed by a domain that does not own them, and a package mounting one of those namespaces later would have been shadowed by a domain that never wanted it. `/auth` was the eleventh member of the family and was repaired on its own; this closes the rest at the seam rather than one route at a time, so the eleventh domain someone adds is boundary-correct without having to remember anything. + +- **The default moved, the mode did not go away.** `match: 'segment'` (the prefix exactly, plus everything under `prefix + '/'`) is the default; `match: 'prefix'` still buys the bare `startsWith` claim for a route that asks for it in writing. One shape genuinely needs it and now declares it: a prefix ending in `'?'` (`/keys?`, `/mcp?`, `/mcp/skill?`), which reproduces the legacy branch's query-string form for adapters that pass the query through in the path. There is no `/` after that `'?'`, so a segment match cannot express it — those three routes match exactly what they always did. +- **What each narrowed claim used to answer, measured per domain rather than assumed.** They were not uniform: `/actionsx`, `/aixx`, `/automationx`, `/metaxyz` and `/packagesomething` answered `401`; `/i18nxx` and `/notificationsx` answered `501`; `/analyticsx` and `/uifoo` fell through unhandled. `/data` was the worst and the reason per-domain measurement was owed — its handler reads the sub-path as an OBJECT NAME, so `GET /datax` answered a **success envelope for a fabricated object** and `GET /datax/foo` **threw** `Record foo not found in x`. Each of the ten now answers the dispatcher's `ROUTE_NOT_FOUND` envelope, which is what they should always have answered. No caller depended on any of these: nothing in the repo builds a dispatch path by concatenating a domain prefix without a separator, and no route-ledger row or SDK method addresses a shape of this kind. +- **The membership skip list, which was the same predicate with a worse consequence.** `enforceProjectMembership` skipped the control plane with `skipPaths.some(p => path.startsWith(p))` and `'/auth'` in the list, so `/authentication/foo` was waved **past the membership check** rather than merely routed somewhere wrong. It was latent — nothing claims `/authentication/*`, so such a request 404s first — and it would have gone live the day any domain claimed a path of that shape. The skip list stops at `'/'`, `'?'` or end-of-string now; the `'?'` form is part of the boundary on purpose, so `/auth?redirect=…` keeps the exemption it has today and the control plane is not newly gated. + +Every domain still claims itself and every path under it, `/auth/me/permissions` included; the registry header comment that described the old rough edges as deliberate no longer describes code that has them. diff --git a/packages/runtime/src/domain-handler-registry.test.ts b/packages/runtime/src/domain-handler-registry.test.ts index 1e73b16eb7..e60c37b98f 100644 --- a/packages/runtime/src/domain-handler-registry.test.ts +++ b/packages/runtime/src/domain-handler-registry.test.ts @@ -5,8 +5,10 @@ * * Two layers under test: * 1. `DomainHandlerRegistry` matching semantics (first-match, exact vs - * prefix, method restriction) — deliberately faithful to the legacy - * if-chain, rough edges included. + * segment vs prefix, method restriction). These were deliberately + * faithful to the legacy if-chain, rough edges included, until #16263 + * made `'segment'` the default; the legacy bare-`startsWith` claim is + * still available to a route that declares `match: 'prefix'`. * 2. `HttpDispatcher` integration: the four seeded builtin domains * (/health /ready /analytics /i18n) behave exactly as their legacy * if-chain branches did, and `registerDomainHandler` is the public @@ -65,17 +67,43 @@ describe('DomainHandlerRegistry', () => { expect(registry.resolve('/a/x', 'GET')?.handler).toBe(first); }); - it("match: 'exact' does not claim sub-paths; default prefix match does (legacy startsWith, rough edges included)", () => { + it("match: 'exact' does not claim sub-paths; the DEFAULT claims the prefix and everything under it, and stops there", () => { const registry = new DomainHandlerRegistry(); registry.register({ prefix: '/health', match: 'exact', handler: okHandler('h') }); registry.register({ prefix: '/i18n', handler: okHandler('i') }); expect(registry.resolve('/health', 'GET')).toBeDefined(); expect(registry.resolve('/health/deep', 'GET')).toBeUndefined(); + expect(registry.resolve('/i18n', 'GET')).toBeDefined(); + expect(registry.resolve('/i18n/locales', 'GET')).toBeDefined(); + // [#16263] The default is `'segment'`. This assertion USED TO READ + // `toBeDefined()` and pinned the legacy rough edge on purpose + // ("bare startsWith also matches '/i18nxx'"); the edge is the defect + // #16263 removed, so the pin is inverted rather than deleted — the + // sibling namespace must be provably released, not merely unasserted. + expect(registry.resolve('/i18nxx', 'GET')).toBeUndefined(); + }); + + it("match: 'prefix' still buys the legacy bare-startsWith claim — it is declared now, not inherited", () => { + const registry = new DomainHandlerRegistry(); + registry.register({ prefix: '/i18n', match: 'prefix', handler: okHandler('i') }); + expect(registry.resolve('/i18n', 'GET')).toBeDefined(); expect(registry.resolve('/i18n/locales', 'GET')).toBeDefined(); - // Faithful legacy semantics: bare startsWith also matches '/i18nxx'. expect(registry.resolve('/i18nxx', 'GET')).toBeDefined(); }); + it("a `?`-suffixed prefix is why 'prefix' survives: no '/' follows the '?', so 'segment' cannot express it", () => { + const registry = new DomainHandlerRegistry(); + registry.register({ prefix: '/keys', match: 'segment', handler: okHandler('k') }); + registry.register({ prefix: '/keys?', match: 'prefix', handler: okHandler('kq') }); + expect(registry.resolve('/keys', 'GET')?.prefix).toBe('/keys'); + expect(registry.resolve('/keys/rotate', 'GET')?.prefix).toBe('/keys'); + expect(registry.resolve('/keys?scope=x', 'GET')?.prefix).toBe('/keys?'); + // …and the segment route alone would NOT have claimed the query form. + const segmentOnly = new DomainHandlerRegistry(); + segmentOnly.register({ prefix: '/keys', match: 'segment', handler: okHandler('k') }); + expect(segmentOnly.resolve('/keys?scope=x', 'GET')).toBeUndefined(); + }); + it('restricts by method when `methods` is set (case-insensitive on input)', () => { const registry = new DomainHandlerRegistry(); registry.register({ prefix: '/health', match: 'exact', methods: ['GET'], handler: okHandler('h') }); diff --git a/packages/runtime/src/domain-handler-registry.ts b/packages/runtime/src/domain-handler-registry.ts index 6a56ae78a8..130fb34ae8 100644 --- a/packages/runtime/src/domain-handler-registry.ts +++ b/packages/runtime/src/domain-handler-registry.ts @@ -27,10 +27,13 @@ * a slot exclusively can still self-register via * {@link HttpDispatcher.registerDomainHandler}. * - * Matching semantics are deliberately faithful to the legacy if-chain, - * INCLUDING its rough edges (`match: 'prefix'` on `/i18n` also matches - * `/i18nxx`, exactly as `startsWith` did) — fixing those edges is explicitly - * not this seam's job; behavior preservation is. + * Matching semantics were deliberately faithful to the legacy if-chain, + * INCLUDING its rough edges, for as long as the migration needed behaviour + * preservation to be the only promise this seam made. That period is over and + * the edges are fixed (#16263): a domain claim now stops at a SEGMENT + * BOUNDARY by default, so `/i18n` no longer claims `/i18nxx`. The legacy + * `startsWith` shape is still reachable, but only where a route ASKS for it in + * writing (`match: 'prefix'`) — see {@link DomainRoute.match}. */ import type { HttpProtocolContext, HttpDispatcherResult } from './http-dispatcher.js'; @@ -60,10 +63,43 @@ export interface DomainRoute { /** Path prefix the domain claims, e.g. `'/i18n'`. */ prefix: string; /** - * `'prefix'` — legacy `startsWith(prefix)` semantics (default). + * How much of the path space this route claims. + * + * `'segment'` — **the default**: the path equals the prefix, or is + * followed by `'/'`. Claims `/i18n` and everything under `/i18n/`, and + * does NOT claim `/i18nxx`. * `'exact'` — the path must equal the prefix exactly. - * `'segment'` — exact, or followed by `'/'` (the legacy - * `=== p || startsWith(p + '/')` branch shape; does NOT claim `/i18nxx`). + * `'prefix'` — bare `startsWith(prefix)`, NO segment boundary: the legacy + * if-chain's shape, which also claims `/i18nxx`. + * + * ## Why `'segment'` is the default and `'prefix'` must be asked for + * + * The reasoning is #16026's, applied to the whole table rather than to one + * prefix. A bare `startsWith` claim reaches SIBLING NAMESPACES: `/authx`, + * `/authentication/foo`, `/datax`, `/metaxyz`, `/uifoo` are not paths of + * the domain that was claiming them by any reading, and each is a + * plausible namespace someone mounts later — a route registered there is + * SHADOWED by a domain that never wanted it. `'segment'` claims the prefix + * exactly and everything under `prefix + '/'`, which is the whole of what + * a domain owns, so narrowing to it removes only claims a domain does not + * own and keeps every sub-path fallthrough intact (#4088's + * `/auth/me/permissions` is the case that pins that half). + * + * `'segment'` was already the codebase's own spelling for a + * boundary-correct claim — `/auth`, `/keys`, `/mcp`, `/mcp/skill`, + * `/security` and `/share-links` each declared it — so this makes the + * table's majority spelling its default rather than introducing a + * convention. + * + * ⚠️ `'prefix'` is NOT deprecated, and one shape genuinely needs it: a + * prefix ending in `'?'` (`'/keys?'`, `'/mcp?'`), which reproduces the + * legacy branch's query-string form for adapters that pass the query + * through in `path`. There is no `/` after that `'?'`, so a segment match + * cannot express it. Those routes declare `match: 'prefix'` in writing. + * + * ⛔ Do not reach for `'prefix'` to widen a domain's claim over its + * lexical neighbours. The default changed because that claim was never + * anything but a migration artefact. */ match?: 'prefix' | 'exact' | 'segment'; /** Restrict to these UPPERCASE HTTP methods. Omit = all methods. */ @@ -362,10 +398,12 @@ export class DomainHandlerRegistry { switch (route.match) { case 'exact': return path === route.prefix; - case 'segment': - return path === route.prefix || path.startsWith(route.prefix + '/'); - default: + case 'prefix': + // Bare `startsWith`, no segment boundary — the legacy + // if-chain's shape, now reachable only by asking for it. return path.startsWith(route.prefix); + default: + return path === route.prefix || path.startsWith(route.prefix + '/'); } } diff --git a/packages/runtime/src/domains/auth.ts b/packages/runtime/src/domains/auth.ts index 824ff7d050..b76e01f043 100644 --- a/packages/runtime/src/domains/auth.ts +++ b/packages/runtime/src/domains/auth.ts @@ -17,9 +17,9 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. * The route this domain claims — `/auth` and its slash-separated sub-paths, * and NOTHING ELSE (#16026). * - * ## Why `match: 'segment'` is spelled out rather than left to the default + * ## Why this claim stops at a segment boundary * - * `DomainRoute.match` defaults to `'prefix'`, i.e. a bare + * `DomainRoute.match` USED TO default to `'prefix'`, i.e. a bare * `path.startsWith('/auth')` with no segment boundary — the legacy if-chain's * shape, which `DomainHandlerRegistry` preserved deliberately when the domains * were lifted out of it. On this prefix that rough edge claims SIBLING @@ -51,6 +51,14 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. * already declare (`/keys`, `/mcp`, `/mcp/skill`), so this is the codebase's * own established spelling for the fix, not a new convention. * + * ⭐ [#16263] It is now also the registry's DEFAULT, so this line no longer + * changes what `/auth` matches — the reasoning above was applied to the whole + * route table rather than to this one prefix. The declaration is kept, not + * deleted: it states at the route what the route claims, which is the fact + * every case in `auth-claim-segment-boundary.test.ts` is about, and it keeps + * this claim pinned to `'segment'` explicitly rather than to whatever the + * default happens to be later. + * * ⚠️ What this does NOT fix, deliberately: the `200 {}` those rows carried. * That answer is manufactured one layer OUT, where the adapter renders a * dispatcher result — the auth service itself answers an honest 404 for every diff --git a/packages/runtime/src/domains/domain-claim-segment-boundary.test.ts b/packages/runtime/src/domains/domain-claim-segment-boundary.test.ts new file mode 100644 index 0000000000..e19e5cd686 --- /dev/null +++ b/packages/runtime/src/domains/domain-claim-segment-boundary.test.ts @@ -0,0 +1,259 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16263] Every dispatcher domain claims its prefix and the paths UNDER it — + * and NOT every path that merely starts with the prefix's characters. + * + * ## The defect this pins + * + * `DomainRoute.match` defaulted to `'prefix'`, a bare + * `path.startsWith(route.prefix)` with no segment boundary, and ten shipped + * routes carried that implicit default. `/auth` was the eleventh and was + * repaired on its own in #16026; this file is the same repair applied to the + * rest of the table, delivered at the seam: the registry's DEFAULT is + * `'segment'` now, so a route claims a sibling namespace only by asking for + * `match: 'prefix'` in writing. + * + * ## Measured per domain, before the fix — NOT inferred from `/auth` + * + * Each of the ten hands its sub-path to a different handler with a different + * parse, so what a lexical extension ANSWERS had to be measured domain by + * domain. Taken through `HttpDispatcher.dispatch()` on the fixture below + * (`origin/main` 7f96e1417e), `GET` unless noted: + * + * /actionsx 401 UNAUTHENTICATED claimed by /actions + * /aixx 401 UNAUTHENTICATED claimed by /ai + * /analyticsx handled=false claimed by /analytics + * /automationx 401 UNAUTHENTICATED claimed by /automation + * /datax 200 SUCCESS claimed by /data <- worst + * /datax/foo THREW "Record foo not found in x" <- worst + * /i18nxx 501 NOT_IMPLEMENTED claimed by /i18n + * /metaxyz 401 UNAUTHENTICATED claimed by /meta + * /notificationsx 501 NOT_IMPLEMENTED claimed by /notifications + * /packagesomething 401 UNAUTHENTICATED claimed by /packages + * /uifoo handled=false claimed by /ui + * + * ⚠️ The two `/data` rows are why "one of them may already be harmless; + * another may be worse" was worth measuring. `/data`'s handler reads + * `req.path.substring(5)` as an OBJECT NAME, so the stray characters became + * the name of an object nobody declared: `GET /datax` answered a SUCCESS + * envelope for a fabricated object, and `GET /datax/foo` threw + * `Record foo not found in x` — an unattributable 500 naming a record and an + * object that were manufactured by the missing boundary. Nothing here is a + * capability being removed; it is a wrong answer being stopped. + * + * ⛔ No live dependent was found for any row: no in-repo caller builds a + * dispatch path by concatenating a domain prefix without a separator (every + * `dispatcher.dispatch(...)` call site in `dispatcher-plugin.ts` writes the + * literal prefix plus a `/`-led sub-path), no `route-ledger.ts` row names a + * shape of this kind, and no SDK method addresses one. + * + * ## ⭐ Why the STILL-CLAIMED rows carry the same weight as the defect rows + * + * A pin asserting only that `/datax` 404s cannot fail in the direction that + * matters most: a "repair" that stopped claiming `/data` ALTOGETHER would pass + * every defect row while deleting the domain. The overshoot controls are the + * only thing that proves the narrowing is a narrowing — each canonical prefix + * and one sub-path under it must still reach its own handler, with the SAME + * answer it gave before the change (the statuses above are unchanged for every + * one of them). + * + * The `?`-suffixed routes are the third class and the one a default flip could + * silently have broken: `/keys?`, `/mcp?` and `/mcp/skill?` have no `/` after + * the `'?'`, so a segment match cannot express them at all. They declare + * `match: 'prefix'` now and are pinned here for exactly that reason. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +function makeDispatcher() { + const objectql = { + find: vi.fn().mockResolvedValue([]), + getObjects: vi.fn().mockReturnValue({}), + executeAction: vi.fn().mockResolvedValue({}), + registry: { + getObject: vi.fn().mockReturnValue(null), + getRegisteredTypes: vi.fn().mockReturnValue([]), + }, + }; + const services: Record = { objectql }; + const kernel: any = { + getState: () => 'running', + getService: (n: string) => services[n] ?? null, + getServiceAsync: async (n: string) => services[n] ?? null, + context: { getService: (n: string) => services[n] ?? null }, + }; + return new HttpDispatcher(kernel, undefined, { enforceProjectMembership: false }); +} + +const dispatch = (method: string, path: string) => + makeDispatcher().dispatch(method, path, {}, {}, { request: new Request(`http://localhost${path}`) } as any); + +/** The registry's own answer for a path — which route, if any, claims it. */ +const claimant = (path: string, method = 'GET'): string | undefined => + (makeDispatcher() as any).domainRegistry.resolve(path, method)?.prefix; + +/** The ten routes that carried the implicit `'prefix'` default, and a lexical extension of each. */ +const SIBLING_NAMESPACES: ReadonlyArray = [ + ['/actions', '/actionsx'], + ['/ai', '/aixx'], + ['/analytics', '/analyticsx'], + ['/automation', '/automationx'], + ['/data', '/datax'], + ['/i18n', '/i18nxx'], + ['/meta', '/metaxyz'], + ['/notifications', '/notificationsx'], + ['/packages', '/packagesomething'], + ['/ui', '/uifoo'], +]; + +/** Paths every domain MUST keep claiming — the overshoot controls. */ +const STILL_CLAIMED: ReadonlyArray = [ + ['/actions', '/actions/obj/act'], + ['/ai', '/ai/chat'], + ['/analytics', '/analytics/query'], + ['/automation', '/automation/flows'], + ['/data', '/data/contacts'], + ['/i18n', '/i18n/locales'], + ['/meta', '/meta/objects'], + ['/notifications', '/notifications/read'], + ['/packages', '/packages/pkg-1'], + ['/ui', '/ui/layouts'], +]; + +describe('#16263: a domain claim stops at a segment boundary', () => { + describe('lexical extensions are NOT claimed — they fall through to ROUTE_NOT_FOUND', () => { + for (const [prefix, sibling] of SIBLING_NAMESPACES) { + it(`${sibling} is not claimed by ${prefix} and answers the ROUTE_NOT_FOUND envelope`, async () => { + // Stated about the REGISTRY, which is where the shadowing lives: + // a package mounting `${sibling}` later must be reachable. + expect(claimant(sibling)).toBeUndefined(); + + // Refusal asserts the ENVELOPE (code + status + route), never a + // bare "it did not succeed" — an unrelated 404 from any other + // layer would otherwise read as this fix working. + const result = await dispatch('GET', sibling); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(404); + expect(result.response?.body?.success).toBe(false); + expect(result.response?.body?.error?.code).toBe('ROUTE_NOT_FOUND'); + expect(result.response?.body?.error?.httpStatus).toBe(404); + expect(result.response?.body?.error?.route).toBe(sibling); + }); + + it(`${sibling}/foo — a sub-path of the sibling namespace — is not claimed by ${prefix} either`, async () => { + expect(claimant(`${sibling}/foo`)).toBeUndefined(); + const result = await dispatch('GET', `${sibling}/foo`); + expect(result.response?.status).toBe(404); + expect(result.response?.body?.error?.code).toBe('ROUTE_NOT_FOUND'); + }); + } + }); + + describe('⭐ the overshoot controls — every domain still claims itself and its sub-paths', () => { + for (const [prefix, subPath] of STILL_CLAIMED) { + it(`${prefix} and ${subPath} still resolve to the ${prefix} route`, () => { + expect(claimant(prefix)).toBe(prefix); + expect(claimant(subPath)).toBe(prefix); + }); + + it(`${prefix} does NOT answer the terminal ROUTE_NOT_FOUND`, async () => { + const result = await dispatch('GET', prefix); + // The domain answered (or declined to handle and fell through to + // the legacy chain) — what it must never be is the dispatcher's + // terminal refusal for an unclaimed path. + expect(result.response?.body?.error?.code).not.toBe('ROUTE_NOT_FOUND'); + }); + } + }); + + describe("the `?`-suffixed routes keep the legacy shape — they declare match: 'prefix'", () => { + for (const [path, expected] of [ + ['/keys?scope=x', '/keys?'], + ['/mcp?x=1', '/mcp?'], + ['/mcp/skill?x=1', '/mcp/skill?'], + ] as const) { + it(`${path} is still claimed by the ${expected} route`, () => { + expect(claimant(path)).toBe(expected); + expect(claimant(path, 'POST')).toBe(expected); + }); + } + + it('the canonical `?`-free forms still resolve to their own segment routes', () => { + expect(claimant('/keys')).toBe('/keys'); + expect(claimant('/mcp')).toBe('/mcp'); + expect(claimant('/mcp/skill')).toBe('/mcp/skill'); + }); + }); + + describe('the domains that already declared match: segment are unchanged', () => { + it('/security and /share-links still refuse their lexical extensions and still claim themselves', () => { + expect(claimant('/security')).toBe('/security'); + expect(claimant('/share-links')).toBe('/share-links'); + expect(claimant('/securityx')).toBeUndefined(); + expect(claimant('/share-linksx')).toBeUndefined(); + }); + + it("the card's own clean rows are untouched — the harness can tell the classes apart", async () => { + for (const path of ['/zzz/foo', '/aut/foo']) { + expect(claimant(path)).toBeUndefined(); + const result = await dispatch('GET', path); + expect(result.response?.status).toBe(404); + expect(result.response?.body?.error?.code).toBe('ROUTE_NOT_FOUND'); + } + }); + }); + + /** + * ⭐ The case the 404 rows above are blind to. + * + * Every row above concludes "not claimed" from a `ROUTE_NOT_FOUND`. That + * observation cannot tell this fix apart from one that KEEPS the wide claim + * and refuses inside each handler: the envelope would be the same and the + * namespace would still be SHADOWED, so a package mounting `/datax` would + * never run. Shadowing is the harm the card names, so it needs an + * observation of its own. + * + * `registerDomainHandler` appends to a first-match-wins table, so a probe + * registered AFTER construction sits BEHIND every builtin domain — exactly + * where a package mounting `/datax` later would sit. It is reachable only + * if the `/data` route declines the path, and the evidence is the probe's + * OWN response coming back out of `dispatch()`, not the absence of a call. + */ + it('⭐ a domain registered at a sibling namespace AFTER construction is REACHED — the claim was released, not merely silenced', async () => { + for (const [, sibling] of SIBLING_NAMESPACES) { + const dispatcher = makeDispatcher(); + const probe = vi.fn(async (req: any) => ({ + handled: true as const, + response: { status: 200, body: { success: true, data: { probe: sibling, path: req.path } } }, + })); + dispatcher.registerDomainHandler({ prefix: sibling, handler: probe }); + + for (const path of [sibling, `${sibling}/foo`]) { + const result = await dispatcher.dispatch('GET', path, {}, {}, { request: new Request(`http://localhost${path}`) } as any); + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(200); + expect(result.response?.body?.data?.probe).toBe(sibling); + expect(result.response?.body?.data?.path).toBe(path); + expect(result.response?.body?.error?.code).toBeUndefined(); + } + expect(probe).toHaveBeenCalledTimes(2); + } + }); + + /** + * The seam itself: a route that ASKS for the legacy shape still gets it. + * Without this, a later "simplification" could delete `'prefix'` entirely + * and every `?`-suffixed route would go dark with no test to say so. + */ + it("match: 'prefix' still means bare startsWith — the legacy shape is reachable, by declaration only", async () => { + const dispatcher = makeDispatcher(); + const wide = vi.fn(async () => ({ handled: true as const, response: { status: 200, body: { success: true, data: { wide: true } } } })); + dispatcher.registerDomainHandler({ prefix: '/widedomain', match: 'prefix', handler: wide }); + + const result = await dispatcher.dispatch('GET', '/widedomainxx', {}, {}, { request: new Request('http://localhost/widedomainxx') } as any); + expect(result.response?.status).toBe(200); + expect(result.response?.body?.data?.wide).toBe(true); + }); +}); diff --git a/packages/runtime/src/domains/keys.ts b/packages/runtime/src/domains/keys.ts index a23464f181..46c1797848 100644 --- a/packages/runtime/src/domains/keys.ts +++ b/packages/runtime/src/domains/keys.ts @@ -42,13 +42,19 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. * The legacy branch matched `=== '/keys' || startsWith('/keys/') || * startsWith('/keys?')` — a segment match PLUS the query-string form some * adapters pass through in `path`. Two entries reproduce that exactly. + * + * [#16263] The second entry now says `match: 'prefix'` out loud. It always + * relied on the bare-`startsWith` default, and that default is `'segment'` + * now; a prefix ending in `'?'` has no `/` after it, so a segment match + * cannot express the query-string form at all. Declaring it keeps this route + * byte-identical to what it has always matched. */ export function createKeysDomains(deps: DomainHandlerDeps): DomainRoute[] { const handler: DomainRoute['handler'] = (req, context) => handleKeysRequest(deps, req.method, req.body, context); return [ { prefix: '/keys', match: 'segment', handler }, - { prefix: '/keys?', handler }, + { prefix: '/keys?', match: 'prefix', handler }, ]; } diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 3503c99d5d..d8736da01c 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -21,13 +21,18 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. * The legacy branches matched `/mcp/skill` (exact or `?`-suffixed) BEFORE * the `/mcp` transport claimed everything else (exact, `/`, or `?` forms). * Entry order reproduces that precedence. + * + * [#16263] The two `?`-suffixed entries now say `match: 'prefix'` out loud — + * they always rode the bare-`startsWith` default, which is `'segment'` now, + * and a prefix ending in `'?'` has no `/` after it for a segment match to + * find. Declaring it keeps both routes matching exactly what they always did. */ export function createMcpDomains(deps: DomainHandlerDeps): DomainRoute[] { return [ { prefix: '/mcp/skill', match: 'segment', handler: (req, context) => handleMcpSkillRequest(deps, req.method, context) }, - { prefix: '/mcp/skill?', handler: (req, context) => handleMcpSkillRequest(deps, req.method, context) }, + { prefix: '/mcp/skill?', match: 'prefix', handler: (req, context) => handleMcpSkillRequest(deps, req.method, context) }, { prefix: '/mcp', match: 'segment', handler: (req, context) => handleMcpRequest(deps, req.body, context) }, - { prefix: '/mcp?', handler: (req, context) => handleMcpRequest(deps, req.body, context) }, + { prefix: '/mcp?', match: 'prefix', handler: (req, context) => handleMcpRequest(deps, req.body, context) }, ]; } diff --git a/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts b/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts new file mode 100644 index 0000000000..48aff0c804 --- /dev/null +++ b/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts @@ -0,0 +1,169 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16263, the sixth site] `enforceProjectMembership`'s control-plane SKIP + * LIST stops at a segment boundary. + * + * ## Why this site ranks ahead of the ten domain claims + * + * It is the identical predicate — `skipPaths.some(p => path.startsWith(p))` + * with `'/auth'` in the list — making the identical mistake about the + * identical prefix: `/authentication/foo` is not under `/auth` by any reading, + * yet it satisfied the test. What differs is the CONSEQUENCE, and the contract + * review that found it said so plainly rather than dressing it up: + * + * a claim that is too wide sends traffic somewhere wrong, + * while a skip list that is too wide sends traffic PAST A CHECK. + * + * ⚠️ It is LATENT, not harmless, and must not be graded as either extreme. + * Nothing claims `/authentication/*` today, so such a request 404s further + * down before the missing membership check can matter. It goes live the day + * any domain claims a path of that shape — and on that day the symptom is a + * non-member reading a scoped route, not a 404. + * + * ## The observation point, stated plainly + * + * These cases call `enforceProjectMembership` directly. That is deliberate and + * it is the honest point: reaching this gate through `dispatch()` needs a + * resolved `context.environmentId`, which comes from a host KernelResolver or + * a single-environment plugin, and standing one up would put the thing under + * test behind two seams that are not what this file is about. The gate's + * inputs are exactly `(context, path)`, and both are supplied here. + * + * The fixture makes the two outcomes DISTINGUISHABLE, which a bare fixture + * does not: `enforceProjectMembership` fails open in a great many ways (no + * auth service, no session, no ObjectQL, a cached membership), and under any + * of them "skipped" and "checked and passed" both return `null` — a test built + * on that could not fail. So the session resolves to a real user in a + * non-platform org, and `sys_environment_member` answers with NO row. A path + * that reaches the check therefore comes back 403; a path that is skipped + * comes back `null`. The two classes are then separated by the answer itself. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from './http-dispatcher.js'; + +const USER_ID = 'user-not-a-member'; +const ENVIRONMENT_ID = 'env-scoped-1'; + +function makeGate() { + const find = vi.fn().mockResolvedValue([]); // no membership row -> not a member + const objectql = { + find, + getObjects: vi.fn().mockReturnValue({}), + registry: { getObject: vi.fn().mockReturnValue(null), getRegisteredTypes: vi.fn().mockReturnValue([]) }, + }; + const auth = { + getApi: async () => ({ + getSession: async () => ({ + user: { id: USER_ID }, + session: { userId: USER_ID, activeOrganizationId: 'org-tenant' }, + }), + }), + }; + const services: Record = { objectql, auth }; + const kernel: any = { + getState: () => 'running', + getService: (n: string) => services[n] ?? null, + getServiceAsync: async (n: string) => services[n] ?? null, + context: { getService: (n: string) => services[n] ?? null }, + }; + const dispatcher = new HttpDispatcher(kernel, undefined, { enforceProjectMembership: true }); + const check = (path: string) => + (dispatcher as any).enforceProjectMembership( + { environmentId: ENVIRONMENT_ID, request: { headers: new Headers() } }, + path, + ); + return { check, find }; +} + +/** Paths the skip list must NOT wave past the check — the defect rows. */ +const MUST_BE_CHECKED = [ + '/authentication/foo', + '/authx', + '/authx/foo', + '/cloudy/foo', + '/healthz', + '/readyx', + '/discoveryx/foo', +]; + +/** Paths the skip list must KEEP waving past — the overshoot controls. */ +const MUST_BE_SKIPPED = [ + '/auth', + '/auth/me/permissions', + '/cloud', + '/cloud/environments/abc', + '/health', + '/ready', + '/discovery', +]; + +describe('#16263: the membership skip list stops at a segment boundary', () => { + describe('sibling namespaces are CHECKED — they no longer ride the control-plane exemption', () => { + for (const path of MUST_BE_CHECKED) { + it(`${path} reaches the membership check and is refused with the 403 envelope`, async () => { + const { check, find } = makeGate(); + const result = await check(path); + + // Asserting the ENVELOPE, not merely "not null": any other + // refusal from any other layer would otherwise read as this + // gate running. + expect(result).not.toBeNull(); + expect(result?.status).toBe(403); + expect(result?.body?.success).toBe(false); + expect(result?.body?.error?.code).toBe('PROJECT_MEMBERSHIP_REQUIRED'); + expect(result?.body?.error?.httpStatus).toBe(403); + + // …and the check really ran, rather than the envelope arriving + // from somewhere that never asked the control plane. + expect(find).toHaveBeenCalledWith('sys_environment_member', expect.objectContaining({ + where: { environment_id: ENVIRONMENT_ID, user_id: USER_ID }, + })); + }); + } + }); + + describe('⭐ the overshoot controls — the control plane is still exempt', () => { + for (const path of MUST_BE_SKIPPED) { + it(`${path} is still skipped before the check runs`, async () => { + const { check, find } = makeGate(); + expect(await check(path)).toBeNull(); + // `null` alone cannot tell "skipped" from "checked and passed" — + // this fixture has no membership row, so a checked path would + // have queried and then 403'd. Never querying is the evidence. + expect(find).not.toHaveBeenCalled(); + }); + } + }); + + describe('the query-string form keeps its exemption — the boundary is `/`, `?` or end', () => { + for (const path of ['/auth?redirect=%2Fapp', '/health?verbose=1', '/cloud?page=2']) { + it(`${path} is still skipped`, async () => { + const { check, find } = makeGate(); + expect(await check(path)).toBeNull(); + expect(find).not.toHaveBeenCalled(); + }); + } + + it('a `?` does not smuggle a sibling namespace back in', async () => { + const { check } = makeGate(); + const result = await check('/authentication/foo?x=1'); + expect(result?.status).toBe(403); + expect(result?.body?.error?.code).toBe('PROJECT_MEMBERSHIP_REQUIRED'); + }); + }); + + it('an ordinary scoped route is unchanged — it was always checked and still is', async () => { + const { check } = makeGate(); + const result = await check('/data/contacts'); + expect(result?.status).toBe(403); + expect(result?.body?.error?.code).toBe('PROJECT_MEMBERSHIP_REQUIRED'); + }); + + it('the public share-link carve-out is unchanged — it is a different rule, below the skip list', async () => { + const { check, find } = makeGate(); + expect(await check('/share-links/tok-123/resolve')).toBeNull(); + expect(find).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 3aff8ea00f..4aa3b05467 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -225,6 +225,27 @@ export interface HttpDispatcherOptions { scopeManager?: EnvironmentScopeManager; } +/** + * Whether `path` IS `prefix` or lies UNDER it — a prefix test that stops at a + * segment boundary (#16263). + * + * The boundary is `'/'`, `'?'` or end-of-string. `charCodeAt` past the end + * yields `NaN`, and `NaN` fails both comparisons, so the end-of-string case is + * the `prefix.length === path.length` equality that `startsWith` already + * established — no separate length check. + * + * ⛔ Not a general path utility and deliberately not exported: it exists for + * the CONTROL-PLANE SKIP LIST, where the query form has to keep matching. A + * domain's route claim is a different question with a different answer — + * `DomainHandlerRegistry`'s `match: 'segment'`, which does not accept `'?'` + * because a `?`-suffixed prefix is spelled as its own route there. + */ +function isPathWithinPrefix(path: string, prefix: string): boolean { + if (!path.startsWith(prefix)) return false; + const next = path.charCodeAt(prefix.length); + return Number.isNaN(next) || next === 47 /* '/' */ || next === 63 /* '?' */; +} + /** * `services.search`'s in-process remedy string (#7939), kept out of the * shared `inProcessServiceMessage('search')` path on purpose: that helper's @@ -1312,8 +1333,34 @@ export class HttpDispatcher { if (!this.enforceMembership) return null; // Control-plane paths — never gated by project membership. + // + // [#16263] The membership skip list stops at a SEGMENT BOUNDARY. It + // was `skipPaths.some(p => path.startsWith(p))`, the same bare + // `startsWith` the domain registry defaulted to — and it is the same + // mistake about the same prefix: `/authentication/foo` is not under + // `/auth`, yet it satisfied `startsWith('/auth')` and was waved past + // this check. + // + // ⚠️ Why this site is repaired ahead of the domain claims even though + // it is the harder one to make fire: a domain claim that is too wide + // sends traffic SOMEWHERE WRONG, while a skip list that is too wide + // sends traffic PAST A CHECK. Nothing claims `/authentication/*` + // today, so such a request 404s further down before the missing + // membership check can matter — LATENT, not harmless. It goes live the + // day any domain claims a path of that shape, and on that day the + // symptom is a non-member reading a scoped route, not a 404. + // + // The boundary is `'/'`, `'?'` or end-of-string, matching the + // `acceptOAuthAccessToken` spelling in `resolveRequestScope` + // (`/^…\/mcp(?:[/?]|$)/`). `'?'` is load-bearing rather than + // decorative: `cleanPath` here has only had a trailing slash stripped, + // so an adapter that passes the query through in `path` presents + // `/auth?redirect=…` — skipped before this change, and it must stay + // skipped. A repair that only accepted `'/'` would newly gate the + // control plane on membership, which is a WIDER change than the one + // this card asks for and in the dangerous direction. const skipPaths = ['/auth', '/cloud', '/health', '/ready', '/discovery']; - if (skipPaths.some(p => path.startsWith(p))) return null; + if (skipPaths.some(p => isPathWithinPrefix(path, p))) return null; // Public share-link resolve/messages — the token IS the authorisation, // so never gate them on project membership (a signed-in non-member