[Backport 2.19] Add query_string nesting depth limit

[finnegancarroll]

Backport of #22477 to the 2.19 branch.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b944fd8)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Escaped characters not handled maxParenthesisNestingDepth treats every " as a quote toggle and every (/) outside quotes as nesting, but the classic query parser also supports backslash escapes (e.g. \(, \), \"). An escaped parenthesis in the user's query will still be counted toward the nesting depth, and an escaped quote inside an unquoted term will flip inQuotes incorrectly, causing legitimate queries to be miscounted (either false rejections or under-counted depth). Consider skipping the next character when \ is seen. static int maxParenthesisNestingDepth(String query) { int maxDepth = 0; int currentDepth = 0; boolean inQuotes = false; for (int i = 0; i < query.length(); i++) { char c = query.charAt(i); if (c == '"') { inQuotes = !inQuotes; } else if (!inQuotes) { if (c == '(') { currentDepth++; if (currentDepth > maxDepth) { maxDepth = currentDepth; } } else if (c == ')') { currentDepth--; } } } return maxDepth; } Static mutable state race maxQueryNestingDepth is a static non-volatile field written from the cluster settings update thread and read on search threads. Same pattern as the pre-existing maxQueryStringLength, but note that without volatile (or equivalent), dynamic updates may not be observed promptly by other threads. Low impact but worth confirming this matches the intended visibility guarantees. private static int maxQueryNestingDepth = SearchService.SEARCH_MAX_QUERY_NESTING_DEPTH.get(Settings.EMPTY);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b944fd8

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle escaped characters in depth counter The depth counter ignores backslash-escaped characters, so a query like field:" toggles inQuotes and can hide/expose parentheses incorrectly, and ( outside quotes still increments depth even though Lucene treats it as an escaped literal. Handle the escape character (</code>) by skipping the next character to align this pre-check with the actual classic parser tokenization. server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java [903-917] for (int i = 0; i < query.length(); i++) { char c = query.charAt(i); + if (c == '\\' && i + 1 < query.length()) { + i++; // skip escaped character + continue; + } if (c == '"') { inQuotes = !inQuotes; } else if (!inQuotes) { if (c == '(') { currentDepth++; if (currentDepth > maxDepth) { maxDepth = currentDepth; } } else if (c == ')') { currentDepth--; } } } Suggestion importance[1-10]: 7 __ Why: Valid concern: Lucene's classic query parser treats \(, \), and \" as escaped literals, so the pre-check may miscount depth or mis-toggle quote state, potentially leading to false positives/negatives compared to actual parser behavior.	Medium
	Ensure test isolation by resetting settings This test can leak transient cluster settings from prior tests if they fail before the finally block cleanup (or if run in isolation after another test leaves state). More importantly, since this test doesn't modify the setting, it's fine — but it also does not reset state on failure. Consider wrapping in try/finally consistent with the other tests, or explicitly resetting to null at the start to guarantee the default limit is in effect regardless of test ordering. server/src/internalClusterTest/java/org/opensearch/search/query/QueryStringNestingDepthIT.java [121-125] - SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { - client().prepareSearch("test_large").setQuery(queryStringQuery(query)).get(); - }); - assertThat(e.getDetailedMessage(), containsString("nesting depth exceeds max allowed depth")); + try { + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { + client().prepareSearch("test_large").setQuery(queryStringQuery(query)).get(); + }); + assertThat(e.getDetailedMessage(), containsString("nesting depth exceeds max allowed depth")); + } finally { + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().putNull(SEARCH_MAX_QUERY_NESTING_DEPTH.getKey())) + ); + } } Suggestion importance[1-10]: 3 __ Why: Minor test hygiene suggestion. Since this test does not modify settings, cleanup isn't strictly necessary, though adding try/finally consistency could improve robustness against test ordering issues.	Low

---

Previous suggestions

Suggestions up to commit b944fd8

Category	Suggestion	Impact
Possible issue	Handle escaped characters in depth calculation The depth counter does not account for escaped characters. A backslash-escaped quote (") or escaped parenthesis ((, )) inside the query will incorrectly toggle inQuotes or change the depth, potentially causing false rejections or bypassing the limit. Skip the next character when a backslash is encountered to align with Lucene's escaping semantics. server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java [903-917] +if (c == '\\' && i + 1 < query.length()) { + i++; // skip escaped character + continue; +} if (c == '"') { inQuotes = !inQuotes; } else if (!inQuotes) { if (c == '(') { currentDepth++; if (currentDepth > maxDepth) { maxDepth = currentDepth; } } else if (c == ')') { currentDepth--; } } Suggestion importance[1-10]: 7 __ Why: Valid concern: Lucene's classic query parser recognizes backslash escaping, so escaped parentheses/quotes could cause miscounts in maxParenthesisNestingDepth, potentially leading to false positives or bypasses. The fix is reasonable and improves correctness alignment with Lucene semantics.	Medium

Suggestions up to commit b944fd8

Category	Suggestion	Impact
Possible issue	Honor backslash escapes when scanning depth The nesting depth scan does not honor backslash escapes, so an escaped quote (") or escaped parenthesis ((, )) is miscounted. This can cause false positives or, worse, false negatives that bypass the depth check. Skip the next character after a backslash and ignore escaped parentheses when counting depth. server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java [903-917] for (int i = 0; i < query.length(); i++) { char c = query.charAt(i); + if (c == '\\' && i + 1 < query.length()) { + i++; // skip escaped character + continue; + } if (c == '"') { inQuotes = !inQuotes; } else if (!inQuotes) { if (c == '(') { currentDepth++; if (currentDepth > maxDepth) { maxDepth = currentDepth; } } else if (c == ')') { currentDepth--; } } } Suggestion importance[1-10]: 6 __ Why: Lucene's classic query parser does treat \(, \), and \" as escaped literals, so the depth scanner could miscount and produce false positives/negatives. However, since this is a pre-check safeguard against StackOverflow (not a semantic parser), and the impact is mostly minor edge cases, the improvement is moderate rather than critical.	Low

[github-actions]

Persistent review updated to latest commit b944fd8

[github-actions]

Persistent review updated to latest commit b944fd8

[github-actions]

❌ Gradle check result for b944fd8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for b944fd8: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.22%. Comparing base (aff3489) to head (b944fd8).
⚠️ Report is 27 commits behind head on 2.19.

Additional details and impacted files

@@             Coverage Diff              @@
##               2.19   #22854      +/-   ##
============================================
+ Coverage     71.92%   72.22%   +0.30%     
+ Complexity    66009    64527    -1482     
============================================
  Files          5342     5103     -239     
  Lines        307392   299869    -7523     
  Branches      44862    44085     -777     
============================================
- Hits         221105   216594    -4511     
+ Misses        67823    65164    -2659     
+ Partials      18464    18111     -353     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.