eslint-factory: scope-aware Octokit client detection in no-github-request-interpolated-route #44706
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| 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}`); | ||
|
|
||
| const OCTOKIT_CLIENT_NAMES = new Set(["github", "octokit", "githubClient", "octokitClient"]); | ||
| const GET_OCTOKIT_MEMBER_OBJECT_NAMES = new Set(["github", "actions"]); | ||
|
|
||
| /** | ||
| * Returns true when the node is a template literal that contains at least one | ||
|
|
@@ -38,14 +39,55 @@ function isStringConcatenation(node: TSESTree.Node): boolean { | |
| return node.type === "BinaryExpression" && node.operator === "+" && !isStaticRouteExpression(node); | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when `node` is the `context.github` member expression. | ||
| */ | ||
| function isContextGithubExpression(node: TSESTree.Node): boolean { | ||
| return ( | ||
| node.type === AST_NODE_TYPES.MemberExpression && !node.computed && node.object.type === AST_NODE_TYPES.Identifier && node.object.name === "context" && node.property.type === AST_NODE_TYPES.Identifier && node.property.name === "github" | ||
| ); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. The const client = actions.getOctokit(token);
client.request(`GET /repos/${owner}/${repo}`);to confirm the branch works as intended. @copilot please address this. |
||
|
|
||
| /** | ||
| * Returns true when a syntactic expression node directly represents a known | ||
| * Octokit client source without scope resolution. Recognizes: | ||
| * - Direct known names: github, octokit, githubClient, octokitClient | ||
| * - `getOctokit(...)` call results (bare or via known module objects, e.g. | ||
| * `github.getOctokit(...)` or `actions.getOctokit(...)`) | ||
| * - `context.github` member expression | ||
| */ | ||
| function isOctokitSourceExpression(node: TSESTree.Node): boolean { | ||
| if (node.type === AST_NODE_TYPES.Identifier && OCTOKIT_CLIENT_NAMES.has(node.name)) return true; | ||
|
|
||
| if (node.type === AST_NODE_TYPES.CallExpression) { | ||
| const callee = node.callee; | ||
| if (callee.type === AST_NODE_TYPES.Identifier && callee.name === "getOctokit") return true; | ||
| if ( | ||
| callee.type === AST_NODE_TYPES.MemberExpression && | ||
| !callee.computed && | ||
| callee.object.type === AST_NODE_TYPES.Identifier && | ||
| GET_OCTOKIT_MEMBER_OBJECT_NAMES.has(callee.object.name) && | ||
| callee.property.type === AST_NODE_TYPES.Identifier && | ||
| callee.property.name === "getOctokit" | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (isContextGithubExpression(node)) return true; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export const noGithubRequestInterpolatedRouteRule = createRule({ | ||
| name: "no-github-request-interpolated-route", | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Disallow template literals with interpolations or string concatenation as the route argument of Octokit.request() calls. " + | ||
| "Octokit clients are detected by well-known names (github, octokit, githubClient, octokitClient), " + | ||
| "identifiers initialized from getOctokit(...) call results, context.github, and simple const aliases of any of these. " + | ||
| 'Use the typed placeholder form instead: "GET /repos/{owner}/{repo}" with a separate params object.', | ||
|
Comment on lines
87
to
91
|
||
| }, | ||
| schema: [], | ||
|
|
@@ -58,23 +100,75 @@ export const noGithubRequestInterpolatedRouteRule = createRule({ | |
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| type SourceCodeScope = ReturnType<typeof sourceCode.getScope>; | ||
|
|
||
| /** | ||
| * Resolves an identifier name to check if it is bound to a known Octokit | ||
| * client source in the visible scope chain. Handles simple single-level | ||
| * assignments: | ||
| * const x = github | ||
| * const x = getOctokit(token) | ||
| * const x = context.github | ||
| */ | ||
| function isIdentifierBoundToOctokitClient(name: string, scopeNode: TSESTree.Node): boolean { | ||
| if (OCTOKIT_CLIENT_NAMES.has(name)) return true; | ||
|
|
||
| let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); | ||
|
Contributor
There was a problem hiding this comment.
💡 Details
The safer and more precise call is to pass the Identifier node itself: function isIdentifierBoundToOctokitClient(
name: string,
scopeNode: TSESTree.Node, // pass the Identifier node, not the CallExpression
): boolean {And in the caller: // In resolveOctokitClientName:
return isIdentifierBoundToOctokitClient(calleeObject.name, calleeObject) ? calleeObject.name : null;
// ^^^^^^^^^^^^^^^^^^^
// Identifier node, not callNodeThis is a low-risk but subtle correctness gap that could surface in non-trivial scope nesting. |
||
| while (scope) { | ||
| const variable = scope.set.get(name); | ||
| if (variable && variable.defs.length > 0) { | ||
| for (const def of variable.defs) { | ||
| if (def.type !== "Variable") continue; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| const declaration = declarator.parent; | ||
| if (!declaration || declaration.type !== AST_NODE_TYPES.VariableDeclaration || declaration.kind !== "const") continue; | ||
| if (declarator.init && isOctokitSourceExpression(declarator.init)) return true; | ||
|
Contributor
There was a problem hiding this comment. ** reassignment makes the Octokit check stale**: only the declaration initializer is inspected, not subsequent write-references, so a reassigned binding produces a false positive (or false negative in the reverse direction). 💡 Details and suggested fixThe current logic only checks let client = getOctokit(token);
client = httpModule; // ← reassigned to non-Octokit
client.request(`GET /repos/${o}/${r}`); // ← still flagged (false positive)And the inverse: let client = http;
client = getOctokit(token); // ← write reference, not a def
client.request(`GET /repos/${o}/${r}`); // ← NOT flagged (false negative)To handle mutable bindings correctly, also check write-references via if (declarator.init && isOctokitSourceExpression(declarator.init)) {
// Only trust const bindings — let/var can be reassigned
const declKind = (def.node as TSESTree.VariableDeclarator).parent?.kind;
if (declKind === 'const') return true;
}This keeps the rule's stated contract ("simple |
||
| } | ||
| return false; | ||
| } | ||
| scope = scope.upper; | ||
| } | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] When the variable is found in scope but has no 💡 Suggested test to surface the issueit('valid: var re-assigned to non-Octokit value is not flagged', () => {
cjsRuleTester.run('...', rule, {
valid: [
// gh was initially octokit but is then overwritten
'var gh = github; gh = someOtherThing; gh.request(`GET /repos/${owner}/${repo}`);',
],
invalid: [],
});
});The PR description scopes this to @copilot please address this. |
||
| } | ||
|
|
||
| /** | ||
| * Returns the display name (for error messages) if the callee object | ||
| * resolves to a known Octokit client, or null otherwise. | ||
| * | ||
| * Recognized shapes: | ||
| * - `<name>.request(...)` where name is in OCTOKIT_CLIENT_NAMES or is a | ||
| * simple alias of an Octokit source (scope-resolved) | ||
| * - `context.github.request(...)` | ||
| */ | ||
| function resolveOctokitClientName(calleeObject: TSESTree.Expression | TSESTree.Super, callNode: TSESTree.Node): string | null { | ||
| if (calleeObject.type === AST_NODE_TYPES.Identifier) { | ||
| return isIdentifierBoundToOctokitClient(calleeObject.name, callNode) ? calleeObject.name : null; | ||
| } | ||
|
|
||
| if (isContextGithubExpression(calleeObject)) { | ||
| return "context.github"; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| const callee = node.callee; | ||
|
|
||
| // Only match <client>.request(...) | ||
| if (callee.type !== AST_NODE_TYPES.MemberExpression) return; | ||
| if (callee.computed) return; | ||
| if (callee.property.type !== AST_NODE_TYPES.Identifier) return; | ||
| if (callee.property.name !== "request") return; | ||
|
|
||
| const clientName = resolveOctokitClientName(callee.object, node); | ||
| if (!clientName) return; | ||
|
|
||
| const firstArg = node.arguments[0]; | ||
| if (!firstArg) return; | ||
|
|
||
| if (isInterpolatedTemplateLiteral(firstArg)) { | ||
| context.report({ | ||
| node: firstArg, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design] Line 46 is very long (170+ characters). The existing helpers in this file each fit comfortably on one readable line. Breaking this into multiple lines would match the surrounding style and improve readability in code review and diffs.
💡 Suggested formatting
@copilot please address this.