Problem

  • This code in drupal_settings_initialize() belongs to the responsibility of the session handler:
      // To prevent session cookies from being hijacked, a user can configure the
      // SSL version of their website to only transfer session cookies via SSL by
      // using PHP's session.cookie_secure setting. The browser will then use two
      // separate session cookies for the HTTPS and HTTP versions of the site. So we
      // must use different session identifiers for HTTPS and HTTP to prevent a
      // cookie collision.
      if ($is_https) {
        ini_set('session.cookie_secure', TRUE);
      }
      $prefix = ini_get('session.cookie_secure') ? 'SSESS' : 'SESS';
      session_name($prefix . substr(hash('sha256', $session_name), 0, 32));
    

Proposed solution

  1. Move those lines into drupal_session_initialize() in session.inc.
CommentFileSizeAuthor
#1 drupal8.session-handler-initialize.1.patch4.53 KBsun
FAILED: [[SimpleTest]]: [MySQL] 52,634 pass(es), 9 fail(s), and 0 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

sun’s picture

Status: Active » Needs review
Issue tags: +API clean-up
FileSize
4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 52,634 pass(es), 9 fail(s), and 0 exception(s). View

This is what I'd propose — simply moves the code into the right spot.

Status: Needs review » Needs work

The last submitted patch, drupal8.session-handler-initialize.1.patch, failed testing.

neclimdul’s picture

I would suggest leaving the automatic cookie domain detection and cleanup outside the session initialization so it can be used without creating a session. Its handy for setting your own cookies and sometimes that should happen outside of having a session.

sun’s picture

Adding as a child to Symfony Session meta issue + the kernelize-bootstrap patch also touches this code in significant ways.

znerol’s picture

I just realized that we test $request->cookies->has(session_name()); in _drupal_bootstrap_page_cache(). Therefore I fear we cannot possibly defer setting the session_name.

sun’s picture

Hm. Good find!

However, the page_cache bootstrap phase comes after booting the kernel, so theoretically, it ought to be possible to consult a service (e.g. session_manager) to (1) set up the session name and (2) check whether the current request has a valid session?

That's not necessarily cookie based even... I wonder how this page cache construct in HEAD ever worked with alternative session + authentication implementations...?

→ In fact, shouldn't this page cache code actually consult the Authentication Manager to check whether any authentication provider returns that there is a valid user session?

The Cookie auth provider would in turn consult Session Manager, which needs to check whether the session cookie name matches the session name?

andypost’s picture

Is the issue still valid?

sun’s picture

Yes, the code still exists, pretty much as before, just lives in DrupalKernel now.

Theoretically, it should simply live on the SessionHandler or SessionManager service itself, but that ain't possible, because the session name is set up in DrupalKernel::bootEnvironment(), before a service container exists.

That said, I don't know why it has to be set up that early. What's the use-case, if you can't use any services, including Authentication?

znerol’s picture

Status: Needs work » Closed (duplicate)