Fix double submit csrf cookie - #4010
Conversation
…-prefix-samesite-none-session / uaa ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR updates UAA’s CookieBasedCsrfTokenRepository to emit and read a __Host--prefixed CSRF cookie on secure requests, aligning cookie attributes (name/prefix, Secure, and Path) with modern browser expectations for stronger CSRF cookie handling.
Changes:
- Generate
__Host-<cookieName>on secure requests and usePath=/for that cookie. - Adjust token loading logic to look for the secure vs non-secure cookie name depending on request security.
- Update unit test helper logic to account for Spring Framework 7
MockHttpServletResponsecookie parsing changes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CookieBasedCsrfTokenRepository.java |
Adds secure-aware cookie naming (__Host-), Secure flag, and Path selection; updates lookup logic accordingly. |
server/src/test/java/org/cloudfoundry/identity/uaa/web/CookieBasedCsrfTokenRepositoryTests.java |
Updates test cookie extraction to handle Set-Cookie header parsing changes and introduces expectations around secure cookie naming. |
Comments suppressed due to low confidence (2)
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CookieBasedCsrfTokenRepository.java:145
loadToken()uses the rawparameterNameto buildexpectedCookieName. IfparameterNameis ever configured with a leading__Host-, secure requests will look for a double-prefixed cookie (__Host-__Host-…) and non-secure requests will look for a__Host-…cookie that browsers may reject. Normalize to an unprefixed base name (as insaveToken()) before applying__Host-based on request security.
boolean isSecure = secure || "https".equals(request.getScheme());
String expectedCookieName = isSecure ? "__Host-" + getParameterName() : getParameterName();
server/src/test/java/org/cloudfoundry/identity/uaa/web/CookieBasedCsrfTokenRepositoryTests.java:172
- The changes introduce different cookie names and paths when the request is secure (
__Host-…withPath=/). The current tests only assert the Secure/HttpOnly flags and don’t verify that theSet-Cookieheader actually uses the expected__Host-name andPath=/(or that non-secure requests keep the unprefixed name and context-path-based Path). Adding explicit assertions for these new behaviors would prevent regressions.
boolean expectSecure = isSecure || "https".equals(protocol);
String expectedCookieName = expectSecure ? "__Host-X-Uaa-Csrf" : "X-Uaa-Csrf";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CookieBasedCsrfTokenRepository.java:147
loadTokenuses the same scheme-based secure detection assaveToken. To keep behavior consistent and correctly reflect a secure transport (including when the container marks the request secure), incorporaterequest.isSecure()and a case-insensitive scheme fallback here too.
boolean isSecure = secure || "https".equals(request.getScheme());
String baseName = getParameterName().startsWith("__Host-") ? getParameterName().substring("__Host-".length()) : getParameterName();
String expectedCookieName = isSecure ? "__Host-" + baseName : baseName;
server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CookieBasedCsrfTokenRepository.java:120
isSecureis derived fromrequest.getScheme()only. In servlet containers (and withForwardedHeaderFilter),request.isSecure()is the canonical signal for a secure connection; relying on scheme alone can be incorrect and is also inconsistent with other cookie-setting code in this repo (e.g.AccountSavingAuthenticationSuccessHandlerusesrequest.isSecure()). Consider incorporatingrequest.isSecure()(and a case-insensitive scheme fallback) in bothsaveTokenandloadToken.
This issue also appears on line 145 of the same file.
boolean isSecure = secure || "https".equals(request.getScheme());
String baseName = token.getParameterName().startsWith("__Host-") ? token.getParameterName().substring("__Host-".length()) : token.getParameterName();
String cookieName = isSecure ? "__Host-" + baseName : baseName;
server/src/test/java/org/cloudfoundry/identity/uaa/web/CookieBasedCsrfTokenRepositoryTests.java:176
- The helper verifies the cookie name and extracts the value from the
Set-Cookieheader, but it doesn’t validate the other__Host-invariants that this PR is meant to enforce (at minimumPath=/, and noDomain=attribute). Adding assertions here would make the tests fail fast if the production code regresses on the host-cookie requirements.
boolean expectSecure = isSecure || "https".equals(protocol);
String expectedCookieName = expectSecure ? "__Host-X-Uaa-Csrf" : "X-Uaa-Csrf";
String setCookie = response.getHeader("Set-Cookie");
if (setCookie == null || !setCookie.contains(expectedCookieName + "=")) {
throw new IllegalStateException("Expected Set-Cookie header for " + expectedCookieName + " but was: " + setCookie);
}
Ticket: double-submit-csrf-cookie-without-host-prefix-samesite-none-session
Fix: Adjusted CookieBasedCsrfTokenRepository to inject the __Host- prefix on the cookie name, strict / path, and Secure attribute dynamically whenever the connection is secure.