fix: Move SPRING_REDIRECT_URI cookie invalidation to correct place

jeskepetr · jeskepetr · commit 66eaebb20834 · 2025-04-14T15:07:58.000+02:00
Invalidating SPRING_REDIRECT_URI cookie right after the cookie is read.

JIRA: LX-1022
risk: high
diff --git a/gooddata-server-oauth2-autoconfigure/src/main/kotlin/CookieServerRequestCache.kt b/gooddata-server-oauth2-autoconfigure/src/main/kotlin/CookieServerRequestCache.kt
@@ -18,14 +18,12 @@
  */
 package com.gooddata.oauth2.server
 
+import java.net.URI
 import org.springframework.http.HttpMethod
-import org.springframework.http.HttpStatus
-import org.springframework.http.server.reactive.ServerHttpRequest
 import org.springframework.security.web.server.savedrequest.ServerRequestCache
 import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatchers
 import org.springframework.web.server.ServerWebExchange
 import reactor.core.publisher.Mono
-import java.net.URI
 
 /**
  * An implementation of [ServerRequestCache] that saves the requested URI in a cookie.
@@ -51,22 +49,7 @@ class CookieServerRequestCache(private val cookieService: ReactiveCookieService)
         Mono.just(exchange)
             .flatMap { cookieService.decodeCookie(it, SPRING_REDIRECT_URI) }
             .map { URI.create(it) }
+            .doOnNext { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
 
-    override fun removeMatchingRequest(exchange: ServerWebExchange): Mono<ServerHttpRequest> {
-        return Mono.just(exchange)
-            .flatMap { webExchange ->
-                if (webExchange.response.statusCode == HttpStatus.UNAUTHORIZED) {
-                    // For 401 responses, always preserve the redirect URI
-                    // If there's an existing one, keep it
-                    // If not, the saveRequest() will set it later
-                    Mono.just(webExchange.request)
-                } else {
-                    // For non-401 responses (like successful authentication),
-                    // we can safely invalidate the cookie as we don't need it anymore
-                    Mono.just(webExchange)
-                        .doOnNext { cookieService.invalidateCookie(it, SPRING_REDIRECT_URI) }
-                        .thenReturn(webExchange.request)
-                }
-            }
-    }
+    override fun removeMatchingRequest(exchange: ServerWebExchange) = Mono.just(exchange.request)
 }
diff --git a/gooddata-server-oauth2-autoconfigure/src/test/kotlin/CookieServerRequestCacheTest.kt b/gooddata-server-oauth2-autoconfigure/src/test/kotlin/CookieServerRequestCacheTest.kt
@@ -23,10 +23,12 @@ import io.mockk.slot
 import io.mockk.spyk
 import io.mockk.verify
 import io.netty.handler.codec.http.cookie.CookieHeaderNames
+import java.net.URI
+import java.time.Duration
+import java.time.Instant
 import org.intellij.lang.annotations.Language
 import org.junit.jupiter.api.Test
 import org.springframework.http.HttpCookie
-import org.springframework.http.HttpStatus
 import org.springframework.mock.http.server.reactive.MockServerHttpRequest
 import org.springframework.mock.web.server.MockServerWebExchange
 import org.springframework.util.CollectionUtils
@@ -35,10 +37,6 @@ import reactor.core.publisher.Mono
 import strikt.api.expectThat
 import strikt.assertions.isEqualTo
 import strikt.assertions.isTrue
-import java.net.URI
-import java.time.Duration
-import java.time.Instant
-import strikt.assertions.isNotNull
 
 internal class CookieServerRequestCacheTest {
 
@@ -68,7 +66,9 @@ internal class CookieServerRequestCacheTest {
     """
 
     private val client: AuthenticationStoreClient = mockk {
-        mockCookieSecurityProperties(this, ORG_ID,
+        mockCookieSecurityProperties(
+            this,
+            ORG_ID,
             CookieSecurityProperties(
                 keySet = CleartextKeysetHandle.read(JsonKeysetReader.withBytes(keyset.toByteArray())),
                 lastRotation = Instant.now(),
@@ -100,22 +100,6 @@ internal class CookieServerRequestCacheTest {
         verify(exactly = 1) { cookieService.createCookie(exchange, SPRING_REDIRECT_URI, any()) }
     }
 
-    @Test
-    fun `should remove redirect URI from cookies`() {
-        val request = MockServerHttpRequest.get("http://localhost/requestURI").queryParam("query", "true").build()
-        val exchange = MockServerWebExchange.from(request)
-        every { cookieService.invalidateCookie(any(), any()) } returns Unit
-
-        val matchingRequest = cache.removeMatchingRequest(exchange)
-
-        expectThat(matchingRequest.blockOptional()) {
-            get { isPresent }.isTrue()
-            get { get() }.isEqualTo(request)
-        }
-
-        verify(exactly = 1) { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
-    }
-
     @Test
     fun `should not load redirect URI when nothing is stored in cookies`() {
         val exchange = MockServerWebExchange.from(
@@ -169,11 +153,10 @@ internal class CookieServerRequestCacheTest {
     }
 
     @Test
-    fun `should preserve redirect URI during 401 response`() {
+    fun `should invalidate cookie after reading redirect URI`() {
         val webExchange = mockk<ServerWebExchange> {
             every { request.uri.host } returns "localhost"
             every { attributes[OrganizationWebFilter.ORGANIZATION_CACHE_KEY] } returns Organization(ORG_ID)
-            every { response.statusCode } returns HttpStatus.UNAUTHORIZED
         }
 
         val redirect = "/requestURI?query=true"
@@ -182,36 +165,12 @@ internal class CookieServerRequestCacheTest {
                 HttpCookie(SPRING_REDIRECT_URI, cookieSerializer.encodeCookieBlocking(webExchange, redirect))
             )
         )
-        exchange.response.statusCode = HttpStatus.UNAUTHORIZED
 
-        val request = cache.removeMatchingRequest(exchange).block()
-
-        // Verify the cookie was not invalidated
-        verify(exactly = 0) { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
-        expectThat(request).isNotNull()
-    }
-
-    @Test
-    fun `should clear redirect URI for non-401 response`() {
-        val webExchange = mockk<ServerWebExchange> {
-            every { request.uri.host } returns "localhost"
-            every { attributes[OrganizationWebFilter.ORGANIZATION_CACHE_KEY] } returns Organization(ORG_ID)
-            every { response.statusCode } returns HttpStatus.FOUND
-        }
-
-        val redirect = "/requestURI?query=true"
-        val exchange = MockServerWebExchange.from(
-            MockServerHttpRequest.get("http://localhost/").cookie(
-                HttpCookie(SPRING_REDIRECT_URI, cookieSerializer.encodeCookieBlocking(webExchange, redirect))
-            )
-        )
-        exchange.response.statusCode = HttpStatus.FOUND
+        val uri = cache.getRedirectUri(exchange).block()
 
-        val request = cache.removeMatchingRequest(exchange).block()
+        expectThat(uri).isEqualTo(URI.create(redirect))
 
-        // Verify the cookie was invalidated
         verify(exactly = 1) { cookieService.invalidateCookie(exchange, SPRING_REDIRECT_URI) }
-        expectThat(request).isNotNull()
     }
 
     companion object {
diff --git a/gooddata-server-oauth2-autoconfigure/src/test/kotlin/UserContextWebFluxTest.kt b/gooddata-server-oauth2-autoconfigure/src/test/kotlin/UserContextWebFluxTest.kt
@@ -23,6 +23,8 @@ import com.ninjasquad.springmockk.SpykBean
 import io.mockk.every
 import io.mockk.mockk
 import io.mockk.verify
+import java.time.Duration
+import java.time.Instant
 import net.javacrumbs.jsonunit.core.util.ResourceUtils
 import org.intellij.lang.annotations.Language
 import org.junit.jupiter.api.Test
@@ -49,8 +51,6 @@ import org.springframework.web.server.ResponseStatusException
 import org.springframework.web.server.ServerWebExchange
 import reactor.core.publisher.Mono
 import reactor.util.context.Context
-import java.time.Duration
-import java.time.Instant
 
 @WebFluxTest(properties = ["spring.security.oauth2.client.applogin.allow-redirect=https://localhost:8443"])
 @Import(ServerOAuth2AutoConfiguration::class, UserContextWebFluxTest.Config::class)
@@ -512,7 +512,6 @@ class UserContextWebFluxTest(
             .exchange()
             .expectStatus().isFound
             .expectHeader().location("/")
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     @Test
@@ -543,7 +542,6 @@ class UserContextWebFluxTest(
             .exchange()
             .expectStatus().isFound
             .expectHeader().location("/userReturnTo")
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     @Test
@@ -582,7 +580,6 @@ class UserContextWebFluxTest(
             .exchange()
             .expectStatus().isFound
             .expectHeader().location("/")
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     @Test
@@ -621,7 +618,6 @@ class UserContextWebFluxTest(
             .exchange()
             .expectStatus().isFound
             .expectHeader().location("/userReturnTo")
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     @Test
@@ -634,7 +630,6 @@ class UserContextWebFluxTest(
         webClient.post().uri("http://localhost/logout")
             .exchange()
             .expectStatus().isEqualTo(HttpStatus.METHOD_NOT_ALLOWED)
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     @Test
@@ -647,7 +642,6 @@ class UserContextWebFluxTest(
         webClient.post().uri("http://localhost/logout/all")
             .exchange()
             .expectStatus().isEqualTo(HttpStatus.METHOD_NOT_ALLOWED)
-            .expectCookie().exists("SPRING_REDIRECT_URI")
     }
 
     private fun encodeCookieBlocking(authenticationToken: String) =
