Merge branch 'master' into safe-config-parsing

src/config-utils.test.ts

@@ -91,6 +91,7 @@ test("load non-empty input", async t => {
       name: my config
       disable-default-queries: true
       queries:
+        - uses: ./
         - uses: ./foo
         - uses: foo/bar@dev
       paths-ignore:
@@ -103,7 +104,8 @@ test("load non-empty input", async t => {
     const expectedConfig = new configUtils.Config();
     expectedConfig.name = 'my config';
     expectedConfig.disableDefaultQueries = true;
-    expectedConfig.additionalQueries.push('foo');
+    expectedConfig.additionalQueries.push(tmpDir);
+    expectedConfig.additionalQueries.push(path.join(tmpDir, 'foo'));
     expectedConfig.externalQueries = [new configUtils.ExternalQuery('foo/bar', 'dev')];
     expectedConfig.pathsIgnore = ['a', 'b'];
     expectedConfig.paths = ['c/d'];
@@ -111,6 +113,8 @@ test("load non-empty input", async t => {
     fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
     setInput('config-file', 'input');
 
+    fs.mkdirSync(path.join(tmpDir, 'foo'));
+
     const actualConfig = await configUtils.loadConfig();
 
     // Should exactly equal the object we constructed earlier
@@ -177,7 +181,10 @@ doInvalidInputTest(
       - hello: world`,
   configUtils.getQueryUsesInvalid);
 
-function doInvalidQueryUsesTest(input: string, inputInErrorMessage: boolean) {
+function doInvalidQueryUsesTest(
+  input: string,
+  expectedErrorMessageGenerator: (configFile: string) => string) {
+
   // Invalid contents of a "queries.uses" field.
   // Should fail with the expected error message
   const inputFileContents = `
@@ -189,14 +196,28 @@ function doInvalidQueryUsesTest(input: string, inputInErrorMessage: boolean) {
   doInvalidInputTest(
     "queries uses \"" + input + "\"",
     inputFileContents,
-    configFile => configUtils.getQueryUsesInvalid(
-      configFile,
-      inputInErrorMessage ? input : undefined));
+    expectedErrorMessageGenerator);
 }
 
 // Various "uses" fields, and the errors they should produce
-doInvalidQueryUsesTest("''", false);
-doInvalidQueryUsesTest("foo/bar", true);
-doInvalidQueryUsesTest("foo/bar@v1@v2", true);
-doInvalidQueryUsesTest("foo@master", true);
-doInvalidQueryUsesTest("https://github.com/foo/bar@master", true);
+doInvalidQueryUsesTest(
+  "''",
+  c => configUtils.getQueryUsesInvalid(c, undefined));
+doInvalidQueryUsesTest(
+  "foo/bar",
+  c => configUtils.getQueryUsesInvalid(c, "foo/bar"));
+doInvalidQueryUsesTest(
+  "foo/bar@v1@v2",
+  c => configUtils.getQueryUsesInvalid(c, "foo/bar@v1@v2"));
+doInvalidQueryUsesTest(
+  "foo@master",
+  c => configUtils.getQueryUsesInvalid(c, "foo@master"));
+doInvalidQueryUsesTest(
+  "https://github.com/foo/bar@master",
+  c => configUtils.getQueryUsesInvalid(c, "https://github.com/foo/bar@master"));
+doInvalidQueryUsesTest(
+  "./foo",
+  c => configUtils.getLocalPathDoesNotExist(c, "foo"));
+doInvalidQueryUsesTest(
+  "./..",
+  c => configUtils.getLocalPathOutsideOfRepository(c, ".."));

src/config-utils.ts

@@ -24,11 +24,17 @@ export class ExternalQuery {
     }
 }
 
+// The set of acceptable values for built-in suites from the codeql bundle
+const builtinSuites = ['security-extended', 'security-and-quality'] as const;
+// Derive the union type from the array values
+type BuiltInSuite = typeof builtinSuites[number];
+
 export class Config {
     public name = "";
     public disableDefaultQueries = false;
     public additionalQueries: string[] = [];
     public externalQueries: ExternalQuery[] = [];
+    public additionalSuites: BuiltInSuite[] = [];
     public pathsIgnore: string[] = [];
     public paths: string[] = [];
 
@@ -42,10 +48,37 @@ export class Config {
 
         // Check for the local path case before we start trying to parse the repository name
         if (queryUses.startsWith("./")) {
-            this.additionalQueries.push(queryUses.slice(2));
+            const localQueryPath = queryUses.slice(2);
+            // 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 = util.getRequiredEnvParam('GITHUB_WORKSPACE');
+            const absoluteQueryPath = path.join(workspacePath, localQueryPath);
+
+            // Check the file exists
+            if (!fs.existsSync(absoluteQueryPath)) {
+                throw new Error(getLocalPathDoesNotExist(configFile, localQueryPath));
+            }
+
+            // Check the local path doesn't jump outside the repo using '..' or symlinks
+            if (!(fs.realpathSync(absoluteQueryPath) + path.sep).startsWith(workspacePath + path.sep)) {
+                throw new Error(getLocalPathOutsideOfRepository(configFile, localQueryPath));
+            }
+
+            this.additionalQueries.push(absoluteQueryPath);
             return;
         }
 
+        // Check for one of the builtin suites
+        if (queryUses.indexOf('/') === -1 && queryUses.indexOf('@') === -1) {
+            const suite = builtinSuites.find((suite) => suite === queryUses);
+            if (suite) {
+                this.additionalSuites.push(suite);
+                return;
+            } else {
+                throw new Error(getQueryUsesInvalid(configFile, queryUses));
+            }
+        }
+
         let tok = queryUses.split('@');
         if (tok.length !== 2) {
             throw new Error(getQueryUsesInvalid(configFile, queryUses));
@@ -92,7 +125,8 @@ export function getQueryUsesInvalid(configFile: string, queryUses?: string): str
     return getConfigFilePropertyError(
         configFile,
         QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
-        'must be non-empty string containing either a local path starting with "./", or be of the form "owner/repo[/path]@ref"' +
+        'must be a built-in suite (' + builtinSuites.join(' or ') +
+            '), a relative path, or be of the form "owner/repo[/path]@ref"' +
             (queryUses !== undefined ? '\n Found: ' + queryUses : ''));
 }
 
@@ -104,6 +138,20 @@ export function getPathsInvalid(configFile: string): string {
     return getConfigFilePropertyError(configFile, PATHS_PROPERTY, 'must be an array of non-empty string');
 }
 
+export function getLocalPathOutsideOfRepository(configFile: string, localPath: string): string {
+    return getConfigFilePropertyError(
+        configFile,
+        QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
+        'is invalid as the local path "' + localPath + '" is output of the repository');
+}
+
+export function getLocalPathDoesNotExist(configFile: string, localPath: string): string {
+    return getConfigFilePropertyError(
+        configFile,
+        QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY,
+        'is invalid as the local path "' + localPath + '" does not exist in the repository');
+}
+
 export function getConfigFileOutsideWorkspaceErrorMessage(configFile: string): string {
     return 'The configuration file "' + configFile + '" is outside of the workspace';
 }

src/finalize-db.ts

@@ -69,32 +69,74 @@ async function finalizeDatabaseCreation(codeqlCmd: string, databaseFolder: strin
   }
 }
 
+interface ResolveQueriesOutput {
+  byLanguage: {
+    [language: string]: {
+      [queryPath: string]: {}
+    }
+  };
+  noDeclaredLanguage: {
+    [queryPath: string]: {}
+  };
+  multipleDeclaredLanguages: {
+    [queryPath: string]: {}
+  };
+}
+
+async function runResolveQueries(codeqlCmd: string, queries: string[]): Promise<ResolveQueriesOutput> {
+  let output = '';
+  const options = {
+    listeners: {
+      stdout: (data: Buffer) => {
+        output += data.toString();
+      }
+    }
+  };
+
+  await exec.exec(
+    codeqlCmd, [
+      'resolve',
+      'queries',
+      ...queries,
+      '--format=bylanguage'
+    ],
+    options);
+
+  return JSON.parse(output);
+}
+
 async function resolveQueryLanguages(codeqlCmd: string, config: configUtils.Config): Promise<Map<string, string[]>> {
   let res = new Map();
 
-  if (config.additionalQueries.length !== 0) {
-    let resolveQueriesOutput = '';
-    const options = {
-      listeners: {
-        stdout: (data: Buffer) => {
-          resolveQueriesOutput += data.toString();
-        }
+  if (!config.disableDefaultQueries || config.additionalSuites.length !== 0) {
+    const suites: string[] = [];
+    for (const language of await util.getLanguages()) {
+      if (!config.disableDefaultQueries) {
+        suites.push(language + '-code-scanning.qls');
       }
-    };
+      for (const additionalSuite of config.additionalSuites) {
+        suites.push(language + '-' + additionalSuite + '.qls');
+      }
+    }
 
-    await exec.exec(
-      codeqlCmd, [
-        'resolve',
-        'queries',
-        ...config.additionalQueries,
-        '--format=bylanguage'
-      ],
-      options);
-
-    const resolveQueriesOutputObject = JSON.parse(resolveQueriesOutput);
+    const resolveQueriesOutputObject = await runResolveQueries(codeqlCmd, suites);
 
     for (const [language, queries] of Object.entries(resolveQueriesOutputObject.byLanguage)) {
-      res[language] = Object.keys(<any>queries);
+      if (res[language] === undefined) {
+        res[language] = [];
+      }
+      res[language].push(...Object.keys(<any>queries));
+    }
+  }
+
+  if (config.additionalQueries.length !== 0) {
+    const resolveQueriesOutputObject = await runResolveQueries(codeqlCmd, config.additionalQueries);
+
+    for (const [language, queries] of Object.entries(resolveQueriesOutputObject.byLanguage)) {
+      if (res[language] === undefined) {
+        res[language] = [];
+      }
+      res[language].push(...Object.keys(<any>queries));
     }
 
     const noDeclaredLanguage = resolveQueriesOutputObject.noDeclaredLanguage;
@@ -120,11 +162,17 @@ async function runQueries(codeqlCmd: string, databaseFolder: string, sarifFolder
   for (let database of fs.readdirSync(databaseFolder)) {
     core.startGroup('Analyzing ' + database);
 
-    const queries: string[] = [];
-    if (!config.disableDefaultQueries) {
-      queries.push(database + '-code-scanning.qls');
+    const queries = queriesPerLanguage[database] || [];
+    if (queries.length === 0) {
+      throw new Error('Unable to analyse ' + database + ' as no queries were selected for this language');
     }
-    queries.push(...(queriesPerLanguage[database] || []));
+
+    // Pass the queries to codeql using a file instead of using the command
+    // line to avoid command line length restrictions, particularly on windows.
+    const querySuite = path.join(databaseFolder, database + '-queries.qls');
+    const querySuiteContents = queries.map(q => '- query: ' + q).join('\n');
+    fs.writeFileSync(querySuite, querySuiteContents);
+    core.debug('Query suite file for ' + database + '...\n' + querySuiteContents);
 
     const sarifFile = path.join(sarifFolder, database + '.sarif');
 
@@ -136,7 +184,7 @@ async function runQueries(codeqlCmd: string, databaseFolder: string, sarifFolder
       '--format=sarif-latest',
       '--output=' + sarifFile,
       '--no-sarif-add-snippets',
-      ...queries
+      querySuite
     ]);
 
     core.debug('SARIF results for database ' + database + ' created at "' + sarifFile + '"');

src/upload-lib.ts

@@ -136,7 +136,7 @@ async function uploadFiles(sarifFiles: string[]): Promise<boolean> {
         }
         core.exportVariable(sentinelEnvVar, sentinelEnvVar);
 
-        const commitOid = util.getRequiredEnvParam('GITHUB_SHA');
+        const commitOid = await util.getCommitOid();
         const workflowRunIDStr = util.getRequiredEnvParam('GITHUB_RUN_ID');
         const ref = util.getRef();
         const analysisKey = await util.getAnalysisKey();

src/util.ts

@@ -1,4 +1,5 @@
 import * as core from '@actions/core';
+import * as exec from '@actions/exec';
 import * as http from '@actions/http-client';
 import * as auth from '@actions/http-client/auth';
 import * as octokit from '@octokit/rest';
@@ -25,13 +26,6 @@ export function should_abort(actionName: string, requireInitActionHasRun: boolea
         return true;
     }
 
-    // Should abort if called on a merge commit for a pull request.
-    if (ref.startsWith('refs/pull/')) {
-        core.warning('The CodeQL ' + actionName + ' action is intended for workflows triggered on `push` events, '
-            + 'but the current workflow is running on a pull request. Aborting.');
-        return true;
-    }
-
     // If the init action is required, then check the it completed successfully.
     if (requireInitActionHasRun && process.env[sharedEnv.CODEQL_ACTION_INIT_COMPLETED] === undefined) {
         core.setFailed('The CodeQL ' + actionName + ' action cannot be used unless the CodeQL init action is run first. Aborting.');
@@ -152,6 +146,21 @@ export async function getLanguages(): Promise<string[]> {
     return languages;
 }
 
+/**
+ * Gets the SHA of the commit that is currently checked out.
+ */
+export async function getCommitOid(): Promise<string> {
+    let commitOid = '';
+    await exec.exec('git', ['rev-parse', 'HEAD'], {
+        silent: true,
+        listeners: {
+            stdout: (data) => { commitOid += data.toString(); },
+            stderr: (data) => { process.stderr.write(data); }
+        }
+    });
+    return commitOid.trim();
+}
+
 /**
  * Get the path of the currently executing workflow.
  */
@@ -204,8 +213,20 @@ export async function getAnalysisKey(): Promise<string> {
  * Get the ref currently being analyzed.
  */
 export function getRef(): string {
-    // it's in the form "refs/heads/master"
-    return getRequiredEnvParam('GITHUB_REF');
+    // Will be in the form "refs/heads/master" on a push event
+    // or in the form "refs/pull/N/merge" on a pull_request event
+    const ref = getRequiredEnvParam('GITHUB_REF');
+
+    // For pull request refs we want to convert from the 'merge' ref
+    // to the 'head' ref, as that is what we want to analyse.
+    // There should have been some code earlier in the workflow to do
+    // the checkout, but we have no way of verifying that here.
+    const pull_ref_regex = /refs\/pull\/(\d+)\/merge/;
+    if (pull_ref_regex.test(ref)) {
+        return ref.replace(pull_ref_regex, 'refs/pull/$1/head');
+    } else {
+        return ref;
+    }
 }
 
 interface StatusReport {