From e8addbb9595cb35b56ab8728348489b3060e10c6 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Mon, 6 Jul 2026 11:23:45 +0100 Subject: [PATCH 1/4] fix(Multivariate): Multivariate options exposed across projects on list `MultivariateFeatureOptionViewSet.get_queryset` resolved the feature by bare `feature_pk`, ignoring `project_pk`. The list action runs no object-level permission check, so any user with a project could read another project's multivariate options by enumerating feature ids. Scope the feature lookup to the URL's project, mirroring `create`. beep boop --- api/features/multivariate/views.py | 5 ++- .../test_unit_multivariate_views.py | 33 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/api/features/multivariate/views.py b/api/features/multivariate/views.py index 8045887eb59f..aecb0dee4a6d 100644 --- a/api/features/multivariate/views.py +++ b/api/features/multivariate/views.py @@ -76,7 +76,10 @@ def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): return MultivariateFeatureOption.objects.none() - feature = get_object_or_404(Feature, pk=self.kwargs["feature_pk"]) + feature = get_object_or_404( + Feature.objects.filter(project__id=self.kwargs["project_pk"]), + pk=self.kwargs["feature_pk"], + ) return feature.multivariate_options.all() diff --git a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py index b98f1323155e..3fb923d257b3 100644 --- a/api/tests/unit/features/multivariate/test_unit_multivariate_views.py +++ b/api/tests/unit/features/multivariate/test_unit_multivariate_views.py @@ -8,8 +8,12 @@ from django.urls import reverse from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from rest_framework import status +from rest_framework.test import APIClient +from features.models import Feature +from features.multivariate.models import MultivariateFeatureOption from features.multivariate.views import MultivariateFeatureOptionViewSet +from projects.models import Project from projects.permissions import NestedProjectPermissions @@ -72,3 +76,32 @@ def test_get_mv_feature_option_by_uuid__nonexistent_uuid__returns_404( # type: # Then assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_list_mv_options__feature_in_other_project__returns_404( + admin_client: APIClient, + project: Project, + organisation_two_project_one: Project, +) -> None: + # Given + feature = Feature.objects.create( + name="feature", + project=organisation_two_project_one, + type="MULTIVARIATE", + initial_value="control", + ) + for percentage_allocation in (30, 30, 40): + MultivariateFeatureOption.objects.create( + feature=feature, + default_percentage_allocation=percentage_allocation, + type="unicode", + string_value=f"multivariate option for {percentage_allocation}%", + ) + + url = f"/api/v1/projects/{project.id}/features/{feature.id}/mv-options/" + + # When + response = admin_client.get(url) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND From 89d50931aae101f68930e8a48b10fe710aeff7b0 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Mon, 6 Jul 2026 11:23:59 +0100 Subject: [PATCH 2/4] fix(Feature external resources): External resources exposed across projects on list `FeatureExternalResourceViewSet.get_queryset` filtered the list by bare `feature_pk`, and `FeaturePermissions` applies no project check on the list action ("handled by the view"). So any authenticated user could read another feature's linked issue/PR resources by enumerating feature ids. Scope the feature lookup to the caller's permitted projects, matching the pattern used by `FeatureViewSet`. beep boop --- .../feature_external_resources/views.py | 12 ++++++-- ...t_unit_feature_external_resources_views.py | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/api/features/feature_external_resources/views.py b/api/features/feature_external_resources/views.py index 253c69cb1b64..6bd959ca84cc 100644 --- a/api/features/feature_external_resources/views.py +++ b/api/features/feature_external_resources/views.py @@ -1,5 +1,6 @@ import re +from common.projects.permissions import VIEW_PROJECT from django.shortcuts import get_object_or_404 from django.utils.decorators import method_decorator from drf_spectacular.utils import extend_schema @@ -41,9 +42,14 @@ def get_queryset(self): # type: ignore[no-untyped-def] if "pk" in self.kwargs: return FeatureExternalResource.objects.filter(id=self.kwargs["pk"]) - else: - features_pk = self.kwargs["feature_pk"] - return FeatureExternalResource.objects.filter(feature=features_pk) + + feature = get_object_or_404( + Feature.objects.filter( + project__in=self.request.user.get_permitted_projects(VIEW_PROJECT) # type: ignore[union-attr] + ), + pk=self.kwargs["feature_pk"], + ) + return feature.external_resources.all() def list(self, request, *args, **kwargs) -> Response: # type: ignore[no-untyped-def] queryset = self.get_queryset() # type: ignore[no-untyped-call] diff --git a/api/tests/unit/features/test_unit_feature_external_resources_views.py b/api/tests/unit/features/test_unit_feature_external_resources_views.py index af38dfac4d2c..c00d128e8ba4 100644 --- a/api/tests/unit/features/test_unit_feature_external_resources_views.py +++ b/api/tests/unit/features/test_unit_feature_external_resources_views.py @@ -934,3 +934,31 @@ def test_create_feature_external_resource__duplicate_feature_and_url__returns_40 response.json()["non_field_errors"][0] == "The fields feature, url must make a unique set." ) + + +def test_list_feature_external_resources__feature_in_other_project__returns_404( + admin_client: APIClient, + project: Project, + organisation_two_project_one: Project, +) -> None: + # Given + feature = Feature.objects.create( + name="feature", project=organisation_two_project_one + ) + FeatureExternalResource.objects.create( + url="https://gitlab.com/owner/repo/-/issues/1", + type="GITLAB_ISSUE", + feature=feature, + metadata='{"status": "open"}', + ) + + url = ( + f"/api/v1/projects/{project.id}" + f"/features/{feature.id}/feature-external-resources/" + ) + + # When + response = admin_client.get(url) + + # Then + assert response.status_code == status.HTTP_404_NOT_FOUND From 670b62e76f628c3788ecdf8f7a1f0420778405f6 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Mon, 6 Jul 2026 11:24:11 +0100 Subject: [PATCH 3/4] fix(Environment permissions): Group permission assignments exposed to any authenticated user `UserPermissionGroupEnvironmentPermissionsViewSet` was gated only by `IsAuthenticated`, unlike its sibling `UserEnvironmentPermissionsViewSet` which also uses `NestedEnvironmentPermissions`. Since the environment client-side api key is not a secret, any authenticated user could read, and delete, another organisation's environment group permission assignments regardless of membership. Add `NestedEnvironmentPermissions` so the caller must hold the relevant permission on the environment. beep boop --- api/environments/permissions/views.py | 2 +- .../test_unit_environments_views.py | 72 ++++++++++++++++++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/api/environments/permissions/views.py b/api/environments/permissions/views.py index 0dbff15e7c4b..07d32d7e48b0 100644 --- a/api/environments/permissions/views.py +++ b/api/environments/permissions/views.py @@ -52,7 +52,7 @@ def perform_update(self, serializer): # type: ignore[no-untyped-def] class UserPermissionGroupEnvironmentPermissionsViewSet(viewsets.ModelViewSet): # type: ignore[type-arg] pagination_class = None - permission_classes = [IsAuthenticated] + permission_classes = [IsAuthenticated, NestedEnvironmentPermissions] def get_queryset(self): # type: ignore[no-untyped-def] if getattr(self, "swagger_fake_view", False): diff --git a/api/tests/unit/environments/permissions/test_unit_environments_views.py b/api/tests/unit/environments/permissions/test_unit_environments_views.py index b5597750423a..9d25b988b062 100644 --- a/api/tests/unit/environments/permissions/test_unit_environments_views.py +++ b/api/tests/unit/environments/permissions/test_unit_environments_views.py @@ -14,7 +14,7 @@ UserEnvironmentPermission, UserPermissionGroupEnvironmentPermission, ) -from organisations.models import Organisation +from organisations.models import Organisation, OrganisationRole from tests.types import WithEnvironmentPermissionsCallable from users.models import FFAdminUser, UserPermissionGroup @@ -273,3 +273,73 @@ def test_delete_user_group_permission__existing_permission__removes_permission( assert not UserPermissionGroupEnvironmentPermission.objects.filter( id=user_group_environment_permission.id ).exists() + + +def test_list_user_group_permission__different_organisation__returns_403( + environment: Environment, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + other_org_user = FFAdminUser.objects.create(email="other-org-user@example.com") + other_org_user.add_organisation(organisation_two, role=OrganisationRole.ADMIN) + other_org_client = APIClient() + other_org_client.force_authenticate(other_org_user) + + user_permission_group = UserPermissionGroup.objects.create( + name="Test group", + organisation=organisation, + ) + user_group_environment_permission = ( + UserPermissionGroupEnvironmentPermission.objects.create( + group=user_permission_group, + environment=environment, + ) + ) + read_permission = EnvironmentPermissionModel.objects.get(key=VIEW_ENVIRONMENT) + user_group_environment_permission.permissions.set([read_permission]) + + url = f"/api/v1/environments/{environment.api_key}/user-group-permissions/" + + # When + response = other_org_client.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_delete_user_group_permission__different_organisation__returns_403( + environment: Environment, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + other_org_user = FFAdminUser.objects.create(email="other-org-user@example.com") + other_org_user.add_organisation(organisation_two, role=OrganisationRole.ADMIN) + other_org_client = APIClient() + other_org_client.force_authenticate(other_org_user) + + user_permission_group = UserPermissionGroup.objects.create( + name="Test group", + organisation=organisation, + ) + user_group_environment_permission = ( + UserPermissionGroupEnvironmentPermission.objects.create( + group=user_permission_group, + environment=environment, + ) + ) + + url = ( + f"/api/v1/environments/{environment.api_key}" + f"/user-group-permissions/{user_group_environment_permission.id}/" + ) + + # When + response = other_org_client.delete(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + assert UserPermissionGroupEnvironmentPermission.objects.filter( + id=user_group_environment_permission.id + ).exists() From 8c490e16aa0fed7ddcf040b5682ff2402fefff7d Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Mon, 6 Jul 2026 11:24:24 +0100 Subject: [PATCH 4/4] fix(GitHub integration): Repositories exposed across organisations `GithubRepositoryViewSet.get_queryset` filtered by bare `github_pk`, and `HasPermissionToGithubConfiguration` only checked membership of the URL's `organisation_pk`. Neither tied the GitHub configuration to that organisation, so a member of one organisation could read (and, via the create path, write) repositories under another organisation's configuration. Validate that the configuration in the URL belongs to the organisation in `HasPermissionToGithubConfiguration`, covering every action. A non-numeric `github_pk` is left for the view to reject with a 400. beep boop --- api/integrations/github/permissions.py | 17 +++++++++++++- .../github/test_unit_github_views.py | 23 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/api/integrations/github/permissions.py b/api/integrations/github/permissions.py index ec14952ed275..dfd21201b04e 100644 --- a/api/integrations/github/permissions.py +++ b/api/integrations/github/permissions.py @@ -1,5 +1,7 @@ from rest_framework.permissions import BasePermission +from integrations.github.models import GithubConfiguration + class HasPermissionToGithubConfiguration(BasePermission): """ @@ -10,4 +12,17 @@ class HasPermissionToGithubConfiguration(BasePermission): def has_permission(self, request, view): # type: ignore[no-untyped-def] organisation_id = view.kwargs.get("organisation_pk") - return request.user.belongs_to(organisation_id=int(organisation_id)) + if not request.user.belongs_to(organisation_id=int(organisation_id)): + return False + + # For nested routes, ensure the GitHub configuration in the URL belongs + # to the organisation the caller is authorised for + if (github_pk := view.kwargs.get("github_pk")) is not None and str( + github_pk + ).isdigit(): + return GithubConfiguration.objects.filter( + id=github_pk, + organisation_id=organisation_id, + ).exists() + + return True diff --git a/api/tests/unit/integrations/github/test_unit_github_views.py b/api/tests/unit/integrations/github/test_unit_github_views.py index 6f6cf5ead487..80cf3707ab12 100644 --- a/api/tests/unit/integrations/github/test_unit_github_views.py +++ b/api/tests/unit/integrations/github/test_unit_github_views.py @@ -261,6 +261,29 @@ def test_get_github_repository__github_pk_not_a_number__returns_400( # type: ig assert response.json() == {"github_pk": ["Must be an integer"]} +def test_get_github_repository__configuration_in_other_organisation__returns_403( + admin_client_new: APIClient, + organisation: Organisation, + organisation_two: Organisation, +) -> None: + # Given + configuration = GithubConfiguration.objects.create( + organisation=organisation_two, + installation_id=7654321, + ) + + url = ( + f"/api/v1/organisations/{organisation.id}" + f"/integrations/github/{configuration.id}/repositories/" + ) + + # When + response = admin_client_new.get(url) + + # Then + assert response.status_code == status.HTTP_403_FORBIDDEN + + @responses.activate def test_create_github_repository__valid_data__returns_201( admin_client_new: APIClient,