From 5dda39e588f9bd96c08a6dea7ad63e1cd270b0c4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 22 May 2026 12:30:00 +0200 Subject: [PATCH] fix(source): scope private key and source selection to current team The Source component now resolves the supplied private key and Git source IDs through team-scoped queries before persisting them, so a selection can only ever reference a resource owned by the current team. The source type is additionally restricted to the supported GitHub/GitLab app classes. The privateKeyId property is marked #[Locked] so it can only change through the dedicated handler rather than a direct property update. Adds feature tests covering team-scoped selection of private keys and Git sources. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/Livewire/Project/Application/Source.php | 12 +- .../ApplicationSourceCrossTeamTest.php | 127 ++++++++++++++++++ 2 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 tests/Feature/ApplicationSourceCrossTeamTest.php diff --git a/app/Livewire/Project/Application/Source.php b/app/Livewire/Project/Application/Source.php index 3ef5ccf7c..f14689ee0 100644 --- a/app/Livewire/Project/Application/Source.php +++ b/app/Livewire/Project/Application/Source.php @@ -3,6 +3,8 @@ namespace App\Livewire\Project\Application; use App\Models\Application; +use App\Models\GithubApp; +use App\Models\GitlabApp; use App\Models\PrivateKey; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Attributes\Locked; @@ -21,7 +23,7 @@ class Source extends Component #[Validate(['nullable', 'string'])] public ?string $privateKeyName = null; - #[Validate(['nullable', 'integer'])] + #[Locked] public ?int $privateKeyId = null; #[Validate(['required', 'string'])] @@ -103,7 +105,8 @@ class Source extends Component { try { $this->authorize('update', $this->application); - $this->privateKeyId = $privateKeyId; + $key = PrivateKey::ownedByCurrentTeam()->findOrFail($privateKeyId); + $this->privateKeyId = $key->id; $this->syncData(true); $this->getPrivateKeys(); $this->application->refresh(); @@ -136,8 +139,11 @@ class Source extends Component try { $this->authorize('update', $this->application); + $allowedSourceTypes = [GithubApp::class, GitlabApp::class]; + abort_unless(in_array($sourceType, $allowedSourceTypes, true), 404); + $source = $sourceType::ownedByCurrentTeam()->findOrFail($sourceId); $this->application->update([ - 'source_id' => $sourceId, + 'source_id' => $source->id, 'source_type' => $sourceType, ]); diff --git a/tests/Feature/ApplicationSourceCrossTeamTest.php b/tests/Feature/ApplicationSourceCrossTeamTest.php new file mode 100644 index 000000000..fc6f920e3 --- /dev/null +++ b/tests/Feature/ApplicationSourceCrossTeamTest.php @@ -0,0 +1,127 @@ + $name, + 'private_key' => "-----BEGIN OPENSSH PRIVATE KEY-----\n{$material}\n-----END OPENSSH PRIVATE KEY-----", + 'fingerprint' => $fingerprint, + 'team_id' => $teamId, + ]); + $key->uuid = (string) new Cuid2; + $key->save(); + + return $key; + }); +} + +beforeEach(function () { + // handleError() turns a ModelNotFoundException into abort(404); rendering the 404 + // page reads InstanceSettings::get(), which findOrFail(0)s. Seed the singleton row. + // `id` is not in $fillable, so it must be set outside of mass assignment. + if (! InstanceSettings::find(0)) { + $settings = new InstanceSettings; + $settings->id = 0; + $settings->save(); + } + + // Team A — the attacker + $this->userA = User::factory()->create(); + $this->teamA = Team::factory()->create(); + $this->teamA->members()->attach($this->userA->id, ['role' => 'owner']); + $this->projectA = Project::factory()->create(['team_id' => $this->teamA->id]); + $this->environmentA = Environment::factory()->create(['project_id' => $this->projectA->id]); + $this->applicationA = Application::factory()->create([ + 'environment_id' => $this->environmentA->id, + 'private_key_id' => null, + 'source_id' => null, + 'source_type' => null, + ]); + + // Team B — the victim (holds the secrets we are trying to steal) + $this->teamB = Team::factory()->create(); + + $this->victimPrivateKey = makePrivateKey('victim-ssh-key', 'VICTIM_KEY_MATERIAL', 'victim-fingerprint', $this->teamB->id); + + $this->victimGithubApp = GithubApp::create([ + 'name' => 'victim-github-app', + 'team_id' => $this->teamB->id, + 'private_key_id' => $this->victimPrivateKey->id, + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'is_public' => false, + ]); + + $this->actingAs($this->userA); + session(['currentTeam' => $this->teamA]); +}); + +test('setPrivateKey rejects a PrivateKey owned by another team (GHSA-xrvp-4pp4-8rrw)', function () { + Livewire::test(Source::class, ['application' => $this->applicationA]) + ->call('setPrivateKey', $this->victimPrivateKey->id); + + $this->applicationA->refresh(); + expect($this->applicationA->private_key_id)->not->toBe($this->victimPrivateKey->id); + expect($this->applicationA->private_key_id)->toBeNull(); +}); + +test('setPrivateKey accepts a PrivateKey owned by the current team', function () { + $ownKey = makePrivateKey('own-ssh-key', 'OWN_KEY_MATERIAL', 'own-fingerprint', $this->teamA->id); + + Livewire::test(Source::class, ['application' => $this->applicationA]) + ->call('setPrivateKey', $ownKey->id); + + $this->applicationA->refresh(); + expect($this->applicationA->private_key_id)->toBe($ownKey->id); +}); + +test('changeSource rejects a GithubApp owned by another team (GHSA-xrvp-4pp4-8rrw)', function () { + Livewire::test(Source::class, ['application' => $this->applicationA]) + ->call('changeSource', $this->victimGithubApp->id, GithubApp::class); + + $this->applicationA->refresh(); + expect($this->applicationA->source_id)->not->toBe($this->victimGithubApp->id); + expect($this->applicationA->source_type)->not->toBe(GithubApp::class); +}); + +test('changeSource rejects an arbitrary class as source_type', function () { + Livewire::test(Source::class, ['application' => $this->applicationA]) + ->call('changeSource', $this->victimGithubApp->id, Server::class); + + $this->applicationA->refresh(); + expect($this->applicationA->source_type)->not->toBe(Server::class); +}); + +test('privateKeyId is locked so submit() cannot persist a client-supplied foreign id', function () { + // Without #[Locked], an attacker could POST {"updates": {"privateKeyId": }, + // "calls": [{"method": "submit"}]} and have syncData(true) write the foreign id through + // Application::update(['private_key_id' => $this->privateKeyId]) — bypassing setPrivateKey() + // and its team-scoped lookup entirely. Locking the property closes that path at the wire layer. + Livewire::test(Source::class, ['application' => $this->applicationA]) + ->set('privateKeyId', $this->victimPrivateKey->id); +})->throws(CannotUpdateLockedPropertyException::class);