Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 BUG: impossible to get remote client IP address from incoming Astro.request #3708

Closed
1 task done
JReinhold opened this issue Jun 24, 2022 · 10 comments 路 Fixed by #3973
Closed
1 task done

馃悰 BUG: impossible to get remote client IP address from incoming Astro.request #3708

JReinhold opened this issue Jun 24, 2022 · 10 comments 路 Fixed by #3973
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@JReinhold
Copy link

JReinhold commented Jun 24, 2022

What version of astro are you using?

1.0.0-beta.56

Are you using an SSR adapter? If so, which one?

Deno

What package manager are you using?

pnpm

What operating system are you using?

Mac

Describe the Bug

The Request object that can be read via Astro.request during SSR only contains a subset of the http.IncomingMessage. This means that properties like socket are not available and thus it's impossible to get the remote IP address of the client.

Reading the request IP is common for geo-related topics like geofencing content, adhering to local laws (eg. EU cookie consent), etc., and I can't currently come up with a valid workaround for this.

It looks like the createRequest function is responsible for this:

export function createRequest({

This is both an issue when using the vite dev server as well as running a production server with the Deno adapter (and most likely all the other SSR adapters as well, but I might be wrong here).

I'm unsure if there are other valuable properties that are being stripped from the IncomingMessage, all I've noticed is the socket property.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ufjx2c?file=src/pages/index.astro

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added this to Needs Triage in 馃悰 Bug Tracker Jun 24, 2022
@matthewp
Copy link
Contributor

Is there a property of a standard Request object (https://developer.mozilla.org/en-US/docs/Web/API/Request) that provides this? Can you get this information from a normal Deno script? If not we can't provide Node-specific API information.

@tony-sull tony-sull moved this from Needs Triage to Needs Information in 馃悰 Bug Tracker Jun 27, 2022
@tony-sull tony-sull added the needs response Issue needs response from OP label Jun 27, 2022
@JReinhold
Copy link
Author

Sadly no, it's not available on a standard Web Request object.
I think that's because the web standards are focused on browsers, and in that world, browsers (or clients) never receives an incoming request, a Request is always something the client creates, and in that sense it doesn't make sense to have the remoteAddess (the requestor) as part of the object.

But it seems like Deno has thought of this, because a Handler in http/server.ts takes a second argument, ConnInfo which contains the remote and local address of the connection. So it's not part of the regular Request, but it is available.

I had hoped that there would be a generic solution across adapters to this problem, but I don't think there is. Eg. Netlify Functions exposes the remote address by adding the x-nf-client-connection-ip property to the header. Cloudflare Workers use the CF-Connecting-IP header. Vercel Edge uses x-real-ip.

So maybe the solution here is to grab it from the Deno ConnInfo and add it to the headers on the Request to mimic the other services?

To match the Deno ConnInfo types the headers could be called x-deno-remote-addr and x-deno-local-addr.

@tony-sull
Copy link
Contributor

Thanks for digging into how Deno handles this, @JReinhold!

Adding a second param similar to ConnInfo seems like a reasonable approach, marking this to discuss with the team to dig deeper into how this would work with all the different SSR adapters

@tony-sull tony-sull added needs discussion Issue needs to be discussed and removed needs response Issue needs response from OP labels Jun 28, 2022
@nrgnrg
Copy link
Contributor

nrgnrg commented Jun 28, 2022

Yep Cloudflare also adds IncomingRequestCfProperties.

I was also trying to find a way around this limitation when implementing the Cloudflare adaptor, but I think this is a dev server only issue, I'm certainly able to access those properties in prod.

In development right now I've got an ugly workaround using globalThis to make the objects consistant across environments.

@natemoo-re
Copy link
Member

Notes from Triage Call

  • We agree that this is an important use case!
  • We want to keep with the web-standard Request object, but the fact that every platform seems to implement some additional metadata here is important.
  • We're very curious how Remix and SvelteKit adapters handle this, they have very similar constraints to us.
  • We'd like to have a short-term fix prior to v1.0, with the possibility of a better long-term fix coming after v1.0.

@natemoo-re natemoo-re added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed needs discussion Issue needs to be discussed labels Jun 29, 2022
@JReinhold
Copy link
Author

Sounds good!

Here's the PR in SvelteKit that adds the clientAddress to the RequestEvent (for each adapter): sveltejs/kit#4289

And it doesn't look like Remix does anything in particular, just relying on userland to get the remote IP from whichever header matches their deployment target. Here's a (userland) utility to get the address by just brute forcing any sensible header: https://github.com/sergiodxa/remix-utils/blob/main/src/server/get-client-id-address.ts
I couldn't find a Remix adapter for Deno (maybe I am just bad at googling) so they don't really have the problem of the missing header in Deno Deploy.

@nrgnrg
Copy link
Contributor

nrgnrg commented Jul 4, 2022

I wonder if a second param was to be added that contained the additional metadata wether it could be dual purpose and be used for other features too such as accessing additional properties added by plugins.

Arguably though thats a different problem entirely.

@matthewp matthewp self-assigned this Jul 15, 2022
@matthewp
Copy link
Contributor

Looking at other frameworks

  • SvelteKit: clientAddress as part of a metadata object.
  • Remix: getClientIPAddress 3rd party package.
  • Next.js: Uses Node.js API, use 3rd party packages or direct Node APIs.

SvelteKit seems closest to how we work, So I'd propose either:

---
Astro.clientAddress; // getter that throws if the adapter doesn't provide it.
---

or

---
import { getClientAddress } from 'astro';

let address = getClientAddress(Astro.request);
---

It would probably work the same way under the hood either way. Slight preference for the first option since we don't have many things you import from astro.

@JReinhold
Copy link
Author

I agree the first option is more appealing. It's simpler to use and easier to discover.

@matthewp
Copy link
Contributor

Draft PR is out: #3973

Just need to update docs.

馃悰 Bug Tracker automation moved this from Needs Information to Done Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants