diff --git a/ext/session/session.c b/ext/session/session.c index c04e19e25edc7..387896f35783c 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -733,6 +733,20 @@ static PHP_INI_MH(OnUpdateSessionStr) return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); } +static PHP_INI_MH(OnUpdateSessionSameSite) +{ + SESSION_CHECK_ACTIVE_STATE; + SESSION_CHECK_OUTPUT_STATE; + + if (new_value && ZSTR_LEN(new_value) > 0 && !php_is_valid_samesite_value(new_value)) { + php_error_docref(NULL, E_WARNING, + "session.cookie_samesite must be \"Strict\", \"Lax\", \"None\", or \"\""); + return FAILURE; + } + + return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); +} + static PHP_INI_MH(OnUpdateSessionBool) { SESSION_CHECK_ACTIVE_STATE; @@ -904,7 +918,7 @@ PHP_INI_BEGIN() STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_httponly", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_samesite, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionSameSite, cookie_samesite, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals) diff --git a/ext/session/tests/session_get_cookie_params_basic.phpt b/ext/session/tests/session_get_cookie_params_basic.phpt index 1c7cdf189fc9d..d9541452d882d 100644 --- a/ext/session/tests/session_get_cookie_params_basic.phpt +++ b/ext/session/tests/session_get_cookie_params_basic.phpt @@ -30,7 +30,7 @@ var_dump(session_set_cookie_params([ "domain" => "baz", "secure" => FALSE, "httponly" => FALSE, - "samesite" => "please"])); + "samesite" => "Strict"])); var_dump(session_get_cookie_params()); var_dump(session_set_cookie_params([ "secure" => TRUE, @@ -107,7 +107,7 @@ array(7) { ["httponly"]=> bool(false) ["samesite"]=> - string(6) "please" + string(6) "Strict" } bool(true) array(7) { @@ -124,6 +124,6 @@ array(7) { ["httponly"]=> bool(false) ["samesite"]=> - string(6) "please" + string(6) "Strict" } Done diff --git a/ext/session/tests/session_get_cookie_params_variation1.phpt b/ext/session/tests/session_get_cookie_params_variation1.phpt index 7ce112c9b94dd..0ab0f233530bc 100644 --- a/ext/session/tests/session_get_cookie_params_variation1.phpt +++ b/ext/session/tests/session_get_cookie_params_variation1.phpt @@ -30,7 +30,7 @@ ini_set("session.cookie_secure", TRUE); var_dump(session_get_cookie_params()); ini_set("session.cookie_httponly", TRUE); var_dump(session_get_cookie_params()); -ini_set("session.cookie_samesite", "foo"); +ini_set("session.cookie_samesite", "Lax"); var_dump(session_get_cookie_params()); ini_set("session.cookie_partitioned", TRUE); var_dump(session_get_cookie_params()); @@ -150,7 +150,7 @@ array(7) { ["httponly"]=> bool(true) ["samesite"]=> - string(3) "foo" + string(3) "Lax" } array(7) { ["lifetime"]=> @@ -166,6 +166,6 @@ array(7) { ["httponly"]=> bool(true) ["samesite"]=> - string(3) "foo" + string(3) "Lax" } Done diff --git a/ext/session/tests/session_set_cookie_params_invalid_ini.phpt b/ext/session/tests/session_set_cookie_params_invalid_ini.phpt new file mode 100644 index 0000000000000..61728c342ea2c --- /dev/null +++ b/ext/session/tests/session_set_cookie_params_invalid_ini.phpt @@ -0,0 +1,22 @@ +--TEST-- +Test session.cookie_samesite with invalid INI value +--INI-- +session.cookie_samesite=Invalid +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +Warning: PHP Startup: session.cookie_samesite must be "Strict", "Lax", "None", or "" in Unknown on line 0 +string(0) "" +Done diff --git a/ext/session/tests/session_set_cookie_params_variation6.phpt b/ext/session/tests/session_set_cookie_params_variation6.phpt index 61243f82751e7..fbf958a0be078 100644 --- a/ext/session/tests/session_set_cookie_params_variation6.phpt +++ b/ext/session/tests/session_set_cookie_params_variation6.phpt @@ -1,7 +1,7 @@ --TEST-- -Test session_set_cookie_params() function : variation +Test session_set_cookie_params() samesite validation --INI-- -session.cookie_samesite=test +session.cookie_samesite=Lax --EXTENSIONS-- session --SKIPIF-- @@ -11,36 +11,56 @@ session ob_start(); -echo "*** Testing session_set_cookie_params() : variation ***\n"; - +echo "-- Valid values --\n"; var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "nothing"])); +var_dump(session_set_cookie_params(["samesite" => "Strict"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_start()); +var_dump(session_set_cookie_params(["samesite" => "None"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "test"])); +var_dump(session_set_cookie_params(["samesite" => ""])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_destroy()); + +echo "-- Invalid value via session_set_cookie_params --\n"; +var_dump(session_set_cookie_params(["samesite" => "Invalid"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "other"])); + +echo "-- Invalid value via ini_set --\n"; +var_dump(ini_set("session.cookie_samesite", "Invalid")); var_dump(ini_get("session.cookie_samesite")); +echo "-- Cannot change while session is active --\n"; +var_dump(session_set_cookie_params(["samesite" => "Lax"])); +var_dump(session_start()); +var_dump(session_set_cookie_params(["samesite" => "Strict"])); +var_dump(session_destroy()); + echo "Done"; ob_end_flush(); ?> --EXPECTF-- -*** Testing session_set_cookie_params() : variation *** -string(4) "test" +-- Valid values -- +string(3) "Lax" bool(true) -string(7) "nothing" +string(6) "Strict" bool(true) -string(7) "nothing" +string(4) "None" +bool(true) +string(0) "" +-- Invalid value via session_set_cookie_params -- -Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d +Warning: session_set_cookie_params(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d +bool(false) +string(0) "" +-- Invalid value via ini_set -- + +Warning: ini_set(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d bool(false) -string(7) "nothing" +string(0) "" +-- Cannot change while session is active -- bool(true) -string(7) "nothing" bool(true) -string(5) "other" + +Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d +bool(false) +bool(true) Done diff --git a/ext/session/tests/session_set_cookie_params_variation7.phpt b/ext/session/tests/session_set_cookie_params_variation7.phpt index 430c6efc36e9e..3780fc0222f1b 100644 --- a/ext/session/tests/session_set_cookie_params_variation7.phpt +++ b/ext/session/tests/session_set_cookie_params_variation7.phpt @@ -33,7 +33,7 @@ try { var_dump(ini_get("session.cookie_secure")); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["secure" => true, "samesite" => "please"])); +var_dump(session_set_cookie_params(["secure" => true, "samesite" => "Strict"])); var_dump(ini_get("session.cookie_secure")); var_dump(ini_get("session.cookie_samesite")); @@ -66,7 +66,7 @@ string(1) "0" string(0) "" bool(true) string(1) "1" -string(6) "please" +string(6) "Strict" string(1) "0" bool(true) string(2) "42" diff --git a/ext/standard/head.c b/ext/standard/head.c index 797d8d66c56ae..f5de327b18f04 100644 --- a/ext/standard/head.c +++ b/ext/standard/head.c @@ -76,6 +76,13 @@ PHPAPI bool php_header(void) } } +PHPAPI bool php_is_valid_samesite_value(zend_string *value) +{ + return zend_string_equals_literal_ci(value, "Strict") + || zend_string_equals_literal_ci(value, "Lax") + || zend_string_equals_literal_ci(value, "None"); +} + #define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\014\"" PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, bool secure, bool httponly, @@ -123,7 +130,11 @@ PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t e return FAILURE; } - /* Should check value of SameSite? */ + if (samesite && ZSTR_LEN(samesite) > 0 && !php_is_valid_samesite_value(samesite)) { + zend_value_error("%s(): \"samesite\" option must be \"Strict\", \"Lax\", \"None\", or \"\"", + get_active_function_name()); + return FAILURE; + } if (value == NULL || ZSTR_LEN(value) == 0) { /* diff --git a/ext/standard/head.h b/ext/standard/head.h index 0272fec2dc215..4eec43b1fae8e 100644 --- a/ext/standard/head.h +++ b/ext/standard/head.h @@ -26,6 +26,8 @@ #define COOKIE_SAMESITE "; SameSite=" #define COOKIE_PARTITIONED "; Partitioned" +PHPAPI bool php_is_valid_samesite_value(zend_string *value); + extern PHP_RINIT_FUNCTION(head); PHPAPI bool php_header(void); diff --git a/ext/standard/tests/setcookie_samesite_validation.phpt b/ext/standard/tests/setcookie_samesite_validation.phpt new file mode 100644 index 0000000000000..3827f04fe8d45 --- /dev/null +++ b/ext/standard/tests/setcookie_samesite_validation.phpt @@ -0,0 +1,52 @@ +--TEST-- +setcookie() and setrawcookie() validate samesite option +--FILE-- + 'Strict'])); +var_dump(setcookie('test', 'value', ['samesite' => 'Lax'])); +var_dump(setcookie('test', 'value', ['samesite' => 'None'])); +var_dump(setcookie('test', 'value', ['samesite' => ''])); + +// Case-insensitive +var_dump(setcookie('test', 'value', ['samesite' => 'strict'])); +var_dump(setcookie('test', 'value', ['samesite' => 'LAX'])); +var_dump(setcookie('test', 'value', ['samesite' => 'NONE'])); + +// setrawcookie uses the same validation +var_dump(setrawcookie('test', 'value', ['samesite' => 'Lax'])); + +// Invalid values +try { + setcookie('test', 'value', ['samesite' => 'Invalid']); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +try { + setcookie('test', 'value', ['samesite' => "Strict\r\nX-Injected: evil"]); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +try { + setrawcookie('test', 'value', ['samesite' => 'Invalid']); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +?> +--EXPECTF-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +setcookie(): "samesite" option must be "Strict", "Lax", "None", or "" +setcookie(): "samesite" option must be "Strict", "Lax", "None", or "" +setrawcookie(): "samesite" option must be "Strict", "Lax", "None", or ""