From 049627486ee37fde310eb83ad1804dd4779664d4 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Mon, 15 Jun 2026 17:01:57 -0600 Subject: [PATCH] fix(security): case-insensitive trustedHosts match; address PR review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold in PR feedback ahead of the 5.0.1 release: - AppUrlResolver: match user.security.trustedHosts case-insensitively (RFC 4343). A mixed-case configured entry or X-Forwarded-Host (e.g. App.Example.Com) previously failed to match app.example.com, causing the CWE-640 path to ignore a legitimately trusted forwarded host and fall back to the container server name. Adds a test. - Role.java: condense the multi-line EAGER rationale comment to one line (CLAUDE.md style). - UserRepositoryEntityGraphTest: fix stale class Javadoc (Role.privileges is EAGER again) and name the collection in the plain-finder test comment. - Docs: soften 'single query' to 'single round trip (bounded, typically one query)' in UserRepository Javadoc, MIGRATION, and the 5.0.1 CHANGELOG entry — the EntityGraph test tolerates a small statement bound across JPA providers. - CHANGELOG: record the trustedHosts case-insensitivity fix under [5.0.1] Fixed. --- CHANGELOG.md | 5 ++++- MIGRATION.md | 4 ++-- .../spring/user/persistence/model/Role.java | 6 +----- .../persistence/repository/UserRepository.java | 3 ++- .../spring/user/util/AppUrlResolver.java | 8 ++++++-- .../repository/UserRepositoryEntityGraphTest.java | 6 +++--- .../spring/user/util/AppUrlResolverTest.java | 15 +++++++++++++++ 7 files changed, 33 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a7a5e1c..790328eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,10 @@ All notable changes to this project are documented here. This project follows [S ## [5.0.1] - Unreleased ### Changed -- Reverted `Role.privileges` to `FetchType.EAGER` (it was changed to `LAZY` in 5.0.0). The 5.0.0 change made `role.getPrivileges()` throw `LazyInitializationException` when accessed outside an open transaction/session — a surprising footgun for consumers — in exchange for a negligible gain, since privileges are small, static reference data with no bulk-load path. `User.roles` remains `LAZY` (that is where the real N+1 win is), and the authentication path still loads the full `User → roles → privileges` graph in one query via `UserRepository.findWithRolesByEmail`. This is a **non-breaking relaxation**: code written against 5.0.0 continues to work unchanged. See `MIGRATION.md` ("Lazy fetching of roles and privileges"). +- Reverted `Role.privileges` to `FetchType.EAGER` (it was changed to `LAZY` in 5.0.0). The 5.0.0 change made `role.getPrivileges()` throw `LazyInitializationException` when accessed outside an open transaction/session — a surprising footgun for consumers — in exchange for a negligible gain, since privileges are small, static reference data with no bulk-load path. `User.roles` remains `LAZY` (that is where the real N+1 win is), and the authentication path still loads the full `User → roles → privileges` graph in a single round trip via `UserRepository.findWithRolesByEmail`. This is a **non-breaking relaxation**: code written against 5.0.0 continues to work unchanged. See `MIGRATION.md` ("Lazy fetching of roles and privileges"). + +### Fixed +- `user.security.trustedHosts` matching is now case-insensitive (RFC 4343). A mixed-case configured entry or forwarded host (e.g. `App.Example.Com`) now matches `app.example.com`; previously a case mismatch caused the resolver to ignore a legitimately trusted `X-Forwarded-Host` and fall back to the container's server name on the CWE-640 path. ## [5.0.0] - 2026-06-15 diff --git a/MIGRATION.md b/MIGRATION.md index f67b8607..9dc46c89 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -168,7 +168,7 @@ CREATE UNIQUE INDEX ux_privilege_name ON privilege (name); ### Lazy fetching of roles and privileges -**What changed:** The `roles` collection on `User` was previously `FetchType.EAGER` and is now `FetchType.LAZY`. The authentication path (`DSUserDetailsService.loadUserByUsername`) loads the full `User` → `roles` → `privileges` graph via a new `@EntityGraph` repository finder, `UserRepository.findWithRolesByEmail(String email)`, which fetches everything in a **single query**. +**What changed:** The `roles` collection on `User` was previously `FetchType.EAGER` and is now `FetchType.LAZY`. The authentication path (`DSUserDetailsService.loadUserByUsername`) loads the full `User` → `roles` → `privileges` graph via a new `@EntityGraph` repository finder, `UserRepository.findWithRolesByEmail(String email)`, which fetches everything in a **single round trip** (a bounded, typically single query — the exact statement count can vary by JPA provider/version). > **Note (5.0.0 → 5.0.1):** `Role.privileges` was also switched to `LAZY` in 5.0.0, but that was reverted to `EAGER` in **5.0.1** because it made `role.getPrivileges()` throw outside a transaction for marginal benefit. If you are on 5.0.1 or later, only `User.roles` is lazy — `role.getPrivileges()` is safe everywhere. The guidance below is written for 5.0.1+; on 5.0.0 specifically, the privilege caveats also apply. @@ -178,7 +178,7 @@ CREATE UNIQUE INDEX ux_privilege_name ON privilege (name); **Remediation patterns for consumers** that traverse a user's roles on a `User` they obtained outside a transaction: -- **Load through the authentication path or the entity-graph finder.** Use `UserRepository.findWithRolesByEmail(email)` (it initializes roles and privileges in one query) instead of the plain `findByEmail(email)` when you need authorities. +- **Load through the authentication path or the entity-graph finder.** Use `UserRepository.findWithRolesByEmail(email)` (it initializes roles and privileges in a single round trip) instead of the plain `findByEmail(email)` when you need authorities. - **Access the collections inside a transaction.** Annotate the method that reads `user.getRoles()` with `@Transactional` so the persistence session is still open when the lazy collection is first touched. - **Use a DTO projection.** Map the roles/privileges you need into a DTO while still inside the session, then pass the DTO around the detached boundary. - **Initialize before detaching.** If you must hand a `User` to detached code, call `Hibernate.initialize(user.getRolesAsSet())` while the session is open (privileges come along eagerly once the roles are loaded). diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java index 58431a6e..b043bc9e 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/model/Role.java @@ -44,11 +44,7 @@ public class Role implements Serializable { private Set users = new HashSet<>(); /** The privileges. */ - // EAGER by design: privileges are small, static reference data and there is no hot path that loads many Roles at - // once, so eager-loading them is cheap. Keeping this EAGER lets consumers call role.getPrivileges() outside a - // transaction without a LazyInitializationException. The user-load N+1 is addressed by User.roles being LAZY (see - // User), and the authentication path still fetches the full graph in one query via - // UserRepository.findWithRolesByEmail. (Role.privileges was briefly LAZY in 5.0.0; reverted in 5.0.1.) + // EAGER: privileges are static reference data with no bulk-load path; consumers need getPrivileges() outside a transaction. See MIGRATION.md. @ToString.Exclude @ManyToMany(cascade = {CascadeType.PERSIST, CascadeType.MERGE}, fetch = FetchType.EAGER) @JoinTable(name = "roles_privileges", joinColumns = @JoinColumn(name = "role_id", referencedColumnName = "id"), diff --git a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java index 53ee36ad..51bc07c9 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java +++ b/src/main/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepository.java @@ -22,7 +22,8 @@ public interface UserRepository extends JpaRepository { User findByEmail(String email); /** - * Find by email, eagerly loading the user's roles and each role's privileges in a single query via an entity graph. + * Find by email, eagerly loading the user's roles and each role's privileges in a single round trip via an entity + * graph (a bounded, typically single query — the exact statement count can vary by JPA provider/version). * *

This is the finder used on the authentication path (see {@code DSUserDetailsService}). Because {@code User.roles} * is {@link jakarta.persistence.FetchType#LAZY}, callers that must traverse a user's roles (and their privileges) diff --git a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java index 063ad59e..d6c8fe2c 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java +++ b/src/main/java/com/digitalsanctuary/spring/user/util/AppUrlResolver.java @@ -1,6 +1,7 @@ package com.digitalsanctuary.spring.user.util; import java.util.List; +import java.util.Locale; import jakarta.servlet.http.HttpServletRequest; import lombok.extern.slf4j.Slf4j; @@ -41,7 +42,10 @@ public AppUrlResolver(String configuredAppUrl, List trustedHosts) { // Strip any trailing slash to honour the "no trailing slash" contract on resolveAppUrl's return value. // Without this, appUrl + "/user/..." produces double slashes when the consumer misconfigures a trailing slash. this.configuredAppUrl = (trimmed != null && trimmed.endsWith("/")) ? trimmed.replaceAll("/+$", "") : trimmed; - this.trustedHosts = trustedHosts == null ? List.of() : trustedHosts.stream().map(String::trim).toList(); + // Hostnames are case-insensitive (RFC 4343); normalise the allow-list to lower case so a mixed-case + // configured or forwarded host (e.g. "App.Example.Com") still matches "app.example.com". + this.trustedHosts = trustedHosts == null ? List.of() + : trustedHosts.stream().map(s -> s.trim().toLowerCase(Locale.ROOT)).toList(); } /** @@ -60,7 +64,7 @@ public String resolveAppUrl(HttpServletRequest request) { // only, otherwise legitimate multi-proxy chains (e.g. ALB + nginx) never match and silently fall // back to the container's server name. String fwdHost = firstHeaderValue(request.getHeader("X-Forwarded-Host")); - boolean useForwarded = fwdHost != null && !fwdHost.isEmpty() && trustedHosts.contains(stripPort(fwdHost)); + boolean useForwarded = fwdHost != null && !fwdHost.isEmpty() && trustedHosts.contains(stripPort(fwdHost).toLowerCase(Locale.ROOT)); if (fwdHost != null && !fwdHost.isEmpty() && !useForwarded) { log.warn("AppUrlResolver: ignoring untrusted X-Forwarded-Host '{}' (not in user.security.trustedHosts)", sanitizeForLog(fwdHost)); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java index 35d473d8..fc705c10 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java @@ -20,8 +20,8 @@ /** * Database-slice tests verifying that {@link UserRepository#findWithRolesByEmail(String)} eagerly loads the * User → roles → privileges graph via {@code @EntityGraph} in a bounded number of queries, while the plain - * {@link UserRepository#findByEmail(String)} leaves the now-LAZY collections uninitialized. This is the regression guard - * for the EAGER → LAZY switch on {@code User.roles} and {@code Role.privileges}. + * {@link UserRepository#findByEmail(String)} leaves the now-LAZY {@code User.roles} collection uninitialized. This is the + * regression guard for the EAGER → LAZY switch on {@code User.roles}. ({@code Role.privileges} remains EAGER.) */ @DatabaseTest class UserRepositoryEntityGraphTest { @@ -113,7 +113,7 @@ void shouldNotInitializeRolesWhenLoadedViaPlainFindByEmail() { User user = userRepository.findByEmail("lazy@test.com"); - // The plain finder must leave roles uninitialized, proving the EAGER -> LAZY switch took effect. + // The plain finder must leave roles uninitialized, proving the User.roles EAGER -> LAZY switch took effect. assertThat(Hibernate.isInitialized(user.getRolesAsSet())).isFalse(); } diff --git a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java index d63bc447..c40d305e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/util/AppUrlResolverTest.java @@ -55,6 +55,21 @@ void honorsForwardedHostOnlyWhenAllowListed() { assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://trusted.example.com"); } + @Test + void matchesAllowListedForwardedHostCaseInsensitively() { + // Hostnames are case-insensitive (RFC 4343): a mixed-case allow-list entry and a mixed-case + // forwarded host must still match, otherwise the CWE-640 fix silently falls back to the server name. + AppUrlResolver resolver = new AppUrlResolver(null, List.of("Trusted.Example.Com")); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setScheme("http"); + req.setServerName("internal"); + req.setServerPort(8080); + req.addHeader("X-Forwarded-Proto", "https"); + req.addHeader("X-Forwarded-Host", "TRUSTED.example.com"); + req.addHeader("X-Forwarded-Port", "443"); + assertThat(resolver.resolveAppUrl(req)).isEqualTo("https://TRUSTED.example.com"); + } + @Test void honorsAllowListedIpv6ForwardedHostWhenForwarded() { AppUrlResolver resolver = new AppUrlResolver(null, List.of("[::1]"));