roadrunner-server / roadrunner

🤯 High-performance PHP application server, process manager written in Go and powered with plugins
https://docs.roadrunner.dev
MIT License
7.92k stars 411 forks source link

Resolving IP address via trusted proxy is wrong #183

Closed marliotto closed 5 years ago

marliotto commented 5 years ago
// get real ip passing multiple proxy
func (h *Handler) resolveIP(r *Request) {
    if !h.cfg.IsTrusted(r.RemoteAddr) {
        return
    }

    if r.Header.Get("X-Forwarded-For") != "" {
        for _, addr := range strings.Split(r.Header.Get("X-Forwarded-For"), ",") {
            addr = strings.TrimSpace(addr)
            if h.cfg.IsTrusted(addr) {
                r.RemoteAddr = addr
            }
        }
        return
    }

https://github.com/spiral/roadrunner/blob/master/service/http/handler.go#L149

Hello, I think I found some a bug.

If we trust a proxy we should use last IP address from X-Forwarded-For. We shouldn't check IP address from X-Forwarded-For is trusted or not, but some validation is required.

Also, I think we should change value from X-Forwarded-For, after applied new IP address.

For example 127.0.0.1 is trusted proxy, Roadrunner received request

RemoteAddr = 127.0.0.1 X-Forwarded-For: 203.0.113.195, 70.41.3.18, 150.172.238.178

Then roadrunner should send next request

RemoteAddr = 150.172.238.178 X-Forwarded-For: 203.0.113.195, 70.41.3.18

If X-Forwarded-For has only one IP Address, we should remove that header. For example

Received request

RemoteAddr = 127.0.0.1 X-Forwarded-For: 203.0.113.195

Should to send without X-Forwarded-For

RemoteAddr = 203.0.113.195

About X-Forwarded-For https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For Correct example https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/HttpFoundation/Request.php#L790

P.S. I just php developer, so can not fix it

wolfy-j commented 5 years ago

Addressed in new version by @spudro228