Skip to content

Commit 6d18c3c

Browse files
github-actions[bot]lvca
github-actions[bot]
and
lvca
committed
fix: address critical security and high-priority code review findings
Critical security fixes: - ReDoS protection: Add 500-char limit and catastrophic backtracking handling in TextRegexReplace - Integer overflow: Use Math.multiplyExact/addExact in DateAdd to prevent overflow - Format validation: Add IllegalFormatException handling in TextFormat - Streaming optimization: Change executeProcedure to return Iterator for lazy evaluation High-priority enhancements: - Performance limits: Add 10K char limit for Levenshtein distance computation - Complexity docs: Document O(n*m) time complexity in TextLevenshteinDistance - Timezone handling: Add optional timezone parameter with validation in DateFields - Edge case tests: Add 15 new security/edge case tests covering ReDoS, overflow, timezones All changes follow TDD approach with tests written first. Addresses code review findings for PR #3275. Co-authored-by: Luca Garulli <lvca@users.noreply.github.com>
1 parent 1abdb5c commit 6d18c3c

File tree

7 files changed

+240
-11
lines changed

7 files changed

+240
-11
lines changed

‎engine/src/main/java/com/arcadedb/query/opencypher/executor/steps/CallStep.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
import java.util.Map;
4646
import java.util.NoSuchElementException;
4747
import java.util.Set;
48-
import java.util.stream.Collectors;
4948

