feat(unittest): Not avoid PLW1514

[kiblik]

It might be better to fix based on recommendations - not avoid it.

[dryrunsecurity]

DryRun Security Summary

The pull request focuses on the unit test suite for the AppCheckWebApplicationScannerParser class, which ensures that the parser can accurately and reliably process scan reports from the AppCheck Web Application Scanner tool, a crucial aspect of application security.

Expand for full summary

Summary:

The code changes in this pull request focus on the unit test suite for the AppCheckWebApplicationScannerParser class, which is responsible for parsing scan results from the AppCheck Web Application Scanner tool. The test suite covers a wide range of functionality and edge cases, ensuring that the parser can accurately and reliably process scan reports from the AppCheck tool. This is a crucial aspect of application security, as it ensures that the vulnerability information is correctly extracted and presented to the security team for further analysis and remediation.

The key points from an application security perspective are:

1. The parser is designed to handle a wide range of input data formats, including reports with no findings, single finding, multiple findings, duplicate findings, and reports containing HTTP/2 protocol-specific findings.
2. The parser extracts and populates various fields of the Finding object, such as title, date, severity, mitigation, component information, vulnerability IDs, and detailed descriptions, ensuring that the relevant vulnerability information is made available to the security team.
3. The parser extracts the host, port, protocol, and path information from the scan report and associates it with the corresponding finding, providing context about the affected application components.
4. The parser is able to handle non-printable characters in the scan report data, ensuring that the vulnerability details are properly displayed.
5. The parser is able to extract and parse CVSS vectors from the scan report data, which is important for assessing the risk and prioritizing the findings.
6. The test suite includes checks for parsing findings from different scan engines, demonstrating the flexibility and extensibility of the parser.
7. The parser is able to correctly determine the status of the findings, such as whether they are active, false positives, or have an acceptable risk.

Files Changed:

• unittests/tools/test_appcheck_web_application_scanner_parser.py: This file contains the unit test suite for the AppCheckWebApplicationScannerParser class. The test suite covers a wide range of functionality and edge cases, ensuring that the parser can accurately and reliably process scan reports from the AppCheck tool.

Code Analysis

We ran 9 analyzers against 1 file and 0 analyzers had findings. 9 analyzers had no findings.

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

[Maffooch]

I elected to avoid the encoding due the testing of removal of the \00 char a few lines into the test. I wasn't sure if the attempting to encode that null char would break something, or if we wanted to the encode that the file at all

@dogboat what do you think would be the best approach here?

[dogboat]

I think this should be fine. Encodings are super weird but I tested out reading the file explicitly as utf-8 locally and it loads the chars as expected; unsaved requests get the null byte as expected, and it's stripped out/replaced where it should be. We could add something like self.assertTrue("\x00" in finding.unsaved_response) to each of the checks if we want to make sure that stays preserved, dunno why I didn't do that previously.

[kiblik]

I think this should be fine. Encodings are super weird but I tested out reading the file explicitly as utf-8 locally and it loads the chars as expected; unsaved requests get the null byte as expected, and it's stripped out/replaced where it should be. We could add something like self.assertTrue("\x00" in finding.unsaved_response) to each of the checks if we want to make sure that stays preserved, dunno why I didn't do that previously.

I would prefer self.assertIn("\x00", finding.unsaved_response) (I tried to offer it in #10817 as well). But why not?

Will you prepare it or should I? As part of this PR or will you open new one?

[Maffooch]

If there AssertIn change has been made already as part of #10817, then I thing this one is good to go

[mtesauro]

Approved