Repository navigation
fix(javascript): reject an http method token Node refuses at the call site - #9683
XiaoBinGan wants to merge 6 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 632392c78f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Both fixed in An erased TypeScript assertion hides the options object. A leading spread shifts the options slot. Confirmed:
The spread fills both runtime slots and the trailing object is ignored, so this branch was inspecting an argument Node never reads and dropping a real request. A spread argument now leaves the position unresolved and keeps the request — the same rule as the TLS overload-slot reader on You also suggested sharing the normalized selection with 18 cases; the two new ones fail against the pre-fix file and pass here. |
… site `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. The analysis reported one anyway. Measured on Node 24.22.1: | `options.method` | outcome | | --- | --- | | `"GET"`, `"get"`, `"M-SEARCH"`, `"X"`, `"!#$%&'*+-.^_`\|~"` | request constructed | | `""` | request constructed — Node reads it as "not given" and falls back to `GET` | | `"BAD METHOD"`, `"G/ET"`, `"G:ET"`, `"G,ET"`, `"GET "`, `" "`, `"\\t"`, `"GéT"` | THREW `ERR_INVALID_HTTP_TOKEN` | The accepted set is the RFC 9110 token character class, not a list of verbs, so `"GET "` fails on the trailing space while `"get"` and `"M-SEARCH"` pass. `fetch` and `WebSocket` read a different member and validate no token, so they are untouched. A non-string `method` throws `ERR_INVALID_ARG_TYPE`, but that is left undecided because the reported fields do not describe the type check. 16 cases, 8 failing before this commit. `src/domain/javascript`: 5370 to 5386 passing. `tsc` reports only the pre-existing `adbCommands.ts` duplicate `packageName`, which `main` already has; `build` 0; `npm run format:check` 0; `npm run lint` 0 errors.
…quest
Two review findings, each measured on Node 24.22.1. Both were real.
**An erased TypeScript assertion hides the options object.** `http.request(({host: "h",
method: "BAD METHOD"}) as any)` throws `ERR_INVALID_HTTP_TOKEN` at runtime because the
assertion is erased, but `node.arguments[0]` is a `TSAsExpression`, so the raw
`isObjectExpression` test chose the absent second argument and returned false before the
unwrap. The first argument is now unwrapped before the slot is chosen.
**A leading spread shifts the options slot.** `http.request(...[{host, path}, () => {}],
{method: "BAD METHOD"})` spreads the options and the callback into the two runtime slots and
ignores the trailing object, so Node builds a GET request — measured on Node 24.22.1, it
returns OK. This branch inspected that trailing object and dropped a real request. A spread
argument now leaves the position unresolved and keeps the request.
18 cases; the two new ones fail against the pre-fix file and pass here.
`src/domain/javascript`: 5402 to 5404 passing.
`tsc`, `lint` and `build` still fail only on the pre-existing `adbCommands.ts` duplicate
`packageName`, which `main` has and `morluto#9677`/`morluto#9678` fix.
3db1fac to
76926ce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76926ce8da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two review findings, each measured on Node 24.22.1.
**A callable first argument is the callback.** `ClientRequest` 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`. The slot
selection chose argument 1 and dropped a real request; a callable anywhere in the argument
list now leaves the position unresolved.
**`Object.assign` runs getters before it copies.** A getter can rewrite a later member.
Measured on Node 24.22.1,
`http.request({get x() { this.method = "GET"; }, method: "BAD METHOD"})` returns a GET
request. The reader now bails on an options literal carrying a getter.
`src/domain/javascript`: 5413 passing.
|
Both fixed in A callable first argument is the callback.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e36db24d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… positions
Two review findings, each measured on Node 24.22.1.
**Only a callable in slot 0 is the callback.** A callback after the options does not move the
slot, and the method in slot 0 is validated before the callback is registered:
| call | outcome |
| --- | --- |
| `http.request(() => {}, {method: "BAD METHOD"})` | request constructed — slot 0 is the callback |
| `http.request({method: "BAD METHOD"}, () => {})` | THREW `ERR_INVALID_HTTP_TOKEN` |
The previous test looked at every argument, so the second call kept a request Node never
builds.
**A getter only rewrites a member that follows it in copy order.** `Object.assign` walks the
source in order, so a getter after `method` cannot change the value already copied:
| call | outcome |
| --- | --- |
| `{get x() { this.method = "GET"; }, method: "BAD METHOD"}` | request constructed — the getter runs first |
| `{method: "BAD METHOD", get x() { this.method = "GET"; }}` | THREW — the invalid value was copied first |
`methodGetterPrecedesMember` compares the two indices instead of treating any getter as able
to rewrite the member.
`src/domain/javascript`: 5407 passing.
|
Both fixed in Only a callable in slot 0 is the callback. A getter only rewrites a member that follows it.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28e9b304e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ation
Two review findings, each measured on Node 24.22.1.
**An erased assertion leaves the same callback.** `http.request((() => {}) as any,
{method: "BAD METHOD"})` returns a default GET; the guard tested the raw `TSAsExpression`, so it
missed the function and the object-selection code then read argument 1. The slot-0 value is now
unwrapped before the callable test.
**`Object.assign` enumerates integer-index keys before string keys.** Source order is not the
copy order:
| call | outcome |
| --- | --- |
| `{method: "BAD METHOD", get 0() {...}}` | request constructed — getter `0` runs first |
| `{method: "BAD METHOD", get x() {...}}` | THREW — the invalid value was copied first |
| `{get 0() {...}, method: "BAD METHOD"}` | request constructed |
| `{get x() {...}, method: "BAD METHOD"}` | request constructed |
`methodGetterPrecedesMember` now compares `[group, order]` tuples — integer group ascending,
then the string group in insertion order — instead of a single fused index. My first attempt
folded both groups into one number and lost precision past 2^53, which made two of these cases
report the opposite answer.
`src/domain/javascript`: 5411 passing.
|
Both fixed in An erased assertion leaves the same callback.
Worth recording: my first attempt folded both groups into one number (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afe3cae7ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…y-index bound
Three review findings, each measured on Node 24.22.1.
**A redefined key keeps its first insertion position.** JavaScript replaces the descriptor but
not the slot, so enumeration follows the first write:
| call | outcome |
| --- | --- |
| `{x: 0, method: "BAD METHOD", get x() { this.method = "GET"; }}` | request constructed — `x` enumerates first |
| `{y: 0, method: "BAD METHOD", get x() { this.method = "GET"; }}` | THREW — the getter is on a fresh key |
The helper now takes each key's position from its first insertion and its descriptor from the
last, instead of using the AST index of the last write.
**Only an ECMAScript array index enumerates in the integer group.** `4294967295` is the boundary
and is *not* one:
| key | outcome |
| --- | --- |
| `get 4294967294()` | request constructed — an array index, so it runs first |
| `get 4294967295()` | THREW — enumerates with the string keys, after `method` |
The predicate is now `canonical integer, >= 0, < 2 ** 32 - 1` rather than digits-only.
**A spread after the options slot cannot move it.** `http.request({method: "BAD METHOD"},
...[])` still throws `ERR_INVALID_HTTP_TOKEN`; only a spread in slot 0 leaves the position
unresolved.
`src/domain/javascript`: 5414 passing.
|
All three fixed in A redefined key keeps its first insertion position. Confirmed:
The helper now takes each key's position from its first insertion and its descriptor from the last. That is the distinction I had been collapsing: position and descriptor are two different facts about a redefined key, and using the AST index of the last write was reading the wrong one. Only an ECMAScript array index enumerates in the integer group. Confirmed the boundary:
The predicate is A spread after the options slot cannot move it.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a08abb2e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { 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.
A first insertion supplied by an object spread is fresh evidence beyond the handled direct-redefinition case: in http.request({ ...{ x: 0 }, method: "BAD METHOD", get x() { this.method = "GET"; } }), x keeps the position created by the spread, so Node invokes the final getter before copying method and constructs a GET request. This continue instead records x only at the later getter, causing rejectsHttpMethodLiteral to suppress the real request; expand statically known spread keys and overrides in the same object-property representation.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| for (const [index, property] of options.properties.entries()) { | ||
| if (t.isSpreadElement(property)) continue; | ||
| const name = semanticStaticPropertyKey(property.key, property.computed); | ||
| if (name === null) continue; |
There was a problem hiding this comment.
A computed accessor is fresh evidence beyond the handled static getter cases: const k = "x"; http.request({ get [k]() { this.method = "GET"; }, method: "BAD METHOD" }) constructs a GET request because the getter runs before method is copied. semanticStaticPropertyKey returns null here, so this continue erases the accessor and the later literal check suppresses the real request; retain unknown applicability/order when a computed descriptor may enumerate before method.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
| const options = | ||
| firstValue !== undefined && t.isObjectExpression(firstValue) | ||
| ? firstValue | ||
| : unwrapJavaScriptExpression(node.arguments[1] ?? first!).node; |
There was a problem hiding this comment.
A falsy HTTPS head is fresh evidence beyond the handled callback overload: for https.request(null, { method: "BAD METHOD" }) (likewise false, 0, or undefined), Node's HTTPS wrapper leaves its default options object in place and ignores argument 1, so it constructs a GET ClientRequest. This ternary instead selects argument 1 and removes that real request; preserve the call unless slot 1 is proven to be the HTTP/HTTPS options slot, or carry the module-specific overload identity into this decision.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| // 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" })`, |
There was a problem hiding this comment.
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 👍 / 👎.
Summary
http.requestandhttps.requestvalidateoptions.methodwithcheckIsHttpTokenbefore they build the request, so a literal outside the token set throwsERR_INVALID_HTTP_TOKENfrom the call itself and no request exists. The analysis reported one anyway.Measured on Node 24.22.1
options.method"GET","get","M-SEARCH","X","!#$%&'*+-.^_`|~"""GET"BAD METHOD","G/ET","G:ET","G,ET","GET "," ","\t","GéT"ERR_INVALID_HTTP_TOKENTwo consequences worth naming:
"GET "fails on the trailing space while"get"and"M-SEARCH"pass.""as "not given" rather than as an invalid token. Treating it as invalid would drop a real request.fetchandWebSocketread a different member and validate no token, so they are untouched. A non-stringmethodthrowsERR_INVALID_ARG_TYPE, but that is left undecided because the reported fields do not describe the type check.Contract and boundary impact
Expected effects:
Validation performed
src/domain/javascript— 5402 passed.npm run format:check— exit 0 for the files this PR touches. The reported issue is insrc/cli/adbCommands.ts.npm run lint— the one error is theadbCommands.tsduplicate declaration below.npm run build— fails only on that sameadbCommands.tserror.tsc,lintandbuildall fail onsrc/cli/adbCommands.ts(46,9): error TS2451: Cannot redeclare block-scoped variable 'packageName', whichmainate9b6c87c1already has and which this PR does not touch.#9677and#9678are open fixes for it. I did not include it here so this PR keeps one outcome.Before merge
type(scope): outcome.Author readiness
Human owner: @XiaoBinGan