Skip to content

Commit b617ed0

Browse files
committed
fix: record the address the connection came from, not the one it claims
RequestIpProvider reads X-Forwarded-For and friends, which any client can set. That was harmless while it only fed Sentry and Plausible, but this changeset promoted it to user_session.ip, to the GeoIP lookup behind the location shown to the user, and to auth_audit_event.ip on every login and failed login. An attacker signing in with stolen credentials could send the victim's usual home address and appear in the session list as indistinguishable from them - the one signal the screen exists to give, under the attacker's control. The codebase already draws this line: RateLimitInterceptor buckets on request.remoteAddr because it needs a value the client cannot choose. The session and audit paths now read the same value, through a separate accessor; the analytics callers keep the old one. Behind a proxy remoteAddr is the proxy unless the container is told which forwarded headers to trust, so the deployment sets forward-headers-strategy. That also fixes rate limiting, which until now bucketed every request behind the ingress together. Deleting a user now removes their sessions. They carry the person's IP, user agent and city, there is nothing left to revoke, and the foreign key that used to cascade is gone - which is what the lifecycle test was still asserting.
1 parent 9cc9d75 commit b617ed0

8 files changed

Lines changed: 37 additions & 13 deletions

File tree

backend/app/src/test/kotlin/io/tolgee/security/session/LoginFailureAuditTest.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,11 @@ class LoginFailureAuditTest : AuthorizedControllerTest() {
137137
MockMvcRequestBuilders
138138
.post("/api/public/generatetoken")
139139
.contentType(MediaType.APPLICATION_JSON)
140-
.header("X-Forwarded-For", TEST_IP)
141140
.header("User-Agent", TEST_USER_AGENT)
142-
.content(mapper.writeValueAsString(body)),
141+
.with {
142+
it.remoteAddr = TEST_IP
143+
it
144+
}.content(mapper.writeValueAsString(body)),
143145
)
144146
}
145147

backend/app/src/test/kotlin/io/tolgee/security/session/UserSessionLifecycleTest.kt

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -231,18 +231,15 @@ class UserSessionLifecycleTest : AuthorizedControllerTest() {
231231
}
232232

