Fix Qualys parser collapsing findings with same QID but different por…#14528
Fix Qualys parser collapsing findings with same QID but different por…#14528tejas0077 wants to merge 4 commits into
Conversation
|
This will need some consideration/assurance as this will completely break deduplication with existing findings. Posted a comment on #13682 |
|
Hi @valentijnscholten, thank you for the feedback! You raise a valid point. Changing the title format will break A safer approach would be to keep the title unchanged but instead "Qualys Scan": ["title", "severity", "vulnerability_ids", "cwe", "port"] This way:
Would you prefer this approach instead? I can update the PR accordingly. |
Maffooch
left a comment
Maffooch
left a comment
There was a problem hiding this comment.
Qualys is a super popular parser, and breaking deduplication would have far reaching impacts. Port is not a field that could be used for deduplication since port is on the endpoint model rather than the finding model.
Something that could be more palatable is to add the port to the endpoint, and then add multiple endpoints to the finding
|
Thanks @Maffooch, that makes sense! I'll rework the fix to keep a single finding per QID but attach multiple endpoints with the respective ports. That way port-level granularity is preserved without touching titles or breaking deduplication. Will update the PR shortly. |
|
Hi @Maffooch, I've reworked the fix as suggested. Instead of modifying the title, the port is now added directly to the |
|
The title is still being modified, I believe this has to be removed. |
|
Hi @valentijnscholten, thanks for catching that! I've removed the port from the finding title. The title is now back to the original format |
|
Please add some unit tests for this to prevent regressions, add some updates to the release notes, and then I think this should be good! |
tejas0077
force-pushed
the
fix/qualys-port-deduplication
branch
from
1671d06 to
a4e5698
Compare
…n fix - Add test XML with same QID on ports 80, 443, 8080 - Add test verifying each port gets its own endpoint - Add 2.57.x release notes mentioning the fix Addresses review feedback from @Maffooch on PR DefectDojo#14528
github-actions
Bot
added
docker
settings_changes
|
Hi @Maffooch, I have addressed both your review requests: Added unit tests -created a test XML file with the same QID (12345) on three different ports (80, 443, 8080) and added a test that verifies each port gets its own separate endpoint, the finding title remains unchanged as QID-12345 | Test Vulnerability, and all 3 ports are correctly captured. Please take a look! |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
@tejas0077 Can you rebase to get rid of the conflicts? |
tejas0077
force-pushed
the
fix/qualys-port-deduplication
branch
from
a4e5698 to
a998e63
Compare
…n fix - Add test XML with same QID on ports 80, 443, 8080 - Add test verifying each port gets its own endpoint - Add 2.57.x release notes mentioning the fix Addresses review feedback from @Maffooch on PR DefectDojo#14528
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
Hi @valentijnscholten, I've rebased the branch onto the latest bugfix branch. Conflicts are resolved. Please take a look! |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…n fix - Add test XML with same QID on ports 80, 443, 8080 - Add test verifying each port gets its own endpoint - Add 2.57.x release notes mentioning the fix Addresses review feedback from @Maffooch on PR DefectDojo#14528 test(qualys): add unit test for same QID different ports deduplication fix - Add test XML with same QID on ports 80, 443, 8080 - Add test verifying each port gets its own endpoint - Add 2.59.x release notes mentioning the fix Addresses review feedback from @Maffooch on PR DefectDojo#14528
tejas0077
force-pushed
the
fix/qualys-port-deduplication
branch
from
a998e63 to
8104595
Compare
github-actions
Bot
removed
docker
settings_changes
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
3 participants
Description
When importing Qualys scan reports, findings with the same QID but
different ports were being collapsed into a single finding, causing
inaccurate vulnerability counts and loss of port-level granularity.
Root cause: the finding title only used QID and vulnerability name,
so findings with the same QID on different ports (e.g. 80, 5985, 9999)
got deduplicated into one finding.
Fix: port is now added directly to the Endpoint (and LocationData for V3) when present. Each QID+port combination gets its own endpoint on the finding. Finding titles and deduplication are completely unchanged.
Fixes #13682
Test results
Manually traced the parser logic. Port is already extracted from the
XML as temp["port_status"] and is now passed to the Endpoint object when present.
Documentation
No documentation changes needed.
Checklist
Bugfix submitted against the bugfix branch.
Meaningful PR name given.
Proper label added.