Skip to content

Commit 68f2452

Browse files
notSumit25claude
andcommitted
fix: eliminate ReDoS in DdlStatementParser's NOT VALID detection
CodeQL flagged NOT_VALID's regex (\s+NOT\s+VALID\s*;?\s*$ via find()) as polynomial: the leading \s+ retries the match at every whitespace position in the input, so 80KB of padding stalled a request thread for 35s. sql arrives from POST /migrations/analyze, so it is attacker-controlled. The NOT VALID clause can only ever be a bounded trailing tail, so replace the regex with an index-based scan (notValidStart) that walks backward from the end of the string instead of re-scanning the attacker-controlled prefix. Verified equivalent to the old regex's behavior on all prior cases (case insensitivity, multiple internal spaces, trailing semicolons, and the existing negative cases including the trailing-comment non-match). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d5aa961 commit 68f2452

2 files changed

Lines changed: 59 additions & 3 deletions

File tree

backend/src/main/java/com/dbaagent/service/migration/DdlStatementParser.java

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
@Component
1616
public class DdlStatementParser {
1717

18-
private static final Pattern NOT_VALID = Pattern.compile("(?i)\\s+NOT\\s+VALID\\s*;?\\s*$");
1918
private static final Pattern CONCURRENTLY =
2019
Pattern.compile("(?i)\\bCREATE\\s+(UNIQUE\\s+)?INDEX\\s+CONCURRENTLY\\b");
2120
private static final Pattern DEFAULT_FN =
@@ -28,8 +27,9 @@ public class DdlStatementParser {
2827
public Optional<DdlFacts> parse(String sql) {
2928
if (sql == null || sql.isBlank()) return Optional.empty();
3029

31-
boolean notValid = NOT_VALID.matcher(sql).find();
32-
String normalized = notValid ? NOT_VALID.matcher(sql).replaceAll("") : sql;
30+
int notValidAt = notValidStart(sql);
31+
boolean notValid = notValidAt >= 0;
32+
String normalized = notValid ? sql.substring(0, notValidAt) : sql;
3333

3434
boolean concurrently = CONCURRENTLY.matcher(normalized).find();
3535
if (concurrently) {
@@ -145,4 +145,32 @@ private String referencedTable(String sql) {
145145
private String strip(String s) {
146146
return s == null ? null : s.replace("\"", "");
147147
}
148+
149+
private static int trimEnd(String s) {
150+
int e = s.length();
151+
while (e > 0) {
152+
char c = s.charAt(e - 1);
153+
if (c == ';' || Character.isWhitespace(c)) e--;
154+
else break;
155+
}
156+
return e;
157+
}
158+
159+
// A regex tail-match here (\s+NOT\s+VALID...$ via find()) is quadratic on attacker-
160+
// controlled input: CodeQL flagged it, and it measured at 35s for 80KB of input.
161+
// The clause can only ever be a bounded tail, so scan indices instead of the whole string.
162+
private static int notValidStart(String s) {
163+
int e = trimEnd(s);
164+
int p = e - 5;
165+
if (p < 0 || !s.regionMatches(true, p, "VALID", 0, 5)) return -1;
166+
int q = p;
167+
while (q > 0 && Character.isWhitespace(s.charAt(q - 1))) q--;
168+
if (q == p) return -1;
169+
int r = q - 3;
170+
if (r < 0 || !s.regionMatches(true, r, "NOT", 0, 3)) return -1;
171+
int t = r;
172+
while (t > 0 && Character.isWhitespace(s.charAt(t - 1))) t--;
173+
if (t == r) return -1;
174+
return t;
175+
}
148176
}

backend/src/test/java/com/dbaagent/service/migration/DdlStatementParserTest.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package com.dbaagent.service.migration;
22

33
import org.junit.jupiter.api.Test;
4+
import java.time.Duration;
45
import static org.assertj.core.api.Assertions.assertThat;
6+
import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
57

68
class DdlStatementParserTest {
79

@@ -163,4 +165,30 @@ void dropColumnStillClassifiesCorrectly_afterDropConstraintFix() {
163165
void dropNotNull_returnsEmptySoCallerFailsClosed() {
164166
assertThat(parser.parse("ALTER TABLE orders ALTER COLUMN a DROP NOT NULL")).isEmpty();
165167
}
168+
169+
// NOT VALID used to be detected with a regex whose \s+ tail was quadratic on attacker-
170+
// controlled input (CodeQL alerts 152/153) — 80KB of padding stalled parse() for 35s.
171+
// The index-based scan must stay linear regardless of how much whitespace precedes it.
172+
@Test
173+
void notValidDetection_staysFastOnAdversarialWhitespace() {
174+
String sql = "ALTER TABLE orders ADD CONSTRAINT ck CHECK (id > 0) NOT VALID"
175+
+ " ".repeat(50_000);
176+
assertTimeoutPreemptively(Duration.ofSeconds(2), () -> {
177+
var f = parser.parse(sql).orElseThrow();
178+
assertThat(f.notValid()).isTrue();
179+
});
180+
}
181+
182+
@Test
183+
void notValidWithMultipleInternalSpaces_stillDetected() {
184+
var f = parser.parse("ALTER TABLE orders ADD CONSTRAINT ck CHECK (id > 0) NOT VALID").orElseThrow();
185+
assertThat(f.notValid()).isTrue();
186+
}
187+
188+
@Test
189+
void notValidWithTrailingSemicolon_stillParsesAndDetected() {
190+
var f = parser.parse("ALTER TABLE orders ADD CONSTRAINT ck CHECK (id > 0) NOT VALID;").orElseThrow();
191+
assertThat(f.operation()).isEqualTo(DdlOperation.ADD_CHECK);
192+
assertThat(f.notValid()).isTrue();
193+
}
166194
}

0 commit comments

Comments
 (0)