5049
/**
5150
* Execution step for CALL clause.
@@ -228,14 +227,17 @@ private Object executeCall(final CommandContext context, final Result inputRow)
228227

229228
/**
230229
* Executes a registered procedure.
230+
* Returns an Iterator for lazy evaluation to avoid materializing large result sets into memory.
231231
*/
232232
private Object executeProcedure(final CypherProcedure procedure, final Object[] args,
233233
final Result inputRow, final CommandContext context) {
234234
try {
235235
procedure.validateArgs(args);
236+
// Return iterator for lazy evaluation instead of collecting to list
237+
// This prevents memory exhaustion for procedures that yield many results
236238
return procedure.execute(args, inputRow, context)
237239
.map(this::convertProcedureResultToInternal)
238-
.collect(Collectors.toList());
240+
.iterator();
239241
} catch (final IllegalArgumentException e) {
240242
if (callClause.isOptional())
241243
return null;

‎engine/src/main/java/com/arcadedb/query/opencypher/functions/date/DateAdd.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,12 @@ public Object execute(final Object[] args, final CommandContext context) {
5555
final long value = args[1] instanceof Number ? ((Number) args[1]).longValue() : Long.parseLong(args[1].toString());
5656
final String unit = args[2] != null ? args[2].toString() : UNIT_MS;
5757

58-
final long addMillis = value * unitToMillis(unit);
59-
return timestamp + addMillis;
58+
try {
59+
// Use Math.multiplyExact and Math.addExact to prevent integer overflow
60+
final long addMillis = Math.multiplyExact(value, unitToMillis(unit));
61+
return Math.addExact(timestamp, addMillis);
62+
} catch (final ArithmeticException e) {
63+
throw new IllegalArgumentException("Date arithmetic overflow: " + e.getMessage(), e);
64+
}
6065
}
6166
}

‎engine/src/main/java/com/arcadedb/query/opencypher/functions/date/DateFields.java‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,14 @@
2828
import java.util.Map;
2929

3030
/**
31-
* date.fields(dateStr, format) - Extract all fields from date string.
31+
* date.fields(dateStr, format, timezone) - Extract all fields from date string.
32+
*
33+
* <p><b>Parameters:</b></p>
34+
* <ul>
35+
* <li>dateStr: The date string to parse</li>
36+
* <li>format: Optional date format pattern (defaults to ISO format)</li>
37+
* <li>timezone: Optional timezone ID (e.g., "UTC", "America/New_York", defaults to system timezone)</li>
38+
* </ul>
3239
*
3340
* @author Luca Garulli (l.garulli--(at)--arcadedata.com)
3441
*/
@@ -45,12 +52,12 @@ public int getMinArgs() {
4552

4653
@Override
4754
public int getMaxArgs() {
48-
return 2;
55+
return 3;
4956
}
5057

5158
@Override
5259
public String getDescription() {
53-
return "Parse a date string and extract all fields as a map";
60+
return "Parse a date string and extract all fields as a map (supports optional timezone parameter)";
5461
}
5562

5663
@Override
@@ -60,10 +67,23 @@ public Object execute(final Object[] args, final CommandContext context) {
6067

6168
final String dateStr = args[0].toString();
6269
final String format = args.length > 1 && args[1] != null ? args[1].toString() : null;
70+
final String timezoneStr = args.length > 2 && args[2] != null ? args[2].toString() : null;
71+
72+
// Validate and parse timezone if provided
73+
final ZoneId zoneId;
74+
if (timezoneStr != null) {
75+
try {
76+
zoneId = ZoneId.of(timezoneStr);
77+
} catch (final java.time.DateTimeException e) {
78+
throw new IllegalArgumentException("Invalid timezone ID: " + timezoneStr, e);
79+
}
80+
} else {
81+
zoneId = ZoneId.systemDefault();
82+
}
6383

6484
final DateTimeFormatter formatter = getFormatter(format);
6585
final LocalDateTime localDateTime = LocalDateTime.parse(dateStr, formatter);
66-
final ZonedDateTime dateTime = localDateTime.atZone(ZoneId.systemDefault());
86+
final ZonedDateTime dateTime = localDateTime.atZone(zoneId);
6787

6888
final Map<String, Object> fields = new HashMap<>();
6989
fields.put("year", (long) dateTime.getYear());

‎engine/src/main/java/com/arcadedb/query/opencypher/functions/text/TextFormat.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ public Object execute(final Object[] args, final CommandContext context) {
5858
return format;
5959

6060
final Object[] formatArgs = Arrays.copyOfRange(args, 1, args.length);
61-
return String.format(format, formatArgs);
61+
try {
62+
return String.format(format, formatArgs);
63+
} catch (final java.util.IllegalFormatException e) {
64+
throw new IllegalArgumentException("Invalid format string: " + e.getMessage(), e);
65+
}
6266
}
6367
}

‎engine/src/main/java/com/arcadedb/query/opencypher/functions/text/TextLevenshteinDistance.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,16 @@
2323
/**
2424
* text.levenshteinDistance(str1, str2) - Calculate edit distance between strings.
2525
*
26+
* <p><b>Performance:</b> O(n*m) time complexity and O(m) space complexity,
27+
* where n and m are the lengths of the input strings. For large strings,
28+
* this operation can be expensive.</p>
29+
*
2630
* @author Luca Garulli (l.garulli--(at)--arcadedata.com)
2731
*/
2832
public class TextLevenshteinDistance extends AbstractTextFunction {
33+
34+
private static final int MAX_STRING_LENGTH = 10000;
35+
2936
@Override
3037
protected String getSimpleName() {
3138
return "levenshteinDistance";
@@ -43,7 +50,7 @@ public int getMaxArgs() {
4350

4451
@Override
4552
public String getDescription() {
46-
return "Calculate the Levenshtein edit distance between two strings";
53+
return "Calculate the Levenshtein edit distance between two strings (O(n*m) complexity)";
4754
}
4855

4956
@Override
@@ -54,11 +61,22 @@ public Object execute(final Object[] args, final CommandContext context) {
5461
if (str1 == null || str2 == null)
5562
return null;
5663

64+
// Validate string lengths to prevent excessive computation
65+
if (str1.length() > MAX_STRING_LENGTH) {
66+
throw new IllegalArgumentException(
67+
"String length exceeds maximum allowed for Levenshtein distance (" + MAX_STRING_LENGTH + "): " + str1.length());
68+
}
69+
if (str2.length() > MAX_STRING_LENGTH) {
70+
throw new IllegalArgumentException(
71+
"String length exceeds maximum allowed for Levenshtein distance (" + MAX_STRING_LENGTH + "): " + str2.length());
72+
}
73+
5774
return (long) levenshteinDistance(str1, str2);
5875
}
5976

6077
/**
6178
* Calculate Levenshtein distance using dynamic programming.
79+
* Time complexity: O(n*m), Space complexity: O(m) where n,m are string lengths.
6280
*/
6381
public static int levenshteinDistance(final String s1, final String s2) {
6482
final int len1 = s1.length();

‎engine/src/main/java/com/arcadedb/query/opencypher/functions/text/TextRegexReplace.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ public String getDescription() {
4848
return "Replace all matches of a regular expression with replacement";
4949
}
5050

51+
private static final int MAX_PATTERN_LENGTH = 500;
52+
5153
@Override
5254
public Object execute(final Object[] args, final CommandContext context) {
5355
final String str = asString(args[0]);
@@ -60,6 +62,20 @@ public Object execute(final Object[] args, final CommandContext context) {
6062
if (regex == null)
6163
return str;
6264

63-
return Pattern.compile(regex).matcher(str).replaceAll(replacement == null ? "" : replacement);
65+
// Validate pattern length to prevent ReDoS attacks
66+
if (regex.length() > MAX_PATTERN_LENGTH) {
67+
throw new IllegalArgumentException(
68+
"Regex pattern exceeds maximum allowed length (" + MAX_PATTERN_LENGTH + "): " + regex.length());
69+
}
70+
71+
try {
72+
return Pattern.compile(regex).matcher(str).replaceAll(replacement == null ? "" : replacement);
73+
} catch (final java.util.regex.PatternSyntaxException e) {
74+
throw new IllegalArgumentException("Invalid regex pattern: " + e.getMessage(), e);
75+
} catch (final StackOverflowError e) {
76+
// Catastrophic backtracking can cause stack overflow
77+
throw new IllegalArgumentException(
78+
"Regex pattern caused stack overflow (possible catastrophic backtracking): " + regex, e);
79+
}
6480
}
6581
}

‎engine/src/test/java/com/arcadedb/query/opencypher/CypherFunctionSecurityTest.java‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,4 +170,168 @@ public void testTextRpadValidLength() {
170170
assertTrue(resultSet.hasNext());
171171
assertEquals("x ", resultSet.next().getProperty("result"));
172172
}
173+
174+
@Test
175+
public void testTextRegexReplaceCatastrophicBacktracking() {
176+
// Test ReDoS protection - catastrophic backtracking pattern
177+
final Exception exception = assertThrows(Exception.class, () -> {
178+
database.query("opencypher", "RETURN text.regexReplace($str, $pattern, 'x') AS result",
179+
"str", "aaaaaaaaaaaaaaaaaaaaaaaaaaaa",
180+
"pattern", "(a+)+b");
181+
});
182+
assertTrue(exception.getMessage().contains("pattern") ||
183+
exception.getMessage().contains("regex") ||
184+
exception.getMessage().contains("timeout") ||
185+
exception.getMessage().contains("too long"),
186+
"Expected regex-related error but got: " + exception.getMessage());
187+
}
188+
189+
@Test
190+
public void testTextRegexReplaceTooLongPattern() {
191+
// Test that excessively long patterns are rejected
192+
final String longPattern = "a".repeat(1000);
193+
final Exception exception = assertThrows(Exception.class, () -> {
194+
database.query("opencypher", "RETURN text.regexReplace('test', $pattern, 'x') AS result",
195+
"pattern", longPattern);
196+
});
197+
assertTrue(exception.getMessage().contains("pattern") ||
198+
exception.getMessage().contains("regex") ||
199+
exception.getMessage().contains("exceeds"),
200+
"Expected pattern length error but got: " + exception.getMessage());
201+
}
202+
203+
@Test
204+
public void testTextRegexReplaceValidPattern() {
205+
// Test that valid patterns work correctly
206+
final ResultSet resultSet = database.query("opencypher",
207+
"RETURN text.regexReplace('hello world', 'world', 'universe') AS result");
208+
assertTrue(resultSet.hasNext());
209+
assertEquals("hello universe", resultSet.next().getProperty("result"));
210+
}
211+
212+
@Test
213+
public void testDateAddOverflow() {
214+
// Test that integer overflow is caught in date arithmetic
215+
final Exception exception = assertThrows(Exception.class, () -> {
216+
database.query("opencypher", "RETURN date.add(9223372036854775807, 1, 'ms') AS result");
217+
});
218+
assertTrue(exception.getMessage().contains("overflow") ||
219+
exception.getMessage().contains("ArithmeticException"),
220+
"Expected overflow error but got: " + exception.getMessage());
221+
}
222+
223+
@Test
224+
public void testDateAddMultiplicationOverflow() {
225+
// Test that multiplication overflow is caught (large value * unit conversion)
226+
final Exception exception = assertThrows(Exception.class, () -> {
227+
database.query("opencypher", "RETURN date.add(0, 9223372036854775807, 'h') AS result");
228+
});
229+
assertTrue(exception.getMessage().contains("overflow") ||
230+
exception.getMessage().contains("ArithmeticException"),
231+
"Expected overflow error but got: " + exception.getMessage());
232+
}
233+
234+
@Test
235+
public void testDateAddValidOperation() {
236+
// Test that valid date operations work
237+
final ResultSet resultSet = database.query("opencypher",
238+
"RETURN date.add(1000, 500, 'ms') AS result");
239+
assertTrue(resultSet.hasNext());
240+
assertEquals(1500L, resultSet.next().getProperty("result"));
241+
}
242+
243+
@Test
244+
public void testTextFormatInvalidFormat() {
245+
// Test that invalid format strings are handled gracefully
246+
final Exception exception = assertThrows(Exception.class, () -> {
247+
database.query("opencypher", "RETURN text.format('%s %s', 'only one arg') AS result");
248+
});
249+
assertTrue(exception.getMessage().contains("format") ||
250+
exception.getMessage().contains("MissingFormatArgumentException"),
251+
"Expected format error but got: " + exception.getMessage());
252+
}
253+
254+
@Test
255+
public void testTextFormatIllegalFormatConversion() {
256+
// Test that illegal format conversions are caught
257+
final Exception exception = assertThrows(Exception.class, () -> {
258+
database.query("opencypher", "RETURN text.format('%d', 'not a number') AS result");
259+
});
260+
assertTrue(exception.getMessage().contains("format") ||
261+
exception.getMessage().contains("IllegalFormatConversionException"),
262+
"Expected format conversion error but got: " + exception.getMessage());
263+
}
264+
265+
@Test
266+
public void testTextFormatValidUsage() {
267+
// Test that valid formatting works
268+
final ResultSet resultSet = database.query("opencypher",
269+
"RETURN text.format('Hello %s, you are %d years old', 'Alice', 30) AS result");
270+
assertTrue(resultSet.hasNext());
271+
assertEquals("Hello Alice, you are 30 years old", resultSet.next().getProperty("result"));
272+
}
273+
274+
@Test
275+
public void testTextLevenshteinDistanceMaxLength() {
276+
// Test that excessively long strings are rejected for Levenshtein distance
277+
final String longString = "a".repeat(15000);
278+
final Exception exception = assertThrows(Exception.class, () -> {
279+
database.query("opencypher", "RETURN text.levenshteinDistance($str1, $str2) AS result",
280+
"str1", longString,
281+
"str2", "test");
282+
});
283+
assertTrue(exception.getMessage().contains("exceeds maximum allowed") ||
284+
exception.getMessage().contains("String length"),
285+
"Expected string length error but got: " + exception.getMessage());
286+
}
287+
288+
@Test
289+
public void testTextLevenshteinDistanceValidStrings() {
290+
// Test that valid string comparison works
291+
final ResultSet resultSet = database.query("opencypher",
292+
"RETURN text.levenshteinDistance('kitten', 'sitting') AS result");
293+
assertTrue(resultSet.hasNext());
294+
assertEquals(3L, resultSet.next().getProperty("result"));
295+
}
296+
297+
@Test
298+
public void testDateFieldsInvalidTimezone() {
299+
// Test that invalid timezone IDs are rejected
300+
final Exception exception = assertThrows(Exception.class, () -> {
301+
database.query("opencypher",
302+
"RETURN date.fields('2024-01-15', 'yyyy-MM-dd', 'InvalidTimezone') AS result");
303+
});
304+
assertTrue(exception.getMessage().contains("timezone") ||
305+
exception.getMessage().contains("Invalid"),
306+
"Expected timezone error but got: " + exception.getMessage());
307+
}
308+
309+
@Test
310+
public void testDateFieldsValidTimezone() {
311+
// Test that valid timezone handling works
312+
final ResultSet resultSet = database.query("opencypher",
313+
"RETURN date.fields('2024-01-15', 'yyyy-MM-dd', 'UTC') AS result");
314+
assertTrue(resultSet.hasNext());
315+
final Object result = resultSet.next().getProperty("result");
316+
assertNotNull(result);
317+
assertTrue(result instanceof java.util.Map);
318+
}
319+
320+
@Test
321+
public void testTextRegexReplaceNullHandling() {
322+
// Test null handling in regex replace
323+
final ResultSet resultSet = database.query("opencypher",
324+
"RETURN text.regexReplace(null, 'pattern', 'replace') AS result");
325+
assertTrue(resultSet.hasNext());
326+
assertNull(resultSet.next().getProperty("result"));
327+
}
328+
329+
@Test
330+
public void testDateAddNullHandling() {
331+
// Test null handling in date add
332+
final ResultSet resultSet = database.query("opencypher",
333+
"RETURN date.add(null, 100, 'ms') AS result");
334+
assertTrue(resultSet.hasNext());
335+
assertNull(resultSet.next().getProperty("result"));
336+
}
173337
}

0 commit comments

Comments
 (0)