Allow test names to contain "<" or ">" characters (Kitware#2972)

zackgalbreath · web-flow · commit 870e1ee828e9 · 2025-07-09T20:35:31.000Z
We recently received a bug report where test names were not being parsed correctly. https://open.cdash.org/viewTest.php?onlyfailed&buildid=10510619 The issue is that testing_handler's `text()` method gets called multiple times for the test name's character data when it includes escaped XML characters. The solution is to append (rather than replace) the value of `$testName` when this method gets called multiple times.
diff --git a/app/Http/Controllers/TestController.php b/app/Http/Controllers/TestController.php
@@ -174,8 +174,8 @@ public function apiTestSummary(): JsonResponse|StreamedResponse
         }
         $this->setProjectById(intval($_GET['project'] ?? -1));
 
-        $testName = htmlspecialchars($_GET['name'] ?? '');
-        if ($testName === '') {
+        $testName = $_GET['name'] ?? '';
+        if ($testName === '' || !is_string($testName)) {
             abort(400, 'No test name specified.');
         }
 
diff --git a/app/Utils/TestCreator.php b/app/Utils/TestCreator.php
@@ -43,7 +43,7 @@ class TestCreator
     public $testCommand;
     public $testDetails;
     public $testOutput;
-    private $testName;
+    public $testName;
     public $testPath;
     public $testStatus;
 
@@ -135,19 +135,14 @@ public function computeCrc32(): int
         return crc32($crc32_input);
     }
 
