Skip to content

Commit 9b9d595

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

File tree

3 files changed

+514
-1
lines changed

3 files changed

+514
-1
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: 258 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.LinkedHashSet;
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,264 @@ 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+
SLASH,
501+
LINE_COMMENT,
502+
BLOCK_COMMENT,
503+
STRING_SINGLE,
504+
STRING_DOUBLE,
505+
TEMPLATE,
506+
TEMPLATE_EXPR
507+
}
508+
509+
/** Visibility increased for unit testing purposes only */
510+
protected static Set<String> extractJsComments(String code) {
511+
Set<String> comments = new LinkedHashSet<>();
512+
StringBuilder current = null;
513+
514+
Deque<State> stateStack = new ArrayDeque<>();
515+
Deque<Integer> exprDepthStack = new ArrayDeque<>(); // for nested ${...}
516+
517+
State state = State.NORMAL;
518+
int i = 0;
519+
final int n = code.length();
520+
521+
while (i < n) {
522+
char c = code.charAt(i);
523+
524+
switch (state) {
525+
case NORMAL:
526+
if (c == '"') {
527+
stateStack.push(state);
528+
state = State.STRING_DOUBLE;
529+
i++;
530+
} else if (c == '\'') {
531+
stateStack.push(state);
532+
state = State.STRING_SINGLE;
533+
i++;
534+
} else if (c == '`') {
535+
stateStack.push(state);
536+
state = State.TEMPLATE;
537+
i++;
538+
} else if (c == '/') {
539+
state = State.SLASH;
540+
i++;
541+
} else {
542+
i++;
543+
}
544+
break;
545+
546+
case SLASH:
547+
if (i >= n) {
548+
state = State.NORMAL;
549+
break;
550+
}
551+
char d = code.charAt(i);
552+
if (d == '/') {
553+
current = new StringBuilder("//");
554+
state = State.LINE_COMMENT;
555+
i++;
556+
} else if (d == '*') {
557+
current = new StringBuilder("/*");
558+
state = State.BLOCK_COMMENT;
559+
i++;
560+
} else {
561+
state = State.NORMAL;
562+
}
563+
break;
564+
565+
case LINE_COMMENT:
566+
if (i < n) {
567+
char ch = code.charAt(i);
568+
current.append(ch);
569+
i++;
570+
if (isLineTerminator(ch)) {
571+
comments.add(current.toString());
572+
current = null;
573+
state = State.NORMAL;
574+
}
575+
} else {
576+
// EOF inside line comment
577+
comments.add(current.toString());
578+
current = null;
579+
state = State.NORMAL;
580+
}
581+
break;
582+
583+
case BLOCK_COMMENT:
584+
if (i < n) {
585+
char ch = code.charAt(i);
586+
current.append(ch);
587+
if (ch == '*' && i + 1 < n && code.charAt(i + 1) == '/') {
588+
current.append('/');
589+
i += 2;
590+
comments.add(current.toString());
591+
current = null;
592+
state = State.NORMAL;
593+
} else {
594+
i++;
595+
}
596+
} else {
597+
// EOF inside block comment
598+
comments.add(current.toString());
599+
current = null;
600+
state = State.NORMAL;
601+
}
602+
break;
603+
604+
case STRING_DOUBLE:
605+
if (c == '\\') {
606+
i = consumeJsEscape(code, i) + 1;
607+
} else if (c == '"') {
608+
state = stateStack.pop();
609+
i++;
610+
} else {
611+
i++;
612+
}
613+
break;
614+
615+
case STRING_SINGLE:
616+
if (c == '\\') {
617+
i = consumeJsEscape(code, i) + 1;
618+
} else if (c == '\'') {
619+
state = stateStack.pop();
620+
i++;
621+
} else {
622+
i++;
623+
}
624+
break;
625+
626+
case TEMPLATE:
627+
if (c == '\\') {
628+
i = consumeJsEscape(code, i) + 1;
629+
} else if (c == '`') {
630+
state = stateStack.pop();
631+
i++;
632+
} else if (c == '$' && i + 1 < n && code.charAt(i + 1) == '{') {
633+
stateStack.push(state);
634+
exprDepthStack.push(1); // start of template expression
635+
state = State.TEMPLATE_EXPR;
636+
i += 2;
637+
} else {
638+
i++;
639+
}
640+
break;
641+
642+
case TEMPLATE_EXPR:
643+
if (c == '"') {
644+
stateStack.push(state);
645+
state = State.STRING_DOUBLE;
646+
i++;
647+
} else if (c == '\'') {
648+
stateStack.push(state);
649+
state = State.STRING_SINGLE;
650+
i++;
651+
} else if (c == '`') {
652+
stateStack.push(state);
653+
state = State.TEMPLATE;
654+
i++;
655+
} else if (c == '{') {
656+
exprDepthStack.push(exprDepthStack.pop() + 1);
657+
i++;
658+
} else if (c == '}') {
659+
int depth = exprDepthStack.pop() - 1;
660+
if (depth == 0) {
661+
state = stateStack.pop();
662+
} else {
663+
exprDepthStack.push(depth);
664+
}
665+
i++;
666+
} else if (c == '/') {
667+
state = State.SLASH;
668+
i++;
669+
} else {
670+
i++;
671+
}
672+
break;
673+
}
674+
}
675+
676+
// EOF handling: finalize any pending comment regardless of state
677+
if (current != null) {
678+
comments.add(current.toString());
679+
}
680+
681+
return comments;
682+
}
683+
684+
private static int consumeJsEscape(String s, int backslash) {
685+
int n = s.length();
686+
int i = backslash;
687+
if (i + 1 >= n) return i;
688+
689+
char e = s.charAt(i + 1);
690+
691+
if (isLineTerminator(e)) {
692+
if (e == '\r' && i + 2 < n && s.charAt(i + 2) == '\n') return i + 2;
693+
return i + 1;
694+
}
695+
696+
if (e == 'x' || e == 'X') {
697+
int j = i + 2, count = 0;
698+
while (j < n && count < 2 && isHexDigit(s.charAt(j))) {
699+
j++;
700+
count++;
701+
}
702+
return j - 1;
703+
}
704+
705+
if (e == 'u' || e == 'U') {
706+
int j = i + 2;
707+
if (j < n && s.charAt(j) == '{') {
708+
j++;
709+
while (j < n && isHexDigit(s.charAt(j))) j++;
710+
if (j < n && s.charAt(j) == '}') j++;
711+
return j - 1;
712+
} else {
713+
int count = 0;
714+
while (j < n && count < 4 && isHexDigit(s.charAt(j))) {
715+
j++;
716+
count++;
717+
}
718+
return j - 1;
719+
}
720+
}
721+
722+
if (e >= '0' && e <= '7') {
723+
int j = i + 1, count = 0;
724+
while (j < n && count < 3 && s.charAt(j) >= '0' && s.charAt(j) <= '7') {
725+
j++;
726+
count++;
727+
}
728+
return j - 1;
729+
}
730+
731+
return i + 1;
732+
}
733+
734+
private static boolean isHexDigit(char c) {
735+
return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F');
736+
}
737+
738+
private static boolean isLineTerminator(char c) {
739+
return c == '\n' || c == '\r' || c == '\u2028' || c == '\u2029';
740+
}
741+
484742
@Override
485743
public int getRisk() {
486744
return Alert.RISK_HIGH;

0 commit comments

Comments
 (0)