Revert "Fix for vulns not included in host/endpoint views after reopening"

[Maffooch]

Reverts #9077

@WojTecH94 apologies for the revert here. I missed the red X that unit tests were failing when approving this PR

[dryrunsecurity]

Contextual Security Analysis

As DryRun Security performs checks, we’ll summarize them here. You can always dive into the detailed results in the section below for checks.

Status	DryRun Security Check
✅	AI-powered Sensitive Function Check
✅	Configured Sensitive Files Check
✅	AI-powered Sensitive Files Check

Chat with your AI-powered Security Buddy by typing @dryrunsecurity followed by your question into a comment.
Example: @dryrunsecurity What are common security issues with web application cookies?

Install and configure more repositories at DryRun Security

[WojTecH94]

So... Do I underastand it correctly that this bug will not be fixed until DD v3 release as no one can edit models.py file for now?
:(

[mtesauro]

@WojTecH94
No, that's not correct. A PR with a failing test was merged, noticed it was failing and reverted.

That has nothing to do with v3 - it's been our policy for years to only merge PRs which pass the CICD tests. This one slipped by with a broken test and was reverted.

In this specific case, there were failing unit tests of the REST framework: https://github.com/DefectDojo/django-DefectDojo/actions/runs/7034140150

[WojTecH94]

Ok, but if you look at these fails it seems they have nothing to do with the change in a PR...

0s
Run docker load -i nginx/nginx-debian_img
open nginx/nginx-debian_img: no such file or directory
Error: Process completed with exit code [1](https://github.com/DefectDojo/django-DefectDojo/actions/runs/7034140150/job/19611366549#step:4:1).

and:

0s
Run docker load -i nginx/nginx-alpine_img
open nginx/nginx-alpine_img: no such file or directory
Error: Process completed with exit code [1](https://github.com/DefectDojo/django-DefectDojo/actions/runs/7034140150/job/19611366549#step:4:1).

So I thought that this is rather connected with the error:
https://github.com/DefectDojo/django-DefectDojo/pull/9077/checks?check_run_id=19141479574

[mtesauro]

About the unrelated failures:

I can't speak for @Maffooch who did the release but I suspect he was concentrating on getting the release out rather then digging into the failure.

About the "Sensitive Files" failure:

We're trying out a new Github app called DryRun Security which lets us mark certain files as "important" and flag PRs that touch those files. Its working pretty good generally speaking.

The idea is to have that tool flag changes to files that might impact our work towards v3. While it's technically failing, we're using it currently to just let the approvers know to look a bit closer at a PR that modifies one of those 'sensitive' files. Things like parsers aren't covered (for obvious reasons). I wrote about it in more detail in this GH discussion: #8974

[Maffooch]

Hi @WojTecH94

I apologize for all of the confusion here! This is on me for approving a PR with failing unit tests. I will work to improve my review process so that this does not happen again. Here is what I did on Monday during the release to make the decision to revert your PR (#9077)

I triggered the release and found that the PR from the release branch to master had failing unit tests. This very seldomly happens because we are pretty good about ensuring the tests are passing in a PR before merging to dev. That PR is #9180. Luckily, there were only a few changes in this bugfix release, and only on PR that touched any application code.

So I went to #9077 and checked the unit tests that were ran on the latest commit. Lucky again, there is only one commit, so it is pretty easy to narrow it down:

I clicked the view details button to determine where the problem is. There are three attempts here. Sometimes the tests are finnicky (like you have made a point of in the comment above) so we often just run them again. Going to the origianl run on the unit tests (attempt #1) I can see that those failed, but I needed to figure out why:

Because of the verbosity of the logs of these tests, github truncates everything past a few thousande lines. These tests dump all the error at the end, so looking to the full log output is necessary here:

Scrolling to the bottom of the output (link to the output for convenience) I can see the following failures:

2023-11-29T14:51:15.8315534Z �[36muwsgi_1         |�[0m ======================================================================
2023-11-29T14:51:15.8316683Z �[36muwsgi_1         |�[0m FAIL: test_status_evaluation (unittests.test_endpoint_model.TestEndpointStatusModel.test_status_evaluation) [Endpoint with vulnerabilities but all of them are mitigated because of different reasons]
2023-11-29T14:51:15.8316972Z �[36muwsgi_1         |�[0m ----------------------------------------------------------------------
2023-11-29T14:51:15.8317194Z �[36muwsgi_1         |�[0m Traceback (most recent call last):
2023-11-29T14:51:15.8317621Z �[36muwsgi_1         |�[0m   File "/app/unittests/test_endpoint_model.py", line 331, in test_status_evaluation
2023-11-29T14:51:15.8317977Z �[36muwsgi_1         |�[0m     self.assertEqual(ep2.active_findings_count, 0, ep2.active_findings)
2023-11-29T14:51:15.8318449Z �[36muwsgi_1         |�[0m AssertionError: 1 != 0 : <bound method Endpoint.active_findings of <Endpoint: http://foo.bar>>
2023-11-29T14:51:15.8318569Z �[36muwsgi_1         |�[0m 
2023-11-29T14:51:15.8318831Z �[36muwsgi_1         |�[0m ======================================================================
2023-11-29T14:51:15.8319511Z �[36muwsgi_1         |�[0m FAIL: test_status_evaluation (unittests.test_endpoint_model.TestEndpointStatusModel.test_status_evaluation) [Host without vulnerabilities]
2023-11-29T14:51:15.8319791Z �[36muwsgi_1         |�[0m ----------------------------------------------------------------------
2023-11-29T14:51:15.8320009Z �[36muwsgi_1         |�[0m Traceback (most recent call last):
2023-11-29T14:51:15.8320421Z �[36muwsgi_1         |�[0m   File "/app/unittests/test_endpoint_model.py", line 340, in test_status_evaluation
2023-11-29T14:51:15.8320822Z �[36muwsgi_1         |�[0m     self.assertEqual(ep1.host_active_findings_count, 0, ep1.host_active_findings)
2023-11-29T14:51:15.8321363Z �[36muwsgi_1         |�[0m AssertionError: 1 != 0 : <bound method Endpoint.host_active_findings of <Endpoint: ftp://foo.bar>>
2023-11-29T14:51:15.8321480Z �[36muwsgi_1         |�[0m 
2023-11-29T14:51:15.8321736Z �[36muwsgi_1         |�[0m ======================================================================
2023-11-29T14:51:15.8322527Z �[36muwsgi_1         |�[0m FAIL: test_status_evaluation (unittests.test_endpoint_model.TestEndpointStatusModel.test_status_evaluation) [Endpoint with one vulnerabilitiy but EPS is mitigated]
2023-11-29T14:51:15.8322795Z �[36muwsgi_1         |�[0m ----------------------------------------------------------------------
2023-11-29T14:51:15.8323010Z �[36muwsgi_1         |�[0m Traceback (most recent call last):
2023-11-29T14:51:15.8323419Z �[36muwsgi_1         |�[0m   File "/app/unittests/test_endpoint_model.py", line 347, in test_status_evaluation
2023-11-29T14:51:15.8323769Z �[36muwsgi_1         |�[0m     self.assertEqual(ep3.active_findings_count, 0, ep3.active_findings)
2023-11-29T14:51:15.8324229Z �[36muwsgi_1         |�[0m AssertionError: 1 != 0 : <bound method Endpoint.active_findings of <Endpoint: http://bar.foo>>
2023-11-29T14:51:15.8324345Z �[36muwsgi_1         |�[0m 
2023-11-29T14:51:15.8324601Z �[36muwsgi_1         |�[0m ======================================================================
2023-11-29T14:51:15.8325325Z �[36muwsgi_1         |�[0m FAIL: test_status_evaluation (unittests.test_endpoint_model.TestEndpointStatusModel.test_status_evaluation) [Host with vulnerabilities]
2023-11-29T14:51:15.8325606Z �[36muwsgi_1         |�[0m ----------------------------------------------------------------------
2023-11-29T14:51:15.8325823Z �[36muwsgi_1         |�[0m Traceback (most recent call last):
2023-11-29T14:51:15.8326237Z �[36muwsgi_1         |�[0m   File "/app/unittests/test_endpoint_model.py", line 370, in test_status_evaluation
2023-11-29T14:51:15.8326636Z �[36muwsgi_1         |�[0m     self.assertEqual(ep3.host_active_findings_count, 1, ep3.host_active_findings)
2023-11-29T14:51:15.8327123Z �[36muwsgi_1         |�[0m AssertionError: 2 != 1 : <bound method Endpoint.host_active_findings of <Endpoint: http://bar.foo>>
2023-11-29T14:51:15.8327239Z �[36muwsgi_1         |�[0m 
2023-11-29T14:51:15.8327696Z �[36muwsgi_1         |�[0m ----------------------------------------------------------------------
2023-11-29T14:51:15.8327907Z �[36muwsgi_1         |�[0m Ran 2611 tests in 395.867s
2023-11-29T14:51:15.8328057Z �[36muwsgi_1         |�[0m 
2023-11-29T14:51:15.8328270Z �[36muwsgi_1         |�[0m FAILED (failures=4, skipped=475)

Because the tests had run to completion, I know that this is not a failure in the execution of the test, but rather a failure of the test itself. In cases where I encounter this situation during development, I will often just fix the failed test. However, since I was doing a release, I did not want this to be a blocker, so I reverted.

I hope all of this makes sense, and provides the answers/reasoning you are looking for. If not, please do not hesitate to reach out :)

[WojTecH94]

Thank you for your broad explanation. I appreciate all the work you do here.
What is the current status of a bug (#8450)? Does it wait for tests to be tweaked or does it require fix that will somehow pass all the tests?

[mtesauro]

@WojTecH94 TBH, the best path forward is probably to have you 're-do' this PR. I'd think that would be easier than trying to re-start with this PR since it's been merged then reverted. Plus, the code-base of DefectDojo moves pretty quickly so you'd likely have to rebase against the dev branch as well.

We'd be happy to have your contribution added to DefectDojo. (and this time we'll be a bit more careful about merging it)