ascanrulesBeta: Add Example Alerts to InsecureHttpMethodScanRule#6871
ascanrulesBeta: Add Example Alerts to InsecureHttpMethodScanRule#6871Brandosp wants to merge 1 commit intozaproxy:mainfrom
Conversation
|
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
Use @Checkmarx to interact with Checkmarx PR Assistant. |
kingthorin
left a comment
There was a problem hiding this comment.
Getting really close (from my point of view).
...sBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/InsecureHttpMethodScanRule.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/zaproxy/zap/extension/ascanrulesBeta/InsecureHttpMethodScanRuleUnitTest.java
Outdated
Show resolved
Hide resolved
| void shouldHaveExpectedExampleAlert() { | ||
| List<Alert> alerts = rule.getExampleAlerts(); | ||
|
|
||
| assertThat(alerts.size(), is(equalTo(5))); |
There was a problem hiding this comment.
To be consistent with elsewhere in the file/repo this should probably be:
void shouldHaveExpectedExampleAlert() {
// Given / When
List<Alert> alerts = rule.getExampleAlerts();
// Then
assertThat(alerts.size(), is(equalTo(5)));There was a problem hiding this comment.
Also the other assertions are not needed, the default tests already check the references.
...sBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/InsecureHttpMethodScanRule.java
Outdated
Show resolved
Hide resolved
| + "Server: gws\n" | ||
| + "Connection: close") | ||
| .build(), | ||
| buildHttpMethodAlert( |
There was a problem hiding this comment.
This still needs to be extracted to its own method, it's also missing the webdav which IMO should have its own ref (i.e the one in line 569 should be replaced with two build/raise calls in lines 545 and 569, having all the data already filled in).
There was a problem hiding this comment.
Sorry, not exactly sure what you mean by this. I began working on the code based off this comment, and I added to the buildHttpMethodAlert method to check whether or not the httpMethod is part of WEBDAV or not to give webdav its own alert ref
String alertRef = getId() + "-5"; if (WEBDAV_METHODS.contains(httpMethod)) { alertRef = getId() + "-6"; }
You mentioned adding two build/raise calls in line 545 and 569 which I assume this means I should create a buildAlert for specifically WEBDAV but I found this confusing as the main logic filters the data for whether the httpmethod is part of WEBDAV or not making it so the build call on 569 would create distinct alerts, so I imagine my assumption is wrong?
Additionally, when u mention extracting to its own method, do u mean creating a buildExampleAlert helper which would just use the code I have in the getExampleAlerts for the HttpMethodAlert, and then simply calling that helper function to make the code more structured?
There was a problem hiding this comment.
There are two issues:
- The
getExampleAlerts()should not be duplicating what the main code is doing, both should call a common method. If tomorrow we change the description/other info of the lines 553 and 559, this example alert will keep returning the old/different description/other info. We should not need to manually keep main code and examples in sync. Also, note this issue is already happening withbuildInsecureHttpMethodAlert, the example alert has the wrong description/other info. - The webdav case of line 545 is effectively a different alert, note the use of different description and why (IMO) it should have its own ref.
|
Note that you need to rebase the branch to adjust the changelog. |
0df702c to
cb224ce
Compare
Signed-off-by: Brandosp <brandosp05@gmail.com>


Overview
Added example alerts for InsecureHttpMethodScanRule.
Related Issues
Checklist
for more details, please refer to the developer rules and guidelines