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 all 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,173 @@ | ||
| 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); | ||
| }); | ||
|
|
||
| it.each([ | ||
| // A callable first argument is the callback, so Node ignores a later options object. | ||
| [`http.request(() => {}, { method: "BAD METHOD" })`], | ||
| [`http.request(function () {}, { method: "BAD METHOD" })`], | ||
| // A getter runs before the copy and can rewrite the method. | ||
| [`http.request({ get x() { this.method = "GET"; }, method: "BAD METHOD" })`], | ||
| ] as const)( | ||
| "keeps the request for a callable or accessor-written options: %s", | ||
| (call) => { | ||
| expect(requestsFor(call)).toHaveLength(1); | ||
| }, | ||
| ); | ||
|
|
||
| it.each([ | ||
| // A callback after the options does not move the slot; the method in slot 0 is still | ||
| // validated. Measured on Node 24.22.1. | ||
| [`http.request({ method: "BAD METHOD" }, () => {})`], | ||
| // A getter after the member cannot change the value that was already copied. | ||
| [`http.request({ method: "BAD METHOD", get x() { this.method = "GET"; } })`], | ||
| ] as const)( | ||
| "emits no request when the callback or getter follows: %s", | ||
| (call) => { | ||
| expect(requestsFor(call)).toEqual([]); | ||
| }, | ||
| ); | ||
|
|
||
| it("keeps the request when the getter precedes the member", () => { | ||
| // The getter runs before the copy reaches `method`, so it rewrites the source value. | ||
| expect( | ||
| requestsFor( | ||
| `http.request({ get x() { this.method = "GET"; }, method: "BAD METHOD" })`, | ||
| ), | ||
| ).toHaveLength(1); | ||
| }); | ||
|
|
||
| it("keeps the request for a valid options object with a callback", () => { | ||
| expect( | ||
| requestsFor(`http.request({ host: "h", port: 1, path: "/" }, () => {})`), | ||
| ).toHaveLength(1); | ||
| }); | ||
|
|
||
| it.each([ | ||
| // An erased assertion leaves the same function at runtime, so slot 0 is still the callback. | ||
| [`http.request((() => {}) as any, { method: "BAD METHOD" })`], | ||
| // `Object.assign` enumerates integer-index keys before string keys, so getter `0` runs | ||
| // before `method` is copied even though it is written after it. | ||
| [`http.request({ method: "BAD METHOD", get 0() { this.method = "GET"; } })`], | ||
| ] as const)( | ||
| "keeps the request for a callback or integer-key getter: %s", | ||
| (call) => { | ||
| expect(requestsFor(call)).toHaveLength(1); | ||
| }, | ||
| ); | ||
|
|
||
| it("emits no request when a string-key getter follows the member", () => { | ||
| // A string key enumerates in insertion order, so this getter runs after the copy. | ||
| expect( | ||
| requestsFor( | ||
| `http.request({ method: "BAD METHOD", get x() { this.method = "GET"; } })`, | ||
| ), | ||
| ).toEqual([]); | ||
| }); | ||
|
|
||
| it.each([ | ||
| // A redefined key keeps its first insertion position, so this getter enumerates first. | ||
| [ | ||
| `http.request({ x: 0, method: "BAD METHOD", get x() { this.method = "GET"; } })`, | ||
| ], | ||
| // 4294967294 is an array index and enumerates first; 4294967295 is not, and does not. | ||
| [ | ||
| `http.request({ method: "BAD METHOD", get 4294967294() { this.method = "GET"; } })`, | ||
| ], | ||
| // A spread before the options slot can still supply it. | ||
| [`http.request(...[{ method: "BAD METHOD" }])`], | ||
| ] as const)( | ||
| "keeps the request for a reordered or spread-supplied options: %s", | ||
| (call) => { | ||
| expect(requestsFor(call)).toHaveLength(1); | ||
| }, | ||
| ); | ||
|
|
||
| it.each([ | ||
| // A getter on a fresh key enumerates after the member, so the invalid value was copied first. | ||
| [ | ||
| `http.request({ y: 0, method: "BAD METHOD", get x() { this.method = "GET"; } })`, | ||
| ], | ||
| // 4294967295 is not an array index, so it enumerates with the string keys, after `method`. | ||
| [ | ||
| `http.request({ method: "BAD METHOD", get 4294967295() { this.method = "GET"; } })`, | ||
| ], | ||
| // A spread after the options cannot move the slot. | ||
| [`http.request({ method: "BAD METHOD" }, ...[])`], | ||
| ] as const)( | ||
| "emits no request for a getter or spread after the member: %s", | ||
| (call) => { | ||
| expect(requestsFor(call)).toEqual([]); | ||
| }, | ||
| ); | ||
| 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,148 @@ 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. | ||
| */ | ||
| /** | ||
| * Whether a getter is copied before the last write of `member`. | ||
| * | ||
| * `Object.assign` walks the source in **enumeration** order, which is neither source order nor | ||
| * AST order. Two rules decide it, each measured on Node 24.22.1: | ||
| * | ||
| * - An ECMAScript array index (a canonical integer below `2 ** 32 - 1`) enumerates first, | ||
| * ascending. `4294967295` is *not* an array index, so it enumerates with the string keys. | ||
| * - A redefined key keeps its **first** insertion position: `{x: 0, method: "BAD METHOD", | ||
| * get x() {...}}` enumerates the final `x` getter before `method`, so it returns a GET, | ||
| * while the same getter on a fresh key `y` comes after and the call throws | ||
| * `ERR_INVALID_HTTP_TOKEN`. | ||
| */ | ||
| const METHOD_GETTER_ARRAY_INDEX_LIMIT = 2 ** 32 - 1; | ||
|
|
||
| const methodGetterPrecedesMember = ( | ||
| options: t.ObjectExpression, | ||
| member: string, | ||
| ): boolean => { | ||
| // A key's position comes from its first insertion; its descriptor comes from the last. | ||
| const firstIndex = new Map<string, number>(); | ||
| const lastDescriptor = new Map< | ||
| string, | ||
| { readonly node: t.ObjectProperty | t.ObjectMethod; readonly index: number } | ||
| >(); | ||
| for (const [index, property] of options.properties.entries()) { | ||
| if (t.isSpreadElement(property)) continue; | ||
|
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 first insertion supplied by an object spread is fresh evidence beyond the handled direct-redefinition case: in AGENTS.md reference: AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| const name = semanticStaticPropertyKey(property.key, property.computed); | ||
| if (name === null) continue; | ||
|
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 computed accessor is fresh evidence beyond the handled static getter cases: AGENTS.md reference: AGENTS.md:L21-L21 Useful? React with 👍 / 👎. |
||
| if (!firstIndex.has(name)) firstIndex.set(name, index); | ||
| lastDescriptor.set(name, { node: property, index }); | ||
| } | ||
| const orderOf = (name: string): readonly [number, number] => { | ||
| const position = firstIndex.get(name) ?? Number.MAX_SAFE_INTEGER; | ||
| const key = Number(name); | ||
| return Number.isInteger(key) && | ||
| String(key) === name && | ||
| key >= 0 && | ||
| key < METHOD_GETTER_ARRAY_INDEX_LIMIT | ||
| ? [0, key] | ||
| : [1, position]; | ||
| }; | ||
| const before = ( | ||
| left: readonly [number, number], | ||
| right: readonly [number, number], | ||
| ): boolean => | ||
| left[0] !== right[0] ? left[0] < right[0] : left[1] < right[1]; | ||
| let memberOrder: readonly [number, number] | undefined; | ||
| let getterOrder: readonly [number, number] | undefined; | ||
| for (const [name, { node }] of lastDescriptor) { | ||
| const order = orderOf(name); | ||
| if ( | ||
| t.isObjectMethod(node) && | ||
| node.kind === "get" && | ||
| (getterOrder === undefined || before(order, getterOrder)) | ||
| ) | ||
| getterOrder = order; | ||
| if (name === member) memberOrder = order; | ||
| } | ||
| return ( | ||
| getterOrder !== undefined && | ||
| (memberOrder === undefined || before(getterOrder, memberOrder)) | ||
| ); | ||
| }; | ||
|
|
||
| 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. A spread | ||
| // *after* the options cannot move it: `http.request({method: "BAD METHOD"}, ...[])` | ||
| // still throws ERR_INVALID_HTTP_TOKEN. | ||
| const firstArgument = node.arguments[0]; | ||
| if (firstArgument !== undefined && t.isSpreadElement(firstArgument)) | ||
| return false; | ||
| // A callable in slot 0 is the callback, so Node replaces the options with defaults and | ||
| // never reads a later object. Measured on Node 24.22.1, | ||
| // `http.request(() => {}, {method: "BAD METHOD"})` returns a GET ClientRequest, while | ||
| // `http.request({method: "BAD METHOD"}, () => {})` throws ERR_INVALID_HTTP_TOKEN | ||
| // because the method in slot 0 is validated before the callback is registered. | ||
| const head = node.arguments[0]; | ||
| // The value is unwrapped first, because an erased TypeScript assertion leaves the same | ||
| // function at runtime. Measured on Node 24.22.1, | ||
| // `http.request((() => {}) as any, {method: "BAD METHOD"})` returns a default GET. | ||
| const headValue = | ||
| head === undefined ? undefined : unwrapJavaScriptExpression(head).node; | ||
| if ( | ||
| headValue !== undefined && | ||
| (t.isArrowFunctionExpression(headValue) || | ||
| t.isFunctionExpression(headValue)) | ||
| ) | ||
|
XiaoBinGan marked this conversation as resolved.
Resolved
|
||
| 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; | ||
| // `ClientRequest` copies the options with `Object.assign` in property order, so a getter | ||
| // can only rewrite a member that comes after it. Measured on Node 24.22.1, | ||
| // `http.request({get x() { this.method = "GET"; }, method: "BAD METHOD"})` returns a GET | ||
| // request while `{method: "BAD METHOD", get x() {...}}` throws ERR_INVALID_HTTP_TOKEN, | ||
| // because the invalid value was copied first. | ||
| if (methodGetterPrecedesMember(options, "method")) 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 => | ||
|
|
||
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.
This exact getter-before-method call and expected result already appear at lines 81-85, while the later getter-after-method standalone test similarly repeats lines 94-98. The duplicate assertions exercise no distinct behavior and create two places to update for each outcome, so keep one copy of each getter-order regression.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.