-
Notifications
You must be signed in to change notification settings - Fork 455
Refine interpolated-route diagnostics for opaque Octokit route helpers #45364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1c71ca3
f642b7d
5c27ee1
ec16b2b
9f69216
a96a893
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,32 @@ describe("no-github-request-interpolated-route", () => { | |
| }); | ||
| }); | ||
|
|
||
| it("invalid: opaque whole-route helpers are flagged with tailored guidance", () => { | ||
| cjsRuleTester.run("no-github-request-interpolated-route", noGithubRequestInterpolatedRouteRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: "function addReaction(endpoint) { github.request(`POST ${endpoint}`, { content: '+1' }); }", | ||
| errors: [ | ||
| { | ||
| messageId: "opaqueWholeRoute", | ||
| data: { kind: "template literal with interpolations", client: "github" }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: 'function addComment(endpoint) { github.request("POST " + endpoint, { body }); }', | ||
| errors: [ | ||
| { | ||
| messageId: "opaqueWholeRoute", | ||
| data: { kind: "string concatenation expression", client: "github" }, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing regression test for the critical boundary between 💡 Suggested testit("boundary: multi-expression template stays interpolatedRoute, not opaqueWholeRoute", () => {
cjsRuleTester.run("no-github-request-interpolated-route", noGithubRequestInterpolatedRouteRule, {
valid: [],
invalid: [
{
code: "github.request(`POST /repos/${owner}/${repo}/reactions`, { content: '+1' });",
errors: [{ messageId: "interpolatedRoute" }],
},
],
});
});Without this, a future change to |
||
|
|
||
| it("valid: simple aliases bound to non-Octokit sources are not flagged", () => { | ||
| cjsRuleTester.run("no-github-request-interpolated-route", noGithubRequestInterpolatedRouteRule, { | ||
| valid: [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh | |
|
|
||
| const OCTOKIT_CLIENT_NAMES = new Set(["github", "octokit", "githubClient", "octokitClient"]); | ||
| const GET_OCTOKIT_MEMBER_OBJECT_NAMES = new Set(["github", "actions"]); | ||
| const HTTP_METHOD_PREFIXES = new Set(["GET ", "POST ", "PUT ", "PATCH ", "DELETE ", "HEAD ", "OPTIONS "]); | ||
|
|
||
| /** | ||
| * Returns true when the node is a template literal that contains at least one | ||
|
|
@@ -39,6 +40,54 @@ function isStringConcatenation(node: TSESTree.Node): boolean { | |
| return node.type === "BinaryExpression" && node.operator === "+" && !isStaticRouteExpression(node); | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when `text` is an exact HTTP-method route prefix including the | ||
| * required trailing space, such as `GET ` or `POST `. | ||
| */ | ||
| function isValidHttpMethodPrefix(text: string | null | undefined): boolean { | ||
| return typeof text === "string" && HTTP_METHOD_PREFIXES.has(text); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the human-readable route-expression kind string used in diagnostics, | ||
| * or null when `node` is not one of the interpolated route shapes this rule | ||
| * reports on. | ||
| */ | ||
| function getInterpolatedRouteKind(node: TSESTree.Node): string | null { | ||
| if (isInterpolatedTemplateLiteral(node)) return "template literal with interpolations"; | ||
| if (isStringConcatenation(node)) return "string concatenation expression"; | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when the route expression is exactly an HTTP method plus a | ||
| * single opaque route expression, such as: | ||
| * - `POST ${endpoint}` | ||
| * - `"POST " + endpoint` | ||
| * | ||
| * This shape differs from value-into-path interpolation because there is no | ||
| * known static path at the call site to rewrite with `{placeholder}` segments. | ||
| */ | ||
| function isOpaqueWholeRouteInterpolation(node: TSESTree.Node): boolean { | ||
| if (node.type === "TemplateLiteral") { | ||
| const hasTwoQuasis = node.quasis.length === 2; | ||
| const hasSingleExpression = node.expressions.length === 1; | ||
| const hasMethodPrefix = hasTwoQuasis && isValidHttpMethodPrefix(node.quasis[0].value.cooked); | ||
| const hasEmptyTrailingQuasi = hasTwoQuasis && node.quasis[1].value.cooked === ""; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] 💡 Suggested fixconst hasEmptyTrailingQuasi =
hasTwoQuasis && node.quasis[1].value.cooked === "";→ const hasEmptyTrailingQuasi =
hasTwoQuasis && (node.quasis[1].value.cooked ?? null) === "";Or more defensively: const trailingCooked = node.quasis[1]?.value.cooked;
const hasEmptyTrailingQuasi = trailingCooked === "";Also apply the same null-guard to the @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unguarded 💡 Suggested fixconst hasEmptyTrailingQuasi = hasTwoQuasis && (node.quasis[1].value.cooked === "" || node.quasis[1].value.cooked == null);Or extract a helper: function isCookedEmpty(quasi: TSESTree.TemplateElement): boolean {
return quasi.value.cooked === "" || quasi.value.cooked == null;
}Template literals with invalid escapes (e.g., |
||
| const isDynamicWholeRoute = hasSingleExpression && !isStaticRouteExpression(node.expressions[0]); | ||
| return hasMethodPrefix && hasEmptyTrailingQuasi && isDynamicWholeRoute; | ||
| } | ||
|
|
||
| if (node.type === "BinaryExpression" && node.operator === "+") { | ||
| if (node.left.type !== "Literal" || typeof node.left.value !== "string") return false; | ||
| const hasMethodPrefix = isValidHttpMethodPrefix(node.left.value); | ||
| const isRightDynamic = !isStaticRouteExpression(node.right); | ||
| return hasMethodPrefix && isRightDynamic; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BinaryExpression opaque-route check misclassifies right-associative concatenations: 💡 Details and suggested fixThe distinction that makes if (node.type === "BinaryExpression" && node.operator === "+") {
if (node.left.type !== "Literal" || typeof node.left.value !== "string") return false;
const hasMethodPrefix = isValidHttpMethodPrefix(node.left.value);
// Require the right side to be a single identifier (or member expression), not another concatenation
const isRightSingleDynamic = node.right.type === "Identifier" || node.right.type === "MemberExpression";
return hasMethodPrefix && isRightSingleDynamic;
}Add a regression test: {
code: 'github.request("POST " + owner + "/" + repo, { });',
errors: [{ messageId: "interpolatedRoute" }],
} |
||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when `node` is the `context.github` member expression. | ||
| */ | ||
|
|
@@ -88,10 +137,13 @@ export const noGithubRequestInterpolatedRouteRule = createRule({ | |
| "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.', | ||
| "Use the typed placeholder form for value interpolation, or thread a typed route string from the caller when the entire route is dynamic.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| opaqueWholeRoute: | ||
| "Avoid using an opaque whole-route {{kind}} as the route argument of {{client}}.request(). " + | ||
| "When the entire route is dynamic, pass a typed route string from the caller instead of interpolating or concatenating a route variable.", | ||
| interpolatedRoute: | ||
| "Avoid using a {{kind}} as the route argument of {{client}}.request(). " + | ||
| 'Use the typed placeholder form instead — e.g. github.request("GET /repos/{owner}/{repo}", { owner, repo }) — ' + | ||
|
|
@@ -169,22 +221,23 @@ export const noGithubRequestInterpolatedRouteRule = createRule({ | |
| const firstArg = node.arguments[0]; | ||
| if (!firstArg) return; | ||
|
|
||
| if (isInterpolatedTemplateLiteral(firstArg)) { | ||
| const routeKind = getInterpolatedRouteKind(firstArg); | ||
| if (!routeKind) return; | ||
|
|
||
| if (isOpaqueWholeRouteInterpolation(firstArg)) { | ||
| context.report({ | ||
| node: firstArg, | ||
| messageId: "interpolatedRoute", | ||
| data: { kind: "template literal with interpolations", client: clientName }, | ||
| messageId: "opaqueWholeRoute", | ||
| data: { kind: routeKind, client: clientName }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (isStringConcatenation(firstArg)) { | ||
| context.report({ | ||
| node: firstArg, | ||
| messageId: "interpolatedRoute", | ||
| data: { kind: "string concatenation expression", client: clientName }, | ||
| }); | ||
| } | ||
| context.report({ | ||
| node: firstArg, | ||
| messageId: "interpolatedRoute", | ||
| data: { kind: routeKind, client: clientName }, | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
|
|
||
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.
[/tdd] New tests only exercise the
githubclient — the existinginterpolatedRoutetests cover all four known client names (github,octokit,githubClient,octokitClient), but the newopaqueWholeRouteblock tests onlygithub. If a client name is added or removed in future, this gap won't surface.💡 Suggested additions
Add at least one entry per additional client name, mirroring the pattern in the
interpolatedRoutetest block:This gives the test suite the same breadth as the existing
interpolatedRoutecoverage.@copilot please address this.