Prevent queries in workflow overriding default queries

https://github.com/github/codeql-action/pull/127#pullrequestreview-463207781

lib/config-utils.js

@@ -350,6 +350,12 @@ async function getDefaultConfig() {
     const languages = await getLanguages();
     const queries = {};
     await addDefaultQueries(languages, queries);
+    const queryUses = core.getInput('queries');
+    if (queryUses) {
+        queryUses.split(',').forEach(async (query) => {
+            await parseQueryUses('', languages, queries, query);
+        });
+    }
     return {
         languages: languages,
         queries: queries,
@@ -402,7 +408,15 @@ async function loadConfig(configFile) {
     if (!disableDefaultQueries) {
         await addDefaultQueries(languages, queries);
     }
-    if (QUERIES_PROPERTY in parsedYAML) {
+    // If queries were provided using `with` in the action configuration,
+    // they should take precedence over the queries in the config file
+    const queryUses = core.getInput('queries');
+    if (queryUses) {
+        queryUses.split(',').forEach(async (query) => {
+            await parseQueryUses(configFile, languages, queries, query);
+        });
+    }
+    else if (QUERIES_PROPERTY in parsedYAML) {
         if (!(parsedYAML[QUERIES_PROPERTY] instanceof Array)) {
             throw new Error(getQueriesInvalid(configFile));
         }
@@ -460,12 +474,6 @@ async function initConfig() {
     else {
         config = await loadConfig(configFile);
     }
-    // If queries were provided using `with` in the action configuration,
-    // they should take precedence over the queries in the config file
-    const queryUses = core.getInput('queries');
-    if (queryUses) {
-        config = await updateConfigWithQueries(config, queryUses, configFile);
-    }
     // Save the config so we can easily access it again in the future
     await saveConfig(config);
     return config;
@@ -478,20 +486,6 @@ function isLocal(configPath) {
     }
     return (configPath.indexOf("@") === -1);
 }
-async function updateConfigWithQueries(config, queryUses, configPath) {
-    if (isLocal(configPath)) {
-        // Treat the config file as relative to the workspace
-        const workspacePath = util.getRequiredEnvParam('GITHUB_WORKSPACE');
-        configPath = path.resolve(workspacePath, configPath);
-    }
-    const languages = await getLanguages();
-    const queries = {};
-    queryUses.split(',').forEach(async (query) => {
-        await parseQueryUses(configPath, languages, queries, query);
-    });
-    config.queries = queries;
-    return config;
-}
 function getLocalConfig(configFile, workspacePath) {
     // Error if the config file is now outside of the workspace
     if (!(configFile + path.sep).startsWith(workspacePath + path.sep)) {

lib/config-utils.test.js

@@ -224,7 +224,7 @@ ava_1.default("default queries are used", async (t) => {
         t.deepEqual(resolveQueriesArgs[0].extraSearchPath, undefined);
     });
 });
-ava_1.default("Default queries and those from config file can be overridden in action file", async (t) => {
+ava_1.default("Queries from config file can be overridden in workflow file", async (t) => {
     return await util.withTmpDir(async (tmpDir) => {
         process.env['RUNNER_TEMP'] = tmpDir;
         process.env['GITHUB_WORKSPACE'] = tmpDir;
@@ -234,7 +234,7 @@ ava_1.default("Default queries and those from config file can be overridden in a
         - uses: ./foo`;
         fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
         setInput('config-file', 'input');
-        // This config item should take precedence over the config file and the default queries.
+        // This config item should take precedence over the config file but shouldn't affect the default queries.
         setInput('queries', './override');
         fs.mkdirSync(path.join(tmpDir, 'foo'));
         fs.mkdirSync(path.join(tmpDir, 'override'));
@@ -259,23 +259,23 @@ ava_1.default("Default queries and those from config file can be overridden in a
         setInput('languages', 'javascript');
         const config = await configUtils.initConfig();
         // Check resolveQueries was called correctly
-        // It'll be called once for the default queries, once for './foo' (from the config file),
-        // and then finally for './override'
-        t.deepEqual(resolveQueriesArgs.length, 3);
-        t.deepEqual(resolveQueriesArgs[2].queries.length, 1);
-        t.regex(resolveQueriesArgs[2].queries[0], /.*\/override$/);
-        // Now check that the end result contains only the override query, not the others
-        t.deepEqual(config.queries['javascript'].length, 1);
-        t.regex(config.queries['javascript'][0], /.*\/override$/);
+        // It'll be called once for the default queries and once for `./override`,
+        // but won't be called for './foo' from the config file.
+        t.deepEqual(resolveQueriesArgs.length, 2);
+        t.deepEqual(resolveQueriesArgs[1].queries.length, 1);
+        t.regex(resolveQueriesArgs[1].queries[0], /.*\/override$/);
+        // Now check that the end result contains only the default queries and the override query
+        t.deepEqual(config.queries['javascript'].length, 2);
+        t.regex(config.queries['javascript'][0], /javascript-code-scanning.qls$/);
+        t.regex(config.queries['javascript'][1], /.*\/override$/);
     });
 });
-ava_1.default("Multiple overriding queries can be specified in action file", async (t) => {
+ava_1.default("Multiple queries can be specified in workflow file, no config file required", async (t) => {
     return await util.withTmpDir(async (tmpDir) => {
         process.env['RUNNER_TEMP'] = tmpDir;
         process.env['GITHUB_WORKSPACE'] = tmpDir;
         fs.mkdirSync(path.join(tmpDir, 'override1'));
         fs.mkdirSync(path.join(tmpDir, 'override2'));
-        // This config item should take precedence.
         setInput('queries', './override1,./override2');
         const resolveQueriesArgs = [];
         CodeQL.setCodeQL({
@@ -299,16 +299,17 @@ ava_1.default("Multiple overriding queries can be specified in action file", asy
         const config = await configUtils.initConfig();
         // Check resolveQueries was called correctly:
         // It'll be called once for the default queries,
-        // and then once for each of the two overrides
+        // and then once for each of the two queries from the workflow
         t.deepEqual(resolveQueriesArgs.length, 3);
         t.deepEqual(resolveQueriesArgs[1].queries.length, 1);
         t.deepEqual(resolveQueriesArgs[2].queries.length, 1);
         t.regex(resolveQueriesArgs[1].queries[0], /.*\/override1$/);
         t.regex(resolveQueriesArgs[2].queries[0], /.*\/override2$/);
-        // Now check that the end result contains only the override queries, not the defaults
-        t.deepEqual(config.queries['javascript'].length, 2);
-        t.regex(config.queries['javascript'][0], /.*\/override1$/);
-        t.regex(config.queries['javascript'][1], /.*\/override2$/);
+        // Now check that the end result contains both the queries from the workflow, as well as the defaults
+        t.deepEqual(config.queries['javascript'].length, 3);
+        t.regex(config.queries['javascript'][0], /javascript-code-scanning.qls$/);
+        t.regex(config.queries['javascript'][1], /.*\/override1$/);
+        t.regex(config.queries['javascript'][2], /.*\/override2$/);
     });
 });
 ava_1.default("API client used when reading remote config", async (t) => {

src/config-utils.test.ts

@@ -254,7 +254,7 @@ test("default queries are used", async t => {
   });
 });
 
-test("Default queries and those from config file can be overridden in action file", async t => {
+test("Queries from config file can be overridden in workflow file", async t => {
   return await util.withTmpDir(async tmpDir => {
     process.env['RUNNER_TEMP'] = tmpDir;
     process.env['GITHUB_WORKSPACE'] = tmpDir;
@@ -267,7 +267,7 @@ test("Default queries and those from config file can be overridden in action fil
     fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
     setInput('config-file', 'input');
 
-    // This config item should take precedence over the config file and the default queries.
+    // This config item should take precedence over the config file but shouldn't affect the default queries.
     setInput('queries', './override');
 
     fs.mkdirSync(path.join(tmpDir, 'foo'));
@@ -297,19 +297,20 @@ test("Default queries and those from config file can be overridden in action fil
     const config = await configUtils.initConfig();
 
     // Check resolveQueries was called correctly
-    // It'll be called once for the default queries, once for './foo' (from the config file),
-    // and then finally for './override'
-    t.deepEqual(resolveQueriesArgs.length, 3);
-    t.deepEqual(resolveQueriesArgs[2].queries.length, 1);
-    t.regex(resolveQueriesArgs[2].queries[0], /.*\/override$/);
+    // It'll be called once for the default queries and once for `./override`,
+    // but won't be called for './foo' from the config file.
+    t.deepEqual(resolveQueriesArgs.length, 2);
+    t.deepEqual(resolveQueriesArgs[1].queries.length, 1);
+    t.regex(resolveQueriesArgs[1].queries[0], /.*\/override$/);
 
-    // Now check that the end result contains only the override query, not the others
-    t.deepEqual(config.queries['javascript'].length, 1);
-    t.regex(config.queries['javascript'][0], /.*\/override$/);
+    // Now check that the end result contains only the default queries and the override query
+    t.deepEqual(config.queries['javascript'].length, 2);
+    t.regex(config.queries['javascript'][0], /javascript-code-scanning.qls$/);
+    t.regex(config.queries['javascript'][1], /.*\/override$/);
   });
 });
 
-test("Multiple overriding queries can be specified in action file", async t => {
+test("Multiple queries can be specified in workflow file, no config file required", async t => {
   return await util.withTmpDir(async tmpDir => {
     process.env['RUNNER_TEMP'] = tmpDir;
     process.env['GITHUB_WORKSPACE'] = tmpDir;
@@ -317,7 +318,6 @@ test("Multiple overriding queries can be specified in action file", async t => {
     fs.mkdirSync(path.join(tmpDir, 'override1'));
     fs.mkdirSync(path.join(tmpDir, 'override2'));
 
-    // This config item should take precedence.
     setInput('queries', './override1,./override2');
 
     const resolveQueriesArgs: {queries: string[], extraSearchPath: string | undefined}[] = [];
@@ -345,17 +345,18 @@ test("Multiple overriding queries can be specified in action file", async t => {
 
     // Check resolveQueries was called correctly:
     // It'll be called once for the default queries,
-    // and then once for each of the two overrides
+    // and then once for each of the two queries from the workflow
     t.deepEqual(resolveQueriesArgs.length, 3);
     t.deepEqual(resolveQueriesArgs[1].queries.length, 1);
     t.deepEqual(resolveQueriesArgs[2].queries.length, 1);
     t.regex(resolveQueriesArgs[1].queries[0], /.*\/override1$/);
     t.regex(resolveQueriesArgs[2].queries[0], /.*\/override2$/);
 
-    // Now check that the end result contains only the override queries, not the defaults
-    t.deepEqual(config.queries['javascript'].length, 2);
-    t.regex(config.queries['javascript'][0], /.*\/override1$/);
-    t.regex(config.queries['javascript'][1], /.*\/override2$/);
+    // Now check that the end result contains both the queries from the workflow, as well as the defaults
+    t.deepEqual(config.queries['javascript'].length, 3);
+    t.regex(config.queries['javascript'][0], /javascript-code-scanning.qls$/);
+    t.regex(config.queries['javascript'][1], /.*\/override1$/);
+    t.regex(config.queries['javascript'][2], /.*\/override2$/);
   });
 });
 

src/config-utils.ts

@@ -478,6 +478,13 @@ export async function getDefaultConfig(): Promise<Config> {
   const languages = await getLanguages();
   const queries = {};
   await addDefaultQueries(languages, queries);
+  const queryUses = core.getInput('queries');
+  if (queryUses) {
+    queryUses.split(',').forEach(async query => {
+      await parseQueryUses('', languages, queries, query);
+    });
+  }
+
   return {
     languages: languages,
     queries: queries,
@@ -536,7 +543,14 @@ async function loadConfig(configFile: string): Promise<Config> {
     await addDefaultQueries(languages, queries);
   }
 
-  if (QUERIES_PROPERTY in parsedYAML) {
+  // If queries were provided using `with` in the action configuration,
+  // they should take precedence over the queries in the config file
+  const queryUses = core.getInput('queries');
+  if (queryUses) {
+    queryUses.split(',').forEach(async query => {
+      await parseQueryUses(configFile, languages, queries, query);
+    });
+  } else if (QUERIES_PROPERTY in parsedYAML) {
     if (!(parsedYAML[QUERIES_PROPERTY] instanceof Array)) {
       throw new Error(getQueriesInvalid(configFile));
     }
@@ -599,13 +613,6 @@ export async function initConfig(): Promise<Config> {
     config = await loadConfig(configFile);
   }
 
-  // If queries were provided using `with` in the action configuration,
-  // they should take precedence over the queries in the config file
-  const queryUses = core.getInput('queries');
-  if (queryUses) {
-    config = await updateConfigWithQueries(config, queryUses, configFile);
-  }
-
   // Save the config so we can easily access it again in the future
   await saveConfig(config);
   return config;
@@ -620,23 +627,6 @@ function isLocal(configPath: string): boolean {
   return (configPath.indexOf("@") === -1);
 }
 
-async function updateConfigWithQueries(config: Config, queryUses: string, configPath: string): Promise<Config> {
-  if (isLocal(configPath)) {
-    // Treat the config file as relative to the workspace
-    const workspacePath = util.getRequiredEnvParam('GITHUB_WORKSPACE');
-    configPath = path.resolve(workspacePath, configPath);
-  }
-
-  const languages = await getLanguages();
-  const queries = {};
-  queryUses.split(',').forEach(async query => {
-    await parseQueryUses(configPath, languages, queries, query);
-  });
-  config.queries = queries;
-
-  return config;
-}
-
 function getLocalConfig(configFile: string, workspacePath: string): UserConfig {
   // Error if the config file is now outside of the workspace
   if (!(configFile + path.sep).startsWith(workspacePath + path.sep)) {