diff --git a/.changeset/auth-gate-allowlist-anchored.md b/.changeset/auth-gate-allowlist-anchored.md new file mode 100644 index 0000000000..0e112f6168 --- /dev/null +++ b/.changeset/auth-gate-allowlist-anchored.md @@ -0,0 +1,13 @@ +--- +"@objectstack/core": patch +--- + +`isAuthGateAllowlisted` matches allow-listed routes at a mount boundary, so an object named `auth` or a record whose id is `health` no longer bypasses the ADR-0069 authentication-policy gate. + +The predicate that decides which paths are exempt from the password-expiry / enforced-MFA gate matched with two UNANCHORED tests: `path.includes('/auth/')` matched at any position, and an `endsWith` test over `['/health', '/ready', '/discovery', '/me/apps', '/me/localization']` matched at any depth. A path segment whose VALUE merely spelled one of those tokens therefore carried the exemption — and object names and record ids are tenant-controlled. Both transport seams hand the predicate a data-plane path directly (`HttpDispatcher.enforceAuthGate` passes `cleanPath`, `RestServer.enforceAuth` passes `req.path`), so these were reachable requests. Measured on the built package before the repair: `/data/auth/123`, `/meta/auth/objects`, `/data/x/health` and `/data/xyz/me/apps` were all exempt, while `/auth/me` (exempt) and `/data/contacts/1` (gated) held as controls. + +- **What replaced them.** The path is read as segments and each test is anchored to a mount base — `/api/v1`, `/api`, or the empty base the dispatcher sees (the hono adapter hands `dispatch()` the app prefix already stripped) — plus at most one environment scope immediately after that base (`/environments/`, or ADR-0006's superseded `/projects/`), because the dispatcher evaluates the gate before its scoped-URL strip. `/auth/…` at that position stays exempt; the five bootstrap reads are EXACT routes there instead of suffixes. The scope is only recognised immediately after a base, which is why `/data/environments/x/health` is not a scoped `/health`. +- **This only ever removes exemptions.** Measured, not asserted: over a generated corpus of 111,152 paths, the number that are newly exempt is **0** and 25,979 stopped being exempt. The check is kept as a test, with the pre-anchoring predicate transcribed beside it, so a later widening cannot arrive quietly. +- **Every genuinely-exempt shape still is**, pinned in both directions: `/auth/sign-out`, `/health`, `/ready`, `/discovery` (dispatcher shapes); `/api/auth/sign-in`, `/api/v1/auth/change-password`, `/api/v1/auth/me/permissions`, `/api/v1/health`, `/api/v1/me/apps`, `/api/v1/me/localization`; and the scoped `/api/v1/environments//auth/sign-out`. + +**If you serve the API from a non-default mount,** an allow-listed route reached as `${basePath}/${version}/…` with `basePath`/`version` moved off `/api` and `v1` is no longer named by the allow-list. That price cannot be avoided: `/rest/v2/health` and `/data/xyz/health` are the same shape, so a rule that accepts an arbitrary base is the defect itself. It costs nothing at either live seam — the dispatcher's path arrives base-stripped, and REST registers its control-plane routes without `enforceAuth` at all — but if you gate a custom mount through this predicate, mount the remediation routes under one of the named bases. diff --git a/docs/qa/platform-checklist/areas/access-security.json b/docs/qa/platform-checklist/areas/access-security.json index 563c265a7d..6b4219c744 100644 --- a/docs/qa/platform-checklist/areas/access-security.json +++ b/docs/qa/platform-checklist/areas/access-security.json @@ -2599,7 +2599,7 @@ }, "source": [ "packages/plugins/plugin-hono-server/src/current-user-endpoints.ts#tabPermissions (/auth/me/permissions aggregation + most-permissive merge), (/auth/me/localization), (/me/apps requiredPermissions/tabPermissions filter), (the /api/v1 prefix)", - "packages/core/src/security/auth-gate.ts#ALLOW_SUFFIXES (ALLOW_SUFFIXES — /me/apps + /me/localization reachable to gated users)", + "packages/core/src/security/auth-gate.ts#ALLOW_ROUTES (ALLOW_ROUTES — /me/apps + /me/localization reachable to gated users. Was ALLOW_SUFFIXES, an endsWith test that also exempted any path merely ENDING in those two — /data/xyz/me/apps among them; the allow-list is anchored to a mount base now, so these are EXACT routes at a mount and a record id can no longer spell its way into the exemption)", "#7616 (delegated permission-set resolution — the enforcement path's own answer), #2752 (/me/apps registry sourcing), #3391 (effective apiOperations annotation), #4093 (guarded degraded branch), ADR-0090 D5 (additive baseline)", "cross-ref access-security.anonymous-deny-surfaces — the 401 floor this trio is the declared exception to", "cross-ref access-security.fls-mask-and-strip — owns the FLS enforcement this item's clause 2 cross-checks" diff --git a/packages/core/src/security/auth-gate.test.ts b/packages/core/src/security/auth-gate.test.ts index 9ce174c486..de3e5c616f 100644 --- a/packages/core/src/security/auth-gate.test.ts +++ b/packages/core/src/security/auth-gate.test.ts @@ -27,6 +27,128 @@ describe('auth-gate (ADR-0069 session gate)', () => { expect(isAuthGateAllowlisted('/api/v1/auth/sign-out/?x=1')).toBe(true); expect(isAuthGateAllowlisted('/api/v1/data/x/')).toBe(false); }); + + // ── [#16839] The allow-list is ANCHORED ──────────────────────────────── + // + // It used to match unanchored: `path.includes('/auth/')` at ANY position + // and an `endsWith` suffix test at ANY depth. So a segment whose VALUE + // spelled an allow-listed token carried the exemption, and object names + // and record ids are TENANT-CONTROLLED. Both seams hand this predicate a + // data-plane path directly — `HttpDispatcher.enforceAuthGate(context, + // cleanPath)` and `RestServer.enforceAuth` (`req.path`) — so a tenant that + // declared an object named `auth`, or held a record whose id is `health`, + // handed a password-expired / MFA-required session a bypass on that + // object's data routes. + // + // ⛔ These pin the DECISION for the exact paths the card measured, not the + // spelling of the predicate, so they survive a rewrite of it. + describe('[#16839] a tenant-controlled segment cannot buy the exemption', () => { + it('gates the four paths that were falsely exempt', () => { + for (const p of [ + '/data/auth/123', // an object named `auth` + '/meta/auth/objects', // an object named `auth` + '/data/x/health', // a record whose id is `health` + '/data/xyz/me/apps', + ]) { + expect(isAuthGateAllowlisted(p), p).toBe(false); + } + }); + + it('gates the same shapes under the REST mount, where the seam sees the base', () => { + for (const p of [ + '/api/v1/data/auth/123', + '/api/v1/meta/auth/objects', + '/api/v1/data/health', // `/data/:object` with object = `health` + '/api/v1/data/x/health', // `/data/:object/:id` with id = `health` + '/api/v1/data/contacts/me/apps', + '/api/v1/data/environments/x/health', // a scope-shaped OBJECT name, mid-path + ]) { + expect(isAuthGateAllowlisted(p), p).toBe(false); + } + }); + + // ⭐ The card's own two control rows. Without them the block above would + // read the same for a predicate that had simply started refusing + // everything. + it('CONTROL — the genuinely-exempt path stays exempt and the protected one stays gated', () => { + expect(isAuthGateAllowlisted('/auth/me')).toBe(true); + expect(isAuthGateAllowlisted('/data/contacts/1')).toBe(false); + }); + + // The other direction, at full width: every mount shape a real + // remediation / bootstrap route arrives in must still be exempt. An + // anchoring that is too strict fails HERE rather than in production. + it('keeps every genuinely-exempt route shape exempt', () => { + for (const p of [ + // dispatcher shape — the hono adapter strips the app prefix + '/auth/sign-out', '/auth/two-factor/enable', '/auth/me/localization', + '/auth', '/health', '/ready', '/discovery', + // REST + better-auth mounts + '/api/auth', '/api/auth/sign-in', + '/api/v1/auth', '/api/v1/auth/change-password', '/api/v1/auth/me/permissions', + '/api/v1/health', '/api/v1/ready', '/api/v1/discovery', + '/api/v1/me/apps', '/api/v1/me/localization', + // environment-scoped mount — the dispatcher evaluates the gate + // BEFORE its scoped-URL strip, so this spelling reaches the predicate + '/api/v1/environments/env_1/auth/sign-out', + '/environments/env_1/auth/sign-out', + '/api/v1/environments/env_1/discovery', + // the legacy `projects` spelling of the same scope (ADR-0006) + '/api/v1/projects/env_1/auth/sign-out', + ]) { + expect(isAuthGateAllowlisted(p), p).toBe(true); + } + }); + + // ⭐ CLAUSE ② DISCHARGE — the repair only ever REMOVES exemptions. + // + // The dispatch declared "nothing is newly accepted"; this measures it + // instead of asserting it. `preAnchoringAllowlisted` is the predicate + // this file's subject replaced, transcribed verbatim from `origin/main` + // cf6e0a193b, and the corpus is every path of up to four segments drawn + // from the vocabulary the two spellings can disagree on. A single + // `new && !old` row means a path became NEWLY exempt, which is a + // widening and is not this card's to make. + it('is a strict SUBSET of the pre-anchoring allow-list — nothing becomes newly exempt', () => { + const OLD_PREFIXES = ['/api/v1/auth/', '/api/auth/', '/auth/']; + const OLD_SUFFIXES = ['/health', '/ready', '/discovery', '/me/apps', '/me/localization']; + const preAnchoringAllowlisted = (rawPath: string | undefined | null): boolean => { + if (!rawPath) return true; + let path = rawPath.split('?')[0] || '/'; + let end = path.length; + while (end > 1 && path.charCodeAt(end - 1) === 47) end--; + path = path.slice(0, end) || '/'; + if (path.includes('/auth/')) return true; + for (const p of OLD_PREFIXES) if (path.startsWith(p) || path === p.replace(/\/$/, '')) return true; + for (const s of OLD_SUFFIXES) if (path.endsWith(s)) return true; + return false; + }; + + const SEG = ['api', 'v1', 'auth', 'health', 'ready', 'discovery', 'me', 'apps', + 'localization', 'data', 'meta', 'ui', 'environments', 'projects', 'env1', 'x']; + const corpus: string[] = ['/', '']; + for (const a of SEG) { + corpus.push(`/${a}`); + for (const b of SEG) { + corpus.push(`/${a}/${b}`); + for (const c of SEG) { + corpus.push(`/${a}/${b}/${c}`); + for (const d of SEG) corpus.push(`/${a}/${b}/${c}/${d}`); + } + } + } + + const widened = corpus.filter((p) => isAuthGateAllowlisted(p) && !preAnchoringAllowlisted(p)); + expect(widened).toEqual([]); + // Anti-vacuity: the corpus really does exercise both predicates, and + // the repair really did remove exemptions — a corpus that narrowed + // nothing would satisfy the line above without measuring anything. + const narrowed = corpus.filter((p) => !isAuthGateAllowlisted(p) && preAnchoringAllowlisted(p)); + expect(corpus.length).toBeGreaterThan(10_000); + expect(narrowed.length).toBeGreaterThan(0); + expect(narrowed).toContain('/data/x/health'); + }); + }); }); describe('evaluateAuthGate', () => { diff --git a/packages/core/src/security/auth-gate.ts b/packages/core/src/security/auth-gate.ts index 8ffa74f21e..c867f11a0c 100644 --- a/packages/core/src/security/auth-gate.ts +++ b/packages/core/src/security/auth-gate.ts @@ -57,10 +57,67 @@ export function normalizeAuthGate(sessionUser: any): AuthGate | null { } // Endpoints a gated user MUST still reach to remediate or bootstrap the -// remediation UI. Matched against the request path (query stripped). Covers -// both REST (`/api/v1/auth/…`) and dispatcher (`/auth/…`) path shapes. -const ALLOW_PREFIXES = ['/api/v1/auth/', '/api/auth/', '/auth/']; -const ALLOW_SUFFIXES = ['/health', '/ready', '/discovery', '/me/apps', '/me/localization']; +// remediation UI. Matched against the request path (query stripped). +// +// [#16839] Every test below is ANCHORED to a mount boundary, because the +// allow-list is a set of ROUTES and a route is identified by where it sits, +// not by text appearing somewhere in a path. The two predicates this replaced +// were unanchored — `path.includes('/auth/')` matched at ANY position and a +// suffix test matched at ANY depth — so a segment whose VALUE happened to +// spell an allow-listed token carried the exemption: `/data/auth/123` (an +// object named `auth`), `/data/x/health` (a record whose id is `health`), +// `/data/xyz/me/apps`. Both seams pass a data-plane path straight in +// (`HttpDispatcher.enforceAuthGate`, `RestServer.enforceAuth`), so those were +// reachable requests, and object + record names are tenant-controlled. + +/** + * The mount bases an allow-listed route can sit at, as SEGMENT lists, longest + * first. `[]` is the dispatcher shape: the hono adapter hands + * `HttpDispatcher.dispatch` the app prefix already stripped, so a dispatcher + * path arrives as `/auth/…`, `/health`, `/environments//auth/…`. The other + * two are the REST/better-auth mounts (`${basePath}/${version}` and + * better-auth's own `${basePath}/auth`) at their shipped defaults — the same + * three bases the pre-anchoring `ALLOW_PREFIXES` enumerated. + * + * ⚠️ A host that moves `api.basePath`/`api.version` off those defaults is no + * longer named here. That is the deliberate price of anchoring and it cannot + * be avoided: `/rest/v2/health` and `/data/xyz/health` are the SAME SHAPE, so + * a rule that accepts an arbitrary base is the defect. It costs nothing at + * either live seam — the dispatcher's path is base-stripped (matched by `[]`), + * and REST registers its control-plane routes without `enforceAuth` at all. + */ +const MOUNT_BASES: readonly (readonly string[])[] = [['api', 'v1'], ['api'], []]; + +/** + * Segments that open an environment scope between the base and the route + * (`/api/v1/environments//auth/…`). The dispatcher evaluates the gate + * BEFORE its scoped-URL strip, so the scoped spelling reaches this predicate; + * `projects` is ADR-0006's superseded spelling, which the REST scope strip + * still accepts. + */ +const SCOPE_SEGMENTS: readonly string[] = ['environments', 'projects']; + +/** + * The bootstrap reads, as EXACT routes at a mount (replaces the old + * `ALLOW_SUFFIXES` endsWith test). `/health`, `/ready` and `/discovery` are + * the dispatcher's probes and discovery document; `/me/apps` and + * `/me/localization` are the current-user reads the remediation UI needs + * (`plugin-hono-server/src/current-user-endpoints.ts`). + */ +const ALLOW_ROUTES: readonly (readonly string[])[] = [ + ['health'], + ['ready'], + ['discovery'], + ['me', 'apps'], + ['me', 'localization'], +]; + +/** Do `segments` start with every segment of `prefix`? */ +function startsWithSegments(segments: readonly string[], prefix: readonly string[]): boolean { + if (segments.length < prefix.length) return false; + for (let k = 0; k < prefix.length; k++) if (segments[k] !== prefix[k]) return false; + return true; +} /** True when `path` is exempt from the auth gate (auth + remediation + health). */ export function isAuthGateAllowlisted(rawPath: string | undefined | null): boolean { @@ -71,14 +128,34 @@ export function isAuthGateAllowlisted(rawPath: string | undefined | null): boole let end = path.length; while (end > 1 && path.charCodeAt(end - 1) === 47) end--; path = path.slice(0, end) || '/'; - // Any path with an `/auth/` segment is an auth endpoint (covers project- - // scoped mounts like `/api/v1/environments/:env/auth/...`). - if (path.includes('/auth/')) return true; - for (const p of ALLOW_PREFIXES) { - if (path.startsWith(p) || path === p.replace(/\/$/, '')) return true; - } - for (const s of ALLOW_SUFFIXES) { - if (path.endsWith(s)) return true; + // Segment view — `''` entries dropped so `//auth//me` cannot smuggle an + // empty segment past the position tests below. + const segments = path.split('/').filter((s) => s !== ''); + for (const base of MOUNT_BASES) { + if (!startsWithSegments(segments, base)) continue; + let i = base.length; + let scoped = false; + // One optional environment scope, and only immediately after the base — + // which is why `/data/environments/x/health` is NOT a scoped `/health`. + if (i + 1 < segments.length && SCOPE_SEGMENTS.includes(segments[i] as string)) { + i += 2; + scoped = true; + } + if (segments[i] === 'auth') { + // `[//]/auth/…` — the remediation surface. This is the + // anchored replacement for `path.includes('/auth/')`. + if (i + 1 < segments.length) return true; + // Bare `/auth`, exempt only UNSCOPED: that is exactly what the old + // `ALLOW_PREFIXES` equality branch admitted (`/auth`, `/api/auth`, + // `/api/v1/auth`). The scoped spelling was never exempt and must not + // become so here — this repair only ever removes exemptions. + if (!scoped) return true; + } + for (const route of ALLOW_ROUTES) { + if (segments.length - i === route.length && startsWithSegments(segments.slice(i), route)) { + return true; + } + } } return false; } diff --git a/packages/rest/src/auth-gate-allowlist-fault-window.measurement.test.ts b/packages/rest/src/auth-gate-allowlist-fault-window.measurement.test.ts index ff0137c39d..050f45eb8c 100644 --- a/packages/rest/src/auth-gate-allowlist-fault-window.measurement.test.ts +++ b/packages/rest/src/auth-gate-allowlist-fault-window.measurement.test.ts @@ -187,8 +187,11 @@ async function drive(rest: RestServer, path: string, method = 'GET'): Promise { // The census above is about PATTERNS this server mounts. It is not a claim // about concrete requests: `/api/v1/data/:object` with `object` = `health` - // materializes `/api/v1/data/health`, which `isAuthGateAllowlisted` answers - // `true` for (the `ALLOW_SUFFIXES` rule is a suffix test, not a route test). - // That over-broad direction is #7898's subject, ⛔ not this card's, and it - // is recorded here only so the census is not read as "the allow-list never - // fires on this door". - expect(isAuthGateAllowlisted('/api/v1/data/health')).toBe(true); + // materializes `/api/v1/data/health`, and that concrete path used to be + // ALLOW-LISTED — which is what made the over-broad direction reachable at + // this very door. + // + // ⚠️ RE-AIMED IN PLACE (#16839), per this file's own header instruction. + // The superseded assertion and its reason, verbatim: + // + // // ... which `isAuthGateAllowlisted` answers `true` for (the + // // `ALLOW_SUFFIXES` rule is a suffix test, not a route test). + // // That over-broad direction is #7898's subject, ⛔ not this card's + // expect(isAuthGateAllowlisted('/api/v1/data/health')).toBe(true); + // + // The allow-list is anchored to a mount boundary now, so a record id or an + // object name can no longer spell its way into the exemption. The reading + // this leg exists to record is UNCHANGED — the census must still not be + // read as "the allow-list never fires on this door", and the discovery + // document below is what makes it fire. + expect(isAuthGateAllowlisted('/api/v1/data/health')).toBe(false); expect(isAuthGateAllowlisted('/api/v1/data/sys_user')).toBe(false); + // ⭐ POSITIVE CONTROL — the predicate did not simply start refusing + // everything: the allow-listed mounted route the census found still is. + expect(isAuthGateAllowlisted('/api/v1/discovery')).toBe(true); }); });