paragon-review commented on code in PR #13704:
URL: https://github.com/apache/skywalking/pull/13704#discussion_r2811613034
##########
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java:
##########
@@ -859,16 +867,64 @@ private List<Endpoint>
queryEndpointTraffic(MatcherSetResult parseResult,
} else if (matcher.getMatchOp() == PromQLParser.NEQ) {
endpoints.stream().filter(e ->
!e.getName().equals(endpointName)).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.RM) {
- endpoints.stream().filter(e ->
e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.NRM) {
- endpoints.stream().filter(e ->
!e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> !safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
}
} else {
endpoints.stream().limit(limitNum).forEach(result::add);
}
return result;
}
+ private static Pattern compileRegex(final String regex) throws
IllegalExpressionException {
+ try {
+ return Pattern.compile(regex);
+ } catch (PatternSyntaxException e) {
+ throw new IllegalExpressionException("Invalid regex pattern: " +
e.getMessage(), e);
+ }
+ }
+
+ private static boolean safeMatches(final Pattern pattern, final String
input) {
+ return pattern.matcher(new TimeLimitedCharSequence(input,
REGEX_MATCH_TIMEOUT_NANOS)).matches();
+ }
+
Review Comment:
<!-- paragon-review-issue:ac89b405-ac73-490a-9c8c-35a34804ce07-->
### Bug: Regex timeout throws bare RuntimeException, bypassing catch blocks
<!-- **HIGH Severity** -->
<!-- DESCRIPTION START -->
Regex timeout throws bare RuntimeException, bypassing catch blocks. Callers
only catch IllegalExpressionException, so timeouts surface as HTTP 500. Wrap
RuntimeException inside safeMatches as IllegalExpressionException.
<!-- DESCRIPTION END -->
<details>
<summary>View Details</summary>
**Location:**
`oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java`
(lines 893-895)
### Analysis
**Regex timeout throws bare RuntimeException, bypassing catch blocks**
| | |
|---|---|
| **What fails** | When a ReDoS pattern triggers the 5s timeout,
TimeLimitedCharSequence.charAt throws RuntimeException which is not caught by
the IllegalExpressionException catch blocks at lines 342 and 403. |
| **Result** | RuntimeException propagates uncaught past the
IllegalExpressionException catch blocks, resulting in HTTP 500 with a stack
trace leaked to the client. |
| **Expected** | The timeout should be caught and wrapped as
IllegalExpressionException so it returns a proper error response with
ResultStatus.ERROR and a user-friendly message. |
| **Impact** | The entire ReDoS protection goal of this PR is undermined —
attackers still get a 5s thread hang and an ugly 500 error instead of a
graceful rejection. |
<details>
<summary>How to reproduce</summary>
```bash
Send a PromQL query with a catastrophic backtracking regex pattern, e.g.
service_name=~"(a+)+$" against a service list. Wait 5 seconds for the timeout
to trigger.
```
</details>
<details>
<summary>Patch Details</summary>
```diff
- private static boolean safeMatches(final Pattern pattern, final String
input) {
- return pattern.matcher(new TimeLimitedCharSequence(input,
REGEX_MATCH_TIMEOUT_NANOS)).matches();
+ private static boolean safeMatches(final Pattern pattern, final String
input) throws IllegalExpressionException {
+ try {
+ return pattern.matcher(new TimeLimitedCharSequence(input,
REGEX_MATCH_TIMEOUT_NANOS)).matches();
+ } catch (RuntimeException e) {
+ throw new IllegalExpressionException("Regex evaluation timed
out", e);
+ }
```
</details>
<details>
<summary>AI Fix Prompt</summary>
```
Fix this issue: Regex timeout throws bare RuntimeException, bypassing catch
blocks. Callers only catch IllegalExpressionException, so timeouts surface as
HTTP 500. Wrap RuntimeException inside safeMatches as
IllegalExpressionException.
Location:
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java
(lines 893-895)
Problem: When a ReDoS pattern triggers the 5s timeout,
TimeLimitedCharSequence.charAt throws RuntimeException which is not caught by
the IllegalExpressionException catch blocks at lines 342 and 403.
Current behavior: RuntimeException propagates uncaught past the
IllegalExpressionException catch blocks, resulting in HTTP 500 with a stack
trace leaked to the client.
Expected: The timeout should be caught and wrapped as
IllegalExpressionException so it returns a proper error response with
ResultStatus.ERROR and a user-friendly message.
Steps to reproduce: Send a PromQL query with a catastrophic backtracking
regex pattern, e.g. service_name=~"(a+)+$" against a service list. Wait 5
seconds for the timeout to trigger.
Provide a code fix.
```
</details>
</details>
---
<sub>**Tip:** Reply with `@paragon-run` to automatically fix this issue</sub>
##########
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java:
##########
@@ -859,16 +867,64 @@ private List<Endpoint>
queryEndpointTraffic(MatcherSetResult parseResult,
} else if (matcher.getMatchOp() == PromQLParser.NEQ) {
endpoints.stream().filter(e ->
!e.getName().equals(endpointName)).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.RM) {
- endpoints.stream().filter(e ->
e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.NRM) {
- endpoints.stream().filter(e ->
!e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> !safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
}
} else {
endpoints.stream().limit(limitNum).forEach(result::add);
}
return result;
}
+ private static Pattern compileRegex(final String regex) throws
IllegalExpressionException {
+ try {
+ return Pattern.compile(regex);
+ } catch (PatternSyntaxException e) {
+ throw new IllegalExpressionException("Invalid regex pattern: " +
e.getMessage(), e);
+ }
+ }
+
+ private static boolean safeMatches(final Pattern pattern, final String
input) {
+ return pattern.matcher(new TimeLimitedCharSequence(input,
REGEX_MATCH_TIMEOUT_NANOS)).matches();
+ }
+
+ private static class TimeLimitedCharSequence implements CharSequence {
+ private final CharSequence delegate;
+ private final long deadlineNanos;
+
+ private TimeLimitedCharSequence(final CharSequence delegate, final
long timeoutNanos) {
+ this.delegate = delegate;
+ this.deadlineNanos = System.nanoTime() + timeoutNanos;
+ }
+
+ @Override
+ public int length() {
+ return delegate.length();
+ }
+
+ @Override
+ public char charAt(final int index) {
+ if (System.nanoTime() > deadlineNanos) {
+ throw new RuntimeException("Regex matching timed out");
+ }
+ return delegate.charAt(index);
+ }
+
+ @Override
+ public CharSequence subSequence(final int start, final int end) {
Review Comment:
<!-- paragon-review-issue:120ceb17-7abb-46cd-a503-157c78cb728d-->
### Bug: subSequence recomputes deadline from remaining time plus a new
nanoTime call
<!-- **LOW Severity** -->
<!-- DESCRIPTION START -->
subSequence recomputes deadline from remaining time plus a new nanoTime
call. This drifts the effective deadline forward past the original 5s. Pass the
absolute deadlineNanos directly to the child instance.
<!-- DESCRIPTION END -->
<details>
<summary>View Details</summary>
**Location:**
`oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java`
(lines 917-919)
### Analysis
**subSequence recomputes deadline from remaining time plus a new nanoTime
call**
| | |
|---|---|
| **What fails** | TimeLimitedCharSequence.subSequence computes remaining
time then the constructor adds System.nanoTime() again, extending the effective
deadline beyond the intended 5 seconds on each call. |
| **Result** | The timeout window silently extends beyond 5 seconds across
repeated subSequence invocations due to elapsed time between the two
System.nanoTime() calls. |
| **Expected** | The absolute deadline should be preserved across all child
TimeLimitedCharSequence instances without recalculation. |
| **Impact** | The ReDoS timeout protection is weakened — patterns that
trigger many subSequence calls can exceed the 5-second budget. |
<details>
<summary>How to reproduce</summary>
```bash
Use a regex that triggers repeated subSequence calls internally (e.g.,
alternation groups). The cumulative drift from each nanoTime() gap extends the
effective timeout.
```
</details>
<details>
<summary>AI Fix Prompt</summary>
```
Fix this issue: subSequence recomputes deadline from remaining time plus a
new nanoTime call. This drifts the effective deadline forward past the original
5s. Pass the absolute deadlineNanos directly to the child instance.
Location:
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java
(lines 917-919)
Problem: TimeLimitedCharSequence.subSequence computes remaining time then
the constructor adds System.nanoTime() again, extending the effective deadline
beyond the intended 5 seconds on each call.
Current behavior: The timeout window silently extends beyond 5 seconds
across repeated subSequence invocations due to elapsed time between the two
System.nanoTime() calls.
Expected: The absolute deadline should be preserved across all child
TimeLimitedCharSequence instances without recalculation.
Steps to reproduce: Use a regex that triggers repeated subSequence calls
internally (e.g., alternation groups). The cumulative drift from each
nanoTime() gap extends the effective timeout.
Provide a code fix.
```
</details>
</details>
---
<sub>**Tip:** Reply with `@paragon-run` to automatically fix this issue</sub>
##########
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java:
##########
@@ -859,16 +867,64 @@ private List<Endpoint>
queryEndpointTraffic(MatcherSetResult parseResult,
} else if (matcher.getMatchOp() == PromQLParser.NEQ) {
endpoints.stream().filter(e ->
!e.getName().equals(endpointName)).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.RM) {
- endpoints.stream().filter(e ->
e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.NRM) {
- endpoints.stream().filter(e ->
!e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> !safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
}
} else {
endpoints.stream().limit(limitNum).forEach(result::add);
}
return result;
}
+ private static Pattern compileRegex(final String regex) throws
IllegalExpressionException {
Review Comment:
<!-- paragon-review-issue:4a25a634-59ad-4933-a402-4c7dd77d2987-->
### Security: No tests exist for the ReDoS-protection code paths
<!-- **MEDIUM Severity** -->
<!-- DESCRIPTION START -->
No tests exist for the ReDoS-protection code paths. Security-critical code
lacks validation. Add tests for compileRegex, safeMatches, and
TimeLimitedCharSequence.
<!-- DESCRIPTION END -->
<details>
<summary>View Details</summary>
**Location:**
`oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java`
(lines 882-930)
### Analysis
**No tests exist for the ReDoS-protection code paths. Security-critical code
lacks validation**
| | |
|---|---|
| **What fails** | There are zero tests covering TimeLimitedCharSequence,
compileRegex, or safeMatches — all newly added security-critical code for ReDoS
protection. |
| **Result** | No test coverage for the ReDoS protection mechanism, meaning
regressions or bugs (like finding #1) go undetected. |
| **Expected** | Tests should cover: invalid regex →
IllegalExpressionException, catastrophic backtracking → timeout within ~5s,
normal pattern → correct match/no-match, subSequence preserving timeout. |
| **Impact** | Without tests, the ReDoS protection can silently break.
Combined with finding #1 (uncaught RuntimeException), this means the core
security improvement of this PR is unvalidated. |
<details>
<summary>How to reproduce</summary>
```bash
Search the test directory for any test referencing TimeLimitedCharSequence,
compileRegex, or safeMatches. None exist.
```
</details>
<details>
<summary>AI Fix Prompt</summary>
```
Fix this issue: No tests exist for the ReDoS-protection code paths.
Security-critical code lacks validation. Add tests for compileRegex,
safeMatches, and TimeLimitedCharSequence.
Location:
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java
(lines 882-930)
Problem: There are zero tests covering TimeLimitedCharSequence,
compileRegex, or safeMatches — all newly added security-critical code for ReDoS
protection.
Current behavior: No test coverage for the ReDoS protection mechanism,
meaning regressions or bugs (like finding #1) go undetected.
Expected: Tests should cover: invalid regex → IllegalExpressionException,
catastrophic backtracking → timeout within ~5s, normal pattern → correct
match/no-match, subSequence preserving timeout.
Steps to reproduce: Search the test directory for any test referencing
TimeLimitedCharSequence, compileRegex, or safeMatches. None exist.
Provide a code fix.
```
</details>
</details>
---
<sub>**Tip:** Reply with `@paragon-run` to automatically fix this issue</sub>
##########
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java:
##########
@@ -859,16 +867,64 @@ private List<Endpoint>
queryEndpointTraffic(MatcherSetResult parseResult,
} else if (matcher.getMatchOp() == PromQLParser.NEQ) {
endpoints.stream().filter(e ->
!e.getName().equals(endpointName)).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.RM) {
- endpoints.stream().filter(e ->
e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
} else if (matcher.getMatchOp() == PromQLParser.NRM) {
- endpoints.stream().filter(e ->
!e.getName().matches(endpointName)).limit(limitNum).forEach(result::add);
+ final Pattern pattern = compileRegex(endpointName);
+ endpoints.stream().filter(e -> !safeMatches(pattern,
e.getName())).limit(limitNum).forEach(result::add);
}
} else {
endpoints.stream().limit(limitNum).forEach(result::add);
}
return result;
}
+ private static Pattern compileRegex(final String regex) throws
IllegalExpressionException {
+ try {
+ return Pattern.compile(regex);
+ } catch (PatternSyntaxException e) {
+ throw new IllegalExpressionException("Invalid regex pattern: " +
e.getMessage(), e);
+ }
+ }
+
+ private static boolean safeMatches(final Pattern pattern, final String
input) {
+ return pattern.matcher(new TimeLimitedCharSequence(input,
REGEX_MATCH_TIMEOUT_NANOS)).matches();
+ }
+
+ private static class TimeLimitedCharSequence implements CharSequence {
+ private final CharSequence delegate;
+ private final long deadlineNanos;
+
+ private TimeLimitedCharSequence(final CharSequence delegate, final
long timeoutNanos) {
+ this.delegate = delegate;
+ this.deadlineNanos = System.nanoTime() + timeoutNanos;
+ }
+
+ @Override
+ public int length() {
+ return delegate.length();
+ }
+
+ @Override
+ public char charAt(final int index) {
+ if (System.nanoTime() > deadlineNanos) {
Review Comment:
<!-- paragon-review-issue:1f426f8d-5330-4d01-80a0-8bb4f41c009d-->
### Bug: nanoTime comparison uses > operator instead of subtraction
<!-- **LOW Severity** -->
<!-- DESCRIPTION START -->
nanoTime comparison uses > operator instead of subtraction. Long overflow
can cause incorrect deadline checks. Use System.nanoTime() - deadlineNanos >= 0
instead.
<!-- DESCRIPTION END -->
<details>
<summary>View Details</summary>
**Location:**
`oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java`
(lines 910)
### Analysis
**nanoTime comparison uses > operator instead of subtraction**
| | |
|---|---|
| **What fails** | The charAt method compares System.nanoTime() >
deadlineNanos directly, which is technically incorrect for nanoTime values that
overflow across the long boundary. |
| **Result** | Direct comparison may produce incorrect results if nanoTime
wraps around the long overflow boundary. |
| **Expected** | Should use the subtraction idiom (System.nanoTime() -
deadlineNanos >= 0) as used by JDK's own AbstractQueuedSynchronizer for
correctness. |
| **Impact** | Practically zero risk for a 5s window, but violates the
JDK-standard idiom for safe nanoTime comparison. |
<details>
<summary>How to reproduce</summary>
```bash
This is a theoretical issue that would only manifest after ~292 years of JVM
uptime when nanoTime overflows, but the subtraction idiom is the JDK-standard
pattern.
```
</details>
<details>
<summary>Patch Details</summary>
```diff
- if (System.nanoTime() > deadlineNanos) {
+ if (System.nanoTime() - deadlineNanos >= 0) {
```
</details>
<details>
<summary>AI Fix Prompt</summary>
```
Fix this issue: nanoTime comparison uses > operator instead of subtraction.
Long overflow can cause incorrect deadline checks. Use System.nanoTime() -
deadlineNanos >= 0 instead.
Location:
oap-server/server-query-plugin/promql-plugin/src/main/java/org/apache/skywalking/oap/query/promql/handler/PromQLApiHandler.java
(lines 910)
Problem: The charAt method compares System.nanoTime() > deadlineNanos
directly, which is technically incorrect for nanoTime values that overflow
across the long boundary.
Current behavior: Direct comparison may produce incorrect results if
nanoTime wraps around the long overflow boundary.
Expected: Should use the subtraction idiom (System.nanoTime() -
deadlineNanos >= 0) as used by JDK's own AbstractQueuedSynchronizer for
correctness.
Steps to reproduce: This is a theoretical issue that would only manifest
after ~292 years of JVM uptime when nanoTime overflows, but the subtraction
idiom is the JDK-standard pattern.
Provide a code fix.
```
</details>
</details>
---
<sub>**Tip:** Reply with `@paragon-run` to automatically fix this issue</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
