Skip to content

Commit cb0c82e

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

File tree

3 files changed

+397
-1
lines changed

3 files changed

+397
-1
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
66
## Unreleased
77
### Changed
88
- Address potential false positives with the XSLT Injection scan rule when payloads cause a failure which may still contain the expected evidence.
9+
- The External Redirect scan rule has been updated to account for potential false positives involving JavaScript comments.
910

1011
## [74] - 2025-09-18
1112
### Added

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

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,16 @@
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.LinkedHashSet;
2629
import java.util.List;
30+
import java.util.Locale;
2731
import java.util.Map;
32+
import java.util.Set;
2833
import java.util.UUID;
2934
import java.util.regex.Matcher;
3035
import java.util.regex.Pattern;
@@ -478,10 +483,182 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
478483

479484
private static boolean isRedirectPresent(Pattern pattern, String value) {
480485
Matcher matcher = pattern.matcher(value);
486+
if (!matcher.find()) {
487+
return false;
488+
}
489+
Set<String> extractedComments = extractJsComments(value);
490+
String valueWithoutComments = value;
491+
for (String comment : extractedComments) {
492+
valueWithoutComments = valueWithoutComments.replace(comment, "");
493+
}
494+
495+
matcher = pattern.matcher(valueWithoutComments);
481496
return matcher.find()
482497
&& StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
483498
}
484499

500+
private enum State {
501+
DEFAULT,
502+
IN_SINGLE_QUOTE,
503+
IN_DOUBLE_QUOTE,
504+
IN_TEMPLATE,
505+
IN_TEMPLATE_EXPR,
506+
IN_LINE_COMMENT,
507+
IN_BLOCK_COMMENT,
508+
IN_LINE_COMMENT_IN_TEMPLATE,
509+
IN_BLOCK_COMMENT_IN_TEMPLATE
510+
}
511+
512+
/** Visibility increased for unit testing purposes only */
513+
public static Set<String> extractJsComments(String source) {
514+
Set<String> comments = new LinkedHashSet<>();
515+
StringBuilder current = new StringBuilder();
516+
517+
Deque<State> stack = new ArrayDeque<>();
518+
State state = State.DEFAULT;
519+
520+
final int len = source.length();
521+
for (int i = 0; i < len; i++) {
522+
char c = source.charAt(i);
523+
char next = (i + 1 < len) ? source.charAt(i + 1) : 0;
524+
525+
switch (state) {
526+
case DEFAULT:
527+
if (c == '/' && next == '/') {
528+
i++;
529+
current.setLength(0);
530+
current.append("//");
531+
state = State.IN_LINE_COMMENT;
532+
} else if (c == '/' && next == '*') {
533+
i++;
534+
current.setLength(0);
535+
current.append("/*");
536+
state = State.IN_BLOCK_COMMENT;
537+
} else if (c == '"' || c == '\'') {
538+
stack.push(state);
539+
state = (c == '"') ? State.IN_DOUBLE_QUOTE : State.IN_SINGLE_QUOTE;
540+
} else if (c == '`') {
541+
stack.push(state);
542+
state = State.IN_TEMPLATE;
543+
}
544+
break;
545+
546+
case IN_LINE_COMMENT:
547+
if (c == '\n' || c == '\r' || c == '\u2028' || c == '\u2029') {
548+
// Trim trailing whitespace and line terminators before storing
549+
String trimmed =
550+
current.toString().replaceAll("[\\r\\n\\u2028\\u2029]+$", "");
551+
comments.add(trimmed);
552+
state = State.DEFAULT;
553+
} else {
554+
current.append(c);
555+
}
556+
break;
557+
558+
case IN_BLOCK_COMMENT:
559+
current.append(c);
560+
if (c == '*' && next == '/') {
561+
current.append('/');
562+
i++;
563+
comments.add(current.toString());
564+
state = State.DEFAULT;
565+
}
566+
break;
567+
568+
case IN_SINGLE_QUOTE:
569+
if (c == '\\') i++;
570+
else if (c == '\'') state = stack.pop();
571+
break;
572+
573+
case IN_DOUBLE_QUOTE:
574+
if (c == '\\') i++;
575+
else if (c == '"') state = stack.pop();
576+
break;
577+
578+
case IN_TEMPLATE:
579+
if (c == '`') state = stack.pop();
580+
else if (c == '\\') i++;
581+
else if (c == '$' && next == '{') {
582+
stack.push(state);
583+
state = State.IN_TEMPLATE_EXPR;
584+
i++;
585+
}
586+
break;
587+
588+
case IN_TEMPLATE_EXPR:
589+
if (c == '}') state = stack.pop();
590+
else if (c == '\'' || c == '"') {
591+
stack.push(state);
592+
state = (c == '\'') ? State.IN_SINGLE_QUOTE : State.IN_DOUBLE_QUOTE;
593+
} else if (c == '`') {
594+
stack.push(state);
595+
state = State.IN_TEMPLATE;
596+
} else if (c == '/' && next == '/') {
597+
i++;
598+
stack.push(state);
599+
current.setLength(0);
600+
current.append("//");
601+
state = State.IN_LINE_COMMENT_IN_TEMPLATE;
602+
} else if (c == '/' && next == '*') {
603+
i++;
604+
stack.push(state);
605+
current.setLength(0);
606+
current.append("/*");
607+
state = State.IN_BLOCK_COMMENT_IN_TEMPLATE;
608+
}
609+
break;
610+
611+
case IN_LINE_COMMENT_IN_TEMPLATE:
612+
if (c == '\n' || c == '\r' || c == '\u2028' || c == '\u2029') {
613+
String trimmed =
614+
current.toString().replaceAll("[\\r\\n\\u2028\\u2029]+$", "");
615+
comments.add(trimmed);
616+
state = stack.pop();
617+
} else {
618+
current.append(c);
619+
}
620+
break;
621+
622+
case IN_BLOCK_COMMENT_IN_TEMPLATE:
623+
current.append(c);
624+
if (c == '*' && next == '/') {
625+
current.append('/');
626+
i++;
627+
comments.add(current.toString());
628+
state = stack.pop();
629+
}
630+
break;
631+
}
632+
633+
// Special handling: detect </script> while in block comment and not yet closed
634+
if ((state == State.IN_BLOCK_COMMENT || state == State.IN_BLOCK_COMMENT_IN_TEMPLATE)
635+
&& i + 9 < len) {
636+
// Case-insensitive check for </script>
637+
String segment = source.substring(i, Math.min(len, i + 9)).toLowerCase(Locale.ROOT);
638+
if (segment.startsWith("</script")) {
639+
// Capture everything up to start of </script>
640+
current.setLength(current.length() - 1); // Don't include the <
641+
comments.add(current.toString());
642+
state =
643+
(state == State.IN_BLOCK_COMMENT_IN_TEMPLATE && !stack.isEmpty())
644+
? stack.pop()
645+
: State.DEFAULT;
646+
// Do not consume the tag itself (so parser can continue properly)
647+
}
648+
}
649+
}
650+
651+
// Handle unterminated cases (EOF)
652+
if (state == State.IN_LINE_COMMENT
653+
|| state == State.IN_LINE_COMMENT_IN_TEMPLATE
654+
|| state == State.IN_BLOCK_COMMENT
655+
|| state == State.IN_BLOCK_COMMENT_IN_TEMPLATE) {
656+
comments.add(current.toString());
657+
}
658+
659+
return comments;
660+
}
661+
485662
@Override
486663
public int getRisk() {
487664
return Alert.RISK_HIGH;

0 commit comments

Comments
 (0)