diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index eb245431bde..08ac9d9159d 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -13,6 +13,7 @@ module.exports = [ }, rules: { "gh-aw-custom/no-core-setoutput-non-string": "warn", + "gh-aw-custom/no-json-stringify-error": "warn", "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", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 0f66a326b19..b4ac4e27a50 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -1,4 +1,5 @@ import { noCoreSetOutputNonStringRule } from "./rules/no-core-setoutput-non-string"; +import { noJsonStringifyErrorRule } from "./rules/no-json-stringify-error"; 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"; @@ -15,6 +16,7 @@ const plugin = { }, rules: { "no-core-setoutput-non-string": noCoreSetOutputNonStringRule, + "no-json-stringify-error": noJsonStringifyErrorRule, "no-unsafe-catch-error-property": noUnsafeCatchErrorPropertyRule, "no-unsafe-promise-catch-error-property": noUnsafePromiseCatchErrorPropertyRule, "prefer-get-error-message": preferGetErrorMessageRule, diff --git a/eslint-factory/src/rules/no-json-stringify-error.test.ts b/eslint-factory/src/rules/no-json-stringify-error.test.ts new file mode 100644 index 00000000000..0b58fd18d2f --- /dev/null +++ b/eslint-factory/src/rules/no-json-stringify-error.test.ts @@ -0,0 +1,233 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { noJsonStringifyErrorRule } from "./no-json-stringify-error"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +const esmRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + }, +}); + +describe("no-json-stringify-error", () => { + it("valid: JSON.stringify on a non-caught variable is not flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [`const obj = { a: 1 }; JSON.stringify(obj);`, `JSON.stringify({ message: "hello" });`, `JSON.stringify("a string");`, `const data = fetchData(); JSON.stringify(data);`], + invalid: [], + }); + }); + + it("valid: JSON.stringify on a non-error variable inside a catch block is not flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [`try { f(); } catch (err) { const data = { a: 1 }; JSON.stringify(data); }`, `try { f(); } catch (err) { JSON.stringify(someOtherVar); }`, `try { f(); } catch (err) { JSON.stringify({ message: err.message }); }`], + invalid: [], + }); + }); + + it("valid: JSON.stringify with explicit error properties is not flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [`try { f(); } catch (err) { JSON.stringify({ message: err.message, stack: err.stack }); }`, `try { f(); } catch (err) { JSON.stringify({ error: String(err) }); }`], + invalid: [], + }); + }); + + it("valid: JSON.stringify on catch param that shadows outer scope is not flagged outside the catch", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [ + `const err = { a: 1 }; try { f(); } catch (err) { } JSON.stringify(err);`, + `try { f(); } catch (err) { [1].forEach(function(err) { JSON.stringify(err); }); }`, + `try { f(); } catch (err) { items.map(err => JSON.stringify(err)); }`, + ], + invalid: [], + }); + }); + + it("valid: JSON.stringify on promise .catch() non-error param is not flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [`p.catch(function(err) { JSON.stringify(otherVar); })`, `p.catch(err => JSON.stringify(notErr))`], + invalid: [], + }); + }); + + it("valid: bare catch {} without binding is not flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [`try { f(); } catch { JSON.stringify(someObj); }`], + invalid: [], + }); + }); + + it("invalid: JSON.stringify(err) in catch block is flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { core.error(JSON.stringify(err)); }`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { core.error(getErrorMessage(err)); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: JSON.stringify(error, null, 2) in catch block is flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (error) { core.error(\`details: \${JSON.stringify(error, null, 2)}\`); }`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "error" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "error" }, + output: `try { f(); } catch (error) { core.error(\`details: \${getErrorMessage(error)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: JSON.stringify(err) in promise .catch() arrow callback is flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `p.catch(err => core.error(JSON.stringify(err)));`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `p.catch(err => core.error(getErrorMessage(err)));`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: JSON.stringify(err) in promise .catch() function callback is flagged", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `p.catch(function(err) { core.error(JSON.stringify(err)); });`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `p.catch(function(err) { core.error(getErrorMessage(err)); });`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: nested catch — each catch variable is tracked independently", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(JSON.stringify(outer)); }`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "outer" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "outer" }, + output: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(getErrorMessage(outer)); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: works with ES module syntax", () => { + esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `try { fetch(url); } catch (e) { console.error(JSON.stringify(e)); }`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "e" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "e" }, + output: `try { fetch(url); } catch (e) { console.error(getErrorMessage(e)); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: inner catch variable also flagged when outer has JSON.stringify", () => { + cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(JSON.stringify(inner)); } }`, + errors: [ + { + messageId: "jsonStringifyError", + data: { errorVar: "inner" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "inner" }, + output: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(getErrorMessage(inner)); } }`, + }, + ], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/no-json-stringify-error.ts b/eslint-factory/src/rules/no-json-stringify-error.ts new file mode 100644 index 00000000000..6da57870020 --- /dev/null +++ b/eslint-factory/src/rules/no-json-stringify-error.ts @@ -0,0 +1,131 @@ +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}`); + +interface ErrorScope { + varName: string; + isSentinel: boolean; +} + +/** + * Returns true when the function is passed directly as the first argument to a + * `.catch()` call. Named-reference handlers (for example `p.catch(handler)`) + * are intentionally out of scope. + */ +function isCatchCallback(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean { + const parent = node.parent; + if (!parent || parent.type !== AST_NODE_TYPES.CallExpression) return false; + const callee = parent.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false; + const prop = callee.property; + return prop.type === AST_NODE_TYPES.Identifier && prop.name === "catch" && parent.arguments[0] === node; +} + +export const noJsonStringifyErrorRule = createRule({ + name: "no-json-stringify-error", + meta: { + type: "problem", + hasSuggestions: true, + docs: { + description: "Disallow JSON.stringify() on caught error variables — Error properties (message, stack, etc.) are non-enumerable and produce {} silently", + }, + schema: [], + messages: { + jsonStringifyError: + "JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties (message, stack, etc.) are non-enumerable. Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs or explicitly serialize a guarded value after narrowing it.", + useGetErrorMessage: "Replace with getErrorMessage({{errorVar}}) — ensure getErrorMessage is imported from error_helpers.cjs.", + }, + }, + defaultOptions: [], + create(context) { + // Stack tracking caught error variable names. + // Each scope entry holds varName (empty string for sentinel) and isSentinel flag. + const scopeStack: ErrorScope[] = []; + + function getCaughtVarNames(): Set { + const names = new Set(); + // Walk from the innermost active scope outward and stop at the first + // sentinel so non-.catch() callbacks cannot see shadowed catch vars. + for (let i = scopeStack.length - 1; i >= 0; i--) { + const scope = scopeStack[i]; + if (scope.isSentinel) break; + if (scope.varName) names.add(scope.varName); + } + return names; + } + + function enterFunction(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): void { + if (isCatchCallback(node)) { + const params = node.params; + if (params.length === 1 && params[0].type === AST_NODE_TYPES.Identifier) { + scopeStack.push({ varName: params[0].name, isSentinel: false }); + } else { + // No-param or destructuring: push sentinel so outer frames are not affected + scopeStack.push({ varName: "", isSentinel: true }); + } + } else { + // Non-.catch() function: sentinel to avoid false positives from shadowed names + scopeStack.push({ varName: "", isSentinel: true }); + } + } + + function exitFunction(): void { + scopeStack.pop(); + } + + return { + // Track catch clause parameters + CatchClause(node) { + const param = node.param; + if (!param || param.type !== AST_NODE_TYPES.Identifier) { + scopeStack.push({ varName: "", isSentinel: true }); + } else { + scopeStack.push({ varName: param.name, isSentinel: false }); + } + }, + "CatchClause:exit"() { + scopeStack.pop(); + }, + + // Track .catch() callback parameters + ArrowFunctionExpression: enterFunction, + "ArrowFunctionExpression:exit": exitFunction, + FunctionExpression: enterFunction, + "FunctionExpression:exit": exitFunction, + + // Detect JSON.stringify(caughtErrorVar, ...) + CallExpression(node) { + const caughtNames = getCaughtVarNames(); + if (caughtNames.size === 0) return; + + const callee = node.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression) return; + if (callee.computed) return; + const obj = callee.object; + const prop = callee.property; + if (obj.type !== AST_NODE_TYPES.Identifier || obj.name !== "JSON") return; + if (prop.type !== AST_NODE_TYPES.Identifier || prop.name !== "stringify") return; + + const firstArg = node.arguments[0]; + if (!firstArg || firstArg.type !== AST_NODE_TYPES.Identifier) return; + if (!caughtNames.has(firstArg.name)) return; + + const errorVar = firstArg.name; + context.report({ + node, + messageId: "jsonStringifyError", + data: { errorVar }, + suggest: [ + { + messageId: "useGetErrorMessage" as const, + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(node, `getErrorMessage(${errorVar})`); + }, + }, + ], + }); + }, + }; + }, +});