Skip to content

Commit ab4c945

Browse files
psiinonpsiinon
authored
Merge pull request #6677 from kingthorin/redir-wavsep-fps
ascanrules: Address External Redirect False Positives
2 parents d0c4b2c + 0562383 commit ab4c945

File tree

5 files changed

+248
-1
lines changed

5 files changed

+248
-1
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1414
- Address potential false positives with the XSLT Injection scan rule when payloads cause a failure which may still contain the expected evidence.
1515
- Depends on an updated version of the Common Library add-on.
1616
- Reduced usage of error level logging.
17+
- The External Redirect scan rule has been updated to account for potential false positives involving JavaScript comments.
1718

1819
## [74] - 2025-09-18
1920
### Added

‎addOns/ascanrules/ascanrules.gradle.kts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ dependencies {
5252
zapAddOn("oast")
5353

5454
implementation(libs.ascanrules.procyonCompilerTools)
55+
implementation(libs.ascanrules.rhino)
5556

5657
testImplementation(parent!!.childProjects.get("commonlib")!!.sourceSets.test.get().output)
5758
testImplementation(project(":testutils"))

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@
2323
import java.util.ArrayList;
2424
import java.util.Collections;
2525
import java.util.HashMap;
26+
import java.util.HashSet;
2627
import java.util.List;
2728
import java.util.Map;
29+
import java.util.Set;
2830
import java.util.UUID;
2931
import java.util.regex.Matcher;
3032
import java.util.regex.Pattern;
@@ -36,6 +38,10 @@
3638
import org.apache.commons.lang3.StringUtils;
3739
import org.apache.logging.log4j.LogManager;
3840
import org.apache.logging.log4j.Logger;
41+
import org.mozilla.javascript.CompilerEnvirons;
42+
import org.mozilla.javascript.EvaluatorException;
43+
import org.mozilla.javascript.Parser;
44+
import org.mozilla.javascript.ast.AstRoot;
3945
import org.parosproxy.paros.Constant;
4046
import org.parosproxy.paros.core.scanner.AbstractAppParamPlugin;
4147
import org.parosproxy.paros.core.scanner.Alert;
@@ -47,6 +53,7 @@
4753
import org.zaproxy.addon.commonlib.http.HttpFieldsNames;
4854
import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities;
4955
import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability;
56+
import org.zaproxy.zap.utils.Stats;
5057

5158
/**
5259
* Reviewed scan rule for External Redirect
@@ -478,10 +485,43 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
478485

479486
private static boolean isRedirectPresent(Pattern pattern, String value) {
480487
Matcher matcher = pattern.matcher(value);
488+
if (!isPresent(matcher)) {
489+
return false;
490+
}
491+
Set<String> extractedComments = extractJsComments(value);
492+
String valueWithoutComments = value;
493+
for (String comment : extractedComments) {
494+
valueWithoutComments = valueWithoutComments.replace(comment, "");
495+
}
496+
497+
return isPresent(pattern.matcher(valueWithoutComments));
498+
}
499+
500+
private static boolean isPresent(Matcher matcher) {
481501
return matcher.find()
482502
&& StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
483503
}
484504

505+
/** Visibility increased for unit testing purposes only */
506+
protected static Set<String> extractJsComments(String jsSource) {
507+
Set<String> comments = new HashSet<>();
508+
try {
509+
CompilerEnvirons env = new CompilerEnvirons();
510+
env.setRecordingComments(true);
511+
Parser parser = new Parser(env, env.getErrorReporter());
512+
// Rhino drops a character when the snippet ends with a single line comment so add a
513+
// newline
514+
AstRoot ast = parser.parse(jsSource + "\n", null, 1);
515+
if (ast.getComments() != null) {
516+
ast.getComments().forEach(comment -> comments.add(comment.getValue()));
517+
}
518+
} catch (EvaluatorException ee) {
519+
Stats.incCounter("stats.ascan.rule." + PLUGIN_ID + ".jsparse.fail");
520+
LOGGER.debug(ee.getMessage());
521+
}
522+
return comments;
523+
}
524+
485525
@Override
486526
public int getRisk() {
487527
return Alert.RISK_HIGH;

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

Lines changed: 205 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,17 @@
2121

2222
import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
2323
import static org.hamcrest.MatcherAssert.assertThat;
24+
import static org.hamcrest.Matchers.empty;
2425
import static org.hamcrest.Matchers.equalTo;
2526
import static org.hamcrest.Matchers.is;
2627

2728
import fi.iki.elonen.NanoHTTPD;
2829
import fi.iki.elonen.NanoHTTPD.Response;
2930
import java.util.List;
3031
import java.util.Map;
32+
import java.util.Set;
3133
import java.util.stream.Stream;
34+
import org.junit.jupiter.api.Nested;
3235
import org.junit.jupiter.api.Test;
3336
import org.junit.jupiter.params.ParameterizedTest;
3437
import org.junit.jupiter.params.provider.Arguments;
@@ -527,6 +530,48 @@ void shouldReportRedirectWithJsLocationMethods(String jsMethod) throws Exception
527530
assertThat(alertsRaised.get(0).getEvidence().startsWith(HttpHeader.HTTP), equalTo(true));
528531
}
529532

533+
@Test
534+
void shouldNotReportRedirectIfInsideJsComment() throws Exception {
535+
// Given
536+
String test = "/";
537+
String body =
538+
"""
539+
<!DOCTYPE html>
540+
<html>
541+
<head>
542+
<title>Redirect commented out</title>
543+
</head>
544+
<body>
545+
546+
<script>function myRedirectFunction()
547+
{/*
548+
window.location.replace('%s');
549+
*/}
550+
//myRedirectFunction();
551+
</script>
552+
"""
553+
.formatted(CONTENT_TOKEN);
554+
nano.addHandler(
555+
new NanoServerHandler(test) {
556+
@Override
557+
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
558+
String site = getFirstParamValue(session, "site");
559+
if (site != null && !site.isEmpty()) {
560+
String withPayload = body.replace(CONTENT_TOKEN, site);
561+
return newFixedLengthResponse(
562+
NanoHTTPD.Response.Status.OK, NanoHTTPD.MIME_HTML, withPayload);
563+
}
564+
return newFixedLengthResponse("<html><body></body></html>");
565+
}
566+
});
567+
HttpMessage msg = getHttpMessage(test + "?site=xxx");
568+
rule.init(msg, parent);
569+
// When
570+
rule.scan();
571+
// Then
572+
assertThat(alertsRaised, is(empty()));
573+
}
574+
530575
private static Stream<Arguments> createJsMethodBooleanPairs() {
531576
return Stream.of(
532577
Arguments.of("location.reload", true),
@@ -556,7 +601,12 @@ void shouldNotReportRedirectWithJsLocationMethodsWhenConcatenated(
556601
}
557602

558603
@ParameterizedTest
559-
@ValueSource(strings = {"window.open", "window.navigate"})
604+
@ValueSource(
605+
strings = {
606+
"window.open",
607+
"window.navigate",
608+
"let r = /http:\\/\\/[a-z]+/g; window.navigate"
609+
})
560610
void shouldReportRedirectWithJsWindowMethods(String jsMethod) throws Exception {
561611
// Given
562612
String test = "/";
@@ -631,4 +681,158 @@ void shouldFindLocationUrl(String input) {
631681
// Then
632682
assertThat(extracted, is(equalTo("http://www.example.com/")));
633683
}
684+
685+
/** Unit tests for {@link ExternalRedirectScanRule#extractJsComments(String)}. */
686+
@Nested
687+
class ExtractJsCommentsUnitTest {
688+
689+
private static Stream<Arguments> commentProvider() {
690+
return Stream.of(
691+
Arguments.of("Empty line comment", "//", Set.of("//")),
692+
Arguments.of("Empty block comment", "/**/", Set.of("/**/")),
693+
Arguments.of("Block comment", "/* comment \n*/", Set.of("/* comment \n*/")),
694+
Arguments.of(
695+
"Line comment with CRLF",
696+
"console.log('x'); // comment\r\nconsole.log('y');",
697+
Set.of("// comment")),
698+
Arguments.of(
699+
"Block comment containing line terminator + line comment",
700+
"/* block start\n// inside block */ console.log('x');",
701+
Set.of("/* block start\n// inside block */")),
702+
Arguments.of(
703+
"Escaped quote before comment",
704+
"console.log('it\\'s fine'); // real comment",
705+
Set.of("// real comment")),
706+
Arguments.of(
707+
"Escaped backslash before comment",
708+
"console.log('c:\\\\'); // comment",
709+
Set.of("// comment")),
710+
Arguments.of("Single line", "// comment ", Set.of("// comment ")),
711+
Arguments.of(
712+
"Block inside Single line",
713+
"// /* comment; */",
714+
Set.of("// /* comment; */")),
715+
Arguments.of(
716+
"Single line inside Block comment",
717+
"/* comment \n // example */",
718+
Set.of("/* comment \n // example */")),
719+
Arguments.of(
720+
"Inline block",
721+
"console.log(\"example\"); /* console.log('comment'); */",
722+
Set.of("/* console.log('comment'); */")),
723+
Arguments.of(
724+
"Inline single line",
725+
"console.log(\"example\"); // console.log('comment'));",
726+
Set.of("// console.log('comment'));")),
727+
Arguments.of(
728+
"Inline single line (w/ unicode escape)",
729+
"console.log(\"🔥 example\"); // console.log('\u1F525 example');",
730+
Set.of("// console.log('\u1F525 example');")),
731+
Arguments.of(
732+
"Template literal with embedded expression",
733+
"console.log(`value ${1 + 1}`); // comment;",
734+
Set.of("// comment;")),
735+
Arguments.of(
736+
"Template expression with block comment",
737+
"console.log(`value ${ /* block comment */ 42 }`);",
738+
Set.of("/* block comment */")),
739+
Arguments.of(
740+
"Multiline nested template expression",
741+
"console.log(`line1 ${ `inner ${42} // not comment` }`); // real comment",
742+
Set.of("// real comment")),
743+
Arguments.of(
744+
"Nested template with string containing comment-like text",
745+
"console.log(`outer ${ 'string // not comment' }`); // real comment",
746+
Set.of("// real comment")),
747+
Arguments.of(
748+
"Regex literal followed by comment",
749+
"var re = /abc/; // trailing comment",
750+
Set.of("// trailing comment")),
751+
Arguments.of(
752+
"Regex literal containing /* ... */ in class",
753+
"var re = /a\\/\\*b/; // trailing comment",
754+
Set.of("// trailing comment")),
755+
Arguments.of(
756+
"Regex-like in comment",
757+
"/* /http:\\/\\/evil.com/ */",
758+
Set.of("/* /http:\\/\\/evil.com/ */")));
759+
}
760+
761+
@ParameterizedTest(name = "{0}")
762+
@MethodSource("commentProvider")
763+
void shouldFindExpectedComments(String name, String input, Set<String> expectedComments) {
764+
// Given / When
765+
Set<String> actualComments = ExternalRedirectScanRule.extractJsComments(input);
766+
// Then
767+
assertThat(
768+
String.format(
769+
"Test '%s' failed. Expected %s but got %s",
770+
name, expectedComments, actualComments),
771+
actualComments,
772+
is(expectedComments));
773+
}
774+
775+
private static Stream<Arguments> sequentialCommentsProvider() {
776+
return Stream.of(
777+
Arguments.of(
778+
"Single line comment sequence",
779+
"// first\n//second\nconsole.log('x');",
780+
Set.of("// first", "//second")),
781+
Arguments.of(
782+
"Single line and block comment sequence",
783+
"// first\n/*second*/\nconsole.log('x');",
784+
Set.of("// first", "/*second*/")),
785+
Arguments.of(
786+
"Template expression with inner comment",
787+
"console.log(`outer ${ /* inner comment */ 42 }`); // trailing comment",
788+
Set.of("/* inner comment */", "// trailing comment")),
789+
Arguments.of(
790+
"Block comment sequence",
791+
"/* first*/\n/*second*/\nconsole.log('x');",
792+
Set.of("/* first*/", "/*second*/")));
793+
}
794+
795+
@ParameterizedTest(name = "{0}")
796+
@MethodSource("sequentialCommentsProvider")
797+
void shouldIdentifyMultipleComments(
798+
String name, String input, Set<String> expectedComments) {
799+
// Given / When
800+
Set<String> actualComments = ExternalRedirectScanRule.extractJsComments(input);
801+
// Then
802+
assertThat(
803+
"Unexpected comment set for test: " + name,
804+
actualComments,
805+
equalTo(expectedComments));
806+
}
807+
808+
private static Stream<Arguments> nonCommentStringsProvider() {
809+
return Stream.of(
810+
Arguments.of("String containing //", "console.log('not // a comment');"),
811+
Arguments.of(
812+
"String containing /* */",
813+
"console.log('not /* a comment */ either');"),
814+
Arguments.of(
815+
"Unterminated string before comment",
816+
"console.log('unterminated // not a comment"),
817+
Arguments.of("regex literal", "let r = /http:\\/\\/example.com/;"),
818+
Arguments.of(
819+
"regex with comment-like content", "let r = /\\/\\* comment *\\/g;"),
820+
// Unterminated template literal results in JS error
821+
Arguments.of(
822+
"Unterminated template literal",
823+
"console.log(`unterminated template ${1+1} // comment not terminated"),
824+
Arguments.of(
825+
"Inline incomplete block",
826+
"console.log(\"example\"); /* console.log('comment');"));
827+
}
828+
829+
@ParameterizedTest(name = "{0}")
830+
@MethodSource("nonCommentStringsProvider")
831+
void shouldNotFindAComment(String name, String input) {
832+
// Given / When
833+
Set<String> comments = ExternalRedirectScanRule.extractJsComments(input);
834+
// Then
835+
assertThat(comments, is(empty()));
836+
}
837+
}
634838
}

‎gradle/libs.versions.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ slf4j = "2.0.17"
1616

1717
[libraries]
1818
ascanrules-procyonCompilerTools = "org.bitbucket.mstrobel:procyon-compilertools:0.6.0"
19+
ascanrules-rhino = "org.mozilla:rhino:1.8.0"
1920
ascanrulesBeta-diffutils = { module = "com.googlecode.java-diff-utils:diffutils", version.ref = "diffutils" }
2021
ascanrulesBeta-jsoup = { module = "org.jsoup:jsoup", version.ref = "jsoup" }
2122
authhelper-otpJava = "com.github.bastiaanjansen:otp-java:2.1.0"

0 commit comments

Comments
 (0)