You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@directory.apache.org by bd...@apache.org on 2026/05/31 02:06:36 UTC
(directory-scimple) branch develop updated: Cap recursion depth in the SCIM filter and PATCH path parser
This is an automated email from the ASF dual-hosted git repository.
bdemers pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/directory-scimple.git
The following commit(s) were added to refs/heads/develop by this push:
new 895ee055 Cap recursion depth in the SCIM filter and PATCH path parser
895ee055 is described below
commit 895ee055da9cf2aff0ae23042733382abc0af7f7
Author: Brian Demers <bd...@apache.org>
AuthorDate: Sat May 30 21:56:08 2026 -0400
Cap recursion depth in the SCIM filter and PATCH path parser
A small, deeply nested filter or PATCH path (for example "(((((…)))))") recursed until the request thread threw StackOverflowError. Introduce FilterParsers, a single hardened parse facade shared by Filter and PatchOperationPath, which rejects input nested beyond FilterParsers.MAX_NESTING_DEPTH (40) with a FilterParseException thrown during parsing, before the recursive-descent parser can overflow the stack.
Also stop echoing raw input back in the parse-error messages from both entry points.
---
.../apache/directory/scim/spec/filter/Filter.java | 26 +--
.../directory/scim/spec/filter/FilterParsers.java | 177 +++++++++++++++++++++
.../scim/spec/patch/PatchOperationPath.java | 34 +---
.../scim/spec/filter/FilterParsersTest.java | 114 +++++++++++++
.../scim/spec/patch/PatchPathParsingTest.java | 87 ++++++++++
5 files changed, 385 insertions(+), 53 deletions(-)
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/Filter.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/Filter.java
index 0aef1921..457efa07 100644
--- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/Filter.java
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/Filter.java
@@ -19,9 +19,6 @@
package org.apache.directory.scim.spec.filter;
-import org.antlr.v4.runtime.*;
-import org.antlr.v4.runtime.tree.ParseTree;
-import org.antlr.v4.runtime.tree.ParseTreeWalker;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -70,26 +67,9 @@ public class Filter implements Serializable {
}
protected FilterExpression parseFilter(String filter) throws FilterParseException {
- FilterLexer l = new FilterLexer(CharStreams.fromString(filter));
- FilterParser p = new FilterParser(new CommonTokenStream(l));
- p.setBuildParseTree(true);
-
- p.addErrorListener(new BaseErrorListener() {
- @Override
- public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
- throw new IllegalStateException("failed to parse at line " + line + ":" + charPositionInLine + " due to " + msg, e);
- }
- });
-
- try {
- ParseTree tree = p.filter();
- ExpressionBuildingListener expListener = new ExpressionBuildingListener();
- ParseTreeWalker.DEFAULT.walk(expListener, tree);
-
- return expListener.getFilterExpression();
- } catch (IllegalStateException e) {
- throw new FilterParseException("Failed to parse filter: " + filter, e);
- }
+ ExpressionBuildingListener expListener = new ExpressionBuildingListener();
+ FilterParsers.parseFilter(filter, expListener);
+ return expListener.getFilterExpression();
}
@Override
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterParsers.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterParsers.java
new file mode 100644
index 00000000..d468752e
--- /dev/null
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterParsers.java
@@ -0,0 +1,177 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+
+* http://www.apache.org/licenses/LICENSE-2.0
+
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+package org.apache.directory.scim.spec.filter;
+
+import java.util.function.Function;
+
+import org.antlr.v4.runtime.BaseErrorListener;
+import org.antlr.v4.runtime.CharStreams;
+import org.antlr.v4.runtime.CommonTokenStream;
+import org.antlr.v4.runtime.ParserRuleContext;
+import org.antlr.v4.runtime.RecognitionException;
+import org.antlr.v4.runtime.Recognizer;
+import org.antlr.v4.runtime.tree.ErrorNode;
+import org.antlr.v4.runtime.tree.ParseTree;
+import org.antlr.v4.runtime.tree.ParseTreeListener;
+import org.antlr.v4.runtime.tree.ParseTreeWalker;
+import org.antlr.v4.runtime.tree.TerminalNode;
+
+/**
+ * Parses SCIM filter expressions and PATCH operation paths, applying a nesting-depth limit
+ * so that pathologically nested input is rejected as an ordinary parse error rather than
+ * exhausting the thread stack.
+ *
+ * <p>Callers supply their own {@link ParseTreeListener} and read the result from it after
+ * the call returns; the underlying ANTLR lexer/parser and the depth check are internal.
+ */
+public final class FilterParsers {
+
+ /**
+ * Maximum nesting depth accepted while parsing. One unit is counted for each entry into
+ * the recursive {@code filterExpression} or {@code attributeExpression} grammar rule
+ * (roughly one level of {@code ( )} grouping); non-recursive rules are not counted.
+ *
+ * <p>This is a safety bound that keeps recursive-descent parsing from overflowing the
+ * stack — not a promise about an exact number of parentheses. The usable group count is
+ * slightly lower than this value and differs a little between filter and PATCH-path
+ * inputs because of fixed grammar overhead.
+ */
+ public static final int MAX_NESTING_DEPTH = 40;
+
+ private FilterParsers() {
+ }
+
+ /**
+ * Parses a SCIM filter expression, walking the supplied listener over the parse tree.
+ *
+ * @param filter the raw filter string; must not be {@code null}
+ * @param listener the listener to walk over the resulting parse tree
+ * @throws FilterParseException if the input is {@code null}, malformed, or more deeply
+ * nested than {@link #MAX_NESTING_DEPTH}
+ */
+ public static void parseFilter(String filter, ParseTreeListener listener)
+ throws FilterParseException {
+ parse(filter, FilterParser::filter, listener, "filter expression");
+ }
+
+ /**
+ * Parses a SCIM PATCH operation path, walking the supplied listener over the parse tree.
+ *
+ * @param patchPath the raw PATCH path string; must not be {@code null}
+ * @param listener the listener to walk over the resulting parse tree
+ * @throws FilterParseException if the input is {@code null}, malformed, or more deeply
+ * nested than {@link #MAX_NESTING_DEPTH}
+ */
+ public static void parsePatchPath(String patchPath, ParseTreeListener listener)
+ throws FilterParseException {
+ parse(patchPath, FilterParser::patchPath, listener, "patch path expression");
+ }
+
+ private static void parse(String input, Function<FilterParser, ParseTree> rule,
+ ParseTreeListener listener, String what) throws FilterParseException {
+ if (input == null) {
+ throw new FilterParseException("Filter input must not be null");
+ }
+
+ FilterLexer lexer = new FilterLexer(CharStreams.fromString(input));
+ lexer.removeErrorListeners();
+ lexer.addErrorListener(SyntaxErrorListener.INSTANCE);
+
+ CommonTokenStream tokens = new CommonTokenStream(lexer);
+ FilterParser parser = new FilterParser(tokens);
+ parser.setBuildParseTree(true);
+ parser.removeErrorListeners();
+ parser.addErrorListener(SyntaxErrorListener.INSTANCE);
+ parser.addParseListener(new DepthCountingListener());
+
+ try {
+ ParseTree tree = rule.apply(parser);
+ ParseTreeWalker.DEFAULT.walk(listener, tree);
+ } catch (DepthLimitExceededException e) {
+ // Keep the depth message (no raw input); preserve the cause for diagnostics.
+ throw new FilterParseException(e.getMessage(), e);
+ } catch (IllegalStateException e) {
+ // Static message — never echo ANTLR token text back to the caller.
+ throw new FilterParseException("Failed to parse " + what, e);
+ }
+ }
+
+ /**
+ * Turns ANTLR syntax errors into an {@link IllegalStateException} so they can be wrapped
+ * in a {@link FilterParseException} with a static, leak-free message. Stateless; shared.
+ */
+ private static final class SyntaxErrorListener extends BaseErrorListener {
+
+ static final SyntaxErrorListener INSTANCE = new SyntaxErrorListener();
+
+ @Override
+ public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol,
+ int line, int charPositionInLine, String msg, RecognitionException e) {
+ throw new IllegalStateException(
+ "failed to parse at line " + line + ":" + charPositionInLine, e);
+ }
+ }
+
+ /**
+ * Counts entries into the two recursive grammar rules during parsing and aborts the parse
+ * (before the stack can overflow) once {@link #MAX_NESTING_DEPTH} is exceeded. One instance
+ * per parse, as it holds mutable depth state.
+ */
+ private static final class DepthCountingListener implements ParseTreeListener {
+
+ private int depth;
+
+ @Override
+ public void enterEveryRule(ParserRuleContext ctx) {
+ if (isNestingRule(ctx) && ++depth > MAX_NESTING_DEPTH) {
+ throw new DepthLimitExceededException(MAX_NESTING_DEPTH);
+ }
+ }
+
+ @Override
+ public void exitEveryRule(ParserRuleContext ctx) {
+ if (isNestingRule(ctx)) {
+ depth--;
+ }
+ }
+
+ private static boolean isNestingRule(ParserRuleContext ctx) {
+ int ruleIndex = ctx.getRuleIndex();
+ return ruleIndex == FilterParser.RULE_filterExpression
+ || ruleIndex == FilterParser.RULE_attributeExpression;
+ }
+
+ @Override
+ public void visitTerminal(TerminalNode node) {
+ }
+
+ @Override
+ public void visitErrorNode(ErrorNode node) {
+ }
+ }
+
+ /** Internal sentinel thrown when {@link #MAX_NESTING_DEPTH} is exceeded mid-parse. */
+ private static final class DepthLimitExceededException extends RuntimeException {
+
+ DepthLimitExceededException(int limit) {
+ super("Filter nesting depth exceeds maximum of " + limit + " levels");
+ }
+ }
+}
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/patch/PatchOperationPath.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/patch/PatchOperationPath.java
index dc2ca462..b432a151 100644
--- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/patch/PatchOperationPath.java
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/patch/PatchOperationPath.java
@@ -19,17 +19,8 @@
package org.apache.directory.scim.spec.patch;
-import org.antlr.v4.runtime.BaseErrorListener;
-import org.antlr.v4.runtime.CharStreams;
-import org.antlr.v4.runtime.CommonTokenStream;
-import org.antlr.v4.runtime.RecognitionException;
-import org.antlr.v4.runtime.Recognizer;
-import org.antlr.v4.runtime.tree.ParseTree;
-import org.antlr.v4.runtime.tree.ParseTreeWalker;
-
-import org.apache.directory.scim.spec.filter.FilterLexer;
-import org.apache.directory.scim.spec.filter.FilterParser;
import org.apache.directory.scim.spec.filter.FilterParseException;
+import org.apache.directory.scim.spec.filter.FilterParsers;
import org.apache.directory.scim.spec.filter.ValuePathExpression;
import java.io.Serial;
@@ -47,26 +38,9 @@ public class PatchOperationPath implements Serializable {
}
static ValuePathExpression parsePatchPath(String patchPath) throws FilterParseException {
- FilterLexer l = new FilterLexer(CharStreams.fromString(patchPath));
- FilterParser p = new FilterParser(new CommonTokenStream(l));
- p.setBuildParseTree(true);
-
- p.addErrorListener(new BaseErrorListener() {
- @Override
- public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
- throw new IllegalStateException("failed to parse at line " + line + " due to " + msg, e);
- }
- });
-
- try {
- ParseTree tree = p.patchPath();
- PatchPathListener patchPathListener = new PatchPathListener();
- ParseTreeWalker.DEFAULT.walk(patchPathListener, tree);
-
- return patchPathListener.getValuePathExpression();
- } catch (IllegalStateException e) {
- throw new FilterParseException(e);
- }
+ PatchPathListener patchPathListener = new PatchPathListener();
+ FilterParsers.parsePatchPath(patchPath, patchPathListener);
+ return patchPathListener.getValuePathExpression();
}
@Override
diff --git a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterParsersTest.java b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterParsersTest.java
new file mode 100644
index 00000000..b6cbba68
--- /dev/null
+++ b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterParsersTest.java
@@ -0,0 +1,114 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+
+* http://www.apache.org/licenses/LICENSE-2.0
+
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+package org.apache.directory.scim.spec.filter;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Verifies the nesting-depth limit applied by {@link FilterParsers} when parsing filter
+ * expressions (exercised through the {@link Filter} public API).
+ */
+public class FilterParsersTest {
+
+ // A simple filter predicate "a pr" has a base rule-entry depth of 2 (one filterExpression
+ // + one attributeExpression), and each additional '(' group adds one filterExpression
+ // entry. So the largest nesting that still parses is MAX_NESTING_DEPTH - 2 groups.
+ private static final int LARGEST_PARSING_GROUPS = FilterParsers.MAX_NESTING_DEPTH - 2; // 38
+
+ @Test
+ public void shortValidFilter_parsesSuccessfully() {
+ assertThatCode(() -> new Filter("userName eq \"bjensen\""))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ public void depthAtLimit_largestParsingNesting_parsesSuccessfully() {
+ String nested = "(".repeat(LARGEST_PARSING_GROUPS) + "a pr" + ")".repeat(LARGEST_PARSING_GROUPS);
+
+ assertThatCode(() -> new Filter(nested))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ public void depthOnePastLimit_smallestThrowingNesting_throwsFilterParseException() {
+ int groups = LARGEST_PARSING_GROUPS + 1; // 39 → total rule-entry depth 41 > MAX_NESTING_DEPTH
+ String nested = "(".repeat(groups) + "a pr" + ")".repeat(groups);
+
+ assertThatThrownBy(() -> new Filter(nested))
+ .isInstanceOf(FilterParseException.class)
+ .hasMessageContaining("nesting depth exceeds maximum");
+ }
+
+ @Test
+ @Timeout(value = 2, unit = TimeUnit.SECONDS)
+ public void depthOf200_throwsFilterParseException_noStackOverflow() {
+ String nested = "(".repeat(200) + "a pr" + ")".repeat(200);
+
+ assertThatThrownBy(() -> new Filter(nested))
+ .isInstanceOf(FilterParseException.class)
+ .hasMessageContaining("nesting depth exceeds maximum")
+ .satisfies(ex -> {
+ for (Throwable t = ex; t != null; t = t.getCause()) {
+ assertThat(t).isNotInstanceOf(StackOverflowError.class);
+ }
+ });
+ }
+
+ @Test
+ @Timeout(value = 15, unit = TimeUnit.SECONDS)
+ public void depth1000_doesNotStackOverflow() {
+ String nested = "(".repeat(1000) + "a pr" + ")".repeat(1000);
+
+ assertThatThrownBy(() -> new Filter(nested))
+ .isInstanceOf(FilterParseException.class)
+ .satisfies(ex -> {
+ for (Throwable t = ex; t != null; t = t.getCause()) {
+ assertThat(t).isNotInstanceOf(StackOverflowError.class);
+ }
+ });
+ }
+
+ @Test
+ public void malformedFilter_missingOperand_throwsFilterParseException() {
+ assertThatThrownBy(() -> new Filter("userName eq"))
+ .isInstanceOf(FilterParseException.class);
+ }
+
+ @Test
+ public void malformedFilter_unbalancedParens_throwsFilterParseException() {
+ assertThatThrownBy(() -> new Filter("((a pr)"))
+ .isInstanceOf(FilterParseException.class);
+ }
+
+ @Test
+ public void nullInput_throwsFilterParseException_notNullPointer() {
+ assertThatThrownBy(() -> FilterParsers.parseFilter(null, new ExpressionBuildingListener()))
+ .isInstanceOf(FilterParseException.class)
+ .isNotInstanceOf(NullPointerException.class)
+ .hasMessageContaining("must not be null");
+ }
+}
diff --git a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/patch/PatchPathParsingTest.java b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/patch/PatchPathParsingTest.java
new file mode 100644
index 00000000..2134a17a
--- /dev/null
+++ b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/patch/PatchPathParsingTest.java
@@ -0,0 +1,87 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+
+* http://www.apache.org/licenses/LICENSE-2.0
+
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied. See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+package org.apache.directory.scim.spec.patch;
+
+import org.apache.directory.scim.spec.filter.FilterParseException;
+import org.apache.directory.scim.spec.filter.FilterParsers;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Verifies the nesting-depth limit applied by {@link FilterParsers} when parsing PATCH
+ * operation paths (exercised through {@link PatchOperationPath}).
+ *
+ * <p>Depth tests use the nested-group form {@code attrName[(((a pr)))]}, which drives the
+ * recursive {@code attributeExpression} grammar rule, rather than a flat {@code and}-chain
+ * (which ANTLR handles iteratively and would not exercise the stack guard).
+ */
+public class PatchPathParsingTest {
+
+ // A PATCH-path predicate "attrName[a pr]" has a base rule-entry depth of 1 (one
+ // attributeExpression), and each additional '(' group adds one more. So the largest
+ // nesting that still parses is MAX_NESTING_DEPTH - 1 groups. (The filter and PATCH-path
+ // grammars have different base offsets, which is why this differs from the filter tests.)
+ private static final int LARGEST_PARSING_GROUPS = FilterParsers.MAX_NESTING_DEPTH - 1; // 39
+
+ private static String nestedPatchPath(int groups) {
+ return "attrName[" + "(".repeat(groups) + "a pr" + ")".repeat(groups) + "]";
+ }
+
+ @Test
+ public void depthAtLimit_largestParsingNesting_parsesSuccessfully() {
+ assertThatCode(() -> PatchOperationPath.fromString(nestedPatchPath(LARGEST_PARSING_GROUPS)))
+ .doesNotThrowAnyException();
+ }
+
+ @Test
+ public void depthOnePastLimit_smallestThrowingNesting_throwsFilterParseException() {
+ assertThatThrownBy(() -> PatchOperationPath.fromString(nestedPatchPath(LARGEST_PARSING_GROUPS + 1)))
+ .isInstanceOf(FilterParseException.class)
+ .hasMessageContaining("nesting depth exceeds maximum");
+ }
+
+ @Test
+ @Timeout(value = 2, unit = TimeUnit.SECONDS)
+ public void depthOf200_throwsFilterParseException_noStackOverflow() {
+ assertThatThrownBy(() -> PatchOperationPath.fromString(nestedPatchPath(200)))
+ .isInstanceOf(FilterParseException.class)
+ .hasMessageContaining("nesting depth exceeds maximum")
+ .satisfies(ex -> {
+ for (Throwable t = ex; t != null; t = t.getCause()) {
+ assertThat(t).isNotInstanceOf(StackOverflowError.class);
+ }
+ });
+ }
+
+ @Test
+ public void malformedPatchPath_unbalancedBracket_throwsFilterParseException() {
+ // Confirms the syntax-error path still yields a FilterParseException with a static,
+ // leak-free message (no raw input / ANTLR token text).
+ assertThatThrownBy(() -> PatchOperationPath.fromString("attrName[a pr"))
+ .isInstanceOf(FilterParseException.class)
+ .hasMessage("Failed to parse patch path expression");
+ }
+}