From 36806679ac1d98797fcc342b15cba2b7a6fc317c Mon Sep 17 00:00:00 2001 From: William Allen Date: Thu, 8 Oct 2026 17:00:46 -0400 Subject: [PATCH] Prevent builds from getting moved to dynamic groups Builds are supposed to be assigned to exactly one "regular" build group, and dynamic build groups are supposed to have no associated builds. Various parts of the codebase are inconsistent about this, and unintentionally make it seem as if it's possible to reassign builds to dynamic groups. This fixes the issue by adding checks to ensure users can only move builds between "regular" build groups. I plan to follow this with a separate PR which addresses builds which are incorrectly assigned to dynamic groups. --- app/Http/Controllers/BuildController.php | 12 +- .../Controllers/ExpectedBuildController.php | 7 +- app/cdash/app/Controller/Api/Index.php | 1 + app/cdash/app/Model/BuildGroup.php | 10 + app/cdash/public/api/v1/buildgroup.php | 13 +- app/cdash/public/api/v1/index.php | 5 + app/cdash/tests/CMakeLists.txt | 2 + phpstan-baseline.neon | 2 +- .../js/angular/views/partials/build.html | 3 + .../js/angular/views/partials/buildgroup.html | 5 +- tests/Feature/BuildGroupRuleTest.php | 25 ++- tests/Feature/DynamicBuildGroupTest.php | 207 ++++++++++++++++++ 12 files changed, 282 insertions(+), 10 deletions(-) create mode 100644 tests/Feature/DynamicBuildGroupTest.php diff --git a/app/Http/Controllers/BuildController.php b/app/Http/Controllers/BuildController.php index 8a35a3fb2f..84c8762948 100644 --- a/app/Http/Controllers/BuildController.php +++ b/app/Http/Controllers/BuildController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers; +use App\Enums\BuildGroupType; use App\Models\Build as EloquentBuild; use App\Models\BuildGroup; use App\Models\Project; @@ -484,6 +485,11 @@ private function restApiPost(): JsonResponse // Should we change whether or not this build is expected? if (request()->has('expected') && request()->has('groupid')) { + // The rule would also route future submissions of this build into the group. + if (BuildGroup::findOrFail(request()->integer('groupid'))->type !== BuildGroupType::DAILY) { + abort(400, 'Builds cannot be assigned to dynamic build groups.'); + } + $buildgrouprule->Expected = request()->input('expected'); $buildgrouprule->GroupId = request()->input('groupid'); $buildgrouprule->SetExpected(); @@ -496,9 +502,13 @@ private function restApiPost(): JsonResponse $eloquent_build = EloquentBuild::findOrFail((int) $this->build->Id); - if (BuildGroup::findOrFail((int) $newgroupid)->project()->isNot($eloquent_build->project)) { + $newgroup = BuildGroup::findOrFail((int) $newgroupid); + if ($newgroup->project()->isNot($eloquent_build->project)) { abort(403, 'Requested build group is not associated with this project.'); } + if ($newgroup->type !== BuildGroupType::DAILY) { + abort(400, 'Builds cannot be assigned to dynamic build groups.'); + } // Remove the build from its previous group. $eloquent_build->buildGroups()->detach(); diff --git a/app/Http/Controllers/ExpectedBuildController.php b/app/Http/Controllers/ExpectedBuildController.php index e319349226..b71f167483 100644 --- a/app/Http/Controllers/ExpectedBuildController.php +++ b/app/Http/Controllers/ExpectedBuildController.php @@ -2,6 +2,8 @@ namespace App\Http\Controllers; +use App\Enums\BuildGroupType; +use App\Models\BuildGroup as EloquentBuildGroup; use CDash\Model\BuildGroup; use CDash\Model\BuildGroupRule; use Illuminate\Http\JsonResponse; @@ -103,7 +105,10 @@ private static function rest_post(Request $request, int $siteid, int $buildgroup abort(400, 'newgroupid not specified.'); } - $newgroupid = htmlspecialchars($request->input('newgroupid')); + $newgroupid = $request->integer('newgroupid'); + if (EloquentBuildGroup::findOrFail($newgroupid)->type !== BuildGroupType::DAILY) { + abort(400, 'Builds cannot be assigned to dynamic build groups.'); + } $rule = new BuildGroupRule(); $rule->SiteId = $siteid; diff --git a/app/cdash/app/Controller/Api/Index.php b/app/cdash/app/Controller/Api/Index.php index a40377b97f..0b8e36a8a1 100644 --- a/app/cdash/app/Controller/Api/Index.php +++ b/app/cdash/app/Controller/Api/Index.php @@ -515,6 +515,7 @@ public function beginResponseForBuildGroup(BuildGroup $buildgroup): void $buildgroup_response['id'] = $buildgroup->GetId(); $buildgroup_response['name'] = $groupname; + $buildgroup_response['type'] = $buildgroup->GetType(); $buildgroup_response['linkname'] = urlencode($groupname); $buildgroup_response['position'] = $buildgroup->GetPosition(); diff --git a/app/cdash/app/Model/BuildGroup.php b/app/cdash/app/Model/BuildGroup.php index fb9e0c9a57..9c6826663b 100644 --- a/app/cdash/app/Model/BuildGroup.php +++ b/app/cdash/app/Model/BuildGroup.php @@ -371,11 +371,13 @@ public function Delete(): bool /** @var ?EloquentBuildGroup $newGroup */ $newGroup = $this->eloquent_model->project?->buildgroups()->where([ 'name' => $oldbuild->type, + 'type' => BuildGroupType::DAILY, ])->first(); if ($newGroup === null) { $newGroup = $this->eloquent_model->project?->buildgroups()->where([ 'name' => 'Experimental', + 'type' => BuildGroupType::DAILY, ])->first(); } @@ -410,6 +412,10 @@ public function Delete(): bool return true; } + /** + * Builds are only ever assigned to daily groups. Dynamic groups compute their builds from + * their own rules at display time. + */ public function GetGroupIdFromRule(Build $build): int { $starttime = $build->StartTime; @@ -419,6 +425,7 @@ public function GetGroupIdFromRule(Build $build): int $rule_row = DB::table('build2grouprule') ->join('buildgroup', 'buildgroup.id', '=', 'build2grouprule.groupid') ->where('buildgroup.projectid', '=', $build->ProjectId) + ->where('buildgroup.type', '=', BuildGroupType::DAILY) ->where('build2grouprule.buildtype', '=', $build->Type) ->where('build2grouprule.siteid', '=', $build->SiteId) ->where('build2grouprule.buildname', '=', $build->Name) @@ -435,6 +442,7 @@ public function GetGroupIdFromRule(Build $build): int $name_rule_row = DB::table('build2grouprule') ->join('buildgroup', 'buildgroup.id', '=', 'build2grouprule.groupid') ->where('buildgroup.projectid', '=', $build->ProjectId) + ->where('buildgroup.type', '=', BuildGroupType::DAILY) ->where('build2grouprule.buildtype', '=', $build->Type) ->where('build2grouprule.siteid', '=', -1) ->whereRaw('? LIKE build2grouprule.buildname', [$build->Name]) @@ -454,6 +462,7 @@ public function GetGroupIdFromRule(Build $build): int $default_model = EloquentBuildGroup::where([ 'name' => $build->Type, 'projectid' => $build->ProjectId, + 'type' => BuildGroupType::DAILY, ])->first(); if ($default_model !== null) { return $default_model->id; @@ -462,6 +471,7 @@ public function GetGroupIdFromRule(Build $build): int return EloquentBuildGroup::where([ 'name' => 'Experimental', 'projectid' => (int) $build->ProjectId, + 'type' => BuildGroupType::DAILY, ])->first()->id ?? 0; } diff --git a/app/cdash/public/api/v1/buildgroup.php b/app/cdash/public/api/v1/buildgroup.php index 82140982cd..645924e823 100644 --- a/app/cdash/public/api/v1/buildgroup.php +++ b/app/cdash/public/api/v1/buildgroup.php @@ -17,6 +17,8 @@ namespace CDash\Api\v1\BuildGroup; +use App\Enums\BuildGroupType; +use App\Models\BuildGroup as EloquentBuildGroup; use CDash\Database; use CDash\Model\Build; use CDash\Model\BuildGroup; @@ -197,10 +199,14 @@ function rest_post($pdo, $projectid) if (isset($_POST['builds'])) { // Move builds to a new group. $group = $_POST['group']; - if ($group['id'] < 1) { + $groupid = $group['id']; + if ($groupid < 1) { $error_msg = 'Please select a group for these builds'; abort(400, $error_msg); } + if (EloquentBuildGroup::whereKey($groupid)->firstOrFail()->type !== BuildGroupType::DAILY) { + abort(400, 'Builds cannot be assigned to dynamic build groups.'); + } $builds = $_POST['builds']; if (array_key_exists('expected', $_POST)) { @@ -210,8 +216,6 @@ function rest_post($pdo, $projectid) } foreach ($builds as $buildinfo) { - $groupid = $group['id']; - $Build = new Build(); $Build->Id = (int) $buildinfo['id']; $Build->FillFromId($Build->Id); @@ -247,6 +251,9 @@ function rest_post($pdo, $projectid) $error_msg = 'Please select a BuildGroup to define.'; abort(400, $error_msg); } + if (EloquentBuildGroup::whereKey($groupid)->firstOrFail()->type !== BuildGroupType::DAILY) { + abort(400, 'Builds cannot be assigned to dynamic build groups.'); + } $nameMatch = convert_wildcards($_POST['nameMatch']); $type = $_POST['type']; diff --git a/app/cdash/public/api/v1/index.php b/app/cdash/public/api/v1/index.php index 793a3c2683..2f2e297040 100644 --- a/app/cdash/public/api/v1/index.php +++ b/app/cdash/public/api/v1/index.php @@ -17,6 +17,7 @@ namespace CDash\Api\v1\Index; +use App\Enums\BuildGroupType; use App\Services\ProjectService; use CDash\Controller\Api\Index as IndexController; use CDash\Database; @@ -516,8 +517,12 @@ // Create a separate "all buildgroups" section of our response. // This is used to allow project admins to move builds between groups. +// Builds can't be moved into dynamic groups, so only daily groups are listed. $response['all_buildgroups'] = []; foreach ($controller->buildgroupsResponse as $group) { + if ($group['type'] !== BuildGroupType::DAILY->value) { + continue; + } $response['all_buildgroups'][] = [ 'id' => $group['id'], 'name' => $group['name'], diff --git a/app/cdash/tests/CMakeLists.txt b/app/cdash/tests/CMakeLists.txt index a175237008..bf705cefb4 100644 --- a/app/cdash/tests/CMakeLists.txt +++ b/app/cdash/tests/CMakeLists.txt @@ -333,6 +333,8 @@ add_feature_test_in_transaction(/Feature/ManageOverviewTest) add_feature_test_in_transaction(/Feature/TestOverviewTest) +add_feature_test_in_transaction(/Feature/DynamicBuildGroupTest) + add_feature_test_in_transaction(/Feature/Services/AuthTokenServiceTest) add_feature_test_in_transaction(/Feature/Services/ProjectServiceTest) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d465221ea3..b64ce5fdde 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -738,7 +738,7 @@ parameters: - rawMessage: 'Parameter #1 $string of function htmlspecialchars expects string, mixed given.' identifier: argument.type - count: 3 + count: 2 path: app/Http/Controllers/ExpectedBuildController.php - diff --git a/resources/js/angular/views/partials/build.html b/resources/js/angular/views/partials/build.html index 4d7e2de8a4..d3bdbab3cb 100644 --- a/resources/js/angular/views/partials/build.html +++ b/resources/js/angular/views/partials/build.html @@ -182,7 +182,10 @@ data-cy="build-admin-options-panel" style="padding: 8px 0; display: flex; flex-direction: column; gap: 8px;">
+ -