Skip to content

Commit ff39efb

Browse files
kingthorinkingthorin
committed
ascanrules: Address External Redirect False Positives
Signed-off-by: kingthorin <[email protected]>
1 parent 56520d1 commit ff39efb

File tree

3 files changed

+263
-2
lines changed

3 files changed

+263
-2
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
44
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
55

66
## Unreleased
7-
7+
### Changed
8+
- The External Redirect scan rule has been updated to account for potential false positives involving JavaScript comments.
89

910
## [73] - 2025-09-02
1011
### Changed

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

Lines changed: 153 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,15 @@
2020
package org.zaproxy.zap.extension.ascanrules;
2121

2222
import java.io.IOException;
23+
import java.util.ArrayDeque;
2324
import java.util.ArrayList;
2425
import java.util.Collections;
26+
import java.util.Deque;
2527
import java.util.HashMap;
28+
import java.util.HashSet;
2629
import java.util.List;
2730
import java.util.Map;
31+
import java.util.Set;
2832
import java.util.UUID;
2933
import java.util.regex.Matcher;
3034
import java.util.regex.Pattern;
@@ -137,6 +141,8 @@ public String getReason() {
137141
Pattern.compile(
138142
"(?i)window\\.(?:open|navigate)\\s*\\(\\s*['\"](" + SITE_PATT + ")['\"]");
139143

144+
private static final List<String> JS_PRE_CHECKS = List.of("window.", "location.", "location=");
145+
140146
/** The various (prioritized) payloads to try */
141147
private enum RedirectPayloads {
142148
PLAIN_SITE(REDIRECT_SITE, false),
@@ -476,11 +482,157 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
476482
}
477483

478484
private static boolean isRedirectPresent(Pattern pattern, String value) {
479-
Matcher matcher = pattern.matcher(value);
485+
// We know the payload is present.
486+
// Ensure the value has something we're likely interested in before dealing with comments
487+
if (JS_PRE_CHECKS.stream().noneMatch(chk -> StringUtils.containsIgnoreCase(value, chk))) {
488+
return false;
489+
}
490+
Set<String> extractedComments = extractJsComments(value);
491+
String valueWithoutComments = value;
492+
for (String comment : extractedComments) {
493+
valueWithoutComments = valueWithoutComments.replace(comment, "");
494+
}
495+
496+
Matcher matcher = pattern.matcher(valueWithoutComments);
480497
return matcher.find()
481498
&& StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
482499
}
483500

501+
enum State {
502+
NORMAL,
503+
STRING_DOUBLE,
504+
STRING_SINGLE,
505+
TEMPLATE,
506+
TEMPLATE_EXPR,
507+
SLASH,
508+
LINE_COMMENT,
509+
BLOCK_COMMENT,
510+
BLOCK_COMMENT_STAR
511+
}
512+
513+
private static Set<String> extractJsComments(String code) {
514+
Set<String> comments = new HashSet<>();
515+
StringBuilder currentComment = new StringBuilder();
516+
517+
State state = State.NORMAL;
518+
Deque<State> stack = new ArrayDeque<>();
519+
boolean escape = false;
520+
521+
for (int i = 0; i < code.length(); i++) {
522+
char c = code.charAt(i);
523+
524+
switch (state) {
525+
case NORMAL:
526+
if (c == '"') {
527+
state = State.STRING_DOUBLE;
528+
} else if (c == '\'') {
529+
state = State.STRING_SINGLE;
530+
} else if (c == '`') {
531+
state = State.TEMPLATE;
532+
} else if (c == '/') {
533+
state = State.SLASH;
534+
}
535+
break;
536+
537+
case SLASH:
538+
if (c == '/') {
539+
state = State.LINE_COMMENT;
540+
currentComment.setLength(0);
541+
currentComment.append("//");
542+
} else if (c == '*') {
543+
state = State.BLOCK_COMMENT;
544+
currentComment.setLength(0);
545+
currentComment.append("/*");
546+
} else {
547+
state = State.NORMAL; // not actually a comment
548+
}
549+
break;
550+
551+
case LINE_COMMENT:
552+
currentComment.append(c);
553+
if (c == '\n' || c == '\r' || c == '\u2028' || c == '\u2029') {
554+
comments.add(currentComment.toString());
555+
state = State.NORMAL;
556+
}
557+
break;
558+
559+
case BLOCK_COMMENT:
560+
currentComment.append(c);
561+
if (c == '*') {
562+
state = State.BLOCK_COMMENT_STAR;
563+
}
564+
break;
565+
566+
case BLOCK_COMMENT_STAR:
567+
currentComment.append(c);
568+
if (c == '/') {
569+
comments.add(currentComment.toString());
570+
state = State.NORMAL;
571+
} else {
572+
state = State.BLOCK_COMMENT;
573+
}
574+
break;
575+
576+
case STRING_DOUBLE:
577+
if (escape) {
578+
escape = false;
579+
} else if (c == '\\') {
580+
escape = true;
581+
} else if (c == '"') {
582+
state = State.NORMAL;
583+
}
584+
break;
585+
586+
case STRING_SINGLE:
587+
if (escape) {
588+
escape = false;
589+
} else if (c == '\\') {
590+
escape = true;
591+
} else if (c == '\'') {
592+
state = State.NORMAL;
593+
}
594+
break;
595+
596+
case TEMPLATE:
597+
if (escape) {
598+
escape = false;
599+
} else if (c == '\\') {
600+
escape = true;
601+
} else if (c == '`') {
602+
state = State.NORMAL;
603+
} else if (c == '$' && i + 1 < code.length() && code.charAt(i + 1) == '{') {
604+
stack.push(state);
605+
state = State.TEMPLATE_EXPR;
606+
i++; // skip '{'
607+
}
608+
break;
609+
610+
case TEMPLATE_EXPR:
611+
if (c == '}') {
612+
state = stack.pop();
613+
} else if (c == '"') {
614+
state = State.STRING_DOUBLE;
615+
} else if (c == '\'') {
616+
state = State.STRING_SINGLE;
617+
} else if (c == '`') {
618+
state = State.TEMPLATE;
619+
} else if (c == '/') {
620+
state = State.SLASH;
621+
}
622+
break;
623+
}
624+
}
625+
626+
// Handle unterminated comments at EOF
627+
if (state == State.LINE_COMMENT) {
628+
comments.add(currentComment.toString());
629+
} else if (state == State.BLOCK_COMMENT || state == State.BLOCK_COMMENT_STAR) {
630+
comments.add(currentComment.toString());
631+
}
632+
633+
return comments;
634+
}
635+
484636
@Override
485637
public int getRisk() {
486638
return Alert.RISK_HIGH;

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

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -531,6 +531,114 @@ void shouldReportRedirectWithJsLocationMethods(String jsMethod) throws Exception
531531
assertThat(alertsRaised.get(0).getEvidence().startsWith(HttpHeader.HTTP), equalTo(true));
532532
}
533533

534+
private static Stream<Arguments> provideCommentStrings() {
535+
return Stream.of(
536+
// Some need to be double escaped because of Java
537+
Arguments.of("Block comment", "/* window.location.replace('@@@content@@@');\n*/"),
538+
Arguments.of("Single line", "// window.location.replace('@@@content@@@');"),
539+
Arguments.of(
540+
"Inline block",
541+
"console.log(\"example\"); /* console.log(window.location.replace('@@@content@@@')); */"),
542+
Arguments.of(
543+
"Inline incomplete block",
544+
"console.log(\"example\"); /* console.log(window.location.replace('@@@content@@@')); "),
545+
Arguments.of(
546+
"Inline single line",
547+
"console.log(\"example\"); // console.log(window.location.replace('@@@content@@@'));"),
548+
Arguments.of(
549+
"Inline single line (w/ unicode escape)",
550+
"console.log(\"🔥 example\"); // console.log('\\u1F525 window.location.replace('@@@content@@@')');"),
551+
Arguments.of(
552+
"Inline single line (w/ malformed (leading) unicode escape)",
553+
"console.log(\"🔥 example\"); // console.log('\\u 1F525 window.location.replace('@@@content@@@')');"),
554+
Arguments.of(
555+
"Inline single line (w/ malformed (mid) unicode escape)",
556+
"console.log(\"🔥 example\"); // console.log('\\u1F 525 window.location.replace('@@@content@@@')');"),
557+
Arguments.of(
558+
"Inline single line (surrogate pair unicode escape)",
559+
"console.log(\"example\"); // console.log('\\uD83D\\uDD25 window.location.replace('@@@content@@@')');"),
560+
Arguments.of(
561+
"Inline single line (malformed surrogate pair unicode escape)",
562+
"console.log(\"example\"); // console.log('\\uD83D\\uD D25 window.location.replace('@@@content@@@')');"),
563+
Arguments.of(
564+
"Inline single line (w/ braced unicode escape)",
565+
"console.log(\"🔥 example\"); // console.log('\\u{1F525} window.location.replace('@@@content@@@')');"),
566+
Arguments.of(
567+
"Inline single line (w/ malformed braced unicode escape)",
568+
"console.log(\"🔥 example\"); // console.log('\\u {1F525} window.location.replace('@@@content@@@')');"),
569+
Arguments.of(
570+
"Inline single line (octal escape)",
571+
"console.log(\"example\"); // console.log('\\141 window.location.replace('@@@content@@@')');"),
572+
Arguments.of(
573+
"Inline single line (malformed octal)",
574+
"console.log(\"example\"); // console.log('\\8 window.location.replace('@@@content@@@')');"),
575+
Arguments.of(
576+
"Inline single line (w/ hex escape)",
577+
"console.log(\"example\"); // console.log('\\x41 window.location.replace('@@@content@@@')');"),
578+
Arguments.of(
579+
"Inline single line (w/ malformed (leading) hex escape)",
580+
"console.log(\"example\"); // console.log('\\x 41 window.location.replace('@@@content@@@')');"),
581+
Arguments.of(
582+
"Inline single line (w/ malformed (mid) hex escape)",
583+
"console.log(\"example\"); // console.log('\\x4 1 window.location.replace('@@@content@@@')');"),
584+
Arguments.of(
585+
"Inline single line (w/ single char escapes)",
586+
"console.log(\"example\"); // console.log('\\r\\n\\twindow.location.replace('@@@content@@@')');"),
587+
Arguments.of(
588+
"Embedded template expression",
589+
"console.log('value ${1 + 1}'); // comment with window.location.replace('@@@content@@@');"),
590+
Arguments.of(
591+
"Template literal with embedded expression",
592+
"console.log(`value ${1 + 1}`); // comment with window.location.replace('@@@content@@@');"),
593+
Arguments.of(
594+
"Template literal expression containing //",
595+
"console.log(\"value ${ 'not // a comment' }\"); // real comment window.location.replace('@@@content@@@')"),
596+
Arguments.of(
597+
"Template literal with escaped backtick",
598+
"console.log(\"escaped \\` backtick\"); // trailing comment window.location.replace('@@@content@@@')"));
599+
}
600+
601+
@ParameterizedTest(name = "{0}")
602+
@MethodSource("provideCommentStrings")
603+
void shouldNotReportRedirectIfInsideJsComment(String name, String content) throws Exception {
604+
// Given
605+
String test = "/";
606+
String body =
607+
"""
608+
<!DOCTYPE html>
609+
<html>
610+
<head>
611+
<title>Redirect commented out</title>
612+
</head>
613+
<body>
614+
615+
<script>function myRedirectFunction()
616+
%s
617+
//myRedirectFunction();
618+
</script>
619+
"""
620+
.formatted(content);
621+
nano.addHandler(
622+
new NanoServerHandler(test) {
623+
@Override
624+
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
625+
String site = getFirstParamValue(session, "site");
626+
if (site != null && !site.isEmpty()) {
627+
String withPayload = body.replace(CONTENT_TOKEN, site);
628+
return newFixedLengthResponse(
629+
NanoHTTPD.Response.Status.OK, NanoHTTPD.MIME_HTML, withPayload);
630+
}
631+
return newFixedLengthResponse("<html><body></body></html>");
632+
}
633+
});
634+
HttpMessage msg = getHttpMessage(test + "?site=xxx");
635+
rule.init(msg, parent);
636+
// When
637+
rule.scan();
638+
// Then
639+
assertThat(alertsRaised.size(), equalTo(0));
640+
}
641+
534642
private static Stream<Arguments> createJsMethodBooleanPairs() {
535643
return Stream.of(
536644
Arguments.of("location.reload", true),

0 commit comments

Comments
 (0)