robojerk/codeql-action

Handle errors in workflow queries correctly

Browse source
This commit is contained in:
Sam Partington 2020-08-24 16:44:59 +01:00
parent 7f19f9198a
commit 129713f1a0
6 changed files with 115 additions and 42 deletions
Download patch file Download diff file Expand all files Collapse all files

37
lib/config-utils.js generated
View file

@ -91,7 +91,7 @@ const builtinSuites = ['security-extended', 'security-and-quality'];
* Determine the set of queries associated with suiteName's suites and add them to resultMap.
* Throws an error if suiteName is not a valid builtin suite.
*/
async function addBuiltinSuiteQueries(configFile, languages, resultMap, suiteName) {
async function addBuiltinSuiteQueries(languages, resultMap, suiteName, configFile) {
const suite = builtinSuites.find((suite) => suite === suiteName);
if (!suite) {
throw new Error(getQueryUsesInvalid(configFile, suiteName));
@ -102,7 +102,7 @@ async function addBuiltinSuiteQueries(configFile, languages, resultMap, suiteNam
/**
* Retrieve the set of queries at localQueryPath and add them to resultMap.
*/
async function addLocalQueries(configFile, resultMap, localQueryPath) {
async function addLocalQueries(resultMap, localQueryPath, configFile) {
// Resolve the local path against the workspace so that when this is
// passed to codeql it resolves to exactly the path we expect it to resolve to.
const workspacePath = fs.realpathSync(util.getRequiredEnvParam('GITHUB_WORKSPACE'));
@ -124,7 +124,7 @@ async function addLocalQueries(configFile, resultMap, localQueryPath) {
/**
* Retrieve the set of queries at the referenced remote repo and add them to resultMap.
*/
async function addRemoteQueries(configFile, resultMap, queryUses) {
async function addRemoteQueries(resultMap, queryUses, configFile) {
let tok = queryUses.split('@');
if (tok.length !== 2) {
throw new Error(getQueryUsesInvalid(configFile, queryUses));
@ -157,23 +157,23 @@ async function addRemoteQueries(configFile, resultMap, queryUses) {
* local paths starting with './', or references to remote repos, or
* a finite set of hardcoded terms for builtin suites.
*/
async function parseQueryUses(configFile, languages, resultMap, queryUses) {
async function parseQueryUses(languages, resultMap, queryUses, configFile) {
queryUses = queryUses.trim();
if (queryUses === "") {
throw new Error(getQueryUsesInvalid(configFile));
}
// Check for the local path case before we start trying to parse the repository name
if (queryUses.startsWith("./")) {
await addLocalQueries(configFile, resultMap, queryUses.slice(2));
await addLocalQueries(resultMap, queryUses.slice(2), configFile);
return;
}
// Check for one of the builtin suites
if (queryUses.indexOf('/') === -1 && queryUses.indexOf('@') === -1) {
await addBuiltinSuiteQueries(configFile, languages, resultMap, queryUses);
await addBuiltinSuiteQueries(languages, resultMap, queryUses, configFile);
return;
}
// Otherwise, must be a reference to another repo
await addRemoteQueries(configFile, resultMap, queryUses);
await addRemoteQueries(resultMap, queryUses, configFile);
}
// Regex validating stars in paths or paths-ignore entries.
// The intention is to only allow ** to appear when immediately
@ -221,6 +221,8 @@ function validateAndSanitisePath(originalPath, propertyName, configFile) {
return path;
}
exports.validateAndSanitisePath = validateAndSanitisePath;
// An undefined configFile in some of these functions indicates that
// the property was in a workflow file, not a config file
function getNameInvalid(configFile) {
return getConfigFilePropertyError(configFile, NAME_PROPERTY, 'must be a non-empty string');
}
@ -278,7 +280,12 @@ function getConfigFileDirectoryGivenMessage(configFile) {
}
exports.getConfigFileDirectoryGivenMessage = getConfigFileDirectoryGivenMessage;
function getConfigFilePropertyError(configFile, property, error) {
return 'The configuration file "' + configFile + '" is invalid: property "' + property + '" ' + error;
if (configFile === undefined) {
return 'The workflow property "' + property + '" is invalid: ' + error;
}
else {
return 'The configuration file "' + configFile + '" is invalid: property "' + property + '" ' + error;
}
}
/**
* Gets the set of languages in the current repository
@ -347,12 +354,12 @@ async function getLanguages() {
* Returns true if queries were provided in the workflow file
* (and thus added), otherwise false
*/
function addQueriesFromWorkflowIfRequired(configFile, languages, resultMap) {
async function addQueriesFromWorkflowIfRequired(languages, resultMap, configFile) {
const queryUses = core.getInput('queries');
if (queryUses) {
queryUses.split(',').forEach(async (query) => {
await parseQueryUses(configFile, languages, resultMap, query);
});
for (const query of queryUses.split(',')) {
await parseQueryUses(languages, resultMap, query, configFile);
}
return true;
}
return false;
@ -364,7 +371,7 @@ async function getDefaultConfig() {
const languages = await getLanguages();
const queries = {};
await addDefaultQueries(languages, queries);
addQueriesFromWorkflowIfRequired('', languages, queries);
await addQueriesFromWorkflowIfRequired(languages, queries);
return {
languages: languages,
queries: queries,
@ -419,7 +426,7 @@ async function loadConfig(configFile) {
}
// If queries were provided using `with` in the action configuration,
// they should take precedence over the queries in the config file
const addedQueriesFromAction = addQueriesFromWorkflowIfRequired(configFile, languages, queries);
const addedQueriesFromAction = await addQueriesFromWorkflowIfRequired(languages, queries, configFile);
if (!addedQueriesFromAction && QUERIES_PROPERTY in parsedYAML) {
if (!(parsedYAML[QUERIES_PROPERTY] instanceof Array)) {
throw new Error(getQueriesInvalid(configFile));
@ -428,7 +435,7 @@ async function loadConfig(configFile) {
if (!(QUERIES_USES_PROPERTY in query) || typeof query[QUERIES_USES_PROPERTY] !== "string") {
throw new Error(getQueryUsesInvalid(configFile));
}
await parseQueryUses(configFile, languages, queries, query[QUERIES_USES_PROPERTY]);
await parseQueryUses(languages, queries, query[QUERIES_USES_PROPERTY], configFile);
}
}
if (PATHS_IGNORE_PROPERTY in parsedYAML) {

2
lib/config-utils.js.map
View file

File diff suppressed because one or more lines are too long

28
lib/config-utils.test.js generated
View file

@ -355,6 +355,34 @@ ava_1.default("Multiple queries can be specified in workflow file, no config fil
t.regex(config.queries['javascript'][2], /.*\/override2$/);
});
});
ava_1.default("Invalid queries in workflow file handled correctly", async (t) => {
return await util.withTmpDir(async (tmpDir) => {
process.env['RUNNER_TEMP'] = tmpDir;
process.env['GITHUB_WORKSPACE'] = tmpDir;
setInput('queries', 'foo/bar@v1@v3');
setInput('languages', 'javascript');
// This function just needs to be type-correct; it doesn't need to do anything,
// since we're deliberately passing in invalid data
CodeQL.setCodeQL({
resolveQueries: async function (_queries, _extraSearchPath) {
return {
byLanguage: {
'javascript': {},
},
noDeclaredLanguage: {},
multipleDeclaredLanguages: {},
};
},
});
try {
await configUtils.initConfig();
t.fail('initConfig did not throw error');
}
catch (err) {
t.deepEqual(err, new Error(configUtils.getQueryUsesInvalid(undefined, "foo/bar@v1@v3")));
}
});
});
ava_1.default("API client used when reading remote config", async (t) => {
return await util.withTmpDir(async (tmpDir) => {
process.env['RUNNER_TEMP'] = tmpDir;

2
lib/config-utils.test.js.map
View file

File diff suppressed because one or more lines are too long

31
src/config-utils.test.ts
View file

@ -412,6 +412,37 @@ test("Multiple queries can be specified in workflow file, no config file require
});
});
test("Invalid queries in workflow file handled correctly", async t => {
return await util.withTmpDir(async tmpDir => {
process.env['RUNNER_TEMP'] = tmpDir;
process.env['GITHUB_WORKSPACE'] = tmpDir;
setInput('queries', 'foo/bar@v1@v3');
setInput('languages', 'javascript');
// This function just needs to be type-correct; it doesn't need to do anything,
// since we're deliberately passing in invalid data
CodeQL.setCodeQL({
resolveQueries: async function(_queries: string[], _extraSearchPath: string | undefined) {
return {
byLanguage: {
'javascript': {},
},
noDeclaredLanguage: {},
multipleDeclaredLanguages: {},
};
},
});
try {
await configUtils.initConfig();
t.fail('initConfig did not throw error');
} catch (err) {
t.deepEqual(err, new Error(configUtils.getQueryUsesInvalid(undefined, "foo/bar@v1@v3")));
}
});
});
test("API client used when reading remote config", async t => {
return await util.withTmpDir(async tmpDir => {
process.env['RUNNER_TEMP'] = tmpDir;

57
src/config-utils.ts
View file

@ -146,10 +146,10 @@ const builtinSuites = ['security-extended', 'security-and-quality'] as const;
* Throws an error if suiteName is not a valid builtin suite.
*/
async function addBuiltinSuiteQueries(
configFile: string,
languages: string[],
resultMap: { [language: string]: string[] },
suiteName: string) {
suiteName: string,
configFile?: string) {
const suite = builtinSuites.find((suite) => suite === suiteName);
if (!suite) {
@ -164,9 +164,9 @@ async function addBuiltinSuiteQueries(
* Retrieve the set of queries at localQueryPath and add them to resultMap.
*/
async function addLocalQueries(
configFile: string,
resultMap: { [language: string]: string[] },
localQueryPath: string) {
localQueryPath: string,
configFile?: string) {
// Resolve the local path against the workspace so that when this is
// passed to codeql it resolves to exactly the path we expect it to resolve to.
@ -195,7 +195,7 @@ async function addLocalQueries(
/**
* Retrieve the set of queries at the referenced remote repo and add them to resultMap.
*/
async function addRemoteQueries(configFile: string, resultMap: { [language: string]: string[] }, queryUses: string) {
async function addRemoteQueries(resultMap: { [language: string]: string[] }, queryUses: string, configFile?: string) {
let tok = queryUses.split('@');
if (tok.length !== 2) {
throw new Error(getQueryUsesInvalid(configFile, queryUses));
@ -235,10 +235,10 @@ async function addRemoteQueries(configFile: string, resultMap: { [language: stri
* a finite set of hardcoded terms for builtin suites.
*/
async function parseQueryUses(
configFile: string,
languages: string[],
resultMap: { [language: string]: string[] },
queryUses: string) {
queryUses: string,
configFile?: string) {
queryUses = queryUses.trim();
if (queryUses === "") {
@ -247,18 +247,18 @@ async function parseQueryUses(
// Check for the local path case before we start trying to parse the repository name
if (queryUses.startsWith("./")) {
await addLocalQueries(configFile, resultMap, queryUses.slice(2));
await addLocalQueries(resultMap, queryUses.slice(2), configFile);
return;
}
// Check for one of the builtin suites
if (queryUses.indexOf('/') === -1 && queryUses.indexOf('@') === -1) {
await addBuiltinSuiteQueries(configFile, languages, resultMap, queryUses);
await addBuiltinSuiteQueries(languages, resultMap, queryUses, configFile);
return;
}
// Otherwise, must be a reference to another repo
await addRemoteQueries(configFile, resultMap, queryUses);
await addRemoteQueries(resultMap, queryUses, configFile);
}
// Regex validating stars in paths or paths-ignore entries.
@ -332,6 +332,9 @@ export function validateAndSanitisePath(
return path;
}
// An undefined configFile in some of these functions indicates that
// the property was in a workflow file, not a config file
export function getNameInvalid(configFile: string): string {
return getConfigFilePropertyError(configFile, NAME_PROPERTY, 'must be a non-empty string');
}
@ -344,7 +347,7 @@ export function getQueriesInvalid(configFile: string): string {
return getConfigFilePropertyError(configFile, QUERIES_PROPERTY, 'must be an array');
}
export function getQueryUsesInvalid(configFile: string, queryUses?: string): string {
export function getQueryUsesInvalid(configFile: string | undefined, queryUses?: string): string {
return getConfigFilePropertyError(
configFile,
QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
@ -361,14 +364,14 @@ export function getPathsInvalid(configFile: string): string {
return getConfigFilePropertyError(configFile, PATHS_PROPERTY, 'must be an array of non-empty strings');
}
export function getLocalPathOutsideOfRepository(configFile: string, localPath: string): string {
export function getLocalPathOutsideOfRepository(configFile: string | undefined, localPath: string): string {
return getConfigFilePropertyError(
configFile,
QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
'is invalid as the local path "' + localPath + '" is outside of the repository');
}
export function getLocalPathDoesNotExist(configFile: string, localPath: string): string {
export function getLocalPathDoesNotExist(configFile: string | undefined, localPath: string): string {
return getConfigFilePropertyError(
configFile,
QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
@ -398,8 +401,12 @@ export function getConfigFileDirectoryGivenMessage(configFile: string): string {
return 'The configuration file "' + configFile + '" looks like a directory, not a file';
}
function getConfigFilePropertyError(configFile: string, property: string, error: string): string {
return 'The configuration file "' + configFile + '" is invalid: property "' + property + '" ' + error;
function getConfigFilePropertyError(configFile: string | undefined, property: string, error: string): string {
if (configFile === undefined) {
return 'The workflow property "' + property + '" is invalid: ' + error;
} else {
return 'The configuration file "' + configFile + '" is invalid: property "' + property + '" ' + error;
}
}
/**
@ -475,16 +482,16 @@ async function getLanguages(): Promise<string[]> {
* Returns true if queries were provided in the workflow file
* (and thus added), otherwise false
*/
function addQueriesFromWorkflowIfRequired(
configFile: string,
async function addQueriesFromWorkflowIfRequired(
languages: string[],
resultMap: { [language: string]: string[] }
): boolean {
resultMap: { [language: string]: string[] },
configFile?: string
): Promise<boolean> {
const queryUses = core.getInput('queries');
if (queryUses) {
queryUses.split(',').forEach(async query => {
await parseQueryUses(configFile, languages, resultMap, query);
});
for (const query of queryUses.split(',')) {
await parseQueryUses(languages, resultMap, query, configFile);
}
return true;
}
@ -498,7 +505,7 @@ export async function getDefaultConfig(): Promise<Config> {
const languages = await getLanguages();
const queries = {};
await addDefaultQueries(languages, queries);
addQueriesFromWorkflowIfRequired('', languages, queries);
await addQueriesFromWorkflowIfRequired(languages, queries);
return {
languages: languages,
@ -560,7 +567,7 @@ async function loadConfig(configFile: string): Promise<Config> {
// If queries were provided using `with` in the action configuration,
// they should take precedence over the queries in the config file
const addedQueriesFromAction = addQueriesFromWorkflowIfRequired(configFile, languages, queries);
const addedQueriesFromAction = await addQueriesFromWorkflowIfRequired(languages, queries, configFile);
if (!addedQueriesFromAction && QUERIES_PROPERTY in parsedYAML) {
if (!(parsedYAML[QUERIES_PROPERTY] instanceof Array)) {
throw new Error(getQueriesInvalid(configFile));
@ -569,7 +576,7 @@ async function loadConfig(configFile: string): Promise<Config> {
if (!(QUERIES_USES_PROPERTY in query) || typeof query[QUERIES_USES_PROPERTY] !== "string") {
throw new Error(getQueryUsesInvalid(configFile));
}
await parseQueryUses(configFile, languages, queries, query[QUERIES_USES_PROPERTY]);
await parseQueryUses(languages, queries, query[QUERIES_USES_PROPERTY], configFile);
}
}