Skip to content

Commit

Permalink
Cookie fixes
Browse files Browse the repository at this point in the history 
* ini_set cookie_samesite=Strict on session launch
* JS cookies respect cookie_secure
* enforce the config defaults for session_length and remember_length
  • Loading branch information
@lachlan-00
lachlan-00 committed Aug 4, 2021
1 parent 01acc3c commit efed4e4
Show file tree
Hide file tree
Showing 13 changed files with 63 additions and 24 deletions.
5 changes: 4 additions & 1 deletion public/templates/header.inc.php
View file
Expand Up @@ -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');
Expand Down Expand Up @@ -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, {<?php echo $cookie_string ?>});
});
</script>
<div id="rightbar" class="rightbar-fixed">
Expand Down
11 changes: 7 additions & 4 deletions public/templates/show_html5_player.inc.php
View file
Expand Up @@ -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');
}
Expand Down Expand Up @@ -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, {<?php echo $cookie_string ?>});
});

$("#jquery_jplayer_1").bind($.jPlayer.event.resize, function (event) {
Expand Down
5 changes: 4 additions & 1 deletion public/templates/show_html5_player_headers.inc.php
View file
Expand Up @@ -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) { ?>
<link rel="stylesheet" href="<?php echo $web_path . Ui::find_template('jplayer.midnight.black-iframed.css', true) ?>" type="text/css" />
Expand Down Expand Up @@ -305,7 +308,7 @@ function ToggleReplayGain()

if (replaygainNode != null) {
replaygainEnabled = !replaygainEnabled;
Cookies.set('replaygain', replaygainEnabled, {path: '/', samesite: 'Strict'});
Cookies.set('replaygain', replaygainEnabled, {<?php echo $cookie_string ?>});
ApplyReplayGain();

if (replaygainEnabled) {
Expand Down
2 changes: 1 addition & 1 deletion public/templates/show_login_form.inc.php
View file
Expand Up @@ -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'));
Expand Down
2 changes: 1 addition & 1 deletion public/templates/show_lostpassword_form.inc.php
View file
Expand Up @@ -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'));
Expand Down
6 changes: 5 additions & 1 deletion public/templates/sidebar.inc.php
View file
Expand Up @@ -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');
Expand Down Expand Up @@ -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, {<?php echo $cookie_string ?>});
});

});
Expand Down
2 changes: 1 addition & 1 deletion src/Module/Api/Api.php
View file
Expand Up @@ -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']),
Expand Down
2 changes: 1 addition & 1 deletion src/Module/Api/Method/PingMethod.php
View file
Expand Up @@ -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);
Expand Down
10 changes: 9 additions & 1 deletion src/Module/Application/Logout/LogoutAction.php
View file
Expand Up @@ -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;
Expand All @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion src/Module/System/Core.php
View file
Expand Up @@ -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
Expand Down
17 changes: 10 additions & 7 deletions src/Module/System/Session.php
View file
Expand Up @@ -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));

Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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`= ?';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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'
];
Expand All @@ -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'),
Expand Down
13 changes: 10 additions & 3 deletions src/Module/Util/Captcha/easy_captcha_persistent_grant.php
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand Down
10 changes: 9 additions & 1 deletion src/Repository/Model/Browse.php
View file
Expand Up @@ -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);
}
}

Expand Down

0 comments on commit efed4e4

Please sign in to comment.