From 48369c48e4c4965c7359004231e5c23c3ad084fd Mon Sep 17 00:00:00 2001 From: William Allen Date: Thu, 8 Oct 2026 19:26:21 -0400 Subject: [PATCH] Move GitHub webhook API endpoint to controller The GitHub webhook handler is one of a small handful of API endpoints which don't live under the controller architecture we use for most routes. This PR fixes that, by moving the logic to a controller and performing a light refactor which fixes a few edge cases. Along the way, I wrote some tests to improve our confidence that the changes work as expected. --- .../Controllers/GitHubWebhookController.php | 62 ++++++++ app/cdash/public/api/v1/GitHub/webhook.php | 70 --------- phpstan-baseline.neon | 42 ------ routes/api.php | 3 + tests/Feature/GitHubWebhook.php | 135 +++++++++++++++++- 5 files changed, 193 insertions(+), 119 deletions(-) create mode 100644 app/Http/Controllers/GitHubWebhookController.php delete mode 100644 app/cdash/public/api/v1/GitHub/webhook.php diff --git a/app/Http/Controllers/GitHubWebhookController.php b/app/Http/Controllers/GitHubWebhookController.php new file mode 100644 index 0000000000..dd35232844 --- /dev/null +++ b/app/Http/Controllers/GitHubWebhookController.php @@ -0,0 +1,62 @@ +header('X-GitHub-Event', '')) { + case 'check_run': + // Avoid an infinite loop of reacting to our own activity. + if ($request->json('check_run.name') !== 'CDash' || $request->json('action') === 'rerequested') { + Repository::createOrUpdateCheck($request->json('check_run.head_sha')); + } + break; + case 'status': + Repository::createOrUpdateCheck($request->json('sha')); + break; + } + + return response()->noContent(); + } + + /** + * Adapted from https://gist.github.com/milo/daed6e958ea534e4eba3 + */ + private static function verifySignature(Request $request, string $secret): void + { + $signature = $request->headers->get('X-Hub-Signature'); + if ($signature === null) { + self::fail("HTTP header 'X-Hub-Signature' is missing."); + } + + [$algo, $hash] = explode('=', $signature, 2) + ['', '']; + if (!in_array($algo, hash_hmac_algos(), true)) { + self::fail("Hash algorithm '$algo' is not supported."); + } + + if (!hash_equals(hash_hmac($algo, $request->getContent(), $secret), $hash)) { + self::fail('Hook secret does not match.'); + } + } + + private static function fail(string $message): never + { + Log::warning($message, [ + 'function' => 'GitHub webhook', + ]); + abort(500, $message); + } +} diff --git a/app/cdash/public/api/v1/GitHub/webhook.php b/app/cdash/public/api/v1/GitHub/webhook.php deleted file mode 100644 index d886bdcc0f..0000000000 --- a/app/cdash/public/api/v1/GitHub/webhook.php +++ /dev/null @@ -1,70 +0,0 @@ - 'GitHub webhook', - ]); - abort(500, $msg); - } -} - -$hookSecret = config('cdash.github_webhook_secret'); -if (!is_null($hookSecret)) { - if (!isset($_SERVER['HTTP_X_HUB_SIGNATURE'])) { - handle_error("HTTP header 'X-Hub-Signature' is missing."); - } elseif (!extension_loaded('hash')) { - handle_error("Missing 'hash' extension to check the secret code validity."); - } - [$algo, $hash] = explode('=', $_SERVER['HTTP_X_HUB_SIGNATURE'], 2) + ['', '']; - if (!in_array($algo, hash_algos(), true)) { - handle_error("Hash algorithm '$algo' is not supported."); - } - $rawPost = file_get_contents('php://input'); - if ($hash !== hash_hmac($algo, $rawPost, $hookSecret)) { - handle_error('Hook secret does not match.'); - } -} - -init_api_request(); - -$event = array_key_exists('HTTP_X_GITHUB_EVENT', $_SERVER) ? $_SERVER['HTTP_X_GITHUB_EVENT'] : ''; - -switch ($event) { - case 'check_run': - // Avoid an infinite loop of reacting to our own activity. - if ($_REQUEST['check_run']['name'] !== 'CDash' || - $_REQUEST['action'] === 'rerequested') { - $sha = $_REQUEST['check_run']['head_sha']; - Repository::createOrUpdateCheck($sha); - } - break; - - case 'status': - $sha = $_REQUEST['sha']; - Repository::createOrUpdateCheck($sha); - break; - - default: - break; -} diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d465221ea3..94462f4132 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -12438,48 +12438,6 @@ parameters: count: 4 path: app/cdash/include/filterdataFunctions.php - - - rawMessage: Cannot access offset 'head_sha' on mixed. - identifier: offsetAccess.nonOffsetAccessible - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: Cannot access offset 'name' on mixed. - identifier: offsetAccess.nonOffsetAccessible - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: 'Function handle_error() has no return type specified.' - identifier: missingType.return - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: 'Function handle_error() has parameter $msg with no type specified.' - identifier: missingType.parameter - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: 'Parameter #2 $data of function hash_hmac expects string, string|false given.' - identifier: argument.type - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: 'Parameter #2 $string of function explode expects string, mixed given.' - identifier: argument.type - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - - - rawMessage: 'Parameter #3 $key of function hash_hmac expects string, bool|string given.' - identifier: argument.type - count: 1 - path: app/cdash/public/api/v1/GitHub/webhook.php - - rawMessage: 'Argument of an invalid type mixed supplied for foreach, only iterables are supported.' identifier: foreach.nonIterable diff --git a/routes/api.php b/routes/api.php index fab08827d8..b9f090a5a3 100755 --- a/routes/api.php +++ b/routes/api.php @@ -2,6 +2,7 @@ use App\Http\Controllers\ExpectedBuildController; use App\Http\Controllers\FilterController; +use App\Http\Controllers\GitHubWebhookController; use App\Http\Controllers\TimelineController; use Illuminate\Support\Facades\Route; @@ -48,6 +49,8 @@ Route::match(['get', 'post', 'delete'], '/v1/expectedbuild.php', ExpectedBuildController::class); +Route::post('/v1/GitHub/webhook.php', GitHubWebhookController::class); + Route::middleware(['auth'])->group(function (): void { Route::get('/v1/manageSubProject.php', 'SubProjectController@apiManageSubProject'); diff --git a/tests/Feature/GitHubWebhook.php b/tests/Feature/GitHubWebhook.php index 660f98d197..777af7977d 100644 --- a/tests/Feature/GitHubWebhook.php +++ b/tests/Feature/GitHubWebhook.php @@ -2,11 +2,18 @@ namespace Tests\Feature; +use App\Models\Build; +use App\Models\BuildUpdate; use Illuminate\Foundation\Testing\DatabaseTransactions; +use Illuminate\Support\Str; +use Illuminate\Testing\TestResponse; +use Symfony\Component\HttpFoundation\Response; use Tests\TestCase; +use Tests\Traits\CreatesProjects; class GitHubWebhook extends TestCase { + use CreatesProjects; use DatabaseTransactions; protected string $endpoint = '/api/v1/GitHub/webhook.php'; @@ -15,8 +22,6 @@ public function setUp(): void { parent::setUp(); - $_SERVER['REQUEST_METHOD'] = 'POST'; - config(['cdash.github_webhook_secret' => 'mock secret']); } @@ -29,16 +34,16 @@ public function testWebhookWithoutRequiredSignature(): void public function testWebhookWithUnsupportedAlgorithm(): void { - $_SERVER['HTTP_X_HUB_SIGNATURE'] = 'zzz=foo'; - $this->post($this->endpoint) + $this->withHeader('X-Hub-Signature', 'zzz=foo') + ->post($this->endpoint) ->assertServerError() ->assertJson(['error' => "Hash algorithm 'zzz' is not supported."], true); } public function testWebhookWithWrongSignature(): void { - $_SERVER['HTTP_X_HUB_SIGNATURE'] = 'sha1=wrong secret'; - $this->post($this->endpoint) + $this->withHeader('X-Hub-Signature', 'sha1=wrong secret') + ->post($this->endpoint) ->assertServerError() ->assertJson(['error' => 'Hook secret does not match.'], true); } @@ -46,7 +51,123 @@ public function testWebhookWithWrongSignature(): void public function testWebhookWithCorrectSignature(): void { $hash = hash_hmac('sha1', '', 'mock secret'); - $_SERVER['HTTP_X_HUB_SIGNATURE'] = "sha1=$hash"; + $this->withHeader('X-Hub-Signature', "sha1=$hash") + ->post($this->endpoint) + ->assertNoContent(); + } + + public function testWebhookWithoutSecretDoesNotRequireSignature(): void + { + config(['cdash.github_webhook_secret' => null]); + $this->post($this->endpoint)->assertNoContent(); } + + public function testSignatureCoversRequestBody(): void + { + $body = (string) json_encode(['sha' => 'abc']); + $hash = hash_hmac('sha1', $body . ' ', 'mock secret'); + + $this->call('POST', $this->endpoint, server: [ + 'HTTP_X_GITHUB_EVENT' => 'status', + 'HTTP_X_HUB_SIGNATURE' => "sha1=$hash", + 'CONTENT_TYPE' => 'application/json', + ], content: $body) + ->assertServerError() + ->assertJson(['error' => 'Hook secret does not match.'], true); + } + + public function testStatusEventCreatesCheck(): void + { + $this->sendEvent('status', ['sha' => $this->createCommit()]) + ->assertServerError() + ->assertJson(['error' => 'No repository interface defined for gitlab'], true); + } + + public function testStatusEventForUnknownCommit(): void + { + $this->createCommit(); + + $this->sendEvent('status', ['sha' => Str::uuid()->toString()])->assertNoContent(); + } + + public function testCheckRunEventCreatesCheck(): void + { + $this->sendEvent('check_run', [ + 'action' => 'completed', + 'check_run' => [ + 'name' => 'Some other check', + 'head_sha' => $this->createCommit(), + ], + ]) + ->assertServerError() + ->assertJson(['error' => 'No repository interface defined for gitlab'], true); + } + + public function testCheckRunEventIgnoresOwnCheck(): void + { + $this->sendEvent('check_run', [ + 'action' => 'completed', + 'check_run' => [ + 'name' => 'CDash', + 'head_sha' => $this->createCommit(), + ], + ])->assertNoContent(); + } + + public function testCheckRunEventRecreatesRerequestedOwnCheck(): void + { + $this->sendEvent('check_run', [ + 'action' => 'rerequested', + 'check_run' => [ + 'name' => 'CDash', + 'head_sha' => $this->createCommit(), + ], + ]) + ->assertServerError() + ->assertJson(['error' => 'No repository interface defined for gitlab'], true); + } + + public function testUnknownEventIsIgnored(): void + { + $this->sendEvent('push', ['sha' => $this->createCommit()])->assertNoContent(); + } + + /** + * Creates a build for a commit in a project which uses an unsupported repository viewer, so that + * creating a check for that commit fails with an error that shows the check was attempted. + */ + private function createCommit(): string + { + $project = $this->makePublicProject(); + $project->cvsviewertype = 'gitlab'; + $project->save(); + + /** @var Build $build */ + $build = $project->builds()->create([ + 'name' => 'build1', + 'uuid' => Str::uuid()->toString(), + ]); + $update = BuildUpdate::factory()->create(); + $build->updateStep()->associate($update)->save(); + + return $update->revision; + } + + /** + * @param array $payload + * + * @return TestResponse + */ + private function sendEvent(string $event, array $payload): TestResponse + { + $body = (string) json_encode($payload); + $hash = hash_hmac('sha1', $body, 'mock secret'); + + return $this->call('POST', $this->endpoint, server: [ + 'HTTP_X_GITHUB_EVENT' => $event, + 'HTTP_X_HUB_SIGNATURE' => "sha1=$hash", + 'CONTENT_TYPE' => 'application/json', + ], content: $body); + } }