fix(environments): validate project field in environment creation#7063
fix(environments): validate project field in environment creation#7063nielskaspers wants to merge 2 commits intoFlagsmith:mainfrom
Conversation
…s/ (Flagsmith#6599) Validate that the `project` field is a valid integer before querying the database. Previously, passing a non-integer string (e.g. '<Project ID>') 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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@nielskaspers is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7063 +/- ##
==========================================
- Coverage 98.33% 98.26% -0.08%
==========================================
Files 1337 1337
Lines 50010 49899 -111
==========================================
- Hits 49178 49032 -146
- Misses 832 867 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Small NIT comments on the tests that could be addressed while we are working on this
| staff_user: FFAdminUser, | ||
| ) -> None: | ||
| # Given | ||
| mock_view.action = "create" |
There was a problem hiding this comment.
NIT for the 2 tests:
Can we instantiate a new mock for each test to avoid shared state?
view = mock.MagicMock()
request = mock.MagicMock()
view.action = "create"
etc...
| assert result is False | ||
|
|
||
|
|
||
| def test_environment_permissions__create_with_none_project__returns_false( |
There was a problem hiding this comment.
Maybe we could parametrize those 2 tests actually?
While we are at it, I could see a separate test case for string-encoded integer ("42") that should pass
- 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
2 participants
Summary
projectfield is a valid integer before querying the database inEnvironmentPermissions.has_permission<Project ID>) caused an unhandledValueErroratProject.objects.get(), resulting in a 500 responseIssue
Fixes #6599
Changes
api/environments/permissions/permissions.py: Addedint()validation with(TypeError, ValueError)handling before the ORM query, following the same pattern used inFeatureSegmentPermissionsQimport