diff --git a/eslint-factory/README.md b/eslint-factory/README.md index e5468641f04..a0d170f6376 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -13,3 +13,19 @@ This project hosts custom ESLint linters for `/actions/setup/js`. - `npm run build` — compile rule sources. - `npm run lint:setup-js` — build and lint all `../actions/setup/js/**/*.cjs` files. - `npm run lint:setup-js:changed` — build and lint `../actions/setup/js/*.cjs` files. + +## Rules + +### `prefer-number-isnan` + +Prefer `Number.isNaN()` over global `isNaN()` to avoid silent coercion of non-numeric inputs. + +Global `isNaN()` coerces its argument before testing, so `isNaN("123")` returns `false` because `"123"` coerces to the number `123` — masking that the input was a string. `Number.isNaN()` is strict and does not coerce, making numeric validation reliable when handling raw inputs such as environment variables or API strings. + +Flagged forms: +- `isNaN(x)` +- `globalThis.isNaN(x)` / `globalThis["isNaN"](x)` +- `window.isNaN(x)` / `window["isNaN"](x)` +- `global.isNaN(x)` / `global["isNaN"](x)` + +Locally shadowed bindings (e.g. `const isNaN = Number.isNaN`) are intentionally excluded. diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index ffc0f025370..eb245431bde 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -16,6 +16,7 @@ module.exports = [ "gh-aw-custom/no-unsafe-catch-error-property": "warn", "gh-aw-custom/no-unsafe-promise-catch-error-property": "warn", "gh-aw-custom/prefer-get-error-message": "warn", + "gh-aw-custom/prefer-number-isnan": "warn", "gh-aw-custom/require-async-entrypoint-catch": "warn", "gh-aw-custom/require-await-core-summary-write": "warn", "gh-aw-custom/require-json-parse-try-catch": "warn", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 854ea1d1f0f..0f66a326b19 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -2,6 +2,7 @@ import { noCoreSetOutputNonStringRule } from "./rules/no-core-setoutput-non-stri import { noUnsafeCatchErrorPropertyRule } from "./rules/no-unsafe-catch-error-property"; import { noUnsafePromiseCatchErrorPropertyRule } from "./rules/no-unsafe-promise-catch-error-property"; import { preferGetErrorMessageRule } from "./rules/prefer-get-error-message"; +import { preferNumberIsNanRule } from "./rules/prefer-number-isnan"; import { requireAsyncEntrypointCatchRule } from "./rules/require-async-entrypoint-catch"; import { requireAwaitCoreSummaryWriteRule } from "./rules/require-await-core-summary-write"; import { requireJsonParseTryCatchRule } from "./rules/require-json-parse-try-catch"; @@ -17,6 +18,7 @@ const plugin = { "no-unsafe-catch-error-property": noUnsafeCatchErrorPropertyRule, "no-unsafe-promise-catch-error-property": noUnsafePromiseCatchErrorPropertyRule, "prefer-get-error-message": preferGetErrorMessageRule, + "prefer-number-isnan": preferNumberIsNanRule, "require-async-entrypoint-catch": requireAsyncEntrypointCatchRule, "require-await-core-summary-write": requireAwaitCoreSummaryWriteRule, "require-json-parse-try-catch": requireJsonParseTryCatchRule, diff --git a/eslint-factory/src/rules/prefer-number-isnan.test.ts b/eslint-factory/src/rules/prefer-number-isnan.test.ts new file mode 100644 index 00000000000..080aef770fe --- /dev/null +++ b/eslint-factory/src/rules/prefer-number-isnan.test.ts @@ -0,0 +1,102 @@ +import { RuleTester } from "eslint"; +import { describe, expect, it } from "vitest"; +import { preferNumberIsNanRule } from "./prefer-number-isnan"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +const esmRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + }, +}); + +describe("prefer-number-isnan", () => { + it("uses the correct docs URL", () => { + expect(preferNumberIsNanRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#prefer-number-isnan"); + }); + + it("valid: Number.isNaN and non-global forms are accepted", () => { + // CJS-only: actions/setup/js targets CommonJS; ESM counterparts tested in the shadowed-bindings block below + cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { + valid: [`Number.isNaN(value);`, `Number["isNaN"](value);`, `foo.isNaN(value);`], + invalid: [], + }); + }); + + it("valid: locally shadowed bindings are intentionally excluded", () => { + esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { + valid: [ + `function isNaN(value) { return false; } isNaN(value);`, + `const isNaN = Number.isNaN; isNaN(value);`, + `const globalThis = { isNaN(value) { return value; } }; globalThis.isNaN(value);`, + `const window = { isNaN(value) { return value; } }; window["isNaN"](value);`, + `const global = { isNaN(value) { return value; } }; global.isNaN(value);`, + // Dynamic computed access — identifier property reference, not string literal "isNaN" + `globalThis[isNaN](value);`, + ], + invalid: [], + }); + }); + + it("valid: isNaN used as a callback reference is not a CallExpression and is not flagged", () => { + cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { + valid: [`values.some(isNaN);`], + invalid: [], + }); + }); + + it("invalid: global isNaN() is flagged with a replacement suggestion", () => { + cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { + valid: [], + invalid: [ + { + code: `isNaN(value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + // Raw string argument (e.g. env var) — suggestion preserves argument so callers must review whether to wrap with Number(...) + code: `isNaN(process.env.PORT);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(process.env.PORT);` }] }], + }, + ], + }); + }); + + it("invalid: global object isNaN() access is flagged for direct and computed forms", () => { + cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { + valid: [], + invalid: [ + { + code: `globalThis.isNaN(value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + code: `globalThis["isNaN"](value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + code: `window.isNaN(value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + code: `window["isNaN"](value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + code: `global.isNaN(value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + { + code: `global["isNaN"](value);`, + errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/prefer-number-isnan.ts b/eslint-factory/src/rules/prefer-number-isnan.ts new file mode 100644 index 00000000000..d6e55111379 --- /dev/null +++ b/eslint-factory/src/rules/prefer-number-isnan.ts @@ -0,0 +1,88 @@ +import { ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); +const GLOBAL_IS_NAN_OBJECTS = new Set(["globalThis", "window", "global"]); + +export const preferNumberIsNanRule = createRule({ + name: "prefer-number-isnan", + meta: { + type: "suggestion", + hasSuggestions: true, + docs: { + description: "Prefer Number.isNaN() over global isNaN() to avoid coercion footguns when validating unknown inputs.", + }, + schema: [], + messages: { + preferNumberIsNaN: "Prefer Number.isNaN(...) over global isNaN(...). Global isNaN() coerces non-number inputs and can hide invalid raw values.", + replaceWithNumberIsNaN: "Replace callee with Number.isNaN — review whether the argument should be wrapped with Number(...).", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + type SourceCodeScope = ReturnType; + + /** + * Checks whether a given identifier name is locally bound in the current scope chain. + * @param node AST node to start the scope search from. + * @param name Identifier name to search for. + * @returns true if the name has a local binding, false otherwise. + */ + function hasLocalBinding(node: TSESTree.Node, name: string): boolean { + let scope: SourceCodeScope | null = sourceCode.getScope(node); + + while (scope) { + const variable = scope.set.get(name); + + if (variable?.defs.some(d => d.type !== "ImportBinding")) { + return true; + } + + scope = scope.upper; + } + + return false; + } + + /** + * Checks whether a MemberExpression property is isNaN, either direct or computed. + * @param node MemberExpression node to inspect. + * @returns true if the property is isNaN. + */ + function isIsNaNProperty(node: TSESTree.MemberExpression): boolean { + const property = node.property; + const isDirectAccess = !node.computed && property.type === "Identifier" && property.name === "isNaN"; + const isComputedAccess = property.type === "Literal" && property.value === "isNaN"; + + return isDirectAccess || isComputedAccess; + } + + function report(node: TSESTree.CallExpression): void { + context.report({ + node: node.callee, + messageId: "preferNumberIsNaN", + suggest: [ + { + messageId: "replaceWithNumberIsNaN", + fix(fixer: TSESLint.RuleFixer) { + return fixer.replaceText(node.callee, "Number.isNaN"); + }, + }, + ], + }); + } + + return { + CallExpression(node) { + if (node.callee.type === "Identifier" && node.callee.name === "isNaN" && !hasLocalBinding(node, "isNaN")) { + report(node); + return; + } + + if (node.callee.type === "MemberExpression" && node.callee.object.type === "Identifier" && GLOBAL_IS_NAN_OBJECTS.has(node.callee.object.name) && !hasLocalBinding(node, node.callee.object.name) && isIsNaNProperty(node.callee)) { + report(node); + } + }, + }; + }, +});