This looks good, can you check failing tests?

Yes, I have checked that. Works on my machine with that ruby version and test seed. Can you trigger rebuild just in case? After that will look more closely.

One issue with supporting Forwarded and trying to remain backwards compatible with X-Forwarded-* is that it can introduce security issues when used with older servers that only support X-Forwarded-*. For example, an attacker can specify the Forwarded header, and if the server sets X-Forwarded-For but rack prefers Forwarded if set, that would result in using attacker specified data. If you are trying to enforce that only certain IP addresses can access a resource through a trusted proxy, this opens up a security issue.

To be safe, if both headers are present, we should only use Forwarded in preference to X-Forwarded-* if the server specifies support for it. This would require an update to SPEC, allowing an entry such as rack.use_forwarded. If the entry is present, the Forwarded header is used and X-Forwarded-* is ignored. Otherwise, X-Forwarded-* is used and Forwarded is ignored.

@tenderlove, @ioquatix Does my analysis here make sense? If so, what are your thoughts on my recommended approach?

Other than that issue, I reviewed this and I'm OK with the changes. The conflict is only due to documentation I added recently, and easily resolved. There are some unrelated changes that would better have been submitted in separate PRs, but I don't think would cause us not to merge this once the security issue is addressed.

@jeremyevans I don't see what support the server implementation would need to have -- its opinions are in REMOTE_ADDR and friends. The (IMO near-insurmountable without explicit configuration) challenge is that we need to know which header was supplied by the immediate-upstream proxy... and we have no means of direct communication there that can be trusted.*

Perhaps, as it's a relatively new header in the real world, it wouldn't be unreasonable for us to declare that an upstream that sets Forwarded is only compliant [and thus supported] if it also guarantees to strip the others -- leaving us free to either treat a mixed header set as we do now, or refuse the request a la https://github.com/rails/rails/blob/7b29bc2179702a04613068e6e6efd6cf7f343440/actionpack/lib/action_dispatch/middleware/remote_ip.rb#L136

* ... unless we can trust all reverse proxies to swallow an incoming Connection header per spec, in which case we could rely on Connection: forwarded. It's my understanding that proxies can be pretty loose with spec behaviour, though -- reverse proxies especially.

@matthewd I think you are correct. This probably cannot be handled by SPEC since it isn't handled by the server itself. Still, we should probably do something to prevent a security issue being introduced. If we assume that proxies that support Forwarded will correctly convert X-Forwarded-* to Forwarded as recommended by the RFC, then the appearance of both could be considered an error. However, as @ioquatix mentioned in a related issue, some proxies may set both to the same values, which also seems reasonable. Maybe the logic should be:

if forwarded_set
  if x_forwarded_set
    if forwarded_set_has_same_info_as_x_forwarded_set
       use either
    else
      raise exception
    end
  else
    use forwarded_set
  end
elsif x_forwarded_set
  use x_forwarded_set
else
  not forwarded, use other header
end

@jeremyevans can you assign this to a milestone, either 2.2 or 3.0 depending on what you think makes the most sense?

If servers need to be on board with the change, I'd say 3.0 wouldn't be a mistake.

@fatkodima do you mind rebasing on master?

@ioquatix I'm sorry, I'm no longer in the context. Feel free to do with this PR whatever you want.

@jeremyevans has created a new PR for this feature/functionality.