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

Support for trusted proxy headers #504

Open
1 task done
harlan-zw opened this issue Aug 10, 2023 · 9 comments
Open
1 task done

Support for trusted proxy headers #504

harlan-zw opened this issue Aug 10, 2023 · 9 comments
Labels
discussion enhancement New feature or request

Comments

@harlan-zw
Copy link
Contributor

Describe the feature

As a module author, for maximum compatibility, it makes life easier to just use the proxy headers and fallback to node specifics. The issue is that these won't be safe in all environments and can be spoofed (i.e x-forwarded-for)

To solve this we need a simple implementation that:

  1. allows users to provide a list of trusted proxies
  2. allows module authors to generate the headers using this list of trusted proxies

I think combining 1 and 2 may be difficult but maybe you have some ideas on that. As a simple first implementation a getRequestTrustedHeaders(event, trustedProxies) may be enough.

Inspo: https://laravel.com/docs/master/requests#configuring-trusted-proxies

Additional information

  • Would you be willing to help implement this feature?
@pi0
Copy link
Member

pi0 commented Aug 11, 2023

We have getRequestHost({ xForwardedHost: true }). Main benefit of using utils for each semi-standard header likt this is that we can support other vendor header as same name. How do you feel about introducing smaller utils like getRequestIP instead?

@harlan-zw
Copy link
Contributor Author

harlan-zw commented Aug 11, 2023

Not sure if this explanation is answering the issue about trusting easily spoofed headers.

H3 should provide a small util function to only get the trusted values, similar to https://github.com/symfony/http-foundation/blob/6.3/Request.php#L783

It would then be on frameworks to allow configuring the trusted proxies and on modules to use that config. Nitro is a good candidate for this, as most of the providers are handling these headers.

Did you want me to make a seperate issue there?

@harlan-zw harlan-zw mentioned this issue Aug 11, 2023
8 tasks
@pi0
Copy link
Member

pi0 commented Aug 14, 2023

I am up to supporting a special event context property such as event.context.isTrustedProxy to automatically enable headers detection however it should be user responsibility solely to write logic. Based on server setup it is different how to determine trustibility.

@harlan-zw
Copy link
Contributor Author

harlan-zw commented Aug 14, 2023

I am up to supporting a special event context property such as event.context.isTrustedProxy to automatically enable headers detection however it should be user responsibility solely to write logic. Based on server setup it is different how to determine trustibility.

Yes, I'm not concerned about users' usage, but about any integration code relying on these headers to work.

Consider this as an example: Nitro implements an in-built rate limiter, to effectively rate limit we need to know the trusted IP of the request. Nitro has no current way to know if it can trust the connecting IP, generically. This is because the x-forwarded-for header can easily be spoofed on some servers without proxies that use it.

Now blow this example up to Nuxt modules and all the other ways we need to know trusted IPs then you should have a better understanding of the problem. The solution requires multiple layers to solve, but as a starting point, we could introduce a way to filter for trusted headers similar to https://github.com/symfony/http-foundation/blob/6.3/Request.php#L2004.

Maybe this issue is better solved completely at a nitro level though

@pi0
Copy link
Member

pi0 commented Aug 14, 2023

Thanks for explaining your use cases. I think it makes sense to introduce a standard context flag down the line from h3 directly so that the ecosystem (nitro, nuxt, nuxt modules) can rely on it to see either they can trust + h3 itself to optimize it's behavior when proxy is trusted to use the header for default event.url 💯

What I am not sure about and hesitated, is the logic to detect if we can trust proxy being directly in h3. it might be in nitro because nitro is provider aware and can include logic for this. same as symfony which is more a full-featured framework. h3 is too way low-end for this.

What i suggest is to:

  • Introduce event.context.trustedHeaders: string[] | undefined
  • Use trusted headers for internal utils behavior auto adjustment
  • Propose a plugin for nitro that can auto set event.context.trustedHeaders

I am still up to support any idea that can be on h3 level, platform agnostic and surely secure as we promise the function is!

@pi0 pi0 added enhancement New feature or request discussion labels Aug 14, 2023
Copy link
Member

atinux commented Aug 14, 2023

I really like this discussion :)

I like the event.context.trusedHeaders option, not sure if the trustedHeaders could be given when creating the app though?

h3/src/app.ts

Lines 50 to 62 in 588c8a3

export interface AppOptions {
debug?: boolean;
onError?: (error: H3Error, event: H3Event) => any;
onRequest?: (event: H3Event) => void | Promise<void>;
onBeforeResponse?: (
event: H3Event,
response: { body?: unknown },
) => void | Promise<void>;
onAfterResponse?: (
event: H3Event,
response?: { body?: unknown },
) => void | Promise<void>;
}

I don't know in term of performance if this will have an impact or not

@Hebilicious
Copy link
Member

@harlan-zw Would that make sense if event.context.trustedProxies: string[] was introduced at the h3 level, and if the array is not empty, when using getHeaders or anything headers related, h3 would check if the ip is in the trustedProxies ?

Then it would be Nuxt/Nitro/Modules responsibility to set trustedProxies (in a plugin or in a middleware)

Doing that way might make it easier for adoption.

@harlan-zw
Copy link
Contributor Author

@harlan-zw Would that make sense if event.context.trustedProxies: string[] was introduced at the h3 level, and if the array is not empty, when using getHeaders or anything headers related, h3 would check if the ip is in the trustedProxies ?

Then it would be Nuxt/Nitro/Modules responsibility to set trustedProxies (in a plugin or in a middleware)

Doing that way might make it easier for adoption.

Seems like a good way to do it 👍

@Baroshem
Copy link

Baroshem commented Aug 21, 2023

Hey guys, great inititative!

Leaving here an issue that was created some time ago in the NuxtSecurity repo that could be useful here as well -> Baroshem/nuxt-security#83

I will update Baroshem/nuxt-security#196 once this will be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants