From 4df6b9258f404458c384312b3785601227b72547 Mon Sep 17 00:00:00 2001 From: subratadeypappu Date: Mon, 29 Sep 2025 19:25:37 +0600 Subject: [PATCH] fix(oauth2): ensure single-valued hd parameter for Spring Boot 3.3.13+ compatibility (#41271) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description **Problem:** Spring Boot 3.3.13 enforces single-valued OAuth2 parameters, causing failures when multiple hd values are present in authorization requests. **Solution:** - Single-valued hd: Always 0 or 1 hd parameter - Domain selection: Use request context to pick the domain - Fallback: Use the first allowed domain when no match is found - Multi-TLD support: Works with .com, .org, .io, etc. - Proxy support: Handles X-Forwarded-Host headers - Case-insensitive: Normalizes domain matching EE Counterpart PR: https://github.com/appsmithorg/appsmith-ee/pull/8211 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Authentication,@tag.Sanity" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: e4e0e93ddb4a2f9a7c2babd9247dcadafa73dc90 > Cypress dashboard. > Tags: `@tag.Authentication,@tag.Sanity` > Spec: >
Mon, 29 Sep 2025 12:34:36 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - New Features - Improved OAuth login for setups with multiple allowed domains. The system now auto-derives the most appropriate domain from incoming requests, supports subdomain and multi-level matches, and gracefully falls back when no match is found. Ensures OAuth parameters remain single-valued for better compatibility and reliability. - Tests - Added comprehensive test coverage for multi-domain handling, subdomain matching, fallback behavior, empty configurations, and parameter single-value validation. --- ...rOAuth2AuthorizationRequestResolverCE.java | 142 +++++++- .../handlers/OAuth2HdParameterTest.java | 307 ++++++++++++++++++ 2 files changed, 441 insertions(+), 8 deletions(-) create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java index c237c05d6d..c3a77714b0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java @@ -38,7 +38,11 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.Base64; import java.util.HashMap; +import java.util.List; +import java.util.Locale; import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; /** * This class is a copy of {@link org.springframework.security.oauth2.client.web.server.DefaultServerOAuth2AuthorizationRequestResolver} @@ -177,7 +181,7 @@ public class CustomServerOAuth2AuthorizationRequestResolverCE implements ServerO builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - addAttributesAndAdditionalParameters(clientRegistration, attributes, additionalParameters); + addAttributesAndAdditionalParameters(exchange, clientRegistration, attributes, additionalParameters); builder.additionalParameters(additionalParameters); // } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) @@ -199,6 +203,7 @@ public class CustomServerOAuth2AuthorizationRequestResolverCE implements ServerO } protected void addAttributesAndAdditionalParameters( + ServerWebExchange exchange, ClientRegistration clientRegistration, Map attributes, Map additionalParameters) { @@ -215,18 +220,139 @@ public class CustomServerOAuth2AuthorizationRequestResolverCE implements ServerO addPkceParameters(attributes, additionalParameters); } if (!commonConfig.getOauthAllowedDomains().isEmpty()) { - if (commonConfig.getOauthAllowedDomains().size() == 1) { - // Incase there's only 1 domain, we can do a further optimization to let the user select a specific one - // from the list - additionalParameters.put( - "hd", commonConfig.getOauthAllowedDomains().get(0)); + List allowedDomains = commonConfig.getOauthAllowedDomains(); + + if (allowedDomains.size() == 1) { + // Single domain case: use it directly + additionalParameters.put("hd", allowedDomains.get(0)); } else { - // Add multiple domains to the list of allowed domains - additionalParameters.put("hd", commonConfig.getOauthAllowedDomains()); + // Multiple domains case: derive candidate domain from request context + String candidateDomain = deriveDomainFromRequest(exchange); + + if (candidateDomain != null) { + // Domain was successfully derived and matched + additionalParameters.put("hd", candidateDomain); + log.debug("Using derived domain '{}' for hd parameter", candidateDomain); + } else { + // No domain could be derived or matched, fallback to first allowed domain + String fallbackDomain = allowedDomains.get(0); + additionalParameters.put("hd", fallbackDomain); + log.debug( + "No matching domain derived, using fallback domain '{}' for hd parameter", fallbackDomain); + } } } } + /** + * Derives a candidate domain from the incoming request using existing tenant/domain logic. + * This method leverages the same mechanisms used elsewhere in the codebase for domain inference. + * + * @param exchange The ServerWebExchange containing the request + * @return The derived domain candidate, or null if no domain could be derived + */ + protected String deriveDomainFromRequest(ServerWebExchange exchange) { + try { + ServerHttpRequest request = exchange.getRequest(); + + // Extract host from request headers with fallback chain + String host = extractHostFromRequest(request); + if (host == null || host.isEmpty()) { + return null; + } + + // Normalize host: strip port, lowercase, remove trailing dot + host = normalizeHost(host); + + // Get and normalize allowed domains + List allowedDomains = commonConfig.getOauthAllowedDomains(); + if (allowedDomains == null || allowedDomains.isEmpty()) { + return null; + } + + List normalizedAllowed = allowedDomains.stream() + .filter(Objects::nonNull) + .map(d -> d.trim().toLowerCase(Locale.ROOT)) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toList()); + + // Find the most specific domain match + return findBestDomainMatch(host, normalizedAllowed); + + } catch (Exception e) { + log.debug("Error deriving domain from request", e); + return null; + } + } + + /** + * Extracts host from request using fallback chain: X-Forwarded-Host -> URI host -> Host header + */ + private String extractHostFromRequest(ServerHttpRequest request) { + // Prefer X-Forwarded-Host header (for proxy environments) + String xfHost = request.getHeaders().getFirst("X-Forwarded-Host"); + if (xfHost != null && !xfHost.isBlank()) { + // If comma-separated, take the first + int comma = xfHost.indexOf(','); + return (comma >= 0 ? xfHost.substring(0, comma) : xfHost).trim(); + } + + // Fallback to request URI host + if (request.getURI() != null && request.getURI().getHost() != null) { + return request.getURI().getHost(); + } + + // Final fallback to Host header + if (request.getHeaders().getHost() != null) { + return request.getHeaders().getHost().getHostString(); + } + + return null; + } + + /** + * Normalizes host by removing port, converting to lowercase, and removing trailing dots + */ + private String normalizeHost(String host) { + if (host == null || host.isEmpty()) { + return host; + } + + // Strip port + int colon = host.indexOf(':'); + if (colon >= 0) { + host = host.substring(0, colon); + } + + // Convert to lowercase + host = host.toLowerCase(Locale.ROOT); + + // Remove trailing dot + if (host.endsWith(".")) { + host = host.substring(0, host.length() - 1); + } + + return host; + } + + /** + * Finds the most specific domain match using suffix matching + */ + private String findBestDomainMatch(String host, List normalizedAllowed) { + String best = null; + + for (String allowed : normalizedAllowed) { + if (host.equals(allowed) || host.endsWith("." + allowed)) { + // Prefer the most specific (longest) match + if (best == null || allowed.length() > best.length()) { + best = allowed; + } + } + } + + return best; + } + /** * This function sets the state query parameter sent to the OAuth2 resource server along with the parameter of the * referer which initiated this OAuth2 login. On successful login, we will redirect back to the client's index page diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java new file mode 100644 index 0000000000..95061e142d --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java @@ -0,0 +1,307 @@ +package com.appsmith.server.authentication.handlers; + +import com.appsmith.server.authentication.handlers.ce.CustomServerOAuth2AuthorizationRequestResolverCE; +import com.appsmith.server.authentication.oauth2clientrepositories.CustomOauth2ClientRepositoryManager; +import com.appsmith.server.configurations.CommonConfig; +import com.appsmith.server.helpers.RedirectHelper; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.security.oauth2.client.registration.ClientRegistration; +import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; +import org.springframework.security.oauth2.core.AuthorizationGrantType; +import org.springframework.security.oauth2.core.ClientAuthenticationMethod; +import org.springframework.security.oauth2.core.oidc.OidcScopes; +import org.springframework.web.server.ServerWebExchange; + +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.when; + +/** + * Test class to verify OAuth2 hd parameter handling for Spring Boot 3.3.13+ compatibility. + * + * This test ensures that: + * - OAuth2 authorization requests contain at most one hd parameter + * - Domain selection works correctly for single and multiple domain configurations + * - The implementation is robust across different TLDs and domain patterns + * - Fallback behavior works when no domain match is found + */ +@ExtendWith(MockitoExtension.class) +class OAuth2HdParameterTest { + + @Mock + private ReactiveClientRegistrationRepository clientRegistrationRepository; + + @Mock + private CommonConfig commonConfig; + + @Mock + private RedirectHelper redirectHelper; + + @Mock + private CustomOauth2ClientRepositoryManager oauth2ClientManager; + + private CustomServerOAuth2AuthorizationRequestResolverCE resolver; + private ClientRegistration clientRegistration; + + @BeforeEach + void setUp() { + resolver = new CustomServerOAuth2AuthorizationRequestResolverCE( + clientRegistrationRepository, commonConfig, redirectHelper, oauth2ClientManager); + + // Create a mock Google OAuth client registration + clientRegistration = ClientRegistration.withRegistrationId("google") + .clientId("test-client-id") + .clientSecret("test-client-secret") + .clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC) + .authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE) + .redirectUri("http://localhost:8080/login/oauth2/code/google") + .scope(OidcScopes.OPENID, OidcScopes.EMAIL, OidcScopes.PROFILE) + .authorizationUri("https://accounts.google.com/o/oauth2/v2/auth") + .tokenUri("https://www.googleapis.com/oauth2/v4/token") + .userInfoUri("https://www.googleapis.com/oauth2/v3/userinfo") + .userNameAttributeName("email") + .clientName("Google") + .build(); + } + + @Test + void testSingleDomainConfiguration() throws Exception { + // Arrange + List singleDomain = Arrays.asList("company.com"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(singleDomain); + + ServerWebExchange exchange = MockServerWebExchange.from( + MockServerHttpRequest.get("https://app.appsmith.com/oauth2/authorization/google")); + + // Act + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); + + // Assert + assertTrue(additionalParameters.containsKey("hd"), "hd parameter should be present for single domain"); + assertEquals("company.com", additionalParameters.get("hd"), "hd should be the single configured domain"); + assertFalse(additionalParameters.get("hd") instanceof List, "hd should not be a List"); + } + + @Test + void testMultipleDomainsWithMatchingSubdomain() throws Exception { + // Arrange + List multipleDomains = Arrays.asList("company.com", "partner.org"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(multipleDomains); + + // Request from company.appsmith.com should derive "company" subdomain + ServerWebExchange exchange = MockServerWebExchange.from( + MockServerHttpRequest.get("https://company.appsmith.com/oauth2/authorization/google")); + + // Act + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); + + // Assert + assertTrue(additionalParameters.containsKey("hd"), "hd parameter should be present when domain matches"); + assertEquals("company.com", additionalParameters.get("hd"), "hd should be the matching derived domain"); + assertFalse(additionalParameters.get("hd") instanceof List, "hd should not be a List"); + } + + @Test + void testMultipleDomainsWithNonMatchingSubdomain() throws Exception { + // Arrange + List multipleDomains = Arrays.asList("company.com", "partner.org"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(multipleDomains); + + // Request from demo.appsmith.com should derive "demo" subdomain which doesn't match + ServerWebExchange exchange = MockServerWebExchange.from( + MockServerHttpRequest.get("https://demo.appsmith.com/oauth2/authorization/google")); + + // Act + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); + + // Assert - should fallback to first domain when no match found + assertTrue( + additionalParameters.containsKey("hd"), + "hd parameter should be present, falling back to first domain when derived domain doesn't match"); + assertEquals( + "company.com", + additionalParameters.get("hd"), + "hd should fallback to the first allowed domain when no match is found"); + assertFalse(additionalParameters.get("hd") instanceof List, "hd should not be a List"); + } + + @Test + void testEmptyDomainConfiguration() throws Exception { + // Arrange + List emptyDomains = Arrays.asList(); + when(commonConfig.getOauthAllowedDomains()).thenReturn(emptyDomains); + + ServerWebExchange exchange = MockServerWebExchange.from( + MockServerHttpRequest.get("https://app.appsmith.com/oauth2/authorization/google")); + + // Act + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); + + // Assert + assertFalse( + additionalParameters.containsKey("hd"), + "hd parameter should not be present when no domains configured"); + } + + @Test + void testSpringBoot3_3_13Compatibility() throws Exception { + // Arrange - setup the scenario that would cause the original error + List multipleDomains = Arrays.asList("example.com", "test.com", "demo.org"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(multipleDomains); + + ServerWebExchange exchange = MockServerWebExchange.from( + MockServerHttpRequest.get("https://demo.appsmith.com/oauth2/authorization/google")); + + // Act + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); + + // Assert - verify all OAuth2 parameters are single-valued + for (Map.Entry entry : additionalParameters.entrySet()) { + Object value = entry.getValue(); + + // The critical assertion: no parameter should be a List, Set, or array + assertFalse( + value instanceof List, + "OAuth2 parameter '" + entry.getKey() + "' cannot be a List - this would cause " + + "'OAuth 2 parameters can only have a single value: " + entry.getKey() + + "' error in Spring Boot 3.3.13+"); + + assertFalse(value instanceof java.util.Set, "OAuth2 parameter '" + entry.getKey() + "' cannot be a Set"); + + assertFalse(value instanceof Object[], "OAuth2 parameter '" + entry.getKey() + "' cannot be an array"); + + // If present, must be a single value (String, etc.) + if ("hd".equals(entry.getKey())) { + assertTrue(value instanceof String, "hd parameter must be a String when present"); + } + } + } + + @Test + void testRobustDomainMatching() throws Exception { + // Test domain matching with various TLDs and multi-level subdomains + List multipleDomains = Arrays.asList("acme.com", "widgets.org", "example.net"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(multipleDomains); + + // Test 1: Exact domain match + ServerWebExchange exchange1 = + MockServerWebExchange.from(MockServerHttpRequest.get("https://acme.com/oauth2/authorization/google")); + + Map attributes1 = new HashMap<>(); + Map additionalParameters1 = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange1, attributes1, additionalParameters1); + + assertTrue(additionalParameters1.containsKey("hd"), "hd parameter should be present for exact domain match"); + assertEquals("acme.com", additionalParameters1.get("hd"), "Should match acme.com exactly"); + + // Test 2: Subdomain match with .org TLD + ServerWebExchange exchange2 = MockServerWebExchange.from( + MockServerHttpRequest.get("https://tools.widgets.org/oauth2/authorization/google")); + + Map attributes2 = new HashMap<>(); + Map additionalParameters2 = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange2, attributes2, additionalParameters2); + + assertTrue(additionalParameters2.containsKey("hd"), "hd parameter should be present for subdomain match"); + assertEquals("widgets.org", additionalParameters2.get("hd"), "Should match widgets.org by suffix"); + + // Test 3: Most specific match (longest domain wins) + List specificDomains = Arrays.asList("example.com", "app.example.com"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(specificDomains); + + ServerWebExchange exchange3 = MockServerWebExchange.from( + MockServerHttpRequest.get("https://admin.app.example.com/oauth2/authorization/google")); + + Map attributes3 = new HashMap<>(); + Map additionalParameters3 = new HashMap<>(); + invokeAddAttributesAndAdditionalParameters(exchange3, attributes3, additionalParameters3); + + assertTrue(additionalParameters3.containsKey("hd"), "hd parameter should be present for most specific match"); + assertEquals( + "app.example.com", additionalParameters3.get("hd"), "Should pick the most specific (longest) domain"); + } + + @Test + void testDomainDerivationLogic() throws Exception { + // Test the domain derivation method directly with realistic scenarios + List allowedDomains = Arrays.asList("acme.com", "widgets.org", "example.net"); + when(commonConfig.getOauthAllowedDomains()).thenReturn(allowedDomains); + + // Test case 1: Exact domain match - acme.com should match acme.com + ServerWebExchange exchange1 = + MockServerWebExchange.from(MockServerHttpRequest.get("https://acme.com/oauth2/authorization/google")); + String derived1 = invokeDeriveFromRequest(exchange1); + assertEquals("acme.com", derived1, "Should match acme.com exactly"); + + // Test case 2: Subdomain match - api.widgets.org should match widgets.org + ServerWebExchange exchange2 = MockServerWebExchange.from( + MockServerHttpRequest.get("https://api.widgets.org/oauth2/authorization/google")); + String derived2 = invokeDeriveFromRequest(exchange2); + assertEquals("widgets.org", derived2, "Should match widgets.org by subdomain"); + + // Test case 3: Multi-level subdomain - admin.api.example.net should match example.net + ServerWebExchange exchange3 = MockServerWebExchange.from( + MockServerHttpRequest.get("https://admin.api.example.net/oauth2/authorization/google")); + String derived3 = invokeDeriveFromRequest(exchange3); + assertEquals("example.net", derived3, "Should match example.net by multi-level subdomain"); + + // Test case 4: No match - unknown.com should return null + ServerWebExchange exchange4 = MockServerWebExchange.from( + MockServerHttpRequest.get("https://unknown.com/oauth2/authorization/google")); + String derived4 = invokeDeriveFromRequest(exchange4); + assertNull(derived4, "Should return null for unknown domain"); + + // Test case 5: Case insensitive match - ACME.COM should match acme.com + ServerWebExchange exchange5 = + MockServerWebExchange.from(MockServerHttpRequest.get("https://ACME.COM/oauth2/authorization/google")); + String derived5 = invokeDeriveFromRequest(exchange5); + assertEquals("acme.com", derived5, "Should match acme.com case-insensitively"); + } + + /** + * Uses reflection to invoke the protected addAttributesAndAdditionalParameters method. + */ + private void invokeAddAttributesAndAdditionalParameters( + ServerWebExchange exchange, Map attributes, Map additionalParameters) + throws Exception { + Method method = CustomServerOAuth2AuthorizationRequestResolverCE.class.getDeclaredMethod( + "addAttributesAndAdditionalParameters", + ServerWebExchange.class, + ClientRegistration.class, + Map.class, + Map.class); + method.setAccessible(true); + method.invoke(resolver, exchange, clientRegistration, attributes, additionalParameters); + } + + /** + * Uses reflection to invoke the protected deriveDomainFromRequest method. + */ + private String invokeDeriveFromRequest(ServerWebExchange exchange) throws Exception { + Method method = CustomServerOAuth2AuthorizationRequestResolverCE.class.getDeclaredMethod( + "deriveDomainFromRequest", ServerWebExchange.class); + method.setAccessible(true); + return (String) method.invoke(resolver, exchange); + } +}