Skip to content

NEW Allow setting "samesite" attribute for individual cookies. #11632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
emteknetnz merged 1 commit into from
Mar 13, 2025
Merged

NEW Allow setting "samesite" attribute for individual cookies. #11632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 37 additions & 45 deletions src/Control/Cookie.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,16 @@ class Cookie
{
use Configurable;

public const SAMESITE_LAX = 'Lax';

public const SAMESITE_STRICT = 'Strict';

public const SAMESITE_LAX = 'Lax';

public const SAMESITE_NONE = 'None';
Comment on lines 17 to 21
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the order so it's most to least secure


/**
* @config
*
* @var bool
*/
private static $report_errors = true;
private static bool $report_errors = true;
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fair amount of unrelated strict typing being added in this PR


/**
* Must be "Strict", "Lax", or "None"
* @config
*/
private static string $default_samesite = Cookie::SAMESITE_LAX;

Expand All @@ -38,73 +32,71 @@ class Cookie
*
* @return Cookie_Backend
*/
public static function get_inst()
public static function get_inst(): Cookie_Backend
{
return Injector::inst()->get('SilverStripe\\Control\\Cookie_Backend');
return Injector::inst()->get(Cookie_Backend::class);
}

/**
* Set a cookie variable.
*
* Expiry time is set in days, and defaults to 90.
*
* @param string $name
* @param mixed $value
* @param float $expiry
* @param string $path
* @param string $domain
* @param bool $secure
* @param bool $httpOnly
*
* See http://php.net/set_session
*/
public static function set(
$name,
$value,
$expiry = 90,
$path = null,
$domain = null,
$secure = false,
$httpOnly = true
string $name,
string|false $value,
int $expiry = 90,
?string $path = null,
?string $domain = null,
bool $secure = false,
bool $httpOnly = true,
string $sameSite = ''
Comment on lines +48 to +55
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • $value can only be a string or false if you trace through the calls in the actual implementation.
  • $expiry can only be int.
  • Chose to keep the nullability of $path and $domain to reduce the change of unexpected side effects and regressions given we're so close to the beta.

Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types are shared with CookieJar and Cookie_Backend as well.

) {
return Cookie::get_inst()->set($name, $value, $expiry, $path, $domain, $secure, $httpOnly);
if ($sameSite === '') {
$sameSite = static::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;
}
static::validateSameSite($sameSite);
return Cookie::get_inst()->set($name, $value, $expiry, $path, $domain, $secure, $httpOnly, $sameSite);
}

/**
* Get the cookie value by name. Returns null if not set.
*
* @param string $name
* @param bool $includeUnsent
*
* @return null|string
*/
public static function get($name, $includeUnsent = true)
public static function get(string $name, bool $includeUnsent = true): ?string
{
return Cookie::get_inst()->get($name, $includeUnsent);
}

/**
* Get all the cookies.
*
* @param bool $includeUnsent
*
* @return array
*/
public static function get_all($includeUnsent = true)
public static function get_all(bool $includeUnsent = true): array
{
return Cookie::get_inst()->getAll($includeUnsent);
}

/**
* @param string $name
* @param null|string $path
* @param null|string $domain
* @param bool $secure
* @param bool $httpOnly
* Force the expiry of a cookie by name
*/
public static function force_expiry(
string $name,
?string $path = null,
?string $domain = null,
bool $secure = false,
bool $httpOnly = true,
string $sameSite = ''
): void {
Cookie::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly, $sameSite);
}

/**
* Get the default value for the "samesite" cookie attribute.
*/
public static function force_expiry($name, $path = null, $domain = null, $secure = false, $httpOnly = true)
public static function getDefaultSameSite(): string
{
return Cookie::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly);
return static::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;
}

/**
Expand Down
131 changes: 59 additions & 72 deletions src/Control/CookieJar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,52 +20,52 @@ class CookieJar implements Cookie_Backend
*
* @var array Existing cookies sent by the browser
*/
protected $existing = [];
protected array $existing = [];

/**
* Hold the current cookies (ie: a mix of those that were sent to us and we
* have set without the ones we've cleared)
*
* @var array The state of cookies once we've sent the response
*/
protected $current = [];
protected array $current = [];

/**
* Hold any NEW cookies that were set by the application and will be sent
* in the next response
*
* @var array New cookies set by the application
*/
protected $new = [];
protected array $new = [];

/**
* When creating the backend we want to store the existing cookies in our
* "existing" array. This allows us to distinguish between cookies we received
* or we set ourselves (and didn't get from the browser)
*
* @param array $cookies The existing cookies to load into the cookie jar.
* Omit this to default to $_COOKIE
* @inheritDoc
Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed a lot of duplicate PHPDoc from this class

*/
public function __construct($cookies = [])
public function __construct(array $cookies = [])
{
$this->current = $this->existing = func_num_args()
? ($cookies ?: []) // Convert empty values to blank arrays
: $_COOKIE;
}

