From 093007b5354558653978662e640cc3dc1ccb9f8a Mon Sep 17 00:00:00 2001 From: sgiehl Date: Tue, 17 Mar 2026 18:32:57 +0100 Subject: [PATCH 1/3] CoreUpdater: prevent token creation/disclosure via oneClickResults for anonymous users --- plugins/CoreUpdater/Controller.php | 27 ++- .../tests/Integration/ControllerTest.php | 170 ++++++++++++++++++ tests/UI/specs/OneClickUpdate_spec.js | 7 +- 3 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 plugins/CoreUpdater/tests/Integration/ControllerTest.php diff --git a/plugins/CoreUpdater/Controller.php b/plugins/CoreUpdater/Controller.php index be3d19408d0..94c8cab9bfe 100644 --- a/plugins/CoreUpdater/Controller.php +++ b/plugins/CoreUpdater/Controller.php @@ -171,6 +171,7 @@ public function oneClickUpdate() try { $messages = $this->updater->updatePiwik($useHttps); + $this->refreshUpdateDetailsToken(); } catch (ArchiveDownloadException $e) { $view->httpsFail = $useHttps; $view->error = $e->getMessage(); @@ -244,7 +245,13 @@ public function oneClickResults() $view = new View('@CoreUpdater/updateHttpError'); $view->error = $error; } else { - $updateDetailsToken = $this->createUpdateDetailsTokenIfMissing(); + $updateDetailsToken = null; + if (Piwik::hasUserSuperUserAccess()) { + $updateDetailsToken = $this->getUpdateDetailsToken(); + if ($updateDetailsToken === null) { + $updateDetailsToken = $this->refreshUpdateDetailsToken(); + } + } $runUpdaterUrl = Url::getCurrentUrlWithoutQueryString(); if ($updateDetailsToken !== null) { @@ -474,14 +481,9 @@ private function checkUpdateDetailsToken(): bool return $config->General['update_details_token'] === Request::fromRequest()->getStringParameter('updateDetailsToken', ''); } - private function createUpdateDetailsTokenIfMissing(): ?string + private function refreshUpdateDetailsToken(): string { $config = Config::getInstance(); - - if (!empty($config->General['update_details_token'])) { - return null; - } - $token = Common::generateUniqId(); $config->General['update_details_token'] = $token; @@ -490,6 +492,17 @@ private function createUpdateDetailsTokenIfMissing(): ?string return $token; } + private function getUpdateDetailsToken(): ?string + { + $config = Config::getInstance(); + + if (empty($config->General['update_details_token'])) { + return null; + } + + return $config->General['update_details_token']; + } + private function removeUpdateDetailsToken(): void { $config = Config::getInstance(); diff --git a/plugins/CoreUpdater/tests/Integration/ControllerTest.php b/plugins/CoreUpdater/tests/Integration/ControllerTest.php new file mode 100644 index 00000000000..cff7c221408 --- /dev/null +++ b/plugins/CoreUpdater/tests/Integration/ControllerTest.php @@ -0,0 +1,170 @@ +originalGet = $_GET; + $this->originalPost = $_POST; + + $general = Config::getInstance()->General; + $this->hadEnableAutoUpdate = array_key_exists('enable_auto_update', $general); + $this->hadEnableInternetFeatures = array_key_exists('enable_internet_features', $general); + $this->hadUpdateDetailsToken = array_key_exists('update_details_token', $general); + $this->originalEnableAutoUpdate = $general['enable_auto_update'] ?? null; + $this->originalEnableInternetFeatures = $general['enable_internet_features'] ?? null; + $this->originalUpdateDetailsToken = $general['update_details_token'] ?? null; + + Config::getInstance()->General['enable_auto_update'] = 1; + Config::getInstance()->General['enable_internet_features'] = 1; + } + + public function tearDown(): void + { + $_GET = $this->originalGet; + $_POST = $this->originalPost; + + FakeAccess::clearAccess(); + + $config = Config::getInstance(); + if ($this->hadEnableAutoUpdate) { + $config->General['enable_auto_update'] = $this->originalEnableAutoUpdate; + } else { + unset($config->General['enable_auto_update']); + } + + if ($this->hadEnableInternetFeatures) { + $config->General['enable_internet_features'] = $this->originalEnableInternetFeatures; + } else { + unset($config->General['enable_internet_features']); + } + + if ($this->hadUpdateDetailsToken) { + $config->General['update_details_token'] = $this->originalUpdateDetailsToken; + } else { + unset($config->General['update_details_token']); + } + + $config->forceSave(); + + parent::tearDown(); + } + + public function testOneClickResultsDoesNotCreateOrExposeTokenForAnonymousRequest() + { + FakeAccess::clearAccess(false); + unset(Config::getInstance()->General['update_details_token']); + + $_GET = []; + $_POST = []; + + $result = $this->buildController()->oneClickResults(); + + self::assertStringNotContainsString('updateDetailsToken=', $result); + self::assertEmpty(Config::getInstance()->General['update_details_token'] ?? null); + } + + public function testOneClickResultsOnlyExposesStoredTokenForSuperUserContext() + { + $token = '1234567890abcdef1234567890abcdef'; + Config::getInstance()->General['update_details_token'] = $token; + Config::getInstance()->forceSave(); + + $_GET = []; + $_POST = []; + + FakeAccess::clearAccess(true); + $superUserResult = $this->buildController()->oneClickResults(); + self::assertStringContainsString('updateDetailsToken=' . urlencode($token), $superUserResult); + + FakeAccess::clearAccess(false); + $anonymousResult = $this->buildController()->oneClickResults(); + self::assertStringNotContainsString('updateDetailsToken=' . urlencode($token), $anonymousResult); + } + + public function testOneClickResultsCreatesAndExposesTokenForSuperUserWhenMissing() + { + FakeAccess::clearAccess(true); + unset(Config::getInstance()->General['update_details_token']); + + $_GET = []; + $_POST = []; + + $result = $this->buildController()->oneClickResults(); + $createdToken = Config::getInstance()->General['update_details_token'] ?? null; + + self::assertNotEmpty($createdToken); + self::assertStringContainsString('updateDetailsToken=' . urlencode($createdToken), $result); + } + + public function testOneClickUpdateRotatesStoredUpdateDetailsTokenAfterSuccessfulUpdate() + { + FakeAccess::clearAccess(true); + Config::getInstance()->General['update_details_token'] = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; + + $_GET = []; + $_POST = []; + $_GET['nonce'] = Nonce::getNonce('oneClickUpdate'); + + ob_start(); + try { + $this->buildController()->oneClickUpdate(); + $output = ob_get_clean(); + } catch (\Throwable $e) { + ob_end_clean(); + throw $e; + } + + $rotatedToken = Config::getInstance()->General['update_details_token'] ?? ''; + + self::assertStringContainsString('action=oneClickResults', $output); + self::assertNotSame('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', $rotatedToken); + self::assertSame(1, preg_match('/^[a-f0-9]{32}$/', $rotatedToken)); + } + + public function provideContainerConfig() + { + return array( + 'Piwik\Access' => new FakeAccess(), + ); + } + + private function buildController(): Controller + { + $updater = $this->createMock(Updater::class); + $updater->method('updatePiwik')->willReturn(array()); + + return new Controller($updater); + } +} diff --git a/tests/UI/specs/OneClickUpdate_spec.js b/tests/UI/specs/OneClickUpdate_spec.js index 448942061d5..13a93c9a153 100644 --- a/tests/UI/specs/OneClickUpdate_spec.js +++ b/tests/UI/specs/OneClickUpdate_spec.js @@ -90,8 +90,11 @@ describe("OneClickUpdate", function () { const updateDetailsToken = readUpdateDetailsTokenFromConfig(); const runUpdaterLink = await page.$eval('.footer a', link => link.getAttribute('href')); - expect(updateDetailsToken).to.be.ok; - expect(runUpdaterLink).to.match(new RegExp(`[?&]updateDetailsToken=${updateDetailsToken}(?:&|$)`)); + if (updateDetailsToken) { + expect(runUpdaterLink).to.match(new RegExp(`[?&]updateDetailsToken=${updateDetailsToken}(?:&|$)`)); + } else { + expect(runUpdaterLink).to.not.match(/[?&]updateDetailsToken=/); + } }); it('should login successfully after the update', async function () { From df10b9d7c7b01fcde870c80b42fb02254070700d Mon Sep 17 00:00:00 2001 From: sgiehl Date: Mon, 27 Apr 2026 15:22:37 +0200 Subject: [PATCH 2/3] fix parameter name --- plugins/CoreUpdater/templates/runUpdaterAndExit_welcome.twig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/CoreUpdater/templates/runUpdaterAndExit_welcome.twig b/plugins/CoreUpdater/templates/runUpdaterAndExit_welcome.twig index 6f61acbe854..9da13a413c6 100644 --- a/plugins/CoreUpdater/templates/runUpdaterAndExit_welcome.twig +++ b/plugins/CoreUpdater/templates/runUpdaterAndExit_welcome.twig @@ -67,7 +67,7 @@ {{ 'CoreUpdater_UpdateDetailsHidden1'|translate }}
From 8f0795a666c982a5730113a898e9ddbefc5fac74 Mon Sep 17 00:00:00 2001 From: sgiehl Date: Mon, 27 Apr 2026 18:10:45 +0200 Subject: [PATCH 3/3] updates expected test files --- .../CoreUpdaterDb_main_no_update_token.png | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/CoreUpdater/tests/UI/expected-screenshots/CoreUpdaterDb_main_no_update_token.png b/plugins/CoreUpdater/tests/UI/expected-screenshots/CoreUpdaterDb_main_no_update_token.png index fb5da11314d..40faaffdec8 100644 --- a/plugins/CoreUpdater/tests/UI/expected-screenshots/CoreUpdaterDb_main_no_update_token.png +++ b/plugins/CoreUpdater/tests/UI/expected-screenshots/CoreUpdaterDb_main_no_update_token.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:32f28a4da0027d6d880e3701108481c831d94337c8ce3f666491140955694a5a -size 328472 +oid sha256:abafcbeb6fdb9adcb6f6515465425d303a6c3e373579ee718d64b51f19fc2f9c +size 328301