Since #2631220: Session fixation for anonymous users - discard invalid session identifiers instead of accepting them strict mode is enabled for PHP sessions. When an anonymous request carries a session cookie whose ID does not exist in the sessions table (e.g. the session was garbage collected, while the cookie is still valid in the browser), PHP rejects the ID, generates a new one and immediately queues a Set-Cookie header for it.
If the request does not store anything in the session, SessionManager::save() considers the session obsolete and calls SessionManager::destroy(). That method calls setcookie($session_name, '', ...) because the request contained a session cookie, but it never removes the header PHP already queued. The response therefore contains two Set-Cookie headers for the same cookie:
HTTP/1.1 302 Found Set-Cookie: SSESSa38ae…=3t8kebttfpkk…r7iqtjgg5g; path=/; domain=…; secure; HttpOnly; SameSite=Lax Set-Cookie: SSESSa38ae…=deleted; expires=Thu, 01 Jan 1970 00:00:01 GMT; Max-Age=0; path=/; domain=…; secure; HttpOnly
Browsers apply the last header, so functionally the stale cookie is removed.
However:
Set-Cookie header field in the same response with the same cookie-name.Symfony's AbstractSessionHandler::destroy() handles this case with SessionUtils::popSessionCookie(), but Drupal's SessionHandler::destroy() overrides it and only deletes the database record (see also #3187037).
curl -sk -D - -o /dev/null -b "SSESS<hash>=invalidsessionid1234567890" https://example.com/
Set-Cookie headers for the session cookie: one with a new session ID followed by one with the value deleted.In SessionManager::destroy(), remove any queued Set-Cookie header for the session cookie (keeping all other cookies) before sending the deletion cookie, similar to SessionUtils::popSessionCookie().
SessionUtils::popSessionCookie() instead of a Drupal helper.None.
New protected method SessionManager::removeSessionCookieHeaders().
None.
N/A
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
zero2one commentedComment #3
zero2one commentedComment #4
zero2one commentedComment #5
zero2one commentedComment #7
sourav_paulHi @zero2one,
I've tested this locally on 11.4.x (11.4.9-dev, PHP 8.5.9, MariaDB 10.11, DDEV + nginx). Here's what I found.
Reproducing the bug
-------------------
I fired a request with a bogus session cookie at a page that stores nothing in the session (/admin as anonymous) and got:
set-cookie: SSESS...=e2ec3029458fde98bdb5dacdc113ca55; expires=...; Max-Age=2000000; ...
set-cookie: SSESS...=deleted; expires=Thu, 01 Jan 1970 00:00:01 GMT; Max-Age=0; ...
Exactly the problem described in the issue: strict mode queues a fresh session ID nobody will ever use, and SessionManager::destroy() then adds the deletion cookie on top of it. Two headers for the same cookie in one response.
After the patch
---------------
Same requests (/admin, /user, a 404 path) now come back with a single header:
set-cookie: SSESS...=deleted; expires=Thu, 01 Jan 1970 00:00:01 GMT; Max-Age=0; ...
Other cookies are left alone, and a normal login still sets exactly one session cookie as before, so day-to-day session behaviour hasn't changed.
Test runs
---------
- The new testInvalidSessionCookieOnEmptySession fails without the SessionManager.php change (9 passed, 1 failed) and passes with it, so it really does pin down this bug.
- Whole SessionTest class: 10 passed.
- Whole "Session" group: everything passes except SessionHttpsTest ("Call to a member function getSecure() on null"). I re-ran it with the patch stashed and it errors the same way, so that one's pre-existing on my setup (HTTPS mock issue), not caused by this MR.
- Patch applies cleanly to current 11.4.x, no conflicts, php -l clean on both files.
On the suggestion to reuse Symfony's SessionUtils::popSessionCookie() or keeping new helper SessionManager::removeSessionCookieHeaders()
---------------------------------------------------------------------
I looked into this properly and landed on "keep Drupal helper". Short version why:
1. SessionUtils is marked @internal in Symfony, so it carries no BC promise - it can change or disappear in any minor release while core just pins symfony/http-foundation ^7.4. The @see in the docblock credits Symfony's approach without taking on that risk, which feels like the right trade-off.
2. The semantics don't actually fit what destroy() needs. popSessionCookie() only removes the header matching one exact session ID, and keeps other pending cookies for the same name. We're about to send the deletion cookie for that name, so anything left over still gives us the same set-and-delete pair. Worse, session_id() returns an empty string after session_destroy() (which destroy() calls first) - so a straight call would quietly remove nothing while still reporting it found something. I verified both of these with a small live test.
3. From Drupal's side: the bug lives in SessionManager, not the save handler, and SessionHandler::doDestroy() is a documented extension point contrib overrides - a fix on that path can be bypassed. A protected method on SessionManager stays overridable and falls under Drupal's own BC guarantees, which a static call into someone else's internal class doesn't.
Conclusion
----------
The fix does what it says, the new test covers the issue, and I'm not seeing regressions in session handling. Hence moving to 'Needs Review'.
[Used AI for formatting]
Comment #8
quietone commentedIssues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.
I am adding a reminder that our policy states that comments an issue summaries are to be in the contributors own words and to be concise. We ask that you respect the time other contributors put into reviewing you prose. See the warning box at Policy on the use of AI when contributing to Drupal.
@sourav_paul, that looks like Markdown, which is not used by Drupal core issue.
Comment #9
sourav_paulThanks @quietone for the review and guidance.
I’ll keep that in mind for future contributions.
Comment #10
zero2one commentedThank you @sourav_paul for validating the bug and testing out the merge request.