From 75976d9882313a1851361b655ab65a9a0349e7dc Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:13:20 +0200 Subject: [PATCH] fix(keys): check duplicate private keys in the key's own team The duplicate check used the signed-in user's current team, or all teams when there was none. Check the team the key belongs to. Co-Authored-By: Claude Opus 5.5 --- .../Controllers/Api/SecurityController.php | 2 +- app/Models/PrivateKey.php | 25 ++-- .../PrivateKeyFingerprintTeamScopeTest.php | 135 ++++++++++++++++++ 3 files changed, 152 insertions(+), 10 deletions(-) create mode 100644 tests/Feature/PrivateKeyFingerprintTeamScopeTest.php diff --git a/app/Http/Controllers/Api/SecurityController.php b/app/Http/Controllers/Api/SecurityController.php index 25430631b9..036a6e9328 100644 --- a/app/Http/Controllers/Api/SecurityController.php +++ b/app/Http/Controllers/Api/SecurityController.php @@ -225,7 +225,7 @@ class SecurityController extends Controller ], 422); } $fingerPrint = PrivateKey::generateFingerprint($request->private_key); - $isFingerPrintExists = PrivateKey::fingerprintExists($fingerPrint); + $isFingerPrintExists = PrivateKey::fingerprintExists($fingerPrint, teamId: $teamId); if ($isFingerPrintExists) { return response()->json([ 'message' => 'Private key already exists.', diff --git a/app/Models/PrivateKey.php b/app/Models/PrivateKey.php index 43aa310cbc..22689dc055 100644 --- a/app/Models/PrivateKey.php +++ b/app/Models/PrivateKey.php @@ -65,7 +65,7 @@ class PrivateKey extends BaseModel } $key->fingerprint = self::generateFingerprint($key->private_key); - if (self::fingerprintExists($key->fingerprint, $key->id)) { + if (self::fingerprintExists($key->fingerprint, $key->id, $key->team_id)) { throw ValidationException::withMessages([ 'private_key' => ['This private key already exists.'], ]); @@ -365,17 +365,24 @@ class PrivateKey extends BaseModel } } - public static function fingerprintExists($fingerprint, $excludeId = null) + /** + * Whether another key with this fingerprint exists in the given team. + * + * The check is always limited to $teamId, never to the signed in user's + * team or to all teams. Without a team id nothing can match, because every + * key belongs to a team. + */ + public static function fingerprintExists(?string $fingerprint, ?int $excludeId = null, ?int $teamId = null): bool { - $query = self::query() - ->where('fingerprint', $fingerprint) - ->where('id', '!=', $excludeId); - - if (currentTeam()) { - $query->where('team_id', currentTeam()->id); + if (is_null($fingerprint) || is_null($teamId)) { + return false; } - return $query->exists(); + return self::query() + ->where('team_id', $teamId) + ->where('fingerprint', $fingerprint) + ->when(! is_null($excludeId), fn ($query) => $query->whereKeyNot($excludeId)) + ->exists(); } public static function cleanupUnusedKeys() diff --git a/tests/Feature/PrivateKeyFingerprintTeamScopeTest.php b/tests/Feature/PrivateKeyFingerprintTeamScopeTest.php new file mode 100644 index 0000000000..206bed51fe --- /dev/null +++ b/tests/Feature/PrivateKeyFingerprintTeamScopeTest.php @@ -0,0 +1,135 @@ +set('app.maintenance.driver', 'file'); + config()->set('cache.default', 'array'); + + InstanceSettings::forceCreate(['id' => 0, 'is_api_enabled' => true]); + + $this->keyMaterial = generateSSHKey('ed25519')['private']; + $this->teamA = Team::factory()->create(); + $this->teamB = Team::factory()->create(); +}); + +function createTeamScopedPrivateKey(Team $team, string $material, string $name = 'key'): PrivateKey +{ + return PrivateKey::create([ + 'name' => $name, + 'private_key' => $material, + 'team_id' => $team->id, + ]); +} + +test('the same key can be saved in two different teams', function () { + $first = createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + $second = createTeamScopedPrivateKey($this->teamB, $this->keyMaterial); + + expect($first->fingerprint)->toBe($second->fingerprint) + ->and(PrivateKey::query()->where('fingerprint', $first->fingerprint)->count())->toBe(2); +}); + +test('a duplicate key in the same team is rejected', function () { + createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + + expect(fn () => createTeamScopedPrivateKey($this->teamA, $this->keyMaterial, 'duplicate')) + ->toThrow(ValidationException::class, 'This private key already exists.'); +}); + +test('updating a key does not collide with itself', function () { + $key = createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + + $key->update(['name' => 'renamed']); + + expect($key->fresh()->name)->toBe('renamed'); +}); + +test('the root team (id 0) is scoped like any other team', function () { + $rootTeam = Team::factory()->create(['id' => 0]); + createTeamScopedPrivateKey($rootTeam, $this->keyMaterial); + $fingerprint = PrivateKey::generateFingerprint($this->keyMaterial); + + expect(PrivateKey::fingerprintExists($fingerprint, teamId: 0))->toBeTrue() + ->and(PrivateKey::fingerprintExists($fingerprint, teamId: $this->teamA->id))->toBeFalse() + ->and(fn () => createTeamScopedPrivateKey($rootTeam, $this->keyMaterial, 'duplicate')) + ->toThrow(ValidationException::class, 'This private key already exists.'); + + createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); +}); + +test('the saving hook checks the team of the key, not the team of the signed in user', function () { + $user = User::factory()->create(); + $this->teamA->members()->attach($user->id, ['role' => 'owner']); + $this->teamB->members()->attach($user->id, ['role' => 'owner']); + $this->actingAs($user); + session(['currentTeam' => $this->teamA]); + + createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + + // Another team may hold the same key while team A is the current team. + createTeamScopedPrivateKey($this->teamB, $this->keyMaterial); + + // A duplicate in team B is rejected even though team A is the current team. + expect(fn () => createTeamScopedPrivateKey($this->teamB, $this->keyMaterial, 'duplicate')) + ->toThrow(ValidationException::class, 'This private key already exists.'); +}); + +test('without a signed in user the check does not see keys of other teams', function () { + expect(auth()->check())->toBeFalse(); + $existing = createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + + expect(PrivateKey::fingerprintExists($existing->fingerprint))->toBeFalse() + ->and(PrivateKey::fingerprintExists($existing->fingerprint, teamId: $this->teamB->id))->toBeFalse() + ->and(PrivateKey::fingerprintExists($existing->fingerprint, teamId: $this->teamA->id))->toBeTrue() + ->and(PrivateKey::fingerprintExists($existing->fingerprint, $existing->id, $this->teamA->id))->toBeFalse(); + + $copy = createTeamScopedPrivateKey($this->teamB, $this->keyMaterial); + + expect($copy->exists)->toBeTrue(); +}); + +describe('POST /api/v1/security/keys', function () { + beforeEach(function () { + $this->user = User::factory()->create(); + $this->teamA->members()->attach($this->user->id, ['role' => 'owner']); + $this->teamB->members()->attach($this->user->id, ['role' => 'owner']); + + session(['currentTeam' => $this->teamA]); + $token = $this->user->createToken('write-token', ['write']); + DB::table('personal_access_tokens')->where('id', $token->accessToken->id)->update(['team_id' => $this->teamA->id]); + $this->bearerToken = $token->plainTextToken; + }); + + test('a key that exists in another team is created in the token team', function () { + createTeamScopedPrivateKey($this->teamB, $this->keyMaterial); + // The browser session points to team B; the API must use the token team. + session(['currentTeam' => $this->teamB]); + + $response = $this->withHeader('Authorization', 'Bearer '.$this->bearerToken) + ->postJson('/api/v1/security/keys', ['private_key' => $this->keyMaterial]); + + $response->assertCreated(); + expect(PrivateKey::query()->where('uuid', $response->json('uuid'))->value('team_id'))->toBe($this->teamA->id); + }); + + test('a key that exists in the token team is rejected', function () { + createTeamScopedPrivateKey($this->teamA, $this->keyMaterial); + session(['currentTeam' => $this->teamB]); + + $this->withHeader('Authorization', 'Bearer '.$this->bearerToken) + ->postJson('/api/v1/security/keys', ['private_key' => $this->keyMaterial]) + ->assertStatus(422) + ->assertJson(['message' => 'Private key already exists.']); + + expect(PrivateKey::query()->where('team_id', $this->teamA->id)->count())->toBe(1); + }); +});