diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index 6afba19fbc..aea997700e 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Address potential false positives with the XSLT Injection scan rule when payloads cause a failure which may still contain the expected evidence. - Depends on an updated version of the Common Library add-on. - Reduced usage of error level logging. +- The External Redirect scan rule has been updated to account for potential false positives involving JavaScript comments. ## [74] - 2025-09-18 ### Added diff --git a/addOns/ascanrules/ascanrules.gradle.kts b/addOns/ascanrules/ascanrules.gradle.kts index c3f08b7218..4cc335cdaf 100644 --- a/addOns/ascanrules/ascanrules.gradle.kts +++ b/addOns/ascanrules/ascanrules.gradle.kts @@ -52,6 +52,7 @@ dependencies { zapAddOn("oast") implementation(libs.ascanrules.procyonCompilerTools) + implementation(libs.ascanrules.rhino) testImplementation(parent!!.childProjects.get("commonlib")!!.sourceSets.test.get().output) testImplementation(project(":testutils")) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java index c192a96665..c4e46e76a9 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java @@ -23,8 +23,10 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -36,6 +38,10 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.mozilla.javascript.CompilerEnvirons; +import org.mozilla.javascript.EvaluatorException; +import org.mozilla.javascript.Parser; +import org.mozilla.javascript.ast.AstRoot; import org.parosproxy.paros.Constant; import org.parosproxy.paros.core.scanner.AbstractAppParamPlugin; import org.parosproxy.paros.core.scanner.Alert; @@ -47,6 +53,7 @@ import org.zaproxy.addon.commonlib.http.HttpFieldsNames; import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities; import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability; +import org.zaproxy.zap.utils.Stats; /** * Reviewed scan rule for External Redirect @@ -478,10 +485,43 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) { private static boolean isRedirectPresent(Pattern pattern, String value) { Matcher matcher = pattern.matcher(value); + if (!isPresent(matcher)) { + return false; + } + Set extractedComments = extractJsComments(value); + String valueWithoutComments = value; + for (String comment : extractedComments) { + valueWithoutComments = valueWithoutComments.replace(comment, ""); + } + + return isPresent(pattern.matcher(valueWithoutComments)); + } + + private static boolean isPresent(Matcher matcher) { return matcher.find() && StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP); } + /** Visibility increased for unit testing purposes only */ + protected static Set extractJsComments(String jsSource) { + Set comments = new HashSet<>(); + try { + CompilerEnvirons env = new CompilerEnvirons(); + env.setRecordingComments(true); + Parser parser = new Parser(env, env.getErrorReporter()); + // Rhino drops a character when the snippet ends with a single line comment so add a + // newline + AstRoot ast = parser.parse(jsSource + "\n", null, 1); + if (ast.getComments() != null) { + ast.getComments().forEach(comment -> comments.add(comment.getValue())); + } + } catch (EvaluatorException ee) { + Stats.incCounter("stats.ascan.rule." + PLUGIN_ID + ".jsparse.fail"); + LOGGER.debug(ee.getMessage()); + } + return comments; + } + @Override public int getRisk() { return Alert.RISK_HIGH; diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java index bdcde85163..7ced41e54b 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java @@ -21,6 +21,7 @@ import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -28,7 +29,9 @@ import fi.iki.elonen.NanoHTTPD.Response; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Stream; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -527,6 +530,48 @@ void shouldReportRedirectWithJsLocationMethods(String jsMethod) throws Exception assertThat(alertsRaised.get(0).getEvidence().startsWith(HttpHeader.HTTP), equalTo(true)); } + @Test + void shouldNotReportRedirectIfInsideJsComment() throws Exception { + // Given + String test = "/"; + String body = + """ + + + + Redirect commented out + + + + + """ + .formatted(CONTENT_TOKEN); + nano.addHandler( + new NanoServerHandler(test) { + @Override + protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) { + String site = getFirstParamValue(session, "site"); + if (site != null && !site.isEmpty()) { + String withPayload = body.replace(CONTENT_TOKEN, site); + return newFixedLengthResponse( + NanoHTTPD.Response.Status.OK, NanoHTTPD.MIME_HTML, withPayload); + } + return newFixedLengthResponse(""); + } + }); + HttpMessage msg = getHttpMessage(test + "?site=xxx"); + rule.init(msg, parent); + // When + rule.scan(); + // Then + assertThat(alertsRaised, is(empty())); + } + private static Stream createJsMethodBooleanPairs() { return Stream.of( Arguments.of("location.reload", true), @@ -556,7 +601,12 @@ void shouldNotReportRedirectWithJsLocationMethodsWhenConcatenated( } @ParameterizedTest - @ValueSource(strings = {"window.open", "window.navigate"}) + @ValueSource( + strings = { + "window.open", + "window.navigate", + "let r = /http:\\/\\/[a-z]+/g; window.navigate" + }) void shouldReportRedirectWithJsWindowMethods(String jsMethod) throws Exception { // Given String test = "/"; @@ -631,4 +681,158 @@ void shouldFindLocationUrl(String input) { // Then assertThat(extracted, is(equalTo("http://www.example.com/"))); } + + /** Unit tests for {@link ExternalRedirectScanRule#extractJsComments(String)}. */ + @Nested + class ExtractJsCommentsUnitTest { + + private static Stream commentProvider() { + return Stream.of( + Arguments.of("Empty line comment", "//", Set.of("//")), + Arguments.of("Empty block comment", "/**/", Set.of("/**/")), + Arguments.of("Block comment", "/* comment \n*/", Set.of("/* comment \n*/")), + Arguments.of( + "Line comment with CRLF", + "console.log('x'); // comment\r\nconsole.log('y');", + Set.of("// comment")), + Arguments.of( + "Block comment containing line terminator + line comment", + "/* block start\n// inside block */ console.log('x');", + Set.of("/* block start\n// inside block */")), + Arguments.of( + "Escaped quote before comment", + "console.log('it\\'s fine'); // real comment", + Set.of("// real comment")), + Arguments.of( + "Escaped backslash before comment", + "console.log('c:\\\\'); // comment", + Set.of("// comment")), + Arguments.of("Single line", "// comment ", Set.of("// comment ")), + Arguments.of( + "Block inside Single line", + "// /* comment; */", + Set.of("// /* comment; */")), + Arguments.of( + "Single line inside Block comment", + "/* comment \n // example */", + Set.of("/* comment \n // example */")), + Arguments.of( + "Inline block", + "console.log(\"example\"); /* console.log('comment'); */", + Set.of("/* console.log('comment'); */")), + Arguments.of( + "Inline single line", + "console.log(\"example\"); // console.log('comment'));", + Set.of("// console.log('comment'));")), + Arguments.of( + "Inline single line (w/ unicode escape)", + "console.log(\"🔥 example\"); // console.log('\u1F525 example');", + Set.of("// console.log('\u1F525 example');")), + Arguments.of( + "Template literal with embedded expression", + "console.log(`value ${1 + 1}`); // comment;", + Set.of("// comment;")), + Arguments.of( + "Template expression with block comment", + "console.log(`value ${ /* block comment */ 42 }`);", + Set.of("/* block comment */")), + Arguments.of( + "Multiline nested template expression", + "console.log(`line1 ${ `inner ${42} // not comment` }`); // real comment", + Set.of("// real comment")), + Arguments.of( + "Nested template with string containing comment-like text", + "console.log(`outer ${ 'string // not comment' }`); // real comment", + Set.of("// real comment")), + Arguments.of( + "Regex literal followed by comment", + "var re = /abc/; // trailing comment", + Set.of("// trailing comment")), + Arguments.of( + "Regex literal containing /* ... */ in class", + "var re = /a\\/\\*b/; // trailing comment", + Set.of("// trailing comment")), + Arguments.of( + "Regex-like in comment", + "/* /http:\\/\\/evil.com/ */", + Set.of("/* /http:\\/\\/evil.com/ */"))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("commentProvider") + void shouldFindExpectedComments(String name, String input, Set expectedComments) { + // Given / When + Set actualComments = ExternalRedirectScanRule.extractJsComments(input); + // Then + assertThat( + String.format( + "Test '%s' failed. Expected %s but got %s", + name, expectedComments, actualComments), + actualComments, + is(expectedComments)); + } + + private static Stream sequentialCommentsProvider() { + return Stream.of( + Arguments.of( + "Single line comment sequence", + "// first\n//second\nconsole.log('x');", + Set.of("// first", "//second")), + Arguments.of( + "Single line and block comment sequence", + "// first\n/*second*/\nconsole.log('x');", + Set.of("// first", "/*second*/")), + Arguments.of( + "Template expression with inner comment", + "console.log(`outer ${ /* inner comment */ 42 }`); // trailing comment", + Set.of("/* inner comment */", "// trailing comment")), + Arguments.of( + "Block comment sequence", + "/* first*/\n/*second*/\nconsole.log('x');", + Set.of("/* first*/", "/*second*/"))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("sequentialCommentsProvider") + void shouldIdentifyMultipleComments( + String name, String input, Set expectedComments) { + // Given / When + Set actualComments = ExternalRedirectScanRule.extractJsComments(input); + // Then + assertThat( + "Unexpected comment set for test: " + name, + actualComments, + equalTo(expectedComments)); + } + + private static Stream nonCommentStringsProvider() { + return Stream.of( + Arguments.of("String containing //", "console.log('not // a comment');"), + Arguments.of( + "String containing /* */", + "console.log('not /* a comment */ either');"), + Arguments.of( + "Unterminated string before comment", + "console.log('unterminated // not a comment"), + Arguments.of("regex literal", "let r = /http:\\/\\/example.com/;"), + Arguments.of( + "regex with comment-like content", "let r = /\\/\\* comment *\\/g;"), + // Unterminated template literal results in JS error + Arguments.of( + "Unterminated template literal", + "console.log(`unterminated template ${1+1} // comment not terminated"), + Arguments.of( + "Inline incomplete block", + "console.log(\"example\"); /* console.log('comment');")); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("nonCommentStringsProvider") + void shouldNotFindAComment(String name, String input) { + // Given / When + Set comments = ExternalRedirectScanRule.extractJsComments(input); + // Then + assertThat(comments, is(empty())); + } + } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index bb3bd814e6..41894ca318 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -16,6 +16,7 @@ slf4j = "2.0.17" [libraries] ascanrules-procyonCompilerTools = "org.bitbucket.mstrobel:procyon-compilertools:0.6.0" +ascanrules-rhino = "org.mozilla:rhino:1.8.0" ascanrulesBeta-diffutils = { module = "com.googlecode.java-diff-utils:diffutils", version.ref = "diffutils" } ascanrulesBeta-jsoup = { module = "org.jsoup:jsoup", version.ref = "jsoup" } authhelper-otpJava = "com.github.bastiaanjansen:otp-java:2.1.0"