diff --git a/README.md b/README.md index 23dcda7..8e6b94f 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+):** 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 f41d873..5cec25c 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; @@ -15,10 +16,19 @@ 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; +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 jakarta.servlet.http.HttpServletRequest; import com.digitalsanctuary.spring.user.UserConfiguration; import com.digitalsanctuary.spring.user.roles.RolesAndPrivilegesConfig; import com.digitalsanctuary.spring.user.util.AppUrlResolver; @@ -27,7 +37,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 +180,87 @@ 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(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)) { + 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 3fcc790..b152b33 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)); @@ -303,6 +311,24 @@ 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. 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); unprotectedURIs.add(logoutSuccessURI); 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 b5ee404..cf955ff 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/api/UserAPIUnitTest.java b/src/test/java/com/digitalsanctuary/spring/user/api/UserAPIUnitTest.java index 385ee6f..74316d8 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(); } 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 0000000..8a3782d --- /dev/null +++ b/src/test/java/com/digitalsanctuary/spring/user/security/RequestCacheHardeningTest.java @@ -0,0 +1,133 @@ +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 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 { + 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); + } +} 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 85b21e1..818cfb7 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,64 @@ 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 the /favicon.ico browser probe") + void shouldAllowFaviconIco() throws Exception { + mockMvc.perform(get("/favicon.ico")).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()); + } + + @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)); + } + } } 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 80e178a..c7ce9c8 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 {