Skip to content

Commit 917a623

Browse files
sgiehlsgiehl
committed
CoreUpdater: prevent token creation/disclosure via oneClickResults
1 parent 50431de commit 917a623

3 files changed

Lines changed: 195 additions & 9 deletions

File tree

‎plugins/CoreUpdater/Controller.php‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ public function oneClickUpdate()
171171

172172
try {
173173
$messages = $this->updater->updatePiwik($useHttps);
174+
$this->refreshUpdateDetailsToken();
174175
} catch (ArchiveDownloadException $e) {
175176
$view->httpsFail = $useHttps;
176177
$view->error = $e->getMessage();
@@ -244,7 +245,13 @@ public function oneClickResults()
244245
$view = new View('@CoreUpdater/updateHttpError');
245246
$view->error = $error;
246247
} else {
247-
$updateDetailsToken = $this->createUpdateDetailsTokenIfMissing();
248+
$updateDetailsToken = null;
249+
if (Piwik::hasUserSuperUserAccess()) {
250+
$updateDetailsToken = $this->getUpdateDetailsToken();
251+
if ($updateDetailsToken === null) {
252+
$updateDetailsToken = $this->refreshUpdateDetailsToken();
253+
}
254+
}
248255
$runUpdaterUrl = Url::getCurrentUrlWithoutQueryString();
249256

250257
if ($updateDetailsToken !== null) {
@@ -474,14 +481,9 @@ private function checkUpdateDetailsToken(): bool
474481
return $config->General['update_details_token'] === Request::fromRequest()->getStringParameter('updateDetailsToken', '');
475482
}
476483

477-
private function createUpdateDetailsTokenIfMissing(): ?string
484+
private function refreshUpdateDetailsToken(): string
478485
{
479486
$config = Config::getInstance();
480-
481-
if (!empty($config->General['update_details_token'])) {
482-
return null;
483-
}
484-
485487
$token = Common::generateUniqId();
486488

487489
$config->General['update_details_token'] = $token;
@@ -490,6 +492,17 @@ private function createUpdateDetailsTokenIfMissing(): ?string
490492
return $token;
491493
}
492494

495+
private function getUpdateDetailsToken(): ?string
496+
{
497+
$config = Config::getInstance();
498+
499+
if (empty($config->General['update_details_token'])) {
500+
return null;
501+
}
502+
503+
return $config->General['update_details_token'];
504+
}
505+
493506
private function removeUpdateDetailsToken(): void
494507
{
495508
$config = Config::getInstance();

‎plugins/CoreUpdater/tests/Integration/ControllerTest.php‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
<?php
2+
3+
/**
4+
* Matomo - free/libre analytics platform
5+
*
6+
* @link https://matomo.org
7+
* @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
8+
*/
9+
10+
namespace Piwik\Plugins\CoreUpdater\tests\Integration;
11+
12+
use Piwik\Config;
13+
use Piwik\Nonce;
14+
use Piwik\Plugins\CoreUpdater\Controller;
15+
use Piwik\Plugins\CoreUpdater\Updater;
16+
use Piwik\Tests\Framework\Mock\FakeAccess;
17+
use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
18+
19+
/**
20+
* @group Plugins
21+
* @group CoreUpdater
22+
*/
23+
class ControllerTest extends IntegrationTestCase
24+
{
25+
private $originalGet = [];
26+
private $originalPost = [];
27+
private $originalEnableAutoUpdate;
28+
private $originalEnableInternetFeatures;
29+
private $originalUpdateDetailsToken;
30+
private $hadEnableAutoUpdate = false;
31+
private $hadEnableInternetFeatures = false;
32+
private $hadUpdateDetailsToken = false;
33+
34+
public function setUp(): void
35+
{
36+
parent::setUp();
37+
38+
$this->originalGet = $_GET;
39+
$this->originalPost = $_POST;
40+
41+
$general = Config::getInstance()->General;
42+
$this->hadEnableAutoUpdate = array_key_exists('enable_auto_update', $general);
43+
$this->hadEnableInternetFeatures = array_key_exists('enable_internet_features', $general);
44+
$this->hadUpdateDetailsToken = array_key_exists('update_details_token', $general);
45+
$this->originalEnableAutoUpdate = $general['enable_auto_update'] ?? null;
46+
$this->originalEnableInternetFeatures = $general['enable_internet_features'] ?? null;
47+
$this->originalUpdateDetailsToken = $general['update_details_token'] ?? null;
48+
49+
Config::getInstance()->General['enable_auto_update'] = 1;
50+
Config::getInstance()->General['enable_internet_features'] = 1;
51+
}
52+
53+
public function tearDown(): void
54+
{
55+
$_GET = $this->originalGet;
56+
$_POST = $this->originalPost;
57+
58+
FakeAccess::clearAccess();
59+
60+
$config = Config::getInstance();
61+
if ($this->hadEnableAutoUpdate) {
62+
$config->General['enable_auto_update'] = $this->originalEnableAutoUpdate;
63+
} else {
64+
unset($config->General['enable_auto_update']);
65+
}
66+
67+
if ($this->hadEnableInternetFeatures) {
68+
$config->General['enable_internet_features'] = $this->originalEnableInternetFeatures;
69+
} else {
70+
unset($config->General['enable_internet_features']);
71+
}
72+
73+
if ($this->hadUpdateDetailsToken) {
74+
$config->General['update_details_token'] = $this->originalUpdateDetailsToken;
75+
} else {
76+
unset($config->General['update_details_token']);
77+
}
78+
79+
$config->forceSave();
80+
81+
parent::tearDown();
82+
}
83+
84+
public function testOneClickResultsDoesNotCreateOrExposeTokenForAnonymousRequest()
85+
{
86+
FakeAccess::clearAccess(false);
87+
unset(Config::getInstance()->General['update_details_token']);
88+
89+
$_GET = [];
90+
$_POST = [];
91+
92+
$result = $this->buildController()->oneClickResults();
93+
94+
self::assertStringNotContainsString('updateDetailsToken=', $result);
95+
self::assertEmpty(Config::getInstance()->General['update_details_token'] ?? null);
96+
}
97+
98+
public function testOneClickResultsOnlyExposesStoredTokenForSuperUserContext()
99+
{
100+
$token = '1234567890abcdef1234567890abcdef';
101+
Config::getInstance()->General['update_details_token'] = $token;
102+
Config::getInstance()->forceSave();
103+
104+
$_GET = [];
105+
$_POST = [];
106+
107+
FakeAccess::clearAccess(true);
108+
$superUserResult = $this->buildController()->oneClickResults();
109+
self::assertStringContainsString('updateDetailsToken=' . urlencode($token), $superUserResult);
110+
111+
FakeAccess::clearAccess(false);
112+
$anonymousResult = $this->buildController()->oneClickResults();
113+
self::assertStringNotContainsString('updateDetailsToken=' . urlencode($token), $anonymousResult);
114+
}
115+
116+
public function testOneClickResultsCreatesAndExposesTokenForSuperUserWhenMissing()
117+
{
118+
FakeAccess::clearAccess(true);
119+
unset(Config::getInstance()->General['update_details_token']);
120+
121+
$_GET = [];
122+
$_POST = [];
123+
124+
$result = $this->buildController()->oneClickResults();
125+
$createdToken = Config::getInstance()->General['update_details_token'] ?? null;
126+
127+
self::assertNotEmpty($createdToken);
128+
self::assertStringContainsString('updateDetailsToken=' . urlencode($createdToken), $result);
129+
}
130+
131+
public function testOneClickUpdateRotatesStoredUpdateDetailsTokenAfterSuccessfulUpdate()
132+
{
133+
FakeAccess::clearAccess(true);
134+
Config::getInstance()->General['update_details_token'] = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
135+
136+
$_GET = [];
137+
$_POST = [];
138+
$_GET['nonce'] = Nonce::getNonce('oneClickUpdate');
139+
140+
ob_start();
141+
try {
142+
$this->buildController()->oneClickUpdate();
143+
$output = ob_get_clean();
144+
} catch (\Throwable $e) {
145+
ob_end_clean();
146+
throw $e;
147+
}
148+
149+
$rotatedToken = Config::getInstance()->General['update_details_token'] ?? '';
150+
151+
self::assertStringContainsString('action=oneClickResults', $output);
152+
self::assertNotSame('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', $rotatedToken);
153+
self::assertSame(1, preg_match('/^[a-f0-9]{32}$/', $rotatedToken));
154+
}
155+
156+
public function provideContainerConfig()
157+
{
158+
return array(
159+
'Piwik\Access' => new FakeAccess(),
160+
);
161+
}
162+
163+
private function buildController(): Controller
164+
{
165+
$updater = $this->createMock(Updater::class);
166+
$updater->method('updatePiwik')->willReturn(array());
167+
168+
return new Controller($updater);
169+
}
170+
}

‎tests/UI/specs/OneClickUpdate_spec.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,11 @@ describe("OneClickUpdate", function () {
9090
const updateDetailsToken = readUpdateDetailsTokenFromConfig();
9191
const runUpdaterLink = await page.$eval('.footer a', link => link.getAttribute('href'));
9292

93-
expect(updateDetailsToken).to.be.ok;
94-
expect(runUpdaterLink).to.match(new RegExp(`[?&]updateDetailsToken=${updateDetailsToken}(?:&|$)`));
93+
if (updateDetailsToken) {
94+
expect(runUpdaterLink).to.match(new RegExp(`[?&]updateDetailsToken=${updateDetailsToken}(?:&|$)`));
95+
} else {
96+
expect(runUpdaterLink).to.not.match(/[?&]updateDetailsToken=/);
97+
}
9598
});
9699

97100
it('should login successfully after the update', async function () {

0 commit comments

Comments
 (0)
⚡