Skip to content

Commit 063e7f6

Browse files
Alexey PortnovAlexey Portnov
authored andcommitted
fix(files): validate firmware download input and repair UI param name (#1004)
DownloadNewFirmwareAction read $data['url']/['md5']/['size'] with no checks, producing "Undefined array key" warnings (Sentry #27054, 48 events). The frontend was also sending params.updateLink while the backend expected url, so downloads triggered from the UI never actually worked. Validate url (http/https) and md5 (32 hex) up-front and return HTTP 400 on bad input. Normalize md5 to lowercase since WorkerDownloader compares against md5_file() output. Drop the dead 'size' field from download_settings.json: WorkerDownloader reads file size from the Content-Length header and never touches settings['size']. Update the UI to send params.url, and mark md5 as required in the OpenAPI attribute to match actual worker behavior.
1 parent 4bab4a6 commit 063e7f6

4 files changed

Lines changed: 39 additions & 18 deletions

File tree

sites/admin-cabinet/assets/js/pbx/Update/update-index.js

Lines changed: 2 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

sites/admin-cabinet/assets/js/src/Update/update-index.js

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,10 +184,9 @@ const updatePBX = {
184184
// Prepare parameters for firmware download
185185
const params = {};
186186
const $aLink = $(e.target).closest('a');
187-
params.updateLink = $aLink.attr('href');
187+
params.url = $aLink.attr('href');
188188
params.md5 = $aLink.attr('data-md5');
189189
params.version = $aLink.attr('data-version');
190-
params.size = $aLink.attr('data-size');
191190
$aLink.find('i').addClass('loading');
192191
updatePBX.upgradeInProgress = true;
193192
FilesAPI.downloadFirmware(params, updatePBX.cbAfterStartDownloadFirmware);

src/PBXCoreREST/Controllers/Files/RestController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ public function uploadStatus(): void
247247
operationId: 'downloadFirmware'
248248
)]
249249
#[ApiParameterRef('url', required: true)]
250-
#[ApiParameterRef('md5')]
250+
#[ApiParameterRef('md5', required: true)]
251251
#[ApiResponse(200, 'rest_response_200_firmware_downloading')]
252252
#[ApiResponse(400, 'rest_response_400_bad_request', 'PBXApiResult')]
253253
#[ApiResponse(401, 'rest_response_401_unauthorized', 'PBXApiResult')]

src/PBXCoreREST/Lib/Files/DownloadNewFirmwareAction.php

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,25 +39,51 @@ class DownloadNewFirmwareAction extends Injectable
3939
* Downloads the firmware file from the provided URL.
4040
*
4141
* @param array $data The data array containing the following parameters:
42-
* - Md5: The MD5 hash of the file.
43-
* - size: The size of the file.
44-
* - version: The version of the file.
45-
* - Url: The download URL of the file.
42+
* - url: The download URL of the file (required).
43+
* - md5: The MD5 hash of the file (required, used for integrity verification).
44+
* - version: The version of the file (optional, used as directory name).
4645
*
4746
* @return PBXApiResult An object containing the result of the API call.
4847
*/
4948
public static function main(array $data): PBXApiResult
5049
{
50+
$res = new PBXApiResult();
51+
$res->processor = __METHOD__;
52+
53+
$url = is_string($data['url'] ?? null) ? trim($data['url']) : '';
54+
$md5 = is_string($data['md5'] ?? null) ? trim($data['md5']) : '';
55+
56+
if ($url === '') {
57+
$res->success = false;
58+
$res->messages['error'][] = 'Download URL is required';
59+
$res->httpCode = 400;
60+
return $res;
61+
}
62+
if (!filter_var($url, FILTER_VALIDATE_URL) || !preg_match('#^https?://#i', $url)) {
63+
$res->success = false;
64+
$res->messages['error'][] = 'Download URL must be a valid http(s) URL';
65+
$res->httpCode = 400;
66+
return $res;
67+
}
68+
if ($md5 === '' || !preg_match('/^[a-f0-9]{32}$/i', $md5)) {
69+
$res->success = false;
70+
$res->messages['error'][] = 'A valid MD5 hash (32 hex characters) is required';
71+
$res->httpCode = 400;
72+
return $res;
73+
}
74+
// Normalize to lowercase: WorkerDownloader compares against md5_file() which returns lowercase.
75+
$md5 = strtolower($md5);
76+
5177
$di = Di::getDefault();
5278
if ($di !== null) {
5379
$uploadDir = $di->getConfig()->path('www.uploadDir');
5480
} else {
5581
$uploadDir = '/tmp';
5682
}
57-
$version = $data['version'] ?? $data['md5'] ?? (string)time();
83+
$version = $data['version'] ?? $md5;
5884
// Security: sanitize version to prevent shell injection via directory path
59-
$version = preg_replace('/[^a-zA-Z0-9._\-]/', '', $version);
60-
if (empty($version)) {
85+
$version = preg_replace('/[^a-zA-Z0-9._\-]/', '', (string)$version);
86+
if ($version === '') {
6187
$version = (string)time();
6288
}
6389
$firmwareDirTmp = "$uploadDir/{$version}";
@@ -71,9 +97,8 @@ public static function main(array $data): PBXApiResult
7197

7298
$download_settings = [
7399
'res_file' => "$firmwareDirTmp/update.img",
74-
'url' => $data['url'],
75-
'size' => $data['size'],
76-
'md5' => $data['md5'],
100+
'url' => $url,
101+
'md5' => $md5,
77102
];
78103

79104
$workerDownloaderPath = Util::getFilePathByClassName(WorkerDownloader::class);
@@ -84,8 +109,6 @@ public static function main(array $data): PBXApiResult
84109
$php = Util::which('php');
85110
Processes::mwExecBg("$php -f $workerDownloaderPath start " . escapeshellarg("$firmwareDirTmp/download_settings.json"));
86111

87-
$res = new PBXApiResult();
88-
$res->processor = __METHOD__;
89112
$res->success = true;
90113
$res->data['filename'] = $download_settings['res_file'];
91114
$res->data[FilesConstants::D_STATUS] = FilesConstants::DOWNLOAD_IN_PROGRESS;

0 commit comments

Comments
 (0)