Skip to content
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.

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

Aliases: Throw exception if index is null when creating alias #7976

Closed
wants to merge 3 commits into from

Conversation

polyfractal
Copy link
Contributor

Fixes a bug where alias creation would allow null for index name, which thereby applied the alias to all indices. This patch makes the validator throw an exception if the index is null.

Fixes #7863

POST /_aliases
{
   "actions": [
      {
         "add": {
            "alias": "empty-alias",
            "index": null
         }
      }
   ]
}
{
   "error": "ActionRequestValidationException[Validation Failed: 1: Alias action [add]: [index] may not be null;]",
   "status": 400
}

Edit:

The reason this bug wasn't caught by the existing tests is because the old test for nullness only validated against a cluster which had zero indices. The null index is translated into "_all", and since there are no indices, this fails because the index doesn't exist. So the test passes.

However, as soon as you add an index, "_all" resolves and you get the situation described in the original bug report: null index is accepted by the alias, resolves to "_all" and gets applied to everything.

The Remove section independently checked the size of indices, but now that
it is being checked at the end of verification it is not needed inside the
Remove clause
The reason this bug wasn't caught by the existing tests is because the old test for nullness
only validated against a cluster which had zero indices.  The null index is translated into "_all",
and since there are no indices, this fails because the index doesn't exist.

However, as soon as you add an index, "_all" resolves and you get the situation described in the
original bug report (but which is not tested by the suite).
@rjernst
Copy link
Member

rjernst commented Oct 3, 2014

What is here looks good, but I think we should do more. Right now not passing any index at all results in using _all, which I think is very dangerous?

Ryans-MacBook-Pro:~ rjernst$ curl -i -XPOST 'http://localhost:9200/_aliases' -d '{"actions":[{"add":{"alias":"myalias"}}]}'
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Content-Length: 21

{"acknowledged":true}

@polyfractal
Copy link
Contributor Author

@rjernst Just looked into this, and it appears the patch will also protect against complete omission of the field.

The indices() method ensures that the array of indices is always initialized (even if empty), which means the new else clause introduced in the patch will throw an exception if indices is omitted entirely.

$ curl -XPOST "http://localhost:9200/_aliases" -d'
{
   "actions":[
      {
         "add":{
            "alias":"empty-alias"
         }
      }
   ]
}'

{
   "error": "ActionRequestValidationException[Validation Failed: 1: Alias action [add]: [index] may not be null;]",
   "status": 400
}

@rjernst
Copy link
Member

rjernst commented Oct 3, 2014

Cool! LGTM.

My only thought is maybe the error message should be more generic, since it could be a missing "index" key, or index set to null?

@s1monw
Copy link
Contributor

s1monw commented Oct 6, 2014

LGTM too agreed with @rjernst

@s1monw s1monw removed the review label Oct 6, 2014
polyfractal added a commit that referenced this pull request Oct 10, 2014
Fixes a bug where alias creation would allow `null` for index name, which thereby
applied the alias to _all_ indices.  This patch makes the validator throw an
exception if the index is null.

```bash
POST /_aliases
{
   "actions": [
      {
         "add": {
            "alias": "empty-alias",
            "index": null
         }
      }
   ]
}
```
```json
{
   "error": "ActionRequestValidationException[Validation Failed: 1: Alias action [add]: [index] may not be null;]",
   "status": 400
}
```

The reason this bug wasn't caught by the existing tests is because
the old test for nullness only validated against a cluster which had
zero indices.  The null index is translated into "_all", and since
there are no indices, this fails because the index doesn't exist.
 So the test passes.

However, as soon as you add an index, "_all" resolves and you get the
situation described in the original bug report:  null index is
accepted by the alias, resolves to "_all" and gets applied to everything.

Fixes #7976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants