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

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
Issue tags: +API clean-up
StatusFileSize
new drupal8.session-handler-initialize.1.patch4.53 KB

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

neclimdul
He/Him
commented

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

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Parent issue: » #1858196: [meta] Leverage Symfony Session components
Related issues: +#2016629: Refactor bootstrap to better utilize the kernel

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

znerol’s picture

znerol commented

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

sun
Primary language German
Location Karlsruhe
commented

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

andypost
he/him
Primary language Russian
commented

Is the issue still valid?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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

znerol commented
Status: Needs work » Closed (duplicate)

Closing this as a duplicate in favor of #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service. In addition the other will help with #2342593: Remove mixed SSL support from core.