Merge pull request phpbb#6256 from marc1706/ticket/16825

[ticket/16825] Adjust handling of session ID when requiring cookies
paul999 · Aug 19, 2021 · 49b01d0 · 49b01d0
2 parents c1408ec + 217fc07
commit 49b01d0
Show file tree

Hide file tree

Showing 13 changed files with 32 additions and 30 deletions.
diff --git a/phpBB/includes/acp/acp_main.php b/phpBB/includes/acp/acp_main.php
@@ -60,8 +60,15 @@ function main($id, $mode)
 		{
 			if ($action === 'admlogout')
 			{
-				$user->unset_admin();
-				redirect(append_sid("{$phpbb_root_path}index.$phpEx"));
+				if (check_link_hash($request->variable('hash', ''), 'acp_logout'))
+				{
+					$user->unset_admin();
+					redirect(append_sid("{$phpbb_root_path}index.$phpEx"));
+				}
+				else
+				{
+					redirect(append_sid("{$phpbb_admin_path}index.$phpEx"));
+				}
 			}
 
 			if (!confirm_box(true))

diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php
@@ -3716,7 +3716,7 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
 	// Generate logged in/logged out status
 	if ($user->data['user_id'] != ANONYMOUS)
 	{
-		$u_login_logout = append_sid("{$phpbb_root_path}ucp.$phpEx", 'mode=logout');
+		$u_login_logout = append_sid("{$phpbb_root_path}ucp.$phpEx", 'mode=logout&amp;hash=' . generate_link_hash('ucp_logout'));
 		$l_login_logout = $user->lang['LOGOUT'];
 	}
 	else

diff --git a/phpBB/includes/functions_acp.php b/phpBB/includes/functions_acp.php
@@ -85,7 +85,7 @@ function adm_page_header($page_title)
 		'PHPBB_MAJOR'			=> $phpbb_major,
 
 		'U_LOGOUT'				=> append_sid("{$phpbb_root_path}ucp.$phpEx", 'mode=logout'),
-		'U_ADM_LOGOUT'			=> append_sid("{$phpbb_admin_path}index.$phpEx", 'action=admlogout'),
+		'U_ADM_LOGOUT'			=> append_sid("{$phpbb_admin_path}index.$phpEx", 'action=admlogout&amp;hash=' . generate_link_hash('acp_logout')),
 		'U_ADM_INDEX'			=> append_sid("{$phpbb_admin_path}index.$phpEx"),
 		'U_INDEX'				=> append_sid("{$phpbb_root_path}index.$phpEx"),
 

diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php
@@ -275,7 +275,7 @@ function session_begin($update_session_page = true)
 			$SID = '?sid=';
 			$_SID = '';
 
-			if (empty($this->session_id))
+			if (empty($this->session_id) && $phpbb_container->getParameter('session.force_sid'))
 			{
 				$this->session_id = $_SID = $request->variable('sid', '');
 				$SID = '?sid=' . $this->session_id;
@@ -284,7 +284,7 @@ function session_begin($update_session_page = true)
 		}
 		else
 		{
-			$this->session_id = $_SID = $request->variable('sid', '');
+			$this->session_id = $_SID = $phpbb_container->getParameter('session.force_sid') ? $request->variable('sid', '') : '';
 			$SID = '?sid=' . $this->session_id;
 		}
 

diff --git a/phpBB/styles/prosilver/template/index_body.html b/phpBB/styles/prosilver/template/index_body.html
@@ -25,7 +25,7 @@ <h3><a href="{U_LOGIN_LOGOUT}">{L_LOGIN_LOGOUT}</a><!-- IF S_REGISTER_ENABLED --
 				<a href="{U_SEND_PASSWORD}">{L_FORGOT_PASS}</a>
 			<!-- ENDIF -->
 			<!-- IF S_AUTOLOGIN_ENABLED -->
-				<span class="responsive-hide">|</span> <label for="autologin">{L_LOG_ME_IN} <input type="checkbox" tabindex="4" name="autologin" id="autologin" /></label>
+				<span class="responsive-hide">|</span> <label for="autologin">{L_LOG_ME_IN} <input type="checkbox" tabindex="4" name="autologin" id="autologin" checked /></label>
 			<!-- ENDIF -->
 			<input type="submit" tabindex="5" name="login" value="{L_LOGIN}" class="button1 button button-form-bold" />
 			{S_LOGIN_REDIRECT}

diff --git a/phpBB/styles/prosilver/template/login_body.html b/phpBB/styles/prosilver/template/login_body.html
@@ -28,7 +28,7 @@ <h2 class="login-title"><!-- IF LOGIN_EXPLAIN -->{LOGIN_EXPLAIN}<!-- ELSE -->{L_
 		<!-- IF S_DISPLAY_FULL_LOGIN -->
 		<dl>
 			<dt>&nbsp;</dt>
-			<!-- IF S_AUTOLOGIN_ENABLED --><dd><label for="autologin"><input type="checkbox" name="autologin" id="autologin" tabindex="4" /> {L_LOG_ME_IN}</label></dd><!-- ENDIF -->
+			<!-- IF S_AUTOLOGIN_ENABLED --><dd><label for="autologin"><input type="checkbox" name="autologin" id="autologin" tabindex="4" checked /> {L_LOG_ME_IN}</label></dd><!-- ENDIF -->
 			<dd><label for="viewonline"><input type="checkbox" name="viewonline" id="viewonline" tabindex="5" /> {L_HIDE_ME}</label></dd>
 		</dl>
 		<!-- ENDIF -->

diff --git a/phpBB/styles/prosilver/template/viewforum_body.html b/phpBB/styles/prosilver/template/viewforum_body.html
@@ -106,7 +106,7 @@ <h3><a href="{U_LOGIN_LOGOUT}">{L_LOGIN_LOGOUT}</a><!-- IF S_REGISTER_ENABLED --
 				<dl>
 					<dt><label for="password">{L_PASSWORD}{L_COLON}</label></dt>
 					<dd><input type="password" tabindex="2" id="password" name="password" size="25" class="inputbox autowidth" autocomplete="off" /></dd>
-					<!-- IF S_AUTOLOGIN_ENABLED --><dd><label for="autologin"><input type="checkbox" name="autologin" id="autologin" tabindex="3" /> {L_LOG_ME_IN}</label></dd><!-- ENDIF -->
+					<!-- IF S_AUTOLOGIN_ENABLED --><dd><label for="autologin"><input type="checkbox" name="autologin" id="autologin" tabindex="3" checked /> {L_LOG_ME_IN}</label></dd><!-- ENDIF -->
 					<dd><label for="viewonline"><input type="checkbox" name="viewonline" id="viewonline" tabindex="4" /> {L_HIDE_ME}</label></dd>
 				</dl>
 				<dl>

diff --git a/phpBB/ucp.php b/phpBB/ucp.php
@@ -103,7 +103,7 @@
 	break;
 
 	case 'logout':
-		if ($user->data['user_id'] != ANONYMOUS && $request->is_set('sid') && $request->variable('sid', '') === $user->session_id)
+		if ($user->data['user_id'] != ANONYMOUS && check_link_hash($request->variable('hash', ''), 'ucp_logout'))
 		{
 			$user->session_kill();
 		}

diff --git a/tests/functional/auth_test.php b/tests/functional/auth_test.php
@@ -60,12 +60,7 @@ public function test_logout()
 		$this->login();
 		$this->add_lang('ucp');
 
-		// logout
-		$crawler = self::request('GET', 'ucp.php?sid=' . $this->sid . '&mode=logout');
-
-		// look for a register link, which should be visible only when logged out
-		$crawler = self::request('GET', 'index.php');
-		$this->assertStringContainsString($this->lang('REGISTER'), $crawler->filter('.navbar')->text());
+		$this->logout();
 	}
 
 	public function test_acp_login()

diff --git a/tests/functional/mcp_test.php b/tests/functional/mcp_test.php
@@ -46,6 +46,7 @@ public function test_handle_quickmod($crawler)
 	public function test_move_post_to_topic($crawler)
 	{
 		$this->login();
+		$this->add_lang('mcp');
 
 		// Select the post in MCP
 		$form = $crawler->selectButton($this->lang('SUBMIT'))->form(array(
@@ -55,18 +56,11 @@ public function test_move_post_to_topic($crawler)
 		$crawler = self::submit($form);
 		$this->assertStringContainsString($this->lang('MERGE_POSTS'), $crawler->filter('html')->text());
 
-		return $crawler;
-	}
-
-	/**
-	* @depends test_move_post_to_topic
-	*/
-	public function test_confirm_result($crawler)
-	{
-		$this->add_lang('mcp');
 		$form = $crawler->selectButton('Yes')->form();
 		$crawler = self::submit($form);
 		$this->assertStringContainsString($this->lang('POSTS_MERGED_SUCCESS'), $crawler->text());
+
+		return $crawler;
 	}
 
 	public function test_delete_logs()

diff --git a/tests/functional/report_post_captcha_test.php b/tests/functional/report_post_captcha_test.php
@@ -64,8 +64,8 @@ protected function set_reporting_guest($report_post_allowed)
 		$values = $form->getValues();
 		$values["setting[1][2][f_report]"] = $report_post_allowed;
 		$form->setValues($values);
-		$crawler = self::submit($form);
+		self::submit($form);
 
-		$crawler = self::request('GET', 'ucp.php?mode=logout&sid=' . $this->sid);
+		$this->logout();
 	}
 }
diff --git a/tests/functional/ucp_profile_test.php b/tests/functional/ucp_profile_test.php
@@ -89,7 +89,10 @@ public function test_autologin_keys_manage()
 		$this->assertStringContainsString($key_id, $crawler->filter('label[for="' . $key_id . '"]')->text());
 
 		$form = $crawler->selectButton('submit')->form();
-		$form['keys'][0]->tick();
+		foreach ($form['keys'] as $key)
+		{
+			$key->tick();
+		}
 		$crawler = self::submit($form);
 		$this->assertStringContainsString($this->lang('AUTOLOGIN_SESSION_KEYS_DELETED'), $crawler->filter('html')->text());
 

diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php
@@ -829,10 +829,13 @@ protected function logout()
 	{
 		$this->add_lang('ucp');
 
-		$crawler = self::request('GET', 'ucp.php?sid=' . $this->sid . '&mode=logout');
+		$crawler = self::request('GET', 'index.php');
+		$logout_link = $crawler->filter('a[title="' . $this->lang('LOGOUT') . '"]')->attr('href');
+		self::request('GET', $logout_link);
+
+		$crawler = self::request('GET', $logout_link);
 		$this->assertStringContainsString($this->lang('REGISTER'), $crawler->filter('.navbar')->text());
 		unset($this->sid);
-
 	}
 
 	/**