From be814a3ba55362dafc5ebffce704a59e2731a5bb Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Sat, 26 Sep 2026 09:32:49 +0200 Subject: [PATCH] Take redirects off the Host header. Same-site redirects are now relative paths, and a saved page on another site is ignored. The HTTP to HTTPS redirect takes its host from the new SITE_URL setting, and without it the pages refuse plain HTTP. --- .env.example | 4 ++ config.lib.php | 19 ++++++++++ publish/addeditor.php | 10 ++--- publish/edit.php | 8 ++-- publish/index.php | 6 +-- publish/login.php | 31 ++++++--------- publish/logout.php | 29 +++++---------- session.lib.php | 9 +++++ tests/php/Pages/AddEditorTest.php | 6 +-- tests/php/Pages/EditTest.php | 4 +- tests/php/Pages/HttpsTest.php | 23 ++++++++++++ tests/php/Pages/LoginTest.php | 60 ++++++++++++++++++++++++------ tests/php/Pages/SubmitTest.php | 2 +- tests/php/Support/PageTestCase.php | 7 +++- tests/php/fixtures/router.php | 3 +- tests/php/fixtures/test.env | 1 + 16 files changed, 149 insertions(+), 73 deletions(-) create mode 100644 tests/php/Pages/HttpsTest.php diff --git a/.env.example b/.env.example index 26f060b..714341a 100644 --- a/.env.example +++ b/.env.example @@ -7,6 +7,10 @@ DB_USER=xxx DB_PASS= DB_NAME=xxx +# Address of the site. The pages send plain HTTP requests to this host +# over HTTPS. If it is empty, the pages refuse plain HTTP. +SITE_URL=https://xxx.org.za + # Google reCAPTCHA v2 keys (https://www.google.com/recaptcha/admin). # Leave RECAPTCHA_SITE_KEY empty to turn the captcha off. RECAPTCHA_SITE_KEY= diff --git a/config.lib.php b/config.lib.php index db7d6e8..6b01fe1 100644 --- a/config.lib.php +++ b/config.lib.php @@ -40,6 +40,25 @@ function config($key, $default = NULL) { return array_key_exists($key, $values) ? $values[$key] : $default; } +/* + * Send a plain HTTP request to the same page over HTTPS, and stop. The + * host comes from SITE_URL, because the browser controls the Host header. + * Without SITE_URL, the page refuses plain HTTP. + */ +function require_https() { + if (isset($_SERVER['HTTPS'])) + return; + $site = parse_url((string)config('SITE_URL', '')); + if (empty($site['host'])) { + http_response_code(403); + echo "This page needs HTTPS.\n"; + } else { + $port = isset($site['port']) ? ':' . $site['port'] : ''; + header('Location: https://' . $site['host'] . $port . $_SERVER['REQUEST_URI']); + } + exit; +} + // Always log PHP errors; show them only if DISPLAY_ERRORS is set (for development) ini_set('log_errors', '1'); ini_set('display_errors', (string)config('DISPLAY_ERRORS', '0')); diff --git a/publish/addeditor.php b/publish/addeditor.php index fe9423c..3520cac 100644 --- a/publish/addeditor.php +++ b/publish/addeditor.php @@ -1,15 +1,13 @@ The editor was not added: the editors.password column is too short for the password hash. Widen it to VARCHAR(255).

-
" method="post"> +
diff --git a/publish/edit.php b/publish/edit.php index 30047a1..b398574 100644 --- a/publish/edit.php +++ b/publish/edit.php @@ -1,13 +1,11 @@ diff --git a/publish/index.php b/publish/index.php index 20b94e3..06cb4f7 100644 --- a/publish/index.php +++ b/publish/index.php @@ -1,8 +1,6 @@ diff --git a/publish/login.php b/publish/login.php index aac4f82..765e31d 100644 --- a/publish/login.php +++ b/publish/login.php @@ -1,8 +1,6 @@ Access denied. Please try again.

