Problem/Motivation

Currently the SessionHttpsTest::testHttpsSession() returns early half way through. The reason is that the mixed-mode SSL tests apparently cannot be run when the test-site is accessed via SSL. However, due to a lack of isolation between test-cases, the mixed-mode SSL tests aren't run when executed via a plain connection neither.

The problem is that on line 111 SessionHttpsTest::httpsUrl() is called which sets $this->request->server->set('HTTPS', 'on'); and therefore the subsequent call to $this->request->isSecure() always will return TRUE.

 108     // Verify that empty secure SID cannot be used on the secure site.
 109     $this->curlClose();
 110     $cookie = $secure_session_name . '=';
 111     $this->drupalGet($this->httpsUrl('admin/config'), array(), array('Cookie: ' . $cookie));
 112     $this->assertResponse(403);
 113 
 114     // Clear browser cookie jar.
 115     $this->cookies = array();
 116 
 117     if ($this->request->isSecure()) {
 118       // The functionality does not make sense when running on HTTPS.
 119       return;
 120     }

Proposed resolution

Extract the mixed mode SSL test into its own test method.

Remaining tasks

User interface changes

API changes

Original report by @heddn

While working on #961508: Dual http/https session cookies interfere with drupal_valid_token(), I noticed that the tests never actually get run. They seemed to stop at line 110. When removing the early exist, it caused a bunch of errors, so let's fix those, then proceed with #961508: Dual http/https session cookies interfere with drupal_valid_token().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn’s picture

heddn’s picture

Status: Active » Needs review

Let's see how the tests fare in #1. Essentially, while working on #961508: Dual http/https session cookies interfere with drupal_valid_token(), I noticed that the tests never actually get run. They seemed to stop at line 110. When removing the early exist, it caused a bunch of errors, so let's fix those, then proceed with #961508: Dual http/https session cookies interfere with drupal_valid_token().

heddn’s picture

That didn't seem to phase the testbot, which is good sinc it didn't do as well locally. For those reviewing this issue, notice the difference in test results. A lot more tests ran with this patch.

TESTS_ONLY -- Drupal\system\Tests\Session\SessionHttpsTest 35 0 0
Patch -- Drupal\system\Tests\Session\SessionHttpsTest 79 0 0

cosmicdreams’s picture

Issue summary: View changes
heddn’s picture

1: 2122761-SessionHttpsTest.patch queued for re-testing.

heddn’s picture

1: 2122761-TESTS_ONLY.patch queued for re-testing.

znerol’s picture

Issue summary: View changes
FileSize
1.21 KB

Wow, this is really bad. Let's just isolate the test-case into its own method.

sun’s picture

     if ($this->request->isSecure()) {

Not to mention that run-tests.sh is a CLI script and this code runs in the test runner (as opposed to in the child test site), so the condition heavily depends on the fake/mock Request created by run-tests.sh/TestBase to get actually set up correctly for HTTPS. (...does the Request object even support that?)

The fake request is controlled by the --url parameter to run-tests.sh, and apparently, testbots do not pass a secure URL:

run-tests.sh … --url http://drupaltestbot953/checkout …

Thus, this test code doesn't ever seem to get executed... - except of mayhaps the very rare case in which you run the test locally and your local dev site supports SSL and you pass HTTPS URL to run-tests.sh...

That's a preexisting issue though, and this comment not really meant to hold up this patch - splitting those tests makes sense to me.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is a good fix for now, but this test needs more research, given that $form is modified but not used in multiple places.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice catch.

Committed and pushed to 8.x.Thanks!

  • Commit cf7e2db on 8.x by webchick:
    Issue #2122761 by znerol, heddn: SessionHttpsTest only runs half the...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.