From c255903ee3cc899cb99fddde50ae73d7054b5d6d Mon Sep 17 00:00:00 2001 From: Simon Hamp Date: Tue, 29 Sep 2026 15:59:16 +0100 Subject: [PATCH] Skip revoked GitHub tokens and queue one Satis build per release The token resolver handed out stored tokens without checking them, so a revoked installation or user token failed everything it was passed to, including the Satis build. It now tries each one against the plugin's repo and moves on after a 401 or 404. PluginSyncService drops its own copy of the resolver and uses the shared one. Both webhook handlers now only act on the "published" release action, and the first delivery of a release claims it, so the per-repo webhook and the GitHub App webhook no longer queue a build each. Co-Authored-By: Claude Opus 5.5 --- .../GitHubAppWebhookController.php | 4 + .../Controllers/PluginWebhookController.php | 17 +++ app/Jobs/Concerns/ResolvesGitHubToken.php | 46 ++++++- app/Jobs/SyncPluginReleases.php | 4 +- app/Models/Plugin.php | 20 +++ app/Services/PluginSyncService.php | 33 +---- .../Feature/GitHubAppRepositoryEventsTest.php | 63 ++++++++- tests/Feature/PluginSyncServiceTest.php | 28 ++++ tests/Feature/PluginWebhookTest.php | 87 +++++++++++++ .../Feature/SatisSync/SatisBuildTokenTest.php | 123 ++++++++++++++++++ 10 files changed, 390 insertions(+), 35 deletions(-) create mode 100644 tests/Feature/SatisSync/SatisBuildTokenTest.php diff --git a/app/Http/Controllers/GitHubAppWebhookController.php b/app/Http/Controllers/GitHubAppWebhookController.php index 9f9c3a036..d9e3de281 100644 --- a/app/Http/Controllers/GitHubAppWebhookController.php +++ b/app/Http/Controllers/GitHubAppWebhookController.php @@ -198,6 +198,10 @@ protected function handleRepositoryEvent(string $event, array $payload): JsonRes ->get() ->filter(fn (Plugin $plugin): bool => $plugin->isActive()); + if ($event === 'release') { + $plugins = $plugins->filter(fn (Plugin $plugin): bool => $plugin->claimReleaseEvent($payload)); + } + $syncService = app(PluginSyncService::class); foreach ($plugins as $plugin) { diff --git a/app/Http/Controllers/PluginWebhookController.php b/app/Http/Controllers/PluginWebhookController.php index a1b57fc23..938539988 100644 --- a/app/Http/Controllers/PluginWebhookController.php +++ b/app/Http/Controllers/PluginWebhookController.php @@ -29,6 +29,10 @@ public function __invoke(Request $request, string $secret, PluginSyncService $sy } if ($event === 'release') { + if (! $plugin->claimReleaseEvent($this->payload($request))) { + return response()->json(['success' => true, 'message' => 'Release event ignored']); + } + // Sync plugin metadata to update latest_version $syncService->sync($plugin); @@ -69,4 +73,17 @@ public function __invoke(Request $request, string $secret, PluginSyncService $sy 'releases_sync' => 'queued', ]); } + + /** + * The event payload. A webhook left on GitHub's default form content type sends it as JSON in + * a "payload" field. + * + * @return array + */ + protected function payload(Request $request): array + { + $payload = $request->input('payload'); + + return is_string($payload) ? (json_decode($payload, true) ?? []) : $request->all(); + } } diff --git a/app/Jobs/Concerns/ResolvesGitHubToken.php b/app/Jobs/Concerns/ResolvesGitHubToken.php index 1974904fb..29888cfa2 100644 --- a/app/Jobs/Concerns/ResolvesGitHubToken.php +++ b/app/Jobs/Concerns/ResolvesGitHubToken.php @@ -4,6 +4,8 @@ use App\Models\Plugin; use App\Services\GitHubAppService; +use Illuminate\Http\Client\ConnectionException; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; trait ResolvesGitHubToken @@ -29,7 +31,7 @@ protected function resolveGitHubTokenFor(Plugin $plugin): ?string if ($installation) { $token = $appService->getInstallationToken($installation); - if ($token) { + if ($token && $this->gitHubAcceptsToken($plugin, $token)) { Log::debug('[GitHub] Using installation token', [ 'plugin_id' => $plugin->id, 'user_id' => $user->id, @@ -42,14 +44,16 @@ protected function resolveGitHubTokenFor(Plugin $plugin): ?string } // Priority 2: User OAuth token - if ($user && $user->hasGitHubToken()) { + $userToken = $user?->getGitHubToken(); + + if ($userToken && $this->gitHubAcceptsToken($plugin, $userToken)) { Log::debug('[GitHub] Using plugin owner OAuth token', [ 'plugin_id' => $plugin->id, 'user_id' => $user->id, 'github_username' => $user->github_username, ]); - return $user->getGitHubToken(); + return $userToken; } // Priority 3: Platform token @@ -62,4 +66,40 @@ protected function resolveGitHubTokenFor(Plugin $plugin): ?string return $platformToken; } + + /** + * Whether GitHub still accepts a stored token for the plugin's repository. Access can be revoked + * on GitHub's side without us hearing about it, and a revoked token fails everything it's handed + * to, Satis builds included. Only a 401 (revoked) or 404 (can't see the repository) counts + * against a token, since a rate limit or an outage says nothing about the token itself. The + * platform token is the last resort, so it isn't tried. + */ + protected function gitHubAcceptsToken(Plugin $plugin, string $token): bool + { + $repo = $plugin->getRepositoryOwnerAndName(); + + if (! $repo) { + return true; + } + + try { + $response = Http::withToken($token) + ->accept('application/vnd.github+json') + ->timeout(10) + ->get("https://api.github.com/repos/{$repo['owner']}/{$repo['repo']}"); + } catch (ConnectionException) { + return true; + } + + if ($response->unauthorized() || $response->notFound()) { + Log::warning('[GitHub] Token rejected, trying the next one', [ + 'plugin_id' => $plugin->id, + 'status' => $response->status(), + ]); + + return false; + } + + return true; + } } diff --git a/app/Jobs/SyncPluginReleases.php b/app/Jobs/SyncPluginReleases.php index e6003c008..1b3533819 100644 --- a/app/Jobs/SyncPluginReleases.php +++ b/app/Jobs/SyncPluginReleases.php @@ -56,7 +56,7 @@ public function handle(SatisService $satisService): void 'plugin_id' => $this->plugin->id, 'owner' => $repo['owner'], 'repo' => $repo['repo'], - 'token_source' => $token ? ($this->plugin->user?->hasGitHubToken() ? 'user_oauth' : 'platform') : 'none', + 'has_token' => $token !== null, ]); $releases = $this->fetchReleases($repo['owner'], $repo['repo'], $token); @@ -104,7 +104,7 @@ public function handle(SatisService $satisService): void 'plugin_name' => $this->plugin->name, ]); - $result = $satisService->build([$this->plugin], $this->getGitHubToken()); + $result = $satisService->build([$this->plugin], $token); if ($result['success'] ?? false) { $this->plugin->update(['satis_synced_at' => now()]); diff --git a/app/Models/Plugin.php b/app/Models/Plugin.php index 09fa1cf78..16d9671d5 100644 --- a/app/Models/Plugin.php +++ b/app/Models/Plugin.php @@ -36,6 +36,7 @@ use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\HasOne; use Illuminate\Support\Collection; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Notification; class Plugin extends Model @@ -818,6 +819,25 @@ public function isReachableViaGitHubApp(): bool return app(GitHubAppService::class)->findInstallationForRepo($this->user, $repo['owner'], $repo['repo']) !== null; } + /** + * Claim a GitHub release event, so each published release is only synced once. GitHub sends + * created, published and released for a single publish, and a repository the GitHub App covers + * can still have its old per-repository webhook, which delivers every event a second time. Only + * the first published event for each release gets through. + * + * @param array $payload + */ + public function claimReleaseEvent(array $payload): bool + { + $releaseId = $payload['release']['id'] ?? null; + + if (($payload['action'] ?? null) !== 'published' || ! $releaseId) { + return false; + } + + return Cache::add("plugin_{$this->id}_published_release_{$releaseId}", true, now()->addHour()); + } + public function getRepositoryOwnerAndName(): ?array { if (! $this->repository_url) { diff --git a/app/Services/PluginSyncService.php b/app/Services/PluginSyncService.php index c3cd7d461..a6bc4dc7b 100644 --- a/app/Services/PluginSyncService.php +++ b/app/Services/PluginSyncService.php @@ -2,6 +2,7 @@ namespace App\Services; +use App\Jobs\Concerns\ResolvesGitHubToken; use App\Jobs\GeneratePluginOgImage; use App\Models\Plugin; use App\Support\CommonMark\CommonMark; @@ -10,6 +11,8 @@ class PluginSyncService { + use ResolvesGitHubToken; + public function sync(Plugin $plugin): bool { Log::info('[PluginSync] Starting sync', ['plugin_id' => $plugin->id, 'name' => $plugin->name]); @@ -28,7 +31,7 @@ public function sync(Plugin $plugin): bool 'repo' => $repo['repo'], ]); - $token = $this->getGitHubToken($plugin); + $token = $this->resolveGitHubTokenFor($plugin); Log::info('[PluginSync] Token resolved', [ 'plugin_id' => $plugin->id, @@ -192,34 +195,6 @@ protected function fetchFileFromGitHub(string $owner, string $repo, string $path return null; } - protected function getGitHubToken(Plugin $plugin): ?string - { - $user = $plugin->user; - $repo = $plugin->getRepositoryOwnerAndName(); - - // Priority 1: Installation token (GitHub App) - if ($user && $repo && $user->isUsingGitHubApp()) { - $appService = app(GitHubAppService::class); - $installation = $appService->findInstallationForRepo($user, $repo['owner'], $repo['repo']); - - if ($installation) { - $token = $appService->getInstallationToken($installation); - - if ($token) { - return $token; - } - } - } - - // Priority 2: User OAuth token - if ($user && $user->hasGitHubToken()) { - return $user->getGitHubToken(); - } - - // Priority 3: Platform token - return config('services.github.token'); - } - protected function extractIosVersion(array $nativephpData): ?string { return $nativephpData['ios']['min_version'] ?? null; diff --git a/tests/Feature/GitHubAppRepositoryEventsTest.php b/tests/Feature/GitHubAppRepositoryEventsTest.php index 2f1208d36..8aa8538b8 100644 --- a/tests/Feature/GitHubAppRepositoryEventsTest.php +++ b/tests/Feature/GitHubAppRepositoryEventsTest.php @@ -48,6 +48,15 @@ private function sendAppWebhook(string $event, array $payload): TestResponse ], $body); } + private function releasePayload(string $action, int $releaseId): array + { + return [ + 'action' => $action, + 'release' => ['id' => $releaseId, 'tag_name' => "v1.0.{$releaseId}"], + 'repository' => ['full_name' => 'acme/camera-plugin'], + ]; + } + private function coveredPlugin(array $attributes = []): Plugin { $user = User::factory()->withGitHubApp()->create(); @@ -90,12 +99,64 @@ public function test_release_event_from_the_app_syncs_releases(): void $mock->shouldReceive('sync')->once()->andReturn(true); }); - $this->sendAppWebhook('release', ['repository' => ['full_name' => 'acme/camera-plugin']]) + $this->sendAppWebhook('release', $this->releasePayload('published', 101)) ->assertOk(); Bus::assertDispatched(SyncPluginReleases::class, fn (SyncPluginReleases $job) => $job->plugin->is($plugin)); } + public function test_only_the_published_release_event_is_synced(): void + { + Bus::fake([SyncPluginReleases::class]); + $this->coveredPlugin(); + + $this->mock(PluginSyncService::class, function (MockInterface $mock): void { + $mock->shouldReceive('sync')->once()->andReturn(true); + }); + + // GitHub sends all three of these when a release is published + foreach (['created', 'published', 'released'] as $action) { + $this->sendAppWebhook('release', $this->releasePayload($action, 101))->assertOk(); + } + + Bus::assertDispatchedTimes(SyncPluginReleases::class, 1); + } + + public function test_a_release_the_repository_webhook_already_delivered_is_not_synced_again(): void + { + Bus::fake([SyncPluginReleases::class]); + $plugin = $this->coveredPlugin(['last_synced_at' => now()]); + + $this->mock(PluginSyncService::class, function (MockInterface $mock): void { + $mock->shouldReceive('sync')->once()->andReturn(true); + }); + + $this->postJson(route('webhooks.plugins', $plugin->webhook_secret), $this->releasePayload('published', 101), [ + 'X-GitHub-Event' => 'release', + ])->assertOk(); + + $this->sendAppWebhook('release', $this->releasePayload('published', 101)) + ->assertOk() + ->assertJson(['plugins' => 0]); + + Bus::assertDispatchedTimes(SyncPluginReleases::class, 1); + } + + public function test_each_published_release_is_synced(): void + { + Bus::fake([SyncPluginReleases::class]); + $this->coveredPlugin(); + + $this->mock(PluginSyncService::class, function (MockInterface $mock): void { + $mock->shouldReceive('sync')->twice()->andReturn(true); + }); + + $this->sendAppWebhook('release', $this->releasePayload('published', 101))->assertOk(); + $this->sendAppWebhook('release', $this->releasePayload('published', 102))->assertOk(); + + Bus::assertDispatchedTimes(SyncPluginReleases::class, 2); + } + public function test_events_for_inactive_plugins_are_ignored(): void { $this->coveredPlugin(['is_active' => false]); diff --git a/tests/Feature/PluginSyncServiceTest.php b/tests/Feature/PluginSyncServiceTest.php index ff886ca7a..3332b6feb 100644 --- a/tests/Feature/PluginSyncServiceTest.php +++ b/tests/Feature/PluginSyncServiceTest.php @@ -3,8 +3,10 @@ namespace Tests\Feature; use App\Models\Plugin; +use App\Models\User; use App\Services\PluginSyncService; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\Client\Request; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Queue; use Tests\TestCase; @@ -172,6 +174,32 @@ public function test_sync_does_not_overwrite_name_when_taken_by_another_plugin() $this->assertEquals('acme/original-name', $plugin->fresh()->name); } + public function test_sync_skips_an_owner_token_github_has_revoked(): void + { + config(['services.github.token' => 'ghp_platform']); + + Http::fake([ + 'api.github.com/repos/acme/test-plugin' => Http::response(['message' => 'Bad credentials'], 401), + 'api.github.com/repos/acme/test-plugin/contents/composer.json' => Http::response([ + 'content' => base64_encode(json_encode(['name' => 'acme/test-plugin'])), + ]), + 'api.github.com/*' => Http::response([], 404), + 'raw.githubusercontent.com/*' => Http::response('', 404), + ]); + + $plugin = Plugin::factory() + ->for(User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_revoked')])) + ->create([ + 'name' => 'acme/test-plugin', + 'repository_url' => 'https://github.com/acme/test-plugin', + ]); + + $this->assertTrue((new PluginSyncService)->sync($plugin)); + + Http::assertSent(fn (Request $request) => str_ends_with($request->url(), '/contents/composer.json') + && $request->hasHeader('Authorization', 'Bearer ghp_platform')); + } + public function test_sync_sets_mobile_min_version_to_null_when_not_in_composer_data(): void { $composerJson = json_encode([ diff --git a/tests/Feature/PluginWebhookTest.php b/tests/Feature/PluginWebhookTest.php index e3e123459..da8b84bd4 100644 --- a/tests/Feature/PluginWebhookTest.php +++ b/tests/Feature/PluginWebhookTest.php @@ -2,9 +2,11 @@ namespace Tests\Feature; +use App\Jobs\SyncPluginReleases; use App\Models\Plugin; use App\Services\PluginSyncService; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Bus; use PHPUnit\Framework\Attributes\Test; use Tests\TestCase; @@ -79,6 +81,91 @@ public function non_ping_event_succeeds_for_unapproved_but_active_plugin(): void ->assertJson(['success' => true]); } + #[Test] + public function published_release_queues_a_release_sync(): void + { + Bus::fake([SyncPluginReleases::class]); + + $plugin = Plugin::factory()->create(['last_synced_at' => now()]); + + $this->mock(PluginSyncService::class, function ($mock) { + $mock->shouldReceive('sync')->once()->andReturn(true); + }); + + $this->postJson( + route('webhooks.plugins', $plugin->webhook_secret), + ['action' => 'published', 'release' => ['id' => 101]], + ['X-GitHub-Event' => 'release'] + )->assertOk()->assertJson(['message' => 'Release sync queued']); + + Bus::assertDispatched(SyncPluginReleases::class, fn (SyncPluginReleases $job) => $job->plugin->is($plugin)); + } + + #[Test] + public function release_events_other_than_published_are_ignored(): void + { + Bus::fake([SyncPluginReleases::class]); + + $plugin = Plugin::factory()->create(['last_synced_at' => now()]); + + $this->mock(PluginSyncService::class, function ($mock) { + $mock->shouldNotReceive('sync'); + }); + + foreach (['created', 'released', 'prereleased', 'edited', 'deleted'] as $action) { + $this->postJson( + route('webhooks.plugins', $plugin->webhook_secret), + ['action' => $action, 'release' => ['id' => 101]], + ['X-GitHub-Event' => 'release'] + )->assertOk()->assertJson(['message' => 'Release event ignored']); + } + + Bus::assertNotDispatched(SyncPluginReleases::class); + } + + #[Test] + public function the_same_release_delivered_twice_is_only_synced_once(): void + { + Bus::fake([SyncPluginReleases::class]); + + $plugin = Plugin::factory()->create(['last_synced_at' => now()]); + + $this->mock(PluginSyncService::class, function ($mock) { + $mock->shouldReceive('sync')->once()->andReturn(true); + }); + + $deliver = fn () => $this->postJson( + route('webhooks.plugins', $plugin->webhook_secret), + ['action' => 'published', 'release' => ['id' => 101]], + ['X-GitHub-Event' => 'release'] + ); + + $deliver()->assertOk()->assertJson(['message' => 'Release sync queued']); + $deliver()->assertOk()->assertJson(['message' => 'Release event ignored']); + + Bus::assertDispatchedTimes(SyncPluginReleases::class, 1); + } + + #[Test] + public function release_payload_sent_as_form_data_is_read(): void + { + Bus::fake([SyncPluginReleases::class]); + + $plugin = Plugin::factory()->create(['last_synced_at' => now()]); + + $this->mock(PluginSyncService::class, function ($mock) { + $mock->shouldReceive('sync')->once()->andReturn(true); + }); + + $this->post( + route('webhooks.plugins', $plugin->webhook_secret), + ['payload' => json_encode(['action' => 'published', 'release' => ['id' => 101]])], + ['X-GitHub-Event' => 'release'] + )->assertOk()->assertJson(['message' => 'Release sync queued']); + + Bus::assertDispatchedTimes(SyncPluginReleases::class, 1); + } + #[Test] public function invalid_secret_returns_404(): void { diff --git a/tests/Feature/SatisSync/SatisBuildTokenTest.php b/tests/Feature/SatisSync/SatisBuildTokenTest.php new file mode 100644 index 000000000..b66cdcc3e --- /dev/null +++ b/tests/Feature/SatisSync/SatisBuildTokenTest.php @@ -0,0 +1,123 @@ + 'https://satis.test', + 'services.satis.api_key' => 'test-key', + 'services.github.token' => 'ghp_platform', + ]); + } + + public function test_the_build_gets_the_owners_token_when_github_accepts_it(): void + { + $plugin = $this->paidPluginFor(User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_owner')])); + + Http::fake([ + 'api.github.com/repos/acme/camera-plugin' => Http::response(['full_name' => 'acme/camera-plugin']), + 'satis.test/*' => Http::response(['job_id' => 'test-123'], 202), + ]); + + (new SatisService)->buildForPlugin($plugin); + + $this->assertSatisBuiltWithToken('ghu_owner'); + } + + public function test_the_build_skips_an_owner_token_github_has_revoked(): void + { + $plugin = $this->paidPluginFor(User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_revoked')])); + + Http::fake([ + 'api.github.com/repos/acme/camera-plugin' => Http::response(['message' => 'Bad credentials'], 401), + 'satis.test/*' => Http::response(['job_id' => 'test-123'], 202), + ]); + + (new SatisService)->buildForPlugin($plugin); + + $this->assertSatisBuiltWithToken('ghp_platform'); + } + + public function test_the_build_skips_an_installation_token_that_cannot_see_the_repository(): void + { + $user = User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_owner')]); + $installation = GitHubInstallation::factory()->for($user)->create(['account_login' => 'acme']); + Cache::put("github_installation_token_{$installation->installation_id}", 'ghs_installation', now()->addMinutes(55)); + + $plugin = $this->paidPluginFor($user); + + Http::fake([ + 'api.github.com/repos/acme/camera-plugin' => fn (Request $request) => $request->hasHeader('Authorization', 'Bearer ghs_installation') + ? Http::response(['message' => 'Not Found'], 404) + : Http::response(['full_name' => 'acme/camera-plugin']), + 'satis.test/*' => Http::response(['job_id' => 'test-123'], 202), + ]); + + (new SatisService)->buildForPlugin($plugin); + + $this->assertSatisBuiltWithToken('ghu_owner'); + } + + public function test_the_build_keeps_the_owners_token_when_github_is_having_trouble(): void + { + $plugin = $this->paidPluginFor(User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_owner')])); + + Http::fake([ + 'api.github.com/repos/acme/camera-plugin' => Http::response(['message' => 'Server Error'], 502), + 'satis.test/*' => Http::response(['job_id' => 'test-123'], 202), + ]); + + (new SatisService)->buildForPlugin($plugin); + + $this->assertSatisBuiltWithToken('ghu_owner'); + } + + public function test_a_release_sync_builds_with_the_token_it_fetched_releases_with(): void + { + $plugin = $this->paidPluginFor(User::factory()->withGitHubApp()->create(['github_token' => encrypt('ghu_revoked')])); + + Http::fake([ + 'api.github.com/repos/acme/camera-plugin' => Http::response(['message' => 'Bad credentials'], 401), + 'api.github.com/repos/acme/camera-plugin/releases*' => Http::response([]), + 'satis.test/*' => Http::response(['job_id' => 'test-123'], 202), + ]); + + (new SyncPluginReleases($plugin))->handle(new SatisService); + + Http::assertSent(fn (Request $request) => str_contains($request->url(), '/releases') + && $request->hasHeader('Authorization', 'Bearer ghp_platform')); + + $this->assertSatisBuiltWithToken('ghp_platform'); + } + + private function paidPluginFor(User $user): Plugin + { + return Plugin::factory()->paid()->approved()->for($user)->create([ + 'repository_url' => 'https://github.com/acme/camera-plugin', + ]); + } + + private function assertSatisBuiltWithToken(string $token): void + { + Http::assertSent(fn (Request $request) => $request->url() === 'https://satis.test/api/build' + && $request['github_token'] === $token); + } +}