From 4aa14560a919759230786d4a5cc55c8ee5b32d02 Mon Sep 17 00:00:00 2001
From: Patrick Hiesel <hiesel@google.com>
Date: Tue, 19 Jun 2018 15:07:50 +0200
Subject: [PATCH] Trim leading and trailing whitespaces in topic name in
 PutTopic

Whitespaces in the topic name can lead to undesired effects when
querying for the topic. This commit just sanitizes whitespaces of any
kind from the start and end of the topic name.

Change-Id: I0bd032e8f43c3d344af8c31eac449f04b8c5d1d2
---
 Documentation/rest-api-changes.txt            |  3 ++-
 .../server/restapi/change/PutTopic.java       |  7 +++++-
 .../acceptance/rest/change/TopicIT.java       | 22 +++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e64bdb3d5a..fb0a0dffd1 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -889,7 +889,8 @@ If the change does not have a topic an empty string is returned.
 Sets the topic of a change.
 
 The new topic must be provided in the request body inside a
-link:#topic-input[TopicInput] entity.
+link:#topic-input[TopicInput] entity. Any leading or trailing whitespace
+in the topic name will be removed.
 
 .Request
 ----
diff --git a/java/com/google/gerrit/server/restapi/change/PutTopic.java b/java/com/google/gerrit/server/restapi/change/PutTopic.java
index 468590562b..45a837a34a 100644
--- a/java/com/google/gerrit/server/restapi/change/PutTopic.java
+++ b/java/com/google/gerrit/server/restapi/change/PutTopic.java
@@ -75,7 +75,12 @@ public class PutTopic extends RetryingRestModifyView<ChangeResource, TopicInput,
           String.format("topic length exceeds the limit (%s)", ChangeUtil.TOPIC_MAX_LENGTH));
     }
 
-    Op op = new Op(input != null ? input : new TopicInput());
+    TopicInput sanitizedInput = input == null ? new TopicInput() : input;
+    if (sanitizedInput.topic != null) {
+      sanitizedInput.topic = sanitizedInput.topic.trim();
+    }
+
+    Op op = new Op(sanitizedInput);
     try (BatchUpdate u =
         updateFactory.create(
             dbProvider.get(), req.getChange().getProject(), req.getUser(), TimeUtil.nowTs())) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java b/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
index 3121812792..1d928a2243 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
@@ -14,6 +14,8 @@
 
 package com.google.gerrit.acceptance.rest.change;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit.Result;
 import com.google.gerrit.acceptance.RestResponse;
@@ -36,4 +38,24 @@ public class TopicIT extends AbstractDaemonTest {
     response = adminRestSession.put(endpoint, "");
     response.assertNoContent();
   }
+
+  @Test
+  public void leadingAndTrailingWhitespaceGetsSanitized() throws Exception {
+    Result result = createChange();
+    String endpoint = "/changes/" + result.getChangeId() + "/topic";
+    RestResponse response = adminRestSession.put(endpoint, "\t \t topic\t ");
+    response.assertOK();
+
+    assertThat(response.getEntityContent()).isEqualTo(")]}'\n\"topic\"");
+  }
+
+  @Test
+  public void containedWhitespaceDoesNotGetSanitized() throws Exception {
+    Result result = createChange();
+    String endpoint = "/changes/" + result.getChangeId() + "/topic";
+    RestResponse response = adminRestSession.put(endpoint, "t opic");
+    response.assertOK();
+
+    assertThat(response.getEntityContent()).isEqualTo(")]}'\n\"t opic\"");
+  }
 }