From 7a6d047a46ad08f92b7654af6e3f91e78deed36b Mon Sep 17 00:00:00 2001 From: Niels Kaspers Date: Mon, 30 Mar 2026 09:06:03 +0300 Subject: [PATCH 1/2] fix(environments): validate project field in POST /api/v1/environments/ (#6599) Validate that the `project` field is a valid integer before querying the database. Previously, passing a non-integer string (e.g. '') caused an unhandled ValueError resulting in a 500 response. Now returns 403 (permission denied) for invalid values, consistent with how missing or non-existent projects are handled. Follows the same validation pattern used in FeatureSegmentPermissions. --- api/environments/permissions/permissions.py | 11 ++++--- .../test_unit_environments_permissions.py | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/api/environments/permissions/permissions.py b/api/environments/permissions/permissions.py index 374adfbc5113..a6e9c4b19438 100644 --- a/api/environments/permissions/permissions.py +++ b/api/environments/permissions/permissions.py @@ -6,7 +6,7 @@ from common.projects.permissions import ( CREATE_ENVIRONMENT, ) -from django.db.models import Model, Q +from django.db.models import Model from rest_framework import exceptions from rest_framework.permissions import BasePermission, IsAuthenticated @@ -36,9 +36,12 @@ def has_permission(self, request, view): # type: ignore[no-untyped-def] if view.action == "create": try: - project_id = request.data.get("project") - project_lookup = Q(id=project_id) - project = Project.objects.get(project_lookup) + project_id = int(request.data.get("project")) + except (TypeError, ValueError): + return False + + try: + project = Project.objects.get(id=project_id) return request.user.has_project_permission(CREATE_ENVIRONMENT, project) except Project.DoesNotExist: return False diff --git a/api/tests/unit/environments/permissions/test_unit_environments_permissions.py b/api/tests/unit/environments/permissions/test_unit_environments_permissions.py index efde99f62b43..db1ee0939f73 100644 --- a/api/tests/unit/environments/permissions/test_unit_environments_permissions.py +++ b/api/tests/unit/environments/permissions/test_unit_environments_permissions.py @@ -148,6 +148,38 @@ def test_environment_permissions__user_without_create_permission__returns_false( assert result is False +def test_environment_permissions__create_with_non_integer_project__returns_false( + staff_user: FFAdminUser, +) -> None: + # Given + mock_view.action = "create" + mock_view.detail = False + mock_request.user = staff_user + mock_request.data = {"project": "", "name": "Test environment"} + + # When + result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call] + + # Then + assert result is False + + +def test_environment_permissions__create_with_none_project__returns_false( + staff_user: FFAdminUser, +) -> None: + # Given + mock_view.action = "create" + mock_view.detail = False + mock_request.user = staff_user + mock_request.data = {"name": "Test environment"} + + # When + result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call] + + # Then + assert result is False + + def test_environment_permissions__list_action__returns_true( staff_user: FFAdminUser, ) -> None: From 6300dfcc2a0e99051fc6cc191d61b7280b2eb43c Mon Sep 17 00:00:00 2001 From: Niels Kaspers Date: Thu, 2 Apr 2026 17:11:04 +0300 Subject: [PATCH 2/2] refactor(tests): address review feedback on environment permission tests - Parametrize invalid project tests to avoid shared module-level mocks - Use fresh mock instances per test to prevent shared state - Add test for string-encoded integer project ID ("42") that should pass --- .../test_unit_environments_permissions.py | 50 +++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/api/tests/unit/environments/permissions/test_unit_environments_permissions.py b/api/tests/unit/environments/permissions/test_unit_environments_permissions.py index db1ee0939f73..ea9403a984a9 100644 --- a/api/tests/unit/environments/permissions/test_unit_environments_permissions.py +++ b/api/tests/unit/environments/permissions/test_unit_environments_permissions.py @@ -1,5 +1,6 @@ from unittest import mock +import pytest from common.projects.permissions import ( CREATE_ENVIRONMENT, ) @@ -148,36 +149,57 @@ def test_environment_permissions__user_without_create_permission__returns_false( assert result is False -def test_environment_permissions__create_with_non_integer_project__returns_false( +@pytest.mark.parametrize( + "request_data, expected", + [ + ({"project": "", "name": "Test environment"}, False), + ({"name": "Test environment"}, False), + ], +) +def test_environment_permissions__create_with_invalid_project__returns_false( staff_user: FFAdminUser, + request_data: dict, + expected: bool, ) -> None: # Given - mock_view.action = "create" - mock_view.detail = False - mock_request.user = staff_user - mock_request.data = {"project": "", "name": "Test environment"} + view = mock.MagicMock() + request = mock.MagicMock() + view.action = "create" + view.detail = False + request.user = staff_user + request.data = request_data # When - result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call] + result = environment_permissions.has_permission(request, view) # type: ignore[no-untyped-call] # Then - assert result is False + assert result is expected -def test_environment_permissions__create_with_none_project__returns_false( +def test_environment_permissions__create_with_string_integer_project__returns_true( staff_user: FFAdminUser, + project: Project, ) -> None: # Given - mock_view.action = "create" - mock_view.detail = False - mock_request.user = staff_user - mock_request.data = {"name": "Test environment"} + create_environment_permission = ProjectPermissionModel.objects.get( + key=CREATE_ENVIRONMENT + ) + user_project_permission = UserProjectPermission.objects.create( + user=staff_user, project=project + ) + user_project_permission.permissions.set([create_environment_permission]) + view = mock.MagicMock() + request = mock.MagicMock() + view.action = "create" + view.detail = False + request.user = staff_user + request.data = {"project": str(project.id), "name": "Test environment"} # When - result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call] + result = environment_permissions.has_permission(request, view) # type: ignore[no-untyped-call] # Then - assert result is False + assert result is True def test_environment_permissions__list_action__returns_true(