Skip to content
Merged
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
3 changes: 3 additions & 0 deletions .github/skills/agentic-workflows/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Load these files from `github/gh-aw` (they are not available locally).
- `.github/aw/evals.md`
- `.github/aw/experiments.md`
- `.github/aw/github-agentic-workflows.md`
- `.github/aw/github-mcp-server-pagination.md`
- `.github/aw/github-mcp-server.md`
- `.github/aw/instructions.md`
- `.github/aw/linter-workflows.md`
Expand Down Expand Up @@ -64,10 +65,12 @@ Load these files from `github/gh-aw` (they are not available locally).
- `.github/aw/subagents.md`
- `.github/aw/syntax-agentic.md`
- `.github/aw/syntax-core.md`
- `.github/aw/syntax-engine.md`
- `.github/aw/syntax-tools-imports.md`
- `.github/aw/syntax.md`
- `.github/aw/test-coverage.md`
- `.github/aw/test-expression.md`
- `.github/aw/token-optimization-caching-budgets.md`
- `.github/aw/token-optimization.md`
- `.github/aw/triggers.md`
- `.github/aw/update-agentic-workflow.md`
Expand Down
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ module.exports = [
"gh-aw-custom/require-fetch-try-catch": "warn",
"gh-aw-custom/no-core-error-then-setfailed": "warn",
"gh-aw-custom/no-duplicate-constant-values": "warn",
"gh-aw-custom/require-escaped-regexp-interpolation": "warn",
},
},
{
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { noCaughtErrorInterpolationRule } from "./rules/no-caught-error-interpol
import { requireFetchTryCatchRule } from "./rules/require-fetch-try-catch";
import { noCoreErrorThenSetFailedRule } from "./rules/no-core-error-then-setfailed";
import { noDuplicateConstantValuesRule } from "./rules/no-duplicate-constant-values";
import { requireEscapedRegexpInterpolationRule } from "./rules/require-escaped-regexp-interpolation";

const plugin = {
meta: {
Expand Down Expand Up @@ -73,6 +74,7 @@ const plugin = {
"require-fetch-try-catch": requireFetchTryCatchRule,
"no-core-error-then-setfailed": noCoreErrorThenSetFailedRule,
"no-duplicate-constant-values": noDuplicateConstantValuesRule,
"require-escaped-regexp-interpolation": requireEscapedRegexpInterpolationRule,
},
};

Expand Down
122 changes: 122 additions & 0 deletions eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { RuleTester } from "eslint";
import { describe, expect, it } from "vitest";
import { requireEscapedRegexpInterpolationRule } from "./require-escaped-regexp-interpolation";

const cjsRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

describe("require-escaped-regexp-interpolation", () => {
it("uses the correct docs URL", () => {
expect(requireEscapedRegexpInterpolationRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#require-escaped-regexp-interpolation");
});

it("valid: non-interpolated RegExp patterns are accepted", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: ['new RegExp("^[a-z]+$");', "new RegExp(`^[a-z]+$`);", "new RegExp(somePattern);", "new RegExp(somePattern, 'g');"],
invalid: [],
});
});

it("valid: interpolated value already passed through an escape helper is accepted", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [
"new RegExp(`^${escapeRegExp(varName)}$`);",
"new RegExp(`(^|[-_\\\\s])${escapeRegex(qualifier)}($|[-_\\\\s])`);",
"new RegExp(`\\\\$\\\\{${utils.escapeRegExp(varName)}\\\\}`, 'g');",
"new RegExp(`^${ESCAPED_NAME}$`);",
"new RegExp(`^${escapedValue}$`);",
],
invalid: [],
});
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] The escapedValue naming-convention test case (new RegExp(\^${escapedValue}$`)) passes as valid, but there is no corresponding test that unescapedValueis correctly *flagged* as invalid. Without this, a regression in the/escape/i` pattern (e.g. after narrowing it) would go undetected.

💡 Add a companion invalid test
it("invalid: name starting with unescaped should NOT be treated as safe", () => {
  cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
    valid: [],
    invalid: [
      {
        code: "new RegExp(`^${unescapedValue}$`);",
        errors: [{ messageId: "unescapedInterpolation" }],
      },
    ],
  });
});

@copilot please address this.

