v2.2.3

CHANGELOG.md

@@ -7,6 +7,8 @@ All notable changes to `@homebridge/hap-nodejs` will be documented in this file.
 ### Changes
 
 - ci: rename the pr-labeler caller job from stale to label
+- fix: stop one accessory's invalid permissions taking down the whole bridge (#1129)
+- chore(deps): dependency updates
 
 ### Homebridge Dependencies
 

package.json

@@ -52,9 +52,9 @@
     "dist"
   ],
   "dependencies": {
-    "@homebridge/ciao": "1.3.12",
-    "@homebridge/dbus-native": "0.7.9",
-    "bonjour-hap": "3.10.5",
+    "@homebridge/ciao": "^1.3.12",
+    "@homebridge/dbus-native": "^0.7.9",
+    "bonjour-hap": "^3.10.5",
     "debug": "^4.4.3",
     "fast-srp-hap": "^2.0.4",
     "futoin-hkdf": "^1.5.3",
@@ -74,7 +74,7 @@
     "@typescript-eslint/parser": "^8.67.0",
     "commander": "^15.0.0",
     "escape-html": "^1.0.3",
-    "eslint": "^10.8.1",
+    "eslint": "^10.9.0",
     "http-parser-js": "^0.5.10",
     "jest": "^30.4.2",
     "rimraf": "^6.1.3",

src/lib/Accessory.spec.ts

@@ -319,6 +319,198 @@ describe("Accessory", () => {
   });
 
   describe("publish", () => {
+    test("blocks a standalone accessory containing invalid characteristic permissions", async () => {
+      const service = accessory.addService(Service.Switch);
+      const characteristic = service.getCharacteristic(Characteristic.On);
+      characteristic.props.perms = [null as unknown as Perms, Perms.NOTIFY];
+      const publishInfo: PublishInfo = {
+        username: serverUsername,
+        pincode: "000-00-000",
+        category: Categories.SWITCH,
+      };
+
+      await expect(accessory.publish(publishInfo)).rejects.toThrow(
+        /Test Accessory.*Switch.*On.*invalid permissions/,
+      );
+      // @ts-expect-error: verify publication stopped before internal initialization
+      expect(accessory.initialized).toBe(false);
+    });
+
+    test("validates bridged accessories when the bridge is validated", () => {
+      const bridge = new Bridge("TestBridge", uuid.generate("bridge validation delegation"));
+      const bridgedAccessory = new Accessory("Bridged Accessory", uuid.generate("validated bridged accessory"));
+      bridgedAccessory.addService(Service.Switch);
+      bridge.addBridgedAccessory(bridgedAccessory);
+
+      // @ts-expect-error: spying on private method
+      const validateSpy = jest.spyOn(bridgedAccessory, "validateAccessory");
+      // @ts-expect-error: private access
+      bridge.validateAccessory(true);
+
+      expect(validateSpy).toHaveBeenCalledTimes(1);
+      validateSpy.mockRestore();
+    });
+
+    describe("quarantine of bridged accessories with invalid characteristic permissions", () => {
+      let bridge: Bridge;
+      let validAccessory: Accessory;
+      let offendingAccessory: Accessory;
+      let offendingService: Service;
+      let offendingCharacteristic: Characteristic;
+      let warningHandler: Mock;
+      let consoleErrorSpy: jest.SpyInstance;
+      let enqueueSpy: jest.SpyInstance;
+
+      // Serves the /accessories route the way a controller would and reports the aids that came back, which is everything HomeKit gets to see.
+      const servedAids = async (): Promise<number[]> => {
+        callback.mockReset();
+
+        // @ts-expect-error: private access
+        bridge.handleAccessories(connection, callback);
+        await callbackPromise;
+
+        const response = callback.mock.calls[0][1] as AccessoriesResponse;
+        return response.accessories.map(served => served.aid);
+      };
+
+      const corruptPermissions = (): void => {
+        offendingCharacteristic.props.perms = [null as unknown as Perms, Perms.NOTIFY];
+      };
+
+      beforeEach(() => {
+        bridge = new Bridge("TestBridge", uuid.generate("bridge with quarantined accessory"));
+
+        validAccessory = new Accessory("Valid Accessory", uuid.generate("valid accessory"));
+        validAccessory.addService(Service.Switch);
+
+        offendingAccessory = new Accessory("Invalid Accessory", uuid.generate("quarantined accessory"));
+        offendingService = offendingAccessory.addService(Service.Switch);
+        offendingCharacteristic = offendingService.getCharacteristic(Characteristic.On);
+
+        bridge.addBridgedAccessories([ validAccessory, offendingAccessory ]);
+        bridge._identifierCache = new IdentifierCache(serverUsername);
+
+        warningHandler = jest.fn();
+        offendingAccessory.on(AccessoryEventTypes.CHARACTERISTIC_WARNING, warningHandler);
+
+        consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {});
+        // @ts-expect-error: spying on private method
+        enqueueSpy = jest.spyOn(bridge, "enqueueConfigurationUpdate");
+      });
+
+      afterEach(() => {
+        consoleErrorSpy.mockRestore();
+        enqueueSpy.mockRestore();
+      });
+
+      test("serves the bridge and its valid accessories only, attributing the fault once", async () => {
+        corruptPermissions();
+
+        const aids = await servedAids();
+
+        expect(aids).toEqual([ bridge.aid, validAccessory.aid ]);
+        expect(aids).not.toContain(offendingAccessory.aid);
+
+        // the exclusion is presentation-level: the accessory is still bridged, and only what we serve changed
+        expect(bridge.bridgedAccessories).toContain(offendingAccessory);
+        expect(offendingAccessory.bridged).toBe(true);
+        expect(offendingAccessory.bridge).toBe(bridge);
+
+        expect(warningHandler).toHaveBeenCalledTimes(1);
+        expect(warningHandler).toHaveBeenCalledWith(expect.objectContaining({
+          characteristic: offendingCharacteristic,
+          type: CharacteristicWarningType.ERROR_MESSAGE,
+          message: expect.stringMatching(/Invalid Accessory.*excluded from the accessory database.*Switch.*On.*contains invalid permissions/),
+        }));
+      });
+
+      test("excludes an accessory whose permissions are corrupted after it was already served", async () => {
+        expect(await servedAids()).toEqual([ bridge.aid, validAccessory.aid, offendingAccessory.aid ]);
+
+        corruptPermissions();
+
+        expect(await servedAids()).toEqual([ bridge.aid, validAccessory.aid ]);
+      });
+
+      test("serves the accessory again once its permissions are corrected, under the same aid", async () => {
+        corruptPermissions();
+        expect(await servedAids()).not.toContain(offendingAccessory.aid);
+
+        const quarantinedAid = offendingAccessory.aid;
+        warningHandler.mockClear();
+        offendingCharacteristic.props.perms = [ Perms.PAIRED_READ, Perms.NOTIFY ];
+
+        expect(await servedAids()).toEqual([ bridge.aid, validAccessory.aid, offendingAccessory.aid ]);
+        expect(offendingAccessory.aid).toEqual(quarantinedAid);
+        expect(warningHandler).not.toHaveBeenCalled();
+      });
+
+      test("enqueues a configuration update on entering and on leaving quarantine", async () => {
+        await servedAids();
+        enqueueSpy.mockClear();
+
+        corruptPermissions();
+        await servedAids();
+        expect(enqueueSpy).toHaveBeenCalledTimes(1);
+
+        offendingCharacteristic.props.perms = [ Perms.PAIRED_READ, Perms.NOTIFY ];
+        await servedAids();
+        expect(enqueueSpy).toHaveBeenCalledTimes(2);
+      });
+
+      test("settles while the accessory stays quarantined, without repeating itself", async () => {
+        corruptPermissions();
+
+        await servedAids();
+        await servedAids();
+        await servedAids();
+
+        expect(warningHandler).toHaveBeenCalledTimes(1);
+        expect(enqueueSpy).toHaveBeenCalledTimes(1);
+      });
+
+      test("quarantines an accessory whose permissions cannot be JSON-serialized, without failing the request", async () => {
+        offendingCharacteristic.props.perms = [ 1n ] as unknown as Perms[];
+
+        expect(await servedAids()).toEqual([ bridge.aid, validAccessory.aid ]);
+        expect(warningHandler).toHaveBeenCalledTimes(1);
+      });
+
+      test("quarantines an accessory carrying a characteristic that was constructed with invalid permissions", async () => {
+        const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {});
+
+        const malformedCharacteristic = new Characteristic("Malformed", uuid.generate("characteristic constructed with invalid permissions"), {
+          format: Formats.BOOL,
+          perms: [null as unknown as Perms, Perms.NOTIFY],
+        });
+
+        // Construction is where a plugin hands us its own props, and a bare characteristic has nobody subscribed to it, so the console carries the warning.
+        expect(consoleWarnSpy).toHaveBeenCalledTimes(1);
+        expect(consoleWarnSpy.mock.calls[0][0]).toMatch(/Malformed.*contains invalid permissions/);
+
+        offendingService.addCharacteristic(malformedCharacteristic);
+
+        expect(await servedAids()).toEqual([ bridge.aid, validAccessory.aid ]);
+        expect(warningHandler).toHaveBeenCalledTimes(1);
+        expect(warningHandler).toHaveBeenCalledWith(expect.objectContaining({
+          characteristic: malformedCharacteristic,
+          type: CharacteristicWarningType.ERROR_MESSAGE,
+        }));
+
+        consoleWarnSpy.mockRestore();
+      });
+
+      test("excludes the same accessory from the configuration that determines the configuration number", async () => {
+        corruptPermissions();
+
+        const aids = await servedAids();
+        // @ts-expect-error: private access
+        const configuration = bridge.internalHAPRepresentation();
+
+        expect(configuration.map(entry => entry.aid)).toEqual(aids);
+      });
+    });
+
     test.each`
       advertiser                 | republish
       ${MDNSAdvertiser.BONJOUR}  | ${false}