Support neural queries with hybrid DLS

[sharp-pixel]

Description

This is a follow-up to #6416, which preserves a hybrid query as the top-level
query while applying Document-Level Security (DLS) to its execution branches.

The verification introduced by #6416 handles query builders that apply filters
through the standard boolean-wrapper contract. Neural and k-NN queries apply
filters differently:

• neural stores the filter internally and returns the same query builder;
• knn returns a copied query builder containing the combined filter.

As a result, otherwise valid hybrid requests containing these branches can fail
closed after DLS application.

This PR extends hybrid DLS handling without compile-time dependencies on either
optional plugin and without reflecting on plugin-specific getter names.
Security now relies on the public QueryBuilder#filter(QueryBuilder) and
QueryBuilder#visit(QueryBuilderVisitor) contract:

• hybrid builders expose their execution branches through visit();
• a branch that stores an embedded filter exposes it as a direct
  BooleanClause.Occur.FILTER child;
• filter() preserves the original query semantics while adding the supplied
  filter.

After applying DLS, Security verifies that:

• every original hybrid branch is preserved exactly once;
• the filtered hybrid query retains the same query type;
• filter-aware copied branches retain the same runtime query type and registered
  query name;
• the exact DLS query instance is present in each branch's filter path; and
• an existing embedded filter is composed structurally with DLS.

This contract-based approach remains valid for 3.9 and later releases without a
per-release upper-version allowlist. If a query builder does not expose its
branches or embedded filters through visit(), changes query type, drops a
branch, or fails to retain DLS, Security fails closed.

The change also:

• preserves standard query metadata (boost and query name) when a filter-aware
  query returns a copy;
• preserves implicit minimum_should_match behavior when DLS is added to an
  embedded boolean filter;
• rejects parent/child clauses anywhere in the exposed hybrid query tree;
• supports both in-place and copied filter-aware query builders; and
• keeps neural_sparse on the generic boolean-wrapper path.

No new setting, permission, REST API, optional-plugin dependency, or
plugin-specific reflection is introduced.

Companion PR and merge relationship

This PR and
opensearch-project/neural-search#1957
are interlocked for dense neural hybrid DLS:

• Security Support neural queries with hybrid DLS #6428 needs Neural Increments minor patch version number in build.gradle #1957's NeuralQueryBuilder#visit() override to
  discover the embedded neural filter. Without that traversal, Security fails
  closed rather than applying DLS incompletely.
• Neural Increments minor patch version number in build.gradle #1957's cross-plugin DLS integration test needs the contract-based
  verification implemented by Security Support neural queries with hybrid DLS #6428.

This is not a compile-time dependency cycle: both repositories compile and run
their unit tests independently. The practical sequence is to merge and publish
Security #6428, validate Neural #1957 against that Security artifact, and then
merge Neural #1957. Both changes should ship together in OpenSearch 3.9.

Related work

• Follow-up to Support hybrid search with adaptive DLS #6416.
• Interlocked companion change and cross-plugin coverage:
  opensearch-project/neural-search#1957.
• Related Neural Search issue:
  opensearch-project/neural-search#1303.
• Independent dense-neural model-registration blocker: [BUG] User principal creates a Jackson self-reference in ML Commons #6433.

Testing

Focused Security regression tests:

./gradlew test \
  --tests "org.opensearch.security.configuration.DlsFilterLevelActionHandlerTest" \
  --tests "org.opensearch.security.configuration.DlsFlsValveImplTest" \
  --tests "org.opensearch.security.util.ParentChildrenQueryDetectorTest"

Result: passed.

Formatting and static analysis:

./gradlew spotlessJavaCheck checkstyleMain checkstyleTest

Result: passed.

JaCoCo reports 100% line coverage and 91.3% branch coverage across the modified
Security verification methods. The new direct-filter visitor has 100% line and
branch coverage.

The companion HybridQueryDlsIT suite was run against the locally published
Security plugin on a three-node Neural Search cluster. Five of six tests pass,
including direct k-NN, existing-filter, single-clause, suggestion, aggregation,
and hit-filtering coverage. The dense-neural test is currently blocked during
ML model registration by #6433, before the neural query executes.

