From 87f70ecdab5fe86208ecd1be99c523ab5b0cb2e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 00:17:52 +0000 Subject: [PATCH 1/3] Initial plan From 159df853af749e7d458a27ecf763c197106d4231 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 00:21:56 +0000 Subject: [PATCH 2/3] fix: prevent code injection via configvalidators/overridevalidators from user-controlled config - Remove new Function() calls that processed configvalidators/overridevalidators from user-supplied config in Settings constructor (lib/settings.js) - Update validate() method to use DeploymentConfig static validators only - Strip configvalidators/overridevalidators from runtimeConfig before merging in index.js (defense in depth) to prevent admin repo from overriding deployment-only settings - Add security tests verifying config-derived validators are ignored --- index.js | 17 ++++++++-- lib/settings.js | 26 +++------------ test/unit/lib/settings.test.js | 60 ++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 25 deletions(-) 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..541a68f1 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -462,4 +462,64 @@ repository: ); }); }); + + describe('configvalidators and overridevalidators security', () => { + const DeploymentConfig = require('../../../lib/deploymentConfig') + + 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) + // Restore + DeploymentConfig.configvalidators = {} + }) + + 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') + // Restore + DeploymentConfig.configvalidators = {} + }) + + 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') + // Restore + DeploymentConfig.overridevalidators = {} + }) + }) }) // Settings Tests From be7135fa79f5f9a0ac6e5fcd14db2756a4f44bbe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 12 May 2026 00:23:30 +0000 Subject: [PATCH 3/3] refactor: improve test quality for security tests - Move DeploymentConfig import to top of file - Use afterEach hooks for cleanup instead of manual restore --- test/unit/lib/settings.test.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 541a68f1..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') @@ -464,7 +465,10 @@ repository: }); describe('configvalidators and overridevalidators security', () => { - const DeploymentConfig = require('../../../lib/deploymentConfig') + afterEach(() => { + DeploymentConfig.configvalidators = {} + DeploymentConfig.overridevalidators = {} + }) it('should not process configvalidators from user-supplied config', () => { const maliciousConfig = { @@ -498,8 +502,6 @@ repository: const settings = createSettings({ restrictedRepos: [] }) settings.validate('repository', {}, { name: 'test' }) expect(mockValidator).toHaveBeenCalledTimes(1) - // Restore - DeploymentConfig.configvalidators = {} }) it('validate() throws when DeploymentConfig configvalidator returns false', () => { @@ -508,8 +510,6 @@ repository: } const settings = createSettings({ restrictedRepos: [] }) expect(() => settings.validate('repository', {}, {})).toThrow('validation failed') - // Restore - DeploymentConfig.configvalidators = {} }) it('validate() throws when DeploymentConfig overridevalidator returns false', () => { @@ -518,8 +518,6 @@ repository: } const settings = createSettings({ restrictedRepos: [] }) expect(() => settings.validate('repository', {}, {})).toThrow('override denied') - // Restore - DeploymentConfig.overridevalidators = {} }) }) }) // Settings Tests