From 98deb5537f3fabaff8b184e1bb79e85b2a27d01a Mon Sep 17 00:00:00 2001
From: Martijn van Groningen <martijn.v.groningen@gmail.com>
Date: Wed, 9 Apr 2014 12:48:35 +0700
Subject: [PATCH] Better deal with invalid scroll ids.

Closes #5738
---
 .../search/type/TransportSearchHelper.java    | 10 ++++-
 .../search/scroll/SearchScrollTests.java      | 38 ++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/elasticsearch/action/search/type/TransportSearchHelper.java b/src/main/java/org/elasticsearch/action/search/type/TransportSearchHelper.java
index fe8ccabf02f..82a8791cd82 100644
--- a/src/main/java/org/elasticsearch/action/search/type/TransportSearchHelper.java
+++ b/src/main/java/org/elasticsearch/action/search/type/TransportSearchHelper.java
@@ -96,13 +96,21 @@ public abstract class TransportSearchHelper {
         try {
             byte[] decode = Base64.decode(scrollId, Base64.URL_SAFE);
             UnicodeUtil.UTF8toUTF16(decode, 0, decode.length, spare);
-        } catch (IOException e) {
+        } catch (Exception e) {
             throw new ElasticsearchIllegalArgumentException("Failed to decode scrollId", e);
         }
         String[] elements = Strings.splitStringToArray(spare, ';');
+        if (elements.length < 2) {
+            throw new ElasticsearchIllegalArgumentException("Malformed scrollId [" + scrollId + "]");
+        }
+
         int index = 0;
         String type = elements[index++];
         int contextSize = Integer.parseInt(elements[index++]);
+        if (elements.length < contextSize + 2) {
+            throw new ElasticsearchIllegalArgumentException("Malformed scrollId [" + scrollId + "]");
+        }
+
         @SuppressWarnings({"unchecked"}) Tuple<String, Long>[] context = new Tuple[contextSize];
         for (int i = 0; i < contextSize; i++) {
             String element = elements[index++];
diff --git a/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java b/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java
index df9dbdbca79..33a3f9104ae 100644
--- a/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java
+++ b/src/test/java/org/elasticsearch/search/scroll/SearchScrollTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.search.scroll;
 
+import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.action.search.ClearScrollResponse;
 import org.elasticsearch.action.search.SearchRequestBuilder;
 import org.elasticsearch.action.search.SearchResponse;
@@ -26,6 +27,7 @@ import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.common.util.concurrent.UncategorizedExecutionException;
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.sort.SortOrder;
@@ -37,7 +39,7 @@ import java.util.Map;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.elasticsearch.index.query.QueryBuilders.*;
-import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.*;
 
 /**
  *
@@ -300,6 +302,40 @@ public class SearchScrollTests extends ElasticsearchIntegrationTest {
         assertThat(searchResponse2.getHits().hits().length, equalTo(0));
     }
 
+    @Test
+    public void testClearNonExistentScrollId() throws Exception {
+        createIndex("idx");
+        ClearScrollResponse response = client().prepareClearScroll()
+                .addScrollId("cXVlcnlUaGVuRmV0Y2g7MzsyOlpBRC1qOUhrUjhhZ0NtQWUxU2FuWlE7MjpRcjRaNEJ2R1JZV1VEMW02ZGF1LW5ROzI6S0xUal9lZDRTd3lWNUhUU2VSb01CQTswOw==")
+                .get();
+        // Whether we actually clear a scroll, we can't know, since that information isn't serialized in the
+        // free search context response, which is returned from each node we want to clear a particular scroll.
+        assertThat(response.isSucceeded(), is(true));
+    }
+
+    @Test
+    public void testClearIllegalScrollId() throws Exception {
+        createIndex("idx");
+        try {
+            client().prepareClearScroll().addScrollId("c2Nhbjs2OzM0NDg1ODpzRlBLc0FXNlNyNm5JWUc1").get();
+            fail();
+        } catch (ElasticsearchIllegalArgumentException e) {
+        }
+        try {
+            // Fails during base64 decoding (Base64-encoded string must have at least four characters)
+            client().prepareClearScroll().addScrollId("a").get();
+            fail();
+        } catch (ElasticsearchIllegalArgumentException e) {
+        }
+        try {
+            client().prepareClearScroll().addScrollId("abcabc").get();
+            fail();
+            // if running without -ea this will also throw ElasticsearchIllegalArgumentException
+        } catch (UncategorizedExecutionException e) {
+            assertThat(e.getRootCause(), instanceOf(AssertionError.class));
+        }
+    }
+
     @Test
     public void testSimpleScrollQueryThenFetch_clearAllScrollIds() throws Exception {
         client().admin().indices().prepareCreate("test").setSettings(ImmutableSettings.settingsBuilder().put("index.number_of_shards", 3)).execute().actionGet();