Add attribute ID validation and fix null UserWriteMask handling

opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/AccessController.java

@@ -102,8 +102,16 @@ Map<DeleteReferencesItem, AccessResult> checkDeleteReferencesAccess(
   /** The result of an access control check. */
   sealed interface AccessResult {
 
+    /** Access is allowed. */
     AccessResult ALLOWED = new Allowed();
+
+    /** Access is denied due to an invalid attribute id. */
+    AccessResult DENIED_ATTRIBUTE_ID_INVALID = new Denied(StatusCodes.Bad_AttributeIdInvalid);
+
+    /** Access is denied due to insufficient user access rights. */
     AccessResult DENIED_USER_ACCESS = new Denied(StatusCodes.Bad_UserAccessDenied);
+
+    /** Access is denied due to insufficient security mode. */
     AccessResult DENIED_SECURITY_MODE = new Denied(StatusCodes.Bad_SecurityModeInsufficient);
 
     /** Access is allowed. */

opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessController.java

@@ -69,6 +69,12 @@ static Map<ReadValueId, AccessResult> checkReadAccess(
 
     Map<NodeId, AccessControlAttributes> attributes = context.readAccessControlAttributes(nodeIds);
 
+    for (PendingResult<ReadValueId> p : pending) {
+      if (!AttributeId.isValid(p.value.getAttributeId())) {
+        p.result = AccessResult.DENIED_ATTRIBUTE_ID_INVALID;
+      }
+    }
+
     checkAccessRestrictions(context, pending, attributes, ReadValueId::getNodeId);
 
     for (PendingResult<ReadValueId> p : pending) {
@@ -122,12 +128,19 @@ public Map<WriteValue, AccessResult> checkWriteAccess(
 
   static Map<WriteValue, AccessResult> checkWriteAccess(
       AccessControlContext context, List<WriteValue> writeValues) {
+
     List<PendingResult<WriteValue>> pending = writeValues.stream().map(PendingResult::new).toList();
 
     List<NodeId> nodeIds = writeValues.stream().map(WriteValue::getNodeId).toList();
 
     Map<NodeId, AccessControlAttributes> attributes = context.readAccessControlAttributes(nodeIds);
 
+    for (PendingResult<WriteValue> p : pending) {
+      if (!AttributeId.isValid(p.value.getAttributeId())) {
+        p.result = AccessResult.DENIED_ATTRIBUTE_ID_INVALID;
+      }
+    }
+
     checkAccessRestrictions(context, pending, attributes, WriteValue::getNodeId);
 
     for (PendingResult<WriteValue> p : pending) {
@@ -177,19 +190,20 @@ static Map<WriteValue, AccessResult> checkWriteAccess(
         }
       } else {
         UInteger userWriteMask = attributes.get(nodeId).userWriteMask();
-        Set<WriteMask> userWriteMasks =
-            userWriteMask == null ? Set.of() : WriteMask.fromMask(userWriteMask);
+        if (userWriteMask != null) {
+          Set<WriteMask> userWriteMasks = WriteMask.fromMask(userWriteMask);
 
-        // The value of the UserWriteMask attribute implicitly accounts for whether Roles and
-        // Permission are configured and if the current Session is assigned a role that includes the
-        // WriteAttribute PermissionType bit.
-        boolean hasAccess =
-            AttributeId.from(attributeId)
-                .map(id -> userWriteMasks.contains(WriteMask.forAttribute(id)))
-                .orElse(false);
+          // The value of the UserWriteMask attribute implicitly accounts for whether Roles and
+          // Permission are configured and if the current Session is assigned a role that includes
+          // the WriteAttribute PermissionType bit.
+          boolean hasAccess =
+              AttributeId.from(attributeId)
+                  .map(id -> userWriteMasks.contains(WriteMask.forAttribute(id)))
+                  .orElse(false);
 
-        if (!hasAccess) {
-          p.result = AccessResult.DENIED_USER_ACCESS;
+          if (!hasAccess) {
+            p.result = AccessResult.DENIED_USER_ACCESS;
+          }
         }
       }
     }

opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java

@@ -134,6 +134,21 @@ void checkReadAccess_RolePermissions_Denied() {
     assertEquals(AccessResult.DENIED_USER_ACCESS, result);
   }
 
+  @Test
+  void checkReadAccess_InvalidAttributeId() {
+    var nodeId = new NodeId(1, "foo");
+    var invalidAttributeId = UInteger.valueOf(9999);
+    var readValueId = new ReadValueId(nodeId, invalidAttributeId, null, null);
+
+    var attributes = new AccessControlAttributes(null, null, null, null, null, null);
+    attributesMap.put(nodeId, attributes);
+
+    AccessResult result =
+        DefaultAccessController.checkReadAccess(context, List.of(readValueId)).get(readValueId);
+
+    assertEquals(AccessResult.DENIED_ATTRIBUTE_ID_INVALID, result);
+  }
+
   @Test
   void checkWriteAccess_Value_Allowed() {
     var nodeId = new NodeId(1, "foo");
@@ -170,6 +185,22 @@ void checkWriteAccess_Value_Denied() {
     assertEquals(AccessResult.DENIED_USER_ACCESS, result);
   }
 
+  @Test
+  void checkWriteAccess_InvalidAttributeId() {
+    var nodeId = new NodeId(1, "foo");
+    var invalidAttributeId = UInteger.valueOf(9999);
+    var writeValue =
+        new WriteValue(nodeId, invalidAttributeId, null, DataValue.valueOnly(Variant.NULL_VALUE));
+
+    var attributes = new AccessControlAttributes(null, null, null, null, null, null);
+    attributesMap.put(nodeId, attributes);
+
+    AccessResult result =
+        DefaultAccessController.checkWriteAccess(context, List.of(writeValue)).get(writeValue);
+
+    assertEquals(AccessResult.DENIED_ATTRIBUTE_ID_INVALID, result);
+  }
+
   @Test
   void checkWriteAccess_AttributeProtectedByUserWriteMask_Allowed() {
     var nodeId = new NodeId(1, "wm-node");
@@ -228,6 +259,46 @@ void checkWriteAccess_AttributeProtectedByUserWriteMask_Denied() {
     assertEquals(AccessResult.DENIED_USER_ACCESS, results.get(wvBrowseName));
   }
 
+  @Test
+  void checkWriteAccess_NullUserWriteMask_Allowed() {
+    // Simulate a non-existent node - all attributes would be null/unreadable
+    var nonExistentNodeId = new NodeId(1, "non-existent-node");
+
+    var wvDisplayName =
+        new WriteValue(
+            nonExistentNodeId,
+            AttributeId.DisplayName.uid(),
+            null,
+            DataValue.valueOnly(Variant.NULL_VALUE));
+    var wvDescription =
+        new WriteValue(
+            nonExistentNodeId,
+            AttributeId.Description.uid(),
+            null,
+            DataValue.valueOnly(Variant.NULL_VALUE));
+    var wvBrowseName =
+        new WriteValue(
+            nonExistentNodeId,
+            AttributeId.BrowseName.uid(),
+            null,
+            DataValue.valueOnly(Variant.NULL_VALUE));
+
+    // For a non-existent node, all attributes including userWriteMask would be null.
+    // The access check should return ALLOWED so the operation proceeds and fails
+    // later with Bad_NodeIdUnknown rather than incorrectly returning Bad_UserAccessDenied.
+    attributesMap.put(
+        nonExistentNodeId, new AccessControlAttributes(null, null, null, null, null, null));
+    Mockito.when(context.getRoleIds()).thenReturn(Optional.of(List.of()));
+
+    var results =
+        DefaultAccessController.checkWriteAccess(
+            context, List.of(wvDisplayName, wvDescription, wvBrowseName));
+
+    assertEquals(AccessResult.ALLOWED, results.get(wvDisplayName));
+    assertEquals(AccessResult.ALLOWED, results.get(wvDescription));
+    assertEquals(AccessResult.ALLOWED, results.get(wvBrowseName));
+  }
+
   @Test
   void checkWriteAccess_RolePermissions() {
     var nodeId = new NodeId(1, "foo");