diff --git a/.changeset/plugin-security-default-set-answer-not-container.md b/.changeset/plugin-security-default-set-answer-not-container.md new file mode 100644 index 0000000000..a128ef51d6 --- /dev/null +++ b/.changeset/plugin-security-default-set-answer-not-container.md @@ -0,0 +1,57 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298) + +`declaredPermissionSets` carried a docblock stating a short-circuit its code did +not have: + +> The `packages[]` pass only supplies a set where the top level had none — which +> is precisely the option-B artifact. + +The code pushed the flattened top level and then **every** package body +unconditionally, so on today's additive artifact (flattened level *and* +`packages[]` both present) every permission set was collected twice. Nothing +observable came of it — the sole caller is private and takes the first +`isDefault` set, which the flattened copy still supplied — so this corrects a +false written contract on a security-path reader, not a live defect. That +distinction is the point: the sentence was load-bearing, because it was the +stated reason the reader half was revertible on its own and safe to land before +the emitter half (#14512), and the next reader would have believed the mechanism +was there. + +⚠️ Release-notes note: this supersedes one sentence of the #15226 entry in this same +unreleased batch — "The resolution now reads the flattened top level FIRST and then each +package body". That described #15226 accurately when it landed; after this change the +`packages[]` pass runs only where the top level named no default. The earlier entry is +left as written rather than retro-edited, so whoever compiles the notes collapses the two +deliberately instead of reading a contradiction. + +The reader now walks the discipline the docblock claims — start from the +expression this program replaced, `appDefaultPermissionSetName(config.permissions)`, +and consult `packages[]` only where it came back `undefined`. + +- **The condition is the resolved NAME, never the `permissions` container.** + Branching on the container re-creates the silent loss the reader program + exists to remove, one shape further along: a flattened level that carries + permission sets but marks none of them `isDefault` is legal today and + hand-authorable in any `objectstack.config.ts`, and a container-shaped + condition (`Array.isArray(flattened)`, with or without `&& length > 0`) shorts + it past the whole `packages[]` pass and answers `undefined` — nothing thrown, + nothing logged, every member of the app back down to the platform floor alone. + Reading the answer also retires the `[]`-is-truthy trap rather than patching + around it. +- **The package order is resolved BEFORE the top level is consulted.** + `resolveArtifactPackageOrder` refuses a malformed `packages` — not an array, + an entry inlined instead of wrapped under `manifest:`, a duplicate package id + — with an ADR-0112 envelope this reader does not catch, and that refusal must + not become conditional on whether the flattened level happened to name a + default first. An artifact is either loadable or refused; which level answered + is not part of that question. +- **No emitted artifact changes its answer.** Measured, not argued: 26 shapes — + the composed additive artifact, its option-B derivative, the collection-zoo + fixtures behind the #15004 acceptance pin, every config the unit suite drives, + the three malformed-`packages` refusals, and the hand-authored mixed shapes — + return byte-identical results before and after, with `@objectstack/plugin-security` + rebuilt and the change proven present in `dist/` on each leg. diff --git a/packages/plugins/plugin-security/src/app-default-permission-set.test.ts b/packages/plugins/plugin-security/src/app-default-permission-set.test.ts index 4142f46557..3cd5475f83 100644 --- a/packages/plugins/plugin-security/src/app-default-permission-set.test.ts +++ b/packages/plugins/plugin-security/src/app-default-permission-set.test.ts @@ -259,6 +259,56 @@ describe('appSecurityPluginOptions over `packages[]` (ADR-0130 D4, #15007)', () expect(appSecurityPluginOptions({ permissions: [permissionSet('top')] })).toEqual({ fallbackPermissionSet: 'top' }); }); + /** + * [#15007 follow-up] "The top level had none" is the resolved NAME coming + * back `undefined` — never the `permissions` CONTAINER being absent or empty. + * + * Branching on the container re-creates the silent loss this card removed, + * one shape further along. A flattened level that carries permission sets but + * marks none of them `isDefault` is legal today and hand-authorable in any + * `objectstack.config.ts`; a container-shaped condition shorts it past the + * whole `packages[]` pass and answers `undefined` — nothing thrown, nothing + * logged, every member of the app back down to the platform floor alone. + */ + describe('the `packages[]` pass runs wherever the top level named no default', () => { + const corePackage = { + manifest: { + id: CORE_ID, name: 'Core', version: '1.0.0', type: 'app', + permissions: [permissionSet(CORE_PROFILE)], + }, + }; + + it('an EMPTY flattened array does not short-circuit it', () => { + expect(appSecurityPluginOptions({ permissions: [], packages: [corePackage] })) + .toEqual({ fallbackPermissionSet: CORE_PROFILE }); + }); + + it('a NON-EMPTY flattened array that marks no default does not either', () => { + // A `permissions.length > 0` guard passes the case above and fails this + // one — which is the whole reason the condition is the resolved name. + expect( + appSecurityPluginOptions({ + permissions: [{ name: 'core_read_only', label: 'Read only', objects: {} }], + packages: [corePackage], + }), + ).toEqual({ fallbackPermissionSet: CORE_PROFILE }); + }); + + it('a `permissions` key that is not an array at all does not either', () => { + expect(appSecurityPluginOptions({ permissions: null, packages: [corePackage] })) + .toEqual({ fallbackPermissionSet: CORE_PROFILE }); + }); + + it('and once the top level DOES name one, the packages pass cannot change the answer', () => { + expect( + appSecurityPluginOptions({ + permissions: [permissionSet('flattened_wins')], + packages: [corePackage], + }), + ).toEqual({ fallbackPermissionSet: 'flattened_wins' }); + }); + }); + /** * The gate travels with the read: `resolveArtifactPackageOrder` refuses a * malformed `packages` with an ADR-0112 envelope, and this reader does not @@ -293,5 +343,21 @@ describe('appSecurityPluginOptions over `packages[]` (ADR-0130 D4, #15007)', () expect(err.code).toBe('DUPLICATE_ARTIFACT_PACKAGE'); expect(err.status).toBe(422); }); + + it('…and refused just the same when the flattened top level already named a default', () => { + // The package order is resolved BEFORE the top level is consulted, so an + // artifact is either loadable or refused independently of which level + // happens to answer. Move that resolution below the early return and this + // pair turns into a silent accept: a permission surface resolved out of an + // artifact the manifest service refuses moments later. + const notAnArray = refusalOf({ permissions: [permissionSet('flattened_wins')], packages: 'nope' }); + expect(notAnArray.code).toBe('INVALID_ARTIFACT_PACKAGES'); + expect(notAnArray.status).toBe(422); + + const entry = { manifest: { id: CORE_ID, name: 'Core', version: '1.0.0', type: 'app', permissions: [permissionSet(CORE_PROFILE)] } }; + const duplicate = refusalOf({ permissions: [permissionSet('flattened_wins')], packages: [entry, entry] }); + expect(duplicate.code).toBe('DUPLICATE_ARTIFACT_PACKAGE'); + expect(duplicate.status).toBe(422); + }); }); }); diff --git a/packages/plugins/plugin-security/src/app-default-permission-set.ts b/packages/plugins/plugin-security/src/app-default-permission-set.ts index 727c845520..0ab95e533c 100644 --- a/packages/plugins/plugin-security/src/app-default-permission-set.ts +++ b/packages/plugins/plugin-security/src/app-default-permission-set.ts @@ -86,8 +86,9 @@ export function appDefaultPermissionSetName(permissions: unknown): string | unde } /** - * [ADR-0130 D4, #15007] Every permission set a stack config DECLARES — from the - * flattened top level, and from `packages[]`. + * [ADR-0130 D4, #15007] The app-declared default permission-set NAME, resolved + * from wherever the artifact carries the declaration — the flattened top + * level, or `packages[]`. * * ## What this exists to stop * @@ -114,15 +115,41 @@ export function appDefaultPermissionSetName(permissions: unknown): string | unde * this function has to be a superset of the old read rather than a replacement * for it: for every artifact the platform emits today the flattened level * answers first and this returns exactly what it returned before. The - * `packages[]` pass only supplies a set where the top level had none — which is - * precisely the option-B artifact. That is what makes this card revertible on - * its own and safe to land before the emitter half (#14512). + * `packages[]` pass is consulted ONLY where the top level named no default — + * which is precisely the option-B artifact. That is what makes this card + * revertible on its own and safe to land before the emitter half (#14512). + * + * ## The condition is the ANSWER, never the container + * + * "The top level had none" is spelled as `appDefaultPermissionSetName` coming + * back `undefined`, and deliberately NOT as the `permissions` array being + * absent or empty. Branching on the container re-creates the silent loss this + * card exists to remove, one shape further along: a config whose flattened + * level carries permission sets but marks none of them `isDefault` — legal + * today, and expressible by hand in any `objectstack.config.ts` — would + * short-circuit the whole `packages[]` pass and resolve `undefined`, with + * nothing thrown and nothing logged. Reading the container also hands back the + * `[]`-is-truthy trap for free. The answer is the only condition that cannot + * be wrong in either direction, so the answer is what this branches on. + * + * ⚠️ That is deliberately NOT the shape of the sibling reader's condition, and + * the difference is a property of the readers, not an inconsistency to + * converge away. `resolveStackCollection` (`packages/cli/src/utils/ + * stack-collections.ts`, #15006) branches on the CONTAINER — `if + * (Array.isArray(top)) return top;` — and is right to: it returns a whole + * collection, so a top level that carries the key has, by construction, + * already answered, and `composeStacks` flattened that array into the union. + * This reader extracts a DISTINGUISHED ELEMENT out of the collection instead, + * so "the key is present" and "the key answers" are two different facts here + * and one of them is the wrong one to branch on. Same discipline — start from + * the expression this program replaced, consult `packages[]` only where it came + * back empty — read against what each reader's expression actually returns. * * ## The order is `resolveArtifactPackageOrder`'s, not the array's * - * `appDefaultPermissionSetName` resolves the FIRST `isDefault` set, so with more - * than one package declaring one, "first" has to mean the same thing here as it - * does everywhere else the artifact is read. `resolveArtifactPackageOrder` + * The first package body that names a default wins, so with more than one + * package declaring one, "first" has to mean the same thing here as it does + * everywhere else the artifact is read. `resolveArtifactPackageOrder` * (`@objectstack/core`, ADR-0130 D4+D5, #14643) is the ONE place that turns an * artifact into its ordered package list — dependency-topological, so a package * that extends another is read after it regardless of which array slot it @@ -130,36 +157,41 @@ export function appDefaultPermissionSetName(permissions: unknown): string | unde * traversal is a second ordering, and the depended-upon package would win or * lose by authoring accident. * - * ## Two things it deliberately does NOT do - * - * • It does not look inside the SINGULAR `manifest`. That constraint is - * #7001's and it still holds — the harness must not honour a declaration - * `serve.ts` ignores. Note this is not a special case bolted on: an - * artifact carrying no `packages` key makes `resolveArtifactPackageOrder` - * return the caller's own object as the single package body (D4's second - * branch, D7's compatibility term), so that branch reads `permissions` from - * exactly where the old code read it and nowhere else. - * • It does not catch `resolveArtifactPackageOrder`'s refusals. A malformed - * `packages` (not an array, an unwrapped entry, a duplicate package id) - * raises an ADR-0112 envelope here, the same one the manifest service - * raises when it registers that artifact moments later. Swallowing it would - * resolve a permission surface out of an artifact the loader refuses to - * load — the gate travels with the read. + * ## The package order is resolved BEFORE the top level is consulted + * + * Reading that line as a misplaced statement is the expected mistake, so: it is + * placed there on purpose, and moving it below the early return is a behaviour + * change. `resolveArtifactPackageOrder` REFUSES a malformed `packages` (not an + * array, an unwrapped entry, a duplicate package id) with an ADR-0112 envelope, + * and this reader does not catch it — swallowing it would resolve a permission + * surface out of an artifact the loader refuses to load. Resolving the order + * first is what keeps that refusal unconditional: an artifact is either + * loadable or refused, and which answer this reader gives about it must not + * depend on whether its flattened level happened to name a default first. + * + * ## One thing it deliberately does NOT do + * + * It does not look inside the SINGULAR `manifest`. That constraint is #7001's + * and it still holds — the harness must not honour a declaration `serve.ts` + * ignores. Note this is not a special case bolted on: an artifact carrying no + * `packages` key never reaches the package pass at all, so that branch reads + * `permissions` from exactly where the old code read it and nowhere else. */ -function declaredPermissionSets(config: unknown): unknown[] { - const sets: unknown[] = []; +function declaredDefaultPermissionSetName(config: unknown): string | undefined { + const packages = (config as { packages?: unknown } | null | undefined)?.packages; + const bodies = + packages === undefined || packages === null ? [] : resolveArtifactPackageOrder(config); const flattened = (config as { permissions?: unknown } | null | undefined)?.permissions; - if (Array.isArray(flattened)) sets.push(...flattened); - - const packages = (config as { packages?: unknown } | null | undefined)?.packages; - if (packages === undefined || packages === null) return sets; + const fromFlattened = appDefaultPermissionSetName(flattened); + if (fromFlattened !== undefined) return fromFlattened; - for (const body of resolveArtifactPackageOrder(config)) { + for (const body of bodies) { const declared = (body as { permissions?: unknown } | null | undefined)?.permissions; - if (Array.isArray(declared)) sets.push(...declared); + const fromPackage = appDefaultPermissionSetName(declared); + if (fromPackage !== undefined) return fromPackage; } - return sets; + return undefined; } /** @@ -191,13 +223,14 @@ function declaredPermissionSets(config: unknown): unknown[] { * the result straight through — `new SecurityPlugin(appSecurityPluginOptions(config))` * — and a caller cannot get the undefined case subtly wrong. * - * Reads the sets through {@link declaredPermissionSets} — the flattened top - * level `serve.ts` has always read, and, for a multi-package artifact, the - * `packages[]` bodies that carry the same declaration under ADR-0130 D4. + * Resolves the name through {@link declaredDefaultPermissionSetName} — the + * flattened top level `serve.ts` has always read, and, for a multi-package + * artifact, the `packages[]` bodies that carry the same declaration under + * ADR-0130 D4. */ export function appSecurityPluginOptions( config: unknown, ): { fallbackPermissionSet: string } | undefined { - const name = appDefaultPermissionSetName(declaredPermissionSets(config)); + const name = declaredDefaultPermissionSetName(config); return name ? { fallbackPermissionSet: name } : undefined; }