From 6e8d60a4f582efb6c8fab47fc46348416edac38a Mon Sep 17 00:00:00 2001 From: Zack Galbreath Date: Mon, 22 Apr 2024 14:25:24 -0400 Subject: [PATCH] Fix upload_handler for remote workers (#2147) 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 --- .../Commands/MakeStorageDirectories.php | 1 + app/Http/Controllers/SubmissionController.php | 32 +++++++++++ app/cdash/xml_handlers/upload_handler.php | 55 +++++++++++++++---- phpstan-baseline.neon | 12 +--- routes/api.php | 2 + tests/Feature/RemoteWorkers.php | 23 ++++++++ 6 files changed, 102 insertions(+), 23 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/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..6f05be6634 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: @@ -119,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; @@ -141,8 +141,8 @@ public function startElement($parser, $name, $attributes) } } - /** Function endElement - * @throws \Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException + /** + * Function endElement */ public function endElement($parser, $name) { @@ -212,16 +212,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..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 @@ -29262,11 +29257,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 () { 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'); + } }