-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Reject empty IDs #24118
Reject empty IDs #24118
Conversation
When indexing a document via the bulk API where IDs can be explicitly specified, we currently accept an empty ID. This is problematic because such a document can not be obtained via the get API. Instead, we should rejected these requets as accepting them could be a dangerous form of leniency. Additionally, we already have a way of specifying auto-generated IDs and that is to not explicitly specify an ID so we do not need a second way. This commit the individual requests where ID is specified but empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left one comment
@@ -499,6 +499,10 @@ public void process(@Nullable MappingMetaData mappingMd, String concreteIndex) { | |||
} | |||
} | |||
|
|||
if ("".equals(id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiousity, should we do Strings.hasText(id) == false
here, to disallow the id " "
also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should reject " " (at least in this PR), it can be obtained via the get API (with escaping) and rejecting it would be a breaking change. On the indexing API we take care to return a properly-escaped location header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, I think it falls into the larger input validation category anyway
* master: Use sequence numbers to identify out of order delivery in replicas & recovery (elastic#24060) Remove customization of ES_USER and ES_GROUP
When indexing a document via the bulk API where IDs can be explicitly specified, we currently accept an empty ID. This is problematic because such a document can not be obtained via the get API. Instead, we should rejected these requets as accepting them could be a dangerous form of leniency. Additionally, we already have a way of specifying auto-generated IDs and that is to not explicitly specify an ID so we do not need a second way. This commit rejects the individual requests where ID is specified but empty. Relates #24118
When indexing a document via the bulk API where IDs can be explicitly specified, we currently accept an empty ID. This is problematic because such a document can not be obtained via the get API. Instead, we should rejected these requets as accepting them could be a dangerous form of leniency. Additionally, we already have a way of specifying auto-generated IDs and that is to not explicitly specify an ID so we do not need a second way. This commit rejects the individual requests where ID is specified but empty. Relates #24118
When indexing a document via the bulk API where IDs can be explicitly specified, we currently accept an empty ID. This is problematic because such a document can not be obtained via the get API. Instead, we should rejected these requets as accepting them could be a dangerous form of leniency. Additionally, we already have a way of specifying auto-generated IDs and that is to not explicitly specify an ID so we do not need a second way. This commit rejects the individual requests where ID is specified but empty. Relates #24118
Thanks @dakrone. |
When indexing a document via the bulk API where IDs can be explicitly specified, we currently accept an empty ID. This is problematic because such a document can not be obtained via the get API. Instead, we should rejected these requets as accepting them could be a dangerous form of leniency. Additionally, we already have a way of specifying auto-generated IDs and that is to not explicitly specify an ID so we do not need a second way. This commit the individual requests where ID is specified but empty.
Closes #24116