adapter-node: Make remoteAddress of requesting client available

[TheRealThor]

This makes available to endpoints the IP address of client requesting the resource via platform object.

Improved solution to #3972

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: /~https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

⚠️ No Changeset found

Latest commit: e4170e9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[benmccann]

Is this necessary? I would think that you could get the client address from an existing HTTP header. E.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

It feels weird to me to expose remoteAddress specifically. If it is really necessary, I'd probably rather expose the whole request - though others may have different opinions about this

[TheRealThor]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

This header only is set when a proxy is used because in this case "remoteAddress" would only show the IP address of the proxy (who in fact send the request).

I already proposed to show the IP-address as a additional header information to the request object, but Rich Harris suggested it should be an adapter/platform relevant information.

Whether the whole request (raw request) or just the remoteAddress ist exposed, in any case the IP address of the remote client is an important information for security purposes and "it feels weird" to me that to date nobody thought about this so far.

[mrkishi]

Yeah, I wouldn't make the assumption apps are always behind a proxy, at least not with node and custom adapters support.

I don't know about exposing the raw request, but even if it made sense in the end, I wonder if it also makes sense to lift common features so that they work across adapters. While not all adapters may have the remote address available, as pointed out in the other pr, most will, each exposing it in their own way (eg. req.remoteAddress, CF-Connecting-IP and X-Nf-Client-Connection-Ip headers). If they all provided it as a compatible remoteAddress, apps would be at least somewhat portable. What do you think, Ben?

ps. it might be worth it to make it getRemoteAddress() in case some adapters have to do work to get it, but I expect at most they'd need to do a header lookup and I don't know that's measurably different than adding to the closure. But if it turns out there's a lot we could normalize under platform, maybe there's a better/leaner API?

[Conduitry]

I'm a bit worried about lulling people into a false sense of security by having similar APIs in platform across different adapters. But that said, it does provide a lower-stakes way to test out things like this before potentially promoting them to the shared API.

I agree that we don't want to assume that certain headers are present from a proxy in front of the Node app. But also, req.socket.remoteAddress would be the proxy address if there were a proxy, and not the user's address, right? I'm not sure what the best API is here.

[benmccann]

I agree with the feedback on the earlier PR that we shouldn't make this a header because it's not a header. But it does seem reasonable to expose in some way

Making it a field on the SvelteKit request is a somewhat reasonable idea. Vercel and the other serverless paltforms would mostly populate it using x-forwarded-for in the adapter. adapter-node could use either x-forwarded-for or req.remoteAddress (we have options there already for x-forwarded-proto and x-forwarded-host)

I'd want some other folks to weigh in on this as well since it's adding new API surface

[TheRealThor]

But also, req.socket.remoteAddress would be the proxy address if there were a proxy, and not the user's address, right?

The regular case is that the client is directly connected to the server, especially in a node-server context, as proposed here.
However, If it's an external/untrusted proxy , behind which the client sits, still only req.socket.remoteAddress can be trusted because header data like "X-Forwarded-For" etc can be set by the client. Even if it's an internal/trusted proxy, req.socket.remoteAddress still might be needed to see if the sender is indeed the trusted proxy.

The special case, that someone uses an internal proxy, making remoteAddress always returning that proxy address should be addressed in a later commit and/or by the user of the proxy with a header solution.

Generally it would be logical to have the remoteAddress in the request object because every request has a remote origin. Maybe making it optional so that it could be set by the relevant adapter or not of some adapter does not have a remote origin of the request.

[Rich-Harris]

Doing app.render(request, { platform: { req } }) is definitely the easiest option, though I do quite like @mrkishi's idea of making things like this standard across environments — event.remoteAddress or whatever. Are we worried that it wouldn't be accessible within some adapters? Or that once we start adding things like this we'll be faced with an onslaught of requests for random properties to be added to solve niche scenarios?

[mrkishi]

A best-effort platform.remoteAddress is growing on me. Cross-compatibility would not be guaranteed so you'd still need to go to the docs for migration, yet it'd be a lot more convenient to find out if your new adapter also supports it.

I wouldn't be worried about a false sense of API compatibility because, alternatively, users would have used some adapter-specific API and they would've encountered a similar error when migrating. If that is a prevalent worry, we could also go with a Proxy or accessor API so that useful errors are possible, at a (small?) performance cost, eg.

platform.get('remoteAddress') === new Error(`adapter-x has no support for "${property}". see https://adapter-docs/ for details.`)

[benmccann]

I believe remoteAddress with Node's meaning of it is only available in adapter-node. In adapter-vercel, etc. you could use X-Forwarded-For, but it has a different meaning. I suppose that's okay, but it's a bit weird that it'd have different behaviors on different platforms.

[mrkishi]

I suppose we'd need to add options to adapter-node telling it to trust certain proxies, as you'd have to do on a regular node server.

[evdama]

Having the requesting clients IP address available is something we very much need in order to use it to query the ipregistry.org API in order to get information about whether this IP belongs to a botnet, or the latitude/longitute for mapping onto a map.

I've been using Sapper and a firebase SSR cloud function in the past, running nodejs with some express middleware (request-ip), which looks for a bunch of information inside the request header and extracts the requesting IP which I then use to query the ipregistry API 👍

From request-ip:

The user ip is determined by the following order:

