Skip to content

Commit

Permalink
retire: Fix False Positive issue
Browse files Browse the repository at this point in the history
- CHANGELOG > Added fix note.
- Repo > Added handling for fall through of essentially "empty" object.
- Tests > Added additional unit tests/assertions.

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
  • Loading branch information
kingthorin committed Dec 19, 2024
1 parent 393ac34 commit c4f7200
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 7 deletions.
3 changes: 3 additions & 0 deletions addOns/retire/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ All notable changes to this add-on will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased
### Fixed
- An issue that was resulting in False Positives.

### Changed
- Update minimum ZAP version to 2.16.0.
- The scan rule now uses a more specific CWE (Issue 8732).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ private static VulnerabilityData isVersionVulnerable(
vulnData.setRisk(vnext.getRisk());
}
}
return vulnData;
return vulnData.getRisk() > 0 ? vulnData : VulnerabilityData.EMPTY;
}

private static boolean isGoodCandidate(String version) {
Expand All @@ -307,6 +307,11 @@ private static boolean isGoodCandidate(String version) {
return isAllZeros != 0; // Not a good value if all zero
}

/** For testing purposes only */
Map<String, RepoEntry> getEntries() {
return entries;
}

public static class VulnerabilityData {
public static final VulnerabilityData EMPTY = new VulnerabilityData();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
@JsonIgnoreProperties({"above", "cwe"})
public class Vulnerability {

private static final Map<String, Integer> SEVERITY_MAP =
/** Increased visibility for testing purposes only */
static final Map<String, Integer> SEVERITY_MAP =
Map.of("high", Alert.RISK_HIGH, "medium", Alert.RISK_MEDIUM, "low", Alert.RISK_LOW);

private String below;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertSame;
Expand Down Expand Up @@ -183,6 +184,24 @@ void shouldRaiseAlertOnVulnerableContent() {
assertEquals(4, alertsRaised.get(0).getTags().size());
}

@Test
void shouldNotRaiseAlertOnNonVulnerableContent() {
// Given
String content =
"/*!\n"
+ " * Bootstrap v3.4.0 (http://getbootstrap.com)\n"
+ " * Copyright 2011-2016 Twitter, Inc.\n"
+ " * Licensed under the MIT license\n"
+ " */";
HttpMessage msg = createMessage("http://example.com/angular.min.js", content);
msg.getResponseHeader().setHeader(HttpFieldsNames.CONTENT_TYPE, "text/javascript");
given(passiveScanData.isPage200(any())).willReturn(true);
// When
scanHttpResponseReceive(msg);
// Then
assertThat(alertsRaised, hasSize(0));
}

@Test
void shouldRaiseAlertOnHashOfVulnerableContent() {
// Given
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ void shouldReadLibWithVulnerabilities() throws IOException {
assertThat(identifiers.getCve(), contains("cve 1", "cve 2"));
assertThat(identifiers.getSummary(), is("summary"));
assertThat(vuln.getInfo(), contains("info 1", "info 2"));
assertThat(vuln.getSeverity(), is("severity"));
assertThat(vuln.getSeverity(), is("high"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,16 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.zaproxy.addon.retire;
package org.zaproxy.addon.retire.model;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.in;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;

import java.util.Locale;
import java.util.Set;
import org.junit.jupiter.api.Test;
import org.zaproxy.addon.retire.model.Repo;

/** Tests for upstream {@code jsrepository.json} file. */
class UpstreamJsRepositoryUnitTest {
Expand All @@ -31,7 +35,13 @@ class UpstreamJsRepositoryUnitTest {
void shouldParseUpstreamJsRepository() {
// Given
String path = "/org/zaproxy/addon/retire/resources/jsrepository.json";
Set<String> expectedSeverities = Vulnerability.SEVERITY_MAP.keySet();
// When / Then
assertDoesNotThrow(() -> new Repo(path));
Repo repo = assertDoesNotThrow(() -> new Repo(path));
for (RepoEntry entry : repo.getEntries().values()) {
for (Vulnerability vuln : entry.getVulnerabilities()) {
assertThat(vuln.getSeverity().toLowerCase(Locale.ROOT), is(in(expectedSeverities)));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"vulnerabilities" : [ {
"atOrAbove" : "atorabove",
"below" : "below",
"severity" : "severity",
"severity" : "high",
"cwe" : [ "cwe 1", "cwe 2" ],
"identifiers" : {
"CVE" : [ "cve 1", "cve 2" ],
Expand Down

0 comments on commit c4f7200

Please sign in to comment.