From df2af0a3e0347f4ddc20305e6c3cb46f105a3ab8 Mon Sep 17 00:00:00 2001 From: ekklesia Date: Thu, 23 Jul 2026 11:44:45 -0500 Subject: [PATCH] fix: enforce registered authorization permissions Two related fail-closed gaps let a well-formed but never-declared permission id slip through: - POST /api/v1/grants created explicit grants for any well-formed PermissionId without checking the injected permission registry, allowing dormant grants for invented identifiers that could become effective the moment those identifiers were registered later. - createAuthorizationEvaluator validated the permission id only inside the restricted-baseline branch, so a malformed permission combined with the public or authenticated baseline could still return allowed. Both now fail closed: the grants route checks the target permission against the trusted registry before calling store.createGrant (sanitized 403, no store or audit write), and the evaluator validates permission shape before any baseline is allowed to return an allowed decision, without adding a registry dependency to the evaluator itself. The live integration lifecycle test switched from an unregistered calendar.events.read to the registered core.authorization.grants.manage permission, since module permission registration does not exist yet. --- apps/api/src/grants-routes.ts | 10 ++++ apps/api/tests/api-http.integration.ts | 56 ++++++++++++++++++- apps/api/tests/grants-routes.test.ts | 33 +++++++++-- core/src/authorization/evaluator.ts | 24 +++++--- core/tests/authorization-evaluator.test.ts | 44 +++++++++++++++ ...-core-authorization-and-owner-bootstrap.md | 37 +++++++++--- docs/development/authorization.md | 49 +++++++++++++--- 7 files changed, 226 insertions(+), 27 deletions(-) diff --git a/apps/api/src/grants-routes.ts b/apps/api/src/grants-routes.ts index 78fdd4d..2f3d0c0 100644 --- a/apps/api/src/grants-routes.ts +++ b/apps/api/src/grants-routes.ts @@ -164,6 +164,16 @@ export function createGrantsRoutes(options: CreateGrantsRoutesOptions): Hono { return c.json({ code: "INVALID_REQUEST" }, 400); } + // A well-formed PermissionId is not necessarily a *registered* one. Only the trusted registry + // (never the request) decides what may be granted — the same registry `requireManagePermission` + // already consults for the caller's own `core.authorization.grants.manage` check — so a caller + // cannot pre-create a dormant grant for an invented identifier that later becomes effective the + // moment someone registers it. Sanitized identically to any other denial: this must never reveal + // whether the identifier was malformed (400, above) or simply unregistered (403, here). + if (!options.registry.has(parsed.permission)) { + return c.json({ code: "AUTHORIZATION_DENIED" }, 403); + } + try { const grant = await options.store.createGrant({ actor: createUserActor(principal.userId), diff --git a/apps/api/tests/api-http.integration.ts b/apps/api/tests/api-http.integration.ts index 652e291..c997385 100644 --- a/apps/api/tests/api-http.integration.ts +++ b/apps/api/tests/api-http.integration.ts @@ -832,7 +832,13 @@ describe.sequential("live API — platform owner and delegated grant management" const ownerEmail = "authz-owner@example.test"; const managerEmail = "authz-manager@example.test"; const password = "correct horse battery staple"; - const testPermission = "calendar.events.read"; + // Module permission registration does not exist yet, so `core.authorization.grants.manage` — + // the one permission `CORE_PERMISSION_REGISTRY` actually declares — stands in for "some + // registered permission" throughout this scenario's explicit-grant lifecycle. + const testPermission = "core.authorization.grants.manage"; + // Well-formed (passes `PermissionId` shape validation) but never registered — used to prove the + // live route rejects it even for a caller who otherwise has full grant-management authority. + const unregisteredPermission = "calendar.events.read"; it("starts a freshly migrated API and signs up the eventual owner and eventual delegated manager", async () => { database = await createDisposableDatabase("authz_http"); @@ -913,6 +919,54 @@ describe.sequential("live API — platform owner and delegated grant management" expect(response.status).toBe(200); }); + it("rejects a well-formed but unregistered permission even from a caller with full grant-management authority, and writes no grant or audit row", async () => { + const admin = await openAdministrativeClient(database.url); + try { + const grantCountBefore = await safeQuery<{ count: string }>( + admin, + `select count(*)::text as count from "ekklesia_core_grant" where permission_id = $1`, + [unregisteredPermission], + ); + const auditCountBefore = await safeQuery<{ count: string }>( + admin, + `select count(*)::text as count from "ekklesia_core_authorization_audit" where permission_id = $1`, + [unregisteredPermission], + ); + expect(grantCountBefore[0]?.count).toBe("0"); + expect(auditCountBefore[0]?.count).toBe("0"); + + const response = await fetch(`${origin}/api/v1/grants`, { + body: JSON.stringify({ + permission: unregisteredPermission, + subjectUserId: managerUserId, + }), + headers: { + "content-type": "application/json", + cookie: managerCookie, + origin: dashboardOrigin, + }, + method: "POST", + }); + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ code: "AUTHORIZATION_DENIED" }); + + const grantCountAfter = await safeQuery<{ count: string }>( + admin, + `select count(*)::text as count from "ekklesia_core_grant" where permission_id = $1`, + [unregisteredPermission], + ); + const auditCountAfter = await safeQuery<{ count: string }>( + admin, + `select count(*)::text as count from "ekklesia_core_authorization_audit" where permission_id = $1`, + [unregisteredPermission], + ); + expect(grantCountAfter[0]?.count).toBe("0"); + expect(auditCountAfter[0]?.count).toBe("0"); + } finally { + await safeEnd(admin); + } + }); + it("creates an explicit grant via HTTP, deriving sourceKind and actor server-side, and records an atomic audit entry", async () => { const response = await fetch(`${origin}/api/v1/grants`, { body: JSON.stringify({ diff --git a/apps/api/tests/grants-routes.test.ts b/apps/api/tests/grants-routes.test.ts index 0cd368f..0b1b37c 100644 --- a/apps/api/tests/grants-routes.test.ts +++ b/apps/api/tests/grants-routes.test.ts @@ -9,6 +9,7 @@ import { type AuthorizationEvaluator, type AuthorizationStore, AuthorizationStoreError, + CORE_AUTHORIZATION_GRANTS_MANAGE, CORE_PERMISSION_REGISTRY, } from "@ekklesia/core/authorization"; import type { EkklesiaIdentityRuntime } from "@ekklesia/core/identity"; @@ -20,7 +21,13 @@ const trustedOrigin = "https://dashboard.example.test"; const untrustedOrigin = "https://evil.example.test"; const manager = createAuthenticatedPrincipal("manager_1"); const subjectUserId = parseEkklesiaUserId("subject_1"); +// Well-formed but never declared in `CORE_PERMISSION_REGISTRY` — used to prove the routes reject +// (or, for listing, still return) an unregistered permission id rather than assuming registration. const permission = parsePermissionId("calendar.events.read"); +// A permission `CORE_PERMISSION_REGISTRY` actually declares, so tests that must reach +// `store.createGrant`/`store.revokeGrant` can do so without also depending on module permission +// registration, which does not exist yet. +const registeredPermission = CORE_AUTHORIZATION_GRANTS_MANAGE; const grantId = parseGrantId("11111111-1111-1111-1111-111111111111"); function fakeRuntime(overrides: Partial = {}): EkklesiaIdentityRuntime { @@ -170,7 +177,7 @@ describe("POST /api/v1/grants", () => { const createdGrant = { createdAt: new Date("2026-01-01T00:00:00.000Z"), id: grantId, - permission, + permission: registeredPermission, scope: parseAuthorizationScope("calendar-event:123"), sourceKind: "explicit" as const, userId: subjectUserId, @@ -179,7 +186,11 @@ describe("POST /api/v1/grants", () => { const { app } = buildApp({ store }); const response = await app.request("/api/v1/grants", { - body: JSON.stringify({ permission, scope: "calendar-event:123", subjectUserId }), + body: JSON.stringify({ + permission: registeredPermission, + scope: "calendar-event:123", + subjectUserId, + }), headers: { "content-type": "application/json", origin: trustedOrigin }, method: "POST", }); @@ -190,7 +201,7 @@ describe("POST /api/v1/grants", () => { expect(store.createGrant).toHaveBeenCalledWith( expect.objectContaining({ actor: { type: "user", userId: manager.userId }, - permission, + permission: registeredPermission, scope: "calendar-event:123", sourceKind: "explicit", userId: subjectUserId, @@ -198,6 +209,20 @@ describe("POST /api/v1/grants", () => { ); }); + it("rejects a well-formed but unregistered permission with the same sanitized denial as an unauthorized caller, and never calls the store", async () => { + const { app, store } = buildApp({}); + + const response = await app.request("/api/v1/grants", { + body: JSON.stringify({ permission, subjectUserId }), + headers: { "content-type": "application/json", origin: trustedOrigin }, + method: "POST", + }); + + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ code: "AUTHORIZATION_DENIED" }); + expect(store.createGrant).not.toHaveBeenCalled(); + }); + it.each([ "actor", "owner", @@ -256,7 +281,7 @@ describe("POST /api/v1/grants", () => { const { app } = buildApp({ store }); const response = await app.request("/api/v1/grants", { - body: JSON.stringify({ permission, subjectUserId }), + body: JSON.stringify({ permission: registeredPermission, subjectUserId }), headers: { "content-type": "application/json", origin: trustedOrigin }, method: "POST", }); diff --git a/core/src/authorization/evaluator.ts b/core/src/authorization/evaluator.ts index ed8a95e..8b8f43a 100644 --- a/core/src/authorization/evaluator.ts +++ b/core/src/authorization/evaluator.ts @@ -32,8 +32,13 @@ export class AuthorizationEvaluationError extends Error { export interface AuthorizeRequest { readonly principal: EkklesiaPrincipal; - /** Validated internally via the branded `PermissionId` schema; a malformed or unrecognizable - * value denies rather than throwing — "unknown or malformed permissions fail closed." */ + /** Validated internally via the branded `PermissionId` schema *before any baseline can return an + * allowed decision* — a malformed or unrecognizable value denies rather than throwing, for + * `public` and `authenticated` baselines exactly as much as `restricted`. This is shape + * validation only: the evaluator has no registry and cannot tell a well-formed-but-unregistered + * permission from one that is actually registered. Only a trusted caller consulting a + * `PermissionRegistry` (`core/src/authorization/permissions.ts`) before calling `authorize` can + * make that distinction — see that module and `apps/api/src/authorization-middleware.ts`. */ readonly permission: unknown; /** Validated internally against the known `public`/`authenticated`/`restricted` set; anything * else denies rather than throwing, for the same fail-closed reason. */ @@ -80,6 +85,16 @@ export function createAuthorizationEvaluator( return AUTHORIZATION_DENIED; } + // Shape-validated before any baseline is allowed to return `allowed` — a malformed + // permission id must deny under `public` and `authenticated` exactly as much as under + // `restricted`, even though neither of the first two ever reads `parsedPermission.data`. + // This is a pure, synchronous check: it never touches the grant or platform-owner ports, so + // `public`/`authenticated` still resolve with no database lookup at all. + const parsedPermission = safeParsePermissionId(request.permission); + if (!parsedPermission.success) { + return AUTHORIZATION_DENIED; + } + if (parsedBaseline.data === "public") { return AUTHORIZATION_ALLOWED; } @@ -96,11 +111,6 @@ export function createAuthorizationEvaluator( return AUTHORIZATION_DENIED; } - const parsedPermission = safeParsePermissionId(request.permission); - if (!parsedPermission.success) { - return AUTHORIZATION_DENIED; - } - let scope: AuthorizationScope | undefined; if (request.scope !== undefined) { const parsedScope = safeParseAuthorizationScope(request.scope); diff --git a/core/tests/authorization-evaluator.test.ts b/core/tests/authorization-evaluator.test.ts index 8c9ba86..bbc41ca 100644 --- a/core/tests/authorization-evaluator.test.ts +++ b/core/tests/authorization-evaluator.test.ts @@ -115,6 +115,28 @@ describe("Core authorization evaluator", () => { expect(grants.calls).toHaveLength(0); expect(owners.calls).toHaveLength(0); }); + + it("denies an anonymous principal when the permission id is malformed, without throwing or querying grants or ownership", async () => { + const decision = await evaluator().authorize({ + baseline: "public", + permission: "not a valid permission id", + principal: ANONYMOUS_PRINCIPAL, + }); + expect(decision).toEqual({ allowed: false }); + expect(grants.calls).toHaveLength(0); + expect(owners.calls).toHaveLength(0); + }); + + it("denies an authenticated principal when the permission id is malformed", async () => { + const decision = await evaluator().authorize({ + baseline: "public", + permission: "not a valid permission id", + principal: createAuthenticatedPrincipal(userId), + }); + expect(decision).toEqual({ allowed: false }); + expect(grants.calls).toHaveLength(0); + expect(owners.calls).toHaveLength(0); + }); }); describe("authenticated baseline", () => { @@ -145,6 +167,28 @@ describe("Core authorization evaluator", () => { expect(grants.calls).toHaveLength(0); expect(owners.calls).toHaveLength(0); }); + + it("denies an authenticated principal when the permission id is malformed, without throwing or querying grants or ownership", async () => { + const decision = await evaluator().authorize({ + baseline: "authenticated", + permission: "not a valid permission id", + principal: createAuthenticatedPrincipal(userId), + }); + expect(decision).toEqual({ allowed: false }); + expect(grants.calls).toHaveLength(0); + expect(owners.calls).toHaveLength(0); + }); + + it("still denies an anonymous principal when the permission id is malformed too", async () => { + const decision = await evaluator().authorize({ + baseline: "authenticated", + permission: "not a valid permission id", + principal: ANONYMOUS_PRINCIPAL, + }); + expect(decision).toEqual({ allowed: false }); + expect(grants.calls).toHaveLength(0); + expect(owners.calls).toHaveLength(0); + }); }); describe("restricted baseline", () => { diff --git a/docs/architecture/decisions/0013-core-authorization-and-owner-bootstrap.md b/docs/architecture/decisions/0013-core-authorization-and-owner-bootstrap.md index afcdad4..5f84daa 100644 --- a/docs/architecture/decisions/0013-core-authorization-and-owner-bootstrap.md +++ b/docs/architecture/decisions/0013-core-authorization-and-owner-bootstrap.md @@ -46,6 +46,18 @@ or malformed permission id is treated identically to a denied decision, never a would work if declared" response — deny-by-default holds even for permissions nobody has declared yet. +Registration is enforced everywhere a permission id can originate from outside Core's own trusted +route wiring, not only for the caller's own required permission. `requirePermission` +(`apps/api/src/authorization-middleware.ts`) checks the registry for the permission a *route* +requires; `POST /api/v1/grants` (`apps/api/src/grants-routes.ts`) separately checks the registry +for the *target* permission a caller is requesting be granted to someone else, since that value +comes from the request body rather than route wiring. Without the second check, a caller with +`core.authorization.grants.manage` could create a stored grant for a permission id that is merely +well-formed, not registered — a dormant grant that would silently become effective the moment +that identifier was registered later, with no further action or review at registration time. Both +checks reject with the identical sanitized `403 {"code":"AUTHORIZATION_DENIED"}` an ordinary denial +would return, and neither ever calls into the store on rejection. + ### Permissions declare one trusted baseline: public, authenticated, or restricted Every registered permission declares exactly one `PermissionBaseline` at registration time. The @@ -60,14 +72,23 @@ restricted check. ### Unknown permissions and client-supplied baselines fail closed -Two independent fail-closed layers exist on purpose, not one relied on twice. The registry itself -(`PermissionRegistry.get`/`.has`) returns `undefined`/`false` for anything malformed or -unregistered. `requirePermission` treats a registry miss as an immediate sanitized `403`, before -principal resolution or evaluation ever runs. Separately, `createAuthorizationEvaluator` itself -denies (without throwing) any malformed or unrecognized `permission`/`baseline`/`scope` it -receives, so a future caller that reaches the evaluator directly — bypassing the registry, for -example a different transport Core does not yet have — inherits the same deny-by-default guarantee -rather than depending on the registry check having already run. +Two independent fail-closed layers exist on purpose, not one relied on twice, and they check +different things. The registry itself (`PermissionRegistry.get`/`.has`) returns `undefined`/`false` +for anything malformed or unregistered — this is the only layer that knows what is *registered*. +`requirePermission` treats a registry miss as an immediate sanitized `403`, before principal +resolution or evaluation ever runs; `POST /api/v1/grants` performs the equivalent registry check +for the target permission named in its body (see above). Separately, `createAuthorizationEvaluator` +independently re-validates permission *shape* — it holds no registry and cannot recognize +registration at all — denying (without throwing) any malformed or unrecognized +`permission`/`baseline`/`scope` it receives. This shape check runs, and can deny, **before any +baseline is allowed to return an allowed decision** — `public` and `authenticated` are not +exempt, even though neither reads the permission value for any other reason — so a future caller +that reaches the evaluator directly, bypassing the registry entirely (for example a different +transport Core does not yet have), inherits the same fail-closed guarantee for malformed input +rather than depending on a registry check having already run upstream. The evaluator's shape check +and the registry's registration check are deliberately not merged into one: only the registry can +answer "is this permission registered," and the evaluator must remain usable — and safe against +malformed input — by a caller that has no registry to consult at all. ### Explicit and relationship-derived grants share persistence but have distinct source kinds diff --git a/docs/development/authorization.md b/docs/development/authorization.md index 01102ec..177eda3 100644 --- a/docs/development/authorization.md +++ b/docs/development/authorization.md @@ -53,6 +53,33 @@ matter: Module-owned permission declarations and a discovery/composition path for them are out of scope for this slice (deferred to the Module SDK work). +### Two layers, not one: shape versus registration + +`createAuthorizationEvaluator` (`core/src/authorization/evaluator.ts`) has no registry and is +never given one — it independently re-validates `permission`, `baseline`, and `scope` as a pure +shape check (`safeParsePermissionId`/`permissionBaselineSchema`/`safeParseAuthorizationScope`) and +denies a malformed value under **every** baseline, including `public` and `authenticated`, before +any baseline is allowed to return an allowed decision. This is defense in depth for a future +caller that reaches the evaluator directly, not a substitute for the registry: the evaluator has +no way to tell a well-formed-but-unregistered permission id (for example a plausible-looking +`calendar.events.read` that nothing has ever declared) from a genuinely registered one — it can +only reject what is not even shaped like a `PermissionId` at all. + +*Registration* — "does this permission actually exist" — is exclusively the registry's +responsibility, consulted by every trusted caller before the evaluator ever runs: +`requirePermission` (`apps/api/src/authorization-middleware.ts`) looks up the *caller's own* +required permission (for example `core.authorization.grants.manage`, always a hardcoded route +constant, never client-supplied) in the registry before resolving a principal or evaluating +anything; `POST /api/v1/grants` (`apps/api/src/grants-routes.ts`) separately looks up the *target* +permission being granted — the one named in the request body — in the same registry, after body +shape-validation and before ever calling `store.createGrant`, so a caller cannot create a dormant +grant for an invented permission id that might later become effective the moment someone actually +registers it. A malformed target permission (fails `PermissionId` shape validation) still resolves +to the body-level `400 {"code":"INVALID_REQUEST"}` documented below, unchanged; a well-formed but +unregistered target permission instead receives the identical sanitized +`403 {"code":"AUTHORIZATION_DENIED"}` a genuinely denied caller would, so that response never +reveals whether the caller was denied or the permission was simply never registered. + ## Platform-owner bootstrap There is no "first user becomes owner" inference anywhere in this repository, and no HTTP @@ -191,7 +218,7 @@ cannot smuggle a server-derived field through a request that otherwise looks wel | Field | Required | Notes | | --- | --- | --- | | `subjectUserId` | Yes | Must parse as a valid `EkklesiaUserId`. | -| `permission` | Yes | Must parse as a valid `PermissionId`. | +| `permission` | Yes | Must parse as a valid `PermissionId` **and** be registered in `CORE_PERMISSION_REGISTRY` — see [Two layers, not one](#two-layers-not-one-shape-versus-registration). A malformed value is a `400`; a well-formed but unregistered one is a `403`. | | `scope` | No | Must parse as a valid, non-empty `AuthorizationScope` if present. | | `expiresAt` | No | An ISO date string if present. | @@ -225,7 +252,7 @@ A grant, wherever returned: | --- | --- | | `200` / `201` | Success. | | `400 {"code":"INVALID_REQUEST"}` | Malformed query/body/param (including a disallowed extra field). | -| `403 {"code":"AUTHORIZATION_DENIED"}` | Caller lacks `core.authorization.grants.manage`, or the permission id was unregistered. | +| `403 {"code":"AUTHORIZATION_DENIED"}` | Caller lacks `core.authorization.grants.manage`; the caller's own required permission id was unregistered; or, for `POST /api/v1/grants`, the well-formed *target* permission named in the body is not registered. | | `403 {"code":"UNTRUSTED_ORIGIN"}` | Missing or untrusted `Origin` on a mutation route. | | `404 {"code":"AUTHORIZATION_GRANT_NOT_FOUND"}` | The targeted grant id does not exist as an *explicit* grant — this is also what revoking a real relationship-derived grant's id looks like, indistinguishably. | | `503` | Principal resolution, evaluation, or store failure (`PRINCIPAL_RESOLUTION_FAILED`, `AUTHORIZATION_EVALUATION_FAILED`, or an `AuthorizationStoreError` code). | @@ -273,15 +300,23 @@ there are or whether the failure happened during startup. - `apps/api/tests/authorization-middleware.test.ts` — allow/deny/unregistered/infrastructure- failure paths for `requirePermission`, and `requireTrustedOrigin`'s allow/reject behavior. - `apps/api/tests/grants-routes.test.ts` — all three routes' allow/deny paths, disallowed-field - and malformed-field rejection, exact server-derived-argument assertions, sourceKind-filtered - revoke returning 404 for a non-explicit grant, origin enforcement, and store-error mapping. + and malformed-field rejection, a well-formed but unregistered target permission rejected with + `403` and `store.createGrant` never called, exact server-derived-argument assertions, + sourceKind-filtered revoke returning 404 for a non-explicit grant, origin enforcement, and + store-error mapping. - `apps/api/tests/lifecycle.test.ts` — shared-pool-closes-exactly-once coverage described above. +- `core/tests/authorization-evaluator.test.ts` — includes a malformed permission id denying under + every baseline (`public`, `authenticated`, `restricted`), never just `restricted`, and never + consulting the grant or platform-owner ports for `public`/`authenticated`. - `apps/api/tests/api-http.integration.ts` (live PostgreSQL) — end-to-end: anonymous/non-owner denial, real bootstrap against the running API's own database, owner authority over the HTTP grants API, the owner delegating `core.authorization.grants.manage` to another account and that - account then managing grants as a non-owner, a create/list/revoke cycle with audit-trail - verification, disallowed-field rejection, missing/untrusted-origin rejection, a real - relationship-derived grant proven invisible to LIST and unreachable by REVOKE, and an + account then managing grants as a non-owner, a well-formed but unregistered target permission + rejected with `403` (with no grant or audit row written) even for a caller with full + grant-management authority, a create/list/revoke cycle with audit-trail verification (using + `core.authorization.grants.manage` itself as the granted permission, since module permission + registration does not exist yet), disallowed-field rejection, missing/untrusted-origin rejection, + a real relationship-derived grant proven invisible to LIST and unreachable by REVOKE, and an infrastructure-failure scenario (database dropped out from under a live session) asserting a `503` rather than a false `403` or `200`.