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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add connInfo in custom handlers #1827

Open
artpani4 opened this issue Sep 28, 2023 · 11 comments · May be fixed by #2007
Open

add connInfo in custom handlers #1827

artpani4 opened this issue Sep 28, 2023 · 11 comments · May be fixed by #2007

Comments

@artpani4
Copy link

It would be really cool to be able to access connInfo from custom handlers

export const handler: Handlers = {
   async GET(_req, ctx) {
  // access to connInfo somewhere here
     return await ctx.render();
   },
};
@marvinhagemeister marvinhagemeister added the feat New feature or request label Sep 28, 2023
@deer
Copy link
Contributor

deer commented Oct 25, 2023

Everywhere connInfo appears in the source, it's typed as ServeHandlerInfo. But both HandlerContext and MiddlewareHandlerContext are declared with extends ServeHandlerInfo. So the ctx should always have the local and remote addresses available. We spread the connInfo into the contexts here and here.

I think this can be closed as already working. Am I missing something?

@git001
Copy link

git001 commented Nov 2, 2023

let me jump in and ask what's the replacement because it looks deprecated.
connInfo
==> https://github.com/denoland/fresh/blob/main/src/server/compose.ts#L64
===> https://github.com/denoland/fresh/blob/main/src/server/types.ts#L246-L253


export type ServeHandlerInfo = {
  /**
   * Backwards compatible with std/server
   * @deprecated
   */
  localAddr?: Deno.NetAddr;
  remoteAddr: Deno.NetAddr;
};

I ask because some Applications runs behind a reverse proxy and send the client IP in http headers like X-Forwarded-For or Forwarded HTTP Extension which should be parsed and set the remoteAddr with that header value. I think this could be a good candidate for a middleware, couldn't it be?

Another interesting candidate could be the Proxy Protocol

@deer
Copy link
Contributor

deer commented Nov 3, 2023

Hmmm, that sounds like a different issue to me. I still think this original issue is "already working".

Your request sounds like "Fresh should automatically parse X-Fowarded-For headers and use that as remoteAddr". Please correct me if I'm wrong. I guess this would be a cool feature, although it also sounds like a breaking change, if we were to start overriding the existing value with whatever is contained in this header. Instead, perhaps we could do something like this:

export type ServeHandlerInfo = {
  /**
   * Backwards compatible with std/server
   * @deprecated
   */
  localAddr?: Deno.NetAddr;
  remoteAddr: Deno.NetAddr;
  forwardedForAddr?: Deno.NetAddr;
};

But I agree, if this isn't something that gets natively supported in Fresh, it does seem like a good candidate for a plugin providing a single middleware that does this. That way people could just install the plugin, instead of having everyone reinvent the wheel.

Additionally, I think only the localAddr is being deprecated. As far as I can tell, ServeHandlerInfo as a whole is not deprecated.

Edit: I decided to try this out and I did get it working so that ServeHandlerInfo has an optional forwardedForAddr. I did this by modifying the function passed to Deno.serve. @marvinhagemeister, what do you think of this approach? If you're ok with it, I'll write some tests, document it, and open a PR.

@git001
Copy link

git001 commented Nov 3, 2023

Hmmm, that sounds like a different issue to me. I still think this original issue is "already working".

Okay, then sorry for the hijacking. will create a new issue for that or keep the discussion here?

Edit 1:

Your request sounds like "Fresh should automatically parse X-Fowarded-For headers and use that as remoteAddr".

You are right. There are some Frameworks out there which offers this as middleware. Here a example from a golang framework.

https://github.com/go-chi/chi/blob/master/middleware/realip.go

...
func main() {
  r := chi.NewRouter()

  // A good base middleware stack
  r.Use(middleware.RequestID)
  r.Use(middleware.RealIP)
  r.Use(middleware.Logger)
  r.Use(middleware.Recoverer)
...

Edit 2:
Looks like there is also something in rust axum client IP ( https://github.com/imbolc/axum-client-ip )there is also an const https://docs.rs/http/latest/http/header/constant.FORWARDED.html

@deer deer linked a pull request Nov 5, 2023 that will close this issue
@marvinhagemeister
Copy link
Collaborator

I feel like this is something that's easy enough to add as a middleware. I'm not sure if it's a good idea to built that into Fresh by default as it gives a false impression that the header value is trusted. The header value can be easily spoofed and is only somewhat trustworthy if you control the proxy before it and that sets the last value in the X-Forwarded-For header. I feel like Fresh itself cannot make assumptions about where it is run like that.

This should live in a custom middleware for the projects that need it, not in Fresh itself.

@marvinhagemeister marvinhagemeister removed the feat New feature or request label Nov 6, 2023
@git001
Copy link

git001 commented Nov 6, 2023

@marvinhagemeister agree. Is there any middleware overview for fresh? something like "awesome rust" or "awesome golang"? I will try to create such a middleware and create a new issue for that.

@deer
Copy link
Contributor

deer commented Nov 6, 2023

It seems like #1552 is what we need here.

@git001
Copy link

git001 commented Nov 6, 2023

It seems like #1552 is what we need here.

Then I will create a plugin instead of middleware.

@deer
Copy link
Contributor

deer commented Nov 6, 2023

Well, to elaborate, I was referring to your idea about how to share such a middleware. Currently middleware is specific to your particular project. But you can of course create a plugin which provides a single middleware that extracts the header.

The only question is then: how does anyone find your great plugin? We would need some sort of directory, where people can search for Fresh plugins to use. Hence the link to #1552.

Edit: https://fresh.deno.dev/docs/concepts/middleware and https://fresh.deno.dev/docs/concepts/plugins#routes-and-middlewares should provide a starting point on how to get this working. https://github.com/deer/fresh_ga4 is an example of a simple plugin providing a google analytics middleware plugin.

@git001
Copy link

git001 commented Nov 6, 2023

how about to add a "awesome fresh" repo similar to this one for golang https://github.com/avelino/awesome-go

@git001
Copy link

git001 commented Nov 6, 2023

I have now created #2016 and will try to start to create such a plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants