From 2dcacb20ddcebb4eba017fdd6d4e08234c76889d Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 10:48:43 -0600 Subject: [PATCH 1/6] fix(security): harden RequestCache so browser auto-probes cannot hijack the post-login redirect Spring Security's default request cache saves ANY request that triggers authentication. Browsers (Safari/iOS especially) automatically probe URLs like /apple-touch-icon.png while the login page renders; when such a path is protected, the unauthenticated probe overwrites the saved deep link the user actually clicked, so the post-login redirect lands on /apple-touch-icon.png?continue (usually a 404/error page) instead of the user's destination. Contribute a consumer-overridable RequestCache bean (ConditionalOnMissingBean) that only saves plausible user navigations: GET requests that explicitly accept text/html and are not XHR (X-Requested-With), HTMX (HX-Request), static-asset fetches (common asset extensions), or well-known auto-probed paths (/apple-touch-icon*, /favicon*, /.well-known/). Wire it into buildSecurityFilterChain via http.requestCache(...) - no public signature changes, so consumers delegating to buildSecurityFilterChain are unaffected. Consumers can restore Spring Security default behavior by defining their own RequestCache bean (new HttpSessionRequestCache()). --- .../UserSecurityBeansAutoConfiguration.java | 90 ++++++++++++- .../user/security/WebSecurityConfig.java | 8 ++ .../security/RequestCacheHardeningTest.java | 123 ++++++++++++++++++ 3 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java index f41d8731..7d08cb3e 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java @@ -1,6 +1,7 @@ package com.digitalsanctuary.spring.user.security; import java.util.List; +import java.util.Set; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.AutoConfiguration; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -18,7 +19,15 @@ import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; +import org.springframework.security.web.savedrequest.HttpSessionRequestCache; +import org.springframework.security.web.savedrequest.RequestCache; import org.springframework.security.web.session.HttpSessionEventPublisher; +import org.springframework.security.web.util.matcher.AndRequestMatcher; +import org.springframework.security.web.util.matcher.MediaTypeRequestMatcher; +import org.springframework.security.web.util.matcher.NegatedRequestMatcher; +import org.springframework.security.web.util.matcher.RequestHeaderRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; +import org.springframework.http.MediaType; import com.digitalsanctuary.spring.user.UserConfiguration; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; import com.digitalsanctuary.spring.user.util.AppUrlResolver; @@ -27,7 +36,8 @@ /** * Auto-configuration that contributes the library's core, consumer-overridable security beans: - * {@link PasswordEncoder}, {@link SessionRegistry}, {@link RoleHierarchy}, and {@link DaoAuthenticationProvider}. + * {@link PasswordEncoder}, {@link SessionRegistry}, {@link RoleHierarchy}, {@link DaoAuthenticationProvider}, and the hardened + * {@link org.springframework.security.web.savedrequest.RequestCache}. * *

* Each bean is guarded by {@link ConditionalOnMissingBean}, so a consuming application can fully replace any of them simply by defining their own bean @@ -169,6 +179,84 @@ public AuthenticationEventPublisher authenticationEventPublisher(ApplicationEven return new DefaultAuthenticationEventPublisher(applicationEventPublisher); } + /** + * File extensions that identify static-asset fetches. Requests for these are never worth replaying after login, and browsers frequently fetch + * them automatically (icons, manifests, fonts) while the login page itself is rendering — which would otherwise overwrite the user's real + * saved destination. + */ + private static final Set STATIC_ASSET_EXTENSIONS = Set.of("png", "jpg", "jpeg", "gif", "ico", "svg", "webp", "avif", "css", "js", "mjs", + "map", "woff", "woff2", "ttf", "otf", "eot", "webmanifest", "json", "xml"); + + /** + * Root-level paths that browsers and crawlers probe automatically without any markup referencing them. Safari/iOS in particular requests + * {@code /apple-touch-icon.png} and {@code /apple-touch-icon-precomposed.png} on every page load. + */ + private static final Set AUTO_PROBED_PATH_PREFIXES = Set.of("/apple-touch-icon", "/favicon", "/.well-known/"); + + /** + * Creates the library's default {@link RequestCache}: an {@link HttpSessionRequestCache} that only saves requests that plausibly represent a + * user navigation worth returning to after login — {@code GET} requests that explicitly accept {@code text/html} and are not XHR + * ({@code X-Requested-With: XMLHttpRequest}), HTMX ({@code HX-Request}) or static-asset/auto-probed requests (favicons, apple-touch icons, + * {@code /.well-known/**}, common static file extensions). + * + *

+ * Why this matters: Spring Security's default request cache saves any request that triggers authentication. Browsers automatically + * probe protected-by-default URLs such as {@code /apple-touch-icon.png} while the login page renders, overwriting the saved deep link the user + * actually clicked — so the post-login redirect lands on {@code /apple-touch-icon.png?continue} (typically a 404/error page) instead of + * the user's destination. The hardened matcher makes the saved request survive those probes. + *

+ * + *

+ * Backs off entirely if the consuming application defines its own {@link RequestCache} bean, which is also the hook for consumers who want + * Spring Security's default behavior back ({@code new HttpSessionRequestCache()}). + *

+ * + * @return the hardened {@link HttpSessionRequestCache} + */ + @Bean + @ConditionalOnMissingBean(RequestCache.class) + public RequestCache requestCache() { + MediaTypeRequestMatcher acceptsHtml = new MediaTypeRequestMatcher(MediaType.TEXT_HTML); + // Treat "Accept: */*" as NOT an HTML navigation: real browser navigations always list text/html explicitly, + // while auto-probes (touch icons, prefetchers) and API clients typically send */* or a concrete non-HTML type. + acceptsHtml.setIgnoredMediaTypes(Set.of(MediaType.ALL)); + + RequestMatcher isGet = request -> "GET".equalsIgnoreCase(request.getMethod()); + RequestMatcher isStaticAssetOrProbe = UserSecurityBeansAutoConfiguration::isStaticAssetOrAutoProbe; + + RequestMatcher savableNavigation = new AndRequestMatcher(isGet, acceptsHtml, + new NegatedRequestMatcher(new RequestHeaderRequestMatcher("X-Requested-With", "XMLHttpRequest")), + new NegatedRequestMatcher(new RequestHeaderRequestMatcher("HX-Request")), new NegatedRequestMatcher(isStaticAssetOrProbe)); + + HttpSessionRequestCache requestCache = new HttpSessionRequestCache(); + requestCache.setRequestMatcher(savableNavigation); + return requestCache; + } + + /** + * Returns true when the request targets a static asset or a path that browsers/crawlers probe automatically (and which should therefore never + * become a post-login redirect target). + * + * @param request the request to classify + * @return true when the request is a static-asset fetch or well-known auto-probe + */ + private static boolean isStaticAssetOrAutoProbe(jakarta.servlet.http.HttpServletRequest request) { + String path = request.getRequestURI().substring(request.getContextPath().length()).toLowerCase(); + for (String prefix : AUTO_PROBED_PATH_PREFIXES) { + if (path.startsWith(prefix)) { + return true; + } + } + int lastDot = path.lastIndexOf('.'); + int lastSlash = path.lastIndexOf('/'); + if (lastDot > lastSlash) { + String extension = path.substring(lastDot + 1); + // .html/.htm are real pages, everything else in the static set is an asset fetch + return STATIC_ASSET_EXTENSIONS.contains(extension); + } + return false; + } + /** * Creates the library's default {@link AppUrlResolver}, which builds the base URL for security-sensitive email links (password reset, email * verification) from the configured canonical URL ({@code user.security.appUrl}) and/or the trusted-host allow-list diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index 3fcc7902..588a7640 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -20,6 +20,7 @@ import org.springframework.security.web.SecurityFilterChain; import org.springframework.security.web.access.DelegatingMissingAuthorityAccessDeniedHandler; import org.springframework.security.web.authentication.LoginUrlAuthenticationEntryPoint; +import org.springframework.security.web.savedrequest.RequestCache; import org.springframework.security.web.webauthn.authentication.WebAuthnAuthenticationFilter; import com.digitalsanctuary.spring.user.service.DSOAuth2UserService; import com.digitalsanctuary.spring.user.service.DSOidcUserService; @@ -116,6 +117,7 @@ public class WebSecurityConfig { private final MfaConfigProperties mfaConfigProperties; private final Environment environment; private final ApplicationEventPublisher applicationEventPublisher; + private final RequestCache requestCache; /** * Builds the library's security filter chain for Spring Security. @@ -143,6 +145,12 @@ public SecurityFilterChain buildSecurityFilterChain(HttpSecurity http, SessionRe // Always configure exception handling with the injected entry point (HTMX-aware by default) http.exceptionHandling(handling -> handling.authenticationEntryPoint(authenticationEntryPoint)); + // Use the hardened RequestCache (see UserSecurityBeansAutoConfiguration.requestCache()) so automatic browser + // probes of protected URLs (e.g. Safari fetching /apple-touch-icon.png while the login page renders) cannot + // overwrite the user's saved deep link and hijack the post-login redirect. Consumer-overridable via a + // RequestCache bean. + http.requestCache(cache -> cache.requestCache(requestCache)); + // Configure remember-me only if explicitly enabled and key is provided if (rememberMeEnabled && rememberMeKey != null && !rememberMeKey.trim().isEmpty()) { http.rememberMe(rememberMe -> rememberMe.key(rememberMeKey).userDetailsService(userDetailsService)); diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java new file mode 100644 index 00000000..d1c89716 --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java @@ -0,0 +1,123 @@ +package com.digitalsanctuary.spring.user.security; + +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; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.mock.web.MockHttpSession; +import org.springframework.security.web.savedrequest.SavedRequest; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.MvcResult; +import com.digitalsanctuary.spring.user.test.annotations.SecurityTest; + +/** + * Tests for the hardened {@link org.springframework.security.web.savedrequest.RequestCache} contributed by + * {@link UserSecurityBeansAutoConfiguration} and wired into the filter chain by {@link WebSecurityConfig}. + * + *

+ * Background: browsers (Safari/iOS in particular) automatically probe URLs like {@code /apple-touch-icon.png} and + * {@code /apple-touch-icon-precomposed.png} at the site root while rendering any page — including the login page. + * With Spring Security's default request cache, such a probe against a protected URL overwrites the saved + * request created by the user's real navigation (e.g. a deep link from an email), so the post-login redirect sends the + * user to {@code /apple-touch-icon.png?continue} instead of their original destination. + *

+ * + *

+ * The hardened cache only saves requests that are plausibly a user navigation: {@code GET} requests that explicitly + * accept {@code text/html}, are not XHR ({@code X-Requested-With}) or HTMX ({@code HX-Request}) calls, and do not + * target well-known auto-probed/static asset paths (favicons, touch icons, manifests, common static file extensions). + *

+ */ +@SecurityTest +@TestPropertySource(properties = {"user.security.defaultAction=deny"}) +@DisplayName("Hardened RequestCache - saved request survives browser icon probes") +class RequestCacheHardeningTest { + + /** Session attribute where HttpSessionRequestCache stores the saved request. */ + private static final String SAVED_REQUEST_ATTR = "SPRING_SECURITY_SAVED_REQUEST"; + + /** Not present in unprotectedURIs, so it requires authentication under deny. */ + private static final String PROTECTED_PAGE = "/protected.html"; + + /** Auto-probed by Safari/iOS; intentionally NOT in the test unprotectedURIs so it is a protected URL. */ + private static final String TOUCH_ICON = "/apple-touch-icon.png"; + + /** Typical browser navigation Accept header. */ + private static final String BROWSER_ACCEPT = "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8"; + + @Autowired + private MockMvc mockMvc; + + @Test + @DisplayName("should save a browser HTML navigation to a protected page") + void shouldSaveBrowserNavigation() throws Exception { + MvcResult result = mockMvc.perform(get(PROTECTED_PAGE).header("Accept", BROWSER_ACCEPT)).andReturn(); + + SavedRequest saved = savedRequest(result); + assertThat(saved).as("real navigation should be saved for post-login redirect").isNotNull(); + assertThat(saved.getRedirectUrl()).contains(PROTECTED_PAGE); + } + + @Test + @DisplayName("should NOT let an icon probe overwrite the saved navigation (the /apple-touch-icon.png?continue bug)") + void iconProbeMustNotOverwriteSavedRequest() throws Exception { + // 1. User clicks a deep link to a protected page -> redirected to login, request saved. + MvcResult navigation = mockMvc.perform(get(PROTECTED_PAGE).header("Accept", BROWSER_ACCEPT)).andReturn(); + MockHttpSession session = (MockHttpSession) navigation.getRequest().getSession(false); + assertThat(savedRequest(navigation)).isNotNull(); + + // 2. While the login page renders, Safari probes /apple-touch-icon.png (Accept: */*) on the same session. + MvcResult probe = mockMvc.perform(get(TOUCH_ICON).session(session).header("Accept", "*/*")).andReturn(); + + // 3. The saved request must still point at the user's real destination. + SavedRequest saved = savedRequest(probe); + assertThat(saved).as("saved request should survive the icon probe").isNotNull(); + assertThat(saved.getRedirectUrl()).as("icon probe must not hijack the post-login redirect").contains(PROTECTED_PAGE) + .doesNotContain(TOUCH_ICON); + } + + @Test + @DisplayName("should not save an icon probe even when it claims to accept text/html") + void iconPathIsNeverSavedRegardlessOfAcceptHeader() throws Exception { + MvcResult probe = mockMvc.perform(get(TOUCH_ICON).header("Accept", BROWSER_ACCEPT)).andReturn(); + assertThat(savedRequest(probe)).as("auto-probed icon paths must never be saved").isNull(); + } + + @Test + @DisplayName("should not save a request that does not accept text/html (e.g. Accept: image/png)") + void nonHtmlRequestIsNotSaved() throws Exception { + MvcResult probe = mockMvc.perform(get(PROTECTED_PAGE).header("Accept", "image/png")).andReturn(); + assertThat(savedRequest(probe)).as("non-HTML fetches are not user navigations").isNull(); + } + + @Test + @DisplayName("should not save an HTMX partial request") + void htmxRequestIsNotSaved() throws Exception { + MvcResult probe = mockMvc.perform(get(PROTECTED_PAGE).header("Accept", BROWSER_ACCEPT).header("HX-Request", "true")).andReturn(); + assertThat(savedRequest(probe)).as("HTMX partials should not become post-login redirect targets").isNull(); + } + + @Test + @DisplayName("should not save an XHR request") + void xhrRequestIsNotSaved() throws Exception { + MvcResult probe = + mockMvc.perform(get(PROTECTED_PAGE).header("Accept", BROWSER_ACCEPT).header("X-Requested-With", "XMLHttpRequest")).andReturn(); + assertThat(savedRequest(probe)).as("XHR calls should not become post-login redirect targets").isNull(); + } + + @Test + @DisplayName("should not save a POST request") + void postRequestIsNotSaved() throws Exception { + MvcResult probe = mockMvc.perform(post(PROTECTED_PAGE).header("Accept", BROWSER_ACCEPT).with( + org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf())).andReturn(); + assertThat(savedRequest(probe)).as("only GET navigations can be meaningfully replayed after login").isNull(); + } + + private SavedRequest savedRequest(MvcResult result) { + MockHttpSession session = (MockHttpSession) result.getRequest().getSession(false); + return session == null ? null : (SavedRequest) session.getAttribute(SAVED_REQUEST_ATTR); + } +} From 810cb6a7cf36d0c483dfaca3626fbff0400f71cf Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 18:16:35 -0600 Subject: [PATCH 2/6] feat(security): auto-unprotect apple-touch-icon, favicon and /.well-known probe paths Complements the hardened RequestCache: browsers and crawlers automatically probe always-public paths (Safari fetches /apple-touch-icon.png and its precomposed/sized variants on every page load; favicons; /.well-known/** for security.txt, ACME, passkey association files). Under defaultAction=deny these otherwise 302 to the login page unless each consumer remembers to whitelist every variant. getUnprotectedURIsList() now auto-adds /apple-touch-icon*, /favicon* and /.well-known/** for every consumer, mirroring the exclusion set in the hardened RequestCache so probes neither hijack the post-login redirect nor bounce through login. Follows the existing precedent in the same method that auto-unprotects the MFA entry-point URIs. Tested via WebSecurityAuthorizationDenyTest.AutoUnprotectedProbePaths: these paths are absent from application-test.yml's unprotectedURIs, so a 404 (reached the dispatcher) rather than a 302 (login redirect) proves the auto-unprotect applied. --- .../user/security/WebSecurityConfig.java | 10 +++++ .../WebSecurityAuthorizationDenyTest.java | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index 588a7640..cafad515 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -311,6 +311,16 @@ private List getUnprotectedURIsList() { // Add the required user pages and actions to the unprotected URIs from configuration List unprotectedURIs = new ArrayList(); unprotectedURIs.addAll(Arrays.asList(getUnprotectedURIsArray())); + // Auto-unprotect the always-public paths that browsers and crawlers probe automatically without any markup + // referencing them: apple-touch icons (Safari/iOS fetches /apple-touch-icon.png and -precomposed variants on + // every page load, including sized variants), favicons, and /.well-known/** (RFC 8615: security.txt, ACME + // challenges, passkey/associated-domain files, etc.). These are never sensitive, and leaving them protected + // means every probe 302s to the login page. This mirrors the exclusion set in the hardened RequestCache + // (see UserSecurityBeansAutoConfiguration.requestCache()) so a probe neither hijacks the post-login redirect + // nor bounces through login. Consumers no longer need to remember to list these manually. + unprotectedURIs.add("/apple-touch-icon*"); + unprotectedURIs.add("/favicon*"); + unprotectedURIs.add("/.well-known/**"); unprotectedURIs.add(loginPageURI); unprotectedURIs.add(loginActionURI); unprotectedURIs.add(logoutSuccessURI); diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java index 85b21e1a..ede43cdd 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java @@ -90,4 +90,46 @@ void shouldAllowAuthenticatedUserOnProtectedUriWhenDeny() throws Exception { mockMvc.perform(get(UNLISTED_URI).with(user("user@test.com").roles("USER"))) .andExpect(status().isNotFound()); } + + /** + * The framework auto-unprotects always-public browser/crawler probe paths (apple-touch icons, favicons, + * {@code /.well-known/**}) in {@code getUnprotectedURIsList()} so consumers do not have to list them and so the + * probes do not 302 to login. These paths are intentionally absent from {@code application-test.yml}'s + * {@code unprotectedURIs}, so a 404 (no handler, request reached the dispatcher) rather than a 302 proves the + * auto-unprotect applied. + */ + @org.junit.jupiter.api.Nested + @DisplayName("auto-unprotected browser/crawler probe paths") + class AutoUnprotectedProbePaths { + + @Test + @DisplayName("should auto-unprotect /apple-touch-icon.png (Safari's default probe) without it being listed") + void shouldAllowPlainAppleTouchIcon() throws Exception { + mockMvc.perform(get("/apple-touch-icon.png")).andExpect(status().isNotFound()); + } + + @Test + @DisplayName("should auto-unprotect /apple-touch-icon-precomposed.png") + void shouldAllowPrecomposedAppleTouchIcon() throws Exception { + mockMvc.perform(get("/apple-touch-icon-precomposed.png")).andExpect(status().isNotFound()); + } + + @Test + @DisplayName("should auto-unprotect a sized apple-touch-icon variant (/apple-touch-icon-120x120.png)") + void shouldAllowSizedAppleTouchIcon() throws Exception { + mockMvc.perform(get("/apple-touch-icon-120x120.png")).andExpect(status().isNotFound()); + } + + @Test + @DisplayName("should auto-unprotect a favicon variant beyond the listed /favicon.ico (/favicon-32x32.png)") + void shouldAllowFaviconVariant() throws Exception { + mockMvc.perform(get("/favicon-32x32.png")).andExpect(status().isNotFound()); + } + + @Test + @DisplayName("should auto-unprotect /.well-known/** (e.g. security.txt)") + void shouldAllowWellKnown() throws Exception { + mockMvc.perform(get("/.well-known/security.txt")).andExpect(status().isNotFound()); + } + } } From 50693e8c7cba65f52a2d962c76cc24c4f568fd09 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 18:29:35 -0600 Subject: [PATCH 3/6] fix(security): share the RequestCache with LoginSuccessService so consumer overrides work end-to-end Addresses code-review finding M1. LoginSuccessService extends SavedRequestAwareAuthenticationSuccessHandler, which holds its own private default HttpSessionRequestCache and was never handed the injected RequestCache bean. The hardened cache worked only because both sides happened to be an HttpSessionRequestCache on the same session attribute: the filter chain (save side) used the bean while the success handler (read side) used its own default. A consumer who overrode the RequestCache bean with a different implementation or session attribute -- exactly what requestCache()'s @ConditionalOnMissingBean Javadoc advertises -- would have their request saved to one store and read from another, silently breaking the post-login redirect to the originally-requested page. Inject the effective RequestCache into LoginSuccessService and pass it to super.setRequestCache(...) so save and read share one instance and the override contract holds end-to-end. Also from review: add a no-Accept-header RequestCache test (the one matcher branch not previously exercised), document the fail-open URL-encoding assumption in isStaticAssetOrAutoProbe, and tidy import grouping. Full suite: 1005 tests, 0 failures. --- .../UserSecurityBeansAutoConfiguration.java | 8 ++++-- .../user/service/LoginSuccessService.java | 23 +++++++++++++++-- .../security/RequestCacheHardeningTest.java | 10 ++++++++ .../user/service/LoginSuccessServiceTest.java | 25 +++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java index 7d08cb3e..5cec25c1 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/UserSecurityBeansAutoConfiguration.java @@ -16,6 +16,7 @@ import org.springframework.security.authentication.dao.DaoAuthenticationProvider; import org.springframework.security.core.session.SessionRegistry; import org.springframework.security.core.session.SessionRegistryImpl; +import org.springframework.http.MediaType; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; @@ -27,7 +28,7 @@ import org.springframework.security.web.util.matcher.NegatedRequestMatcher; import org.springframework.security.web.util.matcher.RequestHeaderRequestMatcher; import org.springframework.security.web.util.matcher.RequestMatcher; -import org.springframework.http.MediaType; +import jakarta.servlet.http.HttpServletRequest; import com.digitalsanctuary.spring.user.UserConfiguration; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; import com.digitalsanctuary.spring.user.util.AppUrlResolver; @@ -240,7 +241,10 @@ public RequestCache requestCache() { * @param request the request to classify * @return true when the request is a static-asset fetch or well-known auto-probe */ - private static boolean isStaticAssetOrAutoProbe(jakarta.servlet.http.HttpServletRequest request) { + private static boolean isStaticAssetOrAutoProbe(HttpServletRequest request) { + // getRequestURI() excludes the query string but is URL-encoded and unnormalized. That is acceptable here: this + // is a fail-open save-side heuristic (misclassification at worst saves an odd redirect target), never an + // authorization decision, so an encoded dot (%2E) slipping past extension detection has no security impact. String path = request.getRequestURI().substring(request.getContextPath().length()).toLowerCase(); for (String prefix : AUTO_PROBED_PATH_PREFIXES) { if (path.startsWith(prefix)) { diff --git a/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java b/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java index b5ee404f..cf955ff6 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java +++ b/src/main/java/com/digitalsanctuary/spring/user/service/LoginSuccessService.java @@ -5,6 +5,7 @@ import org.springframework.context.ApplicationEventPublisher; import org.springframework.security.core.Authentication; import org.springframework.security.web.authentication.SavedRequestAwareAuthenticationSuccessHandler; +import org.springframework.security.web.savedrequest.RequestCache; import org.springframework.stereotype.Service; import org.thymeleaf.util.StringUtils; import com.digitalsanctuary.spring.user.audit.AuditEvent; @@ -13,7 +14,6 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; /** @@ -28,13 +28,32 @@ * @see SavedRequestAwareAuthenticationSuccessHandler */ @Slf4j -@RequiredArgsConstructor @Service public class LoginSuccessService extends SavedRequestAwareAuthenticationSuccessHandler { /** The event publisher. */ private final ApplicationEventPublisher eventPublisher; + /** + * Constructs the login success handler and wires in the application's effective {@link RequestCache}. + * + *

+ * The injected {@code requestCache} is passed to {@link #setRequestCache(RequestCache)} so this handler reads the saved request from the + * exact same cache the security filter chain writes to. Both sides therefore honor a consumer-supplied {@link RequestCache} bean (see + * {@code UserSecurityBeansAutoConfiguration.requestCache()}); without this, {@link SavedRequestAwareAuthenticationSuccessHandler} would fall back to + * its own default {@link org.springframework.security.web.savedrequest.HttpSessionRequestCache} and a consumer who overrode the cache with a + * different implementation (or a different session attribute) would get their saved request written to one store and read from another, silently + * breaking the post-login redirect to the originally-requested page. + *

+ * + * @param eventPublisher the application event publisher used to emit login audit events + * @param requestCache the effective {@link RequestCache} bean (the library's hardened default, or a consumer override) + */ + public LoginSuccessService(ApplicationEventPublisher eventPublisher, RequestCache requestCache) { + this.eventPublisher = eventPublisher; + super.setRequestCache(requestCache); + } + /** The login success uri. */ @Value("${user.security.loginSuccessURI}") private String loginSuccessUri; diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java index d1c89716..8a3782d4 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java @@ -93,6 +93,16 @@ void nonHtmlRequestIsNotSaved() throws Exception { assertThat(savedRequest(probe)).as("non-HTML fetches are not user navigations").isNull(); } + @Test + @DisplayName("should not save a request with no Accept header (resolves to */*, which is ignored)") + void noAcceptHeaderIsNotSaved() throws Exception { + // A missing Accept header resolves to */* via HeaderContentNegotiationStrategy; */* is in the ignored set, so + // the request is not a confirmed HTML navigation and must not be saved. This exercises the one matcher branch + // not covered by the explicit-Accept cases above. + MvcResult probe = mockMvc.perform(get(PROTECTED_PAGE)).andReturn(); + assertThat(savedRequest(probe)).as("a request without an explicit text/html Accept is not saved").isNull(); + } + @Test @DisplayName("should not save an HTMX partial request") void htmxRequestIsNotSaved() throws Exception { diff --git a/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessServiceTest.java b/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessServiceTest.java index 80e178a2..c7ce9c8e 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessServiceTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/service/LoginSuccessServiceTest.java @@ -27,6 +27,8 @@ import org.mockito.quality.Strictness; import org.springframework.context.ApplicationEventPublisher; import org.springframework.security.core.Authentication; +import org.springframework.security.web.savedrequest.RequestCache; +import org.springframework.security.web.savedrequest.SavedRequest; import org.springframework.test.util.ReflectionTestUtils; import com.digitalsanctuary.spring.user.audit.AuditEvent; @@ -52,6 +54,9 @@ class LoginSuccessServiceTest { @Mock private ApplicationEventPublisher eventPublisher; + @Mock + private RequestCache requestCache; + @Mock private HttpServletRequest request; @@ -160,6 +165,26 @@ void onAuthenticationSuccess_auditEventPublishingFails_continuesLoginFlow() thro verify(eventPublisher).publishEvent(any(AuditEvent.class)); } + @Test + @DisplayName("Should read the saved request from the INJECTED RequestCache, not a private default (M1 regression)") + void onAuthenticationSuccess_readsSavedRequestFromInjectedCache() throws IOException, ServletException { + // Regression guard for the consumer-override contract: LoginSuccessService must read the post-login redirect + // target from the SAME RequestCache bean the security filter chain writes to. If it fell back to its own + // private default HttpSessionRequestCache (the bug), the injected mock below would never be consulted and a + // consumer-supplied RequestCache override would silently break deep-link redirects. + SavedRequest saved = mock(SavedRequest.class); + when(saved.getRedirectUrl()).thenReturn("https://app.example/menus/42"); + when(requestCache.getRequest(request, response)).thenReturn(saved); + when(response.encodeRedirectURL(any())).thenAnswer(invocation -> invocation.getArgument(0)); + when(authentication.getPrincipal()).thenReturn(userDetails); + + loginSuccessService.onAuthenticationSuccess(request, response, authentication); + + // The injected cache was consulted (proves the read side uses it) and drove the redirect to the saved URL. + verify(requestCache, org.mockito.Mockito.atLeastOnce()).getRequest(request, response); + verify(response).sendRedirect("https://app.example/menus/42"); + } + @Test @DisplayName("Should handle saved request in session") void onAuthenticationSuccess_withSavedRequest_logsDebugInfo() throws IOException, ServletException { From f62045a012f260b5e37c60f8e7a16bf70760286f Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 18:37:12 -0600 Subject: [PATCH 4/6] refactor(security): scope auto-unprotect to the exact browser-probed paths (least privilege) Follow-up to code-review finding N4. The auto-unprotect widens the permitAll surface for every consumer, who never opted in and won't audit the framework's internals, so the patterns should match exactly the auto-probed paths rather than a broad prefix: - /apple-touch-icon* -> /apple-touch-icon*.png (every Safari probe ends in .png; keeps a route like /apple-touch-icon-admin protected) - /favicon* -> /favicon.* (the browser auto-probe is /favicon.ico; the dot anchor keeps a route like /favicon-report protected; sized favicons are -referenced static assets, not auto-probes) - /.well-known/** unchanged (RFC 8615 public by design) This also aligns the framework with the tighter /apple-touch-icon*.png pattern the MagicMenu consumer app already uses. The save-side RequestCache heuristic stays intentionally broader: misclassifying there merely fails open (a request is not saved), never grants access. Adds negative tests proving /apple-touch-icon-admin and /favicon-report still require authentication under deny. Full suite: 1007 tests, 0 failures. --- .../user/security/WebSecurityConfig.java | 24 ++++++++++++------- .../WebSecurityAuthorizationDenyTest.java | 24 ++++++++++++++++--- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java index cafad515..b152b332 100644 --- a/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java +++ b/src/main/java/com/digitalsanctuary/spring/user/security/WebSecurityConfig.java @@ -312,14 +312,22 @@ private List getUnprotectedURIsList() { List unprotectedURIs = new ArrayList(); unprotectedURIs.addAll(Arrays.asList(getUnprotectedURIsArray())); // Auto-unprotect the always-public paths that browsers and crawlers probe automatically without any markup - // referencing them: apple-touch icons (Safari/iOS fetches /apple-touch-icon.png and -precomposed variants on - // every page load, including sized variants), favicons, and /.well-known/** (RFC 8615: security.txt, ACME - // challenges, passkey/associated-domain files, etc.). These are never sensitive, and leaving them protected - // means every probe 302s to the login page. This mirrors the exclusion set in the hardened RequestCache - // (see UserSecurityBeansAutoConfiguration.requestCache()) so a probe neither hijacks the post-login redirect - // nor bounces through login. Consumers no longer need to remember to list these manually. - unprotectedURIs.add("/apple-touch-icon*"); - unprotectedURIs.add("/favicon*"); + // referencing them. Because this widens the permitAll surface for EVERY consumer (who never opted in), the + // patterns are deliberately scoped to exactly the auto-probed paths, not a broad prefix: + // - /apple-touch-icon*.png : Safari/iOS probes /apple-touch-icon.png, -precomposed, and sized variants + // (e.g. -120x120, -120x120-precomposed) on every page load; every one ends in .png. The .png anchor keeps a + // route like /apple-touch-icon-admin protected. + // - /favicon.* : the browser auto-probe is /favicon.ico (the .EXT anchor also covers /favicon.svg|.png root + // probes while keeping a route like /favicon-report protected). Sized favicons (/favicon-32x32.png) are + // -referenced assets, not auto-probes, and are served from the app's static path. + // - /.well-known/** : RFC 8615 well-known URIs (security.txt, ACME challenges, passkey/associated-domain + // files) are public by design. + // These are never sensitive, and leaving them protected means every probe 302s to the login page. The intent + // mirrors the exclusion set in the hardened RequestCache (see UserSecurityBeansAutoConfiguration.requestCache()), + // though that save-side heuristic may be broader since misclassifying there merely fails open (a request is not + // saved) rather than granting access. Consumers no longer need to remember to list these manually. + unprotectedURIs.add("/apple-touch-icon*.png"); + unprotectedURIs.add("/favicon.*"); unprotectedURIs.add("/.well-known/**"); unprotectedURIs.add(loginPageURI); unprotectedURIs.add(loginActionURI); diff --git a/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java b/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java index ede43cdd..818cfb7f 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/security/WebSecurityAuthorizationDenyTest.java @@ -121,9 +121,9 @@ void shouldAllowSizedAppleTouchIcon() throws Exception { } @Test - @DisplayName("should auto-unprotect a favicon variant beyond the listed /favicon.ico (/favicon-32x32.png)") - void shouldAllowFaviconVariant() throws Exception { - mockMvc.perform(get("/favicon-32x32.png")).andExpect(status().isNotFound()); + @DisplayName("should auto-unprotect the /favicon.ico browser probe") + void shouldAllowFaviconIco() throws Exception { + mockMvc.perform(get("/favicon.ico")).andExpect(status().isNotFound()); } @Test @@ -131,5 +131,23 @@ void shouldAllowFaviconVariant() throws Exception { void shouldAllowWellKnown() throws Exception { mockMvc.perform(get("/.well-known/security.txt")).andExpect(status().isNotFound()); } + + @Test + @DisplayName("should NOT auto-unprotect a non-.png path sharing the apple-touch-icon prefix (least privilege)") + void shouldStillProtectAppleTouchIconPrefixedRoute() throws Exception { + // /apple-touch-icon*.png is anchored to .png, so a route like /apple-touch-icon-admin is not silently + // made public by the auto-unprotect and still requires authentication under deny (302 -> login). + mockMvc.perform(get("/apple-touch-icon-admin")).andExpect(status().is3xxRedirection()) + .andExpect(redirectedUrl(LOGIN_PAGE)); + } + + @Test + @DisplayName("should NOT auto-unprotect a non-favicon.EXT path sharing the favicon prefix (least privilege)") + void shouldStillProtectFaviconPrefixedRoute() throws Exception { + // /favicon.* is anchored to a dot, so a route like /favicon-report is not silently made public and still + // requires authentication under deny. + mockMvc.perform(get("/favicon-report")).andExpect(status().is3xxRedirection()) + .andExpect(redirectedUrl(LOGIN_PAGE)); + } } } From 7d4f5aed53bb6e823c2e0314402f265dc33ada41 Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 18:41:44 -0600 Subject: [PATCH 5/6] docs(readme): bump install version to 5.1.1 for the next release --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 23dcda75..8e6b94f8 100644 --- a/README.md +++ b/README.md @@ -134,13 +134,13 @@ Spring Boot 4.x brings significant changes including Spring Security 7 and requi com.digitalsanctuary ds-spring-user-framework - 5.1.0 + 5.1.1 ``` **Gradle:** ```groovy -implementation 'com.digitalsanctuary:ds-spring-user-framework:5.1.0' +implementation 'com.digitalsanctuary:ds-spring-user-framework:5.1.1' ``` #### Spring Boot 4.x Key Changes @@ -213,7 +213,7 @@ Follow these steps to get up and running with the Spring User Framework in your **Spring Boot 4.0 / 4.1 (Java 21+):** ```groovy - implementation 'com.digitalsanctuary:ds-spring-user-framework:5.1.0' + implementation 'com.digitalsanctuary:ds-spring-user-framework:5.1.1' ``` **Spring Boot 3.5 (Java 17+):** From b9afb5ebb8febe47edd59ca507d67840a262fc3f Mon Sep 17 00:00:00 2001 From: Devon Hillard Date: Thu, 23 Jul 2026 18:48:30 -0600 Subject: [PATCH 6/6] test: de-flake UserAPIUnitTest setPassword-warning count under parallel execution The suite runs tests in parallel (junit-platform.properties), and UserAPI.warnIfInitialPasswordSetDisabled() is a @PostConstruct hook, so every other test that boots a Spring context (flag false, no StepUpService bean) fires the same 'setPassword is disabled by default' WARN on its own thread into this test's shared-logger ListAppender. The exact-count assertions (isEqualTo(1) / isZero()) therefore failed intermittently in CI when a context boot overlapped the test window (observed: 1007 tests, 1 failed at UserAPIUnitTest.java:816, on a docs-only commit). Count only warnings emitted on the current test thread (the test invokes the method synchronously) so concurrently-booting contexts on other threads cannot pollute the count, and snapshot the appender list to avoid a ConcurrentModificationException during iteration. Deterministic and unrelated to the RequestCache change; fixed here to keep the release build reliable. Full suite: 1007 tests, 0 failures. --- .../spring/user/api/UserAPIUnitTest.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 385ee6f2..74316d83 100644 --- a/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java +++ b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java @@ -799,8 +799,16 @@ void detachAppender() { } private long disabledWarnings() { - return appender.list.stream() + // UserAPI's logger is a JVM-global singleton, and this suite runs tests in parallel (see + // junit-platform.properties). warnIfInitialPasswordSetDisabled() is a @PostConstruct hook, so every other + // test that boots a Spring context (flag false, no StepUpService) fires this same WARN on its own thread + // into this shared appender. Count only warnings emitted on the current test thread — this test invokes + // the method synchronously — so a concurrently-booting context cannot pollute the count. Snapshot the list + // first (List.copyOf) to avoid a ConcurrentModificationException from a concurrent append during iteration. + String testThread = Thread.currentThread().getName(); + return List.copyOf(appender.list).stream() .filter(event -> event.getLevel() == Level.WARN) + .filter(event -> testThread.equals(event.getThreadName())) .filter(event -> event.getFormattedMessage().contains("/user/setPassword is disabled by default")) .count(); }