Merge pull request #36 from github/config_utils_tests

Add tests for config-utils

package.json

@@ -5,7 +5,7 @@
   "description": "CodeQL action",
   "scripts": {
     "build": "tsc",
-    "test": "ava src/**",
+    "test": "ava src/** --serial",
     "lint": "tslint -p . -c tslint.json 'src/**/*.ts'",
     "removeNPMAbsolutePaths": "removeNPMAbsolutePaths . --force"
   },

src/config-utils.test.ts

@@ -0,0 +1,253 @@
+import test from 'ava';
+import * as fs from 'fs';
+import * as path from 'path';
+
+import * as configUtils from './config-utils';
+import * as util from './util';
+
+function setInput(name: string, value: string | undefined) {
+  // Transformation copied from
+  // https://github.com/actions/toolkit/blob/05e39f551d33e1688f61b209ab5cdd335198f1b8/packages/core/src/core.ts#L69
+  const envVar = `INPUT_${name.replace(/ /g, '_').toUpperCase()}`;
+  if (value !== undefined) {
+    process.env[envVar] = value;
+  } else {
+    delete process.env[envVar];
+  }
+}
+
+test("load empty config", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    setInput('config-file', undefined);
+
+    const config = await configUtils.loadConfig();
+
+    t.deepEqual(config, new configUtils.Config());
+  });
+});
+
+test("loading config saves config", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    const configFile = configUtils.getConfigFile();
+    // Sanity check the saved config file does not already exist
+    t.false(fs.existsSync(configFile));
+
+    const config = await configUtils.loadConfig();
+
+    // The saved config file should now exist
+    t.true(fs.existsSync(configFile));
+
+    // And the contents should parse correctly to the config that was returned
+    t.deepEqual(fs.readFileSync(configFile, 'utf8'), JSON.stringify(config));
+  });
+});
+
+test("load input outside of workspace", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    setInput('config-file', '../input');
+
+    try {
+      await configUtils.loadConfig();
+      throw new Error('loadConfig did not throw error');
+    } catch (err) {
+      t.deepEqual(err, new Error(configUtils.getConfigFileOutsideWorkspaceErrorMessage(path.join(tmpDir, '../input'))));
+    }
+  });
+});
+
+test("load non-existent input", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    t.false(fs.existsSync(path.join(tmpDir, 'input')));
+    setInput('config-file', 'input');
+
+    try {
+      await configUtils.loadConfig();
+      throw new Error('loadConfig did not throw error');
+    } catch (err) {
+      t.deepEqual(err, new Error(configUtils.getConfigFileDoesNotExistErrorMessage(path.join(tmpDir, 'input'))));
+    }
+  });
+});
+
+test("load non-empty input", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    // Just create a generic config object with non-default values for all fields
+    const inputFileContents = `
+      name: my config
+      disable-default-queries: true
+      queries:
+        - uses: ./foo
+        - uses: foo/bar@dev
+      paths-ignore:
+        - a
+        - b
+      paths:
+        - c/d`;
+
+    // And the config we expect it to parse to
+    const expectedConfig = new configUtils.Config();
+    expectedConfig.name = 'my config';
+    expectedConfig.disableDefaultQueries = true;
+    expectedConfig.additionalQueries.push('foo');
+    expectedConfig.externalQueries = [new configUtils.ExternalQuery('foo/bar', 'dev')];
+    expectedConfig.pathsIgnore = ['a', 'b'];
+    expectedConfig.paths = ['c/d'];
+
+    fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
+    setInput('config-file', 'input');
+
+    const actualConfig = await configUtils.loadConfig();
+
+    // Should exactly equal the object we constructed earlier
+    t.deepEqual(actualConfig, expectedConfig);
+  });
+});
+
+test("load partially invalid input", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    // The valid parts of this config should be parsed correctly.
+    // The invalid parts should be ignored and left as the default values.
+    const inputFileContents = `
+      name:
+        - foo: bar
+      disable-default-queries: 42
+      queries:
+        - name: foo/bar
+          uses: foo/bar@dev
+      paths-ignore:
+        - a
+        - b
+      paths:
+        - c/d`;
+
+    // And the config we expect it to parse to
+    const expectedConfig = new configUtils.Config();
+    expectedConfig.externalQueries = [new configUtils.ExternalQuery('foo/bar', 'dev')];
+    expectedConfig.pathsIgnore = ['a', 'b'];
+    expectedConfig.paths = ['c/d'];
+
+    fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
+    setInput('config-file', 'input');
+
+    const actualConfig = await configUtils.loadConfig();
+
+    // Should exactly equal the object we constructed earlier
+    t.deepEqual(actualConfig, expectedConfig);
+  });
+});
+
+test("load invalid input - top level entries", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    // Replace the arrays with strings or numbers.
+    // The invalid parts should be ignored and left as the default values.
+    const inputFileContents = `
+      name: my config
+      disable-default-queries: true
+      queries: foo
+      paths-ignore: bar
+      paths: 123`;
+
+    // And the config we expect it to parse to
+    const expectedConfig = new configUtils.Config();
+    expectedConfig.name = 'my config';
+    expectedConfig.disableDefaultQueries = true;
+
+    fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
+    setInput('config-file', 'input');
+
+    const actualConfig = await configUtils.loadConfig();
+
+    // Should exactly equal the object we constructed earlier
+    t.deepEqual(actualConfig, expectedConfig);
+  });
+});
+
+test("load invalid input - queries field type", async t => {
+  return await util.withTmpDir(async tmpDir => {
+    process.env['RUNNER_WORKSPACE'] = tmpDir;
+    process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+    // Invalid contents of the "queries" array.
+    // The invalid parts should be ignored and left as the default values.
+    const inputFileContents = `
+      name: my config
+      disable-default-queries: true
+      queries:
+        - name: foo
+          uses:
+            - hello: world
+        - name: bar
+          uses: github/bar@master`;
+
+    // And the config we expect it to parse to
+    const expectedConfig = new configUtils.Config();
+    expectedConfig.name = 'my config';
+    expectedConfig.disableDefaultQueries = true;
+    expectedConfig.externalQueries.push(new configUtils.ExternalQuery("github/bar", "master"));
+
+    fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
+    setInput('config-file', 'input');
+
+    const actualConfig = await configUtils.loadConfig();
+
+    // Should exactly equal the object we constructed earlier
+    t.deepEqual(actualConfig, expectedConfig);
+  });
+});
+
+// Various "uses" fields, and the errors they should produce
+const testInputs = {
+  "''": configUtils.getQueryUsesBlank(),
+  "foo/bar": configUtils.getQueryUsesIncorrect("foo/bar"),
+  "foo/bar@v1@v2": configUtils.getQueryUsesIncorrect("foo/bar@v1@v2"),
+  "foo@master": configUtils.getQueryUsesIncorrect("foo@master"),
+  "https://github.com/foo/bar@master": configUtils.getQueryUsesIncorrect("https://github.com/foo/bar@master")
+};
+
+for (const [input, result] of Object.entries(testInputs)) {
+  test("load invalid input - queries uses \"" + input + "\"", async t => {
+    return await util.withTmpDir(async tmpDir => {
+      process.env['RUNNER_WORKSPACE'] = tmpDir;
+      process.env['GITHUB_WORKSPACE'] = tmpDir;
+
+      // Invalid contents of a "queries.uses" field.
+      // Should fail with the expected error message
+      const inputFileContents = `
+        name: my config
+        queries:
+          - name: foo
+            uses: ` + input;
+
+      fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8');
+      setInput('config-file', 'input');
+
+      try {
+        await configUtils.loadConfig();
+        throw new Error('loadConfig did not throw error');
+      } catch (err) {
+        t.deepEqual(err, new Error(result));
+      }
+    });
+  });
+}

