Validate plugin event registration (#5356)

* Turn exception message into something human readable

* Pass ipc as parameter to invoke function

* Creating file to validate event name and handler

* Creating tests to validate_event

* Remove ipc from invoke parameter

* Removing ipc parameter being passed to validateEvent

* convert spec to js

* increase line-height for plugins error message

* refactor error messages and implementation

* fix race condition where async error in plugins file could hang run

a quick async error at the root of the plugins file had the potential to hang the run because the ‘exitEarlyWithErr’ listener was registered later than that error was emitted

this enables that error to be tracked so we can properly exit at the appropriate time

it also refactors run.js to not rely on an event emitted on the project and instead passes through an onError handler, which makes more sense since the event was only used in run.js (except for one case). it also makes for easier unit testing

* fix missing reference

* fix duplicate reference

* fix args being passed in incorrectly

* fix way args were handled in server.open

* fix exit early implementation

* fix duplicate logging

* fix unit test

* update snapshot

* fix missing reference

* add e2e test to cover plugin registration validation

* clean up after merge

* add back snapshot

* fix e2e tests

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <github@chary.us>
Co-authored-by: Jennifer Shehane <shehane.jennifer@gmail.com>

packages/desktop-gui/src/styles/components/_alerts.scss

@@ -56,6 +56,10 @@
 
   .alert-content {
     text-align: left;
+
+    p {
+      line-height: 1.5;
+    }
   }
 
   code, pre {

packages/server/__snapshots__/3_plugins_spec.js

@@ -461,3 +461,21 @@ The following error was thrown by a plugin. We stopped running your tests becaus
 
 
 `
+
+exports['e2e plugins projectRoot and configFile passes projectRoot and default configFile to plugins function 1'] = `
+The following validation error was thrown by your plugins file (\`/foo/bar/.projects/plugin-validation-error/cypress/plugins/index.js\`).
+
+ Error: You must pass a valid event name when registering a plugin.
+
+You passed: \`invalid:event\`
+
+The following are valid events:
+- file:preprocessor
+- before:browser:launch
+- task
+- after:screenshot
+
+    [stack trace lines]
+
+
+`

packages/server/lib/errors.coffee

@@ -592,6 +592,11 @@ getMsgByType = (type, arg1 = {}, arg2, arg3) ->
       The following error was thrown by a plugin. We stopped running your tests because a plugin crashed. Please check your plugins file (`#{arg1}`)
       """.trim()
       return {msg: msg, details: arg2}
+    when "PLUGINS_VALIDATION_ERROR"
+      msg = """
+      The following validation error was thrown by your plugins file (`#{arg1}`).
+      """.trim()
+      return {msg: msg, details: arg2}
     when "BUNDLE_ERROR"
       ## IF YOU MODIFY THIS MAKE SURE TO UPDATE
       ## THE ERROR MESSAGE IN THE RUNNER TOO

packages/server/lib/plugins/child/run_plugins.js

@@ -4,10 +4,12 @@
 const _ = require('lodash')
 const debug = require('debug')('cypress:server:plugins:child')
 const Promise = require('bluebird')
+
+const errors = require('../../errors')
 const preprocessor = require('./preprocessor')
 const task = require('./task')
 const util = require('../util')
-const errors = require('../../errors')
+const validateEvent = require('./validate_event')
 
 const ARRAY_METHODS = ['concat', 'push', 'unshift', 'slice', 'pop', 'shift', 'slice', 'splice', 'filter', 'map', 'forEach', 'reduce', 'reverse', 'splice', 'includes']
 
@@ -16,23 +18,9 @@ const registeredEvents = {}
 const invoke = (eventId, args = []) => {
   const event = registeredEvents[eventId]
 
-  if (!event) {
-    sendError(new Error(`No handler registered for event id ${eventId}`))
-
-    return
-  }
-
   return event.handler(...args)
 }
 
-const sendError = (ipc, err) => {
-  ipc.send('error', util.serializeError(err))
-}
-
-const sendWarning = (ipc, warningErr) => {
-  ipc.send('warning', util.serializeError(warningErr))
-}
-
 let plugins
 
 const load = (ipc, config, pluginsFile) => {
@@ -44,6 +32,14 @@ const load = (ipc, config, pluginsFile) => {
   // we track the register calls and then send them all at once
   // to the parent process
   const register = (event, handler) => {
+    const { isValid, error } = validateEvent(event, handler)
+
+    if (!isValid) {
+      ipc.send('load:error', 'PLUGINS_VALIDATION_ERROR', pluginsFile, error.stack)
+
+      return
+    }
+
     if (event === 'task') {
       const existingEventId = _.findKey(registeredEvents, { event: 'task' })
 
@@ -74,12 +70,16 @@ const load = (ipc, config, pluginsFile) => {
 
   Promise
   .try(() => {
+    debug('run plugins function')
+
     return plugins(register, config)
   })
   .then((modifiedCfg) => {
+    debug('plugins file successfully loaded')
     ipc.send('loaded', modifiedCfg, registrations)
   })
   .catch((err) => {
+    debug('plugins file errored:', err && err.stack)
     ipc.send('load:error', 'PLUGINS_FUNCTION_ERROR', pluginsFile, err.stack)
   })
 }
@@ -113,10 +113,9 @@ const execute = (ipc, event, ids, args = []) => {
 
           hasEmittedWarning = true
 
-          sendWarning(ipc,
-            errors.get(
-              'DEPRECATED_BEFORE_BROWSER_LAUNCH_ARGS',
-            ))
+          const warning = errors.get('DEPRECATED_BEFORE_BROWSER_LAUNCH_ARGS')
+
+          ipc.send('warning', util.serializeError(warning))
 
           // eslint-disable-next-line prefer-rest-params
           return boundFn.apply(this, arguments)

packages/server/lib/plugins/child/validate_event.js

@@ -0,0 +1,45 @@
+const _ = require('lodash')
+
+const createErrorResult = (errorMessage) => ({ isValid: false, error: new Error(errorMessage) })
+const createSuccessResult = () => ({ isValid: true })
+
+const validate = (func, arg, errorMessage) => {
+  return func(arg) ? createSuccessResult() : createErrorResult(errorMessage)
+}
+
+const isFunction = (event, handler) => {
+  return validate(_.isFunction, handler, `The handler for the event \`${event}\` must be a function`)
+}
+
+const isObject = (event, handler) => {
+  return validate(_.isPlainObject, handler, `The handler for the event \`${event}\` must be an object`)
+}
+
+const eventValidators = {
+  'file:preprocessor': isFunction,
+  'before:browser:launch': isFunction,
+  'task': isObject,
+  'after:screenshot': isFunction,
+  '_get:task:keys': isFunction,
+  '_get:task:body': isFunction,
+}
+
+const validateEvent = (event, handler) => {
+  const validator = eventValidators[event]
+
+  if (!validator) {
+    const userEvents = _.reject(_.keys(eventValidators), (event) => event.startsWith('_'))
+
+    return createErrorResult(`You must pass a valid event name when registering a plugin.
+
+You passed: \`${event}\`
+
+The following are valid events:
+- ${userEvents.join('\n- ')}
+`)
+  }
+
+  return validator(event, handler)
+}
+
+module.exports = validateEvent

packages/server/lib/server.coffee

@@ -164,6 +164,7 @@ class Server
 
   open: (config = {}, project, onError, onWarning) ->
     debug("server open")
+
     la(_.isPlainObject(config), "expected plain config object", config)
 
     Promise.try =>

packages/server/test/e2e/3_plugins_spec.js

@@ -15,6 +15,7 @@ const pluginAfterScreenshot = Fixtures.projectPath('plugin-after-screenshot')
 const pluginReturnsBadConfig = Fixtures.projectPath('plugin-returns-bad-config')
 const pluginReturnsEmptyBrowsersList = Fixtures.projectPath('plugin-returns-empty-browsers-list')
 const pluginReturnsInvalidBrowser = Fixtures.projectPath('plugin-returns-invalid-browser')
+const pluginValidationError = Fixtures.projectPath('plugin-validation-error')
 
 describe('e2e plugins', function () {
   e2e.setup()
@@ -134,6 +135,16 @@ describe('e2e plugins', function () {
     })
   })
 
+  it('fails when invalid event is registered', function () {
+    e2e.exec(this, {
+      spec: 'app_spec.js',
+      project: pluginValidationError,
+      sanitizeScreenshotDimensions: true,
+      snapshot: true,
+      expectedExitCode: 1,
+    })
+  })
+
   describe('projectRoot and configFile', function () {
     it('passes projectRoot and default configFile to plugins function', function () {
       return e2e.exec(this, {

packages/server/test/support/fixtures/projects/plugin-before-browser-launch-deprecation/cypress/plugins/index.js

@@ -44,7 +44,6 @@ const getHandlersByType = (type) => {
           return launchOptions
         },
         onTask: { assertPsOutput: assertPsOutput('--foo') },
-
       }
 
     case 'return-launch-options-mutate-only-args-property':
@@ -57,7 +56,6 @@ const getHandlersByType = (type) => {
           return launchOptions
         },
         onTask: { assertPsOutput: assertPsOutput(['--foo', '--bar']) },
-
       }
 
     case 'return-undefined-mutate-array':
@@ -70,7 +68,6 @@ const getHandlersByType = (type) => {
           return
         },
         onTask: { assertPsOutput: assertPsOutput([]) },
-
       }
 
     case 'return-unknown-properties':
@@ -83,6 +80,7 @@ const getHandlersByType = (type) => {
 
           return launchOptions
         },
+        onTask: {},
       }
 
     case 'throw-explicit-error':
@@ -90,6 +88,7 @@ const getHandlersByType = (type) => {
         onBeforeBrowserLaunch (browser, launchOptions) {
           throw new Error('Error thrown from plugins handler')
         },
+        onTask: {},
       }
 
     case 'reject-promise':
@@ -101,6 +100,7 @@ const getHandlersByType = (type) => {
             throw new Error('Promise rejected from plugins handler')
           })
         },
+        onTask: {},
       }
 
     default: () => {

packages/server/test/support/fixtures/projects/plugin-validation-error/cypress.json

@@ -0,0 +1 @@
+{}

packages/server/test/support/fixtures/projects/plugin-validation-error/cypress/integration/app_spec.js

@@ -0,0 +1,3 @@
+it('passes', () => {
+  expect(true).to.be.true
+})

packages/server/test/support/fixtures/projects/plugin-validation-error/cypress/plugins/index.js

@@ -0,0 +1,3 @@
+module.exports = (on) => {
+  on('invalid:event', () => {})
+}

packages/server/test/unit/plugins/child/validate_event_spec.js

@@ -0,0 +1,69 @@
+require('../../../spec_helper')
+
+const _ = require('lodash')
+const validateEvent = require('../../../../lib/plugins/child/validate_event')
+
+const events = [
+  ['file:preprocessor', 'a function', () => {}],
+  ['before:browser:launch', 'a function', () => {}],
+  ['after:screenshot', 'a function', () => {}],
+  ['task', 'an object', {}],
+]
+
+describe('lib/plugins/child/validate_event', () => {
+  it('returns error when called with no event name', () => {
+    const { isValid, error } = validateEvent()
+
+    expect(isValid).to.be.false
+    expect(error.message).to.equal(`You must pass a valid event name when registering a plugin.
+
+You passed: \`undefined\`
+
+The following are valid events:
+- file:preprocessor
+- before:browser:launch
+- task
+- after:screenshot
+`)
+  })
+
+  it('returns error when called with no event handler', () => {
+    const { isValid, error } = validateEvent('file:preprocessor')
+
+    expect(isValid).to.be.false
+    expect(error.message).to.equal('The handler for the event `file:preprocessor` must be a function')
+  })
+
+  it('returns error when called with unsupported event name', () => {
+    const { isValid, error } = validateEvent('invalid:event:name', {})
+
+    expect(isValid).to.be.false
+    expect(error.message).to.equal(`You must pass a valid event name when registering a plugin.
+
+You passed: \`invalid:event:name\`
+
+The following are valid events:
+- file:preprocessor
+- before:browser:launch
+- task
+- after:screenshot
+`)
+  })
+
+  _.each(events, ([event, type]) => {
+    it(`returns error when event handler of ${event} is not ${type}`, () => {
+      const { isValid, error } = validateEvent(event, 'invalid type')
+
+      expect(isValid).to.be.false
+      expect(error.message).to.equal(`The handler for the event \`${event}\` must be ${type}`)
+    })
+  })
+
+  _.each(events, ([event, type, validValue]) => {
+    it(`returns success when event handler of ${event} is ${type}`, () => {
+      const { isValid } = validateEvent(event, validValue)
+
+      expect(isValid).to.be.true
+    })
+  })
+})