Giving informative error messages for double slashes in API call URLs #1568

meghasaik · 2021-11-16T23:02:06Z

Signed-off-by: Megha Sai Kavikondala [email protected]

Changes made for giving more informative error messages.

Description

If a URL with an empty index name or having double slash is requested, a pretty random answer is given:

$ curl -X PUT -H 'Content-type: application/json' localhost:9200//_cluster/settings?pretty -d '{}'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "string_index_out_of_bounds_exception",
        "reason" : "String index out of range: 0"
      }
    ],
    "type" : "string_index_out_of_bounds_exception",
    "reason" : "String index out of range: 0"
  },
  "status" : 500
}

Thus, rather than giving a random answer, an informative error message is given:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "invalid_index_name_exception",
        "reason" : "Invalid index name [], reason: Invalid index name [], must not be empty",
        "index" : "",
        "index_uuid" : "_na_"
      }
    ],
    "type" : "invalid_index_name_exception",
    "reason" : "Invalid index name [], reason: Invalid index name [], must not be empty",
    "index" : "",
    "index_uuid" : "_na_"
  },
  "status" : 400
}

Issues Resolved

[List any issues this PR will resolve]

#1499

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

opensearch-ci-bot · 2021-11-16T23:04:04Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2021-11-16T23:04:21Z

✅ Gradle Wrapper Validation success e0dcddc381a7535319c5047a66f0f7bc4ad8b480

meghasaik · 2021-11-16T23:05:07Z

start gradle check

opensearch-ci-bot · 2021-11-16T23:11:17Z

❌ Gradle Precommit failure e0dcddc381a7535319c5047a66f0f7bc4ad8b480
Log 1570

opensearch-ci-bot · 2021-11-16T23:18:24Z

❌ Gradle Check failure e0dcddc381a7535319c5047a66f0f7bc4ad8b480
Log 1102

Reports 1102

opensearch-ci-bot · 2021-11-16T23:23:40Z

❌ Gradle Check failure e0dcddc381a7535319c5047a66f0f7bc4ad8b480
Log 1103

Reports 1103

opensearch-ci-bot · 2021-11-17T00:31:52Z

✅ Gradle Wrapper Validation success bbba7db99fc2f4dc210f340b6e198df0011ef847

opensearch-ci-bot · 2021-11-17T00:36:09Z

✅ Gradle Precommit success bbba7db99fc2f4dc210f340b6e198df0011ef847

opensearch-ci-bot · 2021-11-17T00:55:22Z

❌ Gradle Check failure bbba7db99fc2f4dc210f340b6e198df0011ef847
Log 1105

Reports 1105

meghasaik · 2021-11-17T17:08:49Z

start gradle check

opensearch-ci-bot · 2021-11-17T17:54:06Z

✅ Gradle Check success bbba7db99fc2f4dc210f340b6e198df0011ef847
Log 1112

Reports 1112

andrross · 2021-11-17T18:57:12Z

server/src/main/java/org/opensearch/rest/BytesRestResponse.java

