Skip to content

Mark empty _terms_enum results due to DLS as incomplete (#91720) #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MitchLewis930 wants to merge 1 commit into
base: pr_012_before
Choose a base branch
from
Open

Mark empty _terms_enum results due to DLS as incomplete (#91720) #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/91720.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 91720
summary: Mark empty `_terms_enum` results due to DLS as incomplete
area: Search
type: enhancement
issues:
- 88321
15 changes: 14 additions & 1 deletion ...src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,20 @@ private void asyncNodeOperation(NodeTermsEnumRequest request, Task task, ActionL
ThreadContext threadContext = transportService.getThreadPool().getThreadContext();
final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
for (ShardId shardId : request.shardIds().toArray(new ShardId[0])) {
if (canAccess(shardId, request, frozenLicenseState, threadContext) == false || canMatchShard(shardId, request) == false) {
if (canAccess(shardId, request, frozenLicenseState, threadContext) == false) {
listener.onResponse(
new NodeTermsEnumResponse(
request.nodeId(),
Collections.emptyList(),
"cannot execute [_terms_enum] request on index ["
+ shardId.getIndexName()
+ "] due to "
+ "DLS/FLS security restrictions.",
false
)
);
}
if (canMatchShard(shardId, request) == false) {
// Permission denied or can't match, remove shardID from request
request.remove(shardId);
}
Comment on lines +718 to 734
Copy link

@coderabbitai coderabbitai bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Missing return after early failure response causes undefined behavior.

When canAccess() returns false, the code sends a response to the listener but does not exit the loop or method. Execution continues to canMatchShard() and potentially sends additional responses. This leads to:

  1. Multiple responses sent to the same ActionListener
  2. If multiple shards fail access checks, multiple error responses are sent
  3. After the loop, another response may be sent at line 737 or via the executor at line 746
🐛 Proposed fix: Add return statement after early failure response 
             if (canAccess(shardId, request, frozenLicenseState, threadContext) == false) {
                 listener.onResponse(
                     new NodeTermsEnumResponse(
                         request.nodeId(),
                         Collections.emptyList(),
                         "cannot execute [_terms_enum] request on index ["
                             + shardId.getIndexName()
                             + "] due to "
                             + "DLS/FLS security restrictions.",
                         false
                     )
                 );
+                return;
             }
             if (canMatchShard(shardId, request) == false) {
-                // Permission denied or can't match, remove shardID from request
+                // Can't match index filter, remove shardID from request
                 request.remove(shardId);
             }

Additionally, the comment on line 732 mentions "Permission denied" but that condition is now handled separately above. Consider updating the comment to only reference the index filter check.

🤖 Prompt for AI Agents 
In
`@x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java`
around lines 718 - 734, In TransportTermsEnumAction (inside the shard iteration
where canAccess(shardId, request, frozenLicenseState, threadContext) is checked)
add an immediate return after calling listener.onResponse(...) so the
method/loop exits and no further responses are sent; also update the nearby
comment above the canMatchShard(shardId, request) check (currently mentioning
"Permission denied") to accurately reflect that this branch only handles index
filter/match failures. Ensure the change is made where the NodeTermsEnumResponse
is constructed and where request.remove(shardId) is invoked so no duplicate
listener responses occur.

Expand Down
31 changes: 22 additions & 9 deletions x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/terms_enum/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,18 +549,25 @@ teardown:
- length: { terms: 1 }

- do:
headers: { Authorization: "Basic ZGxzX3NvbWVfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # dls_some_user sees selected docs
headers: { Authorization: "Basic ZGxzX3NvbWVfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # dls_some_user doesn't see all docs
terms_enum:
index: test_security
body: {"field": "foo", "string":"b"}
- length: {terms: 0}
index: test_security
body: { "field": "foo", "string": "b" }

- length: { terms: 0 }
- match: { complete: false }
- match: { _shards.failed: 1 }
- match: { _shards.failures.0.reason.type: "broadcast_shard_operation_failed_exception" }
- match: { _shards.failures.0.reason.reason: "cannot execute [_terms_enum] request on index [test_security] due to DLS/FLS security restrictions." }

- do:
headers: { Authorization: "Basic ZmxzX3VzZXI6eC1wYWNrLXRlc3QtcGFzc3dvcmQ=" } # fls_user can't see field
terms_enum:
index: test_security
body: {"field": "foo", "string":"b"}
- length: {terms: 0}
index: test_security
body: { "field": "foo", "string": "b" }
- length: { terms: 0 }
- match: { complete: true }
- match: { _shards.failed: 0 }

---
"Test security with API keys":
Expand Down Expand Up @@ -612,14 +619,17 @@ teardown:
}
}
- match: { name: "dls_all_user_bad_key" }
- set: { encoded: login_creds}
- set: { encoded: login_creds }
- do:
headers:
Authorization: ApiKey ${login_creds} # dls_all_user bad API key sees selected docs
terms_enum:
index: test_security
body: { "field": "foo", "string": "b" }
- length: { terms: 0 }
- match: { complete: false }
- match: { _shards.failed: 1 }
- match: { _shards.failures.0.reason.type: "broadcast_shard_operation_failed_exception" }

- do:
headers: { Authorization: "Basic ZGxzX3NvbWVfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" } # dls_some_user
Expand All @@ -641,11 +651,14 @@ teardown:
}
}
- match: { name: "dls_some_user_key" }
- set: { encoded: login_creds}
- set: { encoded: login_creds }
- do:
headers:
Authorization: ApiKey ${login_creds} # dls_some_user's API key sees selected user regardless of the key's role descriptor
terms_enum:
index: test_security
body: { "field": "foo", "string": "b" }
- length: { terms: 0 }
- match: { complete: false }
- match: { _shards.failed: 1 }
- match: { _shards.failures.0.reason.type: "broadcast_shard_operation_failed_exception" }