it('valid: standard inline .replace(…, "\\\\$&") escape form is accepted', () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: ['new RegExp(`^${varName.replace(/[.*+?^${}()|[\\\\]\\\\\\\\]/g, "\\\\$&")}$`);', 'new RegExp(`^${qualifier.replace(/[.*+?^${}()|[\\\\]\\\\\\\\]/g, "\\\\$&")}($|[-_\\\\s])`);'],
invalid: [],
});
});

it("valid: unrelated `new` calls to other constructors are not flagged", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: ["new Foo(`^${bar}$`);", "new Date(`${year}-01-01`);"],
invalid: [],
});
});

it("invalid: interpolated loop variable in RegExp pattern without escaping", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "const pattern = new RegExp(`\\\\$\\\\{${varName}\\\\}`, 'g');",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});

it("invalid: interpolated function parameter in RegExp pattern without escaping", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "function hasQualifier(name, qualifier) { return new RegExp(`(^|[-_\\\\s])${qualifier}($|[-_\\\\s])`).test(name); }",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});

it("invalid: multiple unescaped interpolations are each reported", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "new RegExp(`${prefix}-${suffix}`);",
errors: [{ messageId: "unescapedInterpolation" }, { messageId: "unescapedInterpolation" }],
},
],
});
});

it("invalid: mixed escaped and unescaped interpolations only report the unescaped one", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "new RegExp(`${escapeRegExp(prefix)}-${suffix}`);",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});

it("invalid: identifier named unescapedValue is not treated as safe", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "new RegExp(`^${unescapedValue}$`);",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});

it("invalid: escapeHtml call is not treated as a regex-escape helper", () => {
cjsRuleTester.run("require-escaped-regexp-interpolation", requireEscapedRegexpInterpolationRule, {
valid: [],
invalid: [
{
code: "new RegExp(`^${escapeHtml(userInput)}$`);",
errors: [{ messageId: "unescapedInterpolation" }],
},
],
});
});
});
109 changes: 109 additions & 0 deletions eslint-factory/src/rules/require-escaped-regexp-interpolation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);

// Matches function/method names that are specifically regex-escaping helpers,
// e.g. escapeRegExp, escapeRegex, regExpEscape. Requires both "escape" and "reg"
// to be present, preventing false negatives from escapeHtml, unescape, etc.
const ESCAPE_CALL_NAME_PATTERN = /escape.*reg|reg.*escape/i;

// Matches identifier/property names that signal a value has already been
// regex-escaped, e.g. escapedValue, ESCAPED_NAME. Requires the name to START
// with "escaped", so unescapedValue and escapeHelper are never whitelisted.
const ESCAPED_IDENT_PATTERN = /^escaped/i;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] ESCAPE_NAME_PATTERN = /escape/i will also match names like unescapedValue, UNESCAPED_FOO, and the built-in unescape() — all of which are not escape helpers and would silently suppress a warning (false negative).

💡 Suggested fix

Narrow the pattern to only match names that start with or contain a positive "escape" signal, e.g.:

// Only match names that begin with "escape" (case-insensitive) or contain "EscapeReg"
const ESCAPE_NAME_PATTERN = /^escape|escapeReg/i;

Alternatively, enumerate accepted helper names (like escapeRegExp, escapeRegex) for zero false negatives.

Add a test case:

// Should NOT be treated as safe — contains "escape" but is a bad signal:
{ code: "new RegExp(`${unescapedValue}`);" }

@copilot please address this.

/**
* Returns true when `node` is a call expression whose callee name looks like
* a regex-escaping helper (e.g. `escapeRegExp(value)`, `utils.escapeRegex(value)`).
* Both "escape" and "reg" must appear in the name so unrelated helpers such as
* `escapeHtml` or `unescape` are not treated as regex-safe.
*/
function isEscapeHelperCall(node: TSESTree.Node): boolean {
if (node.type !== AST_NODE_TYPES.CallExpression) return false;
const callee = node.callee;
if (callee.type === AST_NODE_TYPES.Identifier) return ESCAPE_CALL_NAME_PATTERN.test(callee.name);
if (callee.type === AST_NODE_TYPES.MemberExpression && !callee.computed && callee.property.type === AST_NODE_TYPES.Identifier) {
return ESCAPE_CALL_NAME_PATTERN.test(callee.property.name);
}
return false;
}

