Exclude well-known ports in expanded redirect-uri
Fixes gh-4836
This commit is contained in:
		
							parent
							
								
									84bb8508ba
								
							
						
					
					
						commit
						c04b3b4114
					
				| 
						 | 
				
			
			@ -164,21 +164,22 @@ public class OAuth2AuthorizationRequestRedirectFilter extends OncePerRequestFilt
 | 
			
		|||
	}
 | 
			
		||||
 | 
			
		||||
	private String expandRedirectUri(HttpServletRequest request, ClientRegistration clientRegistration) {
 | 
			
		||||
		Map<String, String> 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<String, String> uriVariables = new HashMap<>();
 | 
			
		||||
		uriVariables.put("baseUrl", baseUrl);
 | 
			
		||||
		uriVariables.put("registrationId", clientRegistration.getRegistrationId());
 | 
			
		||||
 | 
			
		||||
		return UriComponentsBuilder.fromUriString(clientRegistration.getRedirectUriTemplate())
 | 
			
		||||
			.buildAndExpand(uriVariables)
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -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");
 | 
			
		||||
	}
 | 
			
		||||
}
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
		Loading…
	
		Reference in New Issue