Repository navigation
Conversation
…en, or CONNECT request() sent GET for a method that is not a string, and for most strings that are not an HTTP token. It sent CONNECT too. undici rejects each of them before it connects, with InvalidArgumentError: - not a string: "method must be a string" - not an HTTP token: "invalid request method" - exactly "CONNECT": "invalid method" request() now makes the same checks before it calls fetch(). InvalidArgumentError gets the name, the code UND_ERR_INVALID_ARG and the default message that it has in undici. A falsy method is still the default. It is now "GET" for the guard of request() too, so a falsy method with a body rejects there and no longer in fetch(). A valid method still goes out in upper case, because fetch() sends GET for a spelling outside its method table. GET, HEAD, POST, PUT, DELETE, OPTIONS and PATCH skip the checks and the case map.
Collaborator
Author
|
Status Reproduced on the released bun 1.4.3-canary.1+367d939d9 (linux x64), whose import { createServer } from "node:net";
import { request } from "undici";
const lines = [];
const server = createServer(socket => {
socket.on("data", data => {
lines.push(String(data).split("\r\n")[0]);
socket.end("HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n");
});
});
await new Promise(resolve => server.listen(0, "127.0.0.1", resolve));
const url = `http://127.0.0.1:${server.address().port}/`;
for (const method of [true, 123, "GET POST", "CONNECT", null, "Put", "brew", "connect"]) {
lines.length = 0;
const outcome = await request(url, { method }).then(
async res => (await res.body.text(), "ok"),
err => `${err.name}: ${err.message}`,
);
console.log(JSON.stringify(method).padEnd(10), outcome.padEnd(50), JSON.stringify(lines));
}
server.close();main: This branch (debug build): The first four rows now match undici 6.21.3 and 7.16.0 on Node 26.3.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughUndici now validates request methods before connecting. Falsy methods default to ChangesRequest method validation
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change adds method validation to undici.request, so invalid methods and CONNECT are rejected before any request is sent. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ptured primordial request() puts the method in upper case, so "connect" and "Connect" went out as CONNECT, the one method that undici never sends from request(). After a 200 answer to that request, the next requests to the same origin go out on the same socket, which a server treats as a tunnel. The CONNECT check now runs on the upper-case method. undici rejects only the exact string and sends another case as written, and the tests name that difference. The case map now calls a String.prototype.toUpperCase that is captured when the module loads, so a replaced prototype method does not change the method that goes out.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The reference for the type check and the token check sits on the lines that it covers, and the CONNECT comment names its own reference.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
undiciaccepts arequest()method that is not a string (true) or not a token ("GET POST"). It sendsGET, orCONNECTfor"CONNECT".InvalidArgumentError:method must be a string,invalid request method,invalid method).src/js/thirdparty/undici.js:190maps a non-string tonull, andfetch()readsnullasGET. No user reported this.Fix
requestMethod()makes the same three checks beforefetch(), with undici's class, name, code (UND_ERR_INVALID_ARG) and messages.test/js/first_party/undici/undici.test.ts, rawnode:netserver. 21 of 31 new tests fail on main.Background
undicireplaces the npm package (Resolve bare undici to the installed package, keep the shim as fallback #36102 would prefer an installed one). Itsrequest()wrapsfetch().fetch()sendsGETfor a spelling outside its method table (fetch and Request silently turn an unrecognised HTTP method into GET #42497, fetch, Request, server.fetch: stop sending GET for a method outside the method table #44718). So the upper-casing stays:"Put"still goes out asPUT.node:httppath for as-written spellings. It redoes the native work offetch()in JS, and"post"gets 400 from strict servers.Downsides
CONNECTnow reject. A falsy method with a body rejects with the shim'sError.GET,HEAD,POST,PUT,DELETE,OPTIONS,PATCHpays one token scan per call (0 before). Binary: +875 bytes."connect"rejects, where undici sendsconnect. Here it would go out asCONNECT."brew"still goes out asGET.Notes
Reproduction (a raw TCP server, so that the request line is visible)
methodtrue,123InvalidArgumentError: method must be a stringGET"GET POST"InvalidArgumentError: invalid request methodGET"CONNECT"InvalidArgumentError: invalid methodCONNECTnullGETGETGET"Put"PutPUTPUT"brew"brewGETGET"connect"connectCONNECTInvalidArgumentError: invalid methodOne non-token does not go out as
GETon main:"po\u017ft"(U+017F) goes out asPOST, becausetoUpperCase()maps that character toS. undici rejects it, and so does this PR.What the change is
requestMethod()replaces line 190.GET,HEAD,POST,PUT,DELETE,OPTIONSandPATCHreturn at once. A falsy method is"GET". Then the type check and the token check run on the method as written. Then the method is put in upper case, as before, now through aString.prototype.toUpperCasethat is captured when the module loads. ThenCONNECTis rejected.InvalidArgumentErrorgets the constructor that it has in undici 6.21.3 and 7.16.0 (lib/core/errors.js): name, default message, code.maxRedirections,throwOnError) are those of undici 6. The three rejections are the same in 6.21.3 and 7.16.0.RequestHandlerandRequestconstructors before it connects (6.21.3 makes 20). This PR ports the 3 for the method.A wider probe
69 rows of
request(url, { method }), with and without a body, against undici 7.16.0 on Node 26.3: 16 rows match on main, 35 match with this PR. The rows that change are the rejections. No row that resolved with a valid method resolves differently.Still different from undici after this PR
"Put"asPUT,"get"asGET. Without the upper-casing,fetch()on main sendsGETfor"Put","Delete"and"PatCh", so this PR keeps it. The tests pin"Put","Delete"and"patch"for that reason. These rows pass on main.fetch()("brew","LIST","TRACK") still goes out asGET. That is fetch and Request silently turn an unrecognised HTTP method into GET #42497, and fetch, Request, server.fetch: stop sending GET for a method outside the method table #44718 is the open PR for it."connect"and"Connect"now reject withinvalid method. undici rejects only the exact stringCONNECTand sends the other spellings as written, so this is stricter than undici. The built-in module puts the method in upper case, so on main these spellings go out asCONNECT / HTTP/1.1. After a 200 answer to that request, main sends the next requests to the same origin on the same socket, and a server treats that socket as a tunnel. The tests name the two rows as a difference from undici.PUT(index.js:opts.method || (opts.body ? 'PUT' : 'GET')). The built-in module rejects withBody not allowed for GET or HEAD requests. This PR changes only the rejection fornull,"",0,falseandNaN: a falsy method is now"GET"for the module's own guard, so the guard rejects beforefetch()does.GETorHEADwith a body. undici sends it. The built-in module rejects (an existing test asserts that).request(url, { method: "POST", body: { a: 1 } })sends[object Object]. undici rejects withInvalidArgumentError.request()keep their class: the url check is aTypeError, and themaxRedirectionsand signal checks are a plainError. undici throwsInvalidArgumentErrorfor each.maxRedirectionscheck. So{ method: true, maxRedirections: -1 }now rejects with the method error, as in undici 7.16.0. undici 6.21.3 and main answer with themaxRedirectionserror.{ method: null, body: "x", maxRedirections: -1 }now rejects with the body error. Main answered with themaxRedirectionserror. No test pins these orders.Measurements
Calls per 3,000 requests, counted with
String.prototype.toUpperCaseandRegExp.prototype.execreplaced beforeundiciloads:methodtoUpperCasecalls, mainRegExpexecs, main"GET","POST""PROPFIND","get"The token scan is
checkIsHttpTokenofinternal/validators, which thenode:httpclient calls for every request.node:eventsalready loads that module, so the newrequireloads nothing.Size: the release bundle of the built-in modules (
InternalModuleRegistryConstants.bin, the.bun_builtinssection) is 2,631,735 bytes on main and 2,632,610 bytes with this PR. Measured withsrc/codegen/bundle-modules.ts --debug=OFFon both trees. No release binary was linked for this number.Other open PRs on these files
undiciimport to the installed package and keeps the built-in module as the fallback. It changes the same import line ofundici.test.tstonext/dist/compiled/undici. The PR that lands second takes that specifier witherrorsin the import list.InvalidArgumentErrora constructor, each with a different body. The body here is the one of undici.History
test/js/first_party/undici/undici-primordials.test.ts. Both files pass on a debug build: 57 tests.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file