ascanrules: Address External Redirect False Positives #6677

kingthorin · 2025-08-20T15:27:28Z

Overview

The External Redirect scan rules has been updated to account for potential false positives involving JavaScript comments.

psiinon · 2025-08-20T16:08:20Z

Checkmarx One – Scan Summary & Details – 2532e07b-50c5-43c2-aff1-ddae52a20685

Great job! No new security vulnerabilities introduced in this pull request

Communicate with Checkmarx by submitting a PR comment with @Checkmarx followed by one of the supported commands. Learn about the supported commands here.

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

kingthorin · 2025-08-26T21:13:40Z

Deconflicted

thc202 · 2025-09-02T06:28:46Z

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

@@ -531,6 +531,68 @@ void shouldReportRedirectWithJsLocationMethods(String jsMethod) throws Exception
        assertThat(alertsRaised.get(0).getEvidence().startsWith(HttpHeader.HTTP), equalTo(true));
    }

+    private static Stream<Arguments> provideCommentStrings() {


This should have malformed escapes and comments.

Better run the test coverage, no escapes are actually being tested (the escape char needs to be escaped).

All comments should lead to an alert if it wasn't for the fix, if you revert the scan rule changes and run the test only two cases fail, they all should fail.

For your first bullet can you be more specific? Or provide some examples?

e.g. \u A, \x B, /* not closed ….

Got all that, I think.

Missing cases for \u{HHH} (1 to 6 digits long) and octal, unicode and hex broken half way through, from looking at the latest push.

The hex escape is just two chars (the other ones are not really needed).

There should also be a case for single quote strings and template literals (containing comment chars to make sure those are properly accepted).

\xHH not \xHHHH the latter is just one hex escape followed by two chars (case w/ malformed (mid) hex escape is incorrect, to be middle it would be \xH H).

Thanks, tweaked.

The parsing logic keeps changing everyday which is fine but that means the tests should be updated to cover the new cases, there are still conditions not covered, which should to make sure there are no bugs.

It was changing often to cover cases you kept thinking of, which is great because I wasn't seeing/catching them, and I thought I was extending the test inputs to cover things but maybe not sufficiently.

Just added a few for nested comments 🤷‍♂️

Found, added, and updated code for more edge cases.

Copilot

Pull Request Overview

The External Redirect scan rule has been updated to reduce false positives by excluding JavaScript code that appears within comments from triggering redirect vulnerability alerts.

Added JavaScript comment parsing logic to filter out commented code before pattern matching
Enhanced test coverage with comprehensive comment scenarios including various escape sequences and edge cases
Updated changelog to document the false positive improvement

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
ExternalRedirectScanRule.java	Added JavaScript comment extraction logic and state machine parser to filter out commented code
ExternalRedirectScanRuleUnitTest.java	Added comprehensive test cases covering various JavaScript comment scenarios and edge cases
CHANGELOG.md	Documented the false positive fix for JavaScript comments

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java

thc202 · 2025-09-04T08:04:06Z

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java

-        Matcher matcher = pattern.matcher(value);
+        // We know the payload is present.
+        // Ensure the value has something we're likely interested in before dealing with comments
+        if (JS_PRE_CHECKS.stream().noneMatch(chk -> StringUtils.containsIgnoreCase(value, chk))) {


I'd suggest keeping the previous check (even if that means matching twice), instead of adding additional checks which must be kept in sync with the patterns.

You mean the domain check? Then drop the simple JS checks?

The pattern check. Yes, the JS_PRE_CHECKS.

So do matcher.find both before and after?

Worth doing the StringUtils.startsWithIgnoreCase as well, better extract a method.

Done & done

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java

kingthorin · 2025-09-04T11:25:34Z

Got all those I believe

thc202 · 2025-09-09T08:07:56Z

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

+            Set<String> comments =
+                    ExternalRedirectScanRule.extractJsComments(BODY_TEMPLATE.formatted(content));
+            // Then
+            assertThat(comments, hasSize(1));


The numbers are not enough, it should be checked the content as well otherwise we don't know if the boundaries are properly detected.

I've adjusted this. Indeed some of the results are perhaps not as we'd wish. I've added comments to the situations I feel are debatable .

Signed-off-by: kingthorin <[email protected]>

kingthorin · 2025-09-10T13:56:43Z

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

+                            // What do we really want in this case?
+                            "console.log(\"example\"); /* console.log('comment'); ",
+                            "/* console.log('comment'); \n</script></head><body><H1>Redirect</H1></body></html>"),


For this rule I think we only deal with JS inside HTML, so capturing to the closing script tag is probably reasonable.

In the future we could have a boolean that indicates if the end of script tag is used, or some other behavior to pure JS files.

kingthorin · 2025-09-10T13:57:22Z

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

+                    Arguments.of(
+                            "Template expression with inner comment",
+                            // Is this the behavior we want? I believe this should either be both or
+                            // just trailing. The current result seems wrong to me.
+                            "console.log(`outer ${ /* inner comment */ 42 }`); // trailing comment",
+                            "/* inner comment */"),


I propose that this should simply capture the trailing comment.

kingthorin · 2025-09-10T13:57:52Z

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

+                    // This results in a zero comments situation. I believe that's likely fine as
+                    // the unterminated literal should be an app failure on it's own
+                    //                    Arguments.of(
+                    //                            "Unterminated template literal",
+                    //                            "console.log(`unterminated template ${1+1} //
+                    // comment not terminated", "?"),


