X-Forwarded-For parsing does not support more than one proxy, is a dangerous default

[dominics]

Currently, the last IP address in the X-Forwarded-For header is used by default as the originating IP address for the request:

play-guard/module/app/com/digitaltangible/playguard/package.scala

Lines 12 to 19 in 1c4146c

	} yield ip) getOrElse {
	// Consider X-Forwarded-For as most accurate if it exists
	// Since it is easy to forge an X-Forwarded-For, only consider the last ip added by our proxy as the most accurate
	// https://en.wikipedia.org/wiki/X-Forwarded-For
	request.headers.get("X-Forwarded-For").map(_.split(",").last.trim).getOrElse {
	request.remoteAddress
	}

There's quite a few problems with this approach:

• If no IP is appended to the X-Forwarded-For header at all, the user can spoof their IP address to the rate limiter just by creating an X-Forwarded-For header of their own (effectively bypassing the limiter, or DoSing any IP address they like)
  • This will become more common as load-balancers move to RFC 7239 (Forwarded) headers, which aren't supported at all
• It doesn't support configuration of trusted proxies (those that the determination of IP address should "look through")
• etc.

I'd suggest that the default case in the getOrElse (L12) block should just be request.remoteAddress:

• It can't be spoofed by the user, providing a much safer default
• It provides compatibility with the built-in way of determining the actual IP address in situations where there's more than one proxy: Play's play.http.forwarded.trustedProxies setting
  • Which means it gives us RFC 7239 support "for free" too

I'm guessing play.http.forwarded.trustedProxies might not have existed when this code was conceived, but either way, request.remoteAddress is definitely all that needs to be considered now.

[sief]

you're totally right. I wasn't aware that I can achieve the current behaviour with request.remoteAddress by trusting all proxies. Would you like to submit a pull request?