\n"; ?> -" method="post"> +
diff --git a/publish/logout.php b/publish/logout.php index 8e176e7..343bff1 100644 --- a/publish/logout.php +++ b/publish/logout.php @@ -4,13 +4,11 @@ header( 'Cache-Control: no-store, no-cache, must-revalidate' ); header( 'Cache-Control: post-check=0, pre-check=0', false ); header( 'Pragma: no-cache' ); include_once('../session.lib.php'); -if ( ! isset($_SERVER['HTTPS'])) { - header('Location: https://' . $_SERVER["SERVER_NAME"] . $_SERVER['REQUEST_URI']); - exit; -} +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'] ?? '')) { - header("Location: https://xxx.org.za/"); + header("Location: /"); exit; } if (isset($_SESSION['editor'])) { @@ -30,18 +28,11 @@ if (isset($_SESSION['editor'])) { session_destroy(); } delete_persistent_login(); -if (isset($_SESSION['referrer'])) { - $url = $_SERVER['HTTPS'] ? "https://" : "http://"; - $url .= $_SERVER['HTTP_HOST']; - $url .= $_SESSION['referrer']; - unset($_SESSION['referrer']); - if (preg_match("/logout(\.php)?$/i", $url)) { - $url = "/"; - } - header("Location: $url"); - return; -} else { - header("Location: https://xxx.org.za/"); - return; -} +$referrer = (string)($_SESSION['referrer'] ?? ''); +unset($_SESSION['referrer']); +if (is_local_path($referrer) && !preg_match("/logout(\.php)?$/i", $referrer)) + header('Location: ' . $referrer); +else + header("Location: /"); +return; ?> diff --git a/session.lib.php b/session.lib.php index bc4b644..5a58625 100644 --- a/session.lib.php +++ b/session.lib.php @@ -136,6 +136,15 @@ function csrf_valid($token = NULL) { hash_equals((string)$_SESSION['csrf_token'], $token); } +/* + * TRUE for a path on this site, such as "/article?id=1". A browser sends + * a redirect to "//host" or "/\host" to another site, so those are not + * local paths. + */ +function is_local_path($path) { + return is_string($path) && preg_match('#^/(?![/\\\\])#', $path) === 1; +} + /* * Log an editor in under a new session id, so that a session id that * someone planted or saw before the login is of no use afterwards. diff --git a/tests/php/Pages/AddEditorTest.php b/tests/php/Pages/AddEditorTest.php index 4552970..b2c3832 100644 --- a/tests/php/Pages/AddEditorTest.php +++ b/tests/php/Pages/AddEditorTest.php @@ -20,13 +20,13 @@ class AddEditorTest extends PageTestCase { public function testVisitorsAreSentToTheLogin(): void { $this->cookies = array(); - $this->assertRedirect('https://xxx.org.za/publish/login', $this->get('/publish/addeditor')); - $this->assertRedirect('https://xxx.org.za/publish/login', $this->add(array('csrf_token' => ''))); + $this->assertRedirect('/publish/login', $this->get('/publish/addeditor')); + $this->assertRedirect('/publish/login', $this->add(array('csrf_token' => ''))); $this->assertCount(1, $this->tables()['editors']); } public function testPlainHttpIsSentToHttps(): void { - $this->assertRedirect('https://xxx.org.za/publish/addeditor', $this->get('/publish/addeditor', array('X-Test-Plain' => '1'))); + $this->assertRedirect('https://site.test/publish/addeditor', $this->get('/publish/addeditor', array('X-Test-Plain' => '1'))); } public function testTheFormHasTheToken(): void { diff --git a/tests/php/Pages/EditTest.php b/tests/php/Pages/EditTest.php index 878aa71..eceb86c 100644 --- a/tests/php/Pages/EditTest.php +++ b/tests/php/Pages/EditTest.php @@ -23,11 +23,11 @@ class EditTest extends PageTestCase { public function testVisitorsAreSentToTheLogin(): void { $this->cookies = array(); - $this->assertRedirect('https://xxx.org.za/publish/login', $this->get('/publish/edit')); + $this->assertRedirect('/publish/login', $this->get('/publish/edit')); } public function testPlainHttpIsSentToHttps(): void { - $this->assertRedirect('https://xxx.org.za/publish/edit?id=1', $this->get('/publish/edit?id=1', array('X-Test-Plain' => '1'))); + $this->assertRedirect('https://site.test/publish/edit?id=1', $this->get('/publish/edit?id=1', array('X-Test-Plain' => '1'))); } public function testTheQueueIsShownFirst(): void { diff --git a/tests/php/Pages/HttpsTest.php b/tests/php/Pages/HttpsTest.php new file mode 100644 index 0000000..3c8e917 --- /dev/null +++ b/tests/php/Pages/HttpsTest.php @@ -0,0 +1,23 @@ + ' '); + } + + public function testWithoutASiteUrlPlainHttpIsRefused(): void { + $answer = $this->get('/publish/login', array('X-Test-Plain' => '1')); + $this->assertSame(403, $answer['status']); + $this->assertNull($this->header($answer, 'Location')); + $this->assertSame("This page needs HTTPS.\n", $answer['body']); + } + + public function testWithoutASiteUrlHttpsStillWorks(): void { + $this->assertSame(200, $this->get('/publish/login')['status']); + } +} diff --git a/tests/php/Pages/LoginTest.php b/tests/php/Pages/LoginTest.php index 866b9c8..7ac7859 100644 --- a/tests/php/Pages/LoginTest.php +++ b/tests/php/Pages/LoginTest.php @@ -1,6 +1,7 @@ assertRedirect('https://' . $this->host() . '/publish/login', $answer); - $this->assertRedirect('https://xxx.org.za/publish/edit', $this->get('/publish/login')); - } - - private function host(): string { - return parse_url(self::$base, PHP_URL_HOST) . ':' . parse_url(self::$base, PHP_URL_PORT); + $this->assertRedirect('/publish/login', $answer); + $this->assertRedirect('/publish/edit', $this->get('/publish/login')); } public function testPlainHttpIsSentToHttps(): void { - $this->assertRedirect('https://xxx.org.za/publish/login', $this->get('/publish/login', array('X-Test-Plain' => '1'))); + $this->assertRedirect('https://site.test/publish/login', $this->get('/publish/login', array('X-Test-Plain' => '1'))); + } + + public function testAForgedHostDoesNotMoveTheHttpsRedirect(): void { + $this->assertRedirect('https://site.test/publish/login', + $this->get('/publish/login', array('X-Test-Plain' => '1', 'Host' => 'evil.example'))); } public function testTheFormHasATokenAndTheCaptcha(): void { @@ -60,16 +62,38 @@ class LoginTest extends PageTestCase { $this->get('/article?id=1'); $answer = $this->post('/publish/login', array('login' => '1', 'username' => 'ed', 'password' => 'secret', 'csrf_token' => $token, 'g-recaptcha-response' => 'pass')); - $this->assertRedirect('https://' . $this->host() . '/article?id=1', $answer); + $this->assertRedirect('/article?id=1', $answer); $this->assertArrayNotHasKey('referrer', $this->session()); } + public function testAForgedHostDoesNotMoveTheLoginRedirect(): void { + $token = $this->csrfToken($this->get('/publish/login')['body']); + $this->get('/article?id=1'); + $answer = $this->request('POST', '/publish/login', array('login' => '1', 'username' => 'ed', 'password' => 'secret', + 'csrf_token' => $token, 'g-recaptcha-response' => 'pass'), + array('Host' => 'evil.example')); + $this->assertRedirect('/article?id=1', $answer); + } + + public static function otherSites(): array { + return array(array('@evil.example/'), array('//evil.example/'), array('/\\evil.example/'), array('https://evil.example/')); + } + + #[DataProvider('otherSites')] + public function testALastPageOnAnotherSiteIsIgnored(string $referrer): void { + $token = $this->csrfToken($this->get('/publish/login')['body']); + $this->startSession(array('csrf_token' => $token, 'referrer' => $referrer)); + $answer = $this->post('/publish/login', array('login' => '1', 'username' => 'ed', 'password' => 'secret', + 'csrf_token' => $token, 'g-recaptcha-response' => 'pass')); + $this->assertRedirect('/publish/edit', $answer); + } + public function testWithoutALastPageTheLoginGoesToTheEditPage(): void { $token = $this->csrfToken($this->get('/publish/login')['body']); $this->startSession(array('csrf_token' => $token)); $answer = $this->post('/publish/login', array('login' => '1', 'username' => 'ed', 'password' => 'secret', 'csrf_token' => $token, 'g-recaptcha-response' => 'pass')); - $this->assertRedirect('https://xxx.org.za/publish/edit', $answer); + $this->assertRedirect('/publish/edit', $answer); } public function testTheWrongPasswordIsDenied(): void { @@ -125,7 +149,7 @@ class LoginTest extends PageTestCase { public function testAnEditorWhoIsLoggedInGoesToTheEditPage(): void { $this->loginAs(); - $this->assertRedirect('https://xxx.org.za/publish/edit', $this->get('/publish/login')); + $this->assertRedirect('/publish/edit', $this->get('/publish/login')); } public function testRememberMeKeepsTheEditorLoggedIn(): void { @@ -151,7 +175,7 @@ class LoginTest extends PageTestCase { public function testLogoutNeedsTheToken(): void { $this->loginAs(); - $this->assertRedirect('https://xxx.org.za/', $this->get('/publish/logout?token=wrong')); + $this->assertRedirect('/', $this->get('/publish/logout?token=wrong')); $this->assertSame('ed', $this->session()['editor']); } @@ -164,13 +188,25 @@ class LoginTest extends PageTestCase { $session = self::$state . '/sessions/sess_' . $this->cookies['PHPSESSID']; $answer = $this->get('/publish/logout?token=' . $token); - $this->assertRedirect('https://' . $this->host() . '/article?id=2', $answer); + $this->assertRedirect('/article?id=2', $answer); $this->assertFileDoesNotExist($session); $this->assertArrayNotHasKey('PHPSESSID', $this->cookies); $this->assertArrayNotHasKey('PersistentAuth', $this->cookies); $this->assertSame(array(hash('sha256', 'other device')), array_column($this->tables()['persistent_sessions'], 'session')); } + 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'))); + } + + #[DataProvider('otherSites')] + public function testLogoutIgnoresALastPageOnAnotherSite(string $referrer): void { + $token = $this->loginAs('ed', array('referrer' => $referrer)); + $this->assertRedirect('/', $this->get('/publish/logout?token=' . $token)); + } + public function testLogoutDoesNotGoBackToTheLogoutPage(): void { $token = $this->loginAs('ed', array('referrer' => '/publish/logout')); $this->assertRedirect('/', $this->get('/publish/logout?token=' . $token)); diff --git a/tests/php/Pages/SubmitTest.php b/tests/php/Pages/SubmitTest.php index 9ebf9f9..7aeade2 100644 --- a/tests/php/Pages/SubmitTest.php +++ b/tests/php/Pages/SubmitTest.php @@ -16,7 +16,7 @@ class SubmitTest extends PageTestCase { } public function testPlainHttpIsSentToHttps(): void { - $this->assertRedirect('https://xxx.org.za/publish', $this->get('/publish', array('X-Test-Plain' => '1'))); + $this->assertRedirect('https://site.test/publish', $this->get('/publish', array('X-Test-Plain' => '1'))); } public function testTheFormHasATokenAndTheCaptcha(): void { diff --git a/tests/php/Support/PageTestCase.php b/tests/php/Support/PageTestCase.php index bc76fcd..9d3dfdb 100644 --- a/tests/php/Support/PageTestCase.php +++ b/tests/php/Support/PageTestCase.php @@ -15,6 +15,11 @@ abstract class PageTestCase extends \PHPUnit\Framework\TestCase { protected static string $base; private static $server = NULL; + // Environment variables for the server, on top of the usual ones + protected static function serverEnv(): array { + return array(); + } + // The cookies that the site set, name => value protected array $cookies = array(); @@ -43,7 +48,7 @@ abstract class PageTestCase extends \PHPUnit\Framework\TestCase { 'XXX_TEST_LOG' => self::$state . '/php.log', 'MAIL_LOG' => self::$state . '/mail.jsonl', 'UPLOAD_DIR' => self::$state . '/tmp/uploads', - ); + ) + static::serverEnv(); $log = array('file', self::$state . '/server.log', 'a'); self::$server = proc_open($command, array(0 => array('file', '/dev/null', 'r'), 1 => $log, 2 => $log), $pipes, XXX_ROOT, $env); diff --git a/tests/php/fixtures/router.php b/tests/php/fixtures/router.php index 275093d..8c80e2a 100644 --- a/tests/php/fixtures/router.php +++ b/tests/php/fixtures/router.php @@ -47,7 +47,8 @@ function xxx_page_route() { if (!isset($_SERVER['HTTP_X_TEST_PLAIN'])) $_SERVER['HTTPS'] = 'on'; - $_SERVER['SERVER_NAME'] = 'xxx.org.za'; + // Apache with UseCanonicalName Off also takes SERVER_NAME from the Host header + $_SERVER['SERVER_NAME'] = preg_replace('/:\d+$/', '', $_SERVER['HTTP_HOST'] ?? ''); $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_TEST_ADDRESS'] ?? '192.0.2.1'; $_SERVER['SCRIPT_FILENAME'] = $file; $_SERVER['SCRIPT_NAME'] = $_SERVER['PHP_SELF'] = substr($file, strlen(XXX_ROOT)); diff --git a/tests/php/fixtures/test.env b/tests/php/fixtures/test.env index 1942764..b843953 100644 --- a/tests/php/fixtures/test.env +++ b/tests/php/fixtures/test.env @@ -3,6 +3,7 @@ DB_HOST=db.test DB_USER=test-user DB_PASS=test-pass DB_NAME=test-db +SITE_URL=https://site.test RECAPTCHA_SITE_KEY=test-site-key RECAPTCHA_SECRET=test-secret GOOGLE_ANALYTICS_ID=