fix(oauth2): ensure single-valued hd parameter for Spring Boot 3.3.13+ compatibility#41271
Merged
fix(oauth2): ensure single-valued hd parameter for Spring Boot 3.3.13+ compatibility#41271
Conversation
…+ compatibility - Implement robust domain selection logic with request context matching - Add fallback to first allowed domain when no match found - Support multi-TLD domains with case-insensitive suffix matching - Handle proxy environments with X-Forwarded-Host header support - Normalize host names (strip ports, lowercase, remove trailing dots) - Maintain backward compatibility for existing configurations Fixes OAuth2 authorization request failures in Spring Boot 3.3.13+ where multiple hd parameters cause "OAuth 2 parameters can only have a single value: hd" error. Resolves: Spring Boot 3.3.13 OAuth2 parameter validation issue
Contributor
WalkthroughImplements multi-domain “hd” parameter derivation in OAuth2 authorization request resolution, adding request-aware domain selection with fallback. Introduces helper methods for host extraction/normalization and best-match selection. Updates attributes/parameters builder to accept ServerWebExchange. Adds comprehensive tests validating single/multi-domain, subdomain matching, fallback, and parameter single-valuedness. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as Browser
participant App as Server (App)
participant OAuth as OAuth2 Provider
User->>App: Start OAuth2 login
App->>App: deriveDomainFromRequest(exchange)\n- extractHostFromRequest\n- normalizeHost\n- findBestDomainMatch
alt domain matched
App->>App: Set additional param hd=<matchedDomain>
else no match
App->>App: Fallback to first allowed domain (if any)
end
App->>OAuth: Authorization Request (includes hd if set)
OAuth-->>User: Provider login UI (scoped by hd if present)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java (1)
282-307: Add coverage for X-Forwarded-Host parsing (first value, port trimming).This exercises proxy scenarios the resolver supports.
Add this test below:
@Test void testXForwardedHostHonoredAndTrimmed() throws Exception { when(commonConfig.getOauthAllowedDomains()).thenReturn(Arrays.asList("example.com")); MockServerHttpRequest request = MockServerHttpRequest .get("https://irrelevant.local/oauth2/authorization/google") .header("X-Forwarded-Host", "app.example.com:443, anotherhost") .build(); ServerWebExchange exchange = MockServerWebExchange.from(request); Map<String, Object> attributes = new HashMap<>(); Map<String, Object> additionalParameters = new HashMap<>(); invokeAddAttributesAndAdditionalParameters(exchange, attributes, additionalParameters); assertEquals("example.com", additionalParameters.get("hd"), "Should derive from X-Forwarded-Host (first, no port)"); }app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java (1)
321-336: IPv6 hostname edge-case (optional).
normalizeHoststrips at the first colon, which can mangle bracketless IPv6 literals. Not critical for domain-based matching, but easy to harden.Example tweak:
if (host.startsWith("[") && host.contains("]")) { // keep literal; ports appear after closing bracket } else { int colon = host.indexOf(':'); if (colon >= 0) host = host.substring(0, colon); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
Comment on lines
222
to
+245
| 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<String> 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); | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Null-safety bug + scope guard + normalization for hd.
- NPE if
getOauthAllowedDomains()returns null. hdshould only be added for Google; other IdPs may reject unknown params.- Trim/lowercase config values; single-domain path currently uses raw string.
Apply this diff:
- if (!commonConfig.getOauthAllowedDomains().isEmpty()) {
- List<String> allowedDomains = commonConfig.getOauthAllowedDomains();
- if (allowedDomains.size() == 1) {
- // Single domain case: use it directly
- additionalParameters.put("hd", allowedDomains.get(0));
- } else {
- // 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);
- }
- }
- }
+ List<String> allowedDomainsRaw = commonConfig.getOauthAllowedDomains();
+ if ("google".equalsIgnoreCase(clientRegistration.getRegistrationId())
+ && !CollectionUtils.isEmpty(allowedDomainsRaw)) {
+ List<String> allowedDomains = allowedDomainsRaw.stream()
+ .filter(Objects::nonNull)
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList());
+ if (allowedDomains.isEmpty()) {
+ return;
+ }
+ if (allowedDomains.size() == 1) {
+ // Single domain case: normalized
+ additionalParameters.put("hd", allowedDomains.get(0).toLowerCase(Locale.ROOT));
+ } else {
+ // Multiple domains case: derive candidate domain from request context
+ String candidateDomain = deriveDomainFromRequest(exchange);
+ if (candidateDomain != null) {
+ additionalParameters.put("hd", candidateDomain);
+ log.debug("Using derived domain '{}' for hd parameter", candidateDomain);
+ } else {
+ // Fallback to first normalized domain
+ String fallbackDomain = allowedDomains.get(0).toLowerCase(Locale.ROOT);
+ additionalParameters.put("hd", fallbackDomain);
+ log.debug("No matching domain derived, using fallback domain '{}' for hd parameter", fallbackDomain);
+ }
+ }
+ }Verification ask:
- Confirm you have non-Google providers configured; if yes, gating avoids sending
hdto them. - Ensure config values may contain whitespace/mixed case; normalization prevents subtle mismatches.
---
🏁 Script executed:
```shell
#!/usr/bin/env bash
set -e
echo "Locate getOauthAllowedDomains usage and definition"
rg -nP 'getOauthAllowedDomains' -n
echo "Inspect CommonConfig class"
rg -nP 'class CommonConfig' -n
echo "Show getOauthAllowedDomains method signature"
rg -nP 'getOauthAllowedDomains' -C1
Length of output: 10696
🏁 Script executed:
#!/usr/bin/env bash
sed -n '95,120p' app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.javaLength of output: 1018
Gate hd to Google and normalize domains
getOauthAllowedDomains()always returns a non-null list (populated as an emptyArrayList), so the null-safety check isn’t needed.- Wrap the
hdlogic in a check forclientRegistration.getRegistrationId().equalsIgnoreCase("google")to avoid sendinghdto non-Google providers. - Trim and lowercase each allowed domain before using it in
hdto prevent mismatches.
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java
around lines 222-245, remove the unnecessary null-safety check for
getOauthAllowedDomains() (it always returns a non-null list), then wrap the
entire hd-setting logic so it only runs when
clientRegistration.getRegistrationId().equalsIgnoreCase("google"); before using
domains, normalize allowedDomains by trimming and lowercasing each entry (and do
the same normalization for any derived candidateDomain) so comparisons and the
hd value use normalized domains; keep the single-domain and fallback logic but
use the normalized domain values when putting "hd" into additionalParameters and
when logging.
Comment on lines
+291
to
+299
| 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(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Trust boundary: unguarded X-Forwarded-Host usage.
Blindly trusting X-Forwarded-Host is safe only behind trusted proxies with ForwardedHeaderFilter (or server.forward-headers-strategy) enabled. Otherwise attackers can steer domain selection.
Run this to confirm configuration:
If not enabled, consider:
- Relying on
request.getURI().getHost()only (when not behind proxy), or - Installing
ForwardedHeaderFilterand trusting proxy hops (recommended).
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'ForwardedHeaderFilter' || true
rg -nP -C2 'server\.forward-headers-strategy\s*:\s*' -g '!**/node_modules/**' -g '!**/dist/**' || true
rg -nP -C2 'X-Forwarded-Host' || trueLength of output: 7807
Guard X-Forwarded-Host behind a trusted proxy filter
The current extractHostFromRequest() (CustomServerOAuth2AuthorizationRequestResolverCE.java:291) unconditionally trusts X-Forwarded-Host, allowing host header manipulation. No ForwardedHeaderFilter bean or server.forward-headers-strategy setting was found in the codebase—register Spring’s ForwardedHeaderFilter (or set server.forward-headers-strategy=native) to sanitize proxy headers, or fallback to request.getURI().getHost() when the header isn’t from a trusted proxy.
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java
around lines 291–299, the method unconditionally trusts X-Forwarded-Host which
allows host header spoofing; to fix this, either register Spring’s
ForwardedHeaderFilter bean (or set server.forward-headers-strategy=native) in
the application config so forwarded headers are sanitized by the framework, or
modify extractHostFromRequest to only use X-Forwarded-Host after verifying the
request originated from a trusted proxy (e.g., check remote address against a
trusted proxies list) and otherwise fall back to request.getURI().getHost();
implement one of these approaches and remove unconditional use of the header.
Comment on lines
+104
to
+107
| // Request from company.appsmith.com should derive "company" subdomain | ||
| ServerWebExchange exchange = MockServerWebExchange.from( | ||
| MockServerHttpRequest.get("https://company.appsmith.com/oauth2/authorization/google")); | ||
|
|
Contributor
There was a problem hiding this comment.
Fix false-positive “matching subdomain” test; current URL never matches company.com.
company.appsmith.com cannot suffix-match company.com, so this test only passes via the fallback to the first domain. Use a real subdomain of company.com to validate derivation.
Apply this diff:
- // Request from company.appsmith.com should derive "company" subdomain
- ServerWebExchange exchange = MockServerWebExchange.from(
- MockServerHttpRequest.get("https://company.appsmith.com/oauth2/authorization/google"));
+ // Request from login.company.com should match allowed domain company.com
+ ServerWebExchange exchange = MockServerWebExchange.from(
+ MockServerHttpRequest.get("https://login.company.com/oauth2/authorization/google"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Request from company.appsmith.com should derive "company" subdomain | |
| ServerWebExchange exchange = MockServerWebExchange.from( | |
| MockServerHttpRequest.get("https://company.appsmith.com/oauth2/authorization/google")); | |
| // Request from login.company.com should match allowed domain company.com | |
| ServerWebExchange exchange = MockServerWebExchange.from( | |
| MockServerHttpRequest.get("https://login.company.com/oauth2/authorization/google")); |
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/OAuth2HdParameterTest.java
around lines 104 to 107, the test URL uses "company.appsmith.com" which will not
suffix-match "company.com" and therefore only passes via the fallback; change
the request host to a real subdomain of company.com (for example
"accounts.company.com") so the code path that derives the "company" subdomain is
exercised: replace "https://company.appsmith.com/oauth2/authorization/google"
with "https://accounts.company.com/oauth2/authorization/google" (or another
valid company.com subdomain) and keep the rest of the test unchanged.
mohanarpit
approved these changes
Sep 29, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem:
Spring Boot 3.3.13 enforces single-valued OAuth2 parameters, causing failures when multiple hd values are present in authorization requests.
Solution:
EE Counterpart PR: https://github.com/appsmithorg/appsmith-ee/pull/8211
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18095565045
Commit: e4e0e93
Cypress dashboard.
Tags:
@tag.Authentication,@tag.SanitySpec:
Mon, 29 Sep 2025 12:34:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit