Skip to content
Closed
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
36 changes: 34 additions & 2 deletions src/remote-config/condition-evaluator-internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
CustomSignalOperator,
} from './remote-config-api';
import { createHash } from 'crypto';

import * as vm from 'vm';

/**
* Encapsulates condition evaluation logic to simplify organization and
Expand Down Expand Up @@ -226,7 +226,7 @@ export class ConditionEvaluator {
return compareStrings(
targetCustomSignalValues,
actualCustomSignalValue,
(target, actual) => new RegExp(target).test(actual),
(target, actual) => safeRegexTest(target, actual),
);

// For numeric operators only one target value is allowed.
Expand Down Expand Up @@ -274,6 +274,38 @@ function compareStrings(
return targetValues.some((target) => predicateFn(target, actual));
}

// Pre-compile a single V8 context to avoid the high performance penalty of
// creating a new context for every regex evaluation.
const sharedRegexSandbox = {
pattern: '',
actual: '',
result: false,
};
const sharedRegexContext = vm.createContext(sharedRegexSandbox);
const sharedRegexScript = new vm.Script(`
try {
result = new RegExp(pattern).test(actual);
} catch (e) {
result = false;
}
`);

// Compares a regex against an actual string safely with a timeout to mitigate ReDoS.
function safeRegexTest(pattern: string, actual: string): boolean {
try {
sharedRegexSandbox.pattern = pattern;
sharedRegexSandbox.actual = actual;
sharedRegexSandbox.result = false;

// Timeout is set to 100ms.
sharedRegexScript.runInContext(sharedRegexContext, { timeout: 100 });
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The timeout value 100 is a magic number. It would be more maintainable and readable to define this as a named constant, for example, REGEX_EVALUATION_TIMEOUT_MS.

Suggested change
sharedRegexScript.runInContext(sharedRegexContext, { timeout: 100 });
const REGEX_EVALUATION_TIMEOUT_MS = 100;
sharedRegexScript.runInContext(sharedRegexContext, { timeout: REGEX_EVALUATION_TIMEOUT_MS });

return sharedRegexSandbox.result;
} catch (e) {
// Return false if the regex evaluation times out or throws any other error.
return false;
}
}

// Compares two numbers against each other.
// Calls the predicate function with -1, 0, 1 if actual is less than, equal to, or greater than target.
function compareNumbers(
Expand Down
2 changes: 2 additions & 0 deletions test/unit/remote-config/condition-evaluator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,8 @@ describe('ConditionEvaluator', () => {
const testCases: CustomSignalTestCase[] = [
{ targets: ['foo', '^ba.*$'], actual: 'bar', outcome: true },
{ targets: [' bar '], actual: 'biz', outcome: false },
{ targets: ['^(a+)+$'], actual: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaX', outcome: false },
{ targets: ['[a-z)+'], actual: 'test', outcome: false }, // invalid regex pattern
];

testCases.forEach(runCustomSignalTestCase(CustomSignalOperator.STRING_CONTAINS_REGEX));
Expand Down
Loading