Skip to content

Commit

Permalink
FIX stabilise typed APIs (#10740)
Browse files Browse the repository at this point in the history
Since 4.12 the use of typehints and return types has caused issues with
values fetched directly from config without validation. This has lead to
upgrade woes in a minor version (#10721) with no immediate recourse
other than manual system intervention.

To use types, we should ensure types, leaving a stable API that won't
error on a bad value - or should give a thoughtful and directive error
message if so.

Issue #10721 summary:
SessionMiddleware runs before FlushMiddleware
SessionMiddleware causes a PHP fatal error passing `null` to a `string`
parameter.
`null` comes from config, because default string value doesn't exist. We
need flush for this - but system execution never makes it that far.
  • Loading branch information
NightJar committed Apr 10, 2023
1 parent 403f924 commit 92061a3
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 9 deletions.
16 changes: 11 additions & 5 deletions src/Control/Cookie.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ class Cookie
{
use Configurable;

public const SAMESITE_LAX = 'Lax';

public const SAMESITE_STRICT = 'Strict';

public const SAMESITE_NONE = 'None';

/**
* @config
*
Expand All @@ -25,7 +31,7 @@ class Cookie
* Must be "Strict", "Lax", or "None"
* @config
*/
private static string $default_samesite = 'Lax';
private static string $default_samesite = self::SAMESITE_LAX;

/**
* Fetch the current instance of the cookie backend.
Expand Down Expand Up @@ -110,14 +116,14 @@ public static function force_expiry($name, $path = null, $domain = null, $secure
public static function validateSameSite(string $sameSite): void
{
$validValues = [
'Strict',
'Lax',
'None',
self::SAMESITE_STRICT,
self::SAMESITE_LAX,
self::SAMESITE_NONE,
];
if (!in_array($sameSite, $validValues)) {
throw new LogicException('Cookie samesite must be "Strict", "Lax", or "None"');
}
if ($sameSite === 'None' && !Director::is_https(self::getRequest())) {
if ($sameSite === self::SAMESITE_NONE && !Director::is_https(self::getRequest())) {
Injector::inst()->get(LoggerInterface::class)->warning('Cookie samesite cannot be "None" for non-https requests.');
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Control/CookieJar.php
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ private function getSameSite(string $name): string
{
Deprecation::notice('4.12.0', 'The relevant methods will include a `$sameSite` parameter instead.');
if ($name === session_name()) {
return Session::config()->get('cookie_samesite');
return Session::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
}
return Cookie::config()->get('default_samesite');
return Cookie::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;
}
}
4 changes: 2 additions & 2 deletions src/Control/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class Session
* Must be "Strict", "Lax", or "None".
* @config
*/
private static string $cookie_samesite = 'Lax';
private static string $cookie_samesite = Cookie::SAMESITE_LAX;

/**
* Name of session cache limiter to use.
Expand Down Expand Up @@ -374,7 +374,7 @@ private function buildCookieParams(HTTPRequest $request): array
}
}

$sameSite = static::config()->get('cookie_samesite');
$sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
Cookie::validateSameSite($sameSite);
$secure = $this->isCookieSecure($sameSite, Director::is_https($request));

Expand Down

0 comments on commit 92061a3

Please sign in to comment.