Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retire: Fix False Positive issue #6021

Merged
merged 1 commit into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading