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().
Comment | File | Size | Author |
---|---|---|---|
#7 | 2122761-SessionHttpsTest-7.patch | 1.21 KB | znerol |
#1 | 2122761-TESTS_ONLY.patch | 635 bytes | heddn |
#1 | 2122761-SessionHttpsTest.patch | 757 bytes | heddn |
Comments
Comment #1
heddnComment #2
heddnLet'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().
Comment #3
heddnThat 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
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedComment #5
heddn1: 2122761-SessionHttpsTest.patch queued for re-testing.
Comment #6
heddn1: 2122761-TESTS_ONLY.patch queued for re-testing.
Comment #7
znerol CreditAttribution: znerol commentedWow, this is really bad. Let's just isolate the test-case into its own method.
Comment #8
sunNot 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: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.
Comment #9
dawehnerThis is a good fix for now, but this test needs more research, given that $form is modified but not used in multiple places.
Comment #10
webchickNice catch.
Committed and pushed to 8.x.Thanks!
Comment #12
znerol CreditAttribution: znerol commentedFollowups: