diff --git a/lib/entry-points.js b/lib/entry-points.js index 347343d0ed..33accb5872 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -148296,6 +148296,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()) { @@ -148309,30 +148328,22 @@ 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 @@ -149223,15 +149234,37 @@ async function downloadCacheWithTime(codeQL, languages, logger) { return { trapCaches, trapCacheDownloadTime }; } async function loadUserConfig(actionState, configFile, workspacePath, apiDetails, tempDir) { - if (isLocal(configFile)) { - if (configFile !== userConfigFromActionPath(tempDir)) { - configFile = path10.resolve(workspacePath, configFile); - if (!(configFile + path10.sep).startsWith(workspacePath + path10.sep)) { - throw new ConfigurationError( - getConfigFileOutsideWorkspaceErrorMessage(configFile) - ); + const generatedConfigFile = userConfigFromActionPath(tempDir); + const isWorkspaceRelative = (fp) => (fp + path10.sep).startsWith(workspacePath + path10.sep); + let localFile = false; + if (configFile === generatedConfigFile) { + localFile = true; + } else if (isLocal(configFile)) { + const localFilePath = path10.resolve(workspacePath, configFile); + const workspaceRelative = isWorkspaceRelative(localFilePath); + if (workspaceRelative && fs9.statSync(localFilePath, { throwIfNoEntry: false })?.isFile()) { + configFile = localFilePath; + localFile = true; + } else { + const allowNewFormat = await actionState.features.getValue( + "new_remote_file_addresses" /* NewRemoteFileAddresses */ + ); + if (isRelativePath(configFile) || !allowNewFormat) { + if (workspaceRelative) { + throw new ConfigurationError( + getConfigFileDoesNotExistErrorMessage(localFilePath) + ); + } else { + throw new ConfigurationError( + getConfigFileOutsideWorkspaceErrorMessage( + localFilePath + ) + ); + } } } + } + if (localFile) { const validateConfig = await actionState.features.getValue( "validate_db_config" /* ValidateDbConfig */ ); @@ -149696,18 +149729,19 @@ async function initConfig(actionState, inputs) { await setCppTrapCachingEnvironmentVariables(config, logger); return config; } +function isRelativePath(configPath) { + return configPath.startsWith("./"); +} +function containsAtRef(configPath) { + return configPath.includes("@"); +} function isLocal(configPath) { - if (configPath.indexOf("./") === 0) { + if (isRelativePath(configPath)) { return true; } - return configPath.indexOf("@") === -1; + return !containsAtRef(configPath); } function getLocalConfig(logger, configFile, validateConfig) { - if (!fs9.existsSync(configFile)) { - throw new ConfigurationError( - getConfigFileDoesNotExistErrorMessage(configFile) - ); - } return parseUserConfig( logger, configFile, diff --git a/src/config-utils.test.ts b/src/config-utils.test.ts index 377a1efe98..6d7d2956a7 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, + setupBaseActionsVars, } from "./testing-utils"; import { GitHubVariant, @@ -440,6 +444,7 @@ test.serial("load non-existent input", async (t) => { try { const state = initAllState(); + setupBaseActionsVars({}, state.env); await configUtils.initConfig( state, createTestInitConfigInputs({ @@ -2529,3 +2534,120 @@ 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({}); + const target = callee(configUtils.loadUserConfig) + .withDefaultActionsEnv() + .withFeatures([Feature.NewRemoteFileAddresses]); + + const testTargetWith = async (remoteAddress: string) => { + getRemoteConfig.resetHistory(); + + await target + .withArgs(remoteAddress, tmpDir, SAMPLE_DOTCOM_API_DETAILS, tmpDir) + .passes(t.deepEqual, {}); + + t.true( + getRemoteConfig.calledOnceWithExactly( + sinon.match.any, + remoteAddress, + SAMPLE_DOTCOM_API_DETAILS, + ), + ); + }; + + // When the new format FF is enabled, all inputs that don't explicitly refer + // to a local file that can be found will be tried as remote addresses. + await testTargetWith("repo:file"); + await testTargetWith("input"); + await testTargetWith("../input"); + await testTargetWith("repo@main"); + + // An explicitly local path can still override this. + const explicitlyLocalAddress = "./repo@main"; + + getRemoteConfig.resetHistory(); + + await target + .withArgs( + explicitlyLocalAddress, + tmpDir, + SAMPLE_DOTCOM_API_DETAILS, + tmpDir, + ) + .throws(t); + + t.is(getRemoteConfig.callCount, 0); + }); + }, +); diff --git a/src/config-utils.ts b/src/config-utils.ts index b48d5c1d7c..015d5d24da 100644 --- a/src/config-utils.ts +++ b/src/config-utils.ts @@ -478,24 +478,70 @@ 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)) { - 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); - // Error if the config file is now outside of the workspace - if (!(configFile + path.sep).startsWith(workspacePath + path.sep)) { - throw new ConfigurationError( - errorMessages.getConfigFileOutsideWorkspaceErrorMessage(configFile), - ); + // Compute the path of the generated configuration file. + const generatedConfigFile = userConfigFromActionPath(tempDir); + + // A helper function to decide if the given path is relative to the workspacePath. + const isWorkspaceRelative = (fp: string) => + (fp + path.sep).startsWith(workspacePath + path.sep); + + // We must decide if `configFile` refers to a local or remote file. Using the old remote file + // address format, an address referred to a remote file if it contained '@' somewhere. The + // new format makes all components, except for the repo name optional, and (in theory) a + // repo name could be something like `foo.yml`. That creates ambiguity over whether a + // file is local or not. + let localFile = false; + + if (configFile === generatedConfigFile) { + // The generated file is always local. + localFile = true; + } else if (isLocal(configFile)) { + // If the config file is not generated by the Action, it should be relative to the workspace. + const localFilePath = path.resolve(workspacePath, configFile); + + // Determine whether the resulting path is relative to the workspace. + const workspaceRelative = isWorkspaceRelative(localFilePath); + + // If the path is relative to the workspace and the file exists, then we use it. + if ( + workspaceRelative && + fs.statSync(localFilePath, { throwIfNoEntry: false })?.isFile() + ) { + configFile = localFilePath; + localFile = true; + } else { + const allowNewFormat = await actionState.features.getValue( + Feature.NewRemoteFileAddresses, + ); + + // If the FF for the new format is not enabled or the input path is explicitly local, + // throw the old errors depending on whether the file is outside of the workspace or not. + // Otherwise, we assume the path refers to a remote file. + if (isRelativePath(configFile) || !allowNewFormat) { + if (workspaceRelative) { + throw new ConfigurationError( + errorMessages.getConfigFileDoesNotExistErrorMessage(localFilePath), + ); + } else { + throw new ConfigurationError( + errorMessages.getConfigFileOutsideWorkspaceErrorMessage( + localFilePath, + ), + ); + } } } + } + + // Try to load the file according to whether it is local or not. + if (localFile) { const validateConfig = await actionState.features.getValue( Feature.ValidateDbConfig, ); @@ -1277,13 +1323,43 @@ export async function initConfig( return config; } +/** + * Determines if `configPath` is explicitly local. That is, it starts with "./". + * A configuration file path that starts with "./" is always treated as a local path. + * + * @param configPath The path to test. + */ +function isRelativePath(configPath: string): boolean { + return configPath.startsWith("./"); +} + +/** + * 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. + * This assumes the `OLD_REMOTE_ADDRESS_FORMAT` which must contain a '@' + * character for remote addresses. + * + * @param configPath The path to test. + * @returns True if it is local, or false otherwise. + */ function isLocal(configPath: string): boolean { - // If the path starts with ./, look locally - if (configPath.indexOf("./") === 0) { + // If the path starts with "./", it is explicitly local. + // This allows local paths that would otherwise contain '@' + // to be used with a "./" prefix. + if (isRelativePath(configPath)) { return true; } - return configPath.indexOf("@") === -1; + // Otherwise, the path is also local if it does not contain '@'. + return !containsAtRef(configPath); } export function getLocalConfig( @@ -1291,13 +1367,6 @@ export function getLocalConfig( configFile: string, validateConfig: boolean, ): UserConfig { - // Error if the file does not exist - if (!fs.existsSync(configFile)) { - throw new ConfigurationError( - errorMessages.getConfigFileDoesNotExistErrorMessage(configFile), - ); - } - return parseUserConfig( logger, configFile, 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; }