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

app.UseForwardedHeaders() does not use X-Forwarded headers #6005

Closed
kevinoid opened this issue Feb 3, 2018 · 4 comments
Closed

app.UseForwardedHeaders() does not use X-Forwarded headers #6005

kevinoid opened this issue Feb 3, 2018 · 4 comments
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares bug This issue describes a behavior which is not expected - a bug.

Comments

@kevinoid
Copy link

kevinoid commented Feb 3, 2018

As @ygoe mentioned in dotnet/AspNetCore.Docs#2384 (comment), the behavior of .UseForwardedHeaders without arguments is unexpected and counter-intuitive. Since I was just caught by this as well, I decided to open this issue. Would it be possible to either have .UseForwardedHeaders default to ForwardedHeaders.All or to remove the zero-argument overload of this method?

Thanks,
Kevin

@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2018

Unlikely, this choice was explicit. The x-forwarded headers are actually pretty dangerous if not used carefully, they can lead to spoofing attacks. As such the configuration needs to be explicit.

The zero-argument overload is for use with options configured in ConfigureServices rather than inline.

It may make sense for the middleware to throw on startup if ForwardedHeaders is still set to None.

@kevinoid
Copy link
Author

kevinoid commented Feb 3, 2018

The zero-argument overload is for use with options configured in ConfigureServices rather than inline.

Good to know. Thanks. It could be helpful to add that to the method comment.

It may make sense for the middleware to throw on startup if ForwardedHeaders is still set to None.

That would work for me.

@mattnewell
Copy link

mattnewell commented Sep 10, 2018

We were also caught by this. I'd add a +1 to this suggestion:

It may make sense for the middleware to throw on startup if ForwardedHeaders is still set to None.

See also dotnet/AspNetCore.Docs#2384 (comment)

@aspnet-hello aspnet-hello transferred this issue from aspnet/BasicMiddleware Dec 19, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0 milestone Dec 19, 2018
@aspnet-hello aspnet-hello added area-middleware bug This issue describes a behavior which is not expected - a bug. labels Dec 19, 2018
@shirhatti
Copy link
Contributor

I realize this API in its current iteration is a pit of failure, but we've doced the behavior and at this point do not want to introduce a breaking change.

@shirhatti shirhatti removed this from the 3.0.0 milestone Jan 11, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

7 participants