X-Client-IP
X-Forwarded-For (Header may return multiple IP addresses in the format: "client IP, proxy 1 IP, proxy 2 IP", so we take the the first one.)
CF-Connecting-IP (Cloudflare)
Fastly-Client-Ip (Fastly CDN and Firebase hosting header when forwared to a cloud function)
True-Client-Ip (Akamai and Cloudflare)
X-Real-IP (Nginx proxy/FastCGI)
X-Cluster-Client-IP (Rackspace LB, Riverbed Stingray)
X-Forwarded, Forwarded-For and Forwarded (Variations of #2)
req.connection.remoteAddress
req.socket.remoteAddress
req.connection.socket.remoteAddress
req.info.remoteAddress

If an IP address cannot be found, it will return null.

[evdama]

Maybe using req.socket.remoteAddress is a good first step after which, at least adapter-node, could implement something like this /~https://github.com/pbojinov/request-ip/blob/915d984b46d262e4418a889abc7601c38d47f049/src/index.js#L48 no?

That said, I reckon it would be very useful for either adapter to provide the requesting IP address so it can be used for things like geolocation and whatnot. The request-ip code looks like it could do that for either adapter, so maybe using it in Sveltekit is an option no?

[benmccann]

I suppose we'd need to add options to adapter-node telling it to trust certain proxies, as you'd have to do on a regular node server.

This is what we already do for x-forwarded-proto and x-forwarded-host. I think if we do it here we need to call the field something other than remoteAddress since it no longer behaves the same as Node's remoteAddress. I also wonder if there might be some case where I user would want both the remoteAddress and the x-forwarded-host and by having it behind a flag you can only access one or the other.

[evdama]

... Or that once we start adding things like this we'll be faced with an onslaught of requests for random properties to be added to solve niche scenarios?

Good point but then, just from looking through svk issues and discord channels, there are actually quite a bunch of people who'd need/want the IP address of a remote client... meaning I don't think that feature is niche 🤗

A best-effort platform.remoteAddress is growing on me. Cross-compatibility would not be guaranteed so you'd still need to go to the docs for migration, yet it'd be a lot more convenient to find out if your new adapter also supports it.

Agreed... from a developers point of view that's no big deal I guess and much easier compared to the current situation where in fact it's impossible to do it in case you don't want to use a server.js that imports handler (please correct me if I'm wrong):

import { handler } from '../build/handler.js'
import express from 'express'
import requestIP from 'request-ip'

let IPAddress

const getUserIPAddressMiddleware = async ( request, response, next ) => {
  try {
    IPAddress = await requestIP.getClientIp( request ) // empty string or null if IP address couldn't be retrieved
    console.log( 'IPAddress: ', IPAddress )
    next()
  } catch ( error ) {
    console.error( '🔴  error in getUserIPAddressMiddleware: ', error )
  }
}

const app = express()

// add a route that lives separately from the SvelteKit app
app.get('/nonsvkroute', (request, response, next) => { response.locals = {duck: 'ente'}, next('non-svk route...'), next() })

// let SvelteKit handle everything else, including serving prerendered pages and static assets
app.use(
  getUserIPAddressMiddleware,
  handler
)
app.listen(3000, () => { console.log('Listening on port 3000') })

So here you're able to get the IP address but then how do I get it to svk easily so I could maybe pass it down to the client for display using hooks.js and getSession()?

I would much rather like to use hooks.js and not involve express at all i.e. just use adapter-node with hooks.js and retrieve the clients remote IP address using handle(). But atm there seems no way to do it inside handle() because I've no way to get the remote clients IP address correct?

[evdama]

The horrible war in 🇺🇦 and the impossed sanctions on 🇷🇺 create another use case for having the remote clients IP address available in hooks.js.

Using the remote IP of a client we are going to call the ipregistry.org API whether this is a russian user or not. If so, he/she is redirected to a dedicated page, telling him/her that they can visist our website, but that payments are disabled.

[evdama]

ps. it might be worth it to make it getRemoteAddress() in case some adapters have to do work to get it, but I expect at most they'd need to do a header lookup

• Using something like getRemoteAddress() might indeed be a good way to abstract accross different platforms and/or ways users deploy their SvelteKit apps (proxy, no proxy... etc).
• Depending on the various cases the wayrequest-ip does a header lookup can serve as an example
• getting this feature somewhat harmonized accross platforms/adapters would then also make it easier to migrate back and forth (except for some cases were docs could pave the way)

@benmccann @mrkishi @TheRealThor @Rich-Harris @Conduitry what do you think?

[Rich-Harris]

Ok, recapping some discussion between the maintainers:

• Client IP address is useful information to surface regardless of adapter, and we want the experience of using SvelteKit to be normalised across adapters to the greatest degree possible
• That means that we'd ideally have a property like event.clientAddress
• To populate that property, adapters would need to pass in a function to server.render:

  response = await server.render(request, {
    getClientAddress: () => {...}
  });

• SvelteKit could make event.clientAddress a getter, so that we don't need to invoke the function unnecessarily
• If an adapter doesn't provide getClientAddress, we should throw an error like 'adapter-foo does not make clientAddress available'
• On platforms like Netlify, Cloudflare Pages, Vercel etc we can trust that the IP address is correct. In cases like adapter-node we probably need the user to opt in, because we shouldn't trust the proxy on their behalf (just like how we treat HOST_HEADER and PROTOCOL_HEADER currently)
• Reading event.clientAddress during prerendering should be an error

[Rich-Harris]

going to close this in favour of #4289 — thanks for getting the ball rolling