support query-based terms lookup queries in DLS

[rursprung]

Description

OpenSearch introduced the possibility to use a query rather than just
an id in Terms Lookup Queries in opensearch-project/OpenSearch#18195 in release 3.2.0.
However, support for this was never added to the security plugin for
DLS/FLS.

When trying to use query based TLQ in a DLS this failed for two
reasons:

• the visible failure is an NPE because DlsFilterLevelActionHandler
  unconditionally dereferenced termsLookup().id(), which returns
  null for query-based lookups.
• additionally, the DocumentAllowList privilege bypass only handled
  GetRequest, but query-based TLQ internally resolves via
  SearchRequest and additionally uses a GetSettingsRequest, causing
  the privileges evaluator to block the lookup even after the NPE is
  fixed.

This commit adds proper support for query based TLQ in DLS, both for
the old and the new evaluator, by allowlisting a wildcard entry for the
whole index in question rather than a single document in case query is
used. additionally, SearchRequest and GetSettingsRequest are now
also supported in DocumentAllowList#isAllowed.

Furthermore, the legacy
PrivilegesEvaluatorImpl#checkDocAllowListHeader now delegates to
DocumentAllowList#isAllowed since the old method body was a
hand-inlined copy of the same logic (parse header, check request);
delegating removes the duplication and picks up SearchRequest support
without repeating the new code.

Header parsing in DocumentAllowList#isAllowed is now delegated to
DocumentAllowList#get to centralise error handling.

The wildcard allowlist pattern is already established for Dashboards
multi-tenancy and is scoped to the request's thread context, so it
cannot grant persistent or write access beyond the TLQ resolution.
Existing code with the wildcard has been migrated to use the new
constant to make it easier to find it.

additionally, DlsTermLookupQueryV4EvaluatorTest has been added (in a separate commit) to run the tests from DlsTermLookupQueryTest also against the new evaluator (this unearthed that GetSettingsRequest had to be whitelisted as well; the old evaluator didn't block this!).

Issues Resolved

fixes #6243

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end?
no

Testing

unit & integration testing + manual testing

Check List

• New functionality includes testing
• New functionality has been documented - shouldn't be needed since everyone will just expect this to work?
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java	523	medium	When termsLookup().id() returns null (query-based TLQ), ANY_DOCUMENT_ID ('*') is added to the document allowlist for the lookup index, granting read access to ALL documents in that index for sub-request resolution — broader than the specific-document case. If an attacker can craft a terms lookup query with a null ID in a non-query-based context, this could be used to bypass DLS by accessing arbitrary documents in the lookup index.
src/main/java/org/opensearch/security/privileges/DocumentAllowList.java	64	low	The isAllowed() method now grants privilege bypass to SearchRequest and GetSettingsRequest in addition to GetRequest. These are new internal privilege bypass paths that did not exist before. While the allMatch semantics in isIndicesAllowlisted() appear correct, expanding the set of bypassable request types in a security plugin is a broadened attack surface that warrants maintainer review.
src/test/resources/dlsfls/roles_tlq.yml	28	low	The new test role os_dls_tlq_query_lookup uses a DLS query containing '${user.name}' interpolated directly into a term query's _id lookup. If user.name is not properly sanitized before interpolation, this pattern could be abused in production roles to inject arbitrary query DSL via the username field.

The table above displays the top 10 most important findings.

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

---

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]

PR Reviewer Guide 🔍

