Problem/Motivation

When mixed mode SSL sessions are activated the system is supposed to behave like described in #813492-12: HTTPS sessions use an invalid merge query:

what needs to be tested is: put something in the session as HTTP, login as HTTPS, verify that the session has been carried over, get back to the original anonymous cookie, verify that this session is now inactive

A test has been added in comment #17 of that aforementioned issue, however due to wrong parameters to drupalGet() it was defective from the beginning:

+++ modules/simpletest/tests/session.test	14 Jun 2010 04:00:06 -0000
@@ -354,6 +365,33 @@ class SessionHttpsTestCase extends Drupa
+    // Test that session data saved before login is not available using the ¶
+    // pre-login anonymous cookie.
+    $this->cookies = array();
+    $this->drupalGet('session-test/get', array('Cookie: ' . $anonymous_cookie));
+    $this->assertNoText($session_data, 'Initial anonymous session is inactive after login.');

The cookie-header should be supplied as the third parameter, the second parameter is $options.

Manual verification revealed that in fact HTTP session-data is still available when using the insecure cookie even after the session has been regenerated during login. Note that new session data (added after login) is not available through the old anonymous session.

Proposed resolution

Either accept the current situation and remove the test or fix the test and the implementation.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
FileSize
808 bytes

Status: Needs review » Needs work

The last submitted patch, 1: 2330279-fix-test-only.patch, failed testing.

sun’s picture

If that was the intention of the test (clearly seems so), then we need to fix the code to meet the expectation.

znerol’s picture

znerol’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

This is the minimal fix. Delete the old session record immediately after migrating an anonymous HTTP session to HTTPS. The HTTPS session will be written to the database again.

If this is too risky we might store the old HTTP session-id in a property of SessionHandler and only remove the record when the new one has been written.

sun’s picture

Looks good to me.

Which risks do you see? The only one I can think of would be that the new HTTPS request response produces output before HTTP headers are sent, but in that case you have a much larger problem anyway?

znerol’s picture

Well, if the request is terminated in between SessionHandler::read() and SessionHandler::write(), the session data will be lost.

Fabianx’s picture

Status: Needs review » Needs work

I think it is better to do the garbage collection in ::write() and just set a flag in ::read(). The function are pretty complex already anyway, so this is a good opportunity to give this some OOP feeling:

caller:

$this->addGC($session_id);

consumer:

::write

$this->doGC();

Should solve it nicely and generically.

Also avoids writing in a function that is called ::read, which is the weirdest part of this patch IMHO.

znerol’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
5.47 KB

I'm not too comfortable with adding more methods to SessionHandler. In my opinion it should exclusively implement the PHP SessionHandlerInterface. On the other hand it is great that we can move another (dubious) db query out of SessionManager. In the long run that class should be free of them.

In the long run we might need to come up with an auxiliary service paralleling the SessionHandler capable of handling the migration / cleanup cases (e.g. also SessionManager::delete($uid).

But let's keep the scope narrow here and fix the defect. This probably also should be backported to D7.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Very nice work!

**RTBC** - looks great, tests pass, makes sense, nice future compatibility with $https = FALSE for making further things obsolete.

Works for me :)

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed 304d50a and pushed to 8.0.x. Thanks!

  • alexpott committed 304d50a on 8.0.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...
mgifford’s picture

Version: 8.0.x-dev » 7.x-dev
smussbach’s picture

Status: Patch (to be ported) » Needs review
FileSize
47.72 KB

I made a first attempt for a D7 port but there remain some questions.

I did not know what is best practice to substitute class variables in d7. Is it better to use &drupal_static() or a dedicated function with a static variable as I did?

For some strange reasons I could not create a serious diff for the single test line. Perhaps someone could help with this.

Please review thoroughly, I am eager to learn.

smussbach’s picture

seriously, I don't get it why my patches so often break directly before the upload. (Perhaps it was just a demostration of my strange problems here) Sorry!

The last submitted patch, 14: 2330279-fix-cleanup-after-mixed-mode-session-14-D7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2330279-fix-cleanup-after-mixed-mode-session-15-D7.patch, failed testing.

smussbach’s picture

Status: Needs work » Needs review
FileSize
4.84 KB

Next (and probably last) try for a patch, this time generated from my IDE, not via gitBash. Fingers crossed.
My patches are all the same, so no interdiff is needed.

Status: Needs review » Needs work

The last submitted patch, 18: 2330279-fix-cleanup-after-mixed-mode-session-18-D7.patch, failed testing.

cosmicdreams’s picture

Now that #2342593: Remove mixed SSL support from core has landed, how is this issue impacted?

Edit:
the text formatter appears to be having a problem so here's the link to the issue that removed mixed mode session support:
https://www.drupal.org/node/2342593

and here's the change notice:
https://www.drupal.org/node/2384903

znerol’s picture

This issue is for the D7 backport, so not affected at all.

denix’s picture

Is it in the plans to backport this from D8?

  • alexpott committed 304d50a on 8.1.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...

  • alexpott committed 304d50a on 8.3.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...

  • alexpott committed 304d50a on 8.3.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...

  • alexpott committed 304d50a on 8.4.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...

  • alexpott committed 304d50a on 8.4.x
    Issue #2330279 by znerol: Fixed When operating in mixed mode SSL: data...