/**
* Set a cookie
*
* @param string $name The name of the cookie
* @param string $value The value for the cookie to hold
* @param float $expiry The number of days until expiry; 0 indicates a cookie valid for the current session
* @param string $path The path to save the cookie on (falls back to site base)
* @param string $domain The domain to make the cookie available on
* @param boolean $secure Can the cookie only be sent over SSL?
* @param boolean $httpOnly Prevent the cookie being accessible by JS
Copy link
Copy Markdown
Member

@emteknetnz emteknetnz Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be deleting docblock params when they're explaining something - e.g. $expiry and $path are non obvious.

This applies other instances of deleting docblock params in this PR

Copy link
Copy Markdown
Member Author

@GuySartorelli GuySartorelli Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the @inheritdoc immediately below the text you selected. These are duplicate PHPDocs as mentioned in #11632 (comment)

* @inheritDoc
*/
public function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = true)
{
public function set(
string $name,
string|false $value,
int $expiry = 90,
?string $path = null,
?string $domain = null,
bool $secure = false,
bool $httpOnly = true,
string $sameSite = ''
): void {
if ($sameSite === '') {
$sameSite = Cookie::getDefaultSameSite();
}
Cookie::validateSameSite($sameSite);

//are we setting or clearing a cookie? false values are reserved for clearing cookies (see PHP manual)
$clear = false;
if ($value === false || $value === '' || $expiry < 0) {
Expand All @@ -81,7 +81,7 @@ public function set($name, $value, $expiry = 90, $path = null, $domain = null, $
//set the path up
$path = $path ? $path : Director::baseURL();
//send the cookie
$this->outputCookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly);
$this->outputCookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly, $sameSite);
//keep our variables in check
if ($clear) {
unset($this->new[$name], $this->current[$name]);
Expand All @@ -94,13 +94,9 @@ public function set($name, $value, $expiry = 90, $path = null, $domain = null, $
* Get the cookie value by name
*
* Cookie names are normalised to work around PHP's behaviour of replacing incoming variable name . with _
*
* @param string $name The name of the cookie to get
* @param boolean $includeUnsent Include cookies we've yet to send when fetching values
*
* @return string|null The cookie value or null if unset
* @inheritDoc
*/
public function get($name, $includeUnsent = true)
public function get(string $name, bool $includeUnsent = true): ?string
{
$cookies = $includeUnsent ? $this->current : $this->existing;
if (isset($cookies[$name])) {
Expand All @@ -116,28 +112,25 @@ public function get($name, $includeUnsent = true)
}

/**
* Get all the cookies
*
* @param boolean $includeUnsent Include cookies we've yet to send
* @return array All the cookies
* @inheritDoc
*/
public function getAll($includeUnsent = true)
public function getAll(bool $includeUnsent = true): array
{
return $includeUnsent ? $this->current : $this->existing;
}

/**
* Force the expiry of a cookie by name
*
* @param string $name The name of the cookie to expire
* @param string $path The path to save the cookie on (falls back to site base)
* @param string $domain The domain to make the cookie available on
* @param boolean $secure Can the cookie only be sent over SSL?
* @param boolean $httpOnly Prevent the cookie being accessible by JS
* @inheritDoc
*/
public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = true)
{
$this->set($name, false, -1, $path, $domain, $secure, $httpOnly);
public function forceExpiry(
string $name,
?string $path = null,
?string $domain = null,
bool $secure = false,
bool $httpOnly = true,
string $sameSite = ''
): void {
$this->set($name, false, -1, $path, $domain, $secure, $httpOnly, $sameSite);
}

/**
Expand All @@ -146,33 +139,38 @@ public function forceExpiry($name, $path = null, $domain = null, $secure = false
* @see http://uk3.php.net/manual/en/function.setcookie.php
*
* @param string $name The name of the cookie
* @param string|array $value The value for the cookie to hold
* @param string|false $value The value for the cookie to hold. Empty string or false will clear the cookie.
* @param int $expiry A Unix timestamp indicating when the cookie expires; 0 means it will expire at the end of the session
* @param string $path The path to save the cookie on (falls back to site base)
* @param string $domain The domain to make the cookie available on
* @param boolean $secure Can the cookie only be sent over SSL?
* @param boolean $httpOnly Prevent the cookie being accessible by JS
* @return boolean If the cookie was set or not; doesn't mean it's accepted by the browser
* @param ?string $path The path to save the cookie on (falls back to site base)
* @param ?string $domain The domain to make the cookie available on
* @param bool $secure Can the cookie only be sent over SSL?
* @param bool $httpOnly Prevent the cookie being accessible by JS
* @param string $sameSite The "SameSite" value for the cookie. Must be one of "None", "Lax", or "Strict".
* If $sameSite is left empty, the default will be used.
* @return bool If the cookie was set or not; doesn't mean it's accepted by the browser
*/
protected function outputCookie(
$name,
$value,
$expiry = 90,
$path = null,
$domain = null,
$secure = false,
$httpOnly = true
) {
$sameSite = $this->getSameSite($name);
string $name,
string|false $value,
int $expiry = 90,
?string $path = null,
?string $domain = null,
bool $secure = false,
bool $httpOnly = true,
string $sameSite = ''
): bool {
if ($sameSite === '') {
$sameSite = Cookie::getDefaultSameSite();
}
Cookie::validateSameSite($sameSite);
// if headers aren't sent, we can set the cookie
if (!headers_sent($file, $line)) {
return setcookie($name ?? '', $value ?? '', [
'expires' => $expiry ?? 0,
return setcookie($name, $value, [
'expires' => $expiry,
'path' => $path ?? '',
'domain' => $domain ?? '',
'secure' => $this->cookieIsSecure($sameSite, (bool) $secure),
'httponly' => $httpOnly ?? false,
'httponly' => $httpOnly,
'samesite' => $sameSite,
]);
}
Expand All @@ -192,15 +190,4 @@ private function cookieIsSecure(string $sameSite, bool $secure): bool
{
return $sameSite === 'None' ? true : $secure;
}

/**
* Get the correct samesite value - Session cookies use a different configuration variable.
*/
private function getSameSite(string $name): string
{
if ($name === session_name()) {
return Session::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
}
return Cookie::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;
}
}
Loading
Loading