Address review feedback + access control fix

packages/general/src/transaction/errors.ts

@@ -42,6 +42,12 @@ export class FinalizationError extends MatterError {}
 export class TransactionDestroyedError extends MatterError {}
 
 /**
- * Trhown if a {@link Transaction} cannot commit because state has mutated continuously for too many pre-commit events.
+ * Thrown if a {@link Transaction} cannot commit because state has mutated continuously for too many pre-commit cycles.
+ *
+ * "Pre-commit" is a commit event triggered by {@link Transaction} before stage 1 commit.  During pre-commit listeners
+ * may mutate state.  If state does change, the transaction re-runs pre-commit so all listeners see the same state.
+ *
+ * If state continues to mutate for too many of these cycles then the transaction will abort.  This likely indicates a
+ * logic error that will result in an infinite loop.
  */
 export class UnsettledStateError extends FinalizationError {}

packages/node/src/behavior/cluster/ClusterBehaviorCache.ts

@@ -6,8 +6,8 @@
 
 import { Behavior } from "#behavior/Behavior.js";
 import { ClusterBehavior } from "#index.js";
+import { Schema } from "#model";
 import { ClusterType } from "#types";
-import { Schema } from "../supervision/Schema.js";
 
 /**
  * To save memory we cache behavior implementations specialized for specific clusters.  This allows for efficient

packages/node/src/behavior/context/server/OnlineContext.ts

@@ -27,6 +27,7 @@ export function OnlineContext(options: OnlineContext.Options) {
     let fabric: FabricIndex | undefined;
     let subject: SubjectId;
     let nodeProtocol: NodeProtocol | undefined;
+    let accessLevelCache: Map<AccessControl.Location, number[]> | undefined;
 
     const { exchange } = options;
     const session = exchange?.session;
@@ -167,8 +168,9 @@ export function OnlineContext(options: OnlineContext.Options) {
                 }
 
                 // We already checked access levels in this transaction, so reuse it
-                if (location.accessLevels !== undefined) {
-                    return location.accessLevels.includes(desiredAccessLevel)
+                const cachedAccessLevels = accessLevelCache?.get(location);
+                if (cachedAccessLevels !== undefined) {
+                    return cachedAccessLevels.includes(desiredAccessLevel)
                         ? AccessControl.Authority.Granted
                         : AccessControl.Authority.Unauthorized;
                 }
@@ -182,7 +184,12 @@ export function OnlineContext(options: OnlineContext.Options) {
                     throw new InternalError("AccessControlServer should already be initialized.");
                 }
                 const accessLevels = accessControl.accessLevelsFor(context, location, aclEndpointContextFor(location));
-                location.accessLevels = accessLevels;
+
+                if (accessLevelCache === undefined) {
+                    accessLevelCache = new Map();
+                }
+                accessLevelCache.set(location, accessLevels);
+
                 return accessLevels.includes(desiredAccessLevel)
                     ? AccessControl.Authority.Granted
                     : AccessControl.Authority.Unauthorized;

packages/node/src/behavior/state/managed/Datasource.ts

@@ -253,6 +253,9 @@ function configure(options: Datasource.Options): Internals {
         values[key] = initialValues[key];
     }
 
+    // Location affects security so make it immutable
+    Object.freeze(options.location);
+
     return {
         ...options,
         version: Crypto.getRandomUInt32(),

packages/node/src/node/server/TransactionalInteractionServer.ts

@@ -36,7 +36,7 @@ import {
     WriteRequest,
     WriteResponse,
 } from "#protocol";
-import { EndpointNumber, StatusCode, StatusResponseError, TlvEventFilter, TypeFromSchema } from "#types";
+import { StatusCode, StatusResponseError, TlvEventFilter, TypeFromSchema } from "#types";
 import { AccessControlServer } from "../../behaviors/access-control/AccessControlServer.js";
 import { ServerNode } from "../ServerNode.js";
 
@@ -368,7 +368,7 @@ export class TransactionalInteractionServer extends InteractionServer {
         const invokeCommand = (context: ActionContext) => {
             if (
                 context.authorityAt(command.invokeAcl, {
-                    endpoint: EndpointNumber(1),
+                    endpoint: endpoint.number,
                     cluster: path.clusterId,
                 } as AccessControl.Location) !== AccessControl.Authority.Granted
             ) {

packages/protocol/src/action/request/Invoke.ts

@@ -17,17 +17,17 @@ export interface Invoke extends InvokeRequest {
  * Request invocation of one or more commands.
  */
 export function Invoke(definition: Invoke.Definition): Invoke {
-    let { interactionModelRevision, suppressResponse, timed: timedRequest } = definition;
-    const { commands } = definition;
+    const {
+        commands,
+        interactionModelRevision = FALLBACK_INTERACTIONMODEL_REVISION,
+        suppressResponse = false,
+        timed: timedRequest = false,
+    } = definition;
 
     if (!commands?.length) {
         throw new MalformedRequestError(`Invocation requires at least one command`);
     }
 
-    interactionModelRevision ??= FALLBACK_INTERACTIONMODEL_REVISION;
-    suppressResponse ??= false;
-    timedRequest ??= false;
-
     return {
         invokeRequests: commands,
         interactionModelRevision,

packages/protocol/src/action/request/MalformedRequestError.ts

@@ -4,9 +4,11 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { MatterError } from "#general";
+import { ImplementationError } from "#general";
 
 /**
  * Thrown when an action request does not adhere to the Matter specification.
+ *
+ * This is a client-side error that throws while formulating a request.
  */
-export class MalformedRequestError extends MatterError {}
+export class MalformedRequestError extends ImplementationError {}

packages/protocol/src/action/request/Read.ts

@@ -102,7 +102,7 @@ export function Read(optionsOrSelector: Read.Options | Read.Selector, ...selecto
         const { endpoint } = selector;
 
         // Install data version filter if the endpoint reports it has complete version information
-        if (typeof endpoint === "object" && typeof endpoint.versions === "object" && typeof cluster === "object") {
+        if (typeof endpoint === "object" && endpoint?.versions && typeof cluster === "object" && cluster !== null) {
             const version = endpoint.versions?.[camelize(cluster.name)];
             if (version !== undefined) {
                 const filter = {

packages/protocol/src/action/server/AccessControl.ts

@@ -117,12 +117,6 @@ export namespace AccessControl {
          * enforcement.
          */
         owningFabric?: FabricIndex;
-
-        /**
-         * The access levels already retrieved for this location. With this subtree elements can access the same
-         * access levels without re-evaluating.
-         */
-        accessLevels?: AccessLevel[];
     }
 
     /**