Repository navigation
fix(javascript): reject an http method token Node refuses at the call site #9683
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
881cfdf
76926ce
2e36db2
28e9b30
afe3cae
4a08abb
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 |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { expect, it } from "vitest"; | ||
|
|
||
| import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js"; | ||
|
|
||
| /** | ||
| * `http.request` and `https.request` validate `options.method` with `checkIsHttpToken` | ||
| * before they build the request, so a literal outside the token set throws | ||
| * `ERR_INVALID_HTTP_TOKEN` from the call itself and no request exists. | ||
| * | ||
| * Measured on Node 24.22.1: `"GET"`, `"get"`, `"M-SEARCH"`, `"X"` and the punctuation set | ||
| * `"!#$%&'*+-.^_`|~"` all construct a request; `"BAD METHOD"`, `"G/ET"`, `"G:ET"`, | ||
| * `"G,ET"`, `"GET "`, `" "` and `"GéT"` throw. An empty string is accepted, because Node | ||
| * reads `""` as "not given" and falls back to `GET`. | ||
| */ | ||
| const requestsFor = (body: string) => | ||
| analyzeJavaScriptSemantics( | ||
| `import http from "node:http";\nimport https from "node:https";\nfunction f() { ${body}; }`, | ||
| ).requestOperations; | ||
|
|
||
| it.each([ | ||
| [`http.request({ host: "h", path: "/", method: "BAD METHOD" })`], | ||
| [`http.request({ host: "h", path: "/", method: "G/ET" })`], | ||
| [`http.request({ host: "h", path: "/", method: "G:ET" })`], | ||
| [`http.request({ host: "h", path: "/", method: "GET " })`], | ||
| [`http.request({ host: "h", path: "/", method: " " })`], | ||
| [`http.request({ host: "h", path: "/", method: "GéT" })`], | ||
| [`https.request({ host: "h", path: "/", method: "BAD METHOD" })`], | ||
| [`http.get({ host: "h", path: "/", method: "BAD METHOD" })`], | ||
| ] as const)("emits no request for a rejected method token: %s", (call) => { | ||
| expect(requestsFor(call)).toEqual([]); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [`http.request({ host: "h", path: "/", method: "GET" })`], | ||
| [`http.request({ host: "h", path: "/", method: "get" })`], | ||
| [`http.request({ host: "h", path: "/", method: "M-SEARCH" })`], | ||
| [`http.request({ host: "h", path: "/", method: "X" })`], | ||
| // Node reads an empty method as "not given" and falls back to GET. | ||
| [`http.request({ host: "h", path: "/", method: "" })`], | ||
| [`http.request({ host: "h", path: "/" })`], | ||
| // `fetch` reads a different member and validates no token. | ||
| [`fetch("http://h/", { method: "BAD METHOD" })`], | ||
| // A binding is not a literal, so this file leaves the answer to the runtime. | ||
| [`const m = "BAD METHOD"; http.request({ host: "h", method: m })`], | ||
| ] as const)("keeps the request for an accepted method: %s", (call) => { | ||
| expect(requestsFor(call)).toHaveLength(1); | ||
| }); | ||
|
|
||
| it.each([ | ||
| // An erased TypeScript assertion is the same object at runtime, so the method is still | ||
| // validated. Measured on Node 24.22.1, this throws ERR_INVALID_HTTP_TOKEN. | ||
| [ | ||
| `http.request(({ host: "h", method: "BAD METHOD" } as any))`, | ||
| `http.request(({ host: "h", method: "BAD METHOD" }) as any)`, | ||
| ], | ||
| ] as const)( | ||
| "reads the options through an erased assertion: %s", | ||
| (_name, call) => { | ||
| expect( | ||
| analyzeJavaScriptSemantics( | ||
| `import http from "node:http";\nfunction f() { ${call}; }`, | ||
| ).requestOperations, | ||
| ).toEqual([]); | ||
| }, | ||
| ); | ||
|
|
||
| it("keeps the request when a leading spread shifts the options slot", () => { | ||
| // The spread supplies the options and the callback, so the trailing object is ignored | ||
| // and Node builds a GET request. Measured on Node 24.22.1. | ||
| const operations = analyzeJavaScriptSemantics( | ||
| `import http from "node:http";\nfunction f() { const a = [{ host: "h" }, () => {}]; http.request(...a, { method: "BAD METHOD" }); }`, | ||
| ).requestOperations; | ||
| expect(operations).toHaveLength(1); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ import { | |
| javaScriptExpressionWrapperTarget, | ||
| readExactJavaScriptLiteral, | ||
| semanticStaticPropertyKey, | ||
| staticObjectProperty, | ||
| semanticStaticPropertyName, | ||
| unwrapJavaScriptExpression, | ||
| } from "./javascriptAstValues.js"; | ||
|
|
@@ -271,6 +272,9 @@ const collectRequests = ( | |
| !URL.canParse(endpoint) | ||
| ) | ||
| return; | ||
| // `http`/`https` validate `options.method` with `checkIsHttpToken` before the request | ||
| // exists, so a literal outside the token set throws from the call itself. | ||
| if (rejectsHttpMethodLiteral(node, method)) return; | ||
| output.push({ | ||
| node, | ||
| operation: { | ||
|
|
@@ -579,6 +583,57 @@ const requestFields = ( | |
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * The characters RFC 9110 allows in a method token, which is what Node's | ||
| * `checkIsHttpToken` accepts. | ||
| * | ||
| * Measured on Node 24.22.1, `method: "M-SEARCH"`, `"X"` and the punctuation set | ||
| * `"!#$%&'*+-.^_` | "~"` all construct a request, while `"G/ET"`, `"G:ET"`, `"G(ET"`, | ||
| * `"G,ET"`, `"G"ET"`, `"GéT"`, `" "` and `"\\t"` throw `ERR_INVALID_HTTP_TOKEN`. The set is | ||
| * a closed character class rather than a list of verbs, so `"GET "` fails on the trailing | ||
| * space and `"get"` is accepted. | ||
| */ | ||
| const HTTP_TOKEN = /^[!#$%&'*+\-.^_`|~0-9A-Za-z]*$/u; | ||
|
|
||
| /** | ||
| * Whether a literal `options.method` makes `http.request` throw before any request exists. | ||
| * | ||
| * `http` and `https` validate the method with `checkIsHttpToken` before they build the | ||
| * request, so a literal outside `HTTP_TOKEN` throws `ERR_INVALID_HTTP_TOKEN` from the call | ||
| * itself. Measured on Node 24.22.1, an empty string is accepted: Node treats `""` as "not | ||
| * given" and falls back to `GET`. A non-string value throws `ERR_INVALID_ARG_TYPE` too, but | ||
| * it is left undecided here because the reported fields do not describe the type check. | ||
| */ | ||
| const rejectsHttpMethodLiteral = ( | ||
| node: t.CallExpression | t.OptionalCallExpression | t.NewExpression, | ||
| method: JavaScriptSemanticRequestOperation["method"], | ||
| ): boolean => { | ||
| // `fetch` and `WebSocket` read a different member and do not validate a token. | ||
| if (method !== "get" && method !== "request") return false; | ||
| // A spread before the options slot can supply several runtime arguments, so a later | ||
| // syntactic object is not necessarily the options Node reads. Measured on Node | ||
| // 24.22.1, `http.request(...[{host, path}, () => {}], {method: "BAD METHOD"})` returns | ||
| // a GET request, so the trailing object is ignored and the call must be kept. | ||
| if (node.arguments.some((argument) => t.isSpreadElement(argument))) | ||
| return false; | ||
| // The value is unwrapped before the slot is chosen, because an erased TypeScript | ||
| // assertion is the same object at runtime: measured on Node 24.22.1, | ||
| // `http.request({method: "BAD METHOD"} as any)` throws ERR_INVALID_HTTP_TOKEN. | ||
| const first = node.arguments[0]; | ||
| const firstValue = | ||
| first === undefined ? undefined : unwrapJavaScriptExpression(first).node; | ||
| const options = | ||
| firstValue !== undefined && t.isObjectExpression(firstValue) | ||
| ? firstValue | ||
| : unwrapJavaScriptExpression(node.arguments[1] ?? first!).node; | ||
|
XiaoBinGan marked this conversation as resolved.
Resolved
Comment on lines
+710
to
+713
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.
A falsy HTTPS head is fresh evidence beyond the handled callback overload: for AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| if (!t.isObjectExpression(options)) return false; | ||
| const expression = options; | ||
| const lookup = staticObjectProperty(expression, "method"); | ||
|
XiaoBinGan marked this conversation as resolved.
Resolved
|
||
| if (lookup.status !== "explicit") return false; | ||
| const literal = readExactJavaScriptLiteral(lookup.value); | ||
| if (!literal.found || typeof literal.value !== "string") return false; | ||
| return literal.value !== "" && !HTTP_TOKEN.test(literal.value); | ||
| }; | ||
| const requestEndpoint = ( | ||
| node: t.CallExpression | t.OptionalCallExpression | t.NewExpression, | ||
| ): string | null => | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.