-    /**
-     * Set test name, truncated to 255 characters.
-     */
-    public function setTestName(string $testName): void
-    {
-        $this->testName = substr($testName, 0, 255);
-    }
-
     /**
      * Record this test in the database.
      **/
     public function create(Build $build): void
     {
+        // Truncate testName to 255 characters.
+        $this->testName = substr($this->testName, 0, 255);
+
         $crc32 = $this->computeCrc32();
 
         // TODO: Convert this to Eloquent
diff --git a/app/cdash/include/filterdataFunctions.php b/app/cdash/include/filterdataFunctions.php
@@ -983,7 +983,7 @@ function parse_filter_from_request($field_var, $compare_var, $value_var,
     $fieldinfo = explode('/', $fieldinfo, 2);
     $field = $fieldinfo[0];
     $compare = htmlspecialchars(pdo_real_escape_string($_REQUEST[$compare_var]));
-    $value = htmlspecialchars(pdo_real_escape_string($_REQUEST[$value_var]));
+    $value = pdo_real_escape_string($_REQUEST[$value_var]);
 
     // The following filter types are considered 'date clauses' so that the
     // default date clause of "builds from today only" is not used...
diff --git a/app/cdash/tests/CMakeLists.txt b/app/cdash/tests/CMakeLists.txt
@@ -218,6 +218,7 @@ add_feature_test(/Feature/GraphQL/BuildCommandTypeTest)
 add_feature_test(/Feature/GraphQL/BuildCommandOutputTypeTest)
 
 add_feature_test(/Feature/Submission/Instrumentation/BuildInstrumentationTest)
+add_feature_test(/Feature/Submission/Tests/TestXMLTest)
 
 add_feature_test(/Feature/RouteAccessTest)
 
diff --git a/app/cdash/tests/test_buildproperties.php b/app/cdash/tests/test_buildproperties.php
@@ -63,7 +63,7 @@ public function testUploadBuildProperties()
         $testcreator = new TestCreator();
         $testcreator->projectid = $this->Project->Id;
         $testcreator->testDetails = '';
-        $testcreator->setTestName('BuildPropUnitTest');
+        $testcreator->testName = 'BuildPropUnitTest';
         $testcreator->testPath = '/tmp';
         $testcreator->testCommand = 'echo foo';
         $testcreator->testOutput = 'foo';
diff --git a/app/cdash/tests/test_removebuilds.php b/app/cdash/tests/test_removebuilds.php
@@ -262,7 +262,7 @@ public function testBuildRemovalWorksAsExpected(): void
         $test_creator->projectid = 1;
         $test_creator->alreadyCompressed = false;
         $test_creator->testDetails = 'Completed';
-        $test_creator->setTestName('removal test');
+        $test_creator->testName = 'removal test';
         $test_creator->testPath = '/path/to/removal/test';
         $test_creator->testCommand = 'php test_removebuilds.php';
         $test_creator->testOutput = 'build removed successfully';
@@ -291,7 +291,7 @@ public function testBuildRemovalWorksAsExpected(): void
         $test_creator2->projectid = 1;
         $test_creator2->alreadyCompressed = false;
         $test_creator2->testDetails = 'Completed';
-        $test_creator2->setTestName('shared test');
+        $test_creator2->testName = 'shared test';
         $test_creator2->testPath = '/path/to/shared/test';
         $test_creator2->testCommand = 'php test_sharedbuilds.php';
         $test_creator2->testOutput = 'build shared successfully';
diff --git a/app/cdash/xml_handlers/BazelJSON_handler.php b/app/cdash/xml_handlers/BazelJSON_handler.php
@@ -172,7 +172,7 @@ public function Parse($filename)
             $testCreator->projectid = $this->GetProject()->Id;
             $testCreator->testCommand = $this->CommandLine;
             $testCreator->testDetails = $testdata->details;
-            $testCreator->setTestName($testdata->name);
+            $testCreator->testName = $testdata->name;
             $testCreator->testStatus = $testdata->status;
 
             if (array_key_exists($testdata->name, $this->TestsOutput)) {
diff --git a/app/cdash/xml_handlers/testing_handler.php b/app/cdash/xml_handlers/testing_handler.php
@@ -283,10 +283,10 @@ public function text($parser, $data)
         } elseif ($parent == 'TEST') {
             switch ($element) {
                 case 'NAME':
-                    $this->TestCreator->setTestName($data);
+                    $this->TestCreator->testName .= $data;
                     break;
                 case 'PATH':
-                    $this->TestCreator->testPath = $data;
+                    $this->TestCreator->testPath .= $data;
                     break;
                 case 'FULLCOMMANDLINE':
                     $this->TestCreator->testCommand .= $data;
diff --git a/app/cdash/xml_handlers/testing_j_unit_handler.php b/app/cdash/xml_handlers/testing_j_unit_handler.php
@@ -163,7 +163,7 @@ public function startElement($parser, $name, $attributes): void
                 }
             }
 
-            $this->TestCreator->setTestName($attributes['NAME']);
+            $this->TestCreator->testName = $attributes['NAME'];
             $this->TestCreator->testPath = $attributes['CLASSNAME'];
         } elseif ($name == 'TESTSUITE') {
             // If the XML file doesn't have a <Site> tag then we use the information
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -2715,7 +2715,7 @@ parameters:
 		-
 			message: '#^Parameter \#1 \$string of function htmlspecialchars expects string, mixed given\.$#'
 			identifier: argument.type
-			count: 3
+			count: 2
 			path: app/Http/Controllers/TestController.php
 
 		-
diff --git a/tests/Feature/Submission/Tests/TestXMLTest.php b/tests/Feature/Submission/Tests/TestXMLTest.php
@@ -0,0 +1,93 @@
+<?php
+
+namespace Tests\Feature\Submission\Instrumentation;
+
+use App\Models\Project;
+use Tests\TestCase;
+use Tests\Traits\CreatesProjects;
+use Tests\Traits\CreatesSubmissions;
+
+class TestXMLTest extends TestCase
+{
+    use CreatesProjects;
+    use CreatesSubmissions;
+
+    private Project $project;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->project = $this->makePublicProject();
+
+        // The trait doesn't initialize the default buildgroups for us, so we do it manually
+        $legacy_project = new \CDash\Model\Project();
+        $legacy_project->Id = $this->project->id;
+        $legacy_project->InitialSetup();
+
+        $this->project->refresh();
+    }
+
+    protected function tearDown(): void
+    {
+        $this->project->delete();
+
+        parent::tearDown();
+    }
+
+    /**
+     * Test parsing valid Test.xml file(s).
+     */
+    public function testValidXML(): void
+    {
+        $this->submitFiles($this->project->name, [
+            base_path(
+                'tests/Feature/Submission/Tests/data/angle_brackets_in_test_name.xml'
+            ),
+        ]);
+
+        $this->graphQL('
+            query project($id: ID) {
+              project(id: $id) {
+                builds {
+                  edges {
+                    node {
+                      tests {
+                        edges {
+                          node {
+                            name
+                          }
+                        }
+                      }
+                    }
+                  }
+                }
+              }
+            }
+        ', [
+            'id' => $this->project->id,
+        ])->assertExactJson([
+            'data' => [
+                'project' => [
+                    'builds' => [
+                        'edges' => [
+                            [
+                                'node' => [
+                                    'tests' => [
+                                        'edges' => [
+                                            [
+                                                'node' => [
+                                                    'name' => 'MyTest<parameterized>',
+                                                ],
+                                            ],
+                                        ],
+                                    ],
+                                ],
+                            ],
+                        ],
+                    ],
+                ],
+            ],
+        ]);
+    }
+}
diff --git a/tests/Feature/Submission/Tests/data/angle_brackets_in_test_name.xml b/tests/Feature/Submission/Tests/data/angle_brackets_in_test_name.xml
@@ -0,0 +1,36 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Site BuildName="dummy_test_name"
+  BuildStamp="dummy_stamp"
+  Name="dummy_name"
+  >
+	<Testing>
+		<StartDateTime>Jul 07 11:16 EDT</StartDateTime>
+		<StartTestTime>1751901373</StartTestTime>
+		<TestList>
+			<Test>./MyTest&lt;parameterized&gt;</Test>
+		</TestList>
+		<Test Status="passed">
+			<Name>MyTest&lt;parameterized&gt;</Name>
+			<Path>.</Path>
+			<FullName>./MyTest&lt;parameterized&gt;</FullName>
+			<FullCommandLine>/tmp/bin/mytest</FullCommandLine>
+			<Results>
+				<NamedMeasurement type="numeric/double" name="Execution Time">
+					<Value>0.1</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="numeric/double" name="Processors">
+					<Value>1</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Completion Status">
+					<Value>Completed</Value>
+				</NamedMeasurement>
+				<NamedMeasurement type="text/string" name="Command Line">
+					<Value>/tmp/bin/mytest</Value>
+				</NamedMeasurement>
+			</Results>
+		</Test>
+		<EndDateTime>Jul 07 11:16 EDT</EndDateTime>
+		<EndTestTime>1751901373</EndTestTime>
+		<ElapsedMinutes>0</ElapsedMinutes>
+	</Testing>
+</Site>

