diff --git a/lib/entry-points.js b/lib/entry-points.js index c8136097ee..fca6ae2f04 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -148311,6 +148311,25 @@ function parseOldRemoteFileAddress(input) { ref: pieces.groups.ref.trim() }); } +function parseNewRemoteFileAddress(env, configFile) { + const format = new RegExp( + "^((?[^:@/]+)/)?(?[^:@/]+)(@(?[^:]+))?(:(?.+))?$" + ); + const pieces = format.exec(configFile.trim()); + const repo = pieces?.groups?.repo?.trim(); + if (!pieces?.groups || !repo || repo.length === 0) { + return new Failure(void 0); + } + const owner = pieces.groups.owner?.trim(); + const path29 = pieces.groups.path?.trim(); + const ref = pieces.groups.ref?.trim(); + return new Success({ + owner: owner || getDefaultOwner(env), + repo, + path: path29 || DEFAULT_CONFIG_FILE_NAME, + ref: ref || DEFAULT_CONFIG_FILE_REF + }); +} async function parseRemoteFileAddress(actionState, configFile) { const oldFormatAddressResult = parseOldRemoteFileAddress(configFile); if (oldFormatAddressResult.isSuccess()) { @@ -148324,33 +148343,27 @@ async function parseRemoteFileAddress(actionState, configFile) { getConfigFileRepoOldFormatInvalidMessage(configFile) ); } - const format = new RegExp( - "^((?[^:@/]+)/)?(?[^:@/]+)(@(?[^:]+))?(:(?.+))?$" + const newFormatAddressResult = parseNewRemoteFileAddress( + actionState.env, + configFile ); - const pieces = format.exec(configFile.trim()); - const repo = pieces?.groups?.repo?.trim(); - if (!pieces?.groups || !repo || repo.length === 0) { + if (newFormatAddressResult.isFailure()) { throw new ConfigurationError( getConfigFileRepoFormatInvalidMessage(configFile) ); } - const owner = pieces.groups.owner?.trim(); - const path29 = pieces.groups.path?.trim(); - const ref = pieces.groups.ref?.trim(); - if (path29?.startsWith("/")) { + const address = newFormatAddressResult.value; + if (address.path.startsWith("/")) { throw new ConfigurationError( `The path component of '${configFile}' cannot be an absolute path.` ); } - return { - owner: owner || getDefaultOwner(actionState.env), - repo, - path: path29 || DEFAULT_CONFIG_FILE_NAME, - ref: ref || DEFAULT_CONFIG_FILE_REF - }; + return address; } // src/config/file.ts +var LOCAL_PATH_PREFIX = "./"; +var REMOTE_PATH_PREFIX = "remote="; async function getConfigFileInput({ logger, actions, @@ -149238,7 +149251,10 @@ async function downloadCacheWithTime(codeQL, languages, logger) { return { trapCaches, trapCacheDownloadTime }; } async function loadUserConfig(actionState, configFile, workspacePath, apiDetails, tempDir) { - if (isLocal(configFile)) { + const allowNewFormat = await actionState.features.getValue( + "new_remote_file_addresses" /* NewRemoteFileAddresses */ + ); + if (isLocal(configFile, allowNewFormat)) { if (configFile !== userConfigFromActionPath(tempDir)) { configFile = path10.resolve(workspacePath, configFile); if (!(configFile + path10.sep).startsWith(workspacePath + path10.sep)) { @@ -149252,6 +149268,9 @@ async function loadUserConfig(actionState, configFile, workspacePath, apiDetails ); return getLocalConfig(actionState.logger, configFile, validateConfig); } else { + if (allowNewFormat && isExplicitRemotePath(configFile)) { + configFile = configFile.substring(REMOTE_PATH_PREFIX.length); + } return await getRemoteConfig(actionState, configFile, apiDetails); } } @@ -149711,11 +149730,23 @@ async function initConfig(actionState, inputs) { await setCppTrapCachingEnvironmentVariables(config, logger); return config; } -function isLocal(configPath) { - if (configPath.indexOf("./") === 0) { +function isExplicitLocalPath(configPath) { + return configPath.startsWith(LOCAL_PATH_PREFIX); +} +function isExplicitRemotePath(configPath) { + return configPath.startsWith(REMOTE_PATH_PREFIX); +} +function containsAtRef(configPath) { + return configPath.includes("@"); +} +function isLocal(configPath, allowNewFormat) { + if (isExplicitLocalPath(configPath)) { return true; } - return configPath.indexOf("@") === -1; + if (allowNewFormat && isExplicitRemotePath(configPath)) { + return false; + } + return !containsAtRef(configPath); } function getLocalConfig(logger, configFile, validateConfig) { if (!fs9.existsSync(configFile)) { diff --git a/src/config-utils.test.ts b/src/config-utils.test.ts index 377a1efe98..4a6fa739e2 100644 --- a/src/config-utils.test.ts +++ b/src/config-utils.test.ts @@ -6,12 +6,14 @@ import test, { ExecutionContext } from "ava"; import * as yaml from "js-yaml"; import * as sinon from "sinon"; +import { ActionState } from "./action-common"; import * as actionsUtil from "./actions-util"; import { AnalysisKind, supportedAnalysisKinds } from "./analyses"; import * as api from "./api-client"; import { CachingKind } from "./caching-utils"; import { createStubCodeQL } from "./codeql"; import { UserConfig } from "./config/db-config"; +import * as file from "./config/file"; import * as configUtils from "./config-utils"; import * as errorMessages from "./error-messages"; import { Feature } from "./feature-flags"; @@ -38,6 +40,8 @@ import { makeMacro, initAllState, callee, + SAMPLE_DOTCOM_API_DETAILS, + AssertableTarget, } from "./testing-utils"; import { GitHubVariant, @@ -2529,3 +2533,177 @@ test("determineUserConfig - ignores config file input outside Default Setup if F }); }); }); + +test("loadUserConfig - loads local configuration files", async (t) => { + await withTmpDir(async (workspaceDir) => { + await withTmpDir(async (tmpDir) => { + // Construct the test target. + const loadUserConfig = ( + actionState: ActionState<["Logger", "Env", "FeatureFlags"]>, + filePath: string, + ) => + configUtils.loadUserConfig( + actionState, + filePath, + workspaceDir, + SAMPLE_DOTCOM_API_DETAILS, + tmpDir, + ); + const target = callee(loadUserConfig); + + // `loadUserConfig` should load local configuration files if they are inside the workspace: + const insideOfWorkspace = path.join(workspaceDir, "some-file.yml"); + fs.writeFileSync(insideOfWorkspace, "test-key: present", "utf8"); + + await target + .withArgs(insideOfWorkspace) + .passes(t.deepEqual, { "test-key": "present" }); + + // `loadUserConfig` should normally throw if the path is outside of the workspace: + const outsideOfWorkspace = path.join( + tmpDir, + "not-the-generated-file.yml", + ); + fs.writeFileSync(outsideOfWorkspace, "test-key: present", "utf8"); + + await target + .withArgs(outsideOfWorkspace) + .throws(t, { instanceOf: ConfigurationError }); + + // `loadUserConfig` does not throw if the path is the result of `userConfigFromActionPath`: + const generatedPath = configUtils.userConfigFromActionPath(tmpDir); + fs.writeFileSync(generatedPath, "test-key: present", "utf8"); + + await target + .withArgs(generatedPath) + .passes(t.deepEqual, { "test-key": "present" }); + }); + }); +}); + +test.serial("loadUserConfig - loads remote configuration files", async (t) => { + await withTmpDir(async (tmpDir) => { + const getRemoteConfig = sinon.stub(file, "getRemoteConfig").resolves({}); + + const remoteAddress = "owner/repo/file@ref"; + await callee(configUtils.loadUserConfig) + .withArgs(remoteAddress, tmpDir, SAMPLE_DOTCOM_API_DETAILS, tmpDir) + .passes(t.deepEqual, {}); + + t.true( + getRemoteConfig.calledOnceWithExactly( + sinon.match.any, + remoteAddress, + SAMPLE_DOTCOM_API_DETAILS, + ), + ); + }); +}); + +test.serial( + "loadUserConfig - loads remote configuration files (new format, partial)", + async (t) => { + await withTmpDir(async (tmpDir) => { + const getRemoteConfig = sinon.stub(file, "getRemoteConfig").resolves({}); + + // Construct the basic test target. + const target = callee(configUtils.loadUserConfig) + .withDefaultActionsEnv() + .withFeatures([Feature.NewRemoteFileAddresses]); + + // Utility function to assert that `targetWithArgs` has identified + // the input as a remote file address. + const checkIsRemote = + (address: string) => + async (targetWithArgs: AssertableTarget) => { + // We have stubbed `getRemoteConfig` to resolve to `{}`, so we + // expect that result. + await targetWithArgs.passes(t.deepEqual, {}); + + // And `getRemoteConfig` should have been called exactly once. + t.is(getRemoteConfig.callCount, 1); + + // Get the arguments for the call and check that there were three. + // We don't care about the first, but check that the other two + // match our expectations. We break it down like this to get + // more useful test output. + const args = getRemoteConfig.getCalls()[0].args; + t.is(args.length, 3); + t.deepEqual(args[1], address); + t.deepEqual(args[2], SAMPLE_DOTCOM_API_DETAILS); + }; + + // Utility function to assert that `targetWithArgs` has not identified + // the input as a remote file address. + const checkIsNotRemote = async ( + targetWithArgs: AssertableTarget, + ) => { + // We expect `loadUserConfig` to have thrown if it thinks the path is local, + // since the inputs we provide aren't for files that exist. + await targetWithArgs.throws(t); + + // Additionally, we expect that `getRemoteConfig` wasn't called. + t.is(getRemoteConfig.callCount, 0); + }; + + // Utility function to add the explicit `REMOTE_PATH_PREFIX` to the input. + const withExplicitPrefix = (str: string) => + `${file.REMOTE_PATH_PREFIX}${str}`; + + // Utility to set up a call to `loadUserConfig` with the provided `address` + // and pass it to `assertion`. + const testTargetWith = async ( + address: string, + assertion: ( + targetWithArgs: AssertableTarget>, + ) => Promise, + ) => { + // Reset the stub's history since we re-use it. + getRemoteConfig.resetHistory(); + + // Log the input we are testing so that, in the event of a failure, + // it is easier to see which input was responsible. + t.log(`testTargetWith("${address}")`); + + // Prepare the test call to `loadUserConfig`. + const targetWithArgs = target.withArgs( + address, + tmpDir, + SAMPLE_DOTCOM_API_DETAILS, + tmpDir, + ); + + // Pass it to the provided assertion function. + await assertion(targetWithArgs); + }; + + // Since this input contains an '@' character, it is treated as a remote path + // by the old logic even without the explicit prefix. + const remoteWithoutPrefix = "repo@main"; + await testTargetWith( + remoteWithoutPrefix, + checkIsRemote(remoteWithoutPrefix), + ); + await testTargetWith( + withExplicitPrefix(remoteWithoutPrefix), + checkIsRemote(remoteWithoutPrefix), + ); + // It is only treated as a local path with the corresponding prefix. + await testTargetWith(`./${remoteWithoutPrefix}`, checkIsNotRemote); + + // The following test inputs are examples of ambiguous paths. They could refer to + // valid local or remote paths. For each, we check that they are treated as remote + // paths if the explicit remote file prefix is used and as local paths otherwise. + const testInputs = ["repo:file", "input", "../input"]; + + for (const testInput of testInputs) { + for (const addPrefix of [true, false]) { + await testTargetWith( + addPrefix ? withExplicitPrefix(testInput) : testInput, + addPrefix ? checkIsRemote(testInput) : checkIsNotRemote, + ); + } + } + }); + }, +); diff --git a/src/config-utils.ts b/src/config-utils.ts index b48d5c1d7c..948494f531 100644 --- a/src/config-utils.ts +++ b/src/config-utils.ts @@ -31,7 +31,11 @@ import { parseUserConfig, UserConfig, } from "./config/db-config"; -import { getRemoteConfig } from "./config/file"; +import { + getRemoteConfig, + LOCAL_PATH_PREFIX, + REMOTE_PATH_PREFIX, +} from "./config/file"; import { parseRegistries, type RegistryConfigNoCredentials, @@ -478,14 +482,18 @@ async function downloadCacheWithTime( * @param tempDir The temporary directory which may contain a CodeQL Action-generated configuration file. * @returns The loaded configuration file, if successful. */ -async function loadUserConfig( +export async function loadUserConfig( actionState: ActionState<["Logger", "Env", "FeatureFlags"]>, configFile: string, workspacePath: string, apiDetails: api.GitHubApiCombinedDetails, tempDir: string, ): Promise { - if (isLocal(configFile)) { + const allowNewFormat = await actionState.features.getValue( + Feature.NewRemoteFileAddresses, + ); + + if (isLocal(configFile, allowNewFormat)) { if (configFile !== userConfigFromActionPath(tempDir)) { // If the config file is not generated by the Action, it should be relative to the workspace. configFile = path.resolve(workspacePath, configFile); @@ -501,6 +509,12 @@ async function loadUserConfig( ); return getLocalConfig(actionState.logger, configFile, validateConfig); } else { + // Drop the explicit prefix if it is present. Since `REMOTE_PATH_PREFIX` is chosen + // to not conflict with permissible characters in "owner" or "repo" components, + // this does not risk removing valid parts of either component by accident. + if (allowNewFormat && isExplicitRemotePath(configFile)) { + configFile = configFile.substring(REMOTE_PATH_PREFIX.length); + } return await getRemoteConfig(actionState, configFile, apiDetails); } } @@ -1277,13 +1291,58 @@ export async function initConfig( return config; } -function isLocal(configPath: string): boolean { - // If the path starts with ./, look locally - if (configPath.indexOf("./") === 0) { +/** + * Determines if `configPath` is explicitly local. That is, it starts with `LOCAL_PATH_PREFIX`. + * A configuration file path that starts with `LOCAL_PATH_PREFIX` is always treated as a local path. + * + * @param configPath The path to test. + */ +function isExplicitLocalPath(configPath: string): boolean { + return configPath.startsWith(LOCAL_PATH_PREFIX); +} + +/** + * Determines if `configPath` starts with the prefix used to explicitly mark a path + * as a remote path (`REMOTE_PATH_PREFIX`). + * + * @param configPath The path to test. + */ +function isExplicitRemotePath(configPath: string): boolean { + return configPath.startsWith(REMOTE_PATH_PREFIX); +} + +/** + * Determines if `configPath` contains a '@' character. + * + * @param configPath The path to test. + */ +function containsAtRef(configPath: string): boolean { + return configPath.includes("@"); +} + +/** + * Determines if `configPath` refers to a local configuration file. + * + * @param configPath The path to test. + * @returns True if it is local, or false otherwise. + */ +function isLocal(configPath: string, allowNewFormat: boolean): boolean { + // If the path starts with `LOCAL_PATH_PREFIX`, it is explicitly local. + // This allows local paths that would otherwise contain '@' + // to be used with a `LOCAL_PATH_PREFIX` prefix. + if (isExplicitLocalPath(configPath)) { return true; } + // If the path starts with `REMOTE_PATH_PREFIX`, it is explicitly remote. + // This allows users to resolve ambiguity by specifying `REMOTE_PATH_PREFIX`. + if (allowNewFormat && isExplicitRemotePath(configPath)) { + return false; + } - return configPath.indexOf("@") === -1; + // Otherwise, the path is also local if it does not contain '@'. + // This assumes the `OLD_REMOTE_ADDRESS_FORMAT` which must contain a '@' + // character for remote addresses. + return !containsAtRef(configPath); } export function getLocalConfig( diff --git a/src/config/file.ts b/src/config/file.ts index 5ff2dee082..ee6050cf47 100644 --- a/src/config/file.ts +++ b/src/config/file.ts @@ -11,6 +11,19 @@ import { ConfigurationError } from "../util"; import { parseUserConfig, UserConfig } from "./db-config"; import { parseRemoteFileAddress } from "./remote-file"; +/** + * The prefix that can be specified to indicate that a path should be treated as a local file address. + */ +export const LOCAL_PATH_PREFIX = "./"; + +/** + * The prefix that can be specified to indicate that a path should be treated as a remote file address. + * The new remote file address format must start with either an owner or repository name. Both + * are restricted to ASCII characters, '.', and '-'. The prefix chosen here does not interfere with + * those (since it contains an `=`) and is _unlikely_ (but not impossible) to appear in a local file path. + */ +export const REMOTE_PATH_PREFIX = "remote="; + /** * Gets the value that is configured for the configuration file, if any. */ diff --git a/src/config/remote-file.ts b/src/config/remote-file.ts index 15b34af2b6..897b1e910d 100644 --- a/src/config/remote-file.ts +++ b/src/config/remote-file.ts @@ -1,5 +1,5 @@ import { ActionState } from "../action-common"; -import { Env, ActionsEnvVars } from "../environment"; +import { ActionsEnvVars, ReadOnlyEnv } from "../environment"; import * as errorMessages from "../error-messages"; import { Feature } from "../feature-flags"; import { ConfigurationError, Failure, Result, Success } from "../util"; @@ -23,7 +23,7 @@ export const DEFAULT_CONFIG_FILE_NAME = ".github/codeql-action.yaml"; export const DEFAULT_CONFIG_FILE_REF = "main"; /** Extracts the owner from the `GITHUB_REPOSITORY` environment variable. */ -function getDefaultOwner(env: Env): string { +function getDefaultOwner(env: ReadOnlyEnv): string { const currentRepoNwo = env.getRequired(ActionsEnvVars.GITHUB_REPOSITORY); const nwoParts = currentRepoNwo.split("/"); @@ -70,6 +70,42 @@ function parseOldRemoteFileAddress( }); } +/** + * Attempts to parse `input` as a `RemoteFileAddress` using the new format. + * + * @param env The read-only environment to obtain the owner name from if needed. + * @param configFile The input to try and parse. + * @returns A `RemoteFileAddress` value if successful or `undefined` otherwise. + */ +export function parseNewRemoteFileAddress( + env: ReadOnlyEnv, + configFile: string, +): Result { + // retrieve the various parts of the config location, and ensure they're present + const format = new RegExp( + "^((?[^:@/]+)/)?(?[^:@/]+)(@(?[^:]+))?(:(?.+))?$", + ); + const pieces = format.exec(configFile.trim()); + + const repo: string | undefined = pieces?.groups?.repo?.trim(); + + // Check that the regular expression matched and that we have at least the repo name. + if (!pieces?.groups || !repo || repo.length === 0) { + return new Failure(undefined); + } + + const owner: string | undefined = pieces.groups.owner?.trim(); + const path: string | undefined = pieces.groups.path?.trim(); + const ref: string | undefined = pieces.groups.ref?.trim(); + + return new Success({ + owner: owner || getDefaultOwner(env), + repo, + path: path || DEFAULT_CONFIG_FILE_NAME, + ref: ref || DEFAULT_CONFIG_FILE_REF, + }); +} + /** * Attempts to parse `configFile` into an array of `RemoteFileAddress` components. * @@ -101,15 +137,12 @@ export async function parseRemoteFileAddress( } // retrieve the various parts of the config location, and ensure they're present - const format = new RegExp( - "^((?[^:@/]+)/)?(?[^:@/]+)(@(?[^:]+))?(:(?.+))?$", + const newFormatAddressResult = parseNewRemoteFileAddress( + actionState.env, + configFile, ); - const pieces = format.exec(configFile.trim()); - const repo: string | undefined = pieces?.groups?.repo?.trim(); - - // Check that the regular expression matched and that we have at least the repo name. - if (!pieces?.groups || !repo || repo.length === 0) { + if (newFormatAddressResult.isFailure()) { // Neither the old format nor the new format worked. Throw an error that // explains the format we accept. We only mention the new format, since that's // what we want to be used going forward. @@ -118,21 +151,14 @@ export async function parseRemoteFileAddress( ); } - const owner: string | undefined = pieces.groups.owner?.trim(); - const path: string | undefined = pieces.groups.path?.trim(); - const ref: string | undefined = pieces.groups.ref?.trim(); + const address = newFormatAddressResult.value; // Ensure that the path is a relative path. - if (path?.startsWith("/")) { + if (address.path.startsWith("/")) { throw new ConfigurationError( `The path component of '${configFile}' cannot be an absolute path.`, ); } - return { - owner: owner || getDefaultOwner(actionState.env), - repo, - path: path || DEFAULT_CONFIG_FILE_NAME, - ref: ref || DEFAULT_CONFIG_FILE_REF, - }; + return address; } diff --git a/src/testing-utils.ts b/src/testing-utils.ts index 7030831c39..b348a497c3 100644 --- a/src/testing-utils.ts +++ b/src/testing-utils.ts @@ -371,11 +371,30 @@ export interface PassedAssertion { assertionResult: T; } +/** + * A more minimal, exported interface for `CallableEnvBuilder`. This makes it easier to + * define helper functions in tests which expect a value of a compatible type. + */ +export interface AssertableTarget { + passes( + assertion: (val: Awaited, ...assertionArgs: AArgs) => AResult, + ...assertionArgs: AArgs + ): Promise>; + + throws( + t: ExecutionContext, + expectations?: ThrowsExpectation, + ): Promise>; +} + class CallableEnvBuilder< - Args extends readonly any[], - R, - Fs extends ReadonlyArray, -> extends BaseEnvBuilder { + Args extends readonly any[], + R, + Fs extends ReadonlyArray, + > + extends BaseEnvBuilder + implements AssertableTarget +{ private args: Args; constructor(