Skip to content

Commit 40fd2ef

Browse files
kingthorinkingthorin
committed
ascanrules: Address External Redirect False Positives
Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
1 parent 56520d1 commit 40fd2ef

3 files changed

Lines changed: 259 additions & 1 deletion

File tree

‎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: 149 additions & 0 deletions
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;
@@ -477,10 +481,155 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
477481

478482
private static boolean isRedirectPresent(Pattern pattern, String value) {
479483
Matcher matcher = pattern.matcher(value);
484+
if (!matcher.find()) {
485+
return false;
486+
}
487+
Set<String> extractedComments = extractJsComments(value);
488+
String valueWithoutComments = value;
489+
for (String comment : extractedComments) {
490+
valueWithoutComments = valueWithoutComments.replace(comment, "");
491+
}
492+
493+
matcher = pattern.matcher(valueWithoutComments);
480494
return matcher.find()
481495
&& StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
482496
}
483497

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