Skip to content

Commit 75bf783

Browse files
committed
fix: Correctly skip already-revoked and already-deprecated objects
Fixed critical bug in refactored load_data() method where already-revoked and already-deprecated objects were being processed as version changes instead of being skipped. **The Bug:** - When `_detect_revocation()` returned None for already-revoked objects, they fell through to version change processing - Similarly, already-deprecated objects were not being skipped **The Fix:** - Added explicit skip logic for already-revoked objects (when revocation_result is None and object is revoked) - Added explicit skip logic for already-deprecated objects (when deprecation check returns False but object is deprecated) - Updated `_detect_revocation()` to return None|True|False to distinguish: - None: not a revocation scenario - True: newly revoked and validated - False: validation error, skip object **Impact:** This fix reduces the count from 518→377 to the correct 377 in minor_version_changes. All 133 tests now pass (was 130/133). Matches original behavior where already-revoked/deprecated objects exit the if-elif-else chain without being processed as version changes.
1 parent 32c9e7f commit 75bf783

1 file changed

Lines changed: 55 additions & 42 deletions

File tree

mitreattack/diffStix/changelog_helper.py

Lines changed: 55 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
"main",
8181
]
8282

83+
8384
@dataclass
8485
class DomainStatistics:
8586
"""Statistics for a single ATT&CK domain."""
@@ -328,9 +329,7 @@ def release_contributors(self, value: dict):
328329
"""
329330
self._contributor_tracker.release_contributors = value
330331

331-
def _detect_revocation(
332-
self, stix_id: str, old_obj: dict, new_obj: dict, new_attack_objects: dict, domain: str
333-
) -> bool:
332+
def _detect_revocation(self, stix_id: str, old_obj: dict, new_obj: dict, new_attack_objects: dict, domain: str):
334333
"""Detect if an object has been newly revoked.
335334
336335
Parameters
@@ -348,30 +347,33 @@ def _detect_revocation(
348347
349348
Returns
350349
-------
351-
bool
352-
True if the object was newly revoked, False otherwise.
350+
None, True, or False
351+
None if not a revocation scenario (not revoked or already revoked),
352+
True if newly revoked and successfully validated,
353+
False if validation failed (object should be skipped).
353354
"""
355+
# Not revoked at all - continue to deprecation/version checking
354356
if not new_obj.get("revoked"):
355-
return False
357+
return None
356358

357-
# Only work with newly revoked objects
359+
# Already revoked in old version - not a change, but still revoked
360+
# Original code would exit the if block and NOT process as version change
358361
if old_obj.get("revoked"):
359-
return False
362+
return None
360363

364+
# Newly revoked - validate the revocation
361365
if stix_id not in self.data["new"][domain]["relationships"]["revoked-by"]:
362366
logger.error(f"[{stix_id}] revoked object has no revoked-by relationship")
363-
return False
367+
return False # Validation error - skip this object
364368

365369
revoked_by_key = self.data["new"][domain]["relationships"]["revoked-by"][stix_id][0]["target_ref"]
366370
if revoked_by_key not in new_attack_objects:
367-
logger.error(
368-
f"{stix_id} revoked by {revoked_by_key}, but {revoked_by_key} not found in new STIX bundle!!"
369-
)
370-
return False
371+
logger.error(f"{stix_id} revoked by {revoked_by_key}, but {revoked_by_key} not found in new STIX bundle!!")
372+
return False # Validation error - skip this object
371373

372374
revoking_object = new_attack_objects[revoked_by_key]
373375
new_obj["revoked_by"] = revoking_object
374-
return True
376+
return True # Successfully detected new revocation
375377

376378
def _detect_deprecation(self, old_obj: dict, new_obj: dict) -> bool:
377379
"""Detect if an object has been newly deprecated.
@@ -510,35 +512,50 @@ def load_data(self):
510512
detailed_diff = ddiff.to_json()
511513
new_stix_obj["detailed_diff"] = detailed_diff
512514

513-
# Check for revocations
514-
if self._detect_revocation(stix_id, old_stix_obj, new_stix_obj, new_attack_objects, domain):
515+
# Check for revocations (skip object if revocation validation fails or already revoked)
516+
revocation_result = self._detect_revocation(
517+
stix_id, old_stix_obj, new_stix_obj, new_attack_objects, domain
518+
)
519+
if revocation_result is False:
520+
# Revocation validation failed - skip this object entirely (like original 'continue')
521+
continue
522+
elif revocation_result is True:
515523
revocations.add(stix_id)
524+
continue
525+
elif revocation_result is None and new_stix_obj.get("revoked"):
526+
# Object is revoked but was already revoked - skip (matches original if-elif-else behavior)
527+
continue
528+
516529
# Check for deprecations
517-
elif self._detect_deprecation(old_stix_obj, new_stix_obj):
530+
if self._detect_deprecation(old_stix_obj, new_stix_obj):
518531
deprecations.add(stix_id)
532+
continue
533+
elif new_stix_obj.get("x_mitre_deprecated"):
534+
# Object is deprecated but was already deprecated - skip (matches original if-elif-else behavior)
535+
continue
536+
519537
# Process normal version changes
520-
else:
521-
category, old_version, new_version = self._categorize_version_change(
522-
stix_id, old_stix_obj, new_stix_obj
523-
)
538+
category, old_version, new_version = self._categorize_version_change(
539+
stix_id, old_stix_obj, new_stix_obj
540+
)
524541

525-
if category == "major":
526-
major_version_changes.add(stix_id)
527-
elif category == "minor":
528-
minor_version_changes.add(stix_id)
529-
elif category == "other":
530-
other_version_changes.add(stix_id)
531-
elif category == "patch":
532-
patches.add(stix_id)
533-
else:
534-
unchanged.add(stix_id)
542+
if category == "major":
543+
major_version_changes.add(stix_id)
544+
elif category == "minor":
545+
minor_version_changes.add(stix_id)
546+
elif category == "other":
547+
other_version_changes.add(stix_id)
548+
elif category == "patch":
549+
patches.add(stix_id)
550+
else:
551+
unchanged.add(stix_id)
535552

536-
if new_version != old_version:
537-
new_stix_obj["version_change"] = f"{old_version}{new_version}"
553+
if new_version != old_version:
554+
new_stix_obj["version_change"] = f"{old_version}{new_version}"
538555

539-
# Process description and relationship changes
540-
self._process_description_changes(old_stix_obj, new_stix_obj)
541-
self._process_relationship_changes(new_stix_obj, domain)
556+
# Process description and relationship changes
557+
self._process_description_changes(old_stix_obj, new_stix_obj)
558+
self._process_relationship_changes(new_stix_obj, domain)
542559

543560
#############
544561
# New objects
@@ -653,9 +670,7 @@ def _collect_related_objects(
653670

654671
return related_objects
655672

656-
def _create_changelog_entry(
657-
self, old_items: dict, new_items: dict, formatter: callable = None
658-
) -> dict:
673+
def _create_changelog_entry(self, old_items: dict, new_items: dict, formatter: callable = None) -> dict:
659674
"""Create a changelog entry with shared, new, and dropped items.
660675
661676
Parameters
@@ -703,9 +718,7 @@ def find_technique_mitigation_changes(self, new_stix_obj: dict, domain: str):
703718

704719
new_stix_obj["changelog_mitigations"] = self._create_changelog_entry(old_mitigations, new_mitigations)
705720

706-
def _collect_detection_objects(
707-
self, stix_id: str, domain: str, age: str
708-
) -> tuple[dict[str, str], dict[str, str]]:
721+
def _collect_detection_objects(self, stix_id: str, domain: str, age: str) -> tuple[dict[str, str], dict[str, str]]:
709722
"""Collect detection-related objects (datacomponents and detectionstrategies) for a technique.
710723
711724
Parameters

0 commit comments

Comments
 (0)