Skip to content

Commit cad360e

Browse files
committed
fix: take the recorded address from the end of the forwarding header
Reading only remoteAddr was honest but inert: behind the ingress it is a cluster-internal address, so every session on Cloud would have read 'Local network' and the whole GeoIP apparatus - the database mount, the download, the deployment change - would have shown nothing. The binary choice was the mistake. A proxy appends the address it actually saw to X-Forwarded-For while a client can only prepend entries of its own, so counting from the end is unforgeable given a known number of hops. That number is now configuration: 0, the default, keeps using the connection's address and is right for anyone running Tolgee without a proxy; the deployment sets 1 for nginx-ingress. Setting it too high is the failure mode, and the property says so. Tests cover the case that matters - a client prepending its own entry cannot displace the address the proxy observed - plus both fallbacks.
1 parent 23485e6 commit cad360e

3 files changed

Lines changed: 114 additions & 14 deletions

File tree

backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/SessionAuditProperties.kt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,18 @@ class SessionAuditProperties {
4949
)
5050
var purgeEnabled: Boolean = true
5151

52+
@DocProperty(
53+
description =
54+
"How many reverse proxies sit in front of Tolgee. Addresses recorded as evidence - the " +
55+
"session list and the authentication audit log - are read this many entries from the end " +
56+
"of `X-Forwarded-For`, because a proxy appends the address it actually saw while a client " +
57+
"can only prepend entries of its own. Leave at 0 when nothing proxies Tolgee, which uses " +
58+
"the connection's address instead; set it to the number of trusted hops otherwise. Getting " +
59+
"this too high lets clients forge their own address.",
60+
defaultExplanation = "0, no proxy",
61+
)
62+
var trustedProxyCount: Int = 0
63+
5264
@DocProperty(
5365
description =
5466
"Absolute path to a MaxMind-format GeoIP city database (`.mmdb`), used to show an approximate " +

backend/data/src/main/kotlin/io/tolgee/util/RequestIpProvider.kt

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,45 @@
11
package io.tolgee.util
22

3+
import io.tolgee.configuration.tolgee.TolgeeProperties
34
import org.springframework.stereotype.Component
45
import org.springframework.web.context.request.RequestContextHolder
56
import org.springframework.web.context.request.ServletRequestAttributes
67

78
@Component
8-
class RequestIpProvider {
9+
class RequestIpProvider(
10+
private val tolgeeProperties: TolgeeProperties,
11+
) {
912
/**
10-
* The address the connection actually came from, which a client cannot choose - unlike the
11-
* forwarding headers [getClientIp] reads. Anything that ends up in the audit trail or in front of
12-
* a user as evidence uses this, the same value the rate limiter buckets on.
13+
* The address to record as evidence - what the session list shows and the audit log keeps.
1314
*
14-
* Behind a proxy this is the proxy's own address, which is useless for locating anyone but is at
15-
* least never a lie. `server.forward-headers-strategy` would make the container derive it from
16-
* `X-Forwarded-For` instead - do not set it unless the proxy in front is known to overwrite that
17-
* header rather than append to it, because otherwise it hands the client back control of this
18-
* value and of the rate limiter's bucket key.
15+
* A proxy appends the address it actually observed to `X-Forwarded-For`, while a client can only
16+
* prepend entries of its own, so counting from the end of the list is what makes the value
17+
* unforgeable. With no proxy configured the connection's own address is used, which is the same
18+
* value the rate limiter buckets on. Never the front of the list: that entry is whatever the
19+
* caller chose to send.
1920
*/
2021
fun getTrustedClientIp(): String? {
21-
if (RequestContextHolder.getRequestAttributes() == null) {
22-
return null
23-
}
24-
val request = (RequestContextHolder.getRequestAttributes() as ServletRequestAttributes).request
25-
return request.remoteAddr?.take(MAX_IP_LENGTH)
22+
val request = currentRequest() ?: return null
23+
val hops = tolgeeProperties.authentication.sessionAudit.trustedProxyCount
24+
if (hops <= 0) return request.remoteAddr?.take(MAX_IP_LENGTH)
25+
26+
val forwarded =
27+
request
28+
.getHeader("X-Forwarded-For")
29+
?.split(",")
30+
?.map { it.trim() }
31+
?.filter { it.isNotEmpty() }
32+
?: return request.remoteAddr?.take(MAX_IP_LENGTH)
33+
34+
return forwarded
35+
.getOrNull(forwarded.size - hops)
36+
?.take(MAX_IP_LENGTH)
37+
?: request.remoteAddr?.take(MAX_IP_LENGTH)
38+
}
39+
40+
private fun currentRequest(): jakarta.servlet.http.HttpServletRequest? {
41+
val attributes = RequestContextHolder.getRequestAttributes() ?: return null
42+
return (attributes as ServletRequestAttributes).request
2643
}
2744

2845
fun getClientIp(): String? {
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
package io.tolgee.util
2+
3+
import io.tolgee.configuration.tolgee.AuthenticationProperties
4+
import io.tolgee.configuration.tolgee.SessionAuditProperties
5+
import io.tolgee.configuration.tolgee.TolgeeProperties
6+
import io.tolgee.testing.assert
7+
import org.junit.jupiter.api.AfterEach
8+
import org.junit.jupiter.api.Test
9+
import org.springframework.mock.web.MockHttpServletRequest
10+
import org.springframework.web.context.request.RequestContextHolder
11+
import org.springframework.web.context.request.ServletRequestAttributes
12+
13+
class RequestIpProviderTest {
14+
@AfterEach
15+
fun cleanup() {
16+
RequestContextHolder.resetRequestAttributes()
17+
}
18+
19+
@Test
20+
fun `without a proxy it reports the address the connection came from`() {
21+
request(remoteAddr = "203.0.113.7", forwardedFor = "8.8.8.8")
22+
23+
provider(trustedProxyCount = 0).getTrustedClientIp().assert.isEqualTo("203.0.113.7")
24+
}
25+
26+
/**
27+
* The whole point of counting from the end: a client that prepends its own entry cannot push
28+
* itself into the position the proxy writes.
29+
*/
30+
@Test
31+
fun `behind one proxy a forged entry cannot displace the observed address`() {
32+
request(remoteAddr = "10.0.0.1", forwardedFor = "8.8.8.8, 203.0.113.7")
33+
34+
provider(trustedProxyCount = 1).getTrustedClientIp().assert.isEqualTo("203.0.113.7")
35+
}
36+
37+
@Test
38+
fun `a header shorter than the configured hops falls back to the connection`() {
39+
request(remoteAddr = "10.0.0.1", forwardedFor = "203.0.113.7")
40+
41+
provider(trustedProxyCount = 2).getTrustedClientIp().assert.isEqualTo("10.0.0.1")
42+
}
43+
44+
@Test
45+
fun `a missing header falls back to the connection`() {
46+
request(remoteAddr = "10.0.0.1", forwardedFor = null)
47+
48+
provider(trustedProxyCount = 1).getTrustedClientIp().assert.isEqualTo("10.0.0.1")
49+
}
50+
51+
private fun request(
52+
remoteAddr: String,
53+
forwardedFor: String?,
54+
) {
55+
val request = MockHttpServletRequest()
56+
request.remoteAddr = remoteAddr
57+
forwardedFor?.let { request.addHeader("X-Forwarded-For", it) }
58+
RequestContextHolder.setRequestAttributes(ServletRequestAttributes(request))
59+
}
60+
61+
private fun provider(trustedProxyCount: Int): RequestIpProvider {
62+
val properties =
63+
TolgeeProperties().apply {
64+
authentication =
65+
AuthenticationProperties().apply {
66+
sessionAudit = SessionAuditProperties().apply { this.trustedProxyCount = trustedProxyCount }
67+
}
68+
}
69+
return RequestIpProvider(properties)
70+
}
71+
}

0 commit comments

Comments
 (0)