diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 6f24708a24e..6fb19019416 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -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` @@ -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` diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 65462d4da8f..ce7a3a2b1ff 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -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", }, }, { diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 95ffdafa44b..a2b66e8094d 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -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: { @@ -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, }, }; diff --git a/eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts b/eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts new file mode 100644 index 00000000000..3c283cc1f24 --- /dev/null +++ b/eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts @@ -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: [], + }); + }); + + 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" }], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/require-escaped-regexp-interpolation.ts b/eslint-factory/src/rules/require-escaped-regexp-interpolation.ts new file mode 100644 index 00000000000..119d5a8f697 --- /dev/null +++ b/eslint-factory/src/rules/require-escaped-regexp-interpolation.ts @@ -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; + +/** + * 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", + 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]; + 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) }, + }); + } + }, + }; + }, +});