src/config-utils.ts

@@ -4,6 +4,8 @@ import * as fs from 'fs';
 import * as yaml from 'js-yaml';
 import * as path from 'path';
 
+import * as util from './util';
+
 export class ExternalQuery {
     public repository: string;
     public ref: string;
@@ -26,32 +28,37 @@ export class Config {
     public addQuery(queryUses: string) {
         // The logic for parsing the string is based on what actions does for
         // parsing the 'uses' actions in the workflow file
-
+        queryUses = queryUses.trim();
         if (queryUses === "") {
-            throw '"uses" value for queries cannot be blank';
+            throw new Error(getQueryUsesBlank());
         }
 
+        // Check for the local path case before we start trying to parse the repository name
         if (queryUses.startsWith("./")) {
             this.additionalQueries.push(queryUses.slice(2));
             return;
         }
 
         let tok = queryUses.split('@');
         if (tok.length !== 2) {
-            throw '"uses" value for queries must be a path, or owner/repo@ref \n Found: ' + queryUses;
+            throw new Error(getQueryUsesIncorrect(queryUses));
         }
 
         const ref = tok[1];
         tok = tok[0].split('/');
         // The first token is the owner
         // The second token is the repo
         // The rest is a path, if there is more than one token combine them to form the full path
+        if (tok.length < 2) {
+            throw new Error(getQueryUsesIncorrect(queryUses));
+        }
         if (tok.length > 3) {
             tok = [tok[0], tok[1], tok.slice(2).join('/')];
         }
 
-        if (tok.length < 2) {
-            throw '"uses" value for queries must be a path, or owner/repo@ref \n Found: ' + queryUses;
+        // Check none of the parts of the repository name are empty
+        if (tok[0].trim() === '' || tok[1].trim() === '') {
+            throw new Error(getQueryUsesIncorrect(queryUses));
         }
 
         let external = new ExternalQuery(tok[0] + '/' + tok[1], ref);
@@ -62,10 +69,24 @@ export class Config {
     }
 }
 
-const configFolder = process.env['RUNNER_WORKSPACE'] || '/tmp/codeql-action';
+export function getQueryUsesBlank(): string {
+    return '"uses" value for queries cannot be blank';
+}
+
+export function getQueryUsesIncorrect(queryUses: string): string {
+    return '"uses" value for queries must be a path, or owner/repo@ref \n Found: ' + queryUses;
+}
+
+export function getConfigFileOutsideWorkspaceErrorMessage(configFile: string): string {
+    return 'The configuration file "' + configFile + '" is outside of the workspace';
+}
+
+export function getConfigFileDoesNotExistErrorMessage(configFile: string): string {
+    return 'The configuration file "' + configFile + '" does not exist';
+}
 
 function initConfig(): Config {
-    const configFile = core.getInput('config-file');
+    let configFile = core.getInput('config-file');
 
     const config = new Config();
 
@@ -75,60 +96,78 @@ function initConfig(): Config {
         return config;
     }
 
-    try {
-        const parsedYAML = yaml.safeLoad(fs.readFileSync(configFile, 'utf8'));
+    // Treat the config file as relative to the workspace
+    const workspacePath = util.getRequiredEnvParam('GITHUB_WORKSPACE');
+    configFile = path.resolve(workspacePath, configFile);
 
-        if (parsedYAML.name && typeof parsedYAML.name === "string") {
-            config.name = parsedYAML.name;
-        }
+    // Error if the config file is now outside of the workspace
+    if (!(configFile + path.sep).startsWith(workspacePath + path.sep)) {
+        throw new Error(getConfigFileOutsideWorkspaceErrorMessage(configFile));
+    }
 
-        if (parsedYAML['disable-default-queries'] && typeof parsedYAML['disable-default-queries'] === "boolean") {
-            config.disableDefaultQueries = parsedYAML['disable-default-queries'];
-        }
+    // Error if the file does not exist
+    if (!fs.existsSync(configFile)) {
+        throw new Error(getConfigFileDoesNotExistErrorMessage(configFile));
+    }
 
-        const queries = parsedYAML.queries;
-        if (queries && queries instanceof Array) {
-            queries.forEach(query => {
-                if (query.uses && typeof query.uses === "string") {
-                    config.addQuery(query.uses);
-                }
-            });
-        }
+    const parsedYAML = yaml.safeLoad(fs.readFileSync(configFile, 'utf8'));
 
-        const pathsIgnore = parsedYAML['paths-ignore'];
-        if (pathsIgnore && pathsIgnore instanceof Array) {
-            pathsIgnore.forEach(path => {
-                if (typeof path === "string") {
-                    config.pathsIgnore.push(path);
-                }
-            });
-        }
+    if (parsedYAML.name && typeof parsedYAML.name === "string") {
+        config.name = parsedYAML.name;
+    }
 
-        const paths = parsedYAML.paths;
-        if (paths && paths instanceof Array) {
-            paths.forEach(path => {
-                if (typeof path === "string") {
-                    config.paths.push(path);
-                }
-            });
-        }
-    } catch (err) {
-        core.setFailed(err);
+    if (parsedYAML['disable-default-queries'] && typeof parsedYAML['disable-default-queries'] === "boolean") {
+        config.disableDefaultQueries = parsedYAML['disable-default-queries'];
+    }
+
+    const queries = parsedYAML.queries;
+    if (queries && queries instanceof Array) {
+        queries.forEach(query => {
+            if (typeof query.uses === "string") {
+                config.addQuery(query.uses);
+            }
+        });
+    }
+
+    const pathsIgnore = parsedYAML['paths-ignore'];
+    if (pathsIgnore && pathsIgnore instanceof Array) {
+        pathsIgnore.forEach(path => {
+            if (typeof path === "string") {
+                config.pathsIgnore.push(path);
+            }
+        });
+    }
+
+    const paths = parsedYAML.paths;
+    if (paths && paths instanceof Array) {
+        paths.forEach(path => {
+            if (typeof path === "string") {
+                config.paths.push(path);
+            }
+        });
     }
 
     return config;
 }
 
+function getConfigFolder(): string {
+    return util.getRequiredEnvParam('RUNNER_WORKSPACE');
+}
+
+export function getConfigFile(): string {
+    return path.join(getConfigFolder(), 'config');
+}
+
 async function saveConfig(config: Config) {
     const configString = JSON.stringify(config);
-    await io.mkdirP(configFolder);
-    fs.writeFileSync(path.join(configFolder, 'config'), configString, 'utf8');
+    await io.mkdirP(getConfigFolder());
+    fs.writeFileSync(getConfigFile(), configString, 'utf8');
     core.debug('Saved config:');
     core.debug(configString);
 }
 
 export async function loadConfig(): Promise<Config> {
-    const configFile = path.join(configFolder, 'config');
+    const configFile = getConfigFile();
     if (fs.existsSync(configFile)) {
         const configString = fs.readFileSync(configFile, 'utf8');
         core.debug('Loaded config:');

src/util.ts

@@ -377,8 +377,9 @@ export function getToolNames(sarifContents: string): string[] {
 
 // Creates a random temporary directory, runs the given body, and then deletes the directory.
 // Mostly intended for use within tests.
-export async function withTmpDir(body: (tmpDir: string) => Promise<void>) {
+export async function withTmpDir<T>(body: (tmpDir: string) => Promise<T>): Promise<T> {
     const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codeql-action-'));
-    await body(tmpDir);
+    const result = await body(tmpDir);
     fs.rmdirSync(tmpDir, { recursive: true });
+    return result;
 }