Jens-G commented on PR #3821:
URL: https://github.com/apache/thrift/pull/3821#issuecomment-5634151972
### 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 co
ver it.
https://github.com/apache/thrift/blob/7aab2fd27007074b0199215ccae4e3acbf62fda6/lib/nodejs/lib/thrift/web_server.js#L373-L375
https://github.com/apache/thrift/blob/7aab2fd27007074b0199215ccae4e3acbf62fda6/lib/nodejs/lib/thrift/web_server.js#L667-L675
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.
https://github.com/apache/thrift/blob/7aab2fd27007074b0199215ccae4e3acbf62fda6/lib/nodejs/lib/thrift/web_server.js#L450-L460
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react
with 👎.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
