Consistently prevent using exclusion prefix on its own by ywangd · Pull Request #139337 · elastic/elasticsearch

ywangd · 2025-12-11T00:41:05Z

The dash character is used as a prefix to signify index exclusion. It should not be used on its own. For example, an expression like *,- should be invalid. However, today this is handled differently depending on whether security is enabled or disabled. When security is enabled, the standalone exclusion char is ignored and the expression is handled as just *. But when security is disabled, the expression encounters an IndexNotFoundException.

This PR fixes the inconsistency by always throwing InvalidIndexNameException. In addition, this PR also fixes #45504 by ensuring InvalidIndexNameException is thrown for empty index name instead of IndexOutOfBoundsException.

Resolves: #45504

The dash character is used as a prefix to signify index exclusion. It should not be used on its own. For example, an expression like `*,-` should be invalid. However, today this is handled differently depending on whether security is enabled or disabled. When security is enabled, the standalone exclusion char is ignored and the expression is handled as just `*`. But when security is disabled, the expression encounters an IndexNotFoundException. This PR fixes the inconsistency by always throwing IllegalArgumentException.

…clusion

elasticsearchmachine · 2025-12-11T00:41:29Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2025-12-11T00:41:30Z

Hi @ywangd, I've created a changelog YAML for you.

github-actions · 2025-12-11T00:43:10Z

🔍 Preview links for changed docs

docs/reference/elasticsearch/rest-apis/api-conventions.md

github-actions · 2025-12-11T00:43:11Z

ℹ️ Important: Docs version tagging

👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version.

We use applies_to tags to mark version-specific features and changes.

Expand for a quick overview

When to use applies_to tags:

✅ At the page level to indicate which products/deployments the content applies to (mandatory)
✅ When features change state (e.g. preview, ga) in a specific version
✅ When availability differs across deployments and environments

What NOT to do:

❌ Don't remove or replace information that applies to an older version
❌ Don't add new information that applies to a specific version without an applies_to tag
❌ Don't forget that applies_to tags can be used at the page, section, and inline level

🤔 Need help?

Check out the cumulative docs guidelines
Reach out in the #docs Slack channel

ywangd · 2025-12-11T00:44:15Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java

+        if (localIndexExpression.isEmpty()) {
+            throw invalidExpression(false);
+        }


Currently, an empty expression encounters an IndexOutOfBoundsException because we call localIndexExpression.charAt(0) a few lines below. With security disabled, the error is IndexNotFoundException. I think it's better for them to both throw IllegalArgumentException. Strictly speaking, this change is not about exclusion. But since it's an issue of similar nature, I decide to include it as well.

Nice, I think this effectively fixes long-standing #45504 👍 Could you add a note to the PR description to close the issue once merged?

Ah good find. Updated the PR description as well as adding the issue to the changelog.

ywangd · 2025-12-11T00:48:25Z

In addition to index exclusion, there is also remote cluster exclusion. Currently, an expression such as -:foo encounters a NoSuchRemoteClusterException. We could decide to change the exception to IllegalArgumentException as well. But I'll leave it out for this PR.

ywangd · 2025-12-11T00:56:37Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/ReadActionsTests.java

+            final var e = expectThrows(ElasticsearchSecurityException.class, () -> trySearch(expressions).get());
+            assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));


IllegalArgumentException is always wrapped with ElasticsearchSecurityException. I think we could argue whether such wrapping is necessary. But I don't want to go down that path for now. Alternatively, if we want to avoid the wrapping, we could throw either InvalidIndexNameException or IndexNotFoundException instead. Please let me know if you prefer either of these.

On second thought, I decided to use InvalidIndexNameException which seems more specific for the particular issue. eb9498b

InvalidIndexNameException makes sense to me

…clusion

This reverts commit 9f7ecac.

n1v0lg

LGTM 👍

n1v0lg · 2025-12-15T11:08:09Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java

+        if (localIndexExpression.isEmpty()) {
+            throw invalidExpression(false);
+        }


Nice, I think this effectively fixes long-standing #45504 👍 Could you add a note to the PR description to close the issue once merged?

