Skip to content

Commit

Permalink
fix: distinct ID input was not escaped
Browse files Browse the repository at this point in the history
  • Loading branch information
Codex- committed Oct 5, 2024
1 parent 08325d2 commit 97a8019
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 19 deletions.
12 changes: 11 additions & 1 deletion src/main.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ describe("main", () => {
let utilsLogInfoForBranchNameResult: MockInstance<
typeof utils.logInfoForBranchNameResult
>;
let utilsCreateDistinctIdRegexMock: MockInstance<
typeof utils.createDistinctIdRegex
>;

// Return Dispatch
let returnDispatchGetRunIdAndUrlMock: MockInstance<
Expand Down Expand Up @@ -93,6 +96,7 @@ describe("main", () => {
utils,
"logInfoForBranchNameResult",
);
utilsCreateDistinctIdRegexMock = vi.spyOn(utils, "createDistinctIdRegex");

returnDispatchGetRunIdAndUrlMock = vi.spyOn(
returnDispatch,
Expand All @@ -114,6 +118,7 @@ describe("main", () => {
});

it("should successfully complete", async () => {
const distinctIdRegex = new RegExp(testCfg.distinctId);
const returnDispatchSuccessResult = {
success: true,
value: {
Expand All @@ -123,6 +128,7 @@ describe("main", () => {
} as const;

utilsGetBranchNameMock.mockReturnValue(testBranch);
utilsCreateDistinctIdRegexMock.mockReturnValue(distinctIdRegex);
returnDispatchGetWorkflowIdMock.mockResolvedValue(0);
returnDispatchGetRunIdAndUrlMock.mockResolvedValue(
returnDispatchSuccessResult,
Expand Down Expand Up @@ -154,13 +160,17 @@ describe("main", () => {
testBranch,
testCfg.ref,
);
expect(utilsCreateDistinctIdRegexMock).toHaveBeenCalledOnce();
expect(utilsCreateDistinctIdRegexMock).toHaveBeenCalledWith(
testCfg.distinctId,
);

// Get run ID
expect(returnDispatchGetRunIdAndUrlMock).toHaveBeenCalledOnce();
expect(returnDispatchGetRunIdAndUrlMock).toHaveBeenCalledWith({
startTime: Date.now(),
branch: testBranch,
distinctId: testCfg.distinctId,
distinctIdRegex: distinctIdRegex,
workflow: testCfg.workflow,
workflowId: 0,
workflowTimeoutMs: testCfg.workflowTimeoutSeconds * 1000,
Expand Down
10 changes: 8 additions & 2 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
handleActionSuccess,
getRunIdAndUrl,
} from "./return-dispatch.ts";
import { getBranchName, logInfoForBranchNameResult } from "./utils.ts";
import {
createDistinctIdRegex,
getBranchName,
logInfoForBranchNameResult,
} from "./utils.ts";

export async function main(): Promise<void> {
try {
Expand All @@ -27,10 +31,12 @@ export async function main(): Promise<void> {
const branch = getBranchName(config.ref);
logInfoForBranchNameResult(branch, config.ref);

const distinctIdRegex = createDistinctIdRegex(config.distinctId);

const result = await getRunIdAndUrl({
startTime,
branch,
distinctId: config.distinctId,
distinctIdRegex,
workflow: config.workflow,
workflowId,
workflowTimeoutMs: config.workflowTimeoutSeconds * 1000,
Expand Down
11 changes: 2 additions & 9 deletions src/return-dispatch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,6 @@ import * as utils from "./utils.ts";

vi.mock("@actions/core");
vi.mock("./api.ts");
// vi.mock(import('./utils.ts'), async (importOriginal) => {
// const mod = await importOriginal() // type is inferred
// return {
// ...mod,
// // replace some exports
// total: vi.fn(mod.sleep),
// }
// })

describe("return-dispatch", () => {
const {
Expand Down Expand Up @@ -473,6 +465,7 @@ describe("return-dispatch", () => {

describe("getRunIdAndUrl", () => {
const distinctId = crypto.randomUUID();
const distinctIdRegex = new RegExp(distinctId);
const workflow = "workflow.yml";
const workflowId = 123;
const branch: utils.BranchNameResult = Object.freeze({
Expand All @@ -483,7 +476,7 @@ describe("return-dispatch", () => {
const defaultOpts: GetRunIdAndUrlOpts = {
startTime: Date.now(),
branch: branch,
distinctId: distinctId,
distinctIdRegex: distinctIdRegex,
workflow: workflow,
workflowId: workflowId,
workflowTimeoutMs: 100,
Expand Down
7 changes: 3 additions & 4 deletions src/return-dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ActionOutputs } from "./action.ts";
import * as api from "./api.ts";
import * as constants from "./constants.ts";
import type { Result } from "./types.ts";
import { sleep, type BranchNameResult } from "./utils.ts";
import { escapeRegExp, sleep, type BranchNameResult } from "./utils.ts";

export function shouldRetryOrThrow(
error: Error,
Expand Down Expand Up @@ -126,20 +126,19 @@ export function handleActionFail(): void {
export interface GetRunIdAndUrlOpts {
startTime: number;
branch: BranchNameResult;
distinctId: string;
distinctIdRegex: RegExp;
workflow: string | number;
workflowId: number;
workflowTimeoutMs: number;
}
export async function getRunIdAndUrl({
startTime,
branch,
distinctId,
distinctIdRegex,
workflow,
workflowId,
workflowTimeoutMs,
}: GetRunIdAndUrlOpts): Promise<Result<{ id: number; url: string }>> {
const distinctIdRegex = new RegExp(distinctId);
const retryTimeout = Math.max(
constants.WORKFLOW_FETCH_TIMEOUT_MS,
workflowTimeoutMs,
Expand Down
7 changes: 7 additions & 0 deletions src/test-utils/logging.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { type MockInstance, vi, expect } from "vitest";
interface MockedLoggingFunctions {
coreDebugLogMock: MockInstance<(message: string) => void>;
coreInfoLogMock: MockInstance<(message: string) => void>;
coreWarningLogMock: MockInstance<(message: string) => void>;
coreErrorLogMock: MockInstance<(message: string) => void>;
assertOnlyCalled: (
...coreLogMocks: MockInstance<(message: string) => void>[]
Expand All @@ -22,13 +23,18 @@ export function mockLoggingFunctions(): MockedLoggingFunctions {
const coreInfoLogMock: MockInstance<typeof core.info> = vi
.spyOn(core, "info")
.mockImplementation(() => undefined);
const coreWarningLogMock: MockInstance<typeof core.error> = vi.spyOn(
core,
"warning",
);
const coreErrorLogMock: MockInstance<typeof core.error> = vi
.spyOn(core, "error")
.mockImplementation(() => undefined);

const coreLogMockSet = new Set<MockInstance<(message: string) => void>>([
coreDebugLogMock,
coreInfoLogMock,
coreWarningLogMock,
coreErrorLogMock,
]);
const assertOnlyCalled = (
Expand All @@ -44,6 +50,7 @@ export function mockLoggingFunctions(): MockedLoggingFunctions {
return {
coreDebugLogMock,
coreInfoLogMock,
coreWarningLogMock,
coreErrorLogMock,
assertOnlyCalled,
assertNoneCalled,
Expand Down
65 changes: 62 additions & 3 deletions src/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,24 @@ import {
} from "vitest";

import { mockLoggingFunctions } from "./test-utils/logging.mock.ts";
import { getBranchName, logInfoForBranchNameResult, sleep } from "./utils.ts";
import {
createDistinctIdRegex,
escapeRegExp,
getBranchName,
logInfoForBranchNameResult,
sleep,
} from "./utils.ts";

vi.mock("@actions/core");

describe("utils", () => {
const { coreDebugLogMock, coreInfoLogMock, assertOnlyCalled } =
mockLoggingFunctions();
const {
coreDebugLogMock,
coreInfoLogMock,
coreWarningLogMock,
assertOnlyCalled,
assertNoneCalled,
} = mockLoggingFunctions();

afterEach(() => {
vi.resetAllMocks();
Expand Down Expand Up @@ -187,6 +198,54 @@ describe("utils", () => {
await vi.advanceTimersByTimeAsync(1000);

await expect(sleepPromise).resolves.toBeUndefined();

assertNoneCalled();
});
});

describe("escapeRegExp", () => {
const escaped = "\\^\\$\\.\\*\\+\\?\\(\\)\\[\\]\\{\\}\\|\\\\";
const unescaped = "^$.*+?()[]{}|\\";

it("should escape values", () => {
expect(escapeRegExp(unescaped + unescaped)).toBe(escaped + escaped);
assertNoneCalled();
});

it("should handle strings with nothing to escape", () => {
expect(escapeRegExp("abc")).toBe("abc");
assertNoneCalled();
});

it("should return an empty string for empty values", () => {
expect(escapeRegExp("")).toEqual("");
assertNoneCalled();
});
});

describe("createDistinctIdRegex", () => {
it("should return a regex without warning if the input is safe", () => {
expect(createDistinctIdRegex("test-cfg")).toStrictEqual(
new RegExp("test-cfg"),
);
assertNoneCalled();
});

it("should return a regex with warning if the input is required escaping", () => {
const input = "test$.*+?()[]{}|\\cfg";
const escapedInput = escapeRegExp(input);

const distinctId = createDistinctIdRegex(input);

// Behaviour
expect(distinctId).toStrictEqual(new RegExp(escapedInput));

// Logging
assertOnlyCalled(coreWarningLogMock);
expect(coreWarningLogMock).toHaveBeenCalledOnce();
expect(coreWarningLogMock.mock.calls[0]?.[0]).toMatchInlineSnapshot(
`"Unescaped characters found in distinctId input, using: test\\$\\.\\*\\+\\?\\(\\)\\[\\]\\{\\}\\|\\\\cfg"`,
);
});
});
});
36 changes: 36 additions & 0 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,39 @@ export function logInfoForBranchNameResult(
export function sleep(ms: number): Promise<void> {
return new Promise<void>((resolve) => setTimeout(resolve, ms));
}

/**
* Used to match `RegExp`
* [syntax characters](http://ecma-international.org/ecma-262/7.0/#sec-patterns).
*
* https://github.com/lodash/lodash/blob/main/src/escapeRegExp.ts
*/
const reRegExpChar = /[\\^$.*+?()[\]{}|]/g;
const reHasRegExpChar = RegExp(reRegExpChar.source);

/**
* Escapes the `RegExp` special characters "^", "$", "\", ".", "*", "+",
* "?", "(", ")", "[", "]", "{", "}", and "|" in `string`.
*
* https://github.com/lodash/lodash/blob/main/src/escapeRegExp.ts
*/
export function escapeRegExp(str: string): string {
return reHasRegExpChar.test(str)
? str.replace(reRegExpChar, "\\$&")
: str || "";
}

/**
* If the input distinct ID contains unescaped characters, log the
* escaped distinct ID as a warning.
*/
export function createDistinctIdRegex(distinctId: string): RegExp {
const escapedDistinctId = escapeRegExp(distinctId);
if (distinctId !== escapedDistinctId) {
core.warning(
`Unescaped characters found in distinctId input, using: ${escapedDistinctId}`,
);
}

return new RegExp(escapedDistinctId);
}

0 comments on commit 97a8019

Please sign in to comment.