ascanrules: Address External Redirect False Positives

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

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"))

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<String> 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<String> extractJsComments(String jsSource) {
+        Set<String> 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;

addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java

@@ -21,14 +21,17 @@
 
 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;
 
 import fi.iki.elonen.NanoHTTPD;
 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 =
+                """
+                <!DOCTYPE html>
+                <html>
+                <head>
+                <title>Redirect commented out</title>
+                </head>
+                <body>
+
+                <script>function myRedirectFunction()
+                {/*
+                window.location.replace('%s');
+                */}
+                //myRedirectFunction();
+                </script>
+                """
+                        .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("<html><body></body></html>");
+                    }
+                });
+        HttpMessage msg = getHttpMessage(test + "?site=xxx");
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(alertsRaised, is(empty()));
+    }
+
     private static Stream<Arguments> 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<Arguments> 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<String> expectedComments) {
+            // Given /  When
+            Set<String> 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<Arguments> 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<String> expectedComments) {
+            // Given / When
+            Set<String> actualComments = ExternalRedirectScanRule.extractJsComments(input);
+            // Then
+            assertThat(
+                    "Unexpected comment set for test: " + name,
+                    actualComments,
+                    equalTo(expectedComments));
+        }
+
+        private static Stream<Arguments> 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<String> comments = ExternalRedirectScanRule.extractJsComments(input);
+            // Then
+            assertThat(comments, is(empty()));
+        }
+    }
 }

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"