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

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",

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);
+    }
 }

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;
+                        }
+                    }
                 }
             }
 

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

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 () {

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');
+    }
 }