craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.26k stars 634 forks source link

[3.x]: PHPSESSID cookie is always set and it ignores the cookie config settings in general config #12216

Closed boboldehampsink closed 1 year ago

boboldehampsink commented 1 year ago

What happened?

Description

While running a security headers check on the website of a client of mine, I found a warning in Set-Cookie: https://securityheaders.com/?q=dwproperty.com&followRedirects=on

It seems that PHPSESSID is always set without the secure flag. While digging into the Craft source it seems to me that it should be CraftSessionId and it should respect the general config cookie settings? (I have useSecureCookies set to true)

Is this some bug?

Steps to reproduce

  1. Run Craft 3.7.59

Expected behavior

CraftSessionId should be set and it should respect config settings

Actual behavior

No CraftSessionId is set, but a PHPSESSID, and it doesn't respect config settings

Craft CMS version

3.7.59

PHP version

8.1.11

Operating system and version

Ubuntu 22

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

-

i-just commented 1 year ago

Hi, thanks for reaching out. Are you by any chance specifying your own phpSessionName in the config/general.php or are you doing anything session related in config/app.php?

boboldehampsink commented 1 year ago

Oops, did something wrong in config/app.php! Thanks for pointing me to that.

i-just commented 1 year ago

No problem! Glad you got to the bottom of it.

joepagan commented 10 months ago

Just had this where both phpSessionName and sameSiteCookieValue values were not being honoured.

Also because of the config/app.php! Specifically seemed to be the session component doesn't appear to be working with redis on ddev 🤷.

Frustrating that specifically found this while testing a checkout 3ds callback where the session cookie required a "secure" value, where the application was on SSL (with ddev) but the secure value was false (which craft should not do by default). Would be great if Craft threw an exception at least when the config is not being honoured.

brandonkelly commented 10 months ago

@joepagan What exactly what you doing in config/app.php?

joepagan commented 10 months ago

@brandonkelly here is my full config/app.php for you, craft version: 3.7.46:

<?php
use craft\helpers\App;
use Monolog\Handler\StreamHandler;
use Monolog\Logger;

$env = App::parseEnv('$ENVIRONMENT');
$siteName = App::parseEnv('$SITE_NAME');
$redisServer = App::parseEnv('$REDIS_SERVER');
$redisPort = App::parseEnv('$REDIS_PORT');
$devMode = $env === 'dev';

$logger = (new Logger('app'))
    ->pushHandler(new StreamHandler('php://stdout', \Monolog\Logger::DEBUG))
    ->pushHandler(new StreamHandler('php://stderr', \Monolog\Logger::WARNING, false));

$targets = [
    [
        'class' => \samdark\log\PsrTarget::class,
        'except' => [
            'yii\web\HttpException:40*',
        ],
        'levels' => ['error', 'warning'],
        'logVars' => [],
        'logger' => $logger,
        'addTimestampToContext' => true,
    ]
];

if ($devMode) {
    $categories = [
        'application',
        'modules*',
    ];
    foreach ($categories as $category) {
        array_push($targets[0]['except'], $category);
    }
    $targets[] = [
        'class' => \samdark\log\PsrTarget::class,
        'categories' => $categories,
        'except' => [
            'modules\plytixmodule\PlytixModule::init',
            'modules\commerceapi\CommerceApi::init',
            'modules\stockistmodule\StockistModule::init',
            'modules\discontinuedproducts\DiscontinuedProducts::init',
        ],
        'logVars' => [],
        'logger' => $logger,
        'addTimestampToContext' => true,
    ];
}

return [
    'id' => App::env('APP_ID') ?: 'CraftCMS',
    'modules' => [
        'plytix-module' => [
            'class' => \modules\plytixmodule\PlytixModule::class,
        ],
        'commerce-api' => [
            'class' => \modules\commerceapi\CommerceApi::class,
        ],
        'discontinued-products' => [
            'class' => \modules\discontinuedproducts\DiscontinuedProducts::class,
        ],
        'stockist-module' => [
            'class' => \modules\stockistmodule\StockistModule::class,
        ],
    ],
    'bootstrap' => [
        'plytix-module',
        'commerce-api',
        'discontinued-products',
        'stockist-module',
    ],
    'components' => [
        'cache' => [
            'class' => yii\redis\Cache::class,
            'defaultDuration' => App::parseEnv('$REDIS_CACHE_DURATION'),
            'keyPrefix' => preg_replace('/[^A-z0-9\-]*/', '', ($siteName . '_' . $env . '_cache_')),
            'redis' => [
                'class' => yii\redis\Connection::class,
                'hostname' => $redisServer,
                'port' => $redisPort,
                'database' => 0,
            ],
        ],
        'session' => [
            'class' => yii\redis\Session::class,
            'as session' => craft\behaviors\SessionBehavior::class,
            'keyPrefix' => preg_replace('/[^A-z0-9\-]*/', '', ($siteName . '_' . $env . '_session_')),
            'redis' => [
                'class' => yii\redis\Connection::class,
                'hostname' => $redisServer,
                'port' => $redisPort,
                'database' => 1,
            ],
        ],
        'mutex' => [
            'class' => \yii\redis\Mutex::class,
            'keyPrefix' => preg_replace('/[^A-z0-9\-]*/', '', ($siteName . '_' . $env . '_mutex_')),
            'redis' => [
                'class' => yii\redis\Connection::class,
                'hostname' => $redisServer,
                'port' => $redisPort,
                'database' => 2,
            ],
        ],
        'log' => [
            'targets' => $targets,
        ],
        'queue' => [
            'ttr' => 120 * 60,
        ],
    ],
];
brandonkelly commented 10 months ago

That session config looks fine to me.

Frustrating that specifically found this while testing a checkout 3ds callback where the session cookie required a "secure" value, where the application was on SSL (with ddev) but the secure value was false (which craft should not do by default). Would be great if Craft threw an exception at least when the config is not being honoured.

By default cookies will get the secure flag if they’re created on a request with SSL. Doesn’t have anything to do with the session component.

Maybe you accidentally accessed the site over http at some point?

You can force all cookies to get the secure flag via the useSecureCookies config setting in config/general.php.

joepagan commented 10 months ago

@brandonkelly

By default cookies will get the secure flag if they’re created on a request with SSL. Doesn’t have anything to do with the session component.

I wonder if it's something to do with the app not being able to connect to the ddev redis database on redis:6379. It literally works if I comment it out, delete cookies and refresh a page!

Maybe you accidentally accessed the site over http at some point?

I don't think so, been using a reliable https ddev setup for a while now and never use http.

You can force all cookies to get the secure flag via the useSecureCookies config setting in config/general.php.

It doesn't work if the session component is used! I had this already, was ignored!

Here is my in browser cookies before and after commenting out the session component, refreshing the same URL after changes: https://github.com/craftcms/cms/assets/2337910/d211bb24-298d-4830-932e-758563d6c3e1