Skip to content

Commit 2d3fd86

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

File tree

3 files changed

+406
-1
lines changed

3 files changed

+406
-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: 186 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,191 @@ 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+
protected 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 == '\\') {
570+
i++;
571+
} else if (c == '\'') {
572+
state = stack.pop();
573+
}
574+
break;
575+
576+
case IN_DOUBLE_QUOTE:
577+
if (c == '\\') {
578+
i++;
579+
} else if (c == '"') {
580+
state = stack.pop();
581+
}
582+
break;
583+
584+
case IN_TEMPLATE:
585+
if (c == '`') {
586+
state = stack.pop();
587+
} else if (c == '\\') {
588+
i++;
589+
} else if (c == '$' && next == '{') {
590+
stack.push(state);
591+
state = State.IN_TEMPLATE_EXPR;
592+
i++;
593+
}
594+
break;
595+
596+
case IN_TEMPLATE_EXPR:
597+
if (c == '}') {
598+
state = stack.pop();
599+
} else if (c == '\'' || c == '"') {
600+
stack.push(state);
601+
state = (c == '\'') ? State.IN_SINGLE_QUOTE : State.IN_DOUBLE_QUOTE;
602+
} else if (c == '`') {
603+
stack.push(state);
604+
state = State.IN_TEMPLATE;
605+
} else if (c == '/' && next == '/') {
606+
i++;
607+
stack.push(state);
608+
current.setLength(0);
609+
current.append("//");
610+
state = State.IN_LINE_COMMENT_IN_TEMPLATE;
611+
} else if (c == '/' && next == '*') {
612+
i++;
613+
stack.push(state);
614+
current.setLength(0);
615+
current.append("/*");
616+
state = State.IN_BLOCK_COMMENT_IN_TEMPLATE;
617+
}
618+
break;
619+
620+
case IN_LINE_COMMENT_IN_TEMPLATE:
621+
if (c == '\n' || c == '\r' || c == '\u2028' || c == '\u2029') {
622+
String trimmed =
623+
current.toString().replaceAll("[\\r\\n\\u2028\\u2029]+$", "");
624+
comments.add(trimmed);
625+
state = stack.pop();
626+
} else {
627+
current.append(c);
628+
}
629+
break;
630+
631+
case IN_BLOCK_COMMENT_IN_TEMPLATE:
632+
current.append(c);
633+
if (c == '*' && next == '/') {
634+
current.append('/');
635+
i++;
636+
comments.add(current.toString());
637+
state = stack.pop();
638+
}
639+
break;
640+
}
641+
642+
// Special handling: detect </script> while in block comment and not yet closed
643+
if ((state == State.IN_BLOCK_COMMENT || state == State.IN_BLOCK_COMMENT_IN_TEMPLATE)
644+
&& i + 9 < len) {
645+
// Case-insensitive check for </script>
646+
String segment = source.substring(i, Math.min(len, i + 9)).toLowerCase(Locale.ROOT);
647+
if (segment.startsWith("</script")) {
648+
// Capture everything up to start of </script>
649+
current.setLength(current.length() - 1); // Don't include the <
650+
comments.add(current.toString());
651+
state =
652+
(state == State.IN_BLOCK_COMMENT_IN_TEMPLATE && !stack.isEmpty())
653+
? stack.pop()
654+
: State.DEFAULT;
655+
// Do not consume the tag itself (so parser can continue properly)
656+
}
657+
}
658+
}
659+
660+
// Handle unterminated cases (EOF)
661+
if (state == State.IN_LINE_COMMENT
662+
|| state == State.IN_LINE_COMMENT_IN_TEMPLATE
663+
|| state == State.IN_BLOCK_COMMENT
664+
|| state == State.IN_BLOCK_COMMENT_IN_TEMPLATE) {
665+
comments.add(current.toString());
666+
}
667+
668+
return comments;
669+
}
670+
485671
@Override
486672
public int getRisk() {
487673
return Alert.RISK_HIGH;

0 commit comments

Comments
 (0)