feat(runtime): implement server.requestIp + node:http socket.address()
#6165
Conversation
Changed Request.uws_request to the new AnyRequestContext. This allows grabbing the IP from a Request. Unfinished.
Currently using uws's requestIpAsText, which always returns a ipv6 string. We should return a `SocketAddress` object to the user instead, which will contain the formatted address string and what type it is. We'll have to use requestIpAsBinary and parse that ourselves.
if we can't get the ip.
|
Reposting from #3540 (comment), any hope of considering |
src/deps/libuwsockets.cpp
Outdated
| length = sprintf(b, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x", | ||
| b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7], b[8], b[9], b[10], b[11], | ||
| b[12], b[13], b[14], b[15]); | ||
| *is_ipv6 = true; |
There was a problem hiding this comment.
this IPv6 formatter was taken from some usockets code. Maybe this can be later improved to a more intelligent IPv6 formatter. I don't know enough about ipv6 formatting to do this though.
There was a problem hiding this comment.
Maybe some utils like inet_ntop.
// length == 16
if (length == @sizeOf(std.meta.FieldType(std.os.sockaddr.in6, .addr))) {
// from binary to text form, store in new_buf
_ = bun.c_ares.ares_inet_ntop(std.os.AF.INET6, &b, &new_buf, new_buf_len);
const len_ = bun.len(bun.cast([*:0]u8, new_buf[0..]));
// text form ip address
const ip = new_buf[0..len_];
}
| namespace Bun { | ||
| namespace JSSocketAddress { | ||
|
|
||
| static const NeverDestroyed<String> IPv4 = MAKE_STATIC_STRING_IMPL("IPv4"); |
There was a problem hiding this comment.
this shouldn't be in the header
| auto length = us_get_remote_address_info(b, (us_socket_t *)res, dest, port, (int*)is_ipv6); | ||
|
|
||
| if (length == 4) { | ||
| length = sprintf(b, "%u.%u.%u.%u", b[0], b[1], b[2], b[3]); |
There was a problem hiding this comment.
we should use snprintf so that the length is always checked and there's no risk of buffer overflow
There was a problem hiding this comment.
this code is adapted from another part of uSockets
if b[0] is interpreted as a u8, then the max length of this string is 15
the longest ipv6 string is supposed to be 39 characters long, though i'm not certain if the sprintf would respect that bound.
the given b is always 64 bytes long, i don't think a length check is neccecary, but we can do it just to ensure these bounds.
|
I'm going to merge this so that it can be part of v1.0.4, but please address the feedback in your next PR |
| @@ -2354,12 +2369,12 @@ declare module "bun" { | |||
| * ```js | |||
| * export default { | |||
| * async fetch(request, server) { | |||
| * return new Response(server.requestIp(request)); | |||
| * return new Response(server.requestIP(request)); | |||
There was a problem hiding this comment.
Why? In the js world we use camelCase so why was this accepted?
There was a problem hiding this comment.
If you look at the official nodejs documentation https://nodejs.org/dist/latest-v20.x/docs/api/http.html you will find that all acronyms like http are not capitalised besides the first letter when used in camelCase. I think we should align ourself with that and not start writing acronyms in caps. Besides that, currently bun has no capitalised acronyms either so this would be our first (mis)step towards that path.
There was a problem hiding this comment.
Imagine Json.stringify... gross
There was a problem hiding this comment.
Why? In the js world we use
camelCaseso why was this accepted?
it was decided from: https://twitter.com/bunjavascript/status/1707690336494186919
There was a problem hiding this comment.
The thing is this is a JavaScript convention not a nodeJS convention. As bun resides in the JS/TS world I think we should stick to that.
There was a problem hiding this comment.
Why? In the js world we use
camelCaseso why was this accepted?it was decided from: https://twitter.com/bunjavascript/status/1707690336494186919
I think twitter is the worst place to ask for opinions regarding code but that's just me
|
The lack of typescript definitions makes this kinda goofy, having to add |
|
Update your bun-types package |
What does this PR do?
Implements
server.requestIp; Fixes #3540Supersedes #4559, thank you Parzival for getting the initial work, mainly around actually getting to the IP address. I added port support, trying to optimize the # of calls, as well as implement a lightweight object for JS.
This also makes
node:httpsocket.address/remoteAddress/remotePort/remoteFamily workDoes not fix #4660 but this probably lays out most of the work for it to work.
How did you verify your code works?
New tests.