diff --git a/index.js b/index.js index 12aae7c7..19e5b278 100644 --- a/index.js +++ b/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) { diff --git a/lib/settings.js b/lib/settings.js index 919e0185..566848dd 100644 --- a/lib/settings.js +++ b/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,7 +460,7 @@ ${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)) { @@ -486,7 +468,7 @@ ${this.results.reduce((x, y) => { 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)) { diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index b3610651..7569180b 100644 --- a/test/unit/lib/settings.test.js +++ b/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