233233
@Test
234-
fun `sessions cascade away with the user while audit events survive`() {
234+
fun `deleting the user drops their sessions while audit events survive`() {
235235
val user = dbPopulator.createUserIfNotExists("audit-survivor@tolgee.io")
236236
withRequestContext {
237237
jwtService.emitToken(user.id, type = UserSessionType.LOGIN_NATIVE)
238238
}
239239
eventsOf(user.id, AuthAuditEventType.LOGIN).assert.hasSize(1)
240240

241241
executeInNewTransaction {
242-
entityManager
243-
.createNativeQuery("delete from user_account where id = :id")
244-
.setParameter("id", user.id)
245-
.executeUpdate()
242+
userAccountService.delete(userAccountService.get(user.id))
246243
}
247244

248245
userSessionRepository
@@ -328,9 +325,11 @@ class UserSessionLifecycleTest : AuthorizedControllerTest() {
328325
org.springframework.test.web.servlet.request.MockMvcRequestBuilders
329326
.post("/api/public/generatetoken")
330327
.contentType(org.springframework.http.MediaType.APPLICATION_JSON)
331-
.header("X-Forwarded-For", ip)
332328
.header("User-Agent", userAgent)
333-
.content(
329+
.with {
330+
it.remoteAddr = ip
331+
it
332+
}.content(
334333
mapper.writeValueAsString(
335334
mapOf("username" to testData.user.username, "password" to "admin"),
336335
),
@@ -344,7 +343,7 @@ class UserSessionLifecycleTest : AuthorizedControllerTest() {
344343
fn: () -> T,
345344
): T {
346345
val request = MockHttpServletRequest()
347-
ip?.let { request.addHeader("X-Forwarded-For", it) }
346+
ip?.let { request.remoteAddr = it }
348347
userAgent?.let { request.addHeader("User-Agent", it) }
349348
RequestContextHolder.setRequestAttributes(ServletRequestAttributes(request))
350349
try {

backend/data/src/main/kotlin/io/tolgee/repository/UserSessionRepository.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,4 +163,6 @@ interface UserSessionRepository : JpaRepository<UserSession, Long> {
163163
): List<Long>
164164

165165
fun deleteAllByIdIn(ids: Collection<Long>)
166+
167+
fun deleteAllByUserAccountId(userAccountId: Long)
166168
}

backend/data/src/main/kotlin/io/tolgee/security/authentication/UserSessionAccessManager.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class UserSessionAccessManager(
8585
userAccountId = userAccountId,
8686
expiresAt = expiresAt,
8787
actingUserAccountId = actingUserAccountId,
88-
ip = requestIpProvider.getClientIp(),
88+
ip = requestIpProvider.getTrustedClientIp(),
8989
userAgent = requestUserAgentProvider.getUserAgent(),
9090
)
9191
}

backend/data/src/main/kotlin/io/tolgee/service/security/AuthAuditService.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ class AuthAuditService(
6262
this.actingUserAccountId = actingUserAccountId ?: currentActingUserId()
6363
this.deviceId = deviceId ?: currentDeviceId()
6464
this.targetId = targetId
65-
this.ip = requestIpProvider.getClientIp()
65+
this.ip = requestIpProvider.getTrustedClientIp()
6666
this.userAgent = requestUserAgentProvider.getUserAgent()
6767
this.data = data
6868
}

backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import io.tolgee.model.views.ExtendedUserAccountInProject
3030
import io.tolgee.model.views.UserAccountInProjectView
3131
import io.tolgee.model.views.UserAccountWithOrganizationRoleView
3232
import io.tolgee.repository.UserAccountRepository
33+
import io.tolgee.repository.UserSessionRepository
3334
import io.tolgee.service.AiPlaygroundResultService
3435
import io.tolgee.service.AvatarService
3536
import io.tolgee.service.EmailVerificationService
@@ -78,6 +79,8 @@ class UserAccountService(
7879
private val mfaService: MfaService,
7980
@Lazy
8081
private val authAuditService: AuthAuditService,
82+
@Lazy
83+
private val userSessionRepository: UserSessionRepository,
8184
) : Logging {
8285
@Autowired
8386
@Lazy
@@ -226,6 +229,9 @@ class UserAccountService(
226229
}
227230

228231
private fun deleteWithFetchedData(toDelete: UserAccount) {
232+
// The audit trail deliberately outlives the account, but a session row is not audit - it
233+
// carries the person's IP, user agent and city, and there is nothing left to revoke.
234+
userSessionRepository.deleteAllByUserAccountId(toDelete.id)
229235
toDelete.emailVerification?.let {
230236
entityManager.remove(it)
231237
}

backend/data/src/main/kotlin/io/tolgee/service/security/UserSessionService.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class UserSessionService(
4848
isRefresh: Boolean,
4949
) {
5050
val now = currentDateProvider.date
51-
val ip = requestIpProvider.getClientIp()
51+
val ip = requestIpProvider.getTrustedClientIp()
5252
val location = geoIpResolver.resolve(ip)
5353
userSessionRepository.upsert(
5454
id = sequenceIdProvider.next(),

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,21 @@ import org.springframework.web.context.request.ServletRequestAttributes
66

77
@Component
88
class RequestIpProvider {
9+
/**
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. Behind a proxy it is
13+
* the proxy's address unless the deployment sets `server.forward-headers-strategy`, which makes
14+
* the container resolve it from the forwarded headers it is willing to trust.
15+
*/
16+
fun getTrustedClientIp(): String? {
17+
if (RequestContextHolder.getRequestAttributes() == null) {
18+
return null
19+
}
20+
val request = (RequestContextHolder.getRequestAttributes() as ServletRequestAttributes).request
21+
return request.remoteAddr?.take(MAX_IP_LENGTH)
22+
}
23+
924
fun getClientIp(): String? {
1025
if (RequestContextHolder.getRequestAttributes() == null) {
1126
return null

0 commit comments

Comments
 (0)