From 6b79e396cca48fd45d930c488823baf4a07eb9cb Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 08:57:53 +0000 Subject: [PATCH 1/2] Add require-fs-chmod-try-catch ESLint rule for actions/setup/js Flags unguarded fs.chmodSync/fs.fchmodSync calls (throw on missing files, invalid descriptors, or permission errors) with an auto-fix suggestion that wraps them in try/catch and preserves the error as { cause }. Mined from actions/setup/js: fs.chmodSync/fchmodSync is used 18 times to lock down permissions on files/dirs that may hold secrets, but 2 call sites (create_prompt.cjs, start_mcp_gateway.cjs) were unguarded. Adjacent methods (readFileSync/writeFileSync/statSync/etc.) already have dedicated try/catch rules; chmod was a gap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eslint-factory/eslint.config.cjs | 1 + eslint-factory/src/index.ts | 2 + .../rules/require-fs-chmod-try-catch.test.ts | 87 +++++++++++++++++++ .../src/rules/require-fs-chmod-try-catch.ts | 78 +++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts create mode 100644 eslint-factory/src/rules/require-fs-chmod-try-catch.ts diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 0905e07fa1f..5d2371bbaa4 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -48,6 +48,7 @@ module.exports = [ "gh-aw-custom/require-execsync-try-catch": "warn", "gh-aw-custom/require-execfilesync-try-catch": "warn", "gh-aw-custom/require-fs-io-try-catch": "warn", + "gh-aw-custom/require-fs-chmod-try-catch": "warn", "gh-aw-custom/no-setfailed-then-exit-zero": "warn", "gh-aw-custom/no-err-stack-then-string-fallback": "warn", "gh-aw-custom/no-caught-error-interpolation": "warn", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 3fc87c4e13b..776e29bd948 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -59,6 +59,7 @@ import { noStringFallbackForNonStringMessageRule } from "./rules/no-string-fallb import { requireGetExecOutputExitCodeCheckRule } from "./rules/require-getexecoutput-exitcode-check"; import { preferActionsExecOverChildProcessRule } from "./rules/prefer-actions-exec-over-child-process"; import { noMisplacedErrorCodeDefinitionRule } from "./rules/no-misplaced-error-code-definition"; +import { requireFsChmodTryCatchRule } from "./rules/require-fs-chmod-try-catch"; const plugin = { meta: { @@ -127,6 +128,7 @@ const plugin = { "require-getexecoutput-exitcode-check": requireGetExecOutputExitCodeCheckRule, "prefer-actions-exec-over-child-process": preferActionsExecOverChildProcessRule, "no-misplaced-error-code-definition": noMisplacedErrorCodeDefinitionRule, + "require-fs-chmod-try-catch": requireFsChmodTryCatchRule, }, }; diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts new file mode 100644 index 00000000000..c12c997c54a --- /dev/null +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts @@ -0,0 +1,87 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { requireFsChmodTryCatchRule } from "./require-fs-chmod-try-catch"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +function expectedWrapInTryCatchSuggestion(method: string, statement: string, prefix = "") { + return { + messageId: "wrapInTryCatch", + output: + `${prefix}try {\n` + + ` ${statement}\n` + + `} catch (err) {\n` + + ` // TODO: handle permission-change failure for this fs.${method} call.\n` + + ` throw new Error(\n` + + ` "fs.${method} failed: " + (err instanceof Error ? err.message : String(err)),\n` + + ` { cause: err },\n` + + ` );\n` + + `}`, + }; +} + +describe("require-fs-chmod-try-catch", () => { + it("valid: fs.chmodSync and fs.fchmodSync inside try block pass", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [ + `try { fs.chmodSync(path, 0o600); } catch (e) {}`, + `try { fs.fchmodSync(fd, 0o600); } catch (e) {}`, + ], + invalid: [], + }); + }); + + it("valid: other fs methods and non-fs identifiers are ignored", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [`fs.existsSync(path);`, `fs.readFileSync(path, "utf8");`, `fs.statSync(path);`, `mockFs.chmodSync(path, 0o600);`, `storage.fchmodSync(fd, 0o600);`], + invalid: [], + }); + }); + + it("invalid: fs.chmodSync outside try/catch is flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `fs.chmodSync(path, 0o600);`, + errors: [{ messageId: "requireTryCatch", data: { method: "chmodSync", arg: "path" }, suggestions: [expectedWrapInTryCatchSuggestion("chmodSync", "fs.chmodSync(path, 0o600);")] }], + }, + ], + }); + }); + + it("invalid: fs.fchmodSync outside try/catch is flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `fs.fchmodSync(fd, 0o600);`, + errors: [{ messageId: "requireTryCatch", data: { method: "fchmodSync", arg: "fd" }, suggestions: [expectedWrapInTryCatchSuggestion("fchmodSync", "fs.fchmodSync(fd, 0o600);")] }], + }, + ], + }); + }); + + it("invalid: fs.chmodSync after an unrelated try block is still flagged", () => { + cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { + valid: [], + invalid: [ + { + code: `try { doSomethingElse(); } catch (e) {}\nfs.chmodSync(dir, 0o700);`, + errors: [ + { + messageId: "requireTryCatch", + data: { method: "chmodSync", arg: "dir" }, + suggestions: [expectedWrapInTryCatchSuggestion("chmodSync", "fs.chmodSync(dir, 0o700);", "try { doSomethingElse(); } catch (e) {}\n")], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts new file mode 100644 index 00000000000..09bafb80546 --- /dev/null +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts @@ -0,0 +1,78 @@ +import { ESLintUtils } from "@typescript-eslint/utils"; +import { buildTryCatchSuggestion, createFsSyncMethodResolver, findEnclosingStatement, isInsideTryBlock } from "./try-catch-rule-utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +// fs.chmodSync and fs.fchmodSync throw synchronously on missing files/descriptors, permission +// errors (EPERM), or unsupported filesystems (EINVAL on some mounts). actions/setup/js relies on +// these calls to lock down permissions on files that may contain secrets (config files, staged +// artifacts, temp directories); an unguarded throw here crashes the action with a generic stack +// trace instead of a specific message that preserves the original error as `{ cause }`. +const FS_CHMOD_METHODS = new Set(["chmodSync", "fchmodSync"]); + +export const requireFsChmodTryCatchRule = createRule({ + name: "require-fs-chmod-try-catch", + meta: { + type: "problem", + hasSuggestions: true, + docs: { + description: + "Require fs.chmodSync and fs.fchmodSync calls in actions/setup/js scripts to be wrapped in try/catch. " + + "These methods throw synchronously on missing files, invalid descriptors, or permission errors; " + + "without a call-site try/catch, the entrypoint-level catch produces a generic engine-level stack instead of a specific message that preserves the error as `{ cause }`.", + }, + schema: [], + messages: { + requireTryCatch: + "Wrap fs.{{method}}({{arg}}) in try/catch — synchronous chmod calls throw on missing files, invalid descriptors, " + + "or permission errors; without a call-site try/catch, you lose the original error context and get a generic engine-level stack instead of a specific message with `{ cause }`.", + wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + const resolveFsChmodMethod = createFsSyncMethodResolver(sourceCode, FS_CHMOD_METHODS, { allowUnboundFsIdentifier: true }); + + return { + CallExpression(node) { + const methodName = resolveFsChmodMethod(node); + + if (!methodName) return; + + if (isInsideTryBlock(sourceCode, node)) return; + + const argText = node.arguments.length > 0 ? sourceCode.getText(node.arguments[0]) : ""; + const method = methodName; + const stmt = findEnclosingStatement(sourceCode, node); + + context.report({ + node, + messageId: "requireTryCatch", + data: { method, arg: argText }, + suggest: stmt + ? [ + { + messageId: "wrapInTryCatch", + fix(fixer) { + const stmtText = sourceCode.getText(stmt); + const startLine = stmt.loc?.start.line; + const stmtLine = startLine !== undefined ? (sourceCode.lines[startLine - 1] ?? "") : ""; + const indent = stmtLine.match(/^(\s*)/)?.[1] ?? ""; + return fixer.replaceText( + stmt, + buildTryCatchSuggestion(stmtText, { + indent, + todoComment: `TODO: handle permission-change failure for this fs.${method} call.`, + errorPrefix: `fs.${method} failed: `, + }) + ); + }, + }, + ] + : [], + }); + }, + }; + }, +}); From dd5ca6b8d95577726a28c8f83ab4ac180243f41b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:56:37 +0000 Subject: [PATCH 2/2] Document and simplify chmod ESLint rule Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- eslint-factory/README.md | 16 ++++++++++++++++ .../src/rules/require-fs-chmod-try-catch.test.ts | 16 ++-------------- .../src/rules/require-fs-chmod-try-catch.ts | 15 ++------------- eslint-factory/src/rules/try-catch-rule-utils.ts | 4 ++-- 4 files changed, 22 insertions(+), 29 deletions(-) diff --git a/eslint-factory/README.md b/eslint-factory/README.md index 43c617a58be..3abc0ec3830 100644 --- a/eslint-factory/README.md +++ b/eslint-factory/README.md @@ -45,6 +45,7 @@ This project hosts custom ESLint linters for `/actions/setup/js`. | [`require-fetch-response-body-try-catch`](#require-fetch-response-body-try-catch) | Require try/catch around `.json()` or `.text()` on Responses from `fetch(...)` | | [`require-fetch-timeout`](#require-fetch-timeout) | Require `fetch(...)` calls to include a non-nullish abort `signal` option | | [`require-fetch-try-catch`](#require-fetch-try-catch) | Require try/catch around awaited `fetch(...)` calls, including chained promise forms without rejection handlers | +| [`require-fs-chmod-try-catch`](#require-fs-chmod-try-catch) | Require try/catch around `fs.chmodSync` and `fs.fchmodSync` | | [`require-fs-close-sync`](#require-fs-close-sync) | Require `fs.openSync(...)` file descriptors to be closed with `fs.closeSync(fd)` in the same function | | [`require-fs-io-try-catch`](#require-fs-io-try-catch) | Require try/catch around `fs.statSync`, `readdirSync`, `copyFileSync`, `unlinkSync`, and `renameSync` | | [`require-fs-sync-try-catch`](#require-fs-sync-try-catch) | Require try/catch around `fs.readFileSync`, `writeFileSync`, and `appendFileSync` | @@ -396,6 +397,21 @@ try { } ``` +### `require-fs-chmod-try-catch` + +Require `fs.chmodSync` and `fs.fchmodSync` calls to be wrapped in `try/catch`. + +Why: these calls can throw for missing files or descriptors, permission errors, and unsupported filesystems. A call-site catch preserves useful error context. + +**Detected forms:** +- `fs.chmodSync(path, mode)` and `fs["chmodSync"](path, mode)`. +- `fs.fchmodSync(fd, mode)`. +- Bindings imported or required from `fs` / `node:fs`, including destructured bindings. + +**Out of scope:** +- Objects that are not resolved to the Node `fs` / `node:fs` module. +- `try { ... } finally { ... }` without a `catch` clause is still flagged. + ### `require-fs-close-sync` Require file descriptors returned by `fs.openSync(...)` to be closed with `fs.closeSync(fd)` in the same enclosing function. diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts index c12c997c54a..f13d09b8c0e 100644 --- a/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.test.ts @@ -12,26 +12,14 @@ const cjsRuleTester = new RuleTester({ function expectedWrapInTryCatchSuggestion(method: string, statement: string, prefix = "") { return { messageId: "wrapInTryCatch", - output: - `${prefix}try {\n` + - ` ${statement}\n` + - `} catch (err) {\n` + - ` // TODO: handle permission-change failure for this fs.${method} call.\n` + - ` throw new Error(\n` + - ` "fs.${method} failed: " + (err instanceof Error ? err.message : String(err)),\n` + - ` { cause: err },\n` + - ` );\n` + - `}`, + output: `${prefix}try {\n` + ` ${statement}\n` + `} catch (err) {\n` + ` throw new Error(\n` + ` "fs.${method} failed: " + (err instanceof Error ? err.message : String(err)),\n` + ` { cause: err },\n` + ` );\n` + `}`, }; } describe("require-fs-chmod-try-catch", () => { it("valid: fs.chmodSync and fs.fchmodSync inside try block pass", () => { cjsRuleTester.run("require-fs-chmod-try-catch", requireFsChmodTryCatchRule, { - valid: [ - `try { fs.chmodSync(path, 0o600); } catch (e) {}`, - `try { fs.fchmodSync(fd, 0o600); } catch (e) {}`, - ], + valid: [`try { fs.chmodSync(path, 0o600); } catch (e) {}`, `try { fs.fchmodSync(fd, 0o600); } catch (e) {}`], invalid: [], }); }); diff --git a/eslint-factory/src/rules/require-fs-chmod-try-catch.ts b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts index 09bafb80546..792d2e29f30 100644 --- a/eslint-factory/src/rules/require-fs-chmod-try-catch.ts +++ b/eslint-factory/src/rules/require-fs-chmod-try-catch.ts @@ -3,11 +3,6 @@ import { buildTryCatchSuggestion, createFsSyncMethodResolver, findEnclosingState const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); -// fs.chmodSync and fs.fchmodSync throw synchronously on missing files/descriptors, permission -// errors (EPERM), or unsupported filesystems (EINVAL on some mounts). actions/setup/js relies on -// these calls to lock down permissions on files that may contain secrets (config files, staged -// artifacts, temp directories); an unguarded throw here crashes the action with a generic stack -// trace instead of a specific message that preserves the original error as `{ cause }`. const FS_CHMOD_METHODS = new Set(["chmodSync", "fchmodSync"]); export const requireFsChmodTryCatchRule = createRule({ @@ -16,16 +11,11 @@ export const requireFsChmodTryCatchRule = createRule({ type: "problem", hasSuggestions: true, docs: { - description: - "Require fs.chmodSync and fs.fchmodSync calls in actions/setup/js scripts to be wrapped in try/catch. " + - "These methods throw synchronously on missing files, invalid descriptors, or permission errors; " + - "without a call-site try/catch, the entrypoint-level catch produces a generic engine-level stack instead of a specific message that preserves the error as `{ cause }`.", + description: "Require fs.chmodSync and fs.fchmodSync calls in actions/setup/js scripts to be wrapped in try/catch.", }, schema: [], messages: { - requireTryCatch: - "Wrap fs.{{method}}({{arg}}) in try/catch — synchronous chmod calls throw on missing files, invalid descriptors, " + - "or permission errors; without a call-site try/catch, you lose the original error context and get a generic engine-level stack instead of a specific message with `{ cause }`.", + requireTryCatch: "Wrap fs.{{method}}({{arg}}) in try/catch.", wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.", }, }, @@ -63,7 +53,6 @@ export const requireFsChmodTryCatchRule = createRule({ stmt, buildTryCatchSuggestion(stmtText, { indent, - todoComment: `TODO: handle permission-change failure for this fs.${method} call.`, errorPrefix: `fs.${method} failed: `, }) ); diff --git a/eslint-factory/src/rules/try-catch-rule-utils.ts b/eslint-factory/src/rules/try-catch-rule-utils.ts index 2214f82bfc8..e9ff1f91f0a 100644 --- a/eslint-factory/src/rules/try-catch-rule-utils.ts +++ b/eslint-factory/src/rules/try-catch-rule-utils.ts @@ -291,7 +291,7 @@ export function createFsSyncMethodResolver(sourceCode: TSESLint.SourceCode, fsSy type TryCatchSuggestionOptions = { indent: string; - todoComment: string; + todoComment?: string; errorPrefix: string; }; @@ -311,7 +311,7 @@ export function buildTryCatchSuggestion(stmtText: string, options: TryCatchSugge "try {", indentedStatement, `${indent}} catch (err) {`, - `${indent} // ${todoComment}`, + ...(todoComment ? [`${indent} // ${todoComment}`] : []), `${indent} throw new Error(`, `${indent} "${errorPrefix}" + (err instanceof Error ? err.message : String(err)),`, `${indent} { cause: err },`,