Skip to content

Commit 4decd88

Browse files
Jino-TMaffoochclaudepaulOsinski
authored
Validate consistency between ID-based and name-based identifiers in import/reimport (#14636)
* Fix reimport-scan API authorization bypass via conflicting identifiers Validate that ID-resolved objects (test, engagement) are consistent with name-based identifiers (product_name, engagement_name) in both the permission check layer and the AutoCreateContextManager resolution layer. This prevents an attacker from passing their own engagement/test ID to satisfy the permission check while using name-based fields to target a victim's product. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Use ID-based comparisons and add engagement_name check to import - Switch permission checks to use ID comparisons (product_id, engagement_id) where resolved objects are available, with name fallback for unresolved cases - Add engagement_name validation to UserHasImportPermission (was missing) - Fix ruff string quoting in auto_create_context.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Strip undeclared engagement field in reimport permission check The engagement field is not declared on ReImportScanSerializer and gets stripped during validation. The permission check must also strip it so it resolves targets the same way execution does — by name, not by a stale engagement ID from request.data. Update test to verify the engagement param is ignored and permission is checked against the name-resolved target. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix information disclosure in conflict validation error messages Replace error messages that leaked resolved object names (product names, engagement names) with generic messages. An attacker could enumerate object names by sending conflicting ID-based and name-based identifiers and reading the detailed error responses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Paul Osinski <42211303+paulOsinski@users.noreply.github.com>
1 parent 4a0abbd commit 4decd88

File tree

3 files changed

+125
-3
lines changed

3 files changed

+125
-3
lines changed

‎dojo/api_v2/permissions.py‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,13 @@ def has_permission(self, request, view):
473473
# Raise an explicit drf exception here
474474
raise ValidationError(e)
475475
if engagement := converted_dict.get("engagement"):
476-
# existing engagement, nothing special to check
476+
# Validate the resolved engagement's parent chain matches any provided identifiers
477+
if (product := converted_dict.get("product")) and engagement.product_id != product.id:
478+
msg = "The provided identifiers are inconsistent — the engagement does not belong to the specified product."
479+
raise ValidationError(msg)
480+
if (engagement_name := converted_dict.get("engagement_name")) and engagement.name != engagement_name:
481+
msg = "The provided identifiers are inconsistent — the engagement name does not match the specified engagement."
482+
raise ValidationError(msg)
477483
return user_has_permission(
478484
request.user, engagement, Permissions.Import_Scan_Result,
479485
)
@@ -764,6 +770,11 @@ def has_permission(self, request, view):
764770
try:
765771
converted_dict = auto_create.convert_querydict_to_dict(request.data)
766772
auto_create.process_import_meta_data_from_dict(converted_dict)
773+
# engagement is not a declared field on ReImportScanSerializer and will be
774+
# stripped during validation — don't use it in the permission check either,
775+
# so the permission check resolves targets the same way execution does
776+
converted_dict.pop("engagement", None)
777+
converted_dict.pop("engagement_id", None)
767778
# Get an existing product
768779
converted_dict["product_type"] = auto_create.get_target_product_type_if_exists(**converted_dict)
769780
converted_dict["product"] = auto_create.get_target_product_if_exists(**converted_dict)
@@ -774,7 +785,20 @@ def has_permission(self, request, view):
774785
raise ValidationError(e)
775786

776787
if test := converted_dict.get("test"):
777-
# existing test, nothing special to check
788+
# Validate the resolved test's parent chain matches any provided identifiers
789+
if (product := converted_dict.get("product")) and test.engagement.product_id != product.id:
790+
msg = "The provided identifiers are inconsistent — the test does not belong to the specified product."
791+
raise ValidationError(msg)
792+
if (engagement := converted_dict.get("engagement")) and test.engagement_id != engagement.id:
793+
msg = "The provided identifiers are inconsistent — the test does not belong to the specified engagement."
794+
raise ValidationError(msg)
795+
# Also validate by name when the objects were not resolved (e.g. names that match no existing record)
796+
if not converted_dict.get("product") and (product_name := converted_dict.get("product_name")) and test.engagement.product.name != product_name:
797+
msg = "The provided identifiers are inconsistent — the test does not belong to the specified product."
798+
raise ValidationError(msg)
799+
if not converted_dict.get("engagement") and (engagement_name := converted_dict.get("engagement_name")) and test.engagement.name != engagement_name:
800+
msg = "The provided identifiers are inconsistent — the test does not belong to the specified engagement."
801+
raise ValidationError(msg)
778802
return user_has_permission(
779803
request.user, test, Permissions.Import_Scan_Result,
780804
)
@@ -1181,7 +1205,10 @@ def check_auto_create_permission(
11811205
raise ValidationError(msg)
11821206

11831207
if engagement:
1184-
# existing engagement, nothing special to check
1208+
# Validate the resolved engagement's parent chain matches any provided names
1209+
if product is not None and engagement.product_id != product.id:
1210+
msg = "The provided identifiers are inconsistent — the engagement does not belong to the specified product."
1211+
raise ValidationError(msg)
11851212
return user_has_permission(
11861213
user, engagement, Permissions.Import_Scan_Result,
11871214
)

‎dojo/importers/auto_create_context.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,9 @@ def get_target_engagement_if_exists(
181181
"""
182182
if engagement := get_object_or_none(Engagement, pk=engagement_id):
183183
logger.debug("Using existing engagement by id: %s", engagement_id)
184+
if product is not None and engagement.product_id != product.id:
185+
msg = "The provided identifiers are inconsistent — the engagement does not belong to the specified product."
186+
raise ValueError(msg)
184187
return engagement
185188
# if there's no product, then for sure there's no engagement either
186189
if product is None:
@@ -203,6 +206,9 @@ def get_target_test_if_exists(
203206
"""
204207
if test := get_object_or_none(Test, pk=test_id):
205208
logger.debug("Using existing Test by id: %s", test_id)
209+
if engagement is not None and test.engagement_id != engagement.id:
210+
msg = "The provided identifiers are inconsistent — the test does not belong to the specified engagement."
211+
raise ValueError(msg)
206212
return test
207213
# If the engagement is not supplied, we cannot do anything
208214
if not engagement:

‎unittests/test_rest_framework.py‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3429,6 +3429,95 @@ def test_create_not_authorized_product_name_engagement_name_scan_type_title(self
34293429
importer_mock.assert_not_called()
34303430
reimporter_mock.assert_not_called()
34313431

3432+
# Security tests: verify that conflicting ID-based and name-based identifiers are rejected
3433+
3434+
@patch("dojo.importers.default_reimporter.DefaultReImporter.process_scan")
3435+
@patch("dojo.importers.default_importer.DefaultImporter.process_scan")
3436+
@patch("dojo.api_v2.permissions.user_has_permission")
3437+
def test_reimport_engagement_param_ignored_permission_checked_on_name_resolved_target(self, mock, importer_mock, reimporter_mock):
3438+
"""
3439+
Engagement is not a declared field on ReImportScanSerializer — verify
3440+
the permission check uses the name-resolved target, not the engagement param.
3441+
"""
3442+
mock.return_value = False
3443+
importer_mock.return_value = IMPORTER_MOCK_RETURN_VALUE
3444+
reimporter_mock.return_value = REIMPORTER_MOCK_RETURN_VALUE
3445+
3446+
with Path("tests/zap_sample.xml").open(encoding="utf-8") as testfile:
3447+
payload = {
3448+
"minimum_severity": "Low",
3449+
"active": True,
3450+
"verified": True,
3451+
"scan_type": "ZAP Scan",
3452+
"file": testfile,
3453+
# engagement=1 belongs to Product 2 Engagement 1, but it should be ignored
3454+
"engagement": 1,
3455+
# These names resolve to Product 2's Engagement 4 -> Test 4
3456+
"product_name": "Security How-to",
3457+
"engagement_name": "April monthly engagement",
3458+
"version": "1.0.0",
3459+
}
3460+
response = self.client.post(self.url, payload)
3461+
self.assertEqual(403, response.status_code, response.content[:1000])
3462+
# Permission must be checked on name-resolved Test 4 (in Engagement 4),
3463+
# NOT on Test 3 (which belongs to the engagement=1 param)
3464+
mock.assert_called_with(User.objects.get(username="admin"),
3465+
Test.objects.get(id=4),
3466+
Permissions.Import_Scan_Result)
3467+
importer_mock.assert_not_called()
3468+
reimporter_mock.assert_not_called()
3469+
3470+
@patch("dojo.importers.default_reimporter.DefaultReImporter.process_scan")
3471+
@patch("dojo.importers.default_importer.DefaultImporter.process_scan")
3472+
def test_reimport_with_test_id_mismatched_product_name_is_rejected(self, importer_mock, reimporter_mock):
3473+
"""Sending test ID from one product with product_name from another must be rejected."""
3474+
importer_mock.return_value = IMPORTER_MOCK_RETURN_VALUE
3475+
reimporter_mock.return_value = REIMPORTER_MOCK_RETURN_VALUE
3476+
3477+
with Path("tests/zap_sample.xml").open(encoding="utf-8") as testfile:
3478+
payload = {
3479+
"minimum_severity": "Low",
3480+
"active": True,
3481+
"verified": True,
3482+
"scan_type": "ZAP Scan",
3483+
"file": testfile,
3484+
# Test 3 belongs to Engagement 1 -> Product 2 ("Security How-to")
3485+
"test": 3,
3486+
# But product_name points to Product 1 ("Python How-to")
3487+
"product_name": "Python How-to",
3488+
"version": "1.0.0",
3489+
}
3490+
response = self.client.post(self.url, payload)
3491+
self.assertEqual(400, response.status_code, response.content[:1000])
3492+
importer_mock.assert_not_called()
3493+
reimporter_mock.assert_not_called()
3494+
3495+
@patch("dojo.importers.default_reimporter.DefaultReImporter.process_scan")
3496+
@patch("dojo.importers.default_importer.DefaultImporter.process_scan")
3497+
def test_reimport_with_test_id_mismatched_engagement_name_is_rejected(self, importer_mock, reimporter_mock):
3498+
"""Sending test ID from one engagement with engagement_name from another must be rejected."""
3499+
importer_mock.return_value = IMPORTER_MOCK_RETURN_VALUE
3500+
reimporter_mock.return_value = REIMPORTER_MOCK_RETURN_VALUE
3501+
3502+
with Path("tests/zap_sample.xml").open(encoding="utf-8") as testfile:
3503+
payload = {
3504+
"minimum_severity": "Low",
3505+
"active": True,
3506+
"verified": True,
3507+
"scan_type": "ZAP Scan",
3508+
"file": testfile,
3509+
# Test 3 belongs to Engagement 1 ("1st Quarter Engagement")
3510+
"test": 3,
3511+
# But engagement_name points to a different engagement
3512+
"product_name": "Security How-to",
3513+
"engagement_name": "April monthly engagement",
3514+
"version": "1.0.0",
3515+
}
3516+
response = self.client.post(self.url, payload)
3517+
self.assertEqual(400, response.status_code, response.content[:1000])
3518+
importer_mock.assert_not_called()
3519+
reimporter_mock.assert_not_called()
3520+
34323521

34333522
@versioned_fixtures
34343523
class ProductTypeTest(BaseClass.BaseClassTest):

0 commit comments

Comments
 (0)