From 85efa12707b064373745b2b74ffd84270bde8797 Mon Sep 17 00:00:00 2001
From: Patrick Seal <code@plasticrake.com>
Date: Sun, 5 Nov 2023 13:02:26 -0800
Subject: [PATCH] feat: catch more invalid discovery messages

Client#startDiscovery has been changed to emit Client#discovery-invalid
Instead of throwing an error in more cases than before when
processing incoming messages
---
 src/client.ts      | 134 +++++++++++++++++++++++++--------------------
 test/client.js     | 111 +++++++++++++++++++++++++++++++++++++
 test/setup-test.js |   7 +++
 3 files changed, 194 insertions(+), 58 deletions(-)

diff --git a/src/client.ts b/src/client.ts
index 3284b85..799ea39 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -559,77 +559,95 @@ export default class Client extends EventEmitter implements ClientEventEmitter {
           `client.startDiscovery(): socket:message From: ${rinfo.address} ${rinfo.port} Message: ${decryptedMsg}`
         );
 
-        let response: DiscoveryResponse;
-        let sysInfo: Sysinfo;
         try {
-          response = JSON.parse(decryptedMsg);
-          sysInfo = response.system.get_sysinfo;
-        } catch (err) {
-          this.log.debug(
-            `client.startDiscovery(): Error parsing JSON: %s\nFrom: ${rinfo.address} ${rinfo.port} Original: [%s] Decrypted: [${decryptedMsg}]`,
-            err,
-            msg
-          );
-          this.emit('discovery-invalid', {
-            rinfo,
-            response: msg,
-            decryptedResponse: decrypt(msg),
-          });
-          return;
-        }
-
-        if (deviceTypes && deviceTypes.length > 0) {
-          const deviceType = this.getTypeFromSysInfo(sysInfo);
-          if (!(deviceTypes as string[]).includes(deviceType)) {
+          // TODO: Type checking of response/sysInfo could be improved
+          let response: DiscoveryResponse;
+          let sysInfo: Sysinfo;
+          try {
+            response = JSON.parse(decryptedMsg);
+            sysInfo = response.system.get_sysinfo;
+            if (sysInfo == null)
+              throw new Error('system.get_sysinfo is null or undefined');
+            if (!isObjectLike(sysInfo))
+              throw new Error('system.get_sysinfo is not an object');
+          } catch (err) {
             this.log.debug(
-              `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${deviceType}), allowed device types: (%j)`,
-              deviceTypes
+              `client.startDiscovery(): Error parsing JSON: %s\nFrom: ${rinfo.address} ${rinfo.port} Original: [%s] Decrypted: [${decryptedMsg}]`,
+              err,
+              msg
             );
+            this.emit('discovery-invalid', {
+              rinfo,
+              response: msg,
+              decryptedResponse: decryptedMsg,
+            });
             return;
           }
-        }
 
-        let mac: string;
-        if ('mac' in sysInfo) mac = sysInfo.mac;
-        else if ('mic_mac' in sysInfo) mac = sysInfo.mic_mac;
-        else if ('ethernet_mac' in sysInfo) mac = sysInfo.ethernet_mac;
-        else mac = '';
+          if (deviceTypes && deviceTypes.length > 0) {
+            const deviceType = this.getTypeFromSysInfo(sysInfo);
+            if (!(deviceTypes as string[]).includes(deviceType)) {
+              this.log.debug(
+                `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${deviceType}), allowed device types: (%j)`,
+                deviceTypes
+              );
+              return;
+            }
+          }
 
-        if (macAddresses && macAddresses.length > 0) {
-          if (!compareMac(mac, macAddresses)) {
-            this.log.debug(
-              `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${mac}), allowed macs: (%j)`,
-              macAddresses
-            );
-            return;
+          let mac: string;
+          if ('mac' in sysInfo) mac = sysInfo.mac;
+          else if ('mic_mac' in sysInfo) mac = sysInfo.mic_mac;
+          else if ('ethernet_mac' in sysInfo) mac = sysInfo.ethernet_mac;
+          else mac = '';
+
+          if (macAddresses && macAddresses.length > 0) {
+            if (!compareMac(mac, macAddresses)) {
+              this.log.debug(
+                `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${mac}), allowed macs: (%j)`,
+                macAddresses
+              );
+              return;
+            }
           }
-        }
 
-        if (excludeMacAddresses && excludeMacAddresses.length > 0) {
-          if (compareMac(mac, excludeMacAddresses)) {
-            this.log.debug(
-              `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${mac}), excluded mac`
-            );
-            return;
+          if (excludeMacAddresses && excludeMacAddresses.length > 0) {
+            if (compareMac(mac, excludeMacAddresses)) {
+              this.log.debug(
+                `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}] (${mac}), excluded mac`
+              );
+              return;
+            }
           }
-        }
 
-        if (typeof filterCallback === 'function') {
-          if (!filterCallback(sysInfo)) {
-            this.log.debug(
-              `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}], callback`
-            );
-            return;
+          if (typeof filterCallback === 'function') {
+            if (!filterCallback(sysInfo)) {
+              this.log.debug(
+                `client.startDiscovery(): Filtered out: ${sysInfo.alias} [${sysInfo.deviceId}], callback`
+              );
+              return;
+            }
           }
-        }
 
-        this.createOrUpdateDeviceFromSysInfo({
-          sysInfo,
-          host: rinfo.address,
-          port: devicesUseDiscoveryPort ? rinfo.port : undefined,
-          breakoutChildren,
-          options: deviceOptions,
-        });
+          this.createOrUpdateDeviceFromSysInfo({
+            sysInfo,
+            host: rinfo.address,
+            port: devicesUseDiscoveryPort ? rinfo.port : undefined,
+            breakoutChildren,
+            options: deviceOptions,
+          });
+        } catch (err) {
+          this.log.debug(
+            `client.startDiscovery(): Error processing response: %s\nFrom: ${rinfo.address} ${rinfo.port} Original: [%s] Decrypted: [${decryptedMsg}]`,
+            err,
+            msg
+          );
+          this.emit('discovery-invalid', {
+            rinfo,
+            response: msg,
+            decryptedResponse: decrypt(msg),
+          });
+        }
       });
 
       socket.on('error', (err) => {
diff --git a/test/client.js b/test/client.js
index d631f32..2e420a6 100644
--- a/test/client.js
+++ b/test/client.js
@@ -1,6 +1,10 @@
 /* eslint-disable no-unused-expressions */
 
 const sinon = require('sinon');
+const dgram = require('dgram');
+const EventEmitter = require('events');
+const { encrypt } = require('tplink-smarthome-crypto');
+
 const { config, expect, getTestClient, testDevices } = require('./setup');
 
 const { default: Client } = require('../src/client');
@@ -10,6 +14,22 @@ const { default: Bulb } = require('../src/bulb');
 
 const { compareMac } = require('../src/utils');
 
+const validPlugDiscoveryResponse = {
+  system: {
+    get_sysinfo: {
+      alias: 'test',
+      deviceId: 'test',
+      model: 'test',
+      sw_ver: 'test',
+      hw_ver: 'test',
+      type: 'plug',
+      mac: 'test',
+      feature: 'test',
+      relay_state: 0,
+    },
+  },
+};
+
 describe('Client', function () {
   describe('constructor', function () {
     it('should use custom logger', function () {
@@ -181,6 +201,97 @@ describe('Client', function () {
         });
     });
 
+    it('should ignore invalid devices that respond without encryption', function (done) {
+      const socket = new EventEmitter();
+
+      const createSocket = function () {
+        socket.bind = sinon.fake();
+        socket.address = () => ({ address: '1.2.3.4', port: 1234 });
+        socket.setBroadcast = sinon.fake();
+        return socket;
+      };
+
+      sinon.replace(dgram, 'createSocket', createSocket);
+
+      const message = JSON.stringify(validPlugDiscoveryResponse);
+
+      client
+        .startDiscovery({ discoveryInterval: 250 })
+        .on('device-new', (device) => {
+          client.stopDiscovery();
+          done(new Error(`Device should have been ignored: ${device.host}`));
+        })
+        .on('discovery-invalid', ({ rinfo, response }) => {
+          expect(rinfo.address).to.eql('1.2.3.5');
+          expect(rinfo.port).to.eql(1235);
+          expect(response).to.eql(message);
+          done();
+        });
+
+      socket.emit('message', message, {
+        address: '1.2.3.5',
+        port: 1235,
+      });
+    });
+
+    describe('should ignore invalid devices that respond without valid response', function () {
+      [
+        JSON.stringify(''),
+        JSON.stringify('data'),
+        JSON.stringify({}),
+        JSON.stringify({ unexpected: 'data' }),
+        JSON.stringify({ system: undefined }),
+        JSON.stringify({ system: {} }),
+        JSON.stringify({ system: 'data' }),
+        JSON.stringify({ system: { get_sysinfo: undefined } }),
+        JSON.stringify({ system: { get_sysinfo: {} } }),
+        JSON.stringify({ system: { get_sysinfo: 'data' } }),
+        JSON.stringify({ system: { get_sysinfo: { alias: 'test' } } }),
+      ].forEach((t) => {
+        ['encrypted', 'unencrypted'].forEach((te) => {
+          it(`${t} - ${te}`, function (done) {
+            const socket = new EventEmitter();
+
+            const createSocket = function () {
+              socket.bind = sinon.fake();
+              socket.address = () => ({ address: '1.2.3.4', port: 1234 });
+              socket.setBroadcast = sinon.fake();
+              return socket;
+            };
+
+            sinon.replace(dgram, 'createSocket', createSocket);
+
+            let message;
+            if (te === 'encrypted') {
+              message = encrypt(t);
+            } else {
+              message = t;
+            }
+
+            client
+              .startDiscovery({ discoveryInterval: 250 })
+              .on('device-new', (device) => {
+                client.stopDiscovery();
+                done(
+                  new Error(`Device should have been ignored: ${device.host}`)
+                );
+              })
+              .on('discovery-invalid', ({ rinfo, response }) => {
+                expect(rinfo.address).to.eql('1.2.3.5');
+                expect(rinfo.port).to.eql(1235);
+                expect(response).to.eql(message);
+                done();
+              });
+
+            socket.emit('message', message, {
+              address: '1.2.3.5',
+              port: 1235,
+            });
+          });
+        });
+      });
+    });
+
     const events = ['new', 'online', 'offline'];
     const eventTests = [];
     [
diff --git a/test/setup-test.js b/test/setup-test.js
index 2b702cc..34b7d88 100644
--- a/test/setup-test.js
+++ b/test/setup-test.js
@@ -1,3 +1,5 @@
+const sinon = require('sinon');
+
 const {
   config,
   expect,
@@ -112,3 +114,8 @@ describe('Test Environment Setup', function () {
     });
   });
 });
+
+afterEach(() => {
+  // Restore the default sandbox here
+  sinon.restore();
+});