(Review updated until commit d84e39e)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Privilege bypass scope: The PR introduces wildcard (*) document allowlist entries for query-based TLQ, which cause DlsFlsValveImpl and SecurityFlsDlsIndexSearcherWrapper to lift DLS restrictions entirely on the affected index during the TLQ resolution. The allowlist is scoped to the thread context, which limits blast radius, but the effect is stronger than for id-based TLQ (which only allowed a specific doc id). Reviewers should confirm that the internal SearchRequest issued for query-based TLQ cannot be influenced by end-user input in a way that lets a caller retrieve arbitrary documents from the lookup index via the TLQ path.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Overly permissive allowlisting isAllowed for SearchRequest and GetSettingsRequest only checks that all indices named in the request are covered by a wildcard allowlist entry. However, SearchRequest.indices() can contain wildcards/aliases, and a caller could craft a query-based TLQ that references an allowlisted index but the internal SearchRequest may expand to include other data. Since the wildcard entry (ANY_DOCUMENT_ID = "*") bypasses DLS entirely for the matched index (see DlsFlsValveImpl and SecurityFlsDlsIndexSearcherWrapper lifting DLS restriction when the allowlist matches), a user whose DLS role uses a query-based TLQ against index X will have DLS lifted for X during the TLQ resolution — confirm this cannot be abused to read documents from X beyond what the TLQ resolution needs (e.g., if user-controlled query content ends up executed against X). Uncertain: depends on how the resolver constructs the internal SearchRequest and whether its body is bounded. } else if (request instanceof SearchRequest searchRequest) { if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { if (isIndicesAllowlisted(documentAllowList, getSettingsRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } Duplicate map key indicesAndIds.put("tlqdocuments", "1") immediately followed by indicesAndIds.put("tlqdocuments", "2") overwrites the first entry, so the test effectively only checks a single id under tlqdocuments. This preexists but the test file is being modified; worth noting since the test does not exercise what its name suggests. Map<String, String> indicesAndIds = new HashMap<>(); indicesAndIds.put("tlqdocuments", "1"); indicesAndIds.put("tlqdocuments", "2");

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d84e39e

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify all-indices allowlist semantics for search isIndicesAllowlisted requires every index in the request to be allowlisted. For a query-based TLQ, the inner SearchRequest typically targets only the single lookup index, but if the request resolves multiple indices (aliases, wildcards expanded elsewhere) any not-in-allowlist index will cause a false result and reject the entire lookup. Consider whether "all indices must be allowlisted" is truly correct here, or if the lookup-index-only check should be used to avoid false negatives. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-77] } else if (request instanceof SearchRequest searchRequest) { + // Note: For TLQ-generated searches, the request should target only the lookup index. + // If future callers issue multi-index searches with the allowlist header, revisit this policy. if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about the strict all-indices allowlist semantics but only adds a comment without changing behavior. It's a mostly informational note rather than a concrete fix.	Low
	Avoid invoking known-failing super method The @Ignore annotation is placed after @Test, but the method still calls super.testMGet_1337() which is documented as expected to fail. If JUnit's ignore precedence changes or the annotation is removed accidentally, this test will fail. Consider making the body assert the V4-specific expected behavior (e.g., 403 response) instead of delegating to a superclass method known to fail. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("V4 evaluator returns 403 for _mget with concrete unauthorized index; behavior tested separately") public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Intentionally empty: V4 evaluator does not silently strip unauthorized indices from + // explicit _mget requests, so the legacy test expectations do not apply here. } Suggestion importance[1-10]: 3 __ Why: Since the test is @Ignored, JUnit will not execute the body, making the call to super.testMGet_1337() effectively dead code. The suggestion is a minor code quality improvement rather than a functional fix.	Low

---

Previous suggestions

Suggestions up to commit dd84988

Category	Suggestion	Impact
Security	Validate lookup index before allowlisting When lookupId is null (query-based TLQ), the entire index is allowlisted with ANY_DOCUMENT_ID, which grants the user implicit read access to all documents in lookupIndex via internal sub-requests. Consider validating that lookupIndex is not user-controllable in a way that could escalate privileges, or scope the allowlist more narrowly (e.g., by also tracking the query) to limit the bypass surface. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +if (lookupIndex == null || lookupIndex.isEmpty()) { + continue; +} if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { documentAllowlist.add(lookupIndex, DocumentAllowList.ANY_DOCUMENT_ID); } Suggestion importance[1-10]: 4 __ Why: Adding a null/empty check for lookupIndex is a reasonable defensive measure, but termsLookup().index() is typically required and non-null in valid TLQ definitions. The suggestion provides marginal hardening.	Low
	Guard against empty indices in allowlist check isIndicesAllowlisted returns false when indices is null or empty, which means a SearchRequest without any indices specified (interpreted as "all indices" by OpenSearch) would be incorrectly allowlisted only if it explicitly listed the indices. Worse, an attacker could craft a SearchRequest with no indices to broaden scope. Ensure that an empty indices array does not lead to unintended bypass and verify whether SearchRequest.indices() semantics here require explicit, non-empty allowlisted indices (current behavior returns false, which is safer—double-check this matches the TLQ resolution path actually invoked). src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-83] } else if (request instanceof SearchRequest searchRequest) { - if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + String[] indices = searchRequest.indices(); + if (indices != null && indices.length > 0 && isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { - if (isIndicesAllowlisted(documentAllowList, getSettingsRequest.indices())) { + String[] indices = getSettingsRequest.indices(); + if (indices != null && indices.length > 0 && isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } Suggestion importance[1-10]: 3 __ Why: The isIndicesAllowlisted helper already returns false for null or empty indices arrays, so the suggested change is redundant. The score reflects only a minor defensive coding improvement.	Low
General	Avoid invoking failing super in ignored test A test that is expected to fail under V4 should either assert the new (403) behavior explicitly or be removed/ignored permanently with rationale. Calling super.testMGet_1337() inside an @Ignored method is dead code; if the goal is to document the V4 behavior, replace with an actual test asserting the 403 response so regressions in V4 are detected. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("V4 evaluator returns 403 for explicit _mget on unauthorized index; legacy behavior covered by parent test") public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Intentionally not invoking super; legacy DNFOF behavior does not apply to V4. } Suggestion importance[1-10]: 3 __ Why: Since the test is annotated with @Ignore, the body never executes, so calling super.testMGet_1337() is harmless dead code. The suggestion is a minor cleanup with low impact.	Low

Suggestions up to commit 55eb985

Category	Suggestion	Impact
Security	Guard against wildcard-allowlisting protected indices Granting a wildcard ANY_DOCUMENT_ID allowlist on the lookup index whenever id is null means any query-based TLQ effectively lifts DLS restrictions on that entire index for the duration of the request. Ensure the lookup index is not the same as the index being queried (which would silently lift DLS on the user's data index); add a guard or document this constraint to avoid privilege escalation. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { + // Query-based TLQ: lookup index gets wildcard allowlist. Ensure lookup index is distinct + // from any index whose DLS should remain enforced. documentAllowlist.add(lookupIndex, DocumentAllowList.ANY_DOCUMENT_ID); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a legitimate security concern about wildcard allowlisting on a lookup index, but the improved_code only adds a comment without adding an actual guard, so the impact is mostly documentation.	Low
General	Clarify handling of empty indices array isIndicesAllowlisted returns false when indices is null/empty, but a SearchRequest with no explicit indices typically targets all indices (_all). This may incorrectly deny a sub-search that the security layer expanded earlier; conversely, treating empty as "match all" must not bypass checks. Verify the expected behavior and at minimum log/handle the empty-indices case explicitly so query-based TLQ sub-searches built without index names are not silently rejected. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-76] } else if (request instanceof SearchRequest searchRequest) { - if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + String[] indices = searchRequest.indices(); + if (indices == null || indices.length == 0) { + log.debug("SearchRequest with no indices not allowlisted: {}", request); + return false; + } + if (isIndicesAllowlisted(documentAllowList, indices)) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { Suggestion importance[1-10]: 4 __ Why: The suggestion raises a potentially valid concern about empty indices arrays in SearchRequest, but the improved code is functionally equivalent to existing code (both return false) and just adds a debug log, making the impact minimal.	Low
	Assert expected V4 behavior instead of ignoring The @Ignore annotation must come before @Test (or replace it) and JUnit 4's @Ignore order matters less, but invoking super.testMGet_1337() inside a method annotated @Ignore is fine — however, if the intent is to skip, ensure the method body is not executed. Currently @Ignore will skip it, but the comment says "expected to fail"; consider using an assertion of the expected failure (e.g., expect a 403) so regressions are detected, rather than silently ignoring. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") public void testMGet_1337() throws Exception { // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Concrete unauthorized index -> 403 expected. Override to assert this behavior instead of + // running the legacy expectations. + setupWithTlqSecurityConfig(); + // ... assert 403 / expected V4 behavior here } Suggestion importance[1-10]: 3 __ Why: The suggestion is somewhat valid in noting that @Ignore skips the test silently, but the improved_code is incomplete (just a placeholder) and the existing approach is a reasonable temporary measure documented with comments.	Low

Suggestions up to commit a8dcab5

Category	Suggestion	Impact
Security	Restrict wildcard fallback to query-based lookups Adding a wildcard-document allowlist entry whenever lookupId is null grants bypass to the entire lookup index for any TLQ that omits an id, including ones that may not actually use a query (could result in a broader bypass than intended). Verify a query is actually present on the TermsLookup before falling back to wildcard, otherwise restrict to id-based entries to avoid unintended index-wide privilege escalation. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +final var lookupQuery = termsQueryBuilder.termsLookup().query(); if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); -} else { +} else if (lookupQuery != null) { documentAllowlist.add(lookupIndex, DocumentAllowList.WILDCARD_DOCUMENT_ID); } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive suggestion to verify a query exists on the TermsLookup before adding a wildcard allowlist entry, though the surrounding filter already requires termsLookup() != null so the practical risk is lower.	Low
Possible issue	Handle alias/wildcard indices in allowlist check SearchRequest and GetSettingsRequest accept index patterns/aliases (e.g. wildcards like user_*), but isIndicesAllowlisted does exact-string matching against the allowlist. A TLQ-driven sub-request that uses an alias or pattern instead of a concrete index name will fail the bypass check. Resolve indices against cluster state (or restrict allowed bypass to concrete index names only) before comparing, otherwise legitimate query-based TLQ lookups against aliases will be denied. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-83] } else if (request instanceof SearchRequest searchRequest) { + // Note: only concrete index names are supported; aliases/patterns must be resolved by caller if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } else if (request instanceof GetSettingsRequest getSettingsRequest) { if (isIndicesAllowlisted(documentAllowList, getSettingsRequest.indices())) { log.debug("Request {} is allowed by {}", request, documentAllowList); return true; } return false; } Suggestion importance[1-10]: 5 __ Why: Raises a legitimate concern that aliases/patterns are not resolved before allowlist matching, but the improved_code only adds a comment and does not actually fix the issue, limiting its impact.	Low
General	Remove dead super-call in ignored test Calling super.testMGet_1337() inside an @Ignored test is dead code and misleading; with @Ignore the body never executes. Either drop the override entirely (so the parent test runs and passes/fails on its own) or, if it must be skipped under V4, leave only the @Ignore with no body and a comment, to make intent clear. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") -public void testMGet_1337() throws Exception { - // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); +@Ignore("V4 evaluator does not silently strip unauthorized indices from explicit _mget requests.") +public void testMGet_1337() { + // Intentionally skipped under V4 evaluator. } Suggestion importance[1-10]: 2 __ Why: Minor cleanup of misleading dead code in an ignored test; has no functional impact.	Low

Suggestions up to commit ab9eb03

Category	Suggestion	Impact
Possible issue	Handle wildcard index expressions in allowlist check SearchRequest.indices() may contain wildcards/patterns/aliases (e.g., , tlq) that won't literally match an entry like my_index in the allowlist, causing legitimate query-based TLQ sub-search requests on resolved index names to be rejected. Consider matching using pattern semantics or only allowlisting based on the resolved concrete indices, otherwise TLQ may break for any non-literal index expression. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [71-76] +} else if (request instanceof SearchRequest searchRequest) { + if (isIndicesAllowlisted(documentAllowList, searchRequest.indices())) { + log.debug("Request {} is allowed by {}", request, documentAllowList); + return true; + } + return false; +} else if (request instanceof GetSettingsRequest getSettingsRequest) { - Suggestion importance[1-10]: 5 __ Why: Valid concern that wildcard/pattern index expressions in SearchRequest.indices() may not literally match allowlist entries, but the suggestion does not provide a concrete fix and the actual TLQ sub-requests typically use resolved concrete index names.	Low
General	Avoid invoking superclass body in ignored test The overridden test calls super.testMGet_1337() directly, which means if @Ignore is ever removed or the runner ignores it differently, the parent assertions will run unchanged and fail rather than asserting the expected V4 behavior (403). Either delete the body entirely (since @Ignore skips it) or replace it with assertions that capture the actual expected V4 behavior so the documented difference is properly verified. src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryV4EvaluatorTest.java [31-38] @Override @Test -@Ignore("expected to fail with V4 evaluator") +@Ignore("expected to fail with V4 evaluator - see comment") public void testMGet_1337() throws Exception { // V4 evaluator does not silently strip unauthorized indices from explicit _mget requests. - // Legacy uses DNFOF_MATCHER on "indices:data/read/*" to reduce indices; V4 only reduces - // for wildcard/pattern requests or ignore_unavailable=true. Concrete unauthorized index -> 403. - super.testMGet_1337(); + // Concrete unauthorized index -> 403, unlike legacy DNFOF behavior. } Suggestion importance[1-10]: 4 __ Why: The @Ignore annotation prevents execution, so calling super.testMGet_1337() is harmless but slightly misleading. Minor cleanup with low impact.	Low
	Guard against null lookup index When lookupId is null (query-based TLQ), the entire lookup index is allowlisted with a wildcard, which broadens access more than necessary—any document in the lookup index becomes readable via the bypass path during this request. Consider also verifying lookupIndex is non-null and document the wider trust scope, or restrict the bypass strictly to the search sub-request executing the lookup query. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] final var lookupIndex = termsQueryBuilder.termsLookup().index(); final var lookupId = termsQueryBuilder.termsLookup().id(); +if (lookupIndex == null) { + continue; +} if (lookupId != null) { documentAllowlist.add(lookupIndex, lookupId); } else { documentAllowlist.add(lookupIndex, DocumentAllowList.WILDCARD_DOCUMENT_ID); } Suggestion importance[1-10]: 3 __ Why: termsLookup().index() is generally required for valid terms lookup queries, so a null check is defensive but unlikely to be needed. Low impact.	Low

Suggestions up to commit faeba2f

Category	Suggestion	Impact
Possible issue	Add null check for lookup index Add null check for lookupIndex before using it. If termsLookup().index() returns null, calling documentAllowlist.add() with a null index could cause unexpected behavior or exceptions downstream. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [520-527] String lookupIndex = termsQueryBuilder.termsLookup().index(); String lookupId = termsQueryBuilder.termsLookup().id(); -if (lookupId != null) { - documentAllowlist.add(lookupIndex, lookupId); -} else { - documentAllowlist.add(lookupIndex, "*"); +if (lookupIndex != null) { + if (lookupId != null) { + documentAllowlist.add(lookupIndex, lookupId); + } else { + documentAllowlist.add(lookupIndex, "*"); + } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion to add null check for lookupIndex. While termsLookup().index() could potentially return null, the suggestion correctly identifies a defensive programming opportunity to prevent potential NPE or unexpected behavior in documentAllowlist.add().	Medium
	Handle null elements in indices array Add null check for individual index elements in the array. The indices array could contain null elements, which would cause isAllowed() to fail or behave unexpectedly when checking allowlist entries. src/main/java/org/opensearch/security/privileges/DocumentAllowList.java [82-92] private static boolean isIndicesAllowlisted(DocumentAllowList documentAllowList, String[] indices) { if (indices == null || indices.length == 0) { return false; } for (String index : indices) { - if (!documentAllowList.isAllowed(index, "*")) { + if (index == null || !documentAllowList.isAllowed(index, "*")) { return false; } } return true; } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive programming suggestion. While IndicesRequest.indices() typically doesn't return arrays with null elements, adding a null check for individual index elements prevents potential issues if the array contains nulls, improving code robustness.	Low

[github-actions]

Persistent review updated to latest commit ab9eb03

[github-actions]

Persistent review updated to latest commit a8dcab5

[rursprung]

FYI: the presence of this is why the legacy evaluator allowed GetSettingsRequest

[rursprung]

the CI failure is a false reject (can't fetch maven snapshots). i've seen this happen a lot lately!

[rursprung]

CC @srikanthpadakanti (you implemented query support in TLQ in OpenSearch) and @jochenkressin & @nibix (you implemented/contributed the original TLQ-in-DLS support)

[rursprung]

from what i can see all CI failures are false rejects: they either failed due to not being able to fetch maven artefacts (i've seen that a lot now on PRs on this repo - what's going on here?) - or completely unrelated tests failing => probably nothing that i can help sort out here?

[github-actions]

Persistent review updated to latest commit 55eb985

[github-actions]

Persistent review updated to latest commit dd84988

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.20%. Comparing base (757b857) to head (dd84988).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...nsearch/security/privileges/DocumentAllowList.java	77.27%	1 Missing and 4 partials ⚠️
...search/security/configuration/DlsFlsValveImpl.java	50.00%	0 Missing and 1 partial ⚠️
...figuration/SecurityFlsDlsIndexSearcherWrapper.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6244      +/-   ##
==========================================
+ Coverage   75.02%   75.20%   +0.18%     
==========================================
  Files         451      451              
  Lines       29248    29249       +1     
  Branches     4407     4410       +3     
==========================================
+ Hits        21942    21996      +54     
+ Misses       5270     5209      -61     
- Partials     2036     2044       +8     

Files with missing lines	Coverage Δ
...ity/configuration/DlsFilterLevelActionHandler.java	57.58% <100.00%> (+0.77%)	⬆️
...es/actionlevel/legacy/PrivilegesEvaluatorImpl.java	87.01% <100.00%> (+1.11%)	⬆️
...eges/actionlevel/legacy/PrivilegesInterceptor.java	73.33% <100.00%> (ø)
...tgen/DashboardsMultitenancySystemIndexHandler.java	88.41% <100.00%> (ø)
...search/security/configuration/DlsFlsValveImpl.java	67.81% <50.00%> (ø)
...figuration/SecurityFlsDlsIndexSearcherWrapper.java	79.66% <0.00%> (ø)
...nsearch/security/privileges/DocumentAllowList.java	69.88% <77.27%> (+22.29%)	⬆️

... and 8 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.

[nibix]

@rursprung

Thank you for this and sorry for the late response; at the moment, I have quite a few different things going on.

[rursprung]

if we can't reach an understanding of how the whitelisting for the index targeted by the subquery should work before we hit the code freeze for 3.8.0 would it also be ok if i'd split this in two: this PR would only add support for running query-based TLQs in DLS and the authorization part would be a new issue. then it'd already be possible to use query-based TLQ in DLS as long as the user is granted read access to the index targeted by the subquery (a workable workaround which could be documented as such).

[github-actions]

Persistent review updated to latest commit d84e39e