diff --git a/src/main/java/com/jobtracker/config/AuthorizationServerConfig.java b/src/main/java/com/jobtracker/config/AuthorizationServerConfig.java index 5bbbffc7..27968fac 100644 --- a/src/main/java/com/jobtracker/config/AuthorizationServerConfig.java +++ b/src/main/java/com/jobtracker/config/AuthorizationServerConfig.java @@ -32,6 +32,9 @@ import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationConsentService; import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationService; import org.springframework.security.oauth2.server.authorization.OAuth2TokenType; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationContext; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationProvider; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationValidator; import org.springframework.security.oauth2.server.authorization.client.JdbcRegisteredClientRepository; import org.springframework.security.oauth2.server.authorization.client.RegisteredClient; import org.springframework.security.oauth2.server.authorization.client.RegisteredClientRepository; @@ -71,6 +74,7 @@ import java.util.LinkedHashSet; import java.util.Objects; import java.util.UUID; +import java.util.function.Consumer; @Configuration @EnableConfigurationProperties({GptOAuthProperties.class, McpOAuthProperties.class}) @@ -124,6 +128,15 @@ public SecurityFilterChain authorizationServerSecurityFilterChain( .requestMatchers("/login", "/default-ui.css").permitAll() .anyRequest().authenticated()) .with(authorizationServerConfigurer, authorizationServer -> authorizationServer + .authorizationEndpoint(endpoint -> endpoint.authenticationProviders(providers -> + providers.forEach(provider -> { + if (provider instanceof OAuth2AuthorizationCodeRequestAuthenticationProvider codeProvider) { + Consumer validator = + DcrRedirectUriPolicy::validateAuthorizationRedirectUri; + codeProvider.setAuthenticationValidator(validator.andThen( + OAuth2AuthorizationCodeRequestAuthenticationValidator.DEFAULT_SCOPE_VALIDATOR)); + } + }))) // Public clients (Claude/ChatGPT MCP connectors) must be able to // refresh tokens with just their client_id: SAS's built-in public // client support only covers the PKCE authorization_code request. diff --git a/src/main/java/com/jobtracker/config/DcrRedirectUriPolicy.java b/src/main/java/com/jobtracker/config/DcrRedirectUriPolicy.java new file mode 100644 index 00000000..92fd2731 --- /dev/null +++ b/src/main/java/com/jobtracker/config/DcrRedirectUriPolicy.java @@ -0,0 +1,81 @@ +package com.jobtracker.config; + +import org.springframework.security.oauth2.core.ClientAuthenticationMethod; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationContext; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationToken; +import org.springframework.security.oauth2.server.authorization.authentication.OAuth2AuthorizationCodeRequestAuthenticationValidator; +import org.springframework.security.oauth2.server.authorization.client.RegisteredClient; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Objects; + +/** + * RFC 7591 registration policy and RFC 8252 loopback-port exception for DCR public clients. + * All other authorization requests retain Spring Authorization Server's default URI validation. + */ +final class DcrRedirectUriPolicy { + + private DcrRedirectUriPolicy() { + } + + static boolean isValidForRegistration(String value) { + if (value == null || value.length() > 500) { + return false; + } + try { + URI uri = new URI(value); + if (!uri.isAbsolute() || uri.isOpaque() || uri.getHost() == null + || uri.getUserInfo() != null || uri.getFragment() != null + || uri.getPort() > 65535) { + return false; + } + return "https".equalsIgnoreCase(uri.getScheme()) || isHttpLoopback(uri); + } catch (URISyntaxException ex) { + return false; + } + } + + static void validateAuthorizationRedirectUri(OAuth2AuthorizationCodeRequestAuthenticationContext context) { + RegisteredClient client = context.getRegisteredClient(); + OAuth2AuthorizationCodeRequestAuthenticationToken request = context.getAuthentication(); + String requestedRedirectUri = request.getRedirectUri(); + + // DCR registers the full callback URI; RFC 8252 §7.3 permits changing ONLY its port. + // Do not relax validation for static clients, CIMD clients, HTTPS callbacks or non-PKCE clients. + if (requestedRedirectUri != null + && client.getClientId().startsWith("dcr-") + && client.getClientAuthenticationMethods().contains(ClientAuthenticationMethod.NONE) + && client.getClientSettings().isRequireProofKey() + && client.getRedirectUris().stream().anyMatch( + registered -> matchesHttpLoopbackIgnoringPort(registered, requestedRedirectUri))) { + return; + } + + OAuth2AuthorizationCodeRequestAuthenticationValidator.DEFAULT_REDIRECT_URI_VALIDATOR.accept(context); + } + + private static boolean matchesHttpLoopbackIgnoringPort(String registered, String requested) { + if (!isValidForRegistration(registered) || !isValidForRegistration(requested)) { + return false; + } + + URI registeredUri = URI.create(registered); + URI requestedUri = URI.create(requested); + return isHttpLoopback(registeredUri) + && isHttpLoopback(requestedUri) + && registeredUri.getHost().equalsIgnoreCase(requestedUri.getHost()) + && Objects.equals(registeredUri.getRawPath(), requestedUri.getRawPath()) + && Objects.equals(registeredUri.getRawQuery(), requestedUri.getRawQuery()); + } + + private static boolean isHttpLoopback(URI uri) { + if (!"http".equalsIgnoreCase(uri.getScheme()) || uri.getHost() == null) { + return false; + } + String host = uri.getHost(); + return "localhost".equalsIgnoreCase(host) + || "127.0.0.1".equals(host) + || "[::1]".equalsIgnoreCase(host); + } +} diff --git a/src/main/java/com/jobtracker/config/DynamicClientRegistrationController.java b/src/main/java/com/jobtracker/config/DynamicClientRegistrationController.java index de4830aa..92de45f8 100644 --- a/src/main/java/com/jobtracker/config/DynamicClientRegistrationController.java +++ b/src/main/java/com/jobtracker/config/DynamicClientRegistrationController.java @@ -15,8 +15,6 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; -import java.net.URI; -import java.net.URISyntaxException; import java.time.Instant; import java.util.ArrayList; import java.util.LinkedHashMap; @@ -81,9 +79,9 @@ public ResponseEntity> register( "redirect_uris is required and must be a non-empty array")); } - if (rawList.stream().anyMatch(value -> !(value instanceof String uri) || !isValidRedirectUri(uri))) { + if (rawList.stream().anyMatch(value -> !(value instanceof String uri) || !DcrRedirectUriPolicy.isValidForRegistration(uri))) { return ResponseEntity.badRequest() - .body(error("invalid_redirect_uri", "All redirect URIs must be valid HTTPS URLs or HTTP localhost URLs")); + .body(error("invalid_redirect_uri", "Redirect URIs must use HTTPS or HTTP loopback (localhost, 127.0.0.1, [::1])")); } List redirectUris = rawList.stream().map(String.class::cast).distinct().toList(); @@ -147,19 +145,6 @@ public ResponseEntity> register( return ResponseEntity.status(HttpStatus.CREATED).body(response); } - private boolean isValidRedirectUri(String value) { - if (value.length() > 500) return false; - try { - URI uri = new URI(value); - if (!uri.isAbsolute() || uri.isOpaque() || uri.getHost() == null - || uri.getUserInfo() != null || uri.getFragment() != null) return false; - return "https".equalsIgnoreCase(uri.getScheme()) - || ("http".equalsIgnoreCase(uri.getScheme()) && "localhost".equalsIgnoreCase(uri.getHost())); - } catch (URISyntaxException ex) { - return false; - } - } - // authorization_code is always granted; refresh_token is granted when requested. // Anything else (client_credentials, implicit, ...) is silently dropped per RFC 7591 §2. private List resolveGrantTypes(Object grantTypesClaim) { diff --git a/src/test/java/com/jobtracker/integration/DynamicClientRegistrationIT.java b/src/test/java/com/jobtracker/integration/DynamicClientRegistrationIT.java index 1ec52af9..328c3074 100644 --- a/src/test/java/com/jobtracker/integration/DynamicClientRegistrationIT.java +++ b/src/test/java/com/jobtracker/integration/DynamicClientRegistrationIT.java @@ -5,11 +5,23 @@ import com.jobtracker.config.DynamicClientRegistrationController; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.MediaType; +import org.springframework.http.HttpHeaders; +import org.springframework.security.oauth2.core.ClientAuthenticationMethod; +import org.springframework.security.oauth2.server.authorization.client.RegisteredClient; +import org.springframework.security.oauth2.server.authorization.client.RegisteredClientRepository; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; +import java.util.List; +import java.util.Map; + +import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user; + import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -33,6 +45,7 @@ class DynamicClientRegistrationIT extends AbstractIntegrationTest { @Autowired private MockMvc mockMvc; @Autowired private ObjectMapper objectMapper; @Autowired private DynamicClientRegistrationController dcrController; + @Autowired private RegisteredClientRepository registeredClientRepository; @BeforeEach void resetRateLimit() { @@ -245,4 +258,111 @@ void dcrEndpoint_shouldOnlyGrantRequestedScopesWithinAllowed() throws Exception assertThat(scope).contains("openid"); assertThat(scope).contains("read:profile"); } + + @ParameterizedTest + @ValueSource(strings = { + "https://client.example.com/callback", + "http://localhost:8080/callback", + "http://127.0.0.1:49152/callback", + "http://[::1]:49153/callback" + }) + void dcrEndpoint_shouldAcceptHttpsAndHttpLoopbackRedirects(String redirectUri) throws Exception { + MvcResult result = mockMvc.perform(post("/connect/register") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(Map.of("redirect_uris", List.of(redirectUri))))) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.redirect_uris[0]").value(redirectUri)) + .andReturn(); + + String clientId = objectMapper.readTree(result.getResponse().getContentAsString()) + .get("client_id").asText(); + RegisteredClient client = registeredClientRepository.findByClientId(clientId); + assertThat(client).isNotNull(); + assertThat(client.getRedirectUris()).containsExactly(redirectUri); + assertThat(client.getClientSettings().isRequireProofKey()).isTrue(); + assertThat(client.getClientAuthenticationMethods()).containsExactly(ClientAuthenticationMethod.NONE); + } + + @ParameterizedTest + @ValueSource(strings = { + "http://external.example.com/callback", + "http://localhost.evil.example.com/callback", + "http://127.0.0.2:8080/callback", + "http://127.0.0.1.evil.example.com/callback", + "http://[::2]:8080/callback", + "http://user@localhost:8080/callback", + "http://127.0.0.1:8080/callback#fragment", + "http://[::1%25eth0]:8080/callback", + "http://localhost:65536/callback", + "javascript:alert(1)", + "/relative/callback" + }) + void dcrEndpoint_shouldRejectUnsafeRedirectUris(String redirectUri) throws Exception { + mockMvc.perform(post("/connect/register") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(Map.of("redirect_uris", List.of(redirectUri))))) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.error").value("invalid_redirect_uri")); + } + + @ParameterizedTest + @CsvSource({ + "http://localhost:3000/callback, http://localhost:49152/callback", + "http://127.0.0.1:3000/callback, http://127.0.0.1:49152/callback", + "http://[::1]:3000/callback, http://[::1]:49152/callback" + }) + void dcrAuthorization_shouldAcceptEphemeralLoopbackPort(String registeredUri, String requestedUri) + throws Exception { + String clientId = registerClientWithRedirect(registeredUri); + MvcResult result = authorize(clientId, requestedUri) + .andExpect(status().is3xxRedirection()) + .andReturn(); + + String location = result.getResponse().getHeader(HttpHeaders.LOCATION); + assertThat(location).startsWith(requestedUri + "?").contains("code="); + assertThat(location).contains("state=loopback-state"); + } + + @ParameterizedTest + @ValueSource(strings = { + "http://localhost:49152/other", + "http://localhost.evil.example.com:49152/callback?source=one", + "http://127.0.0.1:49152/callback?source=one", + "http://localhost:49152/callback?source=two", + "https://localhost:49152/callback?source=one" + }) + void dcrAuthorization_shouldRejectChangesBeyondLoopbackPort(String requestedUri) throws Exception { + String clientId = registerClientWithRedirect("http://localhost:3000/callback?source=one"); + authorize(clientId, requestedUri).andExpect(status().isBadRequest()); + } + + @Test + void dcrAuthorization_shouldNotAllowDynamicPortForHttpsRedirect() throws Exception { + String clientId = registerClientWithRedirect("https://client.example.com/callback"); + authorize(clientId, "https://client.example.com:49152/callback") + .andExpect(status().isBadRequest()); + } + + private String registerClientWithRedirect(String redirectUri) throws Exception { + MvcResult result = mockMvc.perform(post("/connect/register") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(Map.of("redirect_uris", List.of(redirectUri))))) + .andExpect(status().isCreated()) + .andReturn(); + return objectMapper.readTree(result.getResponse().getContentAsString()).get("client_id").asText(); + } + + private org.springframework.test.web.servlet.ResultActions authorize(String clientId, String redirectUri) + throws Exception { + return mockMvc.perform(get("/oauth2/authorize") + .with(user("loopback-user@example.com").roles("USER")) + .queryParam("response_type", "code") + .queryParam("client_id", clientId) + .queryParam("redirect_uri", redirectUri) + .queryParam("scope", "read:profile") + .queryParam("state", "loopback-state") + .queryParam("code_challenge", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + .queryParam("code_challenge_method", "S256")); + } + }