fixup: address Nora's review on #520 (security blockers)
Some checks failed
Some checks failed
- 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 was merged in pull request #521.
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
|
* <p>See #520. Filter runs at {@code Ordered.HIGHEST_PRECEDENCE} so it
|
||||||
* mutates the request before any Spring Security filter sees 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
|
@Component
|
||||||
@Order(org.springframework.core.Ordered.HIGHEST_PRECEDENCE)
|
@Order(org.springframework.core.Ordered.HIGHEST_PRECEDENCE)
|
||||||
public class AuthTokenCookieFilter extends OncePerRequestFilter {
|
public class AuthTokenCookieFilter extends OncePerRequestFilter {
|
||||||
|
|
||||||
static final String COOKIE_NAME = "auth_token";
|
static final String COOKIE_NAME = "auth_token";
|
||||||
|
static final String SCOPE_PREFIX = "/api/";
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
protected void doFilterInternal(HttpServletRequest request,
|
protected void doFilterInternal(HttpServletRequest request,
|
||||||
HttpServletResponse response,
|
HttpServletResponse response,
|
||||||
FilterChain chain) throws ServletException, IOException {
|
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) {
|
if (request.getHeader(HttpHeaders.AUTHORIZATION) != null) {
|
||||||
chain.doFilter(request, response);
|
chain.doFilter(request, response);
|
||||||
return;
|
return;
|
||||||
@@ -60,8 +79,17 @@ public class AuthTokenCookieFilter extends OncePerRequestFilter {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
for (Cookie c : cookies) {
|
for (Cookie c : cookies) {
|
||||||
if (COOKIE_NAME.equals(c.getName()) && c.getValue() != null && !c.getValue().isEmpty()) {
|
if (COOKIE_NAME.equals(c.getName()) && c.getValue() != null && !c.getValue().isBlank()) {
|
||||||
String decoded = URLDecoder.decode(c.getValue(), StandardCharsets.UTF_8);
|
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);
|
chain.doFilter(new AuthHeaderRequest(request, decoded), response);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -37,12 +37,20 @@ public class SecurityConfig {
|
|||||||
@Bean
|
@Bean
|
||||||
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
|
public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
|
||||||
http
|
http
|
||||||
// CSRF is intentionally disabled: every request from the SvelteKit frontend
|
// CSRF is intentionally disabled. With the cookie-promotion model
|
||||||
// carries an explicit Authorization header (Basic Auth token injected by
|
// (auth_token cookie → Authorization header via AuthTokenCookieFilter,
|
||||||
// hooks.server.ts). Browsers block cross-origin requests from setting custom
|
// see #520), every authenticated request to /api/* now carries the
|
||||||
// headers, so cross-site request forgery via a third-party page is not
|
// credential automatically once the cookie is set. The CSRF defence
|
||||||
// possible with this auth scheme. If the auth model ever changes to
|
// for state-changing endpoints is therefore LOAD-BEARING on:
|
||||||
// cookie-based sessions, CSRF protection must be re-enabled.
|
//
|
||||||
|
// 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())
|
.csrf(csrf -> csrf.disable())
|
||||||
|
|
||||||
.authorizeHttpRequests(auth -> {
|
.authorizeHttpRequests(auth -> {
|
||||||
|
|||||||
@@ -29,6 +29,7 @@ class AuthTokenCookieFilterTest {
|
|||||||
@Test
|
@Test
|
||||||
void promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header() throws Exception {
|
void promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header() throws Exception {
|
||||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||||
|
req.setRequestURI("/api/users/me");
|
||||||
req.setCookies(new Cookie("auth_token", "Basic%20YWRtaW5AZmFtaWx5YXJjaGl2ZS5sb2NhbDpzZWNyZXQ%3D"));
|
req.setCookies(new Cookie("auth_token", "Basic%20YWRtaW5AZmFtaWx5YXJjaGl2ZS5sb2NhbDpzZWNyZXQ%3D"));
|
||||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||||
FilterChain chain = mock(FilterChain.class);
|
FilterChain chain = mock(FilterChain.class);
|
||||||
@@ -47,6 +48,7 @@ class AuthTokenCookieFilterTest {
|
|||||||
@Test
|
@Test
|
||||||
void preserves_explicit_Authorization_header_and_ignores_cookie() throws Exception {
|
void preserves_explicit_Authorization_header_and_ignores_cookie() throws Exception {
|
||||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||||
|
req.setRequestURI("/api/users/me");
|
||||||
req.addHeader("Authorization", "Basic explicit-header-wins");
|
req.addHeader("Authorization", "Basic explicit-header-wins");
|
||||||
req.setCookies(new Cookie("auth_token", "Basic%20cookie-would-have-promoted"));
|
req.setCookies(new Cookie("auth_token", "Basic%20cookie-would-have-promoted"));
|
||||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||||
@@ -54,15 +56,14 @@ class AuthTokenCookieFilterTest {
|
|||||||
|
|
||||||
filter.doFilter(req, res, chain);
|
filter.doFilter(req, res, chain);
|
||||||
|
|
||||||
ArgumentCaptor<HttpServletRequest> captor = ArgumentCaptor.forClass(HttpServletRequest.class);
|
// Forwards the original request unchanged — same instance, no wrapping.
|
||||||
verify(chain).doFilter(captor.capture(), org.mockito.ArgumentMatchers.any(HttpServletResponse.class));
|
verify(chain).doFilter(req, res);
|
||||||
assertThat(captor.getValue().getHeader("Authorization"))
|
|
||||||
.isEqualTo("Basic explicit-header-wins");
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void passes_through_when_no_cookies_at_all() throws Exception {
|
void passes_through_when_no_cookies_at_all() throws Exception {
|
||||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||||
|
req.setRequestURI("/api/users/me");
|
||||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||||
FilterChain chain = mock(FilterChain.class);
|
FilterChain chain = mock(FilterChain.class);
|
||||||
|
|
||||||
@@ -74,6 +75,7 @@ class AuthTokenCookieFilterTest {
|
|||||||
@Test
|
@Test
|
||||||
void passes_through_when_auth_token_cookie_is_absent() throws Exception {
|
void passes_through_when_auth_token_cookie_is_absent() throws Exception {
|
||||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||||
|
req.setRequestURI("/api/users/me");
|
||||||
req.setCookies(new Cookie("some_other_cookie", "value"));
|
req.setCookies(new Cookie("some_other_cookie", "value"));
|
||||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||||
FilterChain chain = mock(FilterChain.class);
|
FilterChain chain = mock(FilterChain.class);
|
||||||
@@ -86,6 +88,7 @@ class AuthTokenCookieFilterTest {
|
|||||||
@Test
|
@Test
|
||||||
void passes_through_when_auth_token_cookie_is_empty() throws Exception {
|
void passes_through_when_auth_token_cookie_is_empty() throws Exception {
|
||||||
MockHttpServletRequest req = new MockHttpServletRequest();
|
MockHttpServletRequest req = new MockHttpServletRequest();
|
||||||
|
req.setRequestURI("/api/users/me");
|
||||||
req.setCookies(new Cookie("auth_token", ""));
|
req.setCookies(new Cookie("auth_token", ""));
|
||||||
MockHttpServletResponse res = new MockHttpServletResponse();
|
MockHttpServletResponse res = new MockHttpServletResponse();
|
||||||
FilterChain chain = mock(FilterChain.class);
|
FilterChain chain = mock(FilterChain.class);
|
||||||
@@ -94,4 +97,38 @@ class AuthTokenCookieFilterTest {
|
|||||||
|
|
||||||
verify(chain).doFilter(req, res);
|
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 = {
|
export const actions = {
|
||||||
login: async ({ request, cookies, fetch }) => {
|
login: async ({ request, cookies, fetch, url }) => {
|
||||||
const data = await request.formData();
|
const data = await request.formData();
|
||||||
const email = data.get('email') as string;
|
const email = data.get('email') as string;
|
||||||
const password = data.get('password') as string;
|
const password = data.get('password') as string;
|
||||||
@@ -37,11 +37,17 @@ export const actions = {
|
|||||||
return fail(500, { error: getErrorMessage('INTERNAL_ERROR') });
|
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, {
|
cookies.set('auth_token', authHeader, {
|
||||||
path: '/',
|
path: '/',
|
||||||
httpOnly: true,
|
httpOnly: true,
|
||||||
sameSite: 'strict',
|
sameSite: 'strict',
|
||||||
secure: false, // set to true when HTTPS is available
|
secure: isHttps,
|
||||||
maxAge: 60 * 60 * 24
|
maxAge: 60 * 60 * 24
|
||||||
});
|
});
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
|
|||||||
Reference in New Issue
Block a user