Repository navigation
Conversation
…SameSite=Lax Without a web.xml, the embedded Tomcat context defaulted to COOKIE+URL session tracking, so a session id supplied only via ";jsessionid=" in the URL was honoured with no fresh Set-Cookie -- a session-fixation vector. Live-reproduced before this fix: a session established by cookie was resumed by replaying the id via the URL parameter with no Cookie header sent at all. Restricts session tracking to COOKIE only, and the session cookie now carries SameSite=Lax (previously unset). Secure is deliberately not forced: Boot opens a plain HTTP listener by default, and an operator terminating TLS in front of it sets Secure themselves, as with any other servlet container. Boot is part of 7.5.0 (unreleased, no production deployments yet), so this lands before anyone is exposed to it. Tests: StrutsBootTomcatTest gains testSessionCookieHasSameSiteLax and a URL-supplied-session-id-is-not-honoured test, with a cookie-based-session- still-works control. Full boot suite: 52/0 failures.
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks, the change is the right one and I verified it locally: both new tests fail against main's StrutsBootServer (the URL-supplied id resumed the session, counter went to 2; no SameSite) and the whole StrutsBootTomcatTest is 16/16 green on this branch. One more effect worth stating in the ticket: on main, the first response of a Boot app already rewrites the id into every link rendered by <s:url> / <s:a> (URL=/sessionProbe.action;jsessionid=…), because Tomcat's encodeURL appends it whenever the session did not arrive by cookie and URL tracking is effective. With this branch those links are clean. That is the more visible symptom and the reason cookie-only is the right default, independent of how the id gets replayed.
Changes requested:
THREAT_MODEL.md§5 lists the Boot container defaults the framework owns ("A deviation from these defaults is a Struts finding"). Please add two bullets there: session tracking is cookie-only, and the session cookie carriesSameSite=Lax. Without them the new defaults are not part of the contract.- The
Secureparagraph in the Javadoc is not true for Boot — see the inline comment. Please drop the sentence; astruts.boot.*setting for the cookie attributes can be a separate ticket. - Comments: keep them functional and short — what the code does, not the scenario it prevents. Inline comments mark the spots. Same for the test Javadoc and the
// GREEN:marker. - Naming: the two defaults are one decision; give them one method (inline).
- Commit headline: please use the repo form, e.g.
WW-5841 fix(boot): restrict session tracking to cookies and set SameSite=Lax.
The branch is 4 commits behind main; none touch boot/, so no rebase is needed for this review.
| // Without a web.xml, a bare embedded context defaults to the servlet-spec | ||
| // tracking modes (COOKIE + URL): a session id supplied as ";jsessionid=" | ||
| // in the URL is accepted and kept alive with no Set-Cookie, which is a | ||
| // session-fixation vector (the id can be planted via a link/Referer | ||
| // before the victim authenticates). Restrict to COOKIE only. |
There was a problem hiding this comment.
Please reduce this to one line that says what the code does, e.g. // Tomcat defaults to COOKIE+URL when no web.xml sets <tracking-mode>; keep cookies only. The surrounding declarations carry one-line comments or none, and the scenario narration belongs in the ticket, not in the file.
| * default (treated as Lax). Secure is deliberately not forced here: Boot opens a plain | ||
| * HTTP listener by default (§5/§5a transport is the operator's), and a Secure cookie would | ||
| * simply be refused by the browser on that default deployment -- an operator who terminates | ||
| * TLS in front of Boot sets this themselves, as with any other servlet container. |
There was a problem hiding this comment.
This paragraph does not hold for Boot. There is no web.xml, StrutsBootApplication only exposes actions(ActionRegistry), and BootSettings carries only port, address and contextPath — an operator has no way to set Secure (or change SameSite) at all. Please drop the sentence. Making the cookie attributes configurable (struts.boot.session.cookie.secure / .sameSite) is a sensible follow-up, but a separate ticket; none of the parked Boot tickets (WW-5793…WW-5808) covers it. The remaining Javadoc can be one line: Session cookie defaults for the embedded context: SameSite=Lax; Secure is left to the operator's TLS setup.
| * simply be refused by the browser on that default deployment -- an operator who terminates | ||
| * TLS in front of Boot sets this themselves, as with any other servlet container. | ||
| */ | ||
| private static void hardenSessionTracking(Context context) { |
There was a problem hiding this comment.
hardenSessionTracking only configures the cookie processor, while the tracking-mode call sits inside the registry-attribute initializer at line 86. The two are one decision; please move the setSessionTrackingModes initializer in here (as its own addServletContainerInitializer call) and name the method for what it does, e.g. configureSessionCookies(Context) or restrictSessionTracking(Context).
| /** | ||
| * Without a web.xml, a bare embedded {@code Context} | ||
| * defaults to the servlet-spec tracking modes (COOKIE + URL), so a session id supplied | ||
| * only via {@code ;jsessionid=} in the URL (no cookie at all) is honoured and kept alive | ||
| * with no fresh {@code Set-Cookie} -- the session-fixation vector. Control: the normal | ||
| * cookie-based flow must still carry a session across requests. | ||
| */ |
There was a problem hiding this comment.
Test Javadoc: one functional line is enough — A session id supplied only in the URL is not resumed; the cookie-based flow still is. The default-modes explanation is now in the production comment, and the rest is narration.
| // Attacker-style request: the session id travels only in the URL, never as a cookie. | ||
| HttpResponse<String> viaUrl = get(server.baseUrl() + "/sessionProbe.action;jsessionid=" + sessionId); | ||
| // GREEN: a brand-new session is created (counter resets to 1, a fresh id is issued) -- | ||
| // the URL-supplied id from the first request was not resumed. |
There was a problem hiding this comment.
Please drop these three comment lines (the // GREEN: marker is a TDD artefact). The assertions already say it: a new session with a different id.
…nts, threat model bullets - fold the tracking-mode initializer and the SameSite cookie processor into one method, restrictSessionTracking(Context) - drop the Secure claim from the Javadoc and reduce the comments to what the code does - reduce the test Javadoc and drop the narrating comment lines - THREAT_MODEL.md section 5: add the cookie-only tracking and SameSite=Lax defaults to the Boot container defaults list
g0w6y
changed the title
|
Addressed in bfc956e:
I'll update the ticket text to drop the |
Fixes WW-5841
Without a
web.xml, the embedded Tomcat contextStrutsBootServerstarts defaulted to the servlet-spec COOKIE+URL session tracking modes, so a session id supplied only via;jsessionid=in the URL (no cookie at all) was accepted and kept alive with no freshSet-Cookie. Live-reproduced before this fix: a session established by cookie was resumed by replaying the id via the URL parameter with no Cookie header sent at all.Restricts session tracking to COOKIE only, and the session cookie now carries
SameSite=Lax. Boot is part of 7.5.0, unreleased, so this lands before anyone is exposed to it.Verified:
StrutsBootTomcatTestgainstestSessionCookieHasSameSiteLaxand a URL-supplied-session-id test, with a cookie-based-session control. Fullboot: 52/0 failures.