Skip to content

Commit 125facc

Browse files
kingthorinkingthorin
committed
ascanrules: Address External Redirect False Positives
Signed-off-by: kingthorin <[email protected]>
1 parent 0bc2adb commit 125facc

File tree

3 files changed

+517
-0
lines changed

3 files changed

+517
-0
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
77
### Fixed
88
- Hidden Files rule raising false positives if server returning 200 for files that don't exist (Issue 8434).
99

10+
### Changed
11+
- The External Redirect scan rule has been updated to account for potential false positives involving JavaScript comments.
12+
1013
## [73] - 2025-09-02
1114
### Changed
1215
- Maintenance changes.

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

Lines changed: 262 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,268 @@ 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 (!isPresent(matcher)) {
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);
494+
return isPresent(matcher);
495+
}
496+
497+
private static boolean isPresent(Matcher matcher) {
480498
return matcher.find()
481499
&& StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
482500
}
483501

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

0 commit comments

Comments
 (0)