Skip to content

Commit 40933c5

Browse files
skjnldsvskjnldsv
authored
Merge pull request #738 from nextcloud/jtr/fix-update-server-response-robustness
fix(updater): harden getUpdateServerResponse with timeouts, retry, and HTTP status validation
2 parents 856af67 + b7f45ac commit 40933c5

File tree

5 files changed

+167
-13
lines changed

5 files changed

+167
-13
lines changed

‎index.php‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -579,28 +579,65 @@ private function getUpdateServerResponse(): array {
579579
$updateURL = $updaterServer . '?version=' . str_replace('.', 'x', $this->getConfigOptionMandatoryString('version')) . 'xxx' . $releaseChannel . 'xx' . urlencode($this->buildTime) . 'x' . PHP_MAJOR_VERSION . 'x' . PHP_MINOR_VERSION . 'x' . PHP_RELEASE_VERSION;
580580
$this->silentLog('[info] updateURL: ' . $updateURL);
581581

582+
$maxRetries = 2;
583+
$lastException = null;
584+
585+
for ($attempt = 1; $attempt <= $maxRetries; $attempt++) {
586+
try {
587+
return $this->fetchUpdateServerResponse($updateURL);
588+
} catch (\Exception $e) {
589+
$lastException = $e;
590+
$this->silentLog('[warn] attempt ' . $attempt . '/' . $maxRetries . ' failed: ' . $e->getMessage());
591+
if ($attempt < $maxRetries) {
592+
sleep(1);
593+
}
594+
}
595+
}
596+
597+
throw $lastException;
598+
}
599+
600+
/**
601+
* @throws \Exception
602+
*/
603+
private function fetchUpdateServerResponse(string $updateURL): array {
582604
// Download update response
583605
$curl = $this->getCurl($updateURL);
606+
curl_setopt($curl, CURLOPT_CONNECTTIMEOUT, 10);
607+
curl_setopt($curl, CURLOPT_TIMEOUT, 30);
584608

585609
/** @var false|string $response */
586610
$response = curl_exec($curl);
611+
587612
if ($response === false) {
588-
throw new \Exception('Could not do request to updater server: ' . curl_error($curl));
613+
$curlError = curl_error($curl);
614+
curl_close($curl);
615+
throw new \Exception('Could not do request to updater server: ' . $curlError);
589616
}
590617

618+
$httpCode = curl_getinfo($curl, CURLINFO_HTTP_CODE);
591619
curl_close($curl);
592620

621+
if ($httpCode !== 200 && $httpCode !== 204) {
622+
$this->silentLog('[warn] update server returned HTTP ' . $httpCode);
623+
throw new \Exception('Update server returned unexpected HTTP status ' . $httpCode);
624+
}
625+
593626
// Response can be empty when no update is available
594627
if ($response === '') {
595628
return [];
596629
}
597630

598631
libxml_use_internal_errors(true);
599-
$xml = simplexml_load_string($response);
600-
if ($xml === false) {
601-
$content = strlen($response) > 200 ? substr($response, 0, 200) . '…' : $response;
602-
$errors = implode("\n", array_map(fn ($error) => $error->message, libxml_get_errors()));
603-
throw new \Exception('Could not parse updater server XML response: ' . $content . "\nErrors:\n" . $errors);
632+
try {
633+
$xml = simplexml_load_string($response);
634+
if ($xml === false) {
635+
$content = strlen($response) > 200 ? substr($response, 0, 200) . '…' : $response;
636+
$errors = implode("\n", array_map(fn ($error) => $error->message, libxml_get_errors()));
637+
throw new \Exception('Could not parse updater server XML response: ' . $content . "\nErrors:\n" . $errors);
638+
}
639+
} finally {
640+
libxml_clear_errors();
604641
}
605642

606643
$response = get_object_vars($xml);

‎lib/Updater.php‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -563,28 +563,65 @@ private function getUpdateServerResponse(): array {
563563
$updateURL = $updaterServer . '?version=' . str_replace('.', 'x', $this->getConfigOptionMandatoryString('version')) . 'xxx' . $releaseChannel . 'xx' . urlencode($this->buildTime) . 'x' . PHP_MAJOR_VERSION . 'x' . PHP_MINOR_VERSION . 'x' . PHP_RELEASE_VERSION;
564564
$this->silentLog('[info] updateURL: ' . $updateURL);
565565

566+
$maxRetries = 2;
567+
$lastException = null;
568+
569+
for ($attempt = 1; $attempt <= $maxRetries; $attempt++) {
570+
try {
571+
return $this->fetchUpdateServerResponse($updateURL);
572+
} catch (\Exception $e) {
573+
$lastException = $e;
574+
$this->silentLog('[warn] attempt ' . $attempt . '/' . $maxRetries . ' failed: ' . $e->getMessage());
575+
if ($attempt < $maxRetries) {
576+
sleep(1);
577+
}
578+
}
579+
}
580+
581+
throw $lastException;
582+
}
583+
584+
/**
585+
* @throws \Exception
586+
*/
587+
private function fetchUpdateServerResponse(string $updateURL): array {
566588
// Download update response
567589
$curl = $this->getCurl($updateURL);
590+
curl_setopt($curl, CURLOPT_CONNECTTIMEOUT, 10);
591+
curl_setopt($curl, CURLOPT_TIMEOUT, 30);
568592

569593
/** @var false|string $response */
570594
$response = curl_exec($curl);
595+
571596
if ($response === false) {
572-
throw new \Exception('Could not do request to updater server: ' . curl_error($curl));
597+
$curlError = curl_error($curl);
598+
curl_close($curl);
599+
throw new \Exception('Could not do request to updater server: ' . $curlError);
573600
}
574601

602+
$httpCode = curl_getinfo($curl, CURLINFO_HTTP_CODE);
575603
curl_close($curl);
576604

605+
if ($httpCode !== 200 && $httpCode !== 204) {
606+
$this->silentLog('[warn] update server returned HTTP ' . $httpCode);
607+
throw new \Exception('Update server returned unexpected HTTP status ' . $httpCode);
608+
}
609+
577610
// Response can be empty when no update is available
578611
if ($response === '') {
579612
return [];
580613
}
581614

582615
libxml_use_internal_errors(true);
583-
$xml = simplexml_load_string($response);
584-
if ($xml === false) {
585-
$content = strlen($response) > 200 ? substr($response, 0, 200) . '…' : $response;
586-
$errors = implode("\n", array_map(fn ($error) => $error->message, libxml_get_errors()));
587-
throw new \Exception('Could not parse updater server XML response: ' . $content . "\nErrors:\n" . $errors);
616+
try {
617+
$xml = simplexml_load_string($response);
618+
if ($xml === false) {
619+
$content = strlen($response) > 200 ? substr($response, 0, 200) . '…' : $response;
620+
$errors = implode("\n", array_map(fn ($error) => $error->message, libxml_get_errors()));
621+
throw new \Exception('Could not parse updater server XML response: ' . $content . "\nErrors:\n" . $errors);
622+
}
623+
} finally {
624+
libxml_clear_errors();
588625
}
589626

590627
$response = get_object_vars($xml);

‎tests/features/bootstrap/FeatureContext.php‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ public function theCliUpdaterIsRun() {
204204
copy($this->buildDir . 'updater.phar', $this->serverDir . 'nextcloud/updater/updater');
205205
chdir($this->serverDir . 'nextcloud/updater');
206206
chmod($this->serverDir . 'nextcloud/updater/updater', 0755);
207-
exec('./updater -n', $output, $returnCode);
207+
exec('./updater -n 2>&1', $output, $returnCode);
208208

209209
// sleep to let the opcache do it's work and invalidate the status.php
210210
sleep(5);
@@ -564,4 +564,54 @@ public function thereIsAConfigForASecondaryAppsDirectoryCalled($name) {
564564
public function phpIsAtLeastInVersion($version) {
565565
$this->skipIt = in_array(version_compare($version, PHP_VERSION, '<'), [0, false], true);
566566
}
567+
568+
/**
569+
* @Given the update server returns HTTP status :statusCode
570+
*/
571+
public function theUpdateServerReturnsHttpStatus(int $statusCode) {
572+
if ($this->skipIt) {
573+
return;
574+
}
575+
576+
$this->runUpdateServer();
577+
578+
$content = '<?php http_response_code(' . $statusCode . '); echo "Server Error";';
579+
file_put_contents($this->updateServerDir . 'index.php', $content);
580+
}
581+
582+
/**
583+
* @Given the update server returns invalid XML
584+
*/
585+
public function theUpdateServerReturnsInvalidXml() {
586+
if ($this->skipIt) {
587+
return;
588+
}
589+
590+
$this->runUpdateServer();
591+
592+
$content = '<?php header("Content-Type: application/xml"); echo "this is not valid xml <><><";';
593+
file_put_contents($this->updateServerDir . 'index.php', $content);
594+
}
595+
596+
/**
597+
* @Given the update server is unreachable
598+
*/
599+
public function theUpdateServerIsUnreachable() {
600+
if ($this->skipIt) {
601+
return;
602+
}
603+
604+
// Point updater.server.url at a port with nothing listening
605+
$configFile = $this->serverDir . 'nextcloud/config/config.php';
606+
$content = file_get_contents($configFile);
607+
$content = preg_replace(
608+
'!\$CONFIG\s*=\s*array\s*\(!',
609+
"\$CONFIG = array(\n 'updater.server.url' => 'http://localhost:8871/',",
610+
$content
611+
);
612+
file_put_contents($configFile, $content);
613+
614+
// Intentionally do NOT start any server on port 8871
615+
}
567616
}
617+

‎tests/features/cli.feature‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,33 @@ Feature: CLI updater
8484
And the installed version should be 26.0.0
8585
And maintenance mode should be off
8686
And upgrade is not required
87+
88+
Scenario: Update server returns HTTP 500 - 26.0.0
89+
Given the current installed version is 26.0.0
90+
And the update server returns HTTP status 500
91+
When the CLI updater is run
92+
Then the return code should not be 0
93+
And the output should contain "Update server returned unexpected HTTP status 500"
94+
And the installed version should be 26.0.0
95+
And maintenance mode should be off
96+
And upgrade is not required
97+
98+
Scenario: Update server returns invalid XML - 26.0.0
99+
Given the current installed version is 26.0.0
100+
And the update server returns invalid XML
101+
When the CLI updater is run
102+
Then the return code should not be 0
103+
And the output should contain "Could not parse updater server XML response"
104+
And the installed version should be 26.0.0
105+
And maintenance mode should be off
106+
And upgrade is not required
107+
108+
Scenario: Update server is unreachable - 26.0.0
109+
Given the current installed version is 26.0.0
110+
And the update server is unreachable
111+
When the CLI updater is run
112+
Then the return code should not be 0
113+
And the output should contain "Could not do request to updater server"
114+
And the installed version should be 26.0.0
115+
And maintenance mode should be off
116+
And upgrade is not required

‎updater.phar‎

173 KB
Binary file not shown.

0 commit comments

Comments
 (0)