n1v0lg · 2025-12-15T11:09:45Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstractionResolver.java

        final Set<String> remoteExpressions
    ) {
+        if (localIndexExpression.isEmpty()) {
+            throw invalidExpression(false);


Nit: I'd add invalidExclusion() and invalidExpression() methods to make the call more self-explanatory (as opposed to the raw boolean arg)

Sure see aff7f77

n1v0lg · 2025-12-15T11:21:52Z

server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java

+            new String[] { "-" },
+            new String[] { "*", "-" },
+            new String[] { "testXXX", "-" },
+            new String[] { "*", "-", "testXXX" }


I'd also add:

new String[] { "-", "-" }, new String[] { "-testXXX", "-" }, new String[] { "*", "-testXXX", "-" }

Pushed 3468c92

n1v0lg · 2025-12-15T11:26:56Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/ReadActionsTests.java

+        createIndicesWithRandomAliases("test1");
+
+        final List<String[]> expressionsList = List.of(
+            new String[] { "-" },


I'd also add:

new String[] { "_all", "-" }, // interestingly this doesn't work in server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java since we don't allow `_` at that validation stage new String[] { "-", "-" }, new String[] { "-testXXX", "-" }, new String[] { "*", "-testXXX", "-" }

I think we should make IndexAbstractionResolver behave the same as IndexNameExpressionResolver for handling _all. It's inconsistent right now. The expression _all is a special match-all only when it is used on its own. Otherwise, it is just a normal name. As a normal name, it is invalid because index cannot begin with _. But with security enabled, an expression like foo*,_all ends up being just _all and expands to match all if foo* matches nothing. But if foo* matches anything, it then encounters InvalidIndexNameException. This is not a sensible behaviour and I think we should instead throw InvalidIndexNameException in both cases.

I added a test for { "_all", "-" } on the core side and assert it throws index name "must. not start with '_'". See 3468c92 along with your suggestion.

n1v0lg · 2025-12-15T11:28:04Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/ReadActionsTests.java

+        createIndicesWithRandomAliases("test1");
+
+        final List<String[]> expressionsList = List.of(
+            new String[] { "" },


Let's also cover:

new String[] { "", "_all" },

Yep see 3468c92

n1v0lg · 2025-12-15T13:25:51Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/ReadActionsTests.java

+            final var e = expectThrows(ElasticsearchSecurityException.class, () -> trySearch(expressions).get());
+            assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));


InvalidIndexNameException makes sense to me

…clusion

elasticsearchmachine · 2025-12-16T00:44:59Z

Hi @ywangd, I've updated the changelog YAML for you.

elasticsearchmachine · 2025-12-16T00:47:59Z

Hi @ywangd, I've updated the changelog YAML for you.

ywangd · 2025-12-16T01:23:08Z

CI Failure is irrelevant but reproducible. I raised #139576

Depends on: elastic#139337

ywangd added 2 commits December 11, 2025 11:36

Merge remote-tracking branch 'origin/main' into fix-bug-with-empty-ex…

98d3a67

…clusion

ywangd requested a review from tvernum December 11, 2025 00:41

ywangd added >bug :Security/Security Security issues without another label v9.3.0 labels Dec 11, 2025

elasticsearchmachine added the Team:Security Meta label for security team label Dec 11, 2025

Update docs/changelog/139337.yaml

733dc4e

ywangd commented Dec 11, 2025

View reviewed changes

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Dec 11, 2025

ywangd added 4 commits December 15, 2025 10:35

use invalidIndexNameException

9f7ecac

Merge remote-tracking branch 'origin/main' into fix-bug-with-empty-ex…

e553ed5

…clusion

Revert "use invalidIndexNameException"

f68b5bc

This reverts commit 9f7ecac.

actual changes

eb9498b

ywangd requested a review from a team December 14, 2025 23:50

fix test

817c2dc

n1v0lg requested review from n1v0lg and removed request for tvernum December 15, 2025 09:55

n1v0lg approved these changes Dec 15, 2025

View reviewed changes

ywangd added 4 commits December 16, 2025 10:18

Merge remote-tracking branch 'origin/main' into fix-bug-with-empty-ex…

e6934c2

…clusion

changelog

5bfc36c

separate methods

aff7f77

feedback

3468c92

ywangd added 2 commits December 16, 2025 11:44

Update docs/changelog/139337.yaml

bae396f

Update docs/changelog/139337.yaml

8b027bd

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 16, 2025

elasticsearchmachine merged commit b909322 into elastic:main Dec 16, 2025
41 checks passed

ywangd deleted the fix-bug-with-empty-exclusion branch December 16, 2025 04:10

breskeby pushed a commit to breskeby/elasticsearch that referenced this pull request Feb 11, 2026

Update test for new exception for empty index (elastic#5044)

613f5b2

Depends on: elastic#139337

breskeby pushed a commit to breskeby/elasticsearch that referenced this pull request Feb 11, 2026

Update test for new exception for empty index (elastic#5044)

3c8ab69

Depends on: elastic#139337

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistently prevent using exclusion prefix on its own#139337

Consistently prevent using exclusion prefix on its own#139337
elasticsearchmachine merged 14 commits intoelastic:mainfrom
ywangd:fix-bug-with-empty-exclusion

ywangd commented Dec 11, 2025 •

edited

Loading

elasticsearchmachine commented Dec 11, 2025

elasticsearchmachine commented Dec 11, 2025

github-actions bot commented Dec 11, 2025 •

edited

Loading

github-actions bot commented Dec 11, 2025

When to use applies_to tags:

What NOT to do:

ywangd Dec 11, 2025

n1v0lg Dec 15, 2025

ywangd Dec 16, 2025

ywangd commented Dec 11, 2025

ywangd Dec 11, 2025

ywangd Dec 14, 2025 •

edited

Loading

n1v0lg Dec 15, 2025

n1v0lg left a comment

n1v0lg Dec 15, 2025

n1v0lg Dec 15, 2025

ywangd Dec 16, 2025

n1v0lg Dec 15, 2025

ywangd Dec 16, 2025

n1v0lg Dec 15, 2025

ywangd Dec 16, 2025

n1v0lg Dec 15, 2025

ywangd Dec 16, 2025

n1v0lg Dec 15, 2025

elasticsearchmachine commented Dec 16, 2025

elasticsearchmachine commented Dec 16, 2025

ywangd commented Dec 16, 2025

Uh oh!

Labels

3 participants

		final var e = expectThrows(ElasticsearchSecurityException.class, () -> trySearch(expressions).get());
		assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));

Conversation

ywangd commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Dec 11, 2025

elasticsearchmachine commented Dec 11, 2025

github-actions bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Preview links for changed docs

github-actions bot commented Dec 11, 2025

ℹ️ Important: Docs version tagging

When to use applies_to tags:

What NOT to do:

🤔 Need help?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Dec 11, 2025

Choose a reason for hiding this comment

ywangd Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 16, 2025

elasticsearchmachine commented Dec 16, 2025

ywangd commented Dec 16, 2025

Uh oh!

Labels

3 participants

ywangd commented Dec 11, 2025 •

edited

Loading

github-actions bot commented Dec 11, 2025 •

edited

Loading

ywangd Dec 14, 2025 •

edited

Loading