Add port number to RealIP middleware #518
Conversation
Fixes #453 Downside: this is not entirely backwards compatible; e.g. my app will actually break because it *assumes* you're running it behind a proxy and won't remove the port. Then again, it already breaks for people not using a proxy, but in certain scenarios (e.g. some closed source app) that will probably never really happen. So ... maybe something for v5? Just thought I'd open a PR in any case.
| @@ -29,7 +30,7 @@ var xRealIP = http.CanonicalHeaderKey("X-Real-IP") | |||
| func RealIP(h http.Handler) http.Handler { | |||
| fn := func(w http.ResponseWriter, r *http.Request) { | |||
| if rip := realIP(r); rip != "" { | |||
| r.RemoteAddr = rip | |||
| r.RemoteAddr = net.JoinHostPort(rip, "0") | |||
There was a problem hiding this comment.
Why would we add :0 port to the address? This doesn't make sense to me. Can you clarify?
There was a problem hiding this comment.
Because that's the format Go uses for RemoteAddr; see #453
There was a problem hiding this comment.
I am wondering if the use of port 0 is actually the best way to go?
Since chi is an HTTP router - wouldn't 1.2.3.4 be equal to 1.2.3.4:80 (the default HTTP port)?
Or am I missing something?
There was a problem hiding this comment.
It's normally set to the source port, not destination port. Any value is "wrong", and at least 0 is "clearly wrong".
|
I think the best solution is to stop overwriting r.RemoteAddr from RealIP middleware and instead setting a value on the request context |
@pkieltyka I think the problem with that approach is that it requires downstream consumers (either other middlewares or handlers) to be aware that the realip middleware is higher in the chain. It also means that removing the middleware (e.g. commenting out the There are also some other disadvantages of the current approach (such as being vulnerable to spoofing attacks and having limited support for different header types) for which I'd like to propose some changes (I'll open a separate issue for that). Thanks for this awesome library! |
Fixes #453
Downside: this is not entirely backwards compatible; e.g. my app will
actually break because it assumes you're running it behind a proxy and
won't remove the port. Then again, it already breaks for people not
using a proxy, but in certain scenarios (e.g. some closed source app)
that will probably never really happen.
So ... maybe something for v5? Just thought I'd open a PR in any case.