From 9fce5a99d8a1c717317d967f307dfdd7fdad2eeb Mon Sep 17 00:00:00 2001
From: Henning Andersen <33268011+henningandersen@users.noreply.github.com>
Date: Mon, 9 Sep 2019 21:48:26 +0200
Subject: [PATCH] Rest Controller wildcard registration (#46487)

Registering two different http methods on the same path using different
wildcard names would result in the last wildcard name being active only.
Now throw an exception instead.

Closes #46482
---
 .../elasticsearch/common/path/PathTrie.java   |  7 ++++++-
 .../common/path/PathTrieTests.java            |  9 ++++-----
 .../rest/RestControllerTests.java             | 19 +++++++++++++++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/common/path/PathTrie.java b/server/src/main/java/org/elasticsearch/common/path/PathTrie.java
index 7b6c660a64a..a6ec8c92dae 100644
--- a/server/src/main/java/org/elasticsearch/common/path/PathTrie.java
+++ b/server/src/main/java/org/elasticsearch/common/path/PathTrie.java
@@ -96,7 +96,12 @@ public class PathTrie<T> {
 
         private void updateKeyWithNamedWildcard(String key) {
             this.key = key;
-            namedWildcard = key.substring(key.indexOf('{') + 1, key.indexOf('}'));
+            String newNamedWildcard = key.substring(key.indexOf('{') + 1, key.indexOf('}'));
+            if (namedWildcard != null && newNamedWildcard.equals(namedWildcard) == false) {
+                throw new IllegalArgumentException("Trying to use conflicting wildcard names for same path: "
+                    + namedWildcard + " and " + newNamedWildcard);
+            }
+            namedWildcard = newNamedWildcard;
         }
 
         private void addInnerChild(String key, TrieNode child) {
diff --git a/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java b/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java
index 555f052872b..42886070958 100644
--- a/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java
+++ b/server/src/test/java/org/elasticsearch/common/path/PathTrieTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.common.path;
 
+import org.elasticsearch.common.path.PathTrie.TrieMatchingMode;
 import org.elasticsearch.rest.RestUtils;
 import org.elasticsearch.test.ESTestCase;
 
@@ -29,8 +30,6 @@ import java.util.Map;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.nullValue;
 
-import org.elasticsearch.common.path.PathTrie.TrieMatchingMode;
-
 public class PathTrieTests extends ESTestCase {
 
     public static final PathTrie.Decoder NO_DECODER = new PathTrie.Decoder() {
@@ -122,13 +121,13 @@ public class PathTrieTests extends ESTestCase {
         PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("{testA}", "test1");
         trie.insert("{testA}/{testB}", "test2");
-        trie.insert("a/{testA}", "test3");
+        trie.insert("a/{testB}", "test3");
         trie.insert("{testA}/b", "test4");
         trie.insert("{testA}/b/c", "test5");
         trie.insert("a/{testB}/c", "test6");
         trie.insert("a/b/{testC}", "test7");
         trie.insert("{testA}/b/{testB}", "test8");
-        trie.insert("x/{testA}/z", "test9");
+        trie.insert("x/{testB}/z", "test9");
         trie.insert("{testA}/{testB}/{testC}", "test10");
 
         Map<String, String> params = new HashMap<>();
@@ -196,7 +195,7 @@ public class PathTrieTests extends ESTestCase {
         trie.insert("a", "test2");
         trie.insert("{testA}/{testB}", "test3");
         trie.insert("a/{testB}", "test4");
-        trie.insert("{testB}/b", "test5");
+        trie.insert("{testA}/b", "test5");
         trie.insert("a/b", "test6");
         trie.insert("{testA}/b/{testB}", "test7");
         trie.insert("x/{testA}/z", "test8");
diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
index 4515897df30..d097afad005 100644
--- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
+++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
@@ -56,6 +56,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
@@ -175,6 +176,24 @@ public class RestControllerTests extends ESTestCase {
         verify(controller).registerAsDeprecatedHandler(deprecatedMethod, deprecatedPath, handler, deprecationMessage, logger);
     }
 
+    public void testRegisterSecondMethodWithDifferentNamedWildcard() {
+        final RestController restController = new RestController(null, null, null, circuitBreakerService, usageService);
+
+        RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values());
+        RestRequest.Method secondMethod =
+            randomFrom(Arrays.stream(RestRequest.Method.values()).filter(m -> m != firstMethod).collect(Collectors.toList()));
+
+        final String path = "/_" + randomAlphaOfLengthBetween(1, 6);
+
+        RestHandler handler = mock(RestHandler.class);
+        restController.registerHandler(firstMethod, path + "/{wildcard1}", handler);
+
+        IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
+            () -> restController.registerHandler(secondMethod, path + "/{wildcard2}", handler));
+
+        assertThat(exception.getMessage(), equalTo("Trying to use conflicting wildcard names for same path: wildcard1 and wildcard2"));
+    }
+
     public void testRestHandlerWrapper() throws Exception {
         AtomicBoolean handlerCalled = new AtomicBoolean(false);
         AtomicBoolean wrapperCalled = new AtomicBoolean(false);