From 88f9529329676c10d87ab6381ec44a96fd144d87 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Fri, 20 May 2022 16:29:39 -0600 Subject: [PATCH] Correctly encode query parameters Issue gh-11235 --- .../saml2/Saml2LogoutConfigurerTests.java | 39 +++++++++++++++---- .../Saml2LogoutBeanDefinitionParserTests.java | 39 +++++++++++++++---- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java index 2d00a18a76..dc85f6216f 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java @@ -20,6 +20,7 @@ import java.nio.charset.StandardCharsets; import java.time.Instant; import java.util.Collection; import java.util.Collections; +import java.util.Map; import java.util.function.Consumer; import org.junit.jupiter.api.AfterEach; @@ -72,6 +73,9 @@ import org.springframework.security.web.authentication.logout.LogoutSuccessHandl import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; +import org.springframework.test.web.servlet.request.RequestPostProcessor; +import org.springframework.web.util.UriComponentsBuilder; +import org.springframework.web.util.UriUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.containsString; @@ -254,10 +258,9 @@ public class Saml2LogoutConfigurerTests { principal.setRelyingPartyRegistrationId("get"); Saml2Authentication user = new Saml2Authentication(principal, "response", AuthorityUtils.createAuthorityList("ROLE_USER")); - MvcResult result = this.mvc - .perform(get("/logout/saml2/slo").param("SAMLRequest", this.apLogoutRequest) - .param("RelayState", this.apLogoutRequestRelayState).param("SigAlg", this.apLogoutRequestSigAlg) - .param("Signature", this.apLogoutRequestSignature).with(authentication(user))) + MvcResult result = this.mvc.perform(get("/logout/saml2/slo").param("SAMLRequest", this.apLogoutRequest) + .param("RelayState", this.apLogoutRequestRelayState).param("SigAlg", this.apLogoutRequestSigAlg) + .param("Signature", this.apLogoutRequestSignature).with(samlQueryString()).with(authentication(user))) .andExpect(status().isFound()).andReturn(); String location = result.getResponse().getHeader("Location"); assertThat(location).startsWith("https://ap.example.org/logout/saml2/response"); @@ -316,8 +319,8 @@ public class Saml2LogoutConfigurerTests { assertThat(this.logoutRequestRepository.loadLogoutRequest(this.request)).isNotNull(); this.mvc.perform(get("/logout/saml2/slo").session(((MockHttpSession) this.request.getSession())) .param("SAMLResponse", this.apLogoutResponse).param("RelayState", this.apLogoutResponseRelayState) - .param("SigAlg", this.apLogoutResponseSigAlg).param("Signature", this.apLogoutResponseSignature)) - .andExpect(status().isFound()).andExpect(redirectedUrl("/login?logout")); + .param("SigAlg", this.apLogoutResponseSigAlg).param("Signature", this.apLogoutResponseSignature) + .with(samlQueryString())).andExpect(status().isFound()).andExpect(redirectedUrl("/login?logout")); verifyNoInteractions(getBean(LogoutHandler.class)); assertThat(this.logoutRequestRepository.loadLogoutRequest(this.request)).isNull(); } @@ -334,8 +337,9 @@ public class Saml2LogoutConfigurerTests { Saml2Utils.samlInflate(Saml2Utils.samlDecode(this.apLogoutResponse)).getBytes(StandardCharsets.UTF_8)); this.mvc.perform(post("/logout/saml2/slo").session((MockHttpSession) this.request.getSession()) .param("SAMLResponse", deflatedApLogoutResponse).param("RelayState", this.rpLogoutRequestRelayState) - .param("SigAlg", this.apLogoutRequestSigAlg).param("Signature", this.apLogoutResponseSignature)) - .andExpect(status().reason(containsString("invalid_signature"))).andExpect(status().isUnauthorized()); + .param("SigAlg", this.apLogoutRequestSigAlg).param("Signature", this.apLogoutResponseSignature) + .with(samlQueryString())).andExpect(status().reason(containsString("invalid_signature"))) + .andExpect(status().isUnauthorized()); verifyNoInteractions(getBean(LogoutHandler.class)); } @@ -398,6 +402,10 @@ public class Saml2LogoutConfigurerTests { return this.spring.getContext().getBean(clazz); } + private SamlQueryStringRequestPostProcessor samlQueryString() { + return new SamlQueryStringRequestPostProcessor(); + } + @EnableWebSecurity @Import(Saml2LoginConfigBeans.class) static class Saml2LogoutDefaultsConfig { @@ -602,4 +610,19 @@ public class Saml2LogoutConfigurerTests { } + static class SamlQueryStringRequestPostProcessor implements RequestPostProcessor { + + @Override + public MockHttpServletRequest postProcessRequest(MockHttpServletRequest request) { + UriComponentsBuilder builder = UriComponentsBuilder.newInstance(); + for (Map.Entry entries : request.getParameterMap().entrySet()) { + builder.queryParam(entries.getKey(), + UriUtils.encode(entries.getValue()[0], StandardCharsets.ISO_8859_1)); + } + request.setQueryString(builder.build(true).toUriString().substring(1)); + return request; + } + + } + } diff --git a/config/src/test/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParserTests.java index 2473c80d51..4dc54b43f0 100644 --- a/config/src/test/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/http/Saml2LogoutBeanDefinitionParserTests.java @@ -19,6 +19,7 @@ package org.springframework.security.config.http; import java.nio.charset.StandardCharsets; import java.time.Instant; import java.util.Collections; +import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -54,6 +55,9 @@ import org.springframework.security.web.authentication.logout.LogoutSuccessHandl import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; +import org.springframework.test.web.servlet.request.RequestPostProcessor; +import org.springframework.web.util.UriComponentsBuilder; +import org.springframework.web.util.UriUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.containsString; @@ -221,10 +225,9 @@ public class Saml2LogoutBeanDefinitionParserTests { principal.setRelyingPartyRegistrationId("get"); Saml2Authentication user = new Saml2Authentication(principal, "response", AuthorityUtils.createAuthorityList("ROLE_USER")); - MvcResult result = this.mvc - .perform(get("/logout/saml2/slo").param("SAMLRequest", this.apLogoutRequest) - .param("RelayState", this.apLogoutRequestRelayState).param("SigAlg", this.apLogoutRequestSigAlg) - .param("Signature", this.apLogoutRequestSignature).with(authentication(user))) + MvcResult result = this.mvc.perform(get("/logout/saml2/slo").param("SAMLRequest", this.apLogoutRequest) + .param("RelayState", this.apLogoutRequestRelayState).param("SigAlg", this.apLogoutRequestSigAlg) + .param("Signature", this.apLogoutRequestSignature).with(samlQueryString()).with(authentication(user))) .andExpect(status().isFound()).andReturn(); String location = result.getResponse().getHeader("Location"); assertThat(location).startsWith("https://ap.example.org/logout/saml2/response"); @@ -281,8 +284,8 @@ public class Saml2LogoutBeanDefinitionParserTests { assertThat(this.logoutRequestRepository.loadLogoutRequest(this.request)).isNotNull(); this.mvc.perform(get("/logout/saml2/slo").session(((MockHttpSession) this.request.getSession())) .param("SAMLResponse", this.apLogoutResponse).param("RelayState", this.apLogoutResponseRelayState) - .param("SigAlg", this.apLogoutResponseSigAlg).param("Signature", this.apLogoutResponseSignature)) - .andExpect(status().isFound()).andExpect(redirectedUrl("/login?logout")); + .param("SigAlg", this.apLogoutResponseSigAlg).param("Signature", this.apLogoutResponseSignature) + .with(samlQueryString())).andExpect(status().isFound()).andExpect(redirectedUrl("/login?logout")); assertThat(this.logoutRequestRepository.loadLogoutRequest(this.request)).isNull(); } @@ -298,8 +301,9 @@ public class Saml2LogoutBeanDefinitionParserTests { Saml2Utils.samlInflate(Saml2Utils.samlDecode(this.apLogoutResponse)).getBytes(StandardCharsets.UTF_8)); this.mvc.perform(post("/logout/saml2/slo").session((MockHttpSession) this.request.getSession()) .param("SAMLResponse", deflatedApLogoutResponse).param("RelayState", this.rpLogoutRequestRelayState) - .param("SigAlg", this.apLogoutRequestSigAlg).param("Signature", this.apLogoutResponseSignature)) - .andExpect(status().reason(containsString("invalid_signature"))).andExpect(status().isUnauthorized()); + .param("SigAlg", this.apLogoutRequestSigAlg).param("Signature", this.apLogoutResponseSignature) + .with(samlQueryString())).andExpect(status().reason(containsString("invalid_signature"))) + .andExpect(status().isUnauthorized()); } @Test @@ -324,4 +328,23 @@ public class Saml2LogoutBeanDefinitionParserTests { return CONFIG_LOCATION_PREFIX + "-" + configName + ".xml"; } + private SamlQueryStringRequestPostProcessor samlQueryString() { + return new SamlQueryStringRequestPostProcessor(); + } + + static class SamlQueryStringRequestPostProcessor implements RequestPostProcessor { + + @Override + public MockHttpServletRequest postProcessRequest(MockHttpServletRequest request) { + UriComponentsBuilder builder = UriComponentsBuilder.newInstance(); + for (Map.Entry entries : request.getParameterMap().entrySet()) { + builder.queryParam(entries.getKey(), + UriUtils.encode(entries.getValue()[0], StandardCharsets.ISO_8859_1)); + } + request.setQueryString(builder.build(true).toUriString().substring(1)); + return request; + } + + } + }