diff --git a/app/Http/Controllers/Api/GithubController.php b/app/Http/Controllers/Api/GithubController.php index 5c073e9c0a..840a11f692 100644 --- a/app/Http/Controllers/Api/GithubController.php +++ b/app/Http/Controllers/Api/GithubController.php @@ -15,9 +15,9 @@ use OpenApi\Attributes as OA; class GithubController extends Controller { - private function removeSensitiveData($githubApp) + private function removeSensitiveData(GithubApp $githubApp, int $teamId) { - if (request()->attributes->get('can_read_sensitive', false) === true) { + if (request()->attributes->get('can_read_sensitive', false) === true && $githubApp->team_id === $teamId) { $githubApp->makeVisible([ 'client_secret', 'webhook_secret', @@ -97,8 +97,8 @@ class GithubController extends Controller ->orWhere('is_system_wide', true); })->get(); - $githubApps = $githubApps->map(function ($app) { - return $this->removeSensitiveData($app); + $githubApps = $githubApps->map(function ($app) use ($teamId) { + return $this->removeSensitiveData($app, $teamId); }); return response()->json($githubApps); diff --git a/app/Http/Controllers/Api/GitlabController.php b/app/Http/Controllers/Api/GitlabController.php index c907af46f3..959a3067aa 100644 --- a/app/Http/Controllers/Api/GitlabController.php +++ b/app/Http/Controllers/Api/GitlabController.php @@ -13,9 +13,9 @@ use OpenApi\Attributes as OA; class GitlabController extends Controller { - private function removeSensitiveData(GitlabApp $gitlabApp) + private function removeSensitiveData(GitlabApp $gitlabApp, int $teamId) { - if (request()->attributes->get('can_read_sensitive', false) === true) { + if (request()->attributes->get('can_read_sensitive', false) === true && $gitlabApp->team_id === $teamId) { $gitlabApp->makeVisible([ 'client_secret', 'webhook_token', @@ -108,8 +108,8 @@ class GitlabController extends Controller ->orWhere('is_system_wide', true); })->get(); - $gitlabApps = $gitlabApps->map(function ($app) { - return $this->removeSensitiveData($app); + $gitlabApps = $gitlabApps->map(function ($app) use ($teamId) { + return $this->removeSensitiveData($app, $teamId); }); return response()->json($gitlabApps); @@ -280,7 +280,7 @@ class GitlabController extends Controller 'gitlab_app_name' => $gitlabApp->name, ]); - return response()->json($this->removeSensitiveData($gitlabApp->fresh()), 201); + return response()->json($this->removeSensitiveData($gitlabApp->fresh(), $teamId), 201); } catch (\Throwable $e) { return handleError($e); } @@ -441,7 +441,7 @@ class GitlabController extends Controller return response()->json([ 'message' => 'GitLab app updated successfully', - 'data' => $this->removeSensitiveData($gitlabApp->fresh()), + 'data' => $this->removeSensitiveData($gitlabApp->fresh(), $teamId), ]); } catch (ModelNotFoundException $e) { return response()->json([ diff --git a/app/Http/Controllers/Api/TeamController.php b/app/Http/Controllers/Api/TeamController.php index 35e01c8314..b9f8572673 100644 --- a/app/Http/Controllers/Api/TeamController.php +++ b/app/Http/Controllers/Api/TeamController.php @@ -56,7 +56,7 @@ class TeamController extends Controller if (is_null($teamId)) { return invalidTokenResponse(); } - $teams = auth()->user()->teams->sortBy('id'); + $teams = auth()->user()->teams->where('id', $teamId)->values(); $teams = $teams->map(function ($team) { return $this->removeSensitiveData($team); }); @@ -100,13 +100,14 @@ class TeamController extends Controller )] public function team_by_id(Request $request) { - $id = $request->id; $teamId = getTeamIdFromToken(); if (is_null($teamId)) { return invalidTokenResponse(); } - $teams = auth()->user()->teams; - $team = $teams->where('id', $id)->first(); + if ((int) $request->id !== (int) $teamId) { + return response()->json(['message' => 'Team not found.'], 404); + } + $team = auth()->user()->teams->where('id', $teamId)->first(); if (is_null($team)) { return response()->json(['message' => 'Team not found.'], 404); } @@ -159,13 +160,14 @@ class TeamController extends Controller )] public function members_by_id(Request $request) { - $id = $request->id; $teamId = getTeamIdFromToken(); if (is_null($teamId)) { return invalidTokenResponse(); } - $teams = auth()->user()->teams; - $team = $teams->where('id', $id)->first(); + if ((int) $request->id !== (int) $teamId) { + return response()->json(['message' => 'Team not found.'], 404); + } + $team = auth()->user()->teams->where('id', $teamId)->first(); if (is_null($team)) { return response()->json(['message' => 'Team not found.'], 404); } diff --git a/tests/Feature/Api/GithubAppsListApiTest.php b/tests/Feature/Api/GithubAppsListApiTest.php index 9a1f1f3d25..cb15cd9347 100644 --- a/tests/Feature/Api/GithubAppsListApiTest.php +++ b/tests/Feature/Api/GithubAppsListApiTest.php @@ -197,6 +197,39 @@ describe('GET /api/v1/github-apps', function () { ]); }); + test('does not return system-wide github app secrets owned by another team', function () { + $otherTeam = Team::factory()->create(); + $otherPrivateKey = PrivateKey::create([ + 'name' => 'System Key', + 'private_key' => validGithubAppsApiPrivateKey(), + 'team_id' => $otherTeam->id, + ]); + GithubApp::create([ + 'name' => 'Foreign System GitHub App', + 'api_url' => 'https://api.github.com', + 'html_url' => 'https://github.com', + 'app_id' => 11111, + 'installation_id' => 22222, + 'client_id' => 'system-client-id', + 'client_secret' => 'foreign-client-secret', + 'webhook_secret' => 'foreign-webhook-secret', + 'private_key_id' => $otherPrivateKey->id, + 'team_id' => $otherTeam->id, + 'is_system_wide' => true, + ]); + + $sensitiveToken = createGithubAppsApiToken($this, ['read', 'read:sensitive']); + + $response = $this->withToken($sensitiveToken) + ->getJson('/api/v1/github-apps') + ->assertSuccessful() + ->assertJsonFragment(['name' => 'Foreign System GitHub App']); + + expect($response->json('0')) + ->not->toHaveKey('client_secret') + ->not->toHaveKey('webhook_secret'); + }); + test('does not return other teams github apps', function () { // Create a GitHub app for this team GithubApp::create([ diff --git a/tests/Feature/Api/GitlabAppsApiTest.php b/tests/Feature/Api/GitlabAppsApiTest.php index 65332c0dd6..f18829c205 100644 --- a/tests/Feature/Api/GitlabAppsApiTest.php +++ b/tests/Feature/Api/GitlabAppsApiTest.php @@ -64,6 +64,36 @@ describe('GET /api/v1/gitlab-apps', function () { expect($response->json('0'))->not->toHaveKey('client_secret') ->and($response->json('0'))->not->toHaveKey('webhook_token'); }); + + test('does not return system-wide gitlab app secrets owned by another team', function () { + $otherTeam = Team::factory()->create(); + GitlabApp::create([ + 'name' => 'Foreign System GitLab', + 'api_url' => 'https://gitlab.com/api/v4', + 'html_url' => 'https://gitlab.com', + 'client_id' => 'foreign-client-id', + 'client_secret' => 'foreign-client-secret', + 'webhook_token' => 'foreign-webhook-token', + 'access_token' => 'foreign-access-token', + 'refresh_token' => 'foreign-refresh-token', + 'team_id' => $otherTeam->id, + 'is_system_wide' => true, + ]); + + session(['currentTeam' => $this->team]); + $sensitiveToken = $this->user->createToken('sensitive-token', ['read', 'read:sensitive'])->plainTextToken; + + $response = $this->withToken($sensitiveToken) + ->getJson('/api/v1/gitlab-apps') + ->assertSuccessful() + ->assertJsonFragment(['name' => 'Foreign System GitLab']); + + expect($response->json('0')) + ->not->toHaveKey('client_secret') + ->not->toHaveKey('webhook_token') + ->not->toHaveKey('access_token') + ->not->toHaveKey('refresh_token'); + }); }); describe('POST /api/v1/gitlab-apps', function () { diff --git a/tests/Feature/Api/TeamTokenTeamApiTest.php b/tests/Feature/Api/TeamTokenTeamApiTest.php index 4f45562ea5..447579e294 100644 --- a/tests/Feature/Api/TeamTokenTeamApiTest.php +++ b/tests/Feature/Api/TeamTokenTeamApiTest.php @@ -8,6 +8,9 @@ use Illuminate\Foundation\Testing\RefreshDatabase; uses(RefreshDatabase::class); beforeEach(function () { + config()->set('app.maintenance.driver', 'file'); + config()->set('cache.default', 'array'); + InstanceSettings::unguarded(fn () => InstanceSettings::updateOrCreate(['id' => 0], ['is_api_enabled' => true])); $this->team = Team::factory()->create(['name' => 'Token Team']); @@ -27,6 +30,27 @@ function teamTokenApiHeaders(string $bearerToken): array } describe('token team endpoints', function () { + test('legacy team endpoints are restricted to the token team', function () { + $otherTeam = Team::factory()->create(['name' => 'Other Team']); + $otherMember = User::factory()->create(); + $otherTeam->members()->attach($this->user->id, ['role' => 'owner']); + $otherTeam->members()->attach($otherMember->id, ['role' => 'member']); + + $this->withHeaders(teamTokenApiHeaders($this->bearerToken)) + ->getJson('/api/v1/teams') + ->assertOk() + ->assertJsonCount(1) + ->assertJsonPath('0.id', $this->team->id); + + $this->withHeaders(teamTokenApiHeaders($this->bearerToken)) + ->getJson("/api/v1/teams/{$otherTeam->id}") + ->assertNotFound(); + + $this->withHeaders(teamTokenApiHeaders($this->bearerToken)) + ->getJson("/api/v1/teams/{$otherTeam->id}/members") + ->assertNotFound(); + }); + test('GET /team returns the token team', function () { $this->withHeaders(teamTokenApiHeaders($this->bearerToken)) ->getJson('/api/v1/team')