kingthorin force-pushed the redir-wavsep-fps branch from a3c7e33 to a252c5b Compare August 20, 2025 15:40

kingthorin force-pushed the redir-wavsep-fps branch 3 times, most recently from 10ed03c to 6b120f2 Compare August 23, 2025 15:41

kingthorin commented Aug 24, 2025

View reviewed changes

...les/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java Outdated Show resolved Hide resolved

kingthorin marked this pull request as ready for review August 24, 2025 22:21

kingthorin requested a review from Copilot August 25, 2025 02:32

This comment was marked as outdated.

Sign in to view

kingthorin force-pushed the redir-wavsep-fps branch 6 times, most recently from 3c91fd7 to 436c2b7 Compare August 26, 2025 21:13

thc202 reviewed Sep 2, 2025

View reviewed changes

kingthorin force-pushed the redir-wavsep-fps branch 5 times, most recently from 0d6d8cb to 6c9a24c Compare September 2, 2025 15:39

kingthorin requested a review from Copilot September 3, 2025 18:05

Copilot AI reviewed Sep 3, 2025

View reviewed changes

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java Outdated Show resolved Hide resolved

.../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java Outdated Show resolved Hide resolved

kingthorin force-pushed the redir-wavsep-fps branch from 6c9a24c to ff39efb Compare September 3, 2025 18:10

thc202 reviewed Sep 4, 2025

View reviewed changes

kingthorin force-pushed the redir-wavsep-fps branch from ff39efb to 40fd2ef Compare September 4, 2025 11:24

kingthorin force-pushed the redir-wavsep-fps branch from 40fd2ef to 89de190 Compare September 4, 2025 11:31

kingthorin force-pushed the redir-wavsep-fps branch 4 times, most recently from 9b9d595 to d6c8475 Compare September 4, 2025 15:42

thc202 reviewed Sep 9, 2025

View reviewed changes

kingthorin force-pushed the redir-wavsep-fps branch from d6c8475 to 125facc Compare September 9, 2025 10:44

ascanrules: Address External Redirect False Positives

525d355

Signed-off-by: kingthorin <[email protected]>

kingthorin force-pushed the redir-wavsep-fps branch from 125facc to 525d355 Compare September 9, 2025 11:24

kingthorin commented Sep 10, 2025

View reviewed changes

Uh oh!

ascanrules: Address External Redirect False Positives #6677

Are you sure you want to change the base?

ascanrules: Address External Redirect False Positives #6677

Uh oh!

Conversation

kingthorin commented Aug 20, 2025

Overview

Uh oh!

psiinon commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

kingthorin commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thc202 Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thc202 Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingthorin Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kingthorin commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psiinon commented Aug 20, 2025 •

edited

Loading

thc202 Sep 2, 2025 •

edited

Loading

thc202 Sep 4, 2025 •

edited

Loading

kingthorin Sep 4, 2025 •

edited

Loading