From c04b3b41144ada79c382eab487bbe108206e8a57 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Sat, 18 Nov 2017 10:41:02 -0500 Subject: [PATCH] Exclude well-known ports in expanded redirect-uri Fixes gh-4836 --- ...th2AuthorizationRequestRedirectFilter.java | 15 +++--- ...thorizationRequestRedirectFilterTests.java | 48 +++++++++++++++++-- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilter.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilter.java index f18df2cc58..7535871489 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilter.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilter.java @@ -164,21 +164,22 @@ public class OAuth2AuthorizationRequestRedirectFilter extends OncePerRequestFilt } private String expandRedirectUri(HttpServletRequest request, ClientRegistration clientRegistration) { - Map uriVariables = new HashMap<>(); - uriVariables.put("scheme", request.getScheme()); - uriVariables.put("serverName", request.getServerName()); - uriVariables.put("serverPort", String.valueOf(request.getServerPort())); - uriVariables.put("contextPath", request.getContextPath()); - uriVariables.put("registrationId", clientRegistration.getRegistrationId()); + int port = request.getServerPort(); + if (("http".equals(request.getScheme()) && port == 80) || ("https".equals(request.getScheme()) && port == 443)) { + port = -1; // Removes the port in UriComponentsBuilder + } String baseUrl = UriComponentsBuilder.newInstance() .scheme(request.getScheme()) .host(request.getServerName()) - .port(request.getServerPort()) + .port(port) .path(request.getContextPath()) .build() .toUriString(); + + Map uriVariables = new HashMap<>(); uriVariables.put("baseUrl", baseUrl); + uriVariables.put("registrationId", clientRegistration.getRegistrationId()); return UriComponentsBuilder.fromUriString(clientRegistration.getRedirectUriTemplate()) .buildAndExpand(uriVariables) diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java index cdec748576..a794eae70d 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java @@ -150,7 +150,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { verifyZeroInteractions(filterChain); - assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=http://localhost:80/login/oauth2/code/registration-1"); + assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=http://localhost/login/oauth2/code/registration-1"); } @Test @@ -182,7 +182,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { assertThat(authorizationRequest.getClientId()).isEqualTo( this.registration2.getClientId()); assertThat(authorizationRequest.getRedirectUri()).isEqualTo( - "http://localhost:80/login/oauth2/code/registration-2"); + "http://localhost/login/oauth2/code/registration-2"); assertThat(authorizationRequest.getScopes()).isEqualTo( this.registration2.getScopes()); assertThat(authorizationRequest.getState()).isNotNull(); @@ -203,7 +203,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { verifyZeroInteractions(filterChain); - assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=token&client_id=client-3&scope=openid%20profile%20email&state=.{15,}&redirect_uri=http://localhost:80/login/oauth2/implicit/registration-3"); + assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=token&client_id=client-3&scope=openid%20profile%20email&state=.{15,}&redirect_uri=http://localhost/login/oauth2/implicit/registration-3"); } @Test @@ -243,7 +243,7 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { verifyZeroInteractions(filterChain); - assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=http://localhost:80/login/oauth2/code/registration-1"); + assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=http://localhost/login/oauth2/code/registration-1"); } @Test @@ -268,6 +268,44 @@ public class OAuth2AuthorizationRequestRedirectFilterTests { assertThat(authorizationRequest.getRedirectUri()).isNotEqualTo( this.registration2.getRedirectUriTemplate()); assertThat(authorizationRequest.getRedirectUri()).isEqualTo( - "http://localhost:80/login/oauth2/code/registration-2"); + "http://localhost/login/oauth2/code/registration-2"); + } + + @Test + public void doFilterWhenAuthorizationRequestIncludesPort80ThenExpandedRedirectUriExcludesPort() throws Exception { + String requestUri = OAuth2AuthorizationRequestRedirectFilter.DEFAULT_AUTHORIZATION_REQUEST_BASE_URI + + "/" + this.registration1.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + request.setScheme("http"); + request.setServerName("example.com"); + request.setServerPort(80); + request.setServletPath(requestUri); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain filterChain = mock(FilterChain.class); + + this.filter.doFilter(request, response, filterChain); + + verifyZeroInteractions(filterChain); + + assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=http://example.com/login/oauth2/code/registration-1"); + } + + @Test + public void doFilterWhenAuthorizationRequestIncludesPort443ThenExpandedRedirectUriExcludesPort() throws Exception { + String requestUri = OAuth2AuthorizationRequestRedirectFilter.DEFAULT_AUTHORIZATION_REQUEST_BASE_URI + + "/" + this.registration1.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + request.setScheme("https"); + request.setServerName("example.com"); + request.setServerPort(443); + request.setServletPath(requestUri); + MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain filterChain = mock(FilterChain.class); + + this.filter.doFilter(request, response, filterChain); + + verifyZeroInteractions(filterChain); + + assertThat(response.getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?response_type=code&client_id=client-1&scope=user&state=.{15,}&redirect_uri=https://example.com/login/oauth2/code/registration-1"); } }