fix(app): Check semver validity of API returned version strings (#2492)

Opentrons · Oct 16, 2018 · d9a48bf · d9a48bf
1 parent 67c627c
commit d9a48bf
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 19 deletions.
diff --git a/app-shell/package.json b/app-shell/package.json
@@ -33,6 +33,7 @@
     "electron-updater": "^3.1.2",
     "fs-extra": "^6.0.1",
     "merge-options": "^1.0.1",
+    "semver": "^5.5.0",
     "uuid": "^3.2.1",
     "winston": "^3.1.0",
     "yargs-parser": "^10.0.0"

diff --git a/app-shell/src/api-update.js b/app-shell/src/api-update.js
@@ -3,6 +3,7 @@
 import assert from 'assert'
 import fse from 'fs-extra'
 import path from 'path'
+import semver from 'semver'
 
 import pkg from '../package.json'
 import createLogger from './log'
@@ -71,6 +72,9 @@ function getVersionFromFilename (filename: string): string {
     shortLabel && labelVersion
       ? `-${SHORT_LABEL_TO_LABEL[shortLabel]}.${labelVersion}`
       : ''
+  const version = `${baseVersion}${label}`
 
-  return `${baseVersion}${label}`
+  assert(semver.valid(version), `"${version}" is not valid semver`)
+
+  return version
 }
diff --git a/app/src/discovery/__tests__/selectors.test.js b/app/src/discovery/__tests__/selectors.test.js
@@ -361,6 +361,26 @@ describe('discovery selectors', () => {
       selector: discovery.getRobotApiVersion,
       expected: null,
     },
+    {
+      name: 'getRobotApiVersion returns API health if serverHealth invalid',
+      // TODO(mc, 2018-10-11): state is a misnomer here, maybe rename it "input"
+      state: {
+        serverHealth: {apiServerVersion: 'not available'},
+        health: {api_version: '4.5.6'},
+      },
+      selector: discovery.getRobotApiVersion,
+      expected: '4.5.6',
+    },
+    {
+      name: 'getRobotApiVersion returns null if all healths invalid',
+      // TODO(mc, 2018-10-11): state is a misnomer here, maybe rename it "input"
+      state: {
+        serverHealth: {apiServerVersion: 'not available'},
+        health: {api_version: 'also not available'},
+      },
+      selector: discovery.getRobotApiVersion,
+      expected: null,
+    },
     {
       name: 'getRobotFirmwareVersion returns serverHealth.smoothieVersion',
       // TODO(mc, 2018-10-11): state is a misnomer here, maybe rename it "input"

diff --git a/app/src/discovery/selectors.js b/app/src/discovery/selectors.js
@@ -8,6 +8,7 @@ import map from 'lodash/map'
 import mapValues from 'lodash/mapValues'
 import pickBy from 'lodash/pickBy'
 import {createSelector} from 'reselect'
+import semver from 'semver'
 
 // TODO(mc, 2018-10-10): fix circular dependency with RPC API client
 // that requires us to bypass the robot entry point here
@@ -130,8 +131,8 @@ export const getConnectedRobot: GetConnectedRobot = createSelector(
 )
 
 export const getRobotApiVersion = (robot: AnyRobot): ?string =>
-  (robot.serverHealth && robot.serverHealth.apiServerVersion) ||
-  (robot.health && robot.health.api_version)
+  (robot.serverHealth && semver.valid(robot.serverHealth.apiServerVersion)) ||
+  (robot.health && semver.valid(robot.health.api_version))
 
 export const getRobotFirmwareVersion = (robot: AnyRobot): ?string =>
   (robot.serverHealth && robot.serverHealth.smoothieVersion) ||

diff --git a/app/src/http-api-client/__tests__/server.test.js b/app/src/http-api-client/__tests__/server.test.js
@@ -69,18 +69,31 @@ describe('server API client', () => {
 
       // test upgrade available
       robot = setCurrent(robot, '3.0.0')
-      expect(getRobotUpdateInfo(state, robot)).toEqual({version, type: 'upgrade'})
+      expect(getRobotUpdateInfo(state, robot)).toEqual({
+        version,
+        type: 'upgrade',
+      })
 
       // test downgrade
       robot = setCurrent(robot, '5.0.0')
-      expect(getRobotUpdateInfo(state, robot)).toEqual({version, type: 'downgrade'})
+      expect(getRobotUpdateInfo(state, robot)).toEqual({
+        version,
+        type: 'downgrade',
+      })
 
       // test no upgrade
       robot = setCurrent(robot, '4.0.0')
       expect(getRobotUpdateInfo(state, robot)).toEqual({version, type: null})
 
+      // test bad version string
+      robot = setCurrent(robot, 'not available')
+      expect(getRobotUpdateInfo(state, robot)).toEqual({version, type: null})
+
       // test unknown robot
-      expect(getRobotUpdateInfo(state, {name: 'foo'})).toEqual({version, type: null})
+      expect(getRobotUpdateInfo(state, {name: 'foo'})).toEqual({
+        version,
+        type: null,
+      })
     })
 
     test('makeGetRobotUpdateRequest', () => {
@@ -192,11 +205,7 @@ describe('server API client', () => {
       })
 
       return fetchIgnoredUpdate(robot)(() => {}).then(() =>
-        expect(client).toHaveBeenCalledWith(
-          robot,
-          'GET',
-          'update/ignore'
-        )
+        expect(client).toHaveBeenCalledWith(robot, 'GET', 'update/ignore')
       )
     })
 
@@ -246,12 +255,9 @@ describe('server API client', () => {
       })
 
       return setIgnoredUpdate(robot, availableUpdate)(() => {}).then(() =>
-        expect(client).toHaveBeenCalledWith(
-          robot,
-          'POST',
-          'update/ignore',
-          {version: availableUpdate}
-        )
+        expect(client).toHaveBeenCalledWith(robot, 'POST', 'update/ignore', {
+          version: availableUpdate,
+        })
       )
     })
 

diff --git a/app/src/http-api-client/server.js b/app/src/http-api-client/server.js
@@ -255,11 +255,12 @@ export const makeGetRobotUpdateInfo = () => {
     RobotUpdateInfo> = createSelector(
     (_, robot) => getRobotApiVersion(robot),
     getApiUpdateVersion,
-    (current, updateVersion) => {
+    (currentVersion, updateVersion) => {
+      const current = semver.valid(currentVersion)
       const upgrade = current && semver.gt(updateVersion, current)
       const downgrade = current && semver.lt(updateVersion, current)
       let type
-      if (!current || (!upgrade && !downgrade)) {
+      if (!upgrade && !downgrade) {
         type = null
       } else {
         type = upgrade ? 'upgrade' : 'downgrade'