Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion lib/analyze-action-post.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/analyze-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/autobuild-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/init-action-post.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lib/init-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/resolve-environment-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/start-proxy-action-post.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/upload-lib.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion lib/upload-sarif-action.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

51 changes: 51 additions & 0 deletions src/config-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ test("load code quality config", async (t) => {

// And the config we expect it to result in
const expectedConfig: configUtils.Config = {
version: actionsUtil.getActionVersion(),
analysisKinds: [AnalysisKind.CodeQuality],
languages: [KnownLanguage.actions],
buildMode: undefined,
Expand Down Expand Up @@ -273,6 +274,55 @@ test("loading config saves config", async (t) => {
});
});

test("loading config with version mismatch throws", async (t) => {
return await withTmpDir(async (tempDir) => {
const logger = getRunnerLogger(true);

const codeql = createStubCodeQL({
async betterResolveLanguages() {
return {
extractors: {
javascript: [{ extractor_root: "" }],
python: [{ extractor_root: "" }],
},
};
},
});

// Sanity check the saved config file does not already exist
t.false(fs.existsSync(configUtils.getPathToParsedConfigFile(tempDir)));

// Sanity check that getConfig returns undefined before we have called initConfig
t.deepEqual(await configUtils.getConfig(tempDir, logger), undefined);

// Stub `getActionVersion` to return some nonsense.
const getActionVersionStub = sinon
.stub(actionsUtil, "getActionVersion")
.returns("does-not-exist");

await configUtils.initConfig(
createTestInitConfigInputs({
languagesInput: "javascript,python",
tempDir,
codeql,
workspacePath: tempDir,
logger,
}),
);

// Restore `getActionVersion`.
getActionVersionStub.restore();

// The saved config file should now exist
t.true(fs.existsSync(configUtils.getPathToParsedConfigFile(tempDir)));

// Trying to read the configuration should now throw an error.
await t.throwsAsync(configUtils.getConfig(tempDir, logger), {
instanceOf: ConfigurationError,
});
});
});

test("load input outside of workspace", async (t) => {
return await withTmpDir(async (tempDir) => {
try {
Expand Down Expand Up @@ -389,6 +439,7 @@ test("load non-empty input", async (t) => {

// And the config we expect it to parse to
const expectedConfig: configUtils.Config = {
version: actionsUtil.getActionVersion(),
analysisKinds: [AnalysisKind.CodeScanning],
languages: [KnownLanguage.javascript],
buildMode: BuildMode.None,
Expand Down
23 changes: 21 additions & 2 deletions src/config-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { performance } from "perf_hooks";
import * as yaml from "js-yaml";
import * as semver from "semver";

import { isAnalyzingPullRequest } from "./actions-util";
import { getActionVersion, isAnalyzingPullRequest } from "./actions-util";
import {
AnalysisConfig,
AnalysisKind,
Expand Down Expand Up @@ -102,6 +102,10 @@ interface IncludeQueryFilter {
* Format of the parsed config file.
*/
export interface Config {
/**
* The version of the CodeQL Action that the configuration is for.
*/
version: string;
/**
* Set of analysis kinds that are enabled.
*/
Expand Down Expand Up @@ -591,6 +595,7 @@ export async function initActionState(
);

return {
version: getActionVersion(),
analysisKinds,
languages,
buildMode,
Expand Down Expand Up @@ -1308,7 +1313,21 @@ export async function getConfig(
const configString = fs.readFileSync(configFile, "utf8");
logger.debug("Loaded config:");
logger.debug(configString);
return JSON.parse(configString) as Config;

const config = JSON.parse(configString) as Partial<Config>;

if (config.version === undefined) {
throw new ConfigurationError(
`Loaded configuration file, but it does not contain the expected 'version' field.`,
);
}
if (config.version !== getActionVersion()) {
throw new ConfigurationError(
`Loaded a configuration file for version '${config.version}', but running version '${getActionVersion()}'`,
);
Comment on lines +1324 to +1327
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different message than the one produced by /init in the other PR. But this is acceptable, we don't really expect to hit this case once /init has been used in the wild for a while.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense for the messages to be different. In the other PR, it's a warning only and it is based on the ref used for the Action. Here, it's an error and based on the CodeQL Action version. In theory, the same CodeQL Action version can be associated with multiple different commits, so this isn't a perfect safeguard against mixing different commits in a workflow, and we may want to revisit that if we think it's enough of a problem.

In any case, since the conditions aren't the same, we probably want these to be different messages.

}

return config as Config;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/testing-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { TestFn } from "ava";
import nock from "nock";
import * as sinon from "sinon";

import { getActionVersion } from "./actions-util";
import { AnalysisKind } from "./analyses";
import * as apiClient from "./api-client";
import { GitHubApiDetails } from "./api-client";
Expand Down Expand Up @@ -356,6 +357,7 @@ export function createTestConfig(overrides: Partial<Config>): Config {
return Object.assign(
{},
{
version: getActionVersion(),
analysisKinds: [AnalysisKind.CodeScanning],
languages: [],
buildMode: undefined,
Expand Down
Loading