Original file line number	Diff line number	Diff line change
`@@ -174,8 +174,8 @@ public function apiTestSummary(): JsonResponse\|StreamedResponse`
`174`	`174`	`}`
`175`	`175`	`$this->setProjectById(intval($_GET['project'] ?? -1));`
`176`	`176`
`177`		`- $testName = htmlspecialchars($_GET['name'] ?? '');`
`178`		`- if ($testName === '') {`
	`177`	`+ $testName = $_GET['name'] ?? '';`
	`178`	`+ if ($testName === '' \|\| !is_string($testName)) {`
`179`	`179`	`abort(400, 'No test name specified.');`
`180`	`180`	`}`
`181`	`181`
Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,7 @@ public function startElement($parser, $name, $attributes): void`
`163`	`163`	`}`
`164`	`164`	`}`
`165`	`165`
`166`		`- $this->TestCreator->setTestName($attributes['NAME']);`
	`166`	`+ $this->TestCreator->testName = $attributes['NAME'];`
`167`	`167`	`$this->TestCreator->testPath = $attributes['CLASSNAME'];`
`168`	`168`	`} elseif ($name == 'TESTSUITE') {`
`169`	`169`	`// If the XML file doesn't have a <Site> tag then we use the information`
Original file line number	Diff line number	Diff line change
`@@ -2715,7 +2715,7 @@ parameters:`
`2715`	`2715`	`-`
`2716`	`2716`	`message: '#^Parameter \#1 \$string of function htmlspecialchars expects string, mixed given\.$#'`
`2717`	`2717`	`identifier: argument.type`
`2718`		`- count: 3`
	`2718`	`+ count: 2`
`2719`	`2719`	`path: app/Http/Controllers/TestController.php`
`2720`	`2720`
`2721`	`2721`	`-`