fix: guard regex matching against catastrophic backtracking (ReDoS)#2012
Conversation
Add SafeRegex utility that wraps input in a TimeoutCharSequence to enforce a wall-clock deadline during regex matching, preventing denial-of-service via patterns like (a+)+b. Fixed locations: - Less: search and display-filter pattern matching - Nano: regex search mode - grep: removed unsafe .* wrapping, use find() instead of matches() - DefaultHistory: HISTORY_IGNORE glob patterns now properly quoted - Commands: history -m glob pattern now uses SafeRegex.compileGlob() - LineReaderImpl: Ctrl-R/Ctrl-S search and history suggestion matching - AttributedString.styleMatches: search highlight rendering
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesSafeRegex ReDoS Protection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)builtins/src/main/java/org/jline/builtins/Nano.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Refactor compileGlob to use a while loop instead of modifying the for-loop counter inside the body (S127) - Remove unused constructor parameter by consolidating TimeoutCharSequence to a single constructor taking a pre-computed deadline (S1172)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java (1)
4406-4416: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap
matcher.matches()inmatchPreviousCommand()withRegexTimeoutExceptionhandling.
SafeRegex.matcher(...)can throwRegexTimeoutException, and this redisplay path should treat timeouts as a miss instead of bubbling out ofgetDisplayedBufferWithPrompts().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java` around lines 4406 - 4416, Handle RegexTimeoutException in matchPreviousCommand() by wrapping the SafeRegex.matcher(...)/matcher.matches() check so a regex timeout is treated as a non-match instead of escaping into getDisplayedBufferWithPrompts(). Update the logic around the History.Entry iteration in LineReaderImpl to catch the timeout, skip that candidate, and continue searching just like a miss.
🧹 Nitpick comments (2)
terminal/src/main/java/org/jline/utils/SafeRegex.java (1)
119-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid mutating the
forloop counter inside the body.Sonar flags Line 124. A
whileloop keeps the escape-consumption logic explicit without changing behavior.♻️ Proposed refactor
- for (int i = 0; i < globPattern.length(); i++) { - char ch = globPattern.charAt(i); - if (ch == '\\' && i + 1 < globPattern.length()) { - appendQuoted(sb, globPattern.charAt(++i)); + int i = 0; + while (i < globPattern.length()) { + char ch = globPattern.charAt(i++); + if (ch == '\\' && i < globPattern.length()) { + appendQuoted(sb, globPattern.charAt(i++)); } else if (ch == '*') { sb.append(".*"); } else { appendQuoted(sb, ch); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@terminal/src/main/java/org/jline/utils/SafeRegex.java` around lines 119 - 131, Refactor SafeRegex.compileGlob to avoid incrementing the for-loop index inside the loop body when consuming an escape sequence. Replace the current for-loop with an explicit while-loop (or equivalent) so the backslash handling remains clear and behavior stays unchanged, keeping the logic in compileGlob and appendQuoted intact.Source: Linters/SAST tools
builtins/src/main/java/org/jline/builtins/Nano.java (1)
1344-1346: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse matcher indices for
matchedLength.m.end() - m.start()gives the same span length without materializinggroup(0).♻️ Proposed refactor
- matchedLength = m.group(0).length(); + matchedLength = m.end() - m.start();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@builtins/src/main/java/org/jline/builtins/Nano.java` around lines 1344 - 1346, Update the matching logic in Nano’s regex handling so matchedLength is derived from the matcher positions instead of calling m.group(0).length(). In the while (m.find()) loop, use m.end() - m.start() for matchedLength, keeping the existing res.add(m.start()) behavior unchanged and preserving the surrounding matcher flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@terminal/src/main/java/org/jline/utils/SafeRegex.java`:
- Around line 31-32: Update the Javadoc in SafeRegex, especially the
compileGlob() reference and the related documentation around
matcher/matches/find, to clearly state that compileGlob() only builds a safe
Pattern and does not itself enforce timeout protection. Mention that callers
must use SafeRegex.matcher/matches/find on the returned Pattern to get the
protection, and adjust the existing glob-style guidance accordingly so it cannot
be misread as safe to match directly.
- Around line 52-66: `LineReaderImpl.matchPreviousCommand` still uses
`SafeRegex.matcher(...)` and calls `matches()` without handling
`RegexTimeoutException`, so update that history-suggestion matching path to
catch the timeout and degrade safely. Mirror the existing handling used at the
other `SafeRegex.matcher` call sites in `LineReaderImpl`, either by wrapping the
match in a try/catch for `RegexTimeoutException` or by treating a timeout as no
suggestion so the interactive search does not fail with an uncaught runtime
exception.
- Around line 155-175: The timeout in SafeRegex.TimeoutCharSequence is currently
started in the constructor, so a Matcher created early can expire before
matching begins; move deadline initialization into the first charAt() access and
keep that deadline shared through subSequence() by reusing the same
TimeoutCharSequence state. Update the TimeoutCharSequence constructors and
charAt() logic so the deadline is lazily set on first use rather than at
construction time.
---
Outside diff comments:
In `@reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java`:
- Around line 4406-4416: Handle RegexTimeoutException in matchPreviousCommand()
by wrapping the SafeRegex.matcher(...)/matcher.matches() check so a regex
timeout is treated as a non-match instead of escaping into
getDisplayedBufferWithPrompts(). Update the logic around the History.Entry
iteration in LineReaderImpl to catch the timeout, skip that candidate, and
continue searching just like a miss.
---
Nitpick comments:
In `@builtins/src/main/java/org/jline/builtins/Nano.java`:
- Around line 1344-1346: Update the matching logic in Nano’s regex handling so
matchedLength is derived from the matcher positions instead of calling
m.group(0).length(). In the while (m.find()) loop, use m.end() - m.start() for
matchedLength, keeping the existing res.add(m.start()) behavior unchanged and
preserving the surrounding matcher flow.
In `@terminal/src/main/java/org/jline/utils/SafeRegex.java`:
- Around line 119-131: Refactor SafeRegex.compileGlob to avoid incrementing the
for-loop index inside the loop body when consuming an escape sequence. Replace
the current for-loop with an explicit while-loop (or equivalent) so the
backslash handling remains clear and behavior stays unchanged, keeping the logic
in compileGlob and appendQuoted intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bf96d9f-8623-44d9-a625-03414e05ef7d
📒 Files selected for processing (9)
builtins/src/main/java/org/jline/builtins/Commands.javabuiltins/src/main/java/org/jline/builtins/Less.javabuiltins/src/main/java/org/jline/builtins/Nano.javabuiltins/src/main/java/org/jline/builtins/PosixCommands.javareader/src/main/java/org/jline/reader/impl/LineReaderImpl.javareader/src/main/java/org/jline/reader/impl/history/DefaultHistory.javaterminal/src/main/java/org/jline/utils/AttributedString.javaterminal/src/main/java/org/jline/utils/RegexTimeoutException.javaterminal/src/main/java/org/jline/utils/SafeRegex.java
- Clarify compileGlob Javadoc: it only builds the pattern, timeout protection requires using SafeRegex.matcher/matches/find - Lazy deadline in TimeoutCharSequence: start the timeout on first charAt() call, not at Matcher creation time, so the window is not consumed before matching begins; share deadline across subSequence() - Catch RegexTimeoutException in matchPreviousCommand() so a timeout degrades to no-suggestion instead of crashing the display loop - Use m.end() - m.start() in Nano doSearch instead of m.group(0).length() to avoid an unnecessary String allocation
|
Addressed all CodeRabbit findings in 20aec53: Inline comments (replied above):
Outside-diff comment:
Nitpick comments:
|
🤖 Augment PR SummarySummary: Hardens user-controlled regex usage across JLine to mitigate ReDoS (catastrophic backtracking) without introducing RE2/J.
|
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| /** | ||
| * Create a {@link Matcher} that will throw {@link RegexTimeoutException} | ||
| * if matching exceeds the given timeout. The timeout starts on the | ||
| * first {@code charAt} call (i.e. when matching actually begins), not |
There was a problem hiding this comment.
terminal/.../SafeRegex.java:65-67 The Javadoc says the timeout starts on the first charAt call, but TimeoutCharSequence only initializes the deadline when calls % CHECK_INTERVAL == 0, so the effective start can be delayed by up to CHECK_INTERVAL calls.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Good catch — the Javadoc was imprecise. The deadline is lazily initialized on the first check (every 1024 charAt calls), not on the very first charAt call. Fixed the wording in 2b7deac to say "first deadline check during matching" instead.
There was a problem hiding this comment.
Good catch — the Javadoc was imprecise. The deadline is lazily initialized on the first check (every 1024 charAt calls), not on the very first charAt call. Fixed the wording in 2b7deac to say "first deadline check during matching" instead.
| matchedLength = m.end() - m.start(); | ||
| } | ||
| } catch (RegexTimeoutException e) { | ||
| // Return whatever matches were found before timeout |
There was a problem hiding this comment.
builtins/.../Nano.java:1344-1351 If a RegexTimeoutException happens before any successful match, matchedLength can remain from a previous search even though res may be empty/partial, which can desync highlighting/selection state.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Valid point. doSearch() now resets matchedLength = 0 before the find loop (2b7deac), so a timeout before any successful match leaves a clean state. highlightEnd() checks matchedLength > 0, so a zero value correctly prevents stale highlighting.
There was a problem hiding this comment.
Valid point. doSearch() now resets matchedLength = 0 before the find loop (2b7deac), so a timeout before any successful match leaves a clean state. highlightEnd() checks matchedLength > 0, so a zero value correctly prevents stale highlighting.
- SafeRegex.matcher() Javadoc now accurately says the timeout starts on the first deadline check, not the first charAt call. - Nano.doSearch() resets matchedLength before the find loop so a RegexTimeoutException before any match doesn't leave a stale value that could desync highlighting.
|
Backport of #2012 to jline-3.x. Add SafeRegex utility that wraps regex input in a TimeoutCharSequence to enforce a wall-clock deadline during matching. Java's backtracking NFA engine can take exponential time on patterns with nested quantifiers (e.g. (a+)+b); the wrapper checks System.nanoTime() every 1024 charAt() calls and throws RegexTimeoutException if the deadline is exceeded. Fixed 8 locations where user-controlled input was matched as a regex without protection: Less, Nano, grep, history -m, HISTORY_IGNORE, Ctrl-R/Ctrl-S search, matchPreviousCommand, and styleMatches. Addresses GHSA-r2xf-8xr9-62gw, GHSA-2v9w-34q6-wpqx, GHSA-ph9c-7hw9-vhhw, GHSA-5q95-hrpc-m3w3. Closes #2018
1 participant
Summary
SafeRegexutility class in theterminalmodule that wraps regex input in aTimeoutCharSequenceto enforce a wall-clock deadline during matching, preventing denial-of-service via patterns with catastrophic backtracking (e.g.(a+)+b).terminal,reader, andbuiltinsmodules where user-controlled input was compiled/matched as a Java regex without validation..*pattern wrapping in the built-ingrepcommand, usingMatcher.find()instead.HISTORY_IGNOREglob patterns and the history-mflag.matchPreviousCommand()withPattern.quote().Addresses: GHSA-r2xf-8xr9-62gw, GHSA-2v9w-34q6-wpqx, GHSA-ph9c-7hw9-vhhw, GHSA-5q95-hrpc-m3w3 and additional unreported locations.
Approach
Rather than adding a dependency on RE2/J, the fix uses a
TimeoutCharSequencewrapper that checksSystem.nanoTime()every 1024charAt()calls during regex matching. When the JDK regex engine backtracks catastrophically, the call count explodes and the deadline is hit within milliseconds. No extra threads or thread interruption needed.Fixed locations
AttributedString.styleMatches()Less.javaNano.javaPosixCommands.java(grep).*wrapping amplified backtrackingCommands.java(history -m)DefaultHistory.matchPatterns()LineReaderImpl.matches()LineReaderImpl.matchPreviousCommand()Test plan
./mvx mvn install -DskipTests— full build passes./mvx mvn test -pl terminal— 372 tests pass./mvx mvn test -pl reader— 210 tests pass./mvx mvn test -pl builtins— 1026 tests pass (23 pre-existing SwingTerminal/Fontconfig errors in headless env)Summary by CodeRabbit
New Features
*) handling in relevant commands.Bug Fixes