/**
* Returns true when `node` is a call of the form
* `value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")` — the standard inline
* pattern for escaping all regex metacharacters before interpolation.
*/
function isRegexEscapeReplaceCall(node: TSESTree.Node): boolean {
if (node.type !== AST_NODE_TYPES.CallExpression) return false;
const { callee, arguments: args } = node;
if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false;
if (callee.property.type !== AST_NODE_TYPES.Identifier || callee.property.name !== "replace") return false;
if (args.length < 2) return false;
const replacement = args[1];
return replacement.type === AST_NODE_TYPES.Literal && typeof replacement.value === "string" && replacement.value === "\\$&";
}

/**
* Returns true when `node` is an identifier or member expression whose name
* indicates the value has already been escaped, e.g. `ESCAPED_FOO` or
* `escapedValue`. The name must start with "escaped" so that `unescapedValue`
* is never treated as safe.
*/
function isEscapedNameReference(node: TSESTree.Node): boolean {
if (node.type === AST_NODE_TYPES.Identifier) return ESCAPED_IDENT_PATTERN.test(node.name);
if (node.type === AST_NODE_TYPES.MemberExpression && !node.computed && node.property.type === AST_NODE_TYPES.Identifier) {
return ESCAPED_IDENT_PATTERN.test(node.property.name);
}
return false;
}

/**
* Returns true when the interpolated expression is recognized as already
* escaped — via a named regex-escape helper call, the standard inline
* `.replace()` form, or a variable name that starts with "escaped".
*/
function isRecognizedAsEscaped(node: TSESTree.Node): boolean {
return isEscapeHelperCall(node) || isRegexEscapeReplaceCall(node) || isEscapedNameReference(node);
}

export const requireEscapedRegexpInterpolationRule = createRule({
name: "require-escaped-regexp-interpolation",
meta: {
type: "problem",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/grill-with-docs] The README.md for eslint-factory documents every other rule with a table entry and dedicated section (e.g. require-fetch-try-catch, no-duplicate-constant-values), but this new rule is not added. Users of the plugin have no way to discover the rule purpose, options, or how to suppress it.

💡 Add a README entry

Add a row to the rules table and a ### require-escaped-regexp-interpolation section following the pattern of existing rules, including: what it detects, motivation, and the escape-helper naming convention for suppression.

@copilot please address this.

docs: {
description:
"Require values interpolated into a `new RegExp()` template-literal pattern to be passed through a regex-escaping helper first. " +
"Unescaped interpolation of a value containing regex metacharacters (e.g. `.`, `*`, `+`, `(`, `)`) can produce unintended matches " +
"or, with attacker-controlled input, a ReDoS-prone pattern.",
},
schema: [],
messages: {
unescapedInterpolation:
"Interpolated value `{{expr}}` in `new RegExp()` template literal is not passed through a regex-escaping helper. " + 'Escape regex metacharacters before interpolating, e.g. `{{expr}}.replace(/[.*+?^${}()|[\\]\\\\]/g, "\\\\$&")`.',
},
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;

return {
NewExpression(node) {
if (node.callee.type !== AST_NODE_TYPES.Identifier || node.callee.name !== "RegExp") return;

const patternArg = node.arguments[0];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] The rule only fires on new RegExp(...) where the callee is a bare Identifier named RegExp. A common pattern like new globalThis.RegExp(...) or const RE = RegExp; new RE(...) would be silently skipped. This is a documented design choice (low false-positive), but the scope limitation should be called out in the rule description or a comment — not silently.

💡 Consider a doc comment at the NewExpression handler
// NOTE: Only bare `new RegExp(...)` is matched; aliased or member-accessed
// constructors (globalThis.RegExp, re2.RegExp, etc.) are intentionally out of
// scope to keep false-positive rate low.
NewExpression(node) {

This makes the deliberate scope explicit for future maintainers.

@copilot please address this.

if (!patternArg || patternArg.type !== AST_NODE_TYPES.TemplateLiteral) return;
if (patternArg.expressions.length === 0) return;

for (const expr of patternArg.expressions) {
if (isRecognizedAsEscaped(expr)) continue;

context.report({
node: expr,
messageId: "unescapedInterpolation",
data: { expr: sourceCode.getText(expr) },
});
}
},
};
},
});
Loading