Skip to content

fix(oauth2): ensure single-valued hd parameter for Spring Boot 3.3.13+ compatibility #41271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
subrata71 merged 1 commit into from
Sep 29, 2025
Merged

fix(oauth2): ensure single-valued hd parameter for Spring Boot 3.3.13+ compatibility #41271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 134 additions & 8 deletions ...h/server/authentication/handlers/ce/CustomServerOAuth2AuthorizationRequestResolverCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@
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}
Expand Down Expand Up @@ -177,7 +181,7 @@ private Mono<OAuth2AuthorizationRequest> authorizationRequest(
builder = OAuth2AuthorizationRequest.authorizationCode();
Map<String, Object> additionalParameters = new HashMap<>();

addAttributesAndAdditionalParameters(clientRegistration, attributes, additionalParameters);
addAttributesAndAdditionalParameters(exchange, clientRegistration, attributes, additionalParameters);

builder.additionalParameters(additionalParameters);
// } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType()))
Expand All @@ -199,6 +203,7 @@ private Mono<OAuth2AuthorizationRequest> authorizationRequest(
}

protected void addAttributesAndAdditionalParameters(
ServerWebExchange exchange,
ClientRegistration clientRegistration,
Map<String, Object> attributes,
Map<String, Object> additionalParameters) {
Expand All @@ -215,16 +220,137 @@ protected void addAttributesAndAdditionalParameters(
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<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);
}
}
}
}
Comment on lines 222 to +245
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Null-safety bug + scope guard + normalization for hd.

  • NPE if getOauthAllowedDomains() returns null.
  • hd should 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 hd to 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.java

Length of output: 1018

Gate hd to Google and normalize domains

  • getOauthAllowedDomains() always returns a non-null list (populated as an empty ArrayList), so the null-safety check isn’t needed.
  • Wrap the hd logic in a check for clientRegistration.getRegistrationId().equalsIgnoreCase("google") to avoid sending hd to non-Google providers.
  • Trim and lowercase each allowed domain before using it in hd to 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.


/**
* 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<String> allowedDomains = commonConfig.getOauthAllowedDomains();
if (allowedDomains == null || allowedDomains.isEmpty()) {
return null;
}

List<String> 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();
}

Comment on lines +291 to +299
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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 ForwardedHeaderFilter and 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' || true

Length 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.

// 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<String> 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;
}

/**
Expand Down
Loading