From 1a55f24b8ab016d26f0bb9bec8495a154ddadf08 Mon Sep 17 00:00:00 2001 From: Zack Galbreath Date: Thu, 18 Apr 2024 12:49:24 -0400 Subject: [PATCH 1/3] Fix upload_handler for remote workers Use an internal, secured API endpoint to store files parsed from Upload.xml when CDash is configured to use remote workers. Follow-up to #2143 --- app/Http/Controllers/SubmissionController.php | 32 ++++++++++ app/cdash/xml_handlers/upload_handler.php | 59 ++++++++++++++++--- phpstan-baseline.neon | 5 -- routes/api.php | 2 + 4 files changed, 85 insertions(+), 13 deletions(-) diff --git a/app/Http/Controllers/SubmissionController.php b/app/Http/Controllers/SubmissionController.php index 9259805032..778aaed4ec 100644 --- a/app/Http/Controllers/SubmissionController.php +++ b/app/Http/Controllers/SubmissionController.php @@ -10,7 +10,9 @@ use CDash\Model\PendingSubmissions; use CDash\Model\Project; use Exception; +use Illuminate\Contracts\Encryption\DecryptException; use Illuminate\Http\JsonResponse; +use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\DB; @@ -182,4 +184,34 @@ private static function displayXMLReturnStatus(array $statusarray, int $http_cod ->view('submission.xml-response', ['statusarray' => $statusarray], $http_code) ->header('Content-Type', 'text/xml'); } + + public function storeUploadedFile(Request $request): Response + { + if (! (bool) config('cdash.remote_workers')) { + return response('This feature is disabled', Response::HTTP_CONFLICT); + } + + if (!$request->has('sha1sum')) { + return response('Bad request', Response::HTTP_BAD_REQUEST); + } + + try { + $sha1sum = decrypt($request->input('sha1sum')); + } catch (DecryptException $e) { + return response('This feature is disabled', Response::HTTP_CONFLICT); + } + + $uploaded_file = array_values(request()->allFiles())[0]; + $stored_path = $uploaded_file->storeAs('upload', $sha1sum); + if ($stored_path === false) { + abort(Response::HTTP_INTERNAL_SERVER_ERROR, 'Failed to store uploaded file'); + } + + if (sha1_file(Storage::path($stored_path)) !== $sha1sum) { + Storage::delete($stored_path); + return response('Uploaded file does not match expected sha1sum', Response::HTTP_BAD_REQUEST); + } + + return response('OK', Response::HTTP_OK); + } } diff --git a/app/cdash/xml_handlers/upload_handler.php b/app/cdash/xml_handlers/upload_handler.php index 631d62b7ea..22a043eac5 100644 --- a/app/cdash/xml_handlers/upload_handler.php +++ b/app/cdash/xml_handlers/upload_handler.php @@ -27,8 +27,10 @@ use Illuminate\Http\File; use Illuminate\Support\Carbon; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; +use Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException; /** * For each uploaded file the following steps occur: @@ -141,8 +143,8 @@ public function startElement($parser, $name, $attributes) } } - /** Function endElement - * @throws \Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException + /** + * Function endElement */ public function endElement($parser, $name) { @@ -165,6 +167,16 @@ public function endElement($parser, $name) // Note: Using stream_filter_append/stream_copy_to_stream is more efficient but // return an "invalid byte sequence" on windows $rhandle = fopen($this->Base64TmpFilename, 'r'); + + // Create tmp file + $this->TmpFilename = tempnam(sys_get_temp_dir(), 'cdash_upload'); + if ($this->TmpFilename === false) { + Log::error('Failed to create temporary filename'); + $this->UploadError = true; + return; + } + chmod($this->TmpFilename, 0o644); + $whandle = fopen($this->TmpFilename, 'w+'); $chunksize = 4096; while (!feof($rhandle)) { @@ -212,16 +224,47 @@ public function endElement($parser, $name) } else { $this->UploadFile->IsUrl = false; - // Store the file if we don't already have it. - $upload_filepath = "upload/{$this->UploadFile->Sha1Sum}"; - if (!Storage::exists($upload_filepath)) { - $result = Storage::putFileAs('upload', new File($this->TmpFilename), $this->UploadFile->Sha1Sum); - if ($result === false) { - Log::error("Failed to store {$this->TmpFilename} as {$upload_filepath}"); + if ((bool) config('cdash.remote_workers')) { + // Make an API request to store this file. + $encrypted_sha1sum = encrypt($this->UploadFile->Sha1Sum); + $fp_to_upload = fopen($this->TmpFilename, 'r'); + if ($fp_to_upload === false) { + Log::error("Failed to open temporary file {$this->TmpFilename} for upload"); + cdash_unlink($this->TmpFilename); $this->UploadError = true; + return; + } + $response = Http::attach( + 'attachment', $fp_to_upload, (string) $this->UploadFile->Sha1Sum + )->post(url('/api/v1/store_upload'), [ + 'sha1sum' => $encrypted_sha1sum, + ]); + fclose($fp_to_upload); + if (!$response->successful()) { + Log::error('Error uploading file via API: ' . $response->status() . ' ' . $response->body()); cdash_unlink($this->TmpFilename); + $this->UploadError = true; return; } + } else { + // Store the file if we don't already have it. + $uploadFilepath = "upload/{$this->UploadFile->Sha1Sum}"; + if (!Storage::exists($uploadFilepath)) { + try { + $fileToUpload = new File($this->TmpFilename); + } catch (FileNotFoundException $e) { + Log::error("Could not find file {$this->TmpFilename} to upload"); + cdash_unlink($this->TmpFilename); + $this->UploadError = true; + return; + } + if (Storage::putFileAs('upload', $fileToUpload, (string) $this->UploadFile->Sha1Sum) === false) { + Log::error("Failed to store {$this->TmpFilename} as {$uploadFilepath}"); + cdash_unlink($this->TmpFilename); + $this->UploadError = true; + return; + } + } } } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 0a55c41670..d2bec53b14 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -29262,11 +29262,6 @@ parameters: count: 1 path: app/cdash/xml_handlers/upload_handler.php - - - message: "#^Parameter \\#3 \\$name of static method Illuminate\\\\Filesystem\\\\FilesystemAdapter\\:\\:putFileAs\\(\\) expects array\\|string\\|null, string\\|false given\\.$#" - count: 1 - path: app/cdash/xml_handlers/upload_handler.php - - message: "#^Property UploadHandler\\:\\:\\$Base64TmpFileWriteHandle has no type specified\\.$#" count: 1 diff --git a/routes/api.php b/routes/api.php index 701f656166..08517178c7 100755 --- a/routes/api.php +++ b/routes/api.php @@ -70,6 +70,8 @@ Route::get('/v1/testOverview.php', 'TestController@apiTestOverview'); +Route::post('/v1/store_upload', 'SubmissionController@storeUploadedFile'); + Route::match(['get', 'post', 'delete'], '/v1/expectedbuild.php', 'ExpectedBuildController@apiResponse'); Route::middleware(['auth'])->group(function () { From b6a514124ce39814ca4ef3065f1ff85b65c7ed48 Mon Sep 17 00:00:00 2001 From: Zack Galbreath Date: Mon, 22 Apr 2024 12:00:40 -0400 Subject: [PATCH 2/3] Use Laravel Storage for temporary upload files --- app/Console/Commands/MakeStorageDirectories.php | 1 + app/cdash/xml_handlers/upload_handler.php | 16 ++-------------- phpstan-baseline.neon | 7 +------ 3 files changed, 4 insertions(+), 20 deletions(-) diff --git a/app/Console/Commands/MakeStorageDirectories.php b/app/Console/Commands/MakeStorageDirectories.php index 98dbdc6c9e..2ed4c71457 100644 --- a/app/Console/Commands/MakeStorageDirectories.php +++ b/app/Console/Commands/MakeStorageDirectories.php @@ -34,6 +34,7 @@ public function handle() "{$storage_path}/app/inprogress", "{$storage_path}/app/parsed", "{$storage_path}/app/public", + "{$storage_path}/app/tmp", "{$storage_path}/app/upload", "{$storage_path}/framework/cache/data", "{$storage_path}/framework/sessions", diff --git a/app/cdash/xml_handlers/upload_handler.php b/app/cdash/xml_handlers/upload_handler.php index 22a043eac5..6f05be6634 100644 --- a/app/cdash/xml_handlers/upload_handler.php +++ b/app/cdash/xml_handlers/upload_handler.php @@ -121,10 +121,8 @@ public function startElement($parser, $name, $attributes) } // Create tmp file - $this->TmpFilename = tempnam(sys_get_temp_dir(), 'cdash_upload'); // TODO Handle error - chmod($this->TmpFilename, 0o644); - - if (empty($this->TmpFilename)) { + $this->TmpFilename = tempnam(storage_path('app/tmp'), 'cdash_upload'); + if ($this->TmpFilename === false) { Log::error('Failed to create temporary filename'); $this->UploadError = true; return; @@ -167,16 +165,6 @@ public function endElement($parser, $name) // Note: Using stream_filter_append/stream_copy_to_stream is more efficient but // return an "invalid byte sequence" on windows $rhandle = fopen($this->Base64TmpFilename, 'r'); - - // Create tmp file - $this->TmpFilename = tempnam(sys_get_temp_dir(), 'cdash_upload'); - if ($this->TmpFilename === false) { - Log::error('Failed to create temporary filename'); - $this->UploadError = true; - return; - } - chmod($this->TmpFilename, 0o644); - $whandle = fopen($this->TmpFilename, 'w+'); $chunksize = 4096; while (!feof($rhandle)) { diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d2bec53b14..9e70a3ad85 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -29139,7 +29139,7 @@ parameters: - message: "#^Construct empty\\(\\) is not allowed\\. Use more strict comparison\\.$#" - count: 3 + count: 2 path: app/cdash/xml_handlers/upload_handler.php - @@ -29217,11 +29217,6 @@ parameters: count: 1 path: app/cdash/xml_handlers/upload_handler.php - - - message: "#^Parameter \\#1 \\$filename of function chmod expects string, string\\|false given\\.$#" - count: 1 - path: app/cdash/xml_handlers/upload_handler.php - - message: "#^Parameter \\#1 \\$stream of function fclose expects resource, resource\\|false given\\.$#" count: 2 From cbc1d7f260b67ef2c8b40e6713808d57cd1a8980 Mon Sep 17 00:00:00 2001 From: Zack Galbreath Date: Mon, 22 Apr 2024 14:01:21 -0400 Subject: [PATCH 3/3] tests for remote uploads --- tests/Feature/RemoteWorkers.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/Feature/RemoteWorkers.php b/tests/Feature/RemoteWorkers.php index 70028bd8d7..18017e8125 100644 --- a/tests/Feature/RemoteWorkers.php +++ b/tests/Feature/RemoteWorkers.php @@ -2,6 +2,7 @@ namespace Tests\Feature; +use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Storage; use Illuminate\Support\Facades\URL; @@ -43,4 +44,26 @@ public function testRemoteWorkerAPIAccessWithInvalidKey() : void self::assertTrue(Storage::exists('inbox/delete_me')); Storage::delete('inbox/delete_me'); } + + public function testStoreUploadedFile() : void + { + Storage::fake('upload'); + $file = UploadedFile::fake()->image('my_upload.jpg'); + + // Unencrypted case. + $response = $this->post('/api/v1/store_upload', [ + 'sha1sum' => 'asdf', + 'file' => $file, + ]); + $response->assertConflict(); + $response->assertSeeText('This feature is disabled'); + + // Encrypted but sha mismatch. + $response = $this->post('/api/v1/store_upload', [ + 'sha1sum' => encrypt('asdf'), + 'file' => $file, + ]); + $response->assertBadRequest(); + $response->assertSeeText('Uploaded file does not match expected sha1sum'); + } }