From 6b41552bbdaf77a9591759c5c622886d5699774d Mon Sep 17 00:00:00 2001
From: RahulGautamSingh <rahultesnik@gmail.com>
Date: Tue, 16 Jan 2024 16:27:26 +0545
Subject: [PATCH] feat(config): warn if `globalOnly` option found in repo
config (#24561)
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
---
lib/config-validator.ts | 17 +-
.../__snapshots__/validation.spec.ts.snap | 8 +-
lib/config/migrate-validate.ts | 2 +-
lib/config/presets/internal/index.spec.ts | 2 +-
lib/config/validation.spec.ts | 169 +++++++++++++-----
lib/config/validation.ts | 42 +++++
lib/workers/repository/reconfigure/index.ts | 2 +-
7 files changed, 188 insertions(+), 54 deletions(-)
diff --git a/lib/config-validator.ts b/lib/config-validator.ts
index 2436b8a8ab..2a8aff79a3 100644
--- a/lib/config-validator.ts
+++ b/lib/config-validator.ts
@@ -17,6 +17,7 @@ import {
let returnVal = 0;
async function validate(
+ isGlobalConfig: boolean,
desc: string,
config: RenovateConfig,
strict: boolean,
@@ -36,7 +37,7 @@ async function validate(
}
}
const massagedConfig = massageConfig(migratedConfig);
- const res = await validateConfig(massagedConfig, isPreset);
+ const res = await validateConfig(isGlobalConfig, massagedConfig, isPreset);
if (res.errors.length) {
logger.error(
{ file: desc, errors: res.errors },
@@ -75,7 +76,7 @@ type PackageJson = {
const parsedContent = await getParsedContent(file);
try {
logger.info(`Validating ${file}`);
- await validate(file, parsedContent, strict);
+ await validate(true, file, parsedContent, strict);
} catch (err) {
logger.warn({ file, err }, 'File is not valid Renovate config');
returnVal = 1;
@@ -96,7 +97,7 @@ type PackageJson = {
const parsedContent = await getParsedContent(file);
try {
logger.info(`Validating ${file}`);
- await validate(file, parsedContent, strict);
+ await validate(false, file, parsedContent, strict);
} catch (err) {
logger.warn({ file, err }, 'File is not valid Renovate config');
returnVal = 1;
@@ -112,12 +113,18 @@ type PackageJson = {
) as PackageJson;
if (pkgJson.renovate) {
logger.info(`Validating package.json > renovate`);
- await validate('package.json > renovate', pkgJson.renovate, strict);
+ await validate(
+ false,
+ 'package.json > renovate',
+ pkgJson.renovate,
+ strict,
+ );
}
if (pkgJson['renovate-config']) {
logger.info(`Validating package.json > renovate-config`);
for (const presetConfig of Object.values(pkgJson['renovate-config'])) {
await validate(
+ false,
'package.json > renovate-config',
presetConfig,
strict,
@@ -134,7 +141,7 @@ type PackageJson = {
const file = process.env.RENOVATE_CONFIG_FILE ?? 'config.js';
logger.info(`Validating ${file}`);
try {
- await validate(file, fileConfig, strict);
+ await validate(true, file, fileConfig, strict);
} catch (err) {
logger.error({ file, err }, 'File is not valid Renovate config');
returnVal = 1;
diff --git a/lib/config/__snapshots__/validation.spec.ts.snap b/lib/config/__snapshots__/validation.spec.ts.snap
index c102eace3c..14aef329c7 100644
--- a/lib/config/__snapshots__/validation.spec.ts.snap
+++ b/lib/config/__snapshots__/validation.spec.ts.snap
@@ -37,6 +37,10 @@ exports[`config/validation validateConfig(config) catches invalid templates 1`]
exports[`config/validation validateConfig(config) errors for all types 1`] = `
[
+ {
+ "message": "Configuration option \`azureWorkItemId\` should be an integer. Found: false (boolean)",
+ "topic": "Configuration Error",
+ },
{
"message": "Configuration option \`enabled\` should be boolean. Found: 1 (number)",
"topic": "Configuration Error",
@@ -57,10 +61,6 @@ exports[`config/validation validateConfig(config) errors for all types 1`] = `
"message": "Configuration option \`packageRules[3].matchPackagePatterns\` should be a list (Array)",
"topic": "Configuration Error",
},
- {
- "message": "Configuration option \`prCommitsPerRunLimit\` should be an integer. Found: false (boolean)",
- "topic": "Configuration Error",
- },
{
"message": "Configuration option \`semanticCommitType\` should be a string",
"topic": "Configuration Error",
diff --git a/lib/config/migrate-validate.ts b/lib/config/migrate-validate.ts
index 81d4649985..a3d8f8b42c 100644
--- a/lib/config/migrate-validate.ts
+++ b/lib/config/migrate-validate.ts
@@ -32,7 +32,7 @@ export async function migrateAndValidate(
}: {
warnings: ValidationMessage[];
errors: ValidationMessage[];
- } = await configValidation.validateConfig(massagedConfig);
+ } = await configValidation.validateConfig(false, massagedConfig);
// istanbul ignore if
if (is.nonEmptyArray(warnings)) {
logger.warn({ warnings }, 'Found renovate config warnings');
diff --git a/lib/config/presets/internal/index.spec.ts b/lib/config/presets/internal/index.spec.ts
index 88c27487f7..fd439b8175 100644
--- a/lib/config/presets/internal/index.spec.ts
+++ b/lib/config/presets/internal/index.spec.ts
@@ -30,7 +30,7 @@ describe('config/presets/internal/index', () => {
const config = await resolveConfigPresets(
massageConfig(presetConfig),
);
- const res = await validateConfig(config, true);
+ const res = await validateConfig(false, config, true);
expect(res.errors).toHaveLength(0);
expect(res.warnings).toHaveLength(0);
} catch (err) {
diff --git a/lib/config/validation.spec.ts b/lib/config/validation.spec.ts
index 744d0e46cb..4290a28a14 100644
--- a/lib/config/validation.spec.ts
+++ b/lib/config/validation.spec.ts
@@ -23,16 +23,48 @@ describe('config/validation', () => {
const config = {
prTitle: 'something',
};
- const { warnings } = await configValidation.validateConfig(config);
+ const { warnings } = await configValidation.validateConfig(false, config);
expect(warnings).toHaveLength(1);
expect(warnings).toMatchSnapshot();
});
+ it('catches global options in repo config', async () => {
+ const config = {
+ binarySource: 'something',
+ username: 'user',
+ };
+ const { warnings } = await configValidation.validateConfig(false, config);
+ expect(warnings).toHaveLength(2);
+ expect(warnings).toMatchObject([
+ {
+ message: `The "binarySource" option is a global option reserved only for bot's global configuration and cannot be configured within repository config file`,
+ },
+ {
+ message: `The "username" option is a global option reserved only for bot's global configuration and cannot be configured within repository config file`,
+ },
+ ]);
+ });
+
+ // false globals are the options which have names same to the another globalOnly option
+ it('does warn for false globals in repo config', async () => {
+ const config = {
+ hostRules: [
+ {
+ username: 'user',
+ token: 'token',
+ password: 'pass',
+ },
+ ],
+ };
+ const { warnings } = await configValidation.validateConfig(false, config);
+ expect(warnings).toHaveLength(0);
+ });
+
it('catches invalid templates', async () => {
const config = {
commitMessage: '{{{something}}',
};
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toHaveLength(1);
expect(errors).toMatchSnapshot();
});
@@ -58,7 +90,7 @@ describe('config/validation', () => {
},
],
};
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toHaveLength(2);
expect(errors).toMatchSnapshot();
});
@@ -83,7 +115,7 @@ describe('config/validation', () => {
},
],
};
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toHaveLength(2);
});
@@ -112,7 +144,7 @@ describe('config/validation', () => {
},
],
};
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toHaveLength(2);
expect(errors).toMatchSnapshot();
});
@@ -127,7 +159,7 @@ describe('config/validation', () => {
},
},
} as any;
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toMatchObject([
{
message:
@@ -154,7 +186,7 @@ describe('config/validation', () => {
},
};
// @ts-expect-error invalid options
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toMatchObject([
{
message:
@@ -174,7 +206,7 @@ describe('config/validation', () => {
randomKey: '',
},
} as any;
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toMatchObject([
{
message:
@@ -187,7 +219,7 @@ describe('config/validation', () => {
const config = {
baseBranches: ['/***$}{]][/'],
};
- const { errors } = await configValidation.validateConfig(config);
+ const { errors } = await configValidation.validateConfig(false, config);
expect(errors).toEqual([
{
topic: 'Configuration Error',
@@ -213,8 +245,10 @@ describe('config/validation', () => {
},
major: null,
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(3);
expect(errors).toMatchSnapshot();
@@ -229,8 +263,10 @@ describe('config/validation', () => {
},
],
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(1);
expect(errors[0].message).toContain('ansible');
@@ -246,6 +282,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
);
expect(warnings).toHaveLength(0);
@@ -264,8 +301,10 @@ describe('config/validation', () => {
},
],
])('validates enabled managers for %s', async (_case, config) => {
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(0);
});
@@ -280,8 +319,10 @@ describe('config/validation', () => {
])(
'errors if included not supported enabled managers for %s',
async (_case, config) => {
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(1);
expect(errors).toMatchSnapshot();
@@ -296,7 +337,7 @@ describe('config/validation', () => {
schedule: ['every 15 mins every weekday'],
timezone: 'Asia',
labels: 5 as any,
- prCommitsPerRunLimit: false as any,
+ azureWorkItemId: false as any,
semanticCommitType: 7 as any,
lockFileMaintenance: false as any,
extends: [':timezone(Europe/Brussel)'],
@@ -319,8 +360,10 @@ describe('config/validation', () => {
],
major: null,
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(1);
expect(errors).toMatchSnapshot();
expect(errors).toHaveLength(15);
@@ -346,8 +389,10 @@ describe('config/validation', () => {
},
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(4);
expect(errors).toMatchSnapshot();
expect(errors).toHaveLength(4);
@@ -364,6 +409,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -381,8 +427,10 @@ describe('config/validation', () => {
fileMatch: ['x?+'],
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(2);
expect(errors).toMatchSnapshot();
@@ -401,6 +449,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -419,6 +468,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
true,
);
@@ -446,6 +496,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
true,
);
@@ -474,6 +525,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
true,
);
@@ -510,6 +562,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as RenovateConfig,
true,
);
@@ -540,6 +593,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
true,
);
@@ -561,6 +615,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -581,6 +636,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -604,6 +660,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -626,6 +683,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as any,
true,
);
@@ -646,6 +704,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -659,6 +718,7 @@ describe('config/validation', () => {
$schema: 'renovate.json',
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -671,6 +731,7 @@ describe('config/validation', () => {
extends: [':timezone', ':timezone(Europe/Berlin)'],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -683,6 +744,7 @@ describe('config/validation', () => {
constraints: { packageRules: [{}] },
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config as never, // TODO: #15963
true,
);
@@ -695,6 +757,7 @@ describe('config/validation', () => {
prBodyDefinitions: {},
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -709,8 +772,10 @@ describe('config/validation', () => {
example2: 'https://www.example2.com/example',
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(0);
});
@@ -723,8 +788,10 @@ describe('config/validation', () => {
} as unknown as string, // intentional incorrect config to check error message
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toMatchObject([
{
@@ -742,8 +809,10 @@ describe('config/validation', () => {
example2: 'http://www.example.com',
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toMatchObject([
{
@@ -773,8 +842,10 @@ describe('config/validation', () => {
},
],
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(errors).toHaveLength(1);
expect(warnings).toHaveLength(1);
expect(errors).toMatchSnapshot();
@@ -792,8 +863,10 @@ describe('config/validation', () => {
},
},
} as never;
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(errors).toHaveLength(1);
expect(warnings).toHaveLength(0);
expect(errors).toMatchSnapshot();
@@ -803,8 +876,10 @@ describe('config/validation', () => {
const config = {
hostType: 'npm',
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(errors).toHaveLength(0);
expect(warnings).toHaveLength(1);
expect(warnings).toMatchSnapshot();
@@ -815,6 +890,7 @@ describe('config/validation', () => {
extends: ['foo', 'bar', 42] as never,
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -829,6 +905,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -847,6 +924,7 @@ describe('config/validation', () => {
],
} as any;
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -866,6 +944,7 @@ describe('config/validation', () => {
],
};
const { warnings, errors } = await configValidation.validateConfig(
+ false,
config,
true,
);
@@ -880,8 +959,10 @@ describe('config/validation', () => {
example2: 'https://www.example2.com/example',
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ true,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toHaveLength(0);
});
@@ -893,8 +974,10 @@ describe('config/validation', () => {
example2: 123,
},
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ true,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toMatchObject([
{
@@ -909,8 +992,10 @@ describe('config/validation', () => {
const config = {
schedule: ['30 5 * * *'],
};
- const { warnings, errors } =
- await configValidation.validateConfig(config);
+ const { warnings, errors } = await configValidation.validateConfig(
+ false,
+ config,
+ );
expect(warnings).toHaveLength(0);
expect(errors).toMatchObject([
{
diff --git a/lib/config/validation.ts b/lib/config/validation.ts
index 34753754e0..acb3253ba4 100644
--- a/lib/config/validation.ts
+++ b/lib/config/validation.ts
@@ -30,6 +30,7 @@ const options = getOptions();
let optionTypes: Record<string, RenovateOptions['type']>;
let optionParents: Record<string, RenovateOptions['parent']>;
+let optionGlobals: Set<string>;
const managerList = getManagerList();
@@ -98,6 +99,7 @@ export function getParentName(parentPath: string | undefined): string {
}
export async function validateConfig(
+ isGlobalConfig: boolean,
config: RenovateConfig,
isPreset?: boolean,
parentPath?: string,
@@ -116,6 +118,7 @@ export async function validateConfig(
}
});
}
+
let errors: ValidationMessage[] = [];
let warnings: ValidationMessage[] = [];
@@ -139,6 +142,24 @@ export async function validateConfig(
message: `The "${key}" object can only be configured at the top level of a config but was found inside "${parentPath}"`,
});
}
+ if (!isGlobalConfig) {
+ if (!optionGlobals) {
+ optionGlobals = new Set<string>();
+ for (const option of options) {
+ if (option.globalOnly) {
+ optionGlobals.add(option.name);
+ }
+ }
+ }
+
+ if (optionGlobals.has(key) && !isFalseGlobal(key, parentPath)) {
+ warnings.push({
+ topic: 'Configuration Error',
+ message: `The "${key}" option is a global option reserved only for bot's global configuration and cannot be configured within repository config file`,
+ });
+ continue;
+ }
+ }
if (key === 'enabledManagers' && val) {
const unsupportedManagers = getUnsupportedEnabledManagers(
val as string[],
@@ -273,6 +294,7 @@ export async function validateConfig(
for (const [subIndex, subval] of val.entries()) {
if (is.object(subval)) {
const subValidation = await validateConfig(
+ isGlobalConfig,
subval as RenovateConfig,
isPreset,
`${currentPath}[${subIndex}]`,
@@ -656,6 +678,7 @@ export async function validateConfig(
.map((option) => option.name);
if (!ignoredObjects.includes(key)) {
const subValidation = await validateConfig(
+ isGlobalConfig,
val,
isPreset,
currentPath,
@@ -732,3 +755,22 @@ function validateRegexManagerFields(
}
}
}
+
+/** An option is a false global if it has the same name as a global only option
+ * but is actually just the field of a non global option or field an children of the non global option
+ * eg. token: it's global option used as the bot's token as well and
+ * also it can be the token used for a platform inside the hostRules configuration
+ */
+function isFalseGlobal(optionName: string, parentPath?: string): boolean {
+ if (parentPath?.includes('hostRules')) {
+ if (
+ optionName === 'token' ||
+ optionName === 'username' ||
+ optionName === 'password'
+ ) {
+ return true;
+ }
+ }
+
+ return false;
+}
diff --git a/lib/workers/repository/reconfigure/index.ts b/lib/workers/repository/reconfigure/index.ts
index 0908bbf82c..22ee0135f9 100644
--- a/lib/workers/repository/reconfigure/index.ts
+++ b/lib/workers/repository/reconfigure/index.ts
@@ -151,7 +151,7 @@ export async function validateReconfigureBranch(
}
// perform validation and provide a passing or failing check run based on result
- const validationResult = await validateConfig(configFileParsed);
+ const validationResult = await validateConfig(false, configFileParsed);
// failing check
if (validationResult.errors.length > 0) {
--
GitLab