Check List

• New functionality includes testing
• New functionality has been documented — behavior, contract, companion
  dependency, and support boundaries are documented above and in code comments;
  no user-facing setting or API is introduced
• New Roles/Permissions have a corresponding security dashboards plugin PR
  — N/A, no roles or permissions were added
• API changes companion pull request created — N/A, this consumes and
  completes an existing public QueryBuilder contract without adding API
  surface
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 80f8fe1)

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 Ambiguous branch matching In isDlsFilterAppliedToEverySubquery, the identity-based matching loop iterates all unmatched originals and only rejects when it finds a second match whose query() differs from the first. When the same original subquery instance appears multiple times in the hybrid (e.g., duplicate branch), all entries in unmatchedOriginalSubqueries share the same query() reference, so the loop silently overwrites preservedOriginalSubquery and consumes only the last matching index rather than pairing each filtered branch with a distinct original slot deterministically. While unmatchedOriginalSubqueries.remove(preservedOriginalSubqueryIndex) still removes one entry, the logic conflates "duplicate legitimate original" with "single filtered branch merging multiple originals" and depends on the caller re-invoking filter() per branch — worth verifying no scenario allows a single filtered branch to satisfy verification for two distinct original slots. List<OriginalSubqueryState> unmatchedOriginalSubqueries = new ArrayList<>(originalSubqueryStates); for (QueryBuilder filteredSubquery : filteredSubqueries) { OriginalSubqueryState preservedOriginalSubquery = null; int preservedOriginalSubqueryIndex = -1; for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { // A filtered branch must not merge multiple original hybrid execution branches. return false; } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; } } if (preservedOriginalSubquery == null) { for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isSameQueryType(filteredSubquery, originalSubquery.query()) && isEmbeddedDlsFilterApplied(filteredSubquery, originalSubquery, filterLevelQueryBuilder)) { preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; // Filtered copies cannot be matched by identity, so retain the hybrid builder's stable order. break; } } } if (preservedOriginalSubquery == null) { return false; } preserveQueryMetadata(filteredSubquery, preservedOriginalSubquery.query()); preserveEmbeddedFilterMetadata(filteredSubquery, preservedOriginalSubquery); unmatchedOriginalSubqueries.remove(preservedOriginalSubqueryIndex); } return unmatchedOriginalSubqueries.isEmpty();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 80f8fe1

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Break after first identity match found The identity-based structural match loop iterates through all unmatched originals but only rejects merges when different query instances match; if the same instance appears twice in the list, both entries will match and the loop keeps overwriting preservedOriginalSubqueryIndex with the last match. This is fine functionally, but the loop should break after the first match to avoid ambiguity and to mirror the stable-order semantics used by the fallback branch. Additionally, when the same instance appears N times, only one should be consumed per filtered subquery. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [395-405] for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { // A filtered branch must not merge multiple original hybrid execution branches. return false; } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; + break; } } Suggestion importance[1-10]: 4 __ Why: Adding a break after the first identity match is a reasonable minor improvement for clarity and stable-order semantics, but the existing logic is functionally correct since duplicate identity matches are permitted and only one entry is consumed via remove. Impact is modest.	Low
	Avoid clobbering already-preserved metadata Unconditionally overwriting boost and queryName on the filtered copy may clobber values the plugin's filter(...) implementation legitimately set (e.g., a plugin that already forwards these). Only copy metadata when the filtered subquery has default/unset values, or verify the filtered copy has not already preserved them, to avoid silently changing user-visible query semantics. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [517-520] } else if (filteredSubquery != originalSubquery && isSameQueryType(filteredSubquery, originalSubquery)) { - filteredSubquery.boost(originalSubquery.boost()); - filteredSubquery.queryName(originalSubquery.queryName()); + if (filteredSubquery.boost() == 1.0f) { + filteredSubquery.boost(originalSubquery.boost()); + } + if (filteredSubquery.queryName() == null) { + filteredSubquery.queryName(originalSubquery.queryName()); + } } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about clobbering plugin-set metadata, but the proposed check (boost == 1.0f and queryName == null) is a fragile heuristic that could itself miss legitimate defaults. The improvement is speculative without more context on plugin behavior.	Low

---

Previous suggestions

Suggestions up to commit 7994f47

Category	Suggestion	Impact
General	Avoid clobbering plugin-set metadata unconditionally This metadata-preservation branch is invoked for arbitrary query types after a plugin returns a filtered copy. If the plugin's filter(...) already copied boost/queryName correctly (or intentionally reset them), unconditionally overwriting them with the original's values may alter user-intended semantics for query types other than ConstantScoreQueryBuilder. Consider only overwriting when the filtered copy still holds default values, to avoid clobbering plugin-set metadata. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [517-520] } else if (filteredSubquery != originalSubquery && isSameQueryType(filteredSubquery, originalSubquery)) { - filteredSubquery.boost(originalSubquery.boost()); - filteredSubquery.queryName(originalSubquery.queryName()); + if (filteredSubquery.boost() == AbstractQueryBuilder.DEFAULT_BOOST) { + filteredSubquery.boost(originalSubquery.boost()); + } + if (filteredSubquery.queryName() == null) { + filteredSubquery.queryName(originalSubquery.queryName()); + } } Suggestion importance[1-10]: 4 __ Why: The concern is reasonable in principle: unconditionally overwriting boost/queryName could clobber intentionally-set values by plugin filter() implementations. However, the impact is minor and the current behavior is likely intentional to preserve original metadata.	Low
	Break on first identity match for stability When multiple original subqueries share the same identity, the loop continues iterating after finding a match and reassigns preservedOriginalSubqueryIndex to the last matching index without incrementing a counter. This is fine functionally, but the ambiguity check compares query() identity only — if two distinct original subqueries happen to both structurally match the same filtered subquery (both identity-equal to it via originalSubquery == filteredSubquery), the check will not fire but the wrong index may be removed. Consider breaking on the first match to preserve stable ordering, matching the fallback branch's behavior. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [395-405] for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { // A filtered branch must not merge multiple original hybrid execution branches. return false; } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; + break; } } Suggestion importance[1-10]: 3 __ Why: The suggestion to break on first match would actually contradict the existing ambiguity check logic that iterates all candidates to detect merging of multiple original branches. The concern about "wrong index removed" is speculative and the existing code appears intentional.	Low

Suggestions up to commit 2e5b9fb

Category	Suggestion	Impact
General	Avoid clobbering plugin-set metadata Unconditionally overwriting boost and queryName on the filtered copy may clobber values the plugin's filter() intentionally set (e.g., a different boost applied by the plugin's builder). Only copy when the filtered copy still has default/unset metadata to avoid silently mutating plugin-provided values. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [514-517] } else if (filteredSubquery != originalSubquery && isSameQueryType(filteredSubquery, originalSubquery)) { - filteredSubquery.boost(originalSubquery.boost()); - filteredSubquery.queryName(originalSubquery.queryName()); + if (filteredSubquery.boost() == AbstractQueryBuilder.DEFAULT_BOOST) { + filteredSubquery.boost(originalSubquery.boost()); + } + if (filteredSubquery.queryName() == null) { + filteredSubquery.queryName(originalSubquery.queryName()); + } } Suggestion importance[1-10]: 4 __ Why: The concern about clobbering plugin-set metadata has some merit, but since the code path specifically handles filtered copies that should preserve original metadata, and the tests validate this exact behavior, the suggested change may actually break intended behavior. The impact is speculative.	Low
	Break after first identity match found When multiple unmatched original subqueries share the same identity (same instance appears twice), the loop overwrites preservedOriginalSubqueryIndex with the last matching entry rather than consuming the first. This is harmless for identity but the check preservedOriginalSubquery.query() != originalSubquery.query() should also break after the first match to avoid over-writing the index and to keep the removal deterministic. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [394-401] if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { // A filtered branch must not merge multiple original hybrid execution branches. return false; } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; + if (preservedOriginalSubquery.query() == originalSubquery.query()) { + break; + } } Suggestion importance[1-10]: 3 __ Why: The suggestion is minor; the existing loop still produces correct behavior since the identity check preservedOriginalSubquery.query() != originalSubquery.query() guards against merging, and overwriting the index with a same-identity match is functionally equivalent. The improvement is a marginal optimization/determinism nit.	Low

Suggestions up to commit bc86fd0

Category	Suggestion	Impact
Possible issue	Re-check parent/child in embedded filters When the original had no embedded filter and the filtered copy exposes exactly one filter that happens to reference the DLS query, this returns true — but the parent-child pre-check in applyFilterLevelDls runs against the original query tree, so a plugin that copies parent/child clauses into a new filter branch would bypass detection. Also verify the filtered embedded filter is not itself a parent/child query before accepting it. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [430-439] List<QueryBuilder> filteredEmbeddedFilters = directFilters(filteredSubquery); if (filteredEmbeddedFilters.size() != 1 || originalSubquery.embeddedFilters().size() > 1) { return false; } QueryBuilder filteredEmbeddedFilter = filteredEmbeddedFilters.get(0); +if (ParentChildrenQueryDetector.hasParentOrChildQuery(filteredEmbeddedFilter)) { + throw new OpenSearchSecurityException("Unable to handle filter level DLS for hybrid queries with parent or child clauses"); +} if (originalSubquery.embeddedFilters().isEmpty()) { return filteredEmbeddedFilter == filterLevelQueryBuilder; } return isDlsFilterAppliedStructurally(filteredEmbeddedFilter, originalSubquery.embeddedFilters().get(0), filterLevelQueryBuilder); Suggestion importance[1-10]: 6 __ Why: The suggestion raises a legitimate concern about parent/child clauses potentially being introduced into embedded filters after the initial pre-check, as demonstrated by the existing test failsClosedWhenNeuralFilterContainsParentChildClause. However, the current test shows that such cases already fail (though with a different message), so the practical security impact is limited.	Low
	Stop after first identity match found The loop continues after finding a first match, and only fails if a second match belongs to a different original query instance. However, preservedOriginalSubqueryIndex gets overwritten to the last matching index, so when two identical original instances match, the wrong (later) entry is removed. Break after the first match to preserve stable pairing order, consistent with the fallback branch below. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [392-402] for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { - if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { - // A filtered branch must not merge multiple original hybrid execution branches. - return false; - } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; + break; } } Suggestion importance[1-10]: 5 __ Why: The observation is partially valid: when multiple identical original instances match structurally, the loop overwrites preservedOriginalSubqueryIndex with the last match rather than the first, which could affect pairing order for equal instances. However, since the matches involve the same query content, functional correctness may not be significantly affected, and the existing test acceptsEqualHybridSubqueriesInDifferentOrder passes.	Low

Suggestions up to commit 1d66f2f

Category	Suggestion	Impact
General	Verify structural equivalence beyond query type isEmbeddedDlsFilterApplied returns true when the original had no embedded filter and the filtered subquery's sole filter is exactly filterLevelQueryBuilder, but it does not verify that the filtered subquery is otherwise structurally equivalent to the original (only isSameQueryType was checked at the call site, which just compares class + name). A plugin builder that silently drops inner content while returning the DLS filter as its sole exposed FILTER child would pass. Consider also validating that the filtered subquery preserves the original's non-filter content, or documenting/enforcing this stricter contract. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [430-438] List<QueryBuilder> filteredEmbeddedFilters = directFilters(filteredSubquery); if (filteredEmbeddedFilters.size() != 1 || originalSubquery.embeddedFilters().size() > 1) { return false; } QueryBuilder filteredEmbeddedFilter = filteredEmbeddedFilters.get(0); if (originalSubquery.embeddedFilters().isEmpty()) { - return filteredEmbeddedFilter == filterLevelQueryBuilder; + return filteredEmbeddedFilter == filterLevelQueryBuilder && filteredSubquery.equals(originalSubquery.query()); } return isDlsFilterAppliedStructurally(filteredEmbeddedFilter, originalSubquery.embeddedFilters().get(0), filterLevelQueryBuilder); Suggestion importance[1-10]: 4 __ Why: The concern about a plugin builder dropping content while returning only the DLS filter is somewhat theoretical, and the proposed .equals() check may not work well since filtered subquery is expected to differ (it now contains the filter). The suggestion raises a valid contract concern but the improved code isn't clearly correct.	Low
Possible issue	Avoid double-counting identical original subqueries When multiple original subqueries share the same identity (e.g. the same instance passed twice) and the structural match succeeds for both entries, the loop keeps overwriting preservedOriginalSubqueryIndex with the last match, but the check preservedOriginalSubquery.query() != originalSubquery.query() allows this. After the loop, only one index is removed, and the same match may be reused for the next filtered subquery, potentially passing a hybrid where some branches were never filtered. Break out of the loop after the first structural match to preserve one-to-one pairing. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [392-402] for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedStructurally(filteredSubquery, originalSubquery.query(), filterLevelQueryBuilder)) { - if (preservedOriginalSubquery != null && preservedOriginalSubquery.query() != originalSubquery.query()) { - // A filtered branch must not merge multiple original hybrid execution branches. - return false; - } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; + break; } } Suggestion importance[1-10]: 3 __ Why: The existing check preservedOriginalSubquery.query() != originalSubquery.query() combined with identity-based structural matching (via == in isDlsFilterAppliedStructurally) already handles the same-instance case, and the test acceptsSameHybridSubqueryInstanceTwice passes. The concern about double-counting appears partially valid only in edge cases, and breaking early could actually change matching semantics.	Low

Suggestions up to commit 3e57e8b

Category	Suggestion	Impact
Security	Snapshot filtered values before comparison The snapshot compares its stored methodParameters (already snapshotted at capture time) against the live map from the filtered query, but the original's map reference may itself have been mutated in place after snapshotting. The comparison uses the snapshot copy correctly, but vector and methodParameters on filteredQuery are read live — if they share the same mutable reference as the original, an in-place mutation would go undetected. Snapshot the filtered query's mutable values with snapshotMutableValue before comparing to ensure defense against in-place mutation on both sides. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [643-654] private boolean matches(QueryBuilder filteredQuery, QueryBuilder filterLevelQueryBuilder) { return Objects.equals(fieldName, KNN_FIELD_NAME_GETTER.apply(filteredQuery)) - && Objects.deepEquals(vector, KNN_VECTOR_GETTER.apply(filteredQuery)) + && Objects.deepEquals(vector, snapshotMutableValue(KNN_VECTOR_GETTER.apply(filteredQuery))) && Objects.equals(k, KNN_K_GETTER.apply(filteredQuery)) && Objects.equals(maxDistance, KNN_MAX_DISTANCE_GETTER.apply(filteredQuery)) && Objects.equals(minScore, KNN_MIN_SCORE_GETTER.apply(filteredQuery)) - && Objects.equals(methodParameters, KNN_METHOD_PARAMETERS_GETTER.apply(filteredQuery)) + && Objects.equals(methodParameters, snapshotMutableValue(KNN_METHOD_PARAMETERS_GETTER.apply(filteredQuery))) && Objects.equals(ignoreUnmapped, KNN_IGNORE_UNMAPPED_GETTER.apply(filteredQuery)) && Objects.equals(rescoreContext, KNN_RESCORE_CONTEXT_GETTER.apply(filteredQuery)) && Objects.equals(expandNested, KNN_EXPAND_NESTED_GETTER.apply(filteredQuery)) && isEmbeddedDlsFilterApplied(KNN_FILTER_GETTER.apply(filteredQuery), filter, filterLevelQueryBuilder); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a reasonable concern about defense-in-depth: if the filtered query shares mutable references (e.g. vector arrays, method-parameter maps) with the original after in-place mutation, comparing against a live read could mask semantic changes. However, in-place mutation would affect both snapshot and live reads equally if they alias, so the practical impact is limited; still, the change adds robustness.	Low
	Tighten ambiguity check for equal branches The ambiguity check only rejects when two matches point to different query() instances, but for non-kNN filtered subqueries, multiple structurally identical originals could match the same filtered branch and be silently collapsed on subsequent iterations. Since the ambiguity check compares against the previously seen match's .query() identity, if a third original matches with the same identity as the first, the second (different-identity) match is already returned false — but if the same original instance appears twice in unmatchedOriginalSubqueries, both entries will match and only one is removed. Verify that equal-but-distinct original instances for non-kNN branches are handled correctly, or apply the same order-preserving break for all subquery types. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [439-455] for (int i = 0; i < unmatchedOriginalSubqueries.size(); i++) { OriginalSubqueryState originalSubquery = unmatchedOriginalSubqueries.get(i); if (isDlsFilterAppliedToSubquery(filteredSubquery, originalSubquery, filterLevelQueryBuilder)) { - if (preservedOriginalSubquery != null - && preservedOriginalSubquery.query() != originalSubquery.query() - && !filteredSubqueryIsKnn) { + if (preservedOriginalSubquery != null && !filteredSubqueryIsKnn) { // A filtered branch must not merge multiple original hybrid execution branches. return false; } preservedOriginalSubquery = originalSubquery; preservedOriginalSubqueryIndex = i; if (filteredSubqueryIsKnn) { - // k-NN copies lose metadata, so equal branches must retain the plugin's stable list order. break; } } } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a subtle edge case where structurally identical non-kNN originals might match the same filtered branch and be collapsed. The relaxed identity check was likely intentional to allow the same instance appearing twice, but the tightened check could improve safety. The correctness impact is unclear without deeper context.	Low

[github-actions]

Persistent review updated to latest commit 5a90870

[github-actions]

Persistent review updated to latest commit ce5a1cd

[codecov]

Codecov Report

❌ Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.75%. Comparing base (c33b831) to head (7994f47).

Files with missing lines	Patch %	Lines
...ity/configuration/DlsFilterLevelActionHandler.java	93.65%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6428   +/-   ##
=======================================
  Coverage   75.75%   75.75%           
=======================================
  Files         457      457           
  Lines       30508    30556   +48     
  Branches     4615     4630   +15     
=======================================
+ Hits        23111    23149   +38     
- Misses       5276     5284    +8     
- Partials     2121     2123    +2     

Files with missing lines	Coverage Δ
...ity/configuration/DlsFilterLevelActionHandler.java	75.13% <93.65%> (+2.40%)	⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cwperks]

@sharp-pixel can you please fix the conflicts post-merge of hybrid query support?

[sharp-pixel]

@sharp-pixel can you please fix the conflicts post-merge of hybrid query support?

looking

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 5aff58b.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java	419	medium	The KNN subquery DLS verification checks only that both the original and filtered subquery names equal 'knn', without confirming the DLS filter is actually present inside the returned builder. All other paths (BoolQueryBuilder) explicitly assert the filter object is reachable in the filter clause. A plugin registering a query builder whose getName() returns 'knn' could return a structurally different builder and this check would still pass, silently bypassing DLS enforcement.
src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java	415	low	The neural query DLS verification returns true based solely on object identity (filteredSubquery == originalSubquery) plus the name 'neural', trusting that the builder mutated itself to include the DLS filter. If the neural builder's filter() implementation is a no-op or fails silently, the identity check still passes and DLS is considered applied without any structural verification.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 5aff58b

[DarshitChanpura]

Multi-knn hybrid queries fail closed under DLS. The knn fast path in isDlsFilterAppliedToSubquery (DlsFilterLevelActionHandler.java:420-422) matches purely on getName() with no reference anchor. Because the outer loop in isDlsFilterAppliedToEverySubquery is order-independent, a single filtered knn subquery matches every knn entry in unmatchedOriginalSubqueries, tripping the "must not merge multiple original branches" guard.

Repro — originals [knn(a), knn(b)], filtered [knn(a'), knn(b')]:

unmatched = {knn(a):1, knn(b):1}
filtered knn(a'):
  (knn(a),1): name match -> preserved = knn(a)
  (knn(b),1): name match -> preserved != null -> return false
-> "Hybrid query did not apply the DLS filter to every subquery"

Any hybrid with 2+ knn clauses (multi-vector-field search) throws. It fails closed, so not a security hole — but it breaks a legitimate workflow, and acceptsKnnHybridSubqueryWithFilterCopy only covers the single-knn case so tests don't catch it. neural/neural_sparse are unaffected (reference-identity match disambiguates).

Verified against neural-search main: HybridQueryBuilder.filter() and visit() both walk the same ArrayList in order and replace in place, so subqueries stay 1:1 and positionally aligned. Simplest fix is to pair filteredSubqueries[i] with originalSubqueries[i] positionally instead of the name-based multiset match; if you want to keep order-independence, consume exactly one original per filtered on the knn path (break after first match) rather than treating a second candidate as a merge. Please add a 2+-knn test either way.

[sharp-pixel]

I am reviewing in depth because several things are broken in their current state.

[github-actions]

Persistent review updated to latest commit 8afbad7

[github-actions]

Persistent review updated to latest commit 3e57e8b

[sharp-pixel]

I reworked the implementation

[DarshitChanpura]

I'm wary of this reflection surface (DlsFilterLevelActionHandler.java:87-127). Reflected bindings to ~10 KNNQueryBuilder/NeuralQueryBuilder getters are fragile: they're concrete method names on optional-plugin internals with no version guard, so a rename or signature change silently disables knn/neural hybrid DLS (fails closed, but the feature just stops working) and unit tests can't catch it since they mock QueryBuilder.

Worth noting applying the filter already uses a core contract - QueryBuilder#filter(QueryBuilder) and #visit(...). The reflection only exists to verify the result. So the clean fix is a core extension point for that verification - e.g. a way to introspect the filter embedded in a leaf builder - so Security can confirm the DLS filter landed without reaching into plugin internals. I'd prefer that over reflection.

If a core change isn't feasible in this PR's timeline, please add a version guard (min k-NN/neural or OpenSearch version where these accessors are known-stable) so drift fails predictably at a known boundary rather than silently.

[sharp-pixel]

Thanks for raising this. Keeping in mind that we will need to backport the fix, version guard is a bit tricky.
I am removing the reflection and will propose a better alernative.

[sharp-pixel]

@DarshitChanpura Can you check the revised implementation?

[github-actions]

Persistent review updated to latest commit 1d66f2f

[DarshitChanpura]

Cross-repo gap: dense-neural depends on neural-search opensearch-project/neural-search#1957 (still open). Security reads a subquery's embedded filter via visit(getChildVisitor(Occur.FILTER)). KNNQueryBuilder exposes it that way and neural_sparse works via bool-wrap, but NeuralQueryBuilder.filter() stores into queryfilter / returns this with no visit() override on main — that override is in opensearch-project/neural-search#1957. Until it ships, dense-neural hybrid+DLS fails closed (as failsClosedWhenNeuralFilterIsNotExposedByVisitor intends).

opensearch-project/neural-search#1957 targets 3.9, same as this PR's V_3_9_0 floor — no version bump needed, but the two must land in the same 3.9 release or dense neural is dead on arrival. Also note in the neural tests that they pass only because stubFilterAwareQuery simulates opensearch-project/neural-search#1957 's unreleased visit(), so they aren't end-to-end proof for dense neural — the IT in opensearch-project/neural-search#1957 is.

[github-actions]

Persistent review updated to latest commit bc86fd0

[github-actions]

Persistent review updated to latest commit 2e5b9fb

[sharp-pixel]

Cross-plugin coordination note: after this Security change lands, hybrid queries containing NeuralQueryBuilder will temporarily fail closed against the current Neural Search implementation because its embedded filter is not yet exposed through QueryBuilderVisitor. This is an availability/compatibility limitation, not a DLS bypass: unauthorized documents are not returned.

opensearch-project/neural-search#1957 adds the required FILTER visitor traversal and cross-plugin integration coverage, enabling dense neural hybrid DLS. The two PRs are intended for a coordinated release, so the Security artifact should not be released without the companion Neural Search change.

[github-actions]

Persistent review updated to latest commit 7994f47

[github-actions]

Persistent review updated to latest commit 80f8fe1