From d264bca304fd9e2693fd827de139ce5487cbafd8 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 5 Sep 2023 13:28:53 -0700 Subject: [PATCH 1/3] Copy more files into the synthetic variant analysis pack Before this change and starting with CLI v2.14.3, if you wanted to run a variant analysis query and the pack it is contained in has at least one query that contains an extensible predicate, this would be an error. The reason is that v2.14.3 introduced deep validation for data extensions. We are not copying the query that contains an extensible predicate to the synthetic pack we are generating. This means that deep validation will fail because there will be extensions that target the missing extensible predicate. This change avoids the problem by copying any query files that contain extensible predicates to the synthetic pack. It uses the internal `generate extensible-predicate-metadata` command to discover which query files contain extensible predicates and copies them. --- extensions/ql-vscode/src/codeql-cli/cli.ts | 34 ++++++++++++ .../src/variant-analysis/run-remote-query.ts | 16 ++++++ .../variant-analysis-manager.test.ts | 54 +++++++++++++++++-- 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/src/codeql-cli/cli.ts b/extensions/ql-vscode/src/codeql-cli/cli.ts index 9be76e11bed..d3cf0700910 100644 --- a/extensions/ql-vscode/src/codeql-cli/cli.ts +++ b/extensions/ql-vscode/src/codeql-cli/cli.ts @@ -123,6 +123,15 @@ export type ResolveExtensionsResult = { }; }; +export type GenerateExtensiblePredicateMetadataResult = { + // There are other properties in this object, but they are + // not relevant for its use in the extension, so we omit them. + extensible_predicates: Array<{ + // pack relative path + path: string; + }>; +}; + /** * The expected output of `codeql resolve qlref`. */ @@ -1458,6 +1467,17 @@ export class CodeQLCliServer implements Disposable { ); } + async generateExtensiblePredicateMetadata( + packRoot: string, + ): Promise { + return await this.runJsonCodeQlCliCommand( + ["generate", "extensible-predicate-metadata"], + [packRoot], + "Generating extensible predicate metadata", + { addFormat: false }, + ); + } + public async getVersion() { if (!this._version) { try { @@ -1830,6 +1850,14 @@ export class CliVersionConstraint { */ public static CLI_VERSION_WITH_QUICK_EVAL_COUNT = new SemVer("2.13.3"); + /** + * CLI version where the `generate extensible-predicate-metadata` + * command was implemented. + */ + public static CLI_VERSION_WITH_EXTENSIBLE_PREDICATE_METADATA = new SemVer( + "2.14.3", + ); + /** * CLI version where the langauge server supports visisbility change notifications. */ @@ -1916,4 +1944,10 @@ export class CliVersionConstraint { CliVersionConstraint.CLI_VERSION_WITH_QUICK_EVAL_COUNT, ); } + + async supportsGenerateExtensiblePredicateMetadata() { + return this.isVersionAtLeast( + CliVersionConstraint.CLI_VERSION_WITH_EXTENSIBLE_PREDICATE_METADATA, + ); + } } diff --git a/extensions/ql-vscode/src/variant-analysis/run-remote-query.ts b/extensions/ql-vscode/src/variant-analysis/run-remote-query.ts index ead06f3c3ce..0d07e08df9a 100644 --- a/extensions/ql-vscode/src/variant-analysis/run-remote-query.ts +++ b/extensions/ql-vscode/src/variant-analysis/run-remote-query.ts @@ -189,6 +189,22 @@ async function copyExistingQueryPack( ) { const toCopy = await cliServer.packPacklist(originalPackRoot, false); + // Also include query files that contain extensible predicates. These query files are not + // needed for the query to run, but they are needed for the query pack to pass deep validation + // of data extensions. + if ( + await cliServer.cliConstraints.supportsGenerateExtensiblePredicateMetadata() + ) { + const metadata = await cliServer.generateExtensiblePredicateMetadata( + originalPackRoot, + ); + metadata.extensible_predicates.forEach((predicate) => { + if (predicate.path.endsWith(".ql")) { + toCopy.push(join(originalPackRoot, predicate.path)); + } + }); + } + [ // also copy the lock file (either new name or old name) and the query file itself. These are not included in the packlist. ...QLPACK_LOCK_FILENAMES.map((f) => join(originalPackRoot, f)), diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts index c27c1d38961..d80836304d9 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts @@ -2,7 +2,7 @@ import { CancellationTokenSource, commands, Uri, window } from "vscode"; import { extLogger } from "../../../../src/common/logging/vscode"; import { setRemoteControllerRepo } from "../../../../src/config"; import * as ghApiClient from "../../../../src/variant-analysis/gh-api/gh-api-client"; -import { join } from "path"; +import { isAbsolute, join } from "path"; import { VariantAnalysisManager } from "../../../../src/variant-analysis/variant-analysis-manager"; import { @@ -275,13 +275,52 @@ describe("Variant Analysis Manager", () => { }); }); + // Test running core java queries to ensure that we can compile queries in packs + // that contain queries with extensible predicates + it("should run a remote query that is part of the java pack", async () => { + if ( + !(await cli.cliConstraints.supportsGenerateExtensiblePredicateMetadata()) + ) { + console.log( + `Skipping test because generating extensible predicate metadata was only introduced in CLI version ${CliVersionConstraint.CLI_VERSION_WITH_EXTENSIBLE_PREDICATE_METADATA}.`, + ); + return; + } + + if (!process.env.TEST_CODEQL_PATH) { + fail( + "TEST_CODEQL_PATH environment variable not set. It should point to the absolute path to a checkout of the codeql repository.", + ); + } + + const queryToRun = + "Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql"; + const extraQuery = "Telemetry/ExtractorInformation.ql"; + + await doVariantAnalysisTest({ + queryPath: join( + process.env.TEST_CODEQL_PATH, + "java/ql/src", + queryToRun, + ), + expectedPackName: "codeql/java-queries", + filesThatExist: [queryToRun, extraQuery], + filesThatDoNotExist: [], + qlxFilesThatExist: [], + dependenciesToCheck: ["codeql/java-all"], + // Don't check the version since it will be the same version + checkVersion: false, + }); + }); + async function doVariantAnalysisTest({ queryPath, expectedPackName, filesThatExist, qlxFilesThatExist, filesThatDoNotExist, - dependenciesToCheck = ["codeql/javascript-all"], + dependenciesToCheck = ["codeql/java-all"], + checkVersion = true, }: { queryPath: string; expectedPackName: string; @@ -289,6 +328,7 @@ describe("Variant Analysis Manager", () => { qlxFilesThatExist: string[]; filesThatDoNotExist: string[]; dependenciesToCheck?: string[]; + checkVersion?: boolean; }) { const fileUri = getFile(queryPath); await variantAnalysisManager.runVariantAnalysis( @@ -339,7 +379,9 @@ describe("Variant Analysis Manager", () => { packFS.fileContents(packFileName).toString("utf-8"), ); expect(qlpackContents.name).toEqual(expectedPackName); - expect(qlpackContents.version).toEqual("0.0.0"); + if (checkVersion) { + expect(qlpackContents.version).toEqual("0.0.0"); + } expect(qlpackContents.dependencies?.["codeql/javascript-all"]).toEqual( "*", ); @@ -357,7 +399,11 @@ describe("Variant Analysis Manager", () => { } function getFile(file: string): Uri { - return Uri.file(join(baseDir, file)); + if (isAbsolute(file)) { + return Uri.file(file); + } else { + return Uri.file(join(baseDir, file)); + } } }); }); From 767051e43fbb1edd6996efb2cc7def6a8906068b Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 5 Sep 2023 13:31:18 -0700 Subject: [PATCH 2/3] Update changelog --- extensions/ql-vscode/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/ql-vscode/CHANGELOG.md b/extensions/ql-vscode/CHANGELOG.md index b76951bdd72..d8f63562fa0 100644 --- a/extensions/ql-vscode/CHANGELOG.md +++ b/extensions/ql-vscode/CHANGELOG.md @@ -5,6 +5,7 @@ - Update how variant analysis results are displayed. For queries with ["path-problem" or "problem" `@kind`](https://codeql.github.com/docs/writing-codeql-queries/metadata-for-codeql-queries/#metadata-properties), you can choose to display the results as rendered alerts or as a table of raw results. For queries with any other `@kind`, the results are displayed as a table. [#2745](https://github.com/github/vscode-codeql/pull/2745) & [#2749](https://github.com/github/vscode-codeql/pull/2749) - When running variant analyses, don't download artifacts for repositories with no results. [#2736](https://github.com/github/vscode-codeql/pull/2736) - Group the extension settings, so that they're easier to find in the Settings UI. [#2706](https://github.com/github/vscode-codeql/pull/2706) +- Fix a bug where variant analysis queries would fail for queries in the `codeql/java-queries` query pack. [#2786](https://github.com/github/vscode-codeql/pull/2786) ## 1.8.10 - 15 August 2023 From d8fb227cc757f92cf2945d0161aacb327cd5666a Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Tue, 5 Sep 2023 14:00:57 -0700 Subject: [PATCH 3/3] Fix failing test --- .../variant-analysis-manager.test.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts b/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts index d80836304d9..e427bfb23d1 100644 --- a/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts @@ -319,7 +319,10 @@ describe("Variant Analysis Manager", () => { filesThatExist, qlxFilesThatExist, filesThatDoNotExist, - dependenciesToCheck = ["codeql/java-all"], + + // A subset of dependencies that we expect should be in the qlpack file. + // The first dependency is assumed to be the core library. + dependenciesToCheck = ["codeql/javascript-all"], checkVersion = true, }: { queryPath: string; @@ -382,10 +385,13 @@ describe("Variant Analysis Manager", () => { if (checkVersion) { expect(qlpackContents.version).toEqual("0.0.0"); } - expect(qlpackContents.dependencies?.["codeql/javascript-all"]).toEqual( - "*", - ); + // Assume the first dependency to check is the core library. + if (dependenciesToCheck.length > 0) { + expect(qlpackContents.dependencies?.[dependenciesToCheck[0]]).toEqual( + "*", + ); + } const qlpackLockContents = load( packFS.fileContents("codeql-pack.lock.yml").toString("utf-8"), );