From 478fab319f43efd0d6fe7e1df893493158164429 Mon Sep 17 00:00:00 2001 From: raccoonback Date: Fri, 14 Mar 2025 08:10:03 +0900 Subject: [PATCH] Prevent double encoding in removeRequestParameter method The removeRequestParameter method was causing URLs to be encoded twice under certain conditions. This fix ensures proper handling of parameters to avoid redundant encoding. Signed-off-by: raccoonback --- .../mvc/filter/BeforeFilterFunctions.java | 7 +- .../filter/BeforeFilterFunctionsTests.java | 172 ++++++++---------- 2 files changed, 84 insertions(+), 95 deletions(-) diff --git a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java index bf4a5180..650f9ca6 100644 --- a/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java +++ b/spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java @@ -45,6 +45,7 @@ import org.springframework.web.server.ResponseStatusException; import org.springframework.web.servlet.function.ServerRequest; import org.springframework.web.util.UriComponentsBuilder; import org.springframework.web.util.UriTemplate; +import org.springframework.web.util.UriUtils; import static org.springframework.cloud.gateway.server.mvc.common.MvcUtils.CIRCUITBREAKER_EXECUTION_EXCEPTION_ATTR; import static org.springframework.util.CollectionUtils.unmodifiableMultiValueMap; @@ -214,10 +215,12 @@ public abstract class BeforeFilterFunctions { MultiValueMap queryParams = new LinkedMultiValueMap<>(request.params()); queryParams.remove(name); + MultiValueMap encodedQueryParams = UriUtils.encodeQueryParams(queryParams); + // remove from uri URI newUri = UriComponentsBuilder.fromUri(request.uri()) - .replaceQueryParams(unmodifiableMultiValueMap(queryParams)) - .build() + .replaceQueryParams(unmodifiableMultiValueMap(encodedQueryParams)) + .build(true) .toUri(); // remove resolved params from request diff --git a/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctionsTests.java b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctionsTests.java index 7944baac..095ec4f9 100644 --- a/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctionsTests.java +++ b/spring-cloud-gateway-server-mvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctionsTests.java @@ -31,168 +31,154 @@ import static org.assertj.core.api.Assertions.assertThat; */ class BeforeFilterFunctionsTests { - @Test - void rewriteRequestParameter() { - MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") - .param("foo", "bar") - .param("baz", "qux") - .buildRequest(null); - - ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); - - ServerRequest result = BeforeFilterFunctions.rewriteRequestParameter("foo", "replacement").apply(request); - - assertThat(result.param("foo")).isPresent().hasValue("replacement"); - assertThat(result.uri().toString()).hasToString("http://localhost/path?baz=qux&foo=replacement"); - } - - @Test - void rewriteOnlyFirstRequestParameter() { - MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") - .param("foo", "bar_1") - .param("foo", "bar_2") - .param("foo", "bar_3") - .param("baz", "qux") - .buildRequest(null); - - ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); - - ServerRequest result = BeforeFilterFunctions.rewriteRequestParameter("foo", "replacement").apply(request); - - assertThat(result.param("foo")).isPresent().hasValue("replacement"); - assertThat(result.uri().toString()).hasToString("http://localhost/path?baz=qux&foo=replacement"); - } - - @Test - void rewriteEncodedRequestParameter() { - MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") - .param("foo[]", "bar") - .param("baz", "qux") - .buildRequest(null); - - ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); - - ServerRequest result = BeforeFilterFunctions.rewriteRequestParameter("foo[]", "replacement[]").apply(request); - - assertThat(result.param("foo[]")).isPresent().hasValue("replacement[]"); - assertThat(result.uri().toString()).hasToString("http://localhost/path?baz=qux&foo%5B%5D=replacement%5B%5D"); - } - - @Test - void rewriteRequestParameterWithEncodedRemainParameters() { - MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") - .param("foo", "bar") - .param("baz[]", "qux[]") - .buildRequest(null); - - ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); - - ServerRequest result = BeforeFilterFunctions.rewriteRequestParameter("foo", "replacement").apply(request); - - assertThat(result.param("foo")).isPresent().hasValue("replacement"); - assertThat(result.uri().toString()).hasToString("http://localhost/path?baz%5B%5D=qux%5B%5D&foo=replacement"); - } - - @Test - void rewriteRequestParameterWithEncodedPath() { - MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path/é/last") - .param("foo", "bar") - .buildRequest(null); - - ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); - - ServerRequest result = BeforeFilterFunctions.rewriteRequestParameter("foo", "replacement").apply(request); - - assertThat(result.param("foo")).isPresent().hasValue("replacement"); - assertThat(result.uri().toString()).hasToString("http://localhost/path/%C3%A9/last?foo=replacement"); - } - @Test void setPath() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/legacy/path") - .buildRequest(null); + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.setPath("/new/path").apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/new/path"); + assertThat(result.uri().toString()).hasToString("http://localhost/new/path"); } @Test void setEncodedPath() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/legacy/path") - .buildRequest(null); + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.setPath("/new/é").apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/new/%C3%A9"); + assertThat(result.uri().toString()).hasToString("http://localhost/new/%C3%A9"); } @Test void setPathWithParameters() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/legacy/path") - .queryParam("foo", "bar") - .buildRequest(null); + .queryParam("foo", "bar") + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.setPath("/new/path").apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/new/path?foo=bar"); + assertThat(result.uri().toString()).hasToString("http://localhost/new/path?foo=bar"); } @Test void setPathWithEncodedParameters() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/legacy/path") - .queryParam("foo[]", "bar[]") - .buildRequest(null); + .queryParam("foo[]", "bar[]") + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.setPath("/new/path").apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/new/path?foo%5B%5D=bar%5B%5D"); + assertThat(result.uri().toString()).hasToString("http://localhost/new/path?foo%5B%5D=bar%5B%5D"); + } + + @Test + void removeRequestParameter() { + MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") + .queryParam("foo", "bar") + .queryParam("baz", "qux") + .buildRequest(null); + + ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); + + ServerRequest result = BeforeFilterFunctions.removeRequestParameter("foo").apply(request); + + assertThat(result.param("foo")).isEmpty(); + assertThat(result.param("baz")).isPresent().hasValue("qux"); + assertThat(result.uri().toString()).hasToString("http://localhost/path?baz=qux"); + } + + @Test + void removeEncodedRequestParameter() { + MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") + .queryParam("foo[]", "bar") + .queryParam("baz", "qux") + .buildRequest(null); + + ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); + + ServerRequest result = BeforeFilterFunctions.removeRequestParameter("foo[]").apply(request); + + assertThat(result.param("foo[]")).isEmpty(); + assertThat(result.param("baz")).isPresent().hasValue("qux"); + assertThat(result.uri().toString()).hasToString("http://localhost/path?baz=qux"); + } + + @Test + void removeRequestParameterWithEncodedRemainParameters() { + MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/path") + .queryParam("foo", "bar") + .queryParam("baz[]", "qux[]") + .buildRequest(null); + + ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); + + ServerRequest result = BeforeFilterFunctions.removeRequestParameter("foo").apply(request); + + assertThat(result.param("foo")).isEmpty(); + assertThat(result.param("baz[]")).isPresent().hasValue("qux[]"); + assertThat(result.uri().toString()).hasToString("http://localhost/path?baz%5B%5D=qux%5B%5D"); + } + + @Test + void removeRequestParameterWithEncodedPath() { + MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/é") + .queryParam("foo", "bar") + .buildRequest(null); + + ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); + + ServerRequest result = BeforeFilterFunctions.removeRequestParameter("foo").apply(request); + + assertThat(result.param("foo")).isEmpty(); + assertThat(result.uri().toString()).hasToString("http://localhost/%C3%A9"); } @Test void stripPrefix() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/depth1/depth2/depth3") - .buildRequest(null); + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.stripPrefix(2).apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/depth3"); + assertThat(result.uri().toString()).hasToString("http://localhost/depth3"); } @Test void stripPrefixWithEncodedPath() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/depth1/depth2/depth3/é") - .buildRequest(null); + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.stripPrefix(2).apply(request); - assertThat(result.uri().toString()).isEqualTo("http://localhost/depth3/%C3%A9"); + assertThat(result.uri().toString()).hasToString("http://localhost/depth3/%C3%A9"); } @Test void stripPrefixWithEncodedParameters() { MockHttpServletRequest servletRequest = MockMvcRequestBuilders.get("http://localhost/depth1/depth2/depth3") - .queryParam("baz[]", "qux[]") - .buildRequest(null); + .queryParam("baz[]", "qux[]") + .buildRequest(null); ServerRequest request = ServerRequest.create(servletRequest, Collections.emptyList()); ServerRequest result = BeforeFilterFunctions.stripPrefix(2).apply(request); assertThat(result.param("baz[]")).isPresent().hasValue("qux[]"); - assertThat(result.uri().toString()).isEqualTo("http://localhost/depth3?baz%5B%5D=qux%5B%5D"); + assertThat(result.uri().toString()).hasToString("http://localhost/depth3?baz%5B%5D=qux%5B%5D"); } }