THRIFT-6203: Replace deprecated url.parse and fs.exists in the Node.js library

[slachiewicz]

JIRA: THRIFT-6203
Client: nodejs

The library called two APIs Node has deprecated, so an application embedding it printed DEP0169 and DEP0006 on every supported runtime: url.parse() and fs.exists() in web_server.js. They become the WHATWG URL class and fs.stat().

Note: this revision originally also replaced require("constants") (DEP0063) in server.js and connection.js, but that was dropped on rebase because upstream ec1066ab4 already removed that code and now leaves the TLS defaults to Node.

Two behaviour changes come with that and are worth reviewing on their own terms.

A malformed request target now gets a 400. url.parse() accepts one and returns a partly-parsed path; new URL() throws. processPost and processGet catch it and answer 400 Bad Request rather than letting the exception reach the request handler. Only the pathname is used, so the base is a fixed http://localhost: built from the Host header, a value that is not a valid authority (example.com:abc, an unbracketed IPv6 address) made new URL() throw and produced a 400 that url.parse() never did.

options.services defaults to {}. A server serving only static files has no reason to pass it, and createWebServer read it without a default. The registration loop does not catch that, because for...in over undefined is a no-op rather than an error, so the first POST failed instead:

TypeError: Cannot read properties of undefined (reading '/')

The WebSocket upgrade listener is unaffected: since 31f5897cb on master it refuses an empty services object with a 403, and web_server_ws.test.js covers that case.

The fs.exists() replacement also drops a redundant fs.statSync() that ran immediately after the async check.

Also carried here: the comment in thrift_4987_xhr_protocol.test.mjs now names Buffer.from(string), the wording left over from the Buffer() replacement in #3819.

Verified: node -e "for (var u in undefined) {}" is a no-op, while indexing undefined throws the TypeError above — the failure could only surface on a request, not at construction.

The new cases in web_server_ws.test.js cover static file serving, a 404, and a request routed to an unregistered service.

• Did you create an Apache Jira ticket? (THRIFT-6203)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit?
• Did you do your best to avoid breaking changes?

This change was created with AI assistance.

[Jens-G]

Code review

Found 1 issue:

1. Defaulting services to {} changes how the WebSocket upgrade listener handles a server created without services. That listener only notices "no services" when Object.keys(services) throws inside its try block. Object.keys(undefined) throws, so today such a request gets 403 No Apache Thrift Service available. Object.keys({}) does not throw, so with this change svc stays undefined, the listener writes 101 Switching Protocols and then throws TypeError: Cannot read properties of undefined (reading 'transport') at svc.transport.DEFAULT_MAX_LENGTH. The test this PR adds ("HTTP static file serving and URL handling") creates exactly such a server (files only, no services) but only sends GET and POST requests, so the suite does not see this. I reproduced it with that configuration: 403 on the merge base, the TypeError on this branch. An explicit if (!svc) check before the handshake would bring back the 403, and an upgrade request in the new test would cover it.

thrift/lib/nodejs/lib/thrift/web_server.js

Lines 373 to 375 in 7aab2fd

	//Setup all of the services
	var services = options.services || {};
	for (var uri in services) {

thrift/lib/nodejs/lib/thrift/web_server.js

Lines 667 to 675 in 7aab2fd

	//Lookup service
	var svc;
	try {
	svc = services[Object.keys(services)[0]];
	} catch (e) {
	socket.write("HTTP/1.1 403 No Apache Thrift Service available\r\n\r\n");
	return;
	}
	//Perform upgrade

One suggestion, below the bar for the list above but verified:

• Only the pathname of the parsed URL is used, so the base does not need to come from the Host header. Built from "http://" + request.headers.host, a Host value that is not a valid authority (for example example.com:abc, or an IPv6 address without brackets) makes new URL throw, and the request gets a 400; url.parse never looked at that header. A fixed base such as http://localhost avoids this.

thrift/lib/nodejs/lib/thrift/web_server.js

Lines 450 to 460 in 7aab2fd

	var uri;
	try {
	uri = new URL(
	request.url,
	"http://" + (request.headers.host || "localhost"),
	).pathname;
	} catch (e) {
	response.writeHead(400, "Bad Request", {});
	response.end();
	return;
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

Follow-up to the review above: master now refuses a WebSocket upgrade when no service is configured, an empty services object included (31f5897, with tests in web_server_ws.test.js). After a rebase onto master, the services || {} default no longer reaches svc.transport, so point 1 needs no change in this PR.

🤖 Generated with Claude Code

[slachiewicz]

Branch rewritten (70cae7b): rebased on current master, which carries the upgrade-listener fix from 31f5897, so point 1 needs nothing here as you said. The suggestion is taken: both new URL() calls now use the fixed base http://localhost, so a Host header that is not a valid authority no longer turns into a 400. The comment cleanup from #3819 (Buffer.from(string) in thrift_4987_xhr_protocol.test.mjs) rides along.

Verified: web_server_ws.test.js → 10 passed on the rebased branch; eslint lib/nodejs/test clean.

This comment was created with AI assistance.