From efed4e4b49874919ad75fd22280b84e57ebcd5b2 Mon Sep 17 00:00:00 2001 From: lachlan Date: Wed, 4 Aug 2021 16:08:53 +1000 Subject: [PATCH] Cookie fixes * ini_set cookie_samesite=Strict on session launch * JS cookies respect cookie_secure * enforce the config defaults for session_length and remember_length --- public/templates/header.inc.php | 5 ++++- public/templates/show_html5_player.inc.php | 11 +++++++---- .../templates/show_html5_player_headers.inc.php | 5 ++++- public/templates/show_login_form.inc.php | 2 +- public/templates/show_lostpassword_form.inc.php | 2 +- public/templates/sidebar.inc.php | 6 +++++- src/Module/Api/Api.php | 2 +- src/Module/Api/Method/PingMethod.php | 2 +- src/Module/Application/Logout/LogoutAction.php | 10 +++++++++- src/Module/System/Core.php | 2 +- src/Module/System/Session.php | 17 ++++++++++------- .../Captcha/easy_captcha_persistent_grant.php | 13 ++++++++++--- src/Repository/Model/Browse.php | 10 +++++++++- 13 files changed, 63 insertions(+), 24 deletions(-) diff --git a/public/templates/header.inc.php b/public/templates/header.inc.php index b4a9da89f0..60d205ea74 100644 --- a/public/templates/header.inc.php +++ b/public/templates/header.inc.php @@ -36,6 +36,9 @@ $htmllang = str_replace("_", "-", AmpConfig::get('lang')); $location = get_location(); $_SESSION['login'] = false; +$cookie_string = (make_bool(AmpConfig::get('cookie_secure'))) + ? "expires: 30, path: '/', secure: true, samesite: 'Strict'" + : "expires: 30, path: '/', samesite: 'Strict'"; // strings for the main page and templates $t_home = T_('Home'); $t_play = T_('Play'); @@ -494,7 +497,7 @@ function libitem_action(item, action) $('#sidebar').show(500); }); - Cookies.set('sidebar_state', newstate, { expires: 30, path: '/', samesite: 'Strict'}); + Cookies.set('sidebar_state', newstate, {}); });
diff --git a/public/templates/show_html5_player.inc.php b/public/templates/show_html5_player.inc.php index fbfe3d6650..0f69525524 100644 --- a/public/templates/show_html5_player.inc.php +++ b/public/templates/show_html5_player.inc.php @@ -10,13 +10,16 @@ // TODO remove me global $dic; -$environment = $dic->get(EnvironmentInterface::class); -$web_path = AmpConfig::get('web_path'); +$environment = $dic->get(EnvironmentInterface::class); +$web_path = AmpConfig::get('web_path'); +$cookie_string = (make_bool(AmpConfig::get('cookie_secure'))) + ? "expires: 7, path: '/', secure: true, samesite: 'Strict'" + : "expires: 7, path: '/', samesite: 'Strict'"; + $autoplay = true; if ($is_share) { $autoplay = ($_REQUEST['autoplay'] === 'true'); } - if (!$iframed) { require_once Ui::find_template('show_html5_player_headers.inc.php'); } @@ -258,7 +261,7 @@ }); $("#jquery_jplayer_1").bind($.jPlayer.event.volumechange, function(event) { - Cookies.set('jp_volume', event.jPlayer.options.volume, { expires: 7, path: '/', samesite: 'Strict'}); + Cookies.set('jp_volume', event.jPlayer.options.volume, {}); }); $("#jquery_jplayer_1").bind($.jPlayer.event.resize, function (event) { diff --git a/public/templates/show_html5_player_headers.inc.php b/public/templates/show_html5_player_headers.inc.php index def7d301ce..9e14c594b4 100644 --- a/public/templates/show_html5_player_headers.inc.php +++ b/public/templates/show_html5_player_headers.inc.php @@ -9,6 +9,9 @@ global $dic; $ajaxUriRetriever = $dic->get(AjaxUriRetrieverInterface::class); $web_path = AmpConfig::get('web_path'); +$cookie_string = (make_bool(AmpConfig::get('cookie_secure'))) + ? "path: '/', secure: true, samesite: 'Strict'" + : "path: '/', samesite: 'Strict'"; if ($iframed || $is_share) { ?> @@ -305,7 +308,7 @@ function ToggleReplayGain() if (replaygainNode != null) { replaygainEnabled = !replaygainEnabled; - Cookies.set('replaygain', replaygainEnabled, {path: '/', samesite: 'Strict'}); + Cookies.set('replaygain', replaygainEnabled, {}); ApplyReplayGain(); if (replaygainEnabled) { diff --git a/public/templates/show_login_form.inc.php b/public/templates/show_login_form.inc.php index db54992958..9bf24446d4 100644 --- a/public/templates/show_login_form.inc.php +++ b/public/templates/show_login_form.inc.php @@ -32,7 +32,7 @@ use Ampache\Module\Util\Ui; $remember_disabled = ''; -if (AmpConfig::get('session_length') >= AmpConfig::get('remember_length')) { +if (AmpConfig::get('session_length', 3600) >= AmpConfig::get('remember_length', 604800)) { $remember_disabled = 'disabled="disabled"'; } $htmllang = str_replace("_", "-", AmpConfig::get('lang')); diff --git a/public/templates/show_lostpassword_form.inc.php b/public/templates/show_lostpassword_form.inc.php index 06762083aa..8dfc6aa22a 100644 --- a/public/templates/show_lostpassword_form.inc.php +++ b/public/templates/show_lostpassword_form.inc.php @@ -29,7 +29,7 @@ use Ampache\Module\System\Core; use Ampache\Module\Util\Ui; -if (AmpConfig::get('session_length') >= AmpConfig::get('remember_length')) { +if (AmpConfig::get('session_length', 3600) >= AmpConfig::get('remember_length', 604800)) { $remember_disabled = 'disabled="disabled"'; } $htmllang = str_replace("_", "-", AmpConfig::get('lang')); diff --git a/public/templates/sidebar.inc.php b/public/templates/sidebar.inc.php index 04d7f6e7fb..349c595a77 100644 --- a/public/templates/sidebar.inc.php +++ b/public/templates/sidebar.inc.php @@ -26,6 +26,10 @@ use Ampache\Module\Api\Ajax; use Ampache\Module\Util\Ui; +$cookie_string = (make_bool(AmpConfig::get('cookie_secure'))) + ? "expires: 30, path: '/', secure: true, samesite: 'Strict'" + : "expires: 30, path: '/', samesite: 'Strict'"; + // strings for the main page and templates $t_home = T_('Home'); $t_browse = T_('Browse'); @@ -134,7 +138,7 @@ if ($header.children(".header-img").hasClass("collapsed")) { sbstate = "collapsed"; } - Cookies.set('sb_' + $header.children(".header-img").attr('id'), sbstate, { expires: 30, path: '/', samesite: 'Strict'}); + Cookies.set('sb_' + $header.children(".header-img").attr('id'), sbstate, {}); }); }); diff --git a/src/Module/Api/Api.php b/src/Module/Api/Api.php index f0694ba328..2fb84570e1 100644 --- a/src/Module/Api/Api.php +++ b/src/Module/Api/Api.php @@ -373,7 +373,7 @@ public static function server_details($token = '') // send the totals $outarray = array('api' => Api::$version, - 'session_expire' => date("c", time() + AmpConfig::get('session_length') - 60), + 'session_expire' => date("c", time() + AmpConfig::get('session_length', 3600) - 60), 'update' => date("c", (int)$details['update']), 'add' => date("c", (int)$details['add']), 'clean' => date("c", (int)$details['clean']), diff --git a/src/Module/Api/Method/PingMethod.php b/src/Module/Api/Method/PingMethod.php index 741eec7108..36b514c6cb 100644 --- a/src/Module/Api/Method/PingMethod.php +++ b/src/Module/Api/Method/PingMethod.php @@ -60,7 +60,7 @@ public static function ping(array $input) // Check and see if we should extend the api sessions (done if valid session is passed) if (Session::exists('api', $input['auth'])) { Session::extend($input['auth']); - $xmldata = array_merge(array('session_expire' => date("c", time() + (int) AmpConfig::get('session_length') - 60)), $xmldata, Api::server_details($input['auth'])); + $xmldata = array_merge(array('session_expire' => date("c", time() + (int) AmpConfig::get('session_length', 3600) - 60)), $xmldata, Api::server_details($input['auth'])); } debug_event(self::class, 'Ping Received from ' . Core::get_server('REMOTE_ADDR'), 5); diff --git a/src/Module/Application/Logout/LogoutAction.php b/src/Module/Application/Logout/LogoutAction.php index fafb2c2b0e..ecf1001d1a 100644 --- a/src/Module/Application/Logout/LogoutAction.php +++ b/src/Module/Application/Logout/LogoutAction.php @@ -22,6 +22,7 @@ namespace Ampache\Module\Application\Logout; +use Ampache\Config\AmpConfig; use Ampache\Config\ConfigContainerInterface; use Ampache\Module\Application\ApplicationActionInterface; use Ampache\Module\Authorization\GuiGatekeeperInterface; @@ -47,8 +48,15 @@ public function __construct( public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gatekeeper): ?ResponseInterface { + $cookie_options = [ + 'expires' => -1, + 'path' => AmpConfig::get('cookie_path'), + 'domain' => AmpConfig::get('cookie_domain'), + 'secure' => make_bool(AmpConfig::get('cookie_secure')), + 'samesite' => 'Strict' + ]; // To end a legitimate session, just call logout. - setcookie($this->configContainer->getSessionName() . '_remember', null, ['expires' => -1, 'samesite' => 'Strict']); + setcookie($this->configContainer->getSessionName() . '_remember', null, $cookie_options); $this->authenticationManager->logout('', false); diff --git a/src/Module/System/Core.php b/src/Module/System/Core.php index 042904fe09..bab41de71f 100644 --- a/src/Module/System/Core.php +++ b/src/Module/System/Core.php @@ -196,7 +196,7 @@ public static function form_register($name, $type = 'post') { // Make ourselves a nice little sid $sid = md5(uniqid((string)rand(), true)); - $window = AmpConfig::get('session_length'); + $window = AmpConfig::get('session_length', 3600); $expire = time() + $window; // Register it diff --git a/src/Module/System/Session.php b/src/Module/System/Session.php index 8607396feb..8d9b7e9276 100644 --- a/src/Module/System/Session.php +++ b/src/Module/System/Session.php @@ -151,7 +151,7 @@ public static function write($key, $value) return true; } - $expire = time() + AmpConfig::get('session_length'); + $expire = time() + AmpConfig::get('session_length', 3600); $sql = 'UPDATE `session` SET `value` = ?, `expire` = ? WHERE `id` = ?'; Dba::write($sql, array($value, $expire, $key)); @@ -315,7 +315,7 @@ public static function create($data) } $agent = (!empty($data['agent'])) ? $data['agent'] : substr(Core::get_server('HTTP_USER_AGENT'), 0, 254); - $expire = time() + AmpConfig::get('session_length'); + $expire = time() + AmpConfig::get('session_length', 3600); if ($type == 'stream') { $expire = time() + AmpConfig::get('stream_length'); } @@ -455,7 +455,7 @@ public static function extend($sid, $type = null) if ($type == 'stream') { $expire = $time + AmpConfig::get('stream_length'); } else { - $expire = $time + AmpConfig::get('session_length'); + $expire = $time + AmpConfig::get('session_length', 3600); } $sql = 'UPDATE `session` SET `expire` = ? WHERE `id`= ?'; @@ -522,6 +522,9 @@ public static function get_geolocation($sid) public function setup(): void { + // enforce strict cookies. you don't need these elsewhere + ini_set('session.cookie_samesite', 'Strict'); + session_set_save_handler( static function (): bool { return true; @@ -598,9 +601,9 @@ public static function create_user_cookie($username) { $session_name = AmpConfig::get('session_name'); $cookie_options = [ - 'expires' => AmpConfig::get('cookie_life'), - 'path' => AmpConfig::get('cookie_path'), - 'domain' => AmpConfig::get('cookie_domain'), + 'expires' => (int)AmpConfig::get('cookie_life'), + 'path' => (string)AmpConfig::get('cookie_path'), + 'domain' => (string)AmpConfig::get('cookie_domain'), 'secure' => make_bool(AmpConfig::get('cookie_secure')), 'samesite' => 'Strict' ]; @@ -618,7 +621,7 @@ public static function create_user_cookie($username) public static function create_remember_cookie($username) { $session_name = AmpConfig::get('session_name'); - $remember_length = time() + AmpConfig::get('remember_length'); + $remember_length = time() + AmpConfig::get('remember_length', 604800); $cookie_options = [ 'expires' => $remember_length, 'path' => AmpConfig::get('cookie_path'), diff --git a/src/Module/Util/Captcha/easy_captcha_persistent_grant.php b/src/Module/Util/Captcha/easy_captcha_persistent_grant.php index f0032a4369..5c14fc5c04 100644 --- a/src/Module/Util/Captcha/easy_captcha_persistent_grant.php +++ b/src/Module/Util/Captcha/easy_captcha_persistent_grant.php @@ -24,6 +24,8 @@ namespace Ampache\Module\Util\Captcha; +use Ampache\Config\AmpConfig; + /** * Class easy_captcha_persistent_grant * shortcut, allow access for an user if captcha was previously solved @@ -52,9 +54,14 @@ public function solved($input = 0) public function grant() { if (!headers_sent()) { - setcookie($this->cookie(), $this->validity_token(), ['expires' => time() + 175 * CAPTCHA_TIMEOUT, 'samesite' => 'Strict']); - //} else { - // // $this->log("::grant", "COOKIES", "too late for cookies"); + $cookie_options = [ + 'expires' => time() + 175 * CAPTCHA_TIMEOUT, + 'path' => AmpConfig::get('cookie_path'), + 'domain' => AmpConfig::get('cookie_domain'), + 'secure' => make_bool(AmpConfig::get('cookie_secure')), + 'samesite' => 'Strict' + ]; + setcookie($this->cookie(), $this->validity_token(), $cookie_options); } } diff --git a/src/Repository/Model/Browse.php b/src/Repository/Model/Browse.php index 35fb5427d9..5ef9f42f0d 100644 --- a/src/Repository/Model/Browse.php +++ b/src/Repository/Model/Browse.php @@ -463,7 +463,15 @@ public function set_type($type, $custom_base = '') public function save_cookie_params($option, $value) { if ($this->get_type()) { - setcookie('browse_' . $this->get_type() . '_' . $option, $value, ['expires' => time() + 31536000, 'path' => "/", 'samesite' => 'Strict']); + $remember_length = time() + 31536000; + $cookie_options = [ + 'expires' => $remember_length, + 'path' => AmpConfig::get('cookie_path'), + 'domain' => AmpConfig::get('cookie_domain'), + 'secure' => make_bool(AmpConfig::get('cookie_secure')), + 'samesite' => 'Strict' + ]; + setcookie('browse_' . $this->get_type() . '_' . $option, $value, $cookie_options); } }