@@ -107,22 +109,30 @@ public BytesRestResponse(RestChannel channel, Exception e) throws IOException {

    public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException {
        ToXContent.Params params = paramsFromRequest(channel.request());
+        boolean doubleSlashPresent = checkForDoubleSlash(channel.request().rawPath());


The double slash check strikes me as a special case when it might be better to handle this more generally. It looks to me that the system correctly parsed the request and identified that the request specified the empty string as the index name. Where it went wrong is that it surfaced an "index out of bounds" failure as opposed to a more sensible parameter validation error (i.e. "No index name specified"). I'm concerned that this double slash check will have unexpected behavior, such as if there are cases where a double slash somewhere in the URL is either acceptable or at least innocuous and this change would be a backwards incompatible change in behavior. Can you fix this bug by doing more strict checks of the index name itself as opposed to looking for double slashes in the URL path?

It's correct that checking double slash is not a good solution, because currently the REST APIs have got the leniency to allow double slash in URL.
However, after taking a deeper look at the error message. I don't think the system identified that the request contains an empty "index" name.

There is no PUT <index>/_cluster/health API, and as the author of the issue said, adding a leading / to any valid PUT /_x/y APIs will result that error message.

The error message only occurs using curl command, but will not occur when using the in Dashboards Console (https://www.elastic.co/guide/en/kibana/7.10/console-kibana.html)

Looks like the string_index_out_of_bounds_exception comes from Java (https://docs.oracle.com/javase/8/docs/api/java/lang/StringIndexOutOfBoundsException.html), and are parsed by OpenSearch to add the underscores (https://github.com/opensearch-project/OpenSearch/blob/1.1.0/server/src/main/java/org/opensearch/OpenSearchException.java#L688)

So I think we should find a better solution for this issue. 😁

I agree with the checking double slash as not being a good solution. Also, as the system does not identify the empty index name as well as the author opening the issue related to empty index or double slash, checking for it's value being null is the change, I have made. Correct me if I am wrong here.

opensearch-ci-bot · 2021-11-18T00:27:57Z

✅ Gradle Wrapper Validation success aabf91b2ec379726b63f765d945d9a03e1c2d5c7

opensearch-ci-bot · 2021-11-18T00:31:42Z

✅ Gradle Precommit success aabf91b2ec379726b63f765d945d9a03e1c2d5c7

opensearch-ci-bot · 2021-11-18T01:11:27Z

✅ Gradle Check success aabf91b2ec379726b63f765d945d9a03e1c2d5c7
Log 1118

Reports 1118

andrross · 2021-11-18T15:33:13Z

server/src/main/java/org/opensearch/rest/BytesRestResponse.java

@@ -107,22 +110,30 @@ public BytesRestResponse(RestChannel channel, Exception e) throws IOException {

    public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException {
        ToXContent.Params params = paramsFromRequest(channel.request());
+        boolean emptyIndex = checkForEmptyIndex(channel.request().params());


I don't think this is quite the right place to be doing the empty index check because this class doesn't know anything about the specific operation that has happened so again there is risk that there are some operations where an empty index is okay and this would be a breaking change. Ideally the parameter validation should happen on the request path in a class that understands the operation being performed and can make assertions about what constitutes valid parameters for that operation. I'd suggest finding the exact spot where the index out of bounds error occurs and see if you can add the validation at or before that point on the request path. Another really good practice in a case like this is to start by writing a test that fails and then go implement the fix.

Also, as Tianli helpfully pointed out above, it may not have anything to do with an empty index parameter name, but the error message does suggest that somewhere we are attempting to do something like String.substring on an empty string or otherwise failing while attempting to parse a string. If you can isolate the exact point where that is happening then it will probably be much easier to figure out the appropriate fix.

Changes related to Informative error messages.

opensearch-ci-bot · 2021-11-20T01:49:51Z

✅ Gradle Wrapper Validation success d0a2c9a

opensearch-ci-bot · 2021-11-20T01:52:56Z

❌ Gradle Check failure d0a2c9a
Log 1158

Reports 1158

opensearch-ci-bot · 2021-11-20T01:53:28Z

❌ Gradle Precommit failure d0a2c9a
Log 1604

Spotless changes

opensearch-ci-bot · 2021-11-20T01:58:00Z

✅ Gradle Wrapper Validation success 9f3a4f8

opensearch-ci-bot · 2021-11-20T02:05:09Z

✅ Gradle Precommit success 9f3a4f8

opensearch-ci-bot · 2021-11-20T02:22:20Z

❌ Gradle Check failure 9f3a4f8
Log 1159

Reports 1159

meghasaik · 2021-11-20T02:26:01Z

start gradle check

opensearch-ci-bot · 2021-11-20T03:08:48Z

✅ Gradle Check success 9f3a4f8
Log 1160

Reports 1160

andrross

Nice job!

andrross · 2021-11-22T16:34:21Z

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java

@@ -250,6 +250,13 @@ public static void validateIndexOrAliasName(String index, BiFunction<String, Str
        if (!Strings.validFileName(index)) {
            throw exceptionCtor.apply(index, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
        }
+        if (index.isEmpty()) {
+            logger.info(() -> new ParameterizedMessage("Empty Index, presence of double slash maybe?"));


No need for the ParameterizedMessage here, you can just pass a string directly.

I will change that.

andrross · 2021-11-22T16:40:46Z

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java

+            logger.info(() -> new ParameterizedMessage("Empty Index, presence of double slash maybe?"));
+            throw exceptionCtor.apply(
+                index,
+                "reason: empty string is an invalid index name (do you have a double slash in the URL by accident?)"


Bit of a nitpick, but I'd match the style of the existing error messages and just say something like "must not be empty". The resulting error message will be "Invalid index name [], must not be empty"
which seems pretty concise and clear to me.

And while I realize the request specifically asked for it in the bug report, I'd avoid including the text about a double slash because it only makes sense if the user is interacting with this API directly via REST API. I suspect it is possible to pass an empty index name via other clients and mentioning something about a double slash in the error message may lead to more confusion.

It makes sense to have it about the empty index name rather than the double slashes.

dblock · 2021-11-22T17:55:50Z

The invalid/empty index name error is great. But it feels like we should be able to check for a double-slash and throw a specific message if this is a common-enough problem, rather than adding "maybe you have a // in the message. WDYT?

Related, any reason why we shouldn't redirect //whatever to /whatever with a 301 if that's a common issue? Do other web servers do anything like that?

meghasaik · 2021-11-22T19:17:55Z

I think the double slash might be limited to the REST API only, Correct me if I am wrong on that. So checking on the empty index name might be better. I agree on your point of adding specific message rather than "// message".
I have not much idea about the other web servers but as //whatever relates to the empty index name, a 400 Bad request might be a better case than a 301 redirect.

opensearch-ci-bot · 2021-11-22T19:23:26Z

✅ Gradle Wrapper Validation success 7d6fdbff0374fe28f5ffbf7ced5595dfb782901a

opensearch-ci-bot · 2021-11-22T19:31:04Z

✅ Gradle Precommit success 7d6fdbff0374fe28f5ffbf7ced5595dfb782901a

opensearch-ci-bot · 2021-11-22T20:00:38Z

✅ Gradle Check success 7d6fdbff0374fe28f5ffbf7ced5595dfb782901a
Log 1177

Reports 1177

andrross · 2021-11-22T20:22:39Z

server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java

@@ -250,6 +250,10 @@ public static void validateIndexOrAliasName(String index, BiFunction<String, Str
        if (!Strings.validFileName(index)) {
            throw exceptionCtor.apply(index, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
        }
+        if (index.isEmpty()) {
+            logger.info("Invalid Index Name [], must not be empty");


Should probably omit the log statement. The other cases in this method do not log here. A misconfigured client could cause your log to grow to a huge size if you log every invalid request.

naming and message changes. Signed-off-by: Megha Sai Kavikondala <[email protected]>

Deleting the log message.

opensearch-ci-bot · 2021-11-22T21:08:24Z

✅ Gradle Wrapper Validation success c5d588f

opensearch-ci-bot · 2021-11-22T21:14:46Z

✅ Gradle Precommit success c5d588f

opensearch-ci-bot · 2021-11-22T21:50:10Z

✅ Gradle Check success c5d588f
Log 1178

Reports 1178

dblock · 2021-11-22T23:01:48Z

Backport to 1.x too please.

meghasaik · 2021-11-22T23:03:32Z

Will do that.

dblock · 2021-12-06T13:19:48Z

Does this still need to be backported?

meghasaik · 2021-12-06T17:03:09Z

This has been backported.

dblock · 2021-12-06T17:21:22Z

@meghasaik Can you please link the PR?

meghasaik · 2021-12-06T17:26:17Z

This is the BackPort PR link: #1601

meghasaik requested a review from a team as a code owner November 16, 2021 23:02

meghasaik requested review from kartg and tlfeng November 16, 2021 23:03

andrross reviewed Nov 17, 2021

View reviewed changes

andrross reviewed Nov 18, 2021

View reviewed changes

Signed-off-by: Megha Sai Kavikondala <[email protected]>

d0a2c9a

Changes related to Informative error messages.

Signed-off-by: Megha Sai Kavikondala <[email protected]>

9f3a4f8

Spotless changes

andrross reviewed Nov 22, 2021

View reviewed changes

meghasaik added 2 commits November 22, 2021 20:59

Signed-off-by: Megha Sai Kavikondala <[email protected]>

6744df3

naming and message changes. Signed-off-by: Megha Sai Kavikondala <[email protected]>

Signed-off-by: Megha Sai Kavikondala <[email protected]>

c5d588f

Deleting the log message.

andrross approved these changes Nov 22, 2021

View reviewed changes

dblock approved these changes Nov 22, 2021

View reviewed changes

dblock merged commit 12a2294 into opensearch-project:main Nov 22, 2021

dblock added the pending backport Identifies an issue or PR that still needs to be backported label Nov 22, 2021

dblock added the backport 1.x label Nov 23, 2021

dblock removed the pending backport Identifies an issue or PR that still needs to be backported label Dec 6, 2021

meghasaik deleted the informative-error-messages branch December 13, 2021 21:22

Giving informative error messages for double slashes in API call URLs #1568

Giving informative error messages for double slashes in API call URLs #1568

Conversation

meghasaik commented Nov 16, 2021 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Nov 16, 2021

opensearch-ci-bot commented Nov 16, 2021

meghasaik commented Nov 16, 2021

opensearch-ci-bot commented Nov 16, 2021

opensearch-ci-bot commented Nov 16, 2021

opensearch-ci-bot commented Nov 16, 2021

opensearch-ci-bot commented Nov 17, 2021

opensearch-ci-bot commented Nov 17, 2021

opensearch-ci-bot commented Nov 17, 2021

meghasaik commented Nov 17, 2021

opensearch-ci-bot commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Nov 18, 2021

opensearch-ci-bot commented Nov 18, 2021

opensearch-ci-bot commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

meghasaik commented Nov 20, 2021

opensearch-ci-bot commented Nov 20, 2021

andrross 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

dblock commented Nov 22, 2021 • edited Loading

meghasaik commented Nov 22, 2021

opensearch-ci-bot commented Nov 22, 2021

opensearch-ci-bot commented Nov 22, 2021

opensearch-ci-bot commented Nov 22, 2021

Choose a reason for hiding this comment

opensearch-ci-bot commented Nov 22, 2021

opensearch-ci-bot commented Nov 22, 2021

opensearch-ci-bot commented Nov 22, 2021

dblock commented Nov 22, 2021

meghasaik commented Nov 22, 2021

dblock commented Dec 6, 2021

meghasaik commented Dec 6, 2021

dblock commented Dec 6, 2021

meghasaik commented Dec 6, 2021

meghasaik commented Nov 16, 2021 •

edited

Loading

dblock commented Nov 22, 2021 •

edited

Loading