Introduce new states for XML Validation (#2804)

josephsnyder · web-flow · commit 7a6a61a2a835 · 2025-04-23T17:36:24.000Z
Change the name of the environment variable from
VALIDATE_XML_SUBMISSIONS to VALIDATE_SUBMISSIONS
Change the handling of the XML validation in the environment to be a
string setting instead of a boolean.

The new state should be as follows
No Value or "SILENT": Log a message in the CDash logs and attempt to
parse XML
"WARN" : Log, return a message with a warning label in the
http response of the submission, and attempt to parse XML
"REJECT" : Log and return 400 to prevent further submissions and do not
attempt
                        to process the XML
diff --git a/.env.example b/.env.example
@@ -118,10 +118,16 @@ DB_PASSWORD=secret
 # to some value other than 'sync'.
 #REMOTE_WORKERS=false
 
-# Whether or not submission XML files are validated before being processed.
-# Enabling this setting will trigger a scan through XML files uploaded in
-# each submission in order to preemptively reject malformed files.
-#VALIDATE_XML_SUBMISSIONS=true
+# Whether or not submission files are validated before being processed.
+# CDash will trigger a scan through certain files uploaded in
+# each submission in order to reject malformed files.  This setting controls
+# how much the user is made aware of issues found with the files.
+# Valid values:
+#  SILENT     <--- This is the default
+#  WARN
+#  REJECT
+
+#VALIDATE_SUBMISSIONS=SILENT
 
 # Should we show the most recent submission time for a project or subproject?
 # Disabling this feature can improve rendering performance of index.php
diff --git a/app/Enums/SubmissionValidationType.php b/app/Enums/SubmissionValidationType.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace App\Enums;
+
+enum SubmissionValidationType: int
+{
+    case SILENT = 0;
+    case WARN = 1;
+    case REJECT = 2;
+}
diff --git a/app/Http/Controllers/SubmissionController.php b/app/Http/Controllers/SubmissionController.php
@@ -2,6 +2,7 @@
 
 namespace App\Http\Controllers;
 
+use App\Enums\SubmissionValidationType;
 use App\Exceptions\BadSubmissionException;
 use App\Jobs\ProcessSubmission;
 use App\Models\Site;
@@ -73,7 +74,7 @@ private function submitProcess(): Response
         @set_time_limit(0);
 
         $statusarray = [];
-
+        $responseMessage = '';
         $projectname = $_GET['project'] ?? '';
 
         if (strlen($projectname) === 0) {
@@ -146,34 +147,29 @@ private function submitProcess(): Response
         // Figure out what type of XML file this is.
         $stored_filename = 'inbox/' . $filename;
         $xml_info = [];
-        try {
-            $xml_info = SubmissionUtils::get_xml_type(Storage::readStream($stored_filename), $stored_filename);
-        } catch (BadSubmissionException $e) {
-            $xml_info['xml_handler'] = '';
-            $message = "Could not determine submission file type for: '{$stored_filename}'";
-            Log::warning($message);
-            if ((bool) config('cdash.validate_xml_submissions') === true) {
-                $this->failProcessing($filename, 400, $message);
-            }
-        }
+        $xml_info = SubmissionUtils::get_xml_type(Storage::readStream($stored_filename), $stored_filename);
+
         if ($xml_info['xml_handler'] !== '') {
             // If validation is enabled and if this file has a corresponding schema, validate it
             $validation_errors = [];
             try {
                 $validation_errors = $xml_info['xml_handler']::validate($stored_filename);
             } catch (FileNotFoundException|UnableToReadFile $e) {
-                Log::warning($e->getMessage());
-                if ((bool) config('cdash.validate_xml_submissions') === true) {
-                    $this->failProcessing($filename, 400, "XML validation failed for $filename:" . PHP_EOL . $e->getMessage());
-                }
+                $message = $e->getMessage();
+                report($message);
+                $this->failProcessing($filename, 500, "Unable to read file for validation $filename:" . PHP_EOL . $message);
             }
             if (count($validation_errors) > 0) {
                 $error_string = implode(PHP_EOL, $validation_errors);
-
+                $message = "XML validation failed: Found issues with file $filename:";
                 // We always log validation failures, but we only send messages back to the client if configured to do so
-                Log::warning("Submission validation failed for file '$filename':" . PHP_EOL);
-                if ((bool) config('cdash.validate_xml_submissions') === true) {
-                    $this->failProcessing($filename, 400, "XML validation failed: rejected file $filename:" . PHP_EOL . $error_string);
+                Log::info($message);
+                $fullMessage = $message . PHP_EOL . $error_string;
+                if (config('cdash.validate_submissions') === SubmissionValidationType::REJECT) {
+                    $this->failProcessing($filename, 400, $fullMessage);
+                }
+                if (config('cdash.validate_submissions') === SubmissionValidationType::WARN) {
+                    $responseMessage .= $fullMessage;
                 }
             }
         }
@@ -219,8 +215,12 @@ private function submitProcess(): Response
             Artisan::call('submission:queue');
         }
 
-        $statusarray['status'] = 'OK';
         $statusarray['message'] = '';
+        if ($responseMessage !== '') {
+            $statusarray['message'] = $responseMessage;
+        }
+
+        $statusarray['status'] = 'OK';
         if ($buildid !== null) {
             $statusarray['buildId'] = $buildid;
         }
diff --git a/app/Jobs/ProcessSubmission.php b/app/Jobs/ProcessSubmission.php
@@ -4,6 +4,7 @@
 
 use AbstractSubmissionHandler;
 use ActionableBuildInterface;
+use App\Exceptions\BadSubmissionException;
 use App\Models\SuccessfulJob;
 use App\Utils\UnparsedSubmissionProcessor;
 use BuildPropertiesJSONHandler;
@@ -133,6 +134,8 @@ private function requeueSubmissionFile($buildid): bool
      * Execute the job.
      *
      * @return void
+     *
+     * @throws BadSubmissionException
      */
     public function handle()
     {
@@ -197,6 +200,7 @@ public function failed(Throwable $exception): void
      * This method could be running on a worker that is either remote or local, so it accepts
      * a file handle or a filename that it can query the CDash API for.
      *
+     * @throws BadSubmissionException
      **/
     private function doSubmit($filename, $projectid, $buildid = null, $expected_md5 = ''): AbstractSubmissionHandler|UnparsedSubmissionProcessor|false
     {
diff --git a/app/cdash/include/ctestparser.php b/app/cdash/include/ctestparser.php
@@ -150,6 +150,8 @@ function parse_put_submission(string $filename, int $projectid, ?string $expecte
 
 /**
  * Main function to parse the incoming xml from ctest
+ *
+ * @throws BadSubmissionException
  */
 function ctest_parse($filehandle, string $filename, $projectid, $expected_md5 = '', ?int $buildid = null): AbstractSubmissionHandler|false
 {
@@ -163,19 +165,8 @@ function ctest_parse($filehandle, string $filename, $projectid, $expected_md5 =
     $Project->Id = $projectid;
     $xml_info = [];
     // Figure out what type of XML file this is.
-    try {
-        $xml_info = SubmissionUtils::get_xml_type($filehandle, $filename);
-    } catch (BadSubmissionException $e) {
-        $xml_info['file_handle'] = $filehandle;
-        $xml_info['xml_handler'] = null;
-        $xml_info['xml_type'] = '';
-        $message = "Could not determine submission file type for: '{$filename}'";
-        Log::warning($message);
-        if ((bool) config('cdash.validate_xml_submissions') === true) {
-            abort(400, $message);
-        }
-    }
-    $filehandle = $xml_info['file_handle'];
+    $xml_info = SubmissionUtils::get_xml_type($filehandle, $filename);
+
     $handler_ref = $xml_info['xml_handler'];
     $file = $xml_info['xml_type'];
     $handler = isset($handler_ref) ? new $handler_ref($Project) : null;
diff --git a/app/cdash/xml_handlers/abstract_xml_handler.php b/app/cdash/xml_handlers/abstract_xml_handler.php
@@ -93,7 +93,7 @@ public static function validate(string $path): array
             $validation_errors = libxml_get_errors();
             foreach ($validation_errors as $error) {
                 if ($error->level === LIBXML_ERR_ERROR || $error->level === LIBXML_ERR_FATAL) {
-                    $errors[] = "ERROR: {$error->message} in {$error->file}, line: {$error->line}, column: {$error->column}";
+                    $errors[] = "WARNING: {$error->message} in {$error->file}, line: {$error->line}, column: {$error->column}";
                 }
             }
             libxml_clear_errors();
diff --git a/config/cdash.php b/config/cdash.php
@@ -1,5 +1,8 @@
 <?php
 
+use App\Enums\SubmissionValidationType;
+use Illuminate\Support\Str;
+
 $cdash_directory_name = env('CDASH_DIRECTORY', 'cdash');
 $cdash = realpath(app_path($cdash_directory_name));
 
@@ -64,7 +67,12 @@
     'show_last_submission' => env('SHOW_LAST_SUBMISSION', true),
     'slow_page_time' => env('SLOW_PAGE_TIME', 10),
     'token_duration' => env('TOKEN_DURATION', 15811200),
-    'validate_xml_submissions' => env('VALIDATE_XML_SUBMISSIONS', false),
+    'validate_submissions' => match (Str::upper((string) env('VALIDATE_SUBMISSIONS'))) {
+        'SILENT' => SubmissionValidationType::SILENT,
+        'WARN' => SubmissionValidationType::WARN,
+        'REJECT' => SubmissionValidationType::REJECT,
+        default => SubmissionValidationType::SILENT,
+    },
     // Specify whether users are allowed to create "full access" authentication tokens
     'allow_full_access_tokens' => env('ALLOW_FULL_ACCESS_TOKENS', true),
     // Specify whether users are allowed to create "submit only" tokens which are valid for all projects
diff --git a/docs/submissions.md b/docs/submissions.md
@@ -79,4 +79,10 @@ CDash automatically detects when its database is unavailable and stores submissi
 
 ## Submission files validation
 
-CDash can be configured to validate submitted XML files before attempting to process them, in order to preemptively reject malformed files. If enabled, once a submission is moved out of the queue, its XML files will be scanned for valid syntax and validated against the [XML schemas](../app/Validators/Schemas/) CDash supports. To use this feature, set `VALIDATE_XML_SUBMISSIONS=true` in your `.env` file.
+CDash can be configured to validate submitted XML files before attempting to process them, in order to preemptively reject malformed files. If enabled, once a submission is moved out of the queue, its XML files will be scanned for valid syntax and validated against the [XML schemas](../app/Validators/Schemas/) CDash supports. To use this feature, set `VALIDATE_SUBMISSIONS` in your `.env` file.
+
+      No Value or "SILENT": Log a message in the CDash logs and attempt to parse XML
+      "WARN"              : Log, return a message with a warning label in the
+                            http response of the submission, and attempt to parse XML
+      "REJECT"            : Log and return 400 to prevent further submissions and do not attempt
+                            to process the XML
diff --git a/tests/Feature/SubmissionValidation.php b/tests/Feature/SubmissionValidation.php
@@ -27,7 +27,7 @@ public function setUp(): void
 
     public function writeEnvEntry(string $value): void
     {
-        file_put_contents($this->ConfigFile, "VALIDATE_XML_SUBMISSIONS={$value}\n", FILE_APPEND | LOCK_EX);
+        file_put_contents($this->ConfigFile, "VALIDATE_SUBMISSIONS={$value}\n", FILE_APPEND | LOCK_EX);
     }
 
     public function submit(string $fileName): bool
@@ -54,20 +54,31 @@ public function testSubmissionValidationNoEnv(): void
     /** Check that error messages are logged but submission succeeds
      *  when the environment variable is set but to false
      */
-    public function testSubmissionValidationFalse(): void
+    public function testSubmissionValidationSilent(): void
     {
-        $this->writeEnvEntry('false');
+        $this->writeEnvEntry('SILENT');
+        $this::assertTrue($this->submit('invalid_Configure.xml'), 'Submission of invalid_Configure.xml was not successful when it should have passed.');
+        $this::assertTrue($this->submit('invalid_syntax_Build.xml'), 'Submission of invalid_syntax_Build.xml  was not successful when it should have passed.');
+        $this::assertTrue($this->submit('valid_Build.xml'), 'Submission of valid_Build.xml was not successful when it should have passed.');
+    }
+
+    /** Check that error messages are logged but submission succeeds
+     *  when the environment variable is set but to WARN
+     */
+    public function testSubmissionValidationWarn(): void
+    {
+        $this->writeEnvEntry('WARN');
         $this::assertTrue($this->submit('invalid_Configure.xml'), 'Submission of invalid_Configure.xml was not successful when it should have passed.');
         $this::assertTrue($this->submit('invalid_syntax_Build.xml'), 'Submission of invalid_syntax_Build.xml  was not successful when it should have passed.');
         $this::assertTrue($this->submit('valid_Build.xml'), 'Submission of valid_Build.xml was not successful when it should have passed.');
     }
 
     /** Check that the submission is dependent upon passing validation
-     *  when the environment variable is set
+     *  when the environment variable is set to REJECT
      */
-    public function testSubmissionValidationTrue(): void
+    public function testSubmissionValidationReject(): void
     {
-        $this->writeEnvEntry('true');
+        $this->writeEnvEntry('REJECT');
         $this::assertFalse($this->submit('invalid_Configure.xml'), 'Submission of invalid_Configure.xml was successful when it should have failed.');
         $this::assertFalse($this->submit('invalid_syntax_Build.xml'), 'Submission of invalid_syntax_Build.xml was successful when it should have failed.');
         $this::assertTrue($this->submit('valid_Build.xml'), 'Submission of valid_Build.xml was not successful when it should have passed.');

Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,7 @@`
`4`	`4`
`5`	`5`	`use AbstractSubmissionHandler;`
`6`	`6`	`use ActionableBuildInterface;`
	`7`	`+use App\Exceptions\BadSubmissionException;`
`7`	`8`	`use App\Models\SuccessfulJob;`
`8`	`9`	`use App\Utils\UnparsedSubmissionProcessor;`
`9`	`10`	`use BuildPropertiesJSONHandler;`
`@@ -133,6 +134,8 @@ private function requeueSubmissionFile($buildid): bool`
`133`	`134`	`* Execute the job.`
`134`	`135`	`*`
`135`	`136`	`* @return void`
	`137`	`+ *`
	`138`	`+ * @throws BadSubmissionException`
`136`	`139`	`*/`
`137`	`140`	`public function handle()`
`138`	`141`	`{`
`@@ -197,6 +200,7 @@ public function failed(Throwable $exception): void`
`197`	`200`	`* This method could be running on a worker that is either remote or local, so it accepts`
`198`	`201`	`* a file handle or a filename that it can query the CDash API for.`
`199`	`202`	`*`
	`203`	`+ * @throws BadSubmissionException`
`200`	`204`	`**/`
`201`	`205`	`private function doSubmit($filename, $projectid, $buildid = null, $expected_md5 = ''): AbstractSubmissionHandler\|UnparsedSubmissionProcessor\|false`
`202`	`206`	`{`
Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ public static function validate(string $path): array`
`93`	`93`	`$validation_errors = libxml_get_errors();`
`94`	`94`	`foreach ($validation_errors as $error) {`
`95`	`95`	`if ($error->level === LIBXML_ERR_ERROR \|\| $error->level === LIBXML_ERR_FATAL) {`
`96`		`- $errors[] = "ERROR: {$error->message} in {$error->file}, line: {$error->line}, column: {$error->column}";`
	`96`	`+ $errors[] = "WARNING: {$error->message} in {$error->file}, line: {$error->line}, column: {$error->column}";`
`97`	`97`	`}`
`98`	`98`	`}`
`99`	`99`	`libxml_clear_errors();`