-
Notifications
You must be signed in to change notification settings - Fork 7.9k
implement same site cookie #2613
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -799,6 +799,7 @@ PHP_INI_BEGIN() | |
STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionString, cookie_domain, php_ps_globals, ps_globals) | ||
STD_PHP_INI_ENTRY("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals) | ||
STD_PHP_INI_ENTRY("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, OnUpdateString, cookie_samesite, php_ps_globals, ps_globals) | ||
STD_PHP_INI_ENTRY("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals) | ||
STD_PHP_INI_ENTRY("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_only_cookies, php_ps_globals, ps_globals) | ||
STD_PHP_INI_ENTRY("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals) | ||
|
@@ -1357,6 +1358,11 @@ static int php_session_send_cookie(void) /* {{{ */ | |
smart_str_appends(&ncookie, COOKIE_HTTPONLY); | ||
} | ||
|
||
if (PS(cookie_samesite)[0]) { | ||
smart_str_appends(&ncookie, COOKIE_SAMESITE); | ||
smart_str_appends(&ncookie, PS(cookie_samesite)); | ||
} | ||
|
||
smart_str_0(&ncookie); | ||
|
||
php_session_remove_cookie(); /* remove already sent session ID cookie */ | ||
|
@@ -1655,18 +1661,18 @@ PHPAPI void session_adapt_url(const char *url, size_t urllen, char **new, size_t | |
* Userspace exported functions * | ||
******************************** */ | ||
|
||
/* {{{ proto bool session_set_cookie_params(int lifetime [, string path [, string domain [, bool secure[, bool httponly]]]]) | ||
/* {{{ proto void session_set_cookie_params(int lifetime [, string path [, string domain [, bool secure[, bool httponly[, string samesite]]]]]) | ||
Set session cookie parameters */ | ||
static PHP_FUNCTION(session_set_cookie_params) | ||
{ | ||
zval *lifetime; | ||
zend_string *path = NULL, *domain = NULL; | ||
zend_string *path = NULL, *domain = NULL, *samesite = NULL; | ||
int argc = ZEND_NUM_ARGS(); | ||
zend_bool secure = 0, httponly = 0; | ||
zend_string *ini_name; | ||
|
||
if (!PS(use_cookies) || | ||
zend_parse_parameters(argc, "z|SSbb", &lifetime, &path, &domain, &secure, &httponly) == FAILURE) { | ||
zend_parse_parameters(argc, "z|SSbbS", &lifetime, &path, &domain, &secure, &httponly, &samesite) == FAILURE) { | ||
return; | ||
} | ||
|
||
|
@@ -1724,6 +1730,12 @@ static PHP_FUNCTION(session_set_cookie_params) | |
zend_string_release(ini_name); | ||
} | ||
|
||
if (argc > 5) { | ||
ini_name = zend_string_init("session.cookie_samesite", sizeof("session.cookie_samesite") - 1, 0); | ||
zend_alter_ini_entry(ini_name, samesite, PHP_INI_USER, PHP_INI_STAGE_RUNTIME); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact... ugh. Are these parameters really all just wrappers for ini_set()? Let's stop that madness and just let people use ini_set(). This is a silly function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too bad that we decided on 2017-10-10 to alternatively allow to pass an option array to this function. Anyhow, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cmb69 feel free to add anything you please to the RFC, its an open WIP |
||
zend_string_release(ini_name); | ||
} | ||
|
||
RETURN_TRUE; | ||
} | ||
/* }}} */ | ||
|
@@ -1743,6 +1755,7 @@ static PHP_FUNCTION(session_get_cookie_params) | |
add_assoc_string(return_value, "domain", PS(cookie_domain)); | ||
add_assoc_bool(return_value, "secure", PS(cookie_secure)); | ||
add_assoc_bool(return_value, "httponly", PS(cookie_httponly)); | ||
add_assoc_string(return_value, "samesite", PS(cookie_samesite)); | ||
} | ||
/* }}} */ | ||
|
||
|
@@ -2621,6 +2634,7 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_session_set_cookie_params, 0, 0, 1) | |
ZEND_ARG_INFO(0, domain) | ||
ZEND_ARG_INFO(0, secure) | ||
ZEND_ARG_INFO(0, httponly) | ||
ZEND_ARG_INFO(0, samesite) | ||
ZEND_END_ARG_INFO() | ||
|
||
ZEND_BEGIN_ARG_INFO(arginfo_session_class_open, 0) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
--TEST-- | ||
Test session_set_cookie_params() function : variation | ||
--INI-- | ||
session.cookie_samesite=test | ||
--SKIPIF-- | ||
<?php include('skipif.inc'); ?> | ||
--FILE-- | ||
<?php | ||
|
||
ob_start(); | ||
|
||
/* | ||
* Prototype : void session_set_cookie_params(int $lifetime [, string $path [, string $domain [, bool $secure [, bool $samesite[, string $samesite]]]]]) | ||
* Description : Set the session cookie parameters | ||
* Source code : ext/session/session.c | ||
*/ | ||
|
||
echo "*** Testing session_set_cookie_params() : variation ***\n"; | ||
|
||
var_dump(ini_get("session.cookie_samesite")); | ||
var_dump(session_set_cookie_params(3600, "/path", "blah", FALSE, FALSE, "nothing")); | ||
var_dump(ini_get("session.cookie_samesite")); | ||
var_dump(session_start()); | ||
var_dump(ini_get("session.cookie_samesite")); | ||
var_dump(session_set_cookie_params(3600, "/path", "blah", FALSE, TRUE, "test")); | ||
var_dump(ini_get("session.cookie_samesite")); | ||
var_dump(session_destroy()); | ||
var_dump(ini_get("session.cookie_samesite")); | ||
var_dump(session_set_cookie_params(3600, "/path", "blah", FALSE, FALSE, "other")); | ||
var_dump(ini_get("session.cookie_samesite")); | ||
|
||
echo "Done"; | ||
ob_end_flush(); | ||
?> | ||
--EXPECTF-- | ||
*** Testing session_set_cookie_params() : variation *** | ||
string(4) "test" | ||
bool(true) | ||
string(7) "nothing" | ||
bool(true) | ||
string(7) "nothing" | ||
|
||
Warning: session_set_cookie_params(): Cannot change session cookie parameters when session is active in %s on line 18 | ||
bool(false) | ||
string(7) "nothing" | ||
bool(true) | ||
string(7) "nothing" | ||
bool(true) | ||
string(5) "other" | ||
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (samesite) {
Testing the var is more readable and less likely to get screwed up because of some silly off-by-one error later on.