diff --git a/publish/logout.php b/publish/logout.php index 343bff1..c486ceb 100644 --- a/publish/logout.php +++ b/publish/logout.php @@ -6,8 +6,8 @@ header( 'Pragma: no-cache' ); include_once('../session.lib.php'); require_once('../config.lib.php'); require_https(); -// Only log out from our own logout link, which carries the CSRF token -if (!csrf_valid($_GET['token'] ?? '')) { +// Only log out from our own logout form, which posts the CSRF token +if (!csrf_valid()) { header("Location: /"); exit; } diff --git a/sidebar.inc.php b/sidebar.inc.php index 35aa597..0b874a3 100644 --- a/sidebar.inc.php +++ b/sidebar.inc.php @@ -29,7 +29,8 @@ if (preg_match("#^(/$|/index|/article)#", $_SERVER['REQUEST_URI'])) {
  • Add/Edit
  • Add editor'."\n"; - echo '
  • Logout
  • '."\n"; + // A form, so that the token stays out of the URL + echo '
  • '.csrf_field().'
  • '."\n"; } //echo '
  • Publishing policy
  • '; ?> diff --git a/style.css b/style.css index e2d04ec..1b4b18a 100644 --- a/style.css +++ b/style.css @@ -146,6 +146,23 @@ footer#bottom p { margin-right: 5px; } +/* The logout button looks like the links next to it */ +#sidebar .links li form { + display: inline; +} +#sidebar .links li button { + padding: 0; + border: 0; + background: none; + font: inherit; + color: inherit; + cursor: pointer; +} +#sidebar .links li button:hover { + color: #FF0000; + text-decoration: underline; +} + #sidebar .links .sideinput { padding: 1px; padding: 0; diff --git a/tests/php/Pages/FrontPagesTest.php b/tests/php/Pages/FrontPagesTest.php index a753275..e8a314d 100644 --- a/tests/php/Pages/FrontPagesTest.php +++ b/tests/php/Pages/FrontPagesTest.php @@ -85,7 +85,9 @@ class FrontPagesTest extends PageTestCase { $token = $this->loginAs(); $body = $this->get('/')['body']; $this->assertStringContainsString('Secret', $body); - $this->assertStringContainsString('Logout', $body); + $this->assertStringContainsString('
    ' + . '
    ', $body); + $this->assertStringNotContainsString('?token=', $body); $this->assertStringContainsString('Add editor', $body); } diff --git a/tests/php/Pages/LoginTest.php b/tests/php/Pages/LoginTest.php index 6e91b29..490548b 100644 --- a/tests/php/Pages/LoginTest.php +++ b/tests/php/Pages/LoginTest.php @@ -182,7 +182,7 @@ class LoginTest extends PageTestCase { // A new browser session with only the cookie unset($this->cookies['PHPSESSID']); $body = $this->get('/')['body']; - $this->assertStringContainsString('/publish/logout?token=', $body); + $this->assertStringContainsString('
    ', $body); $this->assertSame('ed', $this->session()['editor']); } @@ -224,7 +224,15 @@ class LoginTest extends PageTestCase { public function testLogoutNeedsTheToken(): void { $this->loginAs(); - $this->assertRedirect('/', $this->get('/publish/logout?token=wrong')); + $this->assertRedirect('/', $this->post('/publish/logout', array('csrf_token' => 'wrong'))); + $this->assertSame('ed', $this->session()['editor']); + } + + // The token stays out of URLs, which go into logs and the browser history + public function testLogoutNeedsAPost(): void { + $token = $this->loginAs(); + $this->assertRedirect('/', $this->get('/publish/logout?token=' . $token)); + $this->assertRedirect('/', $this->get('/publish/logout?csrf_token=' . $token)); $this->assertSame('ed', $this->session()['editor']); } @@ -236,7 +244,7 @@ class LoginTest extends PageTestCase { $this->cookies['PersistentAuth'] = 'ed|key'; $session = self::$state . '/sessions/sess_' . $this->cookies['PHPSESSID']; - $answer = $this->get('/publish/logout?token=' . $token); + $answer = $this->post('/publish/logout', array('csrf_token' => $token)); $this->assertRedirect('/article?id=2', $answer); $this->assertFileDoesNotExist($session); $this->assertArrayNotHasKey('PHPSESSID', $this->cookies); @@ -247,17 +255,17 @@ class LoginTest extends PageTestCase { public function testAForgedHostDoesNotMoveTheLogoutRedirect(): void { $token = $this->loginAs('ed', array('referrer' => '/article?id=2')); $this->assertRedirect('/article?id=2', - $this->get('/publish/logout?token=' . $token, array('Host' => 'evil.example'))); + $this->post('/publish/logout', array('csrf_token' => $token), array('Host' => 'evil.example'))); } #[DataProvider('otherSites')] public function testLogoutIgnoresALastPageOnAnotherSite(string $referrer): void { $token = $this->loginAs('ed', array('referrer' => $referrer)); - $this->assertRedirect('/', $this->get('/publish/logout?token=' . $token)); + $this->assertRedirect('/', $this->post('/publish/logout', array('csrf_token' => $token))); } public function testLogoutDoesNotGoBackToTheLogoutPage(): void { $token = $this->loginAs('ed', array('referrer' => '/publish/logout')); - $this->assertRedirect('/', $this->get('/publish/logout?token=' . $token)); + $this->assertRedirect('/', $this->post('/publish/logout', array('csrf_token' => $token))); } }