fixup: address Nora's review on #520 (security blockers)
Some checks failed
CI / Compose Bucket Idempotency (pull_request) Successful in 58s
CI / Unit & Component Tests (push) Failing after 2m51s
CI / OCR Service Tests (push) Successful in 16s
CI / Unit & Component Tests (pull_request) Failing after 2m47s
CI / OCR Service Tests (pull_request) Successful in 16s
CI / Backend Unit Tests (pull_request) Successful in 4m9s
CI / fail2ban Regex (pull_request) Successful in 38s
CI / Backend Unit Tests (push) Successful in 4m13s
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Successful in 57s
Some checks failed
CI / Compose Bucket Idempotency (pull_request) Successful in 58s
CI / Unit & Component Tests (push) Failing after 2m51s
CI / OCR Service Tests (push) Successful in 16s
CI / Unit & Component Tests (pull_request) Failing after 2m47s
CI / OCR Service Tests (pull_request) Successful in 16s
CI / Backend Unit Tests (pull_request) Successful in 4m9s
CI / fail2ban Regex (pull_request) Successful in 38s
CI / Backend Unit Tests (push) Successful in 4m13s
CI / fail2ban Regex (push) Successful in 38s
CI / Compose Bucket Idempotency (push) Successful in 57s
- frontend/login: derive cookie `secure` flag from request URL protocol.
Pre-PR the cookie was only read by SSR so the flag didn't matter; now
the cookie IS the API credential and must be Secure on HTTPS or it
leaks a 24h Basic token on plaintext networks. Dev runs over HTTP and
would silently lose the cookie if we hardcoded `secure: true`, so the
flag follows `event.url.protocol === 'https:'`.
- SecurityConfig: rewrite the CSRF-disabled comment. The old
"browsers block cross-origin custom headers" justification no longer
holds once /api/* is authenticated via the cookie. Make the
load-bearing dependencies explicit: SameSite=strict on the auth_token
cookie + Spring's default CORS rejection.
- AuthTokenCookieFilter:
- Scope to /api/* only. /actuator/health and similar must not be
cookie-authenticated.
- Refuse malformed percent-encoding (URLDecoder throws); forward the
request without a promoted Authorization rather than crash.
- Use isBlank() instead of isEmpty() per Nora.
- Javadoc warning: getHeaderNames/getHeaders exposes the Basic
credential; any future header-iterating logger must scrub
Authorization before logging.
- Tests: add `passes_through_unchanged_when_request_is_outside_api_scope`
(/actuator/health with cookie should NOT be wrapped) and
`passes_through_unchanged_when_cookie_value_is_malformed_percent_encoding`.
Tighten the explicit-header test to verify same-instance forwarding
rather than just header equality.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -39,17 +39,36 @@ import java.util.Enumeration;
|
||||
*
|
||||
* <p>See #520. Filter runs at {@code Ordered.HIGHEST_PRECEDENCE} so it
|
||||
* mutates the request before any Spring Security filter sees it.
|
||||
*
|
||||
* <p><b>Scope:</b> only {@code /api/*} requests are touched. The
|
||||
* {@code /actuator/*} block in Caddy plus the open auth/reset paths in
|
||||
* {@link SecurityConfig} must NOT receive a promoted Authorization.
|
||||
*
|
||||
* <p><b>⚠ Log-leakage warning:</b> the wrapped request exposes the
|
||||
* Authorization header via {@code getHeaderNames}/{@code getHeaders}. Any
|
||||
* filter or interceptor that iterates request headers will see the live
|
||||
* Basic credential. Do NOT add a request-header logger downstream of this
|
||||
* filter without explicitly scrubbing the {@code Authorization} field.
|
||||
*/
|
||||
@Component
|
||||
@Order(org.springframework.core.Ordered.HIGHEST_PRECEDENCE)
|
||||
public class AuthTokenCookieFilter extends OncePerRequestFilter {
|
||||
|
||||
static final String COOKIE_NAME = "auth_token";
|
||||
static final String SCOPE_PREFIX = "/api/";
|
||||
|
||||
@Override
|
||||
protected void doFilterInternal(HttpServletRequest request,
|
||||
HttpServletResponse response,
|
||||
FilterChain chain) throws ServletException, IOException {
|
||||
// Scope: only /api/* needs cookie promotion. /actuator/health (open),
|
||||
// /api/auth/forgot-password (open), /login etc. don't.
|
||||
if (!request.getRequestURI().startsWith(SCOPE_PREFIX)) {
|
||||
chain.doFilter(request, response);
|
||||
return;
|
||||
}
|
||||
// An explicit Authorization header wins — this is the SSR fetch path
|
||||
// (hooks.server.ts builds the header itself).
|
||||
if (request.getHeader(HttpHeaders.AUTHORIZATION) != null) {
|
||||
chain.doFilter(request, response);
|
||||
return;
|
||||
@@ -60,8 +79,17 @@ public class AuthTokenCookieFilter extends OncePerRequestFilter {
|
||||
return;
|
||||
}
|
||||
for (Cookie c : cookies) {
|
||||
if (COOKIE_NAME.equals(c.getName()) && c.getValue() != null && !c.getValue().isEmpty()) {
|
||||
String decoded = URLDecoder.decode(c.getValue(), StandardCharsets.UTF_8);
|
||||
if (COOKIE_NAME.equals(c.getName()) && c.getValue() != null && !c.getValue().isBlank()) {
|
||||
String decoded;
|
||||
try {
|
||||
decoded = URLDecoder.decode(c.getValue(), StandardCharsets.UTF_8);
|
||||
} catch (IllegalArgumentException malformed) {
|
||||
// Malformed percent-encoding — refuse to forward a bogus
|
||||
// Authorization header. Spring Security will treat the
|
||||
// request as unauthenticated.
|
||||
chain.doFilter(request, response);
|
||||
return;
|
||||
}
|
||||
chain.doFilter(new AuthHeaderRequest(request, decoded), response);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -37,12 +37,20 @@ public class SecurityConfig {
|
||||
@Bean
|
||||
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
|
||||
http
|
||||
// CSRF is intentionally disabled: every request from the SvelteKit frontend
|
||||
// carries an explicit Authorization header (Basic Auth token injected by
|
||||
// hooks.server.ts). Browsers block cross-origin requests from setting custom
|
||||
// headers, so cross-site request forgery via a third-party page is not
|
||||
// possible with this auth scheme. If the auth model ever changes to
|
||||
// cookie-based sessions, CSRF protection must be re-enabled.
|
||||
// CSRF is intentionally disabled. With the cookie-promotion model
|
||||
// (auth_token cookie → Authorization header via AuthTokenCookieFilter,
|
||||
// see #520), every authenticated request to /api/* now carries the
|
||||
// credential automatically once the cookie is set. The CSRF defence
|
||||
// for state-changing endpoints is therefore LOAD-BEARING on:
|
||||
//
|
||||
// 1. SameSite=strict on the auth_token cookie (login/+page.server.ts).
|
||||
// A cross-site POST from evil.com cannot include the cookie.
|
||||
// 2. CORS — Spring's default rejects cross-origin requests with
|
||||
// credentials unless explicitly allowed (no allowedOrigins config).
|
||||
//
|
||||
// If either of those is ever weakened (e.g. cookie flipped to
|
||||
// SameSite=lax, CORS allowedOrigins expanded), CSRF protection
|
||||
// MUST be re-enabled here.
|
||||
.csrf(csrf -> csrf.disable())
|
||||
|
||||
.authorizeHttpRequests(auth -> {
|
||||
|
||||
@@ -29,6 +29,7 @@ class AuthTokenCookieFilterTest {
|
||||
@Test
|
||||
void promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
req.setCookies(new Cookie("auth_token", "Basic%20YWRtaW5AZmFtaWx5YXJjaGl2ZS5sb2NhbDpzZWNyZXQ%3D"));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
@@ -47,6 +48,7 @@ class AuthTokenCookieFilterTest {
|
||||
@Test
|
||||
void preserves_explicit_Authorization_header_and_ignores_cookie() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
req.addHeader("Authorization", "Basic explicit-header-wins");
|
||||
req.setCookies(new Cookie("auth_token", "Basic%20cookie-would-have-promoted"));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
@@ -54,15 +56,14 @@ class AuthTokenCookieFilterTest {
|
||||
|
||||
filter.doFilter(req, res, chain);
|
||||
|
||||
ArgumentCaptor<HttpServletRequest> captor = ArgumentCaptor.forClass(HttpServletRequest.class);
|
||||
verify(chain).doFilter(captor.capture(), org.mockito.ArgumentMatchers.any(HttpServletResponse.class));
|
||||
assertThat(captor.getValue().getHeader("Authorization"))
|
||||
.isEqualTo("Basic explicit-header-wins");
|
||||
// Forwards the original request unchanged — same instance, no wrapping.
|
||||
verify(chain).doFilter(req, res);
|
||||
}
|
||||
|
||||
@Test
|
||||
void passes_through_when_no_cookies_at_all() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
|
||||
@@ -74,6 +75,7 @@ class AuthTokenCookieFilterTest {
|
||||
@Test
|
||||
void passes_through_when_auth_token_cookie_is_absent() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
req.setCookies(new Cookie("some_other_cookie", "value"));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
@@ -86,6 +88,7 @@ class AuthTokenCookieFilterTest {
|
||||
@Test
|
||||
void passes_through_when_auth_token_cookie_is_empty() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
req.setCookies(new Cookie("auth_token", ""));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
@@ -94,4 +97,38 @@ class AuthTokenCookieFilterTest {
|
||||
|
||||
verify(chain).doFilter(req, res);
|
||||
}
|
||||
|
||||
@Test
|
||||
void passes_through_unchanged_when_request_is_outside_api_scope() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
// /actuator/health and similar must NOT receive a promoted Authorization
|
||||
// header — they have their own access rules and should never be authed
|
||||
// via the cookie.
|
||||
req.setRequestURI("/actuator/health");
|
||||
req.setCookies(new Cookie("auth_token", "Basic%20YWR=="));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
|
||||
filter.doFilter(req, res, chain);
|
||||
|
||||
// Forwards the original request unchanged — same instance, no wrapping.
|
||||
verify(chain).doFilter(req, res);
|
||||
}
|
||||
|
||||
@Test
|
||||
void passes_through_unchanged_when_cookie_value_is_malformed_percent_encoding() throws Exception {
|
||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||
req.setRequestURI("/api/users/me");
|
||||
// Lone "%" without two hex digits → URLDecoder throws → filter must
|
||||
// refuse to forward a bogus Authorization header.
|
||||
req.setCookies(new Cookie("auth_token", "Basic%2"));
|
||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||
FilterChain chain = mock(FilterChain.class);
|
||||
|
||||
filter.doFilter(req, res, chain);
|
||||
|
||||
// Forwards the original request unchanged — Spring Security treats it
|
||||
// as unauthenticated rather than crashing on bad input.
|
||||
verify(chain).doFilter(req, res);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -8,7 +8,7 @@ export const load: PageServerLoad = ({ url }) => {
|
||||
};
|
||||
|
||||
export const actions = {
|
||||
login: async ({ request, cookies, fetch }) => {
|
||||
login: async ({ request, cookies, fetch, url }) => {
|
||||
const data = await request.formData();
|
||||
const email = data.get('email') as string;
|
||||
const password = data.get('password') as string;
|
||||
@@ -37,11 +37,17 @@ export const actions = {
|
||||
return fail(500, { error: getErrorMessage('INTERNAL_ERROR') });
|
||||
}
|
||||
|
||||
// The cookie IS the API credential — promoted to `Authorization: Basic …`
|
||||
// on every browser → backend request by AuthTokenCookieFilter on the
|
||||
// Spring side (see #520). It must be Secure on HTTPS or it leaks
|
||||
// a 24h Basic token on plaintext networks. Dev runs over HTTP and
|
||||
// would silently lose the cookie if we hardcoded secure=true.
|
||||
const isHttps = url.protocol === 'https:';
|
||||
cookies.set('auth_token', authHeader, {
|
||||
path: '/',
|
||||
httpOnly: true,
|
||||
sameSite: 'strict',
|
||||
secure: false, // set to true when HTTPS is available
|
||||
secure: isHttps,
|
||||
maxAge: 60 * 60 * 24
|
||||
});
|
||||
} catch (e) {
|
||||
|
||||
Reference in New Issue
Block a user