Merge pull request #6918 from coollabsio/andrasbacsai/fix-private-key-exposure

Fix Private Key Access Policy for Null Team IDs
2025-10-17 23:07:39 +02:00 · 2025-10-17 23:07:39 +02:00 · 58864b9b20
commit 58864b9b20
parent 691fdffa8b f7427fdea0
8 changed files with 334 additions and 37 deletions
--- a/app/Livewire/Security/PrivateKey/Index.php
+++ b/app/Livewire/Security/PrivateKey/Index.php
@ -12,7 +12,7 @@ class Index extends Component

    public function render()
    {
-        $privateKeys = PrivateKey::ownedByCurrentTeam(['name', 'uuid', 'is_git_related', 'description'])->get();
+        $privateKeys = PrivateKey::ownedByCurrentTeam(['name', 'uuid', 'is_git_related', 'description', 'team_id'])->get();

        return view('livewire.security.private-key.index', [
            'privateKeys' => $privateKeys,
--- a/app/Livewire/Security/PrivateKey/Show.php
+++ b/app/Livewire/Security/PrivateKey/Show.php
@ -79,8 +79,14 @@ private function syncData(bool $toModel = false): void
    public function mount()
    {
        try {
-            $this->private_key = PrivateKey::ownedByCurrentTeam(['name', 'description', 'private_key', 'is_git_related'])->whereUuid(request()->private_key_uuid)->firstOrFail();
+            $this->private_key = PrivateKey::ownedByCurrentTeam(['name', 'description', 'private_key', 'is_git_related', 'team_id'])->whereUuid(request()->private_key_uuid)->firstOrFail();
+
+            // Explicit authorization check - will throw 403 if not authorized
+            $this->authorize('view', $this->private_key);
+
            $this->syncData(false);
+        } catch (\Illuminate\Auth\Access\AuthorizationException $e) {
+            abort(403, 'You do not have permission to view this private key.');
        } catch (\Throwable) {
            abort(404);
        }
--- a/app/Models/PrivateKey.php
+++ b/app/Models/PrivateKey.php
@ -82,9 +82,10 @@ public function getPublicKey()

    public static function ownedByCurrentTeam(array $select = ['*'])
    {
+        $teamId = currentTeam()->id;
        $selectArray = collect($select)->concat(['id']);

-        return self::whereTeamId(currentTeam()->id)->select($selectArray->all());
+        return self::whereTeamId($teamId)->select($selectArray->all());
    }

    public static function validatePrivateKey($privateKey)
--- a/app/Models/User.php
+++ b/app/Models/User.php
@ -338,6 +338,39 @@ public function role()
        return data_get($user, 'pivot.role');
    }

+    /**
+     * Check if the user is an admin or owner of a specific team
+     */
+    public function isAdminOfTeam(int $teamId): bool
+    {
+        $team = $this->teams->where('id', $teamId)->first();
+
+        if (! $team) {
+            return false;
+        }
+
+        $role = $team->pivot->role ?? null;
+
+        return $role === 'admin' || $role === 'owner';
+    }
+
+    /**
+     * Check if the user can access system resources (team_id=0)
+     * Must be admin/owner of root team
+     */
+    public function canAccessSystemResources(): bool
+    {
+        // Check if user is member of root team
+        $rootTeam = $this->teams->where('id', 0)->first();
+
+        if (! $rootTeam) {
+            return false;
+        }
+
+        // Check if user is admin or owner of root team
+        return $this->isAdminOfTeam(0);
+    }
+
    public function requestEmailChange(string $newEmail): void
    {
        // Generate 6-digit code
--- a/app/Policies/PrivateKeyPolicy.php
+++ b/app/Policies/PrivateKeyPolicy.php
@ -20,8 +20,18 @@ public function viewAny(User $user): bool
     */
    public function view(User $user, PrivateKey $privateKey): bool
    {
-        // return $user->teams->contains('id', $privateKey->team_id);
-        return true;
+        // Handle null team_id
+        if ($privateKey->team_id === null) {
+            return false;
+        }
+
+        // System resource (team_id=0): Only root team admins/owners can access
+        if ($privateKey->team_id === 0) {
+            return $user->canAccessSystemResources();
+        }
+
+        // Regular resource: Check team membership
+        return $user->teams->contains('id', $privateKey->team_id);
    }

    /**
@ -29,8 +39,9 @@ public function view(User $user, PrivateKey $privateKey): bool
     */
    public function create(User $user): bool
    {
-        // return $user->isAdmin();
-        return true;
+        // Only admins/owners can create private keys
+        // Members should not be able to create SSH keys that could be used for deployments
+        return $user->isAdmin();
    }

    /**
@ -38,8 +49,19 @@ public function create(User $user): bool
     */
    public function update(User $user, PrivateKey $privateKey): bool
    {
-        // return $user->isAdmin() && $user->teams->contains('id', $privateKey->team_id);
-        return true;
+        // Handle null team_id
+        if ($privateKey->team_id === null) {
+            return false;
+        }
+
+        // System resource (team_id=0): Only root team admins/owners can update
+        if ($privateKey->team_id === 0) {
+            return $user->canAccessSystemResources();
+        }
+
+        // Regular resource: Must be admin/owner of the team
+        return $user->isAdminOfTeam($privateKey->team_id)
+            && $user->teams->contains('id', $privateKey->team_id);
    }

    /**
@ -47,8 +69,19 @@ public function update(User $user, PrivateKey $privateKey): bool
     */
    public function delete(User $user, PrivateKey $privateKey): bool
    {
-        //  return $user->isAdmin() && $user->teams->contains('id', $privateKey->team_id);
-        return true;
+        // Handle null team_id
+        if ($privateKey->team_id === null) {
+            return false;
+        }
+
+        // System resource (team_id=0): Only root team admins/owners can delete
+        if ($privateKey->team_id === 0) {
+            return $user->canAccessSystemResources();
+        }
+
+        // Regular resource: Must be admin/owner of the team
+        return $user->isAdminOfTeam($privateKey->team_id)
+            && $user->teams->contains('id', $privateKey->team_id);
    }

    /**
--- a/package-lock.json
+++ b/package-lock.json
@ -916,8 +916,7 @@
            "resolved": "https://registry.npmjs.org/@socket.io/component-emitter/-/component-emitter-3.1.2.tgz",
            "integrity": "sha512-9BCxFwvbGg/RsZK9tjXd8s4UcwR0MWeFQ1XEKIQVVvAGJyINdrqKMcTRyLoK8Rse1GjzLV9cwjWV1olXRWEXVA==",
            "dev": true,
-            "license": "MIT",
-            "peer": true
+            "license": "MIT"
        },
        "node_modules/@tailwindcss/forms": {
            "version": "0.5.10",
@ -1372,7 +1371,8 @@
            "version": "5.5.0",
            "resolved": "https://registry.npmjs.org/@xterm/xterm/-/xterm-5.5.0.tgz",
            "integrity": "sha512-hqJHYaQb5OptNunnyAnkHyM8aCjZ1MEIDTQu1iIbbTD/xops91NB5yq1ZK/dC2JDbVWtF23zUtl9JE2NqwT87A==",
-            "license": "MIT"
+            "license": "MIT",
+            "peer": true
        },
        "node_modules/asynckit": {
            "version": "0.4.0",
@ -1535,7 +1535,6 @@
            "integrity": "sha512-T0iLjnyNWahNyv/lcjS2y4oE358tVS/SYQNxYXGAJ9/GLgH4VCvOQ/mhTjqU88mLZCQgiG8RIegFHYCdVC+j5w==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "@socket.io/component-emitter": "~3.1.0",
                "debug": "~4.3.1",
@ -1550,7 +1549,6 @@
            "integrity": "sha512-Er2nc/H7RrMXZBFCEim6TCmMk02Z8vLC2Rbi1KEBggpo0fS6l0S1nnapwmIi3yW/+GOJap1Krg4w0Hg80oCqgQ==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "ms": "^2.1.3"
            },
@ -1569,7 +1567,6 @@
            "integrity": "sha512-HqD3yTBfnBxIrbnM1DoD6Pcq8NECnh8d4As1Qgh0z5Gg3jRRIqijury0CL3ghu/edArpUYiYqQiDUQBIs4np3Q==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "engines": {
                "node": ">=10.0.0"
            }
@ -2331,6 +2328,7 @@
            "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==",
            "dev": true,
            "license": "MIT",
+            "peer": true,
            "engines": {
                "node": ">=12"
            },
@ -2407,6 +2405,7 @@
            "integrity": "sha512-wp3HqIIUc1GRyu1XrP6m2dgyE9MoCsXVsWNlohj0rjSkLf+a0jLvEyVubdg58oMk7bhjBWnFClgp8jfAa6Ak4Q==",
            "dev": true,
            "license": "MIT",
+            "peer": true,
            "dependencies": {
                "tweetnacl": "^1.0.3"
            }
@ -2491,7 +2490,6 @@
            "integrity": "sha512-hJVXfu3E28NmzGk8o1sHhN3om52tRvwYeidbj7xKy2eIIse5IoKX3USlS6Tqt3BHAtflLIkCQBkzVrEEfWUyYQ==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "@socket.io/component-emitter": "~3.1.0",
                "debug": "~4.3.2",
@ -2508,7 +2506,6 @@
            "integrity": "sha512-Er2nc/H7RrMXZBFCEim6TCmMk02Z8vLC2Rbi1KEBggpo0fS6l0S1nnapwmIi3yW/+GOJap1Krg4w0Hg80oCqgQ==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "ms": "^2.1.3"
            },
@ -2527,7 +2524,6 @@
            "integrity": "sha512-/GbIKmo8ioc+NIWIhwdecY0ge+qVBSMdgxGygevmdHj24bsfgtCmcUUcQ5ZzcylGFHsN3k4HB4Cgkl96KVnuew==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "@socket.io/component-emitter": "~3.1.0",
                "debug": "~4.3.1"
@ -2542,7 +2538,6 @@
            "integrity": "sha512-Er2nc/H7RrMXZBFCEim6TCmMk02Z8vLC2Rbi1KEBggpo0fS6l0S1nnapwmIi3yW/+GOJap1Krg4w0Hg80oCqgQ==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "dependencies": {
                "ms": "^2.1.3"
            },
@ -2591,7 +2586,8 @@
            "version": "4.1.10",
            "resolved": "https://registry.npmjs.org/tailwindcss/-/tailwindcss-4.1.10.tgz",
            "integrity": "sha512-P3nr6WkvKV/ONsTzj6Gb57sWPMX29EPNPopo7+FcpkQaNsrNpZ1pv8QmrYI2RqEKD7mlGqLnGovlcYnBK0IqUA==",
-            "license": "MIT"
+            "license": "MIT",
+            "peer": true
        },
        "node_modules/tapable": {
            "version": "2.3.0",
@ -2660,6 +2656,7 @@
            "integrity": "sha512-0msEVHJEScQbhkbVTb/4iHZdJ6SXp/AvxL2sjwYQFfBqleHtnCqv1J3sa9zbWz/6kW1m9Tfzn92vW+kZ1WV6QA==",
            "dev": true,
            "license": "MIT",
+            "peer": true,
            "dependencies": {
                "esbuild": "^0.25.0",
                "fdir": "^6.4.4",
@ -2759,6 +2756,7 @@
            "integrity": "sha512-rjOV2ecxMd5SiAmof2xzh2WxntRcigkX/He4YFJ6WdRvVUrbt6DxC1Iujh10XLl8xCDRDtGKMeO3D+pRQ1PP9w==",
            "dev": true,
            "license": "MIT",
+            "peer": true,
            "dependencies": {
                "@vue/compiler-dom": "3.5.16",
                "@vue/compiler-sfc": "3.5.16",
@ -2781,7 +2779,6 @@
            "integrity": "sha512-6XQFvXTkbfUOZOKKILFG1PDK2NDQs4azKQl26T0YS5CxqWLgXajbPZ+h4gZekJyRqFU8pvnbAbbs/3TgRPy+GQ==",
            "dev": true,
            "license": "MIT",
-            "peer": true,
            "engines": {
                "node": ">=10.0.0"
            },
@ -2803,7 +2800,6 @@
            "resolved": "https://registry.npmjs.org/xmlhttprequest-ssl/-/xmlhttprequest-ssl-2.1.2.tgz",
            "integrity": "sha512-TEU+nJVUUnA4CYJFLvK5X9AOeH4KvDvhIfm0vV1GaQRtchnG0hgK5p8hw/xjv8cunWYCsiPCSDzObPyhEwq3KQ==",
            "dev": true,
-            "peer": true,
            "engines": {
                "node": ">=0.4.0"
            }
--- a/resources/views/livewire/security/private-key/index.blade.php
+++ b/resources/views/livewire/security/private-key/index.blade.php
@ -14,22 +14,41 @@
    </div>
    <div class="grid gap-4 lg:grid-cols-2">
        @forelse ($privateKeys as $key)
-            <a class="box group"
-                href="{{ route('security.private-key.show', ['private_key_uuid' => data_get($key, 'uuid')]) }}">
-                <div class="flex flex-col justify-center mx-6">
-                    <div class="box-title">
-                        {{ data_get($key, 'name') }}
+            @can('view', $key)
+                {{-- Admin/Owner: Clickable link --}}
+                <a class="box group"
+                    href="{{ route('security.private-key.show', ['private_key_uuid' => data_get($key, 'uuid')]) }}">
+                    <div class="flex flex-col justify-center mx-6">
+                        <div class="box-title">
+                            {{ data_get($key, 'name') }}
+                        </div>
+                        <div class="box-description">
+                            {{ $key->description }}
+                            @if (!$key->isInUse())
+                                <span
+                                    class="inline-flex items-center px-2 py-0.5 rounded-sm text-xs font-medium bg-yellow-400 text-black">Unused</span>
+                            @endif
+                        </div>
                    </div>
-                    <div class="box-description">
-                        {{ $key->description }}
-                        @if (!$key->isInUse())
-                            <span
-                                class="inline-flex items-center px-2 py-0.5 rounded-sm text-xs font-medium bg-yellow-400 text-black">Unused</span>
-                        @endif
+                </a>
+            @else
+                {{-- Member: Visible but not clickable --}}
+                <div class="box opacity-60 cursor-not-allowed hover:bg-transparent dark:hover:bg-transparent" title="You don't have permission to view this private key">
+                    <div class="flex flex-col justify-center mx-6">
+                        <div class="box-title">
+                            {{ data_get($key, 'name') }}
+                            <span class="ml-2 inline-flex items-center px-2 py-0.5 rounded-sm text-xs font-medium bg-gray-400 dark:bg-gray-600 text-white">View Only</span>
+                        </div>
+                        <div class="box-description">
+                            {{ $key->description }}
+                            @if (!$key->isInUse())
+                                <span
+                                    class="inline-flex items-center px-2 py-0.5 rounded-sm text-xs font-medium bg-yellow-400 text-black">Unused</span>
+                            @endif
+                        </div>
                    </div>
-
                </div>
-            </a>
+            @endcan
        @empty
            <div>No private keys found.</div>
        @endforelse
--- a/tests/Unit/Policies/PrivateKeyPolicyTest.php
+++ b/tests/Unit/Policies/PrivateKeyPolicyTest.php
@ -0,0 +1,209 @@
+<?php
+
+use App\Models\PrivateKey;
+use App\Models\User;
+use App\Policies\PrivateKeyPolicy;
+
+it('allows root team admin to view system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'admin']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeTrue();
+});
+
+it('allows root team owner to view system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'owner']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeTrue();
+});
+
+it('denies regular member of root team to view system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'member']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeFalse();
+});
+
+it('denies non-root team member to view system private key', function () {
+    $teams = collect([
+        (object) ['id' => 1, 'pivot' => (object) ['role' => 'owner']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeFalse();
+});
+
+it('allows team member to view their own team private key', function () {
+    $teams = collect([
+        (object) ['id' => 1, 'pivot' => (object) ['role' => 'member']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 1;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeTrue();
+});
+
+it('denies team member to view another team private key', function () {
+    $teams = collect([
+        (object) ['id' => 1, 'pivot' => (object) ['role' => 'owner']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 2;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->view($user, $privateKey))->toBeFalse();
+});
+
+it('allows root team admin to update system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'admin']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->update($user, $privateKey))->toBeTrue();
+});
+
+it('denies root team member to update system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'member']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->update($user, $privateKey))->toBeFalse();
+});
+
+it('allows team admin to update their own team private key', function () {
+    $teams = collect([
+        (object) ['id' => 1, 'pivot' => (object) ['role' => 'admin']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 1;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->update($user, $privateKey))->toBeTrue();
+});
+
+it('denies team member to update their own team private key', function () {
+    $teams = collect([
+        (object) ['id' => 1, 'pivot' => (object) ['role' => 'member']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 1;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->update($user, $privateKey))->toBeFalse();
+});
+
+it('allows root team admin to delete system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'admin']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->delete($user, $privateKey))->toBeTrue();
+});
+
+it('denies root team member to delete system private key', function () {
+    $teams = collect([
+        (object) ['id' => 0, 'pivot' => (object) ['role' => 'member']],
+    ]);
+
+    $user = Mockery::mock(User::class)->makePartial();
+    $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams);
+
+    $privateKey = new class
+    {
+        public $team_id = 0;
+    };
+
+    $policy = new PrivateKeyPolicy;
+    expect($policy->delete($user, $privateKey))->toBeFalse();
+});