Skip to content

Hotfix: BHE LargeDefaultGroup#882

Merged
chrismaddalena merged 7 commits intomasterfrom
hotfix/fix-bhe-largedefaultgroup
Apr 24, 2026
Merged

Hotfix: BHE LargeDefaultGroup#882
chrismaddalena merged 7 commits intomasterfrom
hotfix/fix-bhe-largedefaultgroup

Conversation

@chrismaddalena
Copy link
Copy Markdown
Collaborator

This PR fixes an issue with LargeDefaultGroup findings that could cause the severity to calculate incorrectly. It also adjusts the logic around determining if a finding is related to tier zero. I cleaned up the logic and added a is_tier_zero flag to each source/target pairing for easy tier zero identification.

Copilot AI review requested due to automatic review settings April 24, 2026 22:36
@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Apr 24, 2026

🤖 Augment PR Summary

Summary: Hotfixes BloodHound Enterprise findings parsing to prevent incorrect severity calculation for LargeDefaultGroups and to improve tier-zero detection.

Changes:

  • Normalize impact/exposure percentage inputs to floats (invalid/missing values treated as 0) before applying severity thresholds.
  • Continue using exposure-based severity for relationship findings, but keep LargeDefaultGroups on the impact-based path.
  • Update tier-zero matching to support dynamic tier-zero tag IDs returned from /asset-group-tags.
  • Add an is_tier_zero flag to grouped findings and propagate it across multiple principals for the same finding/environment.
  • Expand the test suite to cover LargeDefaultGroups severity behavior, tag-id tier-zero asset selection, and invalid percentage inputs.

Technical Notes: _get_tier_zero_group() now returns the tier-zero tag object (when available) instead of only the tag name.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 3 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread ghostwriter/shepherd/tests/test_bloodhound_client.py
Comment thread ghostwriter/shepherd/external/bloodhound/client.py Outdated
Comment thread ghostwriter/shepherd/tests/test_bloodhound_client.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the BloodHound Enterprise findings parsing to fix severity calculation for LargeDefaultGroups relationship findings and improve tier-zero detection by supporting dynamic tier-zero tag IDs.

Changes:

  • Normalize impact_percentage/exposure_percentage inputs to prevent type errors and incorrect severity calculations (notably for LargeDefaultGroups and None exposure values).
  • Update tier-zero detection to use asset_group_tag_id when the tier management engine is enabled, and add an aggregated is_tier_zero flag on grouped findings.
  • Expand/adjust unit tests to cover the new severity normalization and tier-zero tag ID behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
ghostwriter/shepherd/external/bloodhound/client.py Fixes severity calculation robustness and updates tier-zero matching logic (tag ID + fallback), adding an is_tier_zero marker.
ghostwriter/shepherd/tests/test_bloodhound_client.py Adds coverage for invalid percentage inputs and tier-zero tag ID behavior; refactors request mocking.
Comments suppressed due to low confidence (1)

ghostwriter/shepherd/external/bloodhound/client.py:466

  • The PR description mentions adding an is_tier_zero flag to each source/target pairing, but the flag is only added at the grouped finding level. If consumers need tier-zero identification per principal, consider adding is_tier_zero to the dict returned by _build_target_entry() (or otherwise associating it with each entry in principals).
                grouped[unique_key] = {"finding_name": finding_name, **grouped[unique_key]}
                grouped[unique_key]["is_tier_zero"] = is_tier_zero_finding

                # Calculate severity and build target entry
                severity = _calculate_severity(finding)
                grouped[unique_key]["principals"] = [_build_target_entry(finding, severity)]

Comment thread ghostwriter/shepherd/tests/test_bloodhound_client.py
Comment thread ghostwriter/shepherd/tests/test_bloodhound_client.py Outdated
Comment thread ghostwriter/shepherd/tests/test_bloodhound_client.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.20%. Comparing base (58b4833) to head (48b6a94).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
ghostwriter/shepherd/external/bloodhound/client.py 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
+ Coverage   93.19%   93.20%   +0.01%     
==========================================
  Files         393      393              
  Lines       24937    24987      +50     
==========================================
+ Hits        23239    23290      +51     
+ Misses       1698     1697       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chrismaddalena chrismaddalena merged commit aa7f504 into master Apr 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants