Restore: changed behavior for request.host when multiple X-Forwarded-Host are used

[ylecuyer]

As explained in #1829 commit 290523f changed the behavior of request.host when multiple X-Forwarded-Host are used. This PR restores the previous behavior.

I don't know which behavior is the correct one, but this is a breaking change. If we really want to change from last to first we should issue the proper deprecation warning and include the change in the v3 release. I didn't update the changelog yet because I think it would be best to have this fix in 2.2.4 though I'd prefer to have your insight.

Here is the before after:

Before:

ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" -H "X-Forwarded-Host: host3" -H "X-Forwarded-Host: host4" http://localhost:9292
host1
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" -H "X-Forwarded-Host: host3" http://localhost:9292
host1
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" http://localhost:9292
host1
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" http://localhost:9292
host1

After

ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" -H "X-Forwarded-Host: host3" -H "X-Forwarded-Host: host4" http://localhost:9292
host4
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" -H "X-Forwarded-Host: host3" http://localhost:9292
host3
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" -H "X-Forwarded-Host: host2" http://localhost:9292
host2
ylecuyer@lenovo:~$ curl -X GET -H "X-Forwarded-Host: host1" http://localhost:9292
host1

Also it should be noted that rails is also using last https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/http/url.rb#L216 so for consistency if one is changed the other should be changed too.

cc @ioquatix as you are the author of the initial commit and maybe cc @tenderlove for the rails note above.

[ioquatix]

I don't have a strong opinion either way, but is there an RFC which explains the best value to use? I assume the earlier forwarded for details are closer to the client while later ones are more likely to be proxies.

[misdoro]

Ah, we've also had this issue.
We ended up adding a middleware like

class CustomRemoteIp < ActionDispatch::RemoteIp
  X_REAL_IP_HEADER = 'HTTP_TRUE_CLIENT_IP'

  # Since the IP address may not be needed, we store the object here
  # without calculating the IP to keep from slowing down the majority of
  # requests. For those requests that do need to know the IP, the
  # GetCustomIp#calculate_ip method will calculate the memoized client IP address.
  def call(env)
    req = ActionDispatch::Request.new env
    req.remote_ip = GetCustomIp.new(req, check_ip, proxies)
    @app.call(req.env)
  end

  class GetCustomIp < ActionDispatch::RemoteIp::GetIp
    def calculate_ip
      @req.get_header(X_REAL_IP_HEADER).presence || super
    end
  end
end

as a workaround.

[matthewd]

We strictly must use the last (and thus nearest-to-us proxy) details for X-Forwarded-For, because any nominally-further value cannot be trusted: any originating client can claim to be proxying on behalf of anyone they like (including things like private IPs, or localhost).

That doesn't directly map here, but IMO does alone provide some basis for justifying parallel behaviour. (Especially viewed in light of a #1423-style far future.)

On the particular matter of host, I think it also makes sense to go with the nearest proxy's provided value. That's the proxy that said "oh, a request for host4? I know exactly who to ask for that: I'll forward this along to my good friend localhost:9292, they know all about host4." -- the further removed we get, the less likely it is that the final server will recognise a name from three forwardings away: the whole reason that each proxy in the chain has been switching Host headers is presumably to translate the request to the form the next-deeper server would understand.

[ioquatix]

Would it make sense to add an interface so that users can ask for all forwarded hosts and then can select either the first or last as is useful to their own use case?

[matthewd]

My quick thought would be to push the "clean up something that's nominally a list of IP/host entries" behaviour into a public Rack::Util method, and then say that if someone has a more complex need they can read the header themselves [and we've supplied that cleanup help]. My theory there is that first is a pretty nuanced value, given the question of how many proxies you trust to exist and accurately copy a once-true Host header across, vs complete fabrication from an end client. IMO if you don't want last, you perhaps actually want the Nth-last entry, which might generally be first given well-behaved clients. (And I'd consequently prefer to nudge people toward using last as authoritative, by having to work a bit more to get something else, lest they pick arbitrarily and get surprised later.)

... again, all of that applies less strongly to forwarded host in particular, but I guess I'm reluctant to add a list-of-hosts feature because of the precedent it would create for a matching list-of-client-IPs, and that feels like leaving a foot-gun on the table, where I'd rather just note that there is one kept in the safe.

[ioquatix]

I think the value in standardising an interface here, apart from the ones discussed already, is that internally HTTP has got a defacto standard of x-forwarded-whatever and an official standard forwarded. It would be nice for us to have some way to handle this transparently and security for the application since it's a bit of a mess and I think we prefer avoiding bespoke code that ultimately could end up being insecure, i.e. let's guide applications to do the right thing with an easy to use interface if possible.

[ioquatix]

Based on discussion here, it sounds like it would be a security issue to change this behaviour. Thus we will close it. Thanks for your ideas and contributions.

[ioquatix]

Sorry, I may have mis-understood the advice here.

We strictly must use the last (and thus nearest-to-us proxy) details for X-Forwarded-For, because any nominally-further value cannot be trusted: any originating client can claim to be proxying on behalf of anyone they like (including things like private IPs, or localhost).

This means we should apply this PR. @matthewd do you mind confirming and approving this PR?

[jeremyevans]

I fixed this as part of #1834, can you please review that instead?: https://github.com/rack/rack/pull/1834/files#diff-7ce97931f18a63a4d028696a6f4ba81991644dbc2d70eaa664285264e9a5cd64L360

[jeremyevans]

Implemented by #1834