request.secure is set to false if X-Forwarded-For and X-Forwarded-Proto are of different length

[danishin]

Play Version (2.5.x / etc)

2.5

API (Scala / Java / Neither / Both)

Scala

Operating System (Ubuntu 15.10 / MacOS 10.10 / Windows 10)

MacOS 10.11.5

JDK (Oracle 1.8.0_72, OpenJDK 1.8.x, Azul Zing)

OpenJDK 1.8.x

Expected Behavior

If app is behind multiple reverse proxies, X-Forwarded-Forwill correctly be multiple values but X-Forwarded-Proto might not be (which is actually what is recommended in Django framework as it states reverse proxy to strip out all previous X-Forwarded-Proto and insert single flag.). In this case, where two are of different length and X-Forwarded-Proto is single value, it should use that value for secure flag instead of defaulting to false.

Actual Behavior

secure flag is defaulted to false.

---

My current setting is using heroku on top of cloudflare. Heroku seems to strip out / don't add to existing X-Forwarded-Proto, which is set by cloudflare in front. So even when we use Full SSL for cloudflare (which mandates ssl connection for both heroku - cloudflare and cloudflare - client), X-Forwarded-Proto is still set to single https value, which fails play parser for secure flag and make it defaulted to to false.

[danishin]

Similar Discussion: aspnet/BasicMiddleware#18

[danishin]

Way to test this behavior is by firing curl -I -H 'X-Forwarded-For: 9.9.9.9' https://YOUR_APP.herokuapp.com -Land see secure flag to be false even if we accessed via https. Removing X-Forwarded-For will render securecorrectly.

Also heroku seems to ignore any incoming X-Forwarded-Proto headers and simply use https scheme / port to re-write X-Forwarded-Proto, which will always be a single value.

[gmethvin]

I don't agree with the expected behavior as the default, but we could perhaps allow an option to change the behavior. As explained in the Django documentation you linked, it only makes sense to use the single value for X-Forwarded-Proto if you know the value was replaced in a particular way. The proxy actually needs to know that the request came in via HTTPS and all previous proxies the request was sent through were HTTPS.

We could provide configuration to do something similar to what Django does, which I think is a reasonable suggestion, but it should also come with a stern warning that it could lead to false positives if you don't know what you're doing. We could do it with new configuration like this:

play.http.forwarded.secureHeaders {
  X-Forwarded-Proto = https
}

Of course, you can also avoid this altogether and just create your own function to determine if the request is secure:

implicit class RequestHeaderIsSecure(rh: RequestHeader) extends AnyVal {
  def isSecure: Boolean = rh.headers.get("X-Forwarded-Proto").contains("https")
}

[danishin]

@gmethvin

Thanks for the response.

I agree that the way done by Django is the way to go if at all. Changing the default value to anything else like I said before would lead to many false positives.

I'd love to make PR for this if you think it is reasonable, but I'm not really familiar with Play Framework's codebase and just finding that line took me like half an hour. So would love to see someone familiar with code to do this.

And yes, that's the workaround I'm using in my code for now but it requires many TODOs sprinkled all over the place and definitely not the permanent way to go.

[gmethvin]

One option is to add additional logic in the ForwardedHeaderHandler. We would need to parse some configuration and match configuration values here to decide what to do if X-Forwarded-Proto has the wrong number of entries: https://github.com/playframework/playframework/blob/master/framework/src/play-server/src/main/scala/play/core/server/common/ForwardedHeaderHandler.scala#L188

Another way is with a filter, which would allow you to apply an arbitrary function to determine if the request is secure. The filter would just involve copying the request and overriding the secure flag on the RemoteConnection:

class SecureRequestFilter(isSecure: RequestHeader => Boolean) extends EssentialFilter {
  override def apply(next: EssentialAction) = EssentialAction { rh =>
    next(new RequestHeader {
      override lazy val connection = RemoteConnection(rh.remoteAddress, isSecure(rh), rh.clientCertificateChain)
      override def attrs = rh.attrs
      override def target = rh.target
      override def method = rh.method
      override def headers = rh.headers
      override def version = rh.version
    })
  }
}

(This code is based on current master but the idea should work in previous Play versions that support filters.)

[danishin]

Hmm I'm not sure if the second approach of using filter is a good idea. Most people won't have this problem or even aware that this is even a problem. IMO, letting them to inject arbitray function would just giving too much power to people who don't really know why this exists / what to do with it.

I guess, using filter would definitely be a more generic way to deal with this issue iif there are more issues related to X-Forwarded-Proto other than the one mentioned here.

I think, for now, simple configuration in application.conf would be a much harmless way to go about this (given that it is well documented and marked with big caveat)

[gmethvin]

Most people won't have this problem or even aware that this is even a problem. IMO, letting them to inject arbitray function would just giving too much power to people who don't really know why this exists / what to do with it.

The user would only implement something like this if they already knew they had this problem and we referred to the filter in the documentation. It's an addition on top of the default Play behavior.

I guess, using filter would definitely be a more generic way to deal with this issue iif there are more issues related to X-Forwarded-Proto other than the one mentioned here.

That was the idea. It allows more flexibility to match on combinations of different headers and attributes of the request. Sure, this can be dangerous, but so is basically any configuration-based workaround, and I actually think providing configuration and a built-in behavior in the framework makes it seem more normal.

Also that code is simple enough that we could just provide it as an example in the documentation, no changes to the framework required.

[danishin]

If there are enough use cases for modifying RequestHeader, then using filter seems like good generic way to go as long as it's well documented. Also it means less workaround / hack for the framework itself too, I guess.

All in all, for either case, I think documentation is the key as modifying request header is really playing with fire if done incorrectly. Listing common use cases for need for such filter would be nice.

[tjdett]

I ran into this behaviour in a different circumstance, trying to work out why X-Forwarded-Proto wasn't working, in a case where it was being used without an X-Forwarded-For header.

Adding to confusion, I was using nghttpx which also supports automatic RFC 7239 Forwarded headers, but uses obfuscated identifiers by default (which Play Framework doesn't support).

In neither case is there any log output that suggests why things aren't working.

[richdougherty]

After reviewing a lot of different proxy behaviour I can see that no proxies actually support lists in the X-Forwarded-Proto header. They all either have a header value or they don't.

My plan would be to make the following changes:

1. Add a protocol field to the RemoteConnection class. It would have type Option[String].
2. Don't treat the X-Forwarded-Proto header as a list, since it seems to only ever be a single value anyway.
3. Not all servers which set X-Forwarded-For will set or strip the X-Forwarded-Proto header. To prevent protocol spoofing, require the header to be explicitly enabled in configuration, e.g. play.http.forwarded.proto.header = "X-Forwarded-Proto". If the protocol information is unavailable set the protocol in the RemoteConnection to None.
4. For now I'd leave the RemoteConnection secure field as it is. In other words its logic would be:

   def secure: Boolean = proto match {
     case Some("https") => true
     case _ => false
   }

[francisdb]

Another use case where this causes issues:
App deployed on Google cloud with kubernetes, load balancer and ingress controller https termination.
X-Forwarded-For contains both the ip of the user and the ingress controller where X-Forwarded-Proto contains a single https

[andrii-kovalenko-ct]

Another use case where this causes issues:
App deployed on Google cloud with kubernetes, load balancer and ingress controller https termination.
X-Forwarded-For contains both the ip of the user and the ingress controller where X-Forwarded-Proto contains a single https

same issue!