fix: delete suggested reference_controls/threats for requirement nodes AND add annotation support for threats#4064
Conversation
…s AND add annotation support for threats
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightened type hints for a helper; LibraryUpdater now normalizes, deduplicates, resolves (cache → single DB refetch) and replaces ManyToMany threat/reference_control links, detaches old ReferenceControl rows via bulk_update; ThreatImporter now persists an optional ChangesHelper type annotations
Library ReferenceControl detach + bulk-update cohort
RequirementNode M2M replace semantics
Threat importer: annotation persistence
Sequence Diagram(s)sequenceDiagram
participant Updater as "RequirementNode Updater\n(routine)"
participant Cache as "objects_tracked\n(in-memory cache)"
participant DB as "Database\n(Threat/ReferenceControl models)"
Updater->>Updater: receive requirement_node with URN lists
Updater->>Updater: normalize & dedupe URNs (lowercase)
Updater->>Cache: attempt resolve URNs
Cache-->>Updater: return resolved objects + missing URNs
alt missing URNs exist
Updater->>DB: query objects where urn__in=missing_set
DB-->>Updater: return resolved objects
end
Updater->>Updater: assemble final object sets
Updater->>DB: call .set(new_threats) and .set(new_reference_controls)
DB-->>Updater: persist exact M2M relationships
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
monsieurswag
requested review from
Mohamed-Hacene,
ab-smith,
eric-intuitem and
nas-tabchiche
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/core/models.py`:
- Around line 1270-1296: The update currently uses a fallback query
(Threat.objects.filter / ReferenceControl.objects.filter) that may return a
subset and then calls requirement_node_object.threats.set(new_threats) or
requirement_node_object.reference_controls.set(new_reference_controls), which
leads to destructive partial updates when some URNs are unresolved; change the
logic so after building new_threats_urns and new_reference_controls_urns you
verify that every URN maps to a resolved object (compare lengths or check for
None in new_threats/new_reference_controls), and if any are missing raise/return
a validation error (abort the library update) instead of falling back to a
partial queryset — only call requirement_node_object.threats.set(...) and
requirement_node_object.reference_controls.set(...) when all requested URNs are
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b062228-703b-4443-831e-833a5a792f04
📒 Files selected for processing (3)
backend/core/helpers.pybackend/core/models.pybackend/library/utils.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/core/models.py (1)
1270-1299:⚠️ Potential issue | 🟠 MajorAbort the update when a suggested URN does not resolve.
At Line 1275 and Line 1288, the fallback
filter(urn__in=...)can return only a subset of the requested objects. The subsequent.set(...)then silently drops previously suggested threats/reference controls for the missing URNs, so a malformed library payload becomes a destructive partial update instead of an invalid update. Please verify that every incoming URN resolved before calling.set(...)and raise a validation error on any miss.Suggested guard
new_threats_urns = { urn.lower() # URNs are case-insensitive for urn in requirement_node.get("threats", []) } new_threats = [objects_tracked.get(urn) for urn in new_threats_urns] if any(threat is None for threat in new_threats): - # Edge case fix in case we ever have a threat missing from objects_tracked - new_threats = Threat.objects.filter(urn__in=new_threats_urns) + new_threats = list(Threat.objects.filter(urn__in=new_threats_urns)) + missing_threat_urns = new_threats_urns - { + threat.urn.lower() for threat in new_threats + } + if missing_threat_urns: + raise ValidationError( + _("Unknown threat URNs in library update: %(urns)s"), + params={"urns": ", ".join(missing_threat_urns)}, + ) requirement_node_object.threats.set(new_threats) new_reference_controls_urns = { urn.lower() # URNs are case-insensitive for urn in requirement_node.get("reference_controls", []) } new_reference_controls = [ objects_tracked.get(urn) for urn in new_reference_controls_urns ] if any( reference_control is None for reference_control in new_reference_controls ): - # Edge case fix in case we ever have a reference_control missing from objects_tracked - new_reference_controls = ReferenceControl.objects.filter( - urn__in=new_reference_controls_urns - ) + new_reference_controls = list( + ReferenceControl.objects.filter( + urn__in=new_reference_controls_urns + ) + ) + missing_reference_control_urns = ( + new_reference_controls_urns + - { + reference_control.urn.lower() + for reference_control in new_reference_controls + } + ) + if missing_reference_control_urns: + raise ValidationError( + _( + "Unknown reference control URNs in library update: %(urns)s" + ), + params={ + "urns": ", ".join(missing_reference_control_urns) + }, + ) requirement_node_object.reference_controls.set( new_reference_controls )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/core/models.py` around lines 1270 - 1299, The current fallback that replaces missing lookups with Threat.objects.filter(...) and ReferenceControl.objects.filter(...) can silently drop unresolved URNs; update the logic around requirement_node_object.threats.set and requirement_node_object.reference_controls.set to first compare the set of requested URNs (new_threats_urns / new_reference_controls_urns) against the URNs actually resolved from objects_tracked and from the DB queries, and if any requested URN is missing raise a validation error (e.g., Django ValidationError) instead of calling .set(...); keep using objects_tracked as the primary source, only use the DB to attempt resolution and then abort with an explicit error listing unresolved URNs when resolution is incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/core/models.py`:
- Around line 1270-1299: The current fallback that replaces missing lookups with
Threat.objects.filter(...) and ReferenceControl.objects.filter(...) can silently
drop unresolved URNs; update the logic around
requirement_node_object.threats.set and
requirement_node_object.reference_controls.set to first compare the set of
requested URNs (new_threats_urns / new_reference_controls_urns) against the URNs
actually resolved from objects_tracked and from the DB queries, and if any
requested URN is missing raise a validation error (e.g., Django ValidationError)
instead of calling .set(...); keep using objects_tracked as the primary source,
only use the DB to attempt resolution and then abort with an explicit error
listing unresolved URNs when resolution is incomplete.
nas-tabchiche
changed the title
nas-tabchiche
left a comment
nas-tabchiche
left a comment
There was a problem hiding this comment.
Confirmed both fixes work as expected.
One more thing before we can merge: some regression testing in app_tests/api/test_api_libraries.py would be insanely cheap to write, and valuable.
There was a problem hiding this comment.
Adding/removing Threats & Ref. Controls from a requirement node works fine!
However, the fix for Annotation support for Threats doesn't work on my end.
Furthermore, if a Threat/Ref. Control is completely removed from a library, it remains visible in the Threats/Ref. Controls catalog. Will this bug be fixed in another PR? Because this is also an issue that has been reported in the Jira ticket.
…rom its original library
The client complains about the reference control still being displayed as "suggested controls" in its audit. The >>> # AFTER LOADING THE LIBRARY
>>>
>>> for i in loaded_library.threats.all():
... print(i.name, "::::::", i.annotation)
...
Extra Window Memory InjectionXXXXXXXXX :::::: AAA
Socket Filters :::::: BBB
Scheduled Task :::::: None
>>>
>>> # AFTER UPDATING THE LIBRARY
>>>
>>> for i in loaded_library.threats.all():
... print(i.name, "::::::", i.annotation)
...
Extra Window Memory InjectionXXXXXXXXX :::::: AAA
Socket Filters :::::: BBB
Scheduled Task :::::: CCC |
|
A new behavior was introduced by this PR. This can be seen in the following output: >>> # AFTER LOADING THE LIBRARY
>>>
>>> for i in LoadedLibrary.objects.latest("created_at").reference_controls.all(): print(i.ref_id, i.library)
...
ASF-REC-01 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-02 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-03 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-07 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-08 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-09 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
>>> # AFTER UPDATING THE LIBRARY
>>>
>>> for i in R.objects.filter(ref_id__icontains="ASF-REC").all(): print(i.ref_id, i.library)
...
ASF-REC-01 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-02 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-03 ASF-Baseline - UPDATED XXXXXXXXXXXXXX AAAAAAAAAAAAAAA Agile Security Framework - Baseline
ASF-REC-07 None
ASF-REC-08 None
ASF-REC-09 None |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/core/models.py`:
- Around line 831-833: old_reference_controls_urn_set is built from
self.old_library.reference_controls but isn't normalized, causing case-mismatch
false detachments; when creating old_reference_controls_urn_set (and the
analogous set around the 851-856 block) convert each URN to a canonical form
(e.g., .lower().strip() or the same normalization used for the incoming set) by
normalizing values returned by
self.old_library.reference_controls.all().values_list("urn", flat=True) before
putting them into the set so comparisons use the same normalized form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
tarkadia
left a comment
tarkadia
left a comment
There was a problem hiding this comment.
Adding/removing Threats & Ref. Controls from a requirement node still works fine!
The "new behavior" for ReferenceControl works fine too. I'd like to get someon else's opinion whether it's a good approach or if it might create a side effect.
Following our discussion, we won't remove any Threats/Ref. Controls from DB if they are removed from the library file.
We also noticed that the convert_library_v2.py script doesn't set the annotation field for the main locale of the library, but does for extra locales. This will be fixed in another PR.
|
In my opinion, removing reference controls from a library should have the same behavior than unloading a library with reference controls. We should prevent the update if some of the ref controls are used, and delete them if they are not, if the user don't want them anymore in the library, they surely don't want them in the db neither. |
Apparently Eric said we shouldn't delete them. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
That's right. Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
|
Newly added tests (at commit hash: |
4 participants
This PR fixes 2 different bugs which are closely-enough related to be merged into a single PR.
(Details about these bugs can be found in jira
CA-1661)Bug 1:
The first bug is that the
["objects"]["threats"][idx]["annotation"]field path is ignored by the library importer even though it shouldn't (as theThreatmodel does contain anannotationfield, and there's no reason to prevent library creators from modifying it).Bug 2:
The second bug is that library updates can't delete
ReferenceControl/Threatobjects suggested toRequirementNode.For example if a new version of a library remove a reference control URN from a
["objects"]["requirement_nodes"][idx]["reference_controls"]field path, the library update will NOT clear it out (and the user will have outdated/obsolete suggestions).The
["objects"]["requirement_nodes"][idx]["threats"]has the exact same problem.Summary by CodeRabbit
New Features
Improvements
Refactor