fix(app): Remove Electron RPC remote objects from Redux state (#3820)

Some of the remote calls from the app to the shell were leaking Electron RPC remote objects into the Redux state. If the main process does anything weird to these remote objects, the Redux state might be mutated under our feet. This commit removes unnecessary remote calls and ensure returned objects are cloned into plain-old JS objects.
Opentrons · Aug 6, 2019 · d5f3fe3 · d5f3fe3
1 parent f1c9d3a
commit d5f3fe3
Show file tree

Hide file tree

Showing 16 changed files with 252 additions and 239 deletions.
diff --git a/app-shell/src/main.js b/app-shell/src/main.js
@@ -43,7 +43,7 @@ function startUp() {
 
   // wire modules to UI dispatches
   const dispatch = action => {
-    log.debug('Sending action via IPC to renderer', { action })
+    log.silly('Sending action via IPC to renderer', { action })
     mainWindow.webContents.send('dispatch', action)
   }
 

diff --git a/app-shell/src/preload.js b/app-shell/src/preload.js
@@ -3,11 +3,19 @@
 // defines subset of Electron API that renderer process is allowed to access
 // for security reasons
 import { ipcRenderer, remote } from 'electron'
+import cloneDeep from 'lodash/cloneDeep'
+
+const { getConfig } = remote.require('./config')
+const { getRobots } = remote.require('./discovery')
+const { CURRENT_VERSION, CURRENT_RELEASE_NOTES } = remote.require('./update')
 
 global.APP_SHELL_REMOTE = {
   ipcRenderer,
+  CURRENT_VERSION,
+  CURRENT_RELEASE_NOTES,
+  INITIAL_CONFIG: cloneDeep(getConfig()),
+  INITIAL_ROBOTS: getRobots(),
+
+  // TODO(mc, 2019-08-05): remove when we remove __buildrootEnabled FF
   apiUpdate: remote.require('./api-update'),
-  config: remote.require('./config'),
-  discovery: remote.require('./discovery'),
-  update: remote.require('./update'),
 }
diff --git a/app/src/config/__tests__/config.test.js b/app/src/config/__tests__/config.test.js
@@ -1,9 +1,7 @@
 // config tests
 import { updateConfig, configReducer, getConfig } from '..'
 
-import * as mockShell from '../../shell'
-
-jest.mock('../../shell', () => ({ getShellConfig: jest.fn() }))
+jest.mock('../../shell/remote', () => ({ INITIAL_CONFIG: { isConfig: true } }))
 
 describe('config', () => {
   let state
@@ -36,8 +34,6 @@ describe('config', () => {
     })
 
     test('gets store and overrides from remote for initial state', () => {
-      mockShell.getShellConfig.mockReturnValue({ isConfig: true })
-
       expect(configReducer(null, {})).toEqual({ isConfig: true })
     })
 

diff --git a/app/src/config/index.js b/app/src/config/index.js
@@ -3,104 +3,12 @@
 import { setIn } from '@thi.ng/paths'
 import remove from 'lodash/remove'
 
-import { getShellConfig } from '../shell'
+import remote from '../shell/remote'
 
 import type { State, Action, ThunkAction } from '../types'
-import type { LogLevel } from '../logger'
+import type { Config, UpdateConfigAction } from './types'
 
-type UrlProtocol = 'file:' | 'http:'
-
-export type UpdateChannel = 'latest' | 'beta' | 'alpha'
-
-export type DiscoveryCandidates = string | Array<string>
-
-// TODO(mc, 2018-05-17): put this type somewhere common to app and app-shell
-export type Config = {
-  devtools: boolean,
-
-  // app update config
-  update: {
-    channel: UpdateChannel,
-  },
-
-  // robot update config
-  buildroot: {
-    manifestUrl: string,
-  },
-
-  // logging config
-  log: {
-    level: {
-      file: LogLevel,
-      console: LogLevel,
-    },
-  },
-
-  // ui and browser config
-  ui: {
-    width: number,
-    height: number,
-    url: {
-      protocol: UrlProtocol,
-      path: string,
-    },
-    webPreferences: {
-      webSecurity: boolean,
-    },
-  },
-
-  analytics: {
-    appId: string,
-    optedIn: boolean,
-    seenOptIn: boolean,
-  },
-
-  // deprecated; remove with first migration
-  p10WarningSeen: {
-    [id: string]: ?boolean,
-  },
-
-  support: {
-    userId: string,
-    createdAt: number,
-    name: string,
-    email: ?string,
-  },
-
-  discovery: {
-    candidates: DiscoveryCandidates,
-  },
-
-  // internal development flags
-  devInternal?: {
-    allPipetteConfig?: boolean,
-    tempdeckControls?: boolean,
-    enableThermocycler?: boolean,
-    enablePipettePlus?: boolean,
-    enableBuildRoot?: boolean,
-  },
-}
-
-type UpdateConfigAction = {|
-  type: 'config:UPDATE',
-  payload: {|
-    path: string,
-    value: any,
-  |},
-  meta: {|
-    shell: true,
-  |},
-|}
-
-type SetConfigAction = {|
-  type: 'config:SET',
-  payload: {|
-    path: string,
-    value: any,
-  |},
-|}
-
-export type ConfigAction = UpdateConfigAction | SetConfigAction
+export * from './types'
 
 // trigger a config value update to the app-shell via shell middleware
 export function updateConfig(path: string, value: any): UpdateConfigAction {
@@ -113,9 +21,8 @@ export function updateConfig(path: string, value: any): UpdateConfigAction {
 
 // config reducer
 export function configReducer(state: ?Config, action: Action): Config {
-  // initial state
-  // getShellConfig makes a sync RPC call, so use sparingly
-  if (!state) return getShellConfig()
+  // initial state from app-shell preloaded remote
+  if (!state) return remote.INITIAL_CONFIG
 
   switch (action.type) {
     case 'config:SET':
@@ -142,16 +49,17 @@ export function addManualIp(ip: string): ThunkAction {
     const previous: ?string = [].concat(candidates).find(i => i === ip)
     let nextCandidatesList = candidates
     if (!previous) nextCandidatesList = nextCandidatesList.concat(ip)
+
     return dispatch(updateConfig('discovery.candidates', nextCandidatesList))
   }
 }
 
 export function removeManualIp(ip: string): ThunkAction {
   return (dispatch, getState) => {
     const candidates = [].concat(getConfig(getState()).discovery.candidates)
-    remove(candidates, c => {
-      return c === ip
-    })
+
+    remove(candidates, c => c === ip)
+
     return dispatch(updateConfig('discovery.candidates', candidates))
   }
 }
diff --git a/app/src/config/types.js b/app/src/config/types.js
@@ -0,0 +1,95 @@
+// @flow
+import type { LogLevel } from '../logger'
+
+export type UrlProtocol = 'file:' | 'http:'
+
+export type UpdateChannel = 'latest' | 'beta' | 'alpha'
+
+export type DiscoveryCandidates = string | Array<string>
+
+export type Config = {
+  devtools: boolean,
+
+  // app update config
+  update: {
+    channel: UpdateChannel,
+  },
+
+  // robot update config
+  buildroot: {
+    manifestUrl: string,
+  },
+
+  // logging config
+  log: {
+    level: {
+      file: LogLevel,
+      console: LogLevel,
+    },
+  },
+
+  // ui and browser config
+  ui: {
+    width: number,
+    height: number,
+    url: {
+      protocol: UrlProtocol,
+      path: string,
+    },
+    webPreferences: {
+      webSecurity: boolean,
+    },
+  },
+
+  analytics: {
+    appId: string,
+    optedIn: boolean,
+    seenOptIn: boolean,
+  },
+
+  // deprecated; remove with first migration
+  p10WarningSeen: {
+    [id: string]: ?boolean,
+  },
+
+  support: {
+    userId: string,
+    createdAt: number,
+    name: string,
+    email: ?string,
+  },
+
+  discovery: {
+    candidates: DiscoveryCandidates,
+  },
+
+  // internal development flags
+  devInternal?: {
+    allPipetteConfig?: boolean,
+    tempdeckControls?: boolean,
+    enableThermocycler?: boolean,
+    enablePipettePlus?: boolean,
+    enableBuildRoot?: boolean,
+  },
+}
+
+export type UpdateConfigAction = {|
+  type: 'config:UPDATE',
+  payload: {|
+    path: string,
+    value: any,
+  |},
+  meta: {|
+    shell: true,
+  |},
+|}
+
+export type SetConfigAction = {|
+  type: 'config:SET',
+  payload: {|
+    path: string,
+    value: any,
+  |},
+|}
+
+export type ConfigAction = UpdateConfigAction | SetConfigAction
diff --git a/app/src/discovery/__tests__/reducer.test.js b/app/src/discovery/__tests__/reducer.test.js
@@ -1,8 +1,8 @@
 // discovery reducer test
 import { discoveryReducer } from '..'
 
-jest.mock('../../shell', () => ({
-  getShellRobots: () => [
+jest.mock('../../shell/remote', () => ({
+  INITIAL_ROBOTS: [
     { name: 'foo', ip: '192.168.1.1', port: 31950 },
     { name: 'bar', ip: '192.168.1.2', port: 31950 },
   ],

diff --git a/app/src/discovery/index.js b/app/src/discovery/index.js
@@ -4,7 +4,7 @@ import groupBy from 'lodash/groupBy'
 import mapValues from 'lodash/mapValues'
 import some from 'lodash/some'
 
-import { getShellRobots } from '../shell'
+import remote from '../shell/remote'
 import * as actions from './actions'
 
 import type { Service } from '@opentrons/discovery-client'
@@ -29,15 +29,14 @@ export type DiscoveryState = {|
 export const RESTART_PENDING: RestartStatus = 'pending'
 export const RESTART_DOWN: RestartStatus = 'down'
 
-// getShellRobots makes a sync RPC call, so use sparingly
-const initialState: DiscoveryState = {
+const INITIAL_STATE: DiscoveryState = {
   scanning: false,
-  robotsByName: normalizeRobots(getShellRobots()),
   restartsByName: {},
+  robotsByName: normalizeRobots(remote.INITIAL_ROBOTS),
 }
 
 export function discoveryReducer(
-  state: DiscoveryState = initialState,
+  state: DiscoveryState = INITIAL_STATE,
   action: Action
 ): DiscoveryState {
   switch (action.type) {

diff --git a/app/src/epic.js b/app/src/epic.js
@@ -2,8 +2,8 @@
 // root application epic
 import { combineEpics } from 'redux-observable'
 
-import { buildrootUpdateEpic } from './shell'
 import { discoveryEpic } from './discovery'
 import { robotApiEpic } from './robot-api'
+import { shellEpic } from './shell'
 
-export default combineEpics(buildrootUpdateEpic, discoveryEpic, robotApiEpic)
+export default combineEpics(discoveryEpic, robotApiEpic, shellEpic)
diff --git a/app/src/index.js b/app/src/index.js
@@ -10,7 +10,7 @@ import { ConnectedRouter, routerMiddleware } from 'connected-react-router'
 import { createEpicMiddleware } from 'redux-observable'
 
 import createLogger from './logger'
-import { checkShellUpdate, shellMiddleware } from './shell'
+import { checkShellUpdate } from './shell'
 
 import { apiClientMiddleware as robotApiMiddleware } from './robot'
 import { initializeAnalytics, analyticsMiddleware } from './analytics'
@@ -31,7 +31,6 @@ const middleware = applyMiddleware(
   thunk,
   epicMiddleware,
   robotApiMiddleware(),
-  shellMiddleware,
   analyticsMiddleware,
   supportMiddleware,
   routerMiddleware(history)

diff --git a/app/src/shell/__mocks__/remote.js b/app/src/shell/__mocks__/remote.js
@@ -2,23 +2,20 @@
 // keep in sync with app-shell/src/preload.js
 'use strict'
 
+const EventEmitter = require('events')
+
+class MockIpcRenderer extends EventEmitter {
+  send = jest.fn()
+}
+
 module.exports = {
-  ipcRenderer: {
-    on: jest.fn(),
-    send: jest.fn(),
-  },
+  ipcRenderer: new MockIpcRenderer(),
   apiUpdate: {
     getUpdateInfo: jest.fn(),
     getUpdateFileContents: jest.fn(),
   },
-  config: {
-    getConfig: jest.fn(),
-  },
-  discovery: {
-    getRobots: jest.fn(),
-  },
-  update: {
-    CURRENT_VERSION: '0.0.0',
-    CURRENT_RELEASE_NOTES: 'Release notes for 0.0.0',
-  },
+  CURRENT_VERSION: '0.0.0',
+  CURRENT_RELEASE_NOTES: 'Release notes for 0.0.0',
+  INITIAL_CONFIG: {},
+  INITIAL_ROBOTS: [],
 }