Cleanup SessionHttpsTest and fix redirect to non-existing URL after POST requests

+++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
@@ -281,6 +264,84 @@ protected function testCsrfTokenWithMixedModeSsl() {
+    $base_url . 'index.php/';

This line is suspect as we're concatenating 'index.php' to $base_url but we're not storing that result. Effectively this does nothing.

Does the fact that it does nothing break assumptions of what this test is doing? Looks like the following functions may all be wrong as a result;
loginHttp
loginHttps
testCsrfTokenWithMixedModeSsl

Not a full answer but curious.

+++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
@@ -28,44 +43,36 @@ class SessionHttpsTest extends WebTestBase {
-    $this->assertTrue($this->cookies[$secure_session_name]['secure'], 'The secure cookie has the secure attribute');

Even though cookies is a sensical name for this. There isn't any container named cookies in this class or the base class WebTestBase.

however this is a cookieFile. Should that be used?

@znerol
Do you think you can fix the failures? Would be great to not have the security problem for too long.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -793,7 +793,6 @@ protected function initializeRequestGlobals(Request $request) {
   -    global $base_secure_url, $base_insecure_url;
   

   Yay! fewer globals.

2. +++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
   @@ -28,81 +43,75 @@ class SessionHttpsTest extends WebTestBase {
   +    $this->curlCookies = array();
   

   Does nulling out the curlCookies make more sense inside the curlClose(); function?

   Is this something we want to optionally do?

+++ b/core/modules/system/tests/modules/session_test/src/EventSubscriber/SessionTestSubscriber.php
@@ -68,9 +52,9 @@ public function onKernelResponseSessionTest(FilterResponseEvent $event) {
+    $events[KernelEvents::RESPONSE][] = array('onKernelResponseSessionTest');
+    $events[KernelEvents::REQUEST][] = array('onKernelRequestSessionTest');

why are we removing priority?

OK, in reviewing the interdiffs it appears that the priority ensured that the tests get access to specialized logic that was altering the request. It appears that this was a special case that the original developers introduced because they thought it was necessary. Apparently it isn't anymore so great. This patch seems RTBC to me.

Thanks @znerol!

Downgrading to major based on discussion with @znerol; this is important but not release-blocking, and there isn't an actual vulnerability here, just a need for hardening and better test coverage for safe refactoring later.

Thanks @znerol for all your work on this!

What needs to be tested to mark this RTBC.

I do agree that * An absolute URL. isn't very useful. But I do wonder if this could be shortened to a single line:

+   *   The URL prepared in a way such that the https.php mock front controller
+   *   is used.

Say "Absolute URL prepared for the https.php mock front controller"?

I think this is longer and less clear than it needs to be "The URL prepared in a way such that the https.php mock front controller is used."

How about something like:
"URL prepared for the https.php mock front controller."

Are we sure this is not a critical issue as it seems to block #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session ?

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -798,7 +798,6 @@ protected function initializeRequestGlobals(Request $request) {
   -    global $base_secure_url, $base_insecure_url;
   
   @@ -839,8 +838,6 @@ protected function initializeRequestGlobals(Request $request) {
        }
   -    $base_secure_url = str_replace('http://', 'https://', $base_url);
   -    $base_insecure_url = str_replace('https://', 'http://', $base_url);
    
   

   Oh, that is great for itself!

2. +++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
   @@ -28,81 +43,68 @@ class SessionHttpsTest extends WebTestBase {
      protected function setUp() {
        parent::setUp();
   -    $this->request = Request::createFromGlobals();
   -    $this->container->get('request_stack')->push($this->request);
   -  }
    
   -  protected function testHttpsSession() {
   -    if ($this->request->isSecure()) {
   -      $secure_session_name = $this->getSessionName();
   -      $insecure_session_name = substr($this->getSessionName(), 1);
   +    $request = Request::createFromGlobals();
   +    if ($request->isSecure()) {
   +      $this->secureSessionName = $this->getSessionName();
   +      $this->insecureSessionName = substr($this->getSessionName(), 1);
        }
        else {
   -      $secure_session_name = 'S' . $this->getSessionName();
   -      $insecure_session_name = $this->getSessionName();
   +      $this->secureSessionName = 'S' . $this->getSessionName();
   +      $this->insecureSessionName = $this->getSessionName();
        }
   +  }
    
   

   For sure that is part of another issue, but is there a reason we can't create a request object suited for the HTTP and one for the HTTPs usecase?

3. +++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
   @@ -133,62 +126,57 @@ protected function testMixedModeSslSession() {
        $this->assertNotEqual(substr($form[0]['action'], 0, 6), 'https:', 'Password request form action is not secure');
   -    $form[0]['action'] = $this->httpsUrl('user/login');
    
   ...
        $this->assertEqual(substr($form[0]['action'], 0, 6), 'https:', 'Login form action is secure');
   -    $form[0]['action'] = $this->httpsUrl('user/login');
    
   

   Can we do this changes because we drop mix mode?

4. +++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
   @@ -269,13 +251,22 @@ protected function testCsrfTokenWithMixedModeSsl() {
   +    $maximum_redirects = $this->maximumRedirects;
   +    $this->maximumRedirects = 0;
   +    $this->drupalPostForm(NULL, $edit, 'Save');
   +    $this->maximumRedirects = $maximum_redirects;
   +
   +    // Follow the redirect header.
   +    $path = $this->getPathFromLocationHeader(TRUE);
   +    $this->drupalGet($this->httpUrl($path));
   +    $this->assertResponse(200);
   

   It would be great to maybe have a short documentation line telling what is going on: We disable the automatic redirect, in order to do what?...

5. +++ b/core/modules/system/src/Tests/Session/SessionHttpsTest.php
   @@ -284,6 +275,93 @@ protected function testCsrfTokenWithMixedModeSsl() {
   +    // Generate the base_url.
   +    $base_url = $this->container->get('url_generator')->generateFromPath('', array('absolute' => TRUE));
   

   Can't you also use $this->container->get('url_generator')->generateFromRoute('', [], ['absolute' => TRUE]);?

What was I doing trying to test a diff???? Anyways, got the patch in #48.

Just a re-roll

Gonna be bold. It looks great, RTBC!

Needs a beta evaluation (unfrozen: tests-only) and a CR addition to remove $base_url and $base_secure_url. (sniff, I liked those ...)

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -826,7 +826,6 @@ protected function initializeRequestGlobals(Request $request) {
-    global $base_secure_url, $base_insecure_url;

@@ -864,8 +863,6 @@ protected function initializeRequestGlobals(Request $request) {
-    $base_secure_url = str_replace('http://', 'https://', $base_url);
-    $base_insecure_url = str_replace('https://', 'http://', $base_url);

These have been around since d7 - contrib code might be relying on them - we should update a CR or provide a new one. It would be great if we can get rid of DrupalKernel::initializeRequestGlobals() - is there an issue to do that? We could just remove the removals from this issue and do the cleanup there.

@znerol for $is_http_mock and $is_https_mock - i don't think so - that is truly just test stuff for the tests you are changing.

Contrib code like securepages / secureforms / ... indeed tries to rely on those super globals, however it should be fine to remove them with a proper CR, etc.

Thanks for restoring for now.

Back to RTBC

This is a test only change and therefore permitted in beta. Committed d29700f and pushed to 8.0.x. Thanks!