honojs / node-server

Node.js Server for Hono
https://hono.dev
379 stars 46 forks source link

Unable to access remoteAddr in route using node #80

Closed shabbyrobe closed 6 months ago

shabbyrobe commented 1 year ago

In my Deno applicaiton, I'm able to access the client's IP address on each request using some fairly convoluted, but nonetheless functional, means:

const app = new Hono();
app.get("/", (ctx) => {
  return ctx.body(ctx.env.remoteAddr());
});

serve(async (request, connInfo) => {
  const remoteAddr = () => resolveRemoteAddr(connInfo.remoteAddr);
  const env = { remoteAddr };
  return app.fetch(request, env);
}, {
  hostname: "0.0.0.0",
  port: 3001,
});

This does not appear possible in an equivalent node application, using honojs/node-server. The function returned by getRequestListener has access to the IncomingMessage structure, which makes this available via message.socket.remoteAddress, but that is not itself made available to the fetchCallback, and the undici Request object that is passed to the route doesn't appear to provide access to it either.

Am I missing something?

yusukebe commented 1 year ago

Hi @shabbyrobe,

At the moment, the Node.js Adapter doesn't provide support for accessing the remote address. This is due to the fact that the web standard API's Request object doesn't offer a method to access the IP address.

I believe it might be a good idea to introduce functions for retrieving the remote address via env variables. However, implementing this would require modifications to the Node.js Adapter itself.

shabbyrobe commented 1 year ago

Hi @yusukebe, thanks for the context. Happy to try my hand at putting together a PR to add those functions to the adapter if you like? Where would you suggest might be the best place to add them?

yusukebe commented 1 year ago

@shabbyrobe

In my opinion, how about changing this line as follows:

https://github.com/honojs/node-server/blob/48104d5c1a841e50d8aa090e7902cbe8a710fa90/src/server.ts#L8

pass env as the second argument to fetchCallback:

const env = {
  remoteAddress: incoming.socket.remoteAddress,
  //...
}
res = (await fetchCallback(new Request(url.toString(), init), env)) as Response
yusukebe commented 1 year ago

@tangye1234

If you have any opinion, please share it!

tangye1234 commented 1 year ago

@tangye1234

If you have any opinion, please share it!

const env = {
   // in nextjs, sometimes, incoming message is faked without socket
   remoteAddress: incoming.socket?.remoteAddress,
   // maybe useful
   httpVersion: incomming.httpVersion
}
yusukebe commented 1 year ago

@tangye1234 Thanks

yusukebe commented 1 year ago

AWS Lamba adapter also has a specific variable called requestContext in c.env. So I think this method is also fine for Node.js.

https://hono.dev/getting-started/aws-lambda#access-requestcontext

yusukebe commented 1 year ago

@shabbyrobe

Could you create a PR?

kevinvdburgt commented 1 year ago

@tangye1234 If you have any opinion, please share it!

const env = {
   // in nextjs, sometimes, incoming message is faked without socket
   remoteAddress: incoming.socket?.remoteAddress,
   // maybe useful
   httpVersion: incomming.httpVersion
}

I'd prefer to forward the entire incoming object instead, this way it should be possible to upgrade to a WebSocket connection.

mhio commented 9 months ago

I'd prefer to forward the entire incoming object instead, this way it should be possible to upgrade to a WebSocket connection.

It would be good to have the environment specific chunk of incoming info in each hono environment, whatever that may be.

As the client IP address is common to most (all?) it may be useful to separate the IP address info out into it's own Hono data structure on env and munge the address into the right shape for any Hono hosting environment.

Also after patching node-server to pass incoming through, I found hono also needs the incoming to be emulated in tests that use app.request()

yusukebe commented 9 months ago

As the client IP address is common to most (all?) it may be useful to separate the IP address info out into it's own Hono data structure on env and munge the address into the right shape for any Hono hosting environment.

What we have to consider is that there is no way to get the IP address in the Web-Standards API.

For example, Cloudflare Workers can get information such as city and region from request.cf, but not the IP address.

https://developers.cloudflare.com/workers/runtime-apis/request/#incomingrequestcfproperties

So if we use Node.js, we need to add a special API to the Web-Standards API. One way to do this is with Env.

mhio commented 9 months ago

I see from the docs cloudflare inject CF-Connecting-IP as a header for this purpose. So if hono were to have a generic way to access the connection IP address, maybe that's something hono or middleware would do based on what's presented (be that in Env or not)

For the environments that do have a socketish type object, I can see the utility of passing that through from the handler in standard locations so people/Hono can access whatever low level thing they need from that environment but that would need to be clear it's a hosting environment specific property.

Then piggy backing off that, hono or middleware could produce the common client connection info, in some cases reliant on the handler injecting something environment specific.

The getting started guide seems to imply only cloudflare and lagon don't (or can't?) have a handler in between the request and hono.

Deno/Node - function on the connection/socket context Bun - function on the server context CloudFlare header - Fastly header - Vercel function(header) - which is a getter for the x-real-ip header

g3r4n commented 6 months ago

@yusukebe I'm working on a boilerplate similar to https://github.com/t3-oss/create-t3-turbo with remix and hono, but I can't make the expo app working with tRPC because i need to expose the tRPC server served by hono. How can I help moving forward on this issue ? can you give me a little bit a guidance to create a PR ?

yusukebe commented 6 months ago

@g3r4n

Contribution guide: https://github.com/honojs/hono/blob/main/docs/CONTRIBUTING.md

If it's related to the tRPC middleware, create an issue or PR on the middleware repo: https://github.com/honojs/middleware

g3r4n commented 6 months ago

@yusukebe it's not related to the tRPC middleware, it's related to node-server not accessible on the local network. it's only accessible through localhost. If you try to access it through 192.168.XX.XX it doesn't respond.

g3r4n commented 6 months ago

it works if you set the hostname to 0.0.0.0, my bad. I believe we can close this issue. @shabbyrobe do you agree ? does it fixes your issue as well ?

yusukebe commented 6 months ago

Yeah. We can close this issue.

The main purpose of this issue is resolved by https://github.com/honojs/node-server/pull/129

shabbyrobe commented 6 months ago

Yep, looks good, thanks everyone!