Fix code injection via configvalidators/overridevalidators in user-controlled config

index.js

@@ -19,13 +19,24 @@ let deploymentConfig
 
 module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => {
   let appSlug = 'safe-settings'
+
+  function stripDeploymentOnlyProperties (config) {
+    if (config && typeof config === 'object') {
+      const sanitized = Object.assign({}, config)
+      delete sanitized.configvalidators
+      delete sanitized.overridevalidators
+      return sanitized
+    }
+    return config
+  }
+
   async function syncAllSettings (nop, context, repo = context.repo(), ref) {
     try {
       deploymentConfig = await loadYamlFileSystem()
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = Object.assign({}, deploymentConfig, stripDeploymentOnlyProperties(runtimeConfig))
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       if (ref) {
         return Settings.syncAll(nop, context, repo, config, ref)
@@ -54,7 +65,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = Object.assign({}, deploymentConfig, stripDeploymentOnlyProperties(runtimeConfig))
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.sync(nop, context, repo, config, ref)
     } catch (e) {
@@ -79,7 +90,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = Object.assign({}, deploymentConfig, stripDeploymentOnlyProperties(runtimeConfig))
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref)
     } catch (e) {

lib/settings.js

@@ -5,6 +5,7 @@ const errorTemplate = require('./error')
 const Glob = require('./glob')
 const NopCommand = require('./nopcommand')
 const MergeDeep = require('./mergeDeep')
+const DeploymentConfig = require('./deploymentConfig')
 const Archive = require('./plugins/archive')
 const env = require('./env')
 const CONFIG_PATH = env.CONFIG_PATH
@@ -107,26 +108,7 @@ class Settings {
     this.log = context.log
     this.results = []
     this.errors = []
-    this.configvalidators = {}
-    this.overridevalidators = {}
-    const overridevalidators = config.overridevalidators
-    if (this.isIterable(overridevalidators)) {
-      for (const validator of overridevalidators) {
-        // eslint-disable-next-line no-new-func
-        const f = new Function('baseconfig', 'overrideconfig', 'githubContext', validator.script)
-        this.overridevalidators[validator.plugin] = { canOverride: f, error: validator.error }
-      }
-    }
-    const configvalidators = config.configvalidators
-    if (this.isIterable(configvalidators)) {
-      for (const validator of configvalidators) {
-        this.log.debug(`Logging each script: ${typeof validator.script}`)
-        // eslint-disable-next-line no-new-func
-        const f = new Function('baseconfig', 'githubContext', validator.script)
-        this.configvalidators[validator.plugin] = { isValid: f, error: validator.error }
-      }
-    }
-    this.mergeDeep = new MergeDeep(this.log, this.github, [], this.configvalidators, this.overridevalidators)
+    this.mergeDeep = new MergeDeep(this.log, this.github, [])
   }
 
   // Create a check in the Admin repo for safe-settings.
@@ -478,15 +460,15 @@ ${this.results.reduce((x, y) => {
   }
 
   validate (section, baseConfig, overrideConfig) {
-    const configValidator = this.configvalidators[section]
+    const configValidator = DeploymentConfig.configvalidators[section]
     if (configValidator) {
       this.log.debug(`Calling configvalidator for key ${section} `)
       if (!configValidator.isValid(overrideConfig, this.github)) {
         this.log.error(`Error in calling configvalidator for key ${section} ${configValidator.error}`)
         throw new Error(configValidator.error)
       }
     }
-    const overridevalidator = this.overridevalidators[section]
+    const overridevalidator = DeploymentConfig.overridevalidators[section]
     if (overridevalidator) {
       this.log.debug(`Calling overridevalidator for key ${section} `)
       if (!overridevalidator.canOverride(baseConfig, overrideConfig, this.github)) {

test/unit/lib/settings.test.js

@@ -1,6 +1,7 @@
 /* eslint-disable no-undef */
 class Octokit {}
 const Settings = require('../../../lib/settings')
+const DeploymentConfig = require('../../../lib/deploymentConfig')
 const yaml = require('js-yaml')
 // jest.mock('../../../lib/settings', () => {
 //   const OriginalSettings = jest.requireActual('../../../lib/settings')
@@ -462,4 +463,61 @@ repository:
       );
     });
   });
+
+  describe('configvalidators and overridevalidators security', () => {
+    afterEach(() => {
+      DeploymentConfig.configvalidators = {}
+      DeploymentConfig.overridevalidators = {}
+    })
+
+    it('should not process configvalidators from user-supplied config', () => {
+      const maliciousConfig = {
+        restrictedRepos: [],
+        configvalidators: [
+          { plugin: 'repository', script: 'return process.env', error: 'bad' }
+        ]
+      }
+      // Should not throw and should not build instance-level configvalidators
+      const settings = createSettings(maliciousConfig)
+      expect(settings.configvalidators).toBeUndefined()
+    })
+
+    it('should not process overridevalidators from user-supplied config', () => {
+      const maliciousConfig = {
+        restrictedRepos: [],
+        overridevalidators: [
+          { plugin: 'repository', script: 'return process.env', error: 'bad' }
+        ]
+      }
+      // Should not throw and should not build instance-level overridevalidators
+      const settings = createSettings(maliciousConfig)
+      expect(settings.overridevalidators).toBeUndefined()
+    })
+
+    it('validate() uses DeploymentConfig validators, not config-derived validators', () => {
+      const mockValidator = jest.fn().mockReturnValue(true)
+      DeploymentConfig.configvalidators = {
+        repository: { isValid: mockValidator, error: 'test error' }
+      }
+      const settings = createSettings({ restrictedRepos: [] })
+      settings.validate('repository', {}, { name: 'test' })
+      expect(mockValidator).toHaveBeenCalledTimes(1)
+    })
+
+    it('validate() throws when DeploymentConfig configvalidator returns false', () => {
+      DeploymentConfig.configvalidators = {
+        repository: { isValid: () => false, error: 'validation failed' }
+      }
+      const settings = createSettings({ restrictedRepos: [] })
+      expect(() => settings.validate('repository', {}, {})).toThrow('validation failed')
+    })
+
+    it('validate() throws when DeploymentConfig overridevalidator returns false', () => {
+      DeploymentConfig.overridevalidators = {
+        repository: { canOverride: () => false, error: 'override denied' }
+      }
+      const settings = createSettings({ restrictedRepos: [] })
+      expect(() => settings.validate('repository', {}, {})).toThrow('override denied')
+    })
+  })
 }) // Settings Tests