diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 141445630d5..6fb19019416 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -71,7 +71,6 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/test-coverage.md` - `.github/aw/test-expression.md` - `.github/aw/token-optimization-caching-budgets.md` -- `.github/aw/token-optimization-observability.md` - `.github/aw/token-optimization.md` - `.github/aw/triggers.md` - `.github/aw/update-agentic-workflow.md` diff --git a/eslint-factory/README.md b/eslint-factory/README.md index 7eac2767f41..eafc972c8c6 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -41,6 +41,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`. | [`require-mkdirsync-try-catch`](#require-mkdirsync-try-catch) | Require try/catch around `fs.mkdirSync` calls | | [`require-new-url-try-catch`](#require-new-url-try-catch) | Require try/catch around `new URL(variable)` calls | | [`require-parseInt-radix`](#require-parseInt-radix) | Require an explicit radix argument to `parseInt()` | +| [`require-nan-check-after-env-numeric-parse`](#require-nan-check-after-env-numeric-parse) | Require NaN validation after parsing numeric values from `process.env` | | [`require-return-after-core-setfailed`](#require-return-after-core-setfailed) | Require a control-transfer statement after `core.setFailed()` | | [`require-execsync-try-catch`](#require-execsync-try-catch) | Require try/catch around `execSync(...)` calls from `child_process` | | [`require-execfilesync-try-catch`](#require-execfilesync-try-catch) | Require try/catch around `execFileSync(...)` calls from `child_process` | @@ -327,6 +328,33 @@ Flagged forms: Why: omitting the radix allows implicit base detection, which can silently accept prefixes such as `0x`. +### `require-nan-check-after-env-numeric-parse` + +Require NaN validation after parsing numeric values from `process.env`. + +Why: `parseInt`, `parseFloat`, `Number.parseInt`, `Number.parseFloat`, and `Number()` silently return `NaN` for malformed environment input (empty string, typo, unexpected value). An unvalidated `NaN` can propagate silently into comparisons (e.g. rate-limit thresholds, size limits, timeouts), loop bounds, or GitHub API payloads without any error surfacing. + +**Detected parse forms (first argument must trace back to `process.env`):** +- `parseInt(process.env.FOO, 10)` — global `parseInt` +- `parseFloat(process.env.FOO)` — global `parseFloat` +- `Number.parseInt(process.env.FOO, 10)` — `Number.parseInt` +- `Number.parseFloat(process.env.FOO)` — `Number.parseFloat` +- `Number(process.env.FOO)` — `Number` conversion function + +**Detected env-access patterns in the first argument:** +- Direct: `process.env.FOO` +- Logical fallbacks: `process.env.FOO || "default"`, `process.env.FOO ?? "default"` +- Optional chaining: `process.env.FOO?.trim()` +- Ternary: `process.env.FOO ? process.env.FOO : "default"` + +**Considered validated when** the declared variable is passed as the sole argument to `Number.isNaN(...)` or `isNaN(...)` anywhere in the enclosing file scope. + +**Safe pattern:** +```js +const maxRuns = parseInt(process.env.MAX_RUNS, 10); +if (Number.isNaN(maxRuns)) throw new Error("MAX_RUNS must be a valid integer"); +``` + ### `require-mkdirsync-try-catch` Require `fs.mkdirSync` calls to be wrapped in `try/catch`. diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 924668b3035..d996283bf73 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -49,6 +49,7 @@ module.exports = [ "gh-aw-custom/no-duplicate-constant-values": "warn", "gh-aw-custom/require-escaped-regexp-interpolation": "warn", "gh-aw-custom/require-fetch-timeout": "warn", + "gh-aw-custom/require-nan-check-after-env-numeric-parse": "warn", }, }, { diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 09a2439a6d0..a65eb3d6c51 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -35,6 +35,7 @@ import { noCoreErrorThenSetFailedRule } from "./rules/no-core-error-then-setfail import { noDuplicateConstantValuesRule } from "./rules/no-duplicate-constant-values"; import { requireEscapedRegexpInterpolationRule } from "./rules/require-escaped-regexp-interpolation"; import { requireFetchTimeoutRule } from "./rules/require-fetch-timeout"; +import { requireNanCheckAfterEnvNumericParseRule } from "./rules/require-nan-check-after-env-numeric-parse"; const plugin = { meta: { @@ -79,6 +80,7 @@ const plugin = { "no-duplicate-constant-values": noDuplicateConstantValuesRule, "require-escaped-regexp-interpolation": requireEscapedRegexpInterpolationRule, "require-fetch-timeout": requireFetchTimeoutRule, + "require-nan-check-after-env-numeric-parse": requireNanCheckAfterEnvNumericParseRule, }, }; diff --git a/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.test.ts b/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.test.ts new file mode 100644 index 00000000000..9de920fc11a --- /dev/null +++ b/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.test.ts @@ -0,0 +1,234 @@ +import { RuleTester } from "eslint"; +import { describe, expect, it } from "vitest"; +import { requireNanCheckAfterEnvNumericParseRule } from "./require-nan-check-after-env-numeric-parse"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +const esmRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + }, +}); + +describe("require-nan-check-after-env-numeric-parse", () => { + it("uses the correct docs URL", () => { + expect(requireNanCheckAfterEnvNumericParseRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#require-nan-check-after-env-numeric-parse"); + }); + + it("valid: parseInt from process.env validated with Number.isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const maxRuns = parseInt(process.env.MAX_RUNS, 10); if (Number.isNaN(maxRuns)) throw new Error("invalid MAX_RUNS");`, `const port = parseInt(process.env.PORT, 10); if (!Number.isNaN(port)) startServer(port);`], + invalid: [], + }); + }); + + it("valid: parseInt from process.env validated with global isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const maxRuns = parseInt(process.env.MAX_RUNS, 10); if (isNaN(maxRuns)) throw new Error("invalid MAX_RUNS");`], + invalid: [], + }); + }); + + it("valid: Number.parseInt from process.env validated with Number.isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const port = Number.parseInt(process.env.PORT, 10); if (Number.isNaN(port)) throw new Error("invalid PORT");`], + invalid: [], + }); + }); + + it("valid: Number.parseFloat from process.env validated with Number.isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const delay = Number.parseFloat(process.env.DELAY); if (Number.isNaN(delay)) throw new Error("invalid DELAY");`], + invalid: [], + }); + }); + + it("valid: Number() from process.env validated with Number.isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const runId = Number(process.env.RUN_ID); if (Number.isNaN(runId)) throw new Error("invalid RUN_ID");`], + invalid: [], + }); + }); + + it("valid: parseFloat from process.env validated with Number.isNaN", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const rate = parseFloat(process.env.RATE); if (Number.isNaN(rate)) throw new Error("invalid RATE");`], + invalid: [], + }); + }); + + it("valid: parseInt without process.env is not flagged", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const x = parseInt("42", 10);`, `const y = parseInt(someVariable, 10);`, `const z = Number("42");`], + invalid: [], + }); + }); + + it("valid: env access validated with isNaN using logical fallback pattern", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const count = parseInt(process.env.COUNT || "0", 10); if (Number.isNaN(count)) throw new Error();`, `const count = parseInt(process.env.COUNT ?? "0", 10); if (Number.isNaN(count)) throw new Error();`], + invalid: [], + }); + }); + + it("valid: env access validated with isNaN using optional chaining pattern", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const count = parseInt(process.env.COUNT?.trim(), 10); if (Number.isNaN(count)) throw new Error();`], + invalid: [], + }); + }); + + it("valid: env access validated with isNaN using ternary pattern", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const delay = parseInt(process.env.DELAY ? process.env.DELAY : "1000", 10); if (Number.isNaN(delay)) throw new Error();`], + invalid: [], + }); + }); + + it("invalid: parseInt from process.env without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const maxRuns = parseInt(process.env.MAX_RUNS, 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "maxRuns" } }], + }, + { + code: `const port = parseInt(process.env.PORT, 10); startServer(port);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "port" } }], + }, + ], + }); + }); + + it("invalid: parseFloat from process.env without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const rate = parseFloat(process.env.RATE);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "rate" } }], + }, + ], + }); + }); + + it("invalid: Number.parseInt from process.env without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const port = Number.parseInt(process.env.PORT, 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "port" } }], + }, + ], + }); + }); + + it("invalid: Number.parseFloat from process.env without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const delay = Number.parseFloat(process.env.DELAY);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "delay" } }], + }, + ], + }); + }); + + it("invalid: Number() from process.env without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const runId = Number(process.env.RUN_ID);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "runId" } }], + }, + ], + }); + }); + + it("invalid: logical fallback env access without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const maxFileSize = parseInt(process.env.MAX_FILE_SIZE || "1000", 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "maxFileSize" } }], + }, + { + code: `const timeout = parseInt(process.env.TIMEOUT ?? "5000", 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "timeout" } }], + }, + ], + }); + }); + + it("invalid: optional chaining env access without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const port = parseInt(process.env.PORT?.trim(), 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "port" } }], + }, + ], + }); + }); + + it("invalid: ternary-wrapped env access without NaN check", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const configuredDelay = parseInt(process.env.DELAY ? process.env.DELAY : "1000", 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "configuredDelay" } }], + }, + ], + }); + }); + + it("invalid: multiple unvalidated env parse declarations are each reported", () => { + cjsRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: ` +const maxFileSize = parseInt(process.env.MAX_FILE_SIZE, 10); +const maxFileCount = parseInt(process.env.MAX_FILE_COUNT, 10); + `.trim(), + errors: [ + { messageId: "requireNaNCheck", data: { name: "maxFileSize" } }, + { messageId: "requireNaNCheck", data: { name: "maxFileCount" } }, + ], + }, + ], + }); + }); + + it("valid: ESM import style — validated with Number.isNaN", () => { + esmRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [`const count = parseInt(process.env.COUNT, 10); if (Number.isNaN(count)) throw new Error();`], + invalid: [], + }); + }); + + it("invalid: ESM import style — missing NaN check", () => { + esmRuleTester.run("require-nan-check-after-env-numeric-parse", requireNanCheckAfterEnvNumericParseRule, { + valid: [], + invalid: [ + { + code: `const count = parseInt(process.env.COUNT, 10);`, + errors: [{ messageId: "requireNaNCheck", data: { name: "count" } }], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts b/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts new file mode 100644 index 00000000000..33ddb9115b8 --- /dev/null +++ b/eslint-factory/src/rules/require-nan-check-after-env-numeric-parse.ts @@ -0,0 +1,140 @@ +import { ESLintUtils, TSESTree } from "@typescript-eslint/utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +export const requireNanCheckAfterEnvNumericParseRule = createRule({ + name: "require-nan-check-after-env-numeric-parse", + meta: { + type: "problem", + docs: { + description: "Require NaN validation after parsing numeric values from process.env to prevent silent NaN propagation into comparisons, loop bounds, or API payloads.", + }, + schema: [], + messages: { + requireNaNCheck: "Numeric value '{{name}}' parsed from process.env is never validated with Number.isNaN() or isNaN(). Parsing functions silently return NaN for malformed environment input.", + }, + }, + defaultOptions: [], + create(context) { + // Map from variable name to the VariableDeclarator node (for reporting) + const unvalidated = new Map(); + // Set of variable names confirmed to be passed to isNaN / Number.isNaN + const validated = new Set(); + + /** + * Returns true when the given node contains or is a process.env property access. + * Handles member expressions, optional chaining, logical fallbacks, and ternaries. + */ + function containsEnvAccess(node: TSESTree.Node): boolean { + switch (node.type) { + case "MemberExpression": { + const obj = node.object; + // Direct process.env.FOO or process.env["FOO"] + if (obj.type === "MemberExpression" && obj.object.type === "Identifier" && obj.object.name === "process" && !obj.computed && obj.property.type === "Identifier" && obj.property.name === "env") { + return true; + } + // Recurse into the object to handle deeper chains + return containsEnvAccess(obj); + } + case "ChainExpression": + return containsEnvAccess(node.expression); + case "CallExpression": + // process.env.FOO?.trim() — method call chained on an env access + if (node.callee.type === "MemberExpression") { + return containsEnvAccess(node.callee.object); + } + return false; + case "LogicalExpression": + // process.env.FOO || "default" or process.env.FOO ?? "default" + return containsEnvAccess(node.left) || containsEnvAccess(node.right); + case "ConditionalExpression": + // ternary: process.env.FOO ? x : y or cond ? process.env.FOO : y + return containsEnvAccess(node.test) || containsEnvAccess(node.consequent) || containsEnvAccess(node.alternate); + default: + return false; + } + } + + /** + * Returns true when the call expression is a numeric-parse function whose + * first argument traces back to a process.env access. + */ + function isNumericParseCallFromEnv(node: TSESTree.CallExpression): boolean { + const { callee, arguments: args } = node; + + if (args.length === 0 || args[0].type === "SpreadElement") return false; + + const firstArg = args[0] as TSESTree.Expression; + + // Global parseInt(envExpr, ...) or parseFloat(envExpr) + if (callee.type === "Identifier" && (callee.name === "parseInt" || callee.name === "parseFloat")) { + return containsEnvAccess(firstArg); + } + + // Number.parseInt(envExpr, ...) or Number.parseFloat(envExpr) + if ( + callee.type === "MemberExpression" && + callee.object.type === "Identifier" && + callee.object.name === "Number" && + !callee.computed && + callee.property.type === "Identifier" && + (callee.property.name === "parseInt" || callee.property.name === "parseFloat") + ) { + return containsEnvAccess(firstArg); + } + + // Number(envExpr) — Number used as a conversion function + if (callee.type === "Identifier" && callee.name === "Number") { + return containsEnvAccess(firstArg); + } + + return false; + } + + /** + * Returns true when the call expression is isNaN(...) or Number.isNaN(...). + */ + function isIsNaNCall(node: TSESTree.CallExpression): boolean { + const { callee } = node; + + // Global isNaN(x) + if (callee.type === "Identifier" && callee.name === "isNaN") { + return true; + } + + // Number.isNaN(x) + if (callee.type === "MemberExpression" && callee.object.type === "Identifier" && callee.object.name === "Number" && !callee.computed && callee.property.type === "Identifier" && callee.property.name === "isNaN") { + return true; + } + + return false; + } + + return { + VariableDeclarator(node) { + if (node.id.type === "Identifier" && node.init?.type === "CallExpression" && isNumericParseCallFromEnv(node.init as TSESTree.CallExpression)) { + unvalidated.set(node.id.name, node); + } + }, + + CallExpression(node) { + // Track any isNaN(x) / Number.isNaN(x) call where x is an identifier + if (isIsNaNCall(node) && node.arguments.length === 1 && node.arguments[0].type === "Identifier") { + validated.add((node.arguments[0] as TSESTree.Identifier).name); + } + }, + + "Program:exit"() { + for (const [name, declaratorNode] of unvalidated) { + if (!validated.has(name)) { + context.report({ + node: declaratorNode, + messageId: "requireNaNCheck", + data: { name }, + }); + } + } + }, + }; + }, +});