Don't automatically set a cookie domain

Created on 31 January 2018, almost 7 years ago
Updated 20 February 2023, almost 2 years ago

Problem/Motivation

In SessionConfiguration we always set a cookie domain even if one is not configured. If the current domain is example.com the cookie domain is set to .example.com and if the domain is www.example.com we set it to .www.example.com. If the cookie domain is set to .example.com then the cookie will also be available on all the sub-domains. It is more secure if it is not.

Note not setting a cookie domain is an OWASP recommendation:

It is recommended to use a narrow or restricted scope for these two attributes. In this way, the β€œDomain” attribute should not be set (restricting the cookie just to the origin server) and the β€œPath” attribute should be set as restrictive as possible to the web application path that makes use of the session ID.

Proposed resolution

If we don't set a cookie domain then browsers will automatically consider the cookie host-only (i.e. the request's host must exactly match the domain of the cookie).

This means on browsers other than IE and Edge we are more secure but on those browsers we are the same as now. The question is whether or not this normalisation of behaviour is desired on not.

View cookie compliance table for major browsers

Remaining tasks

Manual testing

User interface changes

None

API changes

None

Data model changes

None

πŸ“Œ Task
Status

Needs review

Version

10.1 ✨

Component
BaseΒ  β†’

Last updated 1 day ago

Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • Needs manual testing

    The change/bugfix cannot be fully demonstrated by automated testing, and thus requires manual testing in a variety of environments.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Removing change record tag, as we have one of those.

    Hiding patches because now there's an MR.

    Adding cookie compliance table link to the issue summary.

    We still need manual testing across browsers here, added table to track that to the issue summary.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Left a review, but leaving it as needs review so we can get some comments from the security team and some manual testing

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Added manual testing steps to the issue summary and pinged in #security-discussions to try and solicit some help

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    My tests:

    URL for all: https://8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    Set up:

    CHROME
    * SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    FIREFOX
    * SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    SAFARI
    * SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

  • πŸ‡ͺπŸ‡ΈSpain himanshu5050 Spain

    Re-roll for 9.5.x

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Removing credit from #51 as this issue is 10.1.x only

    Crediting @fjgarlin for manual testing,

    Updated remaining tasks - which is testing in Opera and Edge

  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    The remaining tests:

    URL for all: https://8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    EDGE
    * SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    OPERA
    * SESS... Domain value: .8080-shaal-drupalpod-qhjy155me7p.ws-eu87.gitpod.io

    I think this makes all the listed browsers.

  • Status changed to Needs work almost 2 years ago
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review almost 2 years ago
  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    The above error seems to be reported against the patch for 9.5, not the MR. Setting back to "Needs review".
    Note that I tested the remaining browsers as well, but not sure if further tests or review is needed.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I did some stuff to the MR:

    • rebased on to 10.1.x
    • Updated mentions to 9.2.0 to 10.1.0
    • changed str_pos() to str_contains()
  • Status changed to Needs work almost 2 years ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Oh wow MR 173 not even a 4 digit one.

    There are some open threads in the MR from @larowlan that should be looked at.

  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    Updating status of manual tests to add the ones done at #53.

  • First commit to issue fork.
  • Status changed to Needs review almost 2 years ago
  • πŸ‡ΊπŸ‡ΈUnited States dww

    MR had a bunch of merge conflicts since πŸ“Œ Set SameSite on session cookies Fixed and πŸ› SessionConfiguration::getCookieDomain() return value doc is incorrect. Fixed landed. Manually rebased. Attaching the conflicts.

    I'm not sure I resolved the conflicts correctly in core/lib/Drupal/Core/Session/SessionConfiguration.php. Definitely needs another pair of eyes on what I did.

  • Status changed to Needs work almost 2 years ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Think the merge conflicts look good!

    Only moving to NW as there is a comment in the MR

    Should we have a todo for when we plan to phase this out?
    Should we emit a deprecation warning here so sites know they need to update their services.yml?

    That (I can't tell at least) has been answered

  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    Updated IS to show manual testing in the 5 targeted browsers is done (between #50/#53).

  • πŸ‡§πŸ‡ͺBelgium kriboogh

    Maybe this isn't the ticket to mention this, but when

    SessionConfiguration::getOptions is called we do:
    $options['cookie_domain'] = $this->getCookieDomain($request) ?: '';

    A few lines after we do:
    $options['name'] = $this->getName($request);

    => getName calls getUnprefixedName which in turn depends on $this->options['cookie_domain']:
    elseif (isset($this->options['cookie_domain'])) {

    but at this point $this->options doesn't have cookie_domain key, since we are using a locally scoped variable $options in the getOptions call.

Production build 0.71.5 2024