fix(plugin-security): the app default permission set resolves from the first level that NAMES one (#15298)

.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.

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);
+    });
   });
 });

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,52 +115,83 @@ 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
  * occupies. ⛔ Do not iterate `config.packages` directly here; a second
  * 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;
 }