Disallow topics that contain quotation marks
When performing a query such as topic:quotation"mark, there
is a bug that makes this query fail because of the quotation character.
This change makes it illegal for all future changes to have quotes. The
migration plan is to manually change the existing changes that contain
quotes, to not have quotes.
Change-Id: I97a1c16ced12bfe8c00eaa324f7b2806c0d3354a
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index efc9de8..8930bc9 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6089,6 +6089,7 @@
contain a Change-Id, a newly generated Change-Id is automatically
inserted into the commit message.
|`topic` |optional|The topic to which this change belongs.
+Topic can't contain quotation marks.
|`status` |optional, default to `NEW`|
The status of the change (only `NEW` accepted here).
|`is_private` |optional, default to `false`|
@@ -7098,6 +7099,7 @@
Name of the topic for the revert change. If not set, the default for Revert
endpoint is the topic of the change being reverted, and the default for the
RevertSubmission endpoint is `revert-{submission_id}-{timestamp.now}`.
+Topic can't contain quotation marks.
|=============================
[[revert-submission-info]]
@@ -7520,6 +7522,7 @@
|Field Name ||Description
|`topic` |optional|The topic. +
The topic will be deleted if not set.
+Topic can't contain quotation marks.
|===========================
[[tracking-id-info]]
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index c98a0e8..bbb94ea 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -41,6 +41,7 @@
import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -71,6 +72,7 @@
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.gerrit.server.util.RequestScopePropagator;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
@@ -380,7 +382,11 @@
update.setChangeId(change.getKey().get());
update.setSubjectForCommit("Create change");
update.setBranch(change.getDest().branch());
- update.setTopic(change.getTopic());
+ try {
+ update.setTopic(change.getTopic());
+ } catch (ValidationException ex) {
+ throw new BadRequestException(ex.getMessage());
+ }
update.setPsDescription(patchSetDescription);
update.setPrivate(isPrivate);
update.setWorkInProgress(workInProgress);
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 66a86cc..988d178 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -27,6 +27,7 @@
import com.google.gerrit.entities.PatchSetInfo;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -51,6 +52,7 @@
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
@@ -210,7 +212,8 @@
}
@Override
- public boolean updateChange(ChangeContext ctx) throws ResourceConflictException, IOException {
+ public boolean updateChange(ChangeContext ctx)
+ throws ResourceConflictException, IOException, BadRequestException {
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(psId);
update.setSubjectForCommit("Create patch set " + psId.get());
@@ -266,7 +269,11 @@
}
if (topic != null) {
change.setTopic(topic);
- update.setTopic(topic);
+ try {
+ update.setTopic(topic);
+ } catch (ValidationException ex) {
+ throw new BadRequestException(ex.getMessage());
+ }
}
return true;
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index ceeff11..231359b 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -208,7 +209,8 @@
}
@Override
- public boolean updateChange(ChangeContext ctx) throws ResourceConflictException, IOException {
+ public boolean updateChange(ChangeContext ctx)
+ throws ResourceConflictException, IOException, BadRequestException {
boolean ret = patchSetInserter.updateChange(ctx);
rebasedPatchSet = patchSetInserter.getPatchSet();
return ret;
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 2a54930..2ffa7b6 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -176,6 +176,7 @@
import com.google.gerrit.server.util.MagicBranch;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.util.time.TimeUtil;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.util.cli.CmdLineParser;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -2584,15 +2585,7 @@
.setFireEvent(false));
}
if (!Strings.isNullOrEmpty(magicBranch.topic)) {
- bu.addOp(
- changeId,
- new BatchUpdateOp() {
- @Override
- public boolean updateChange(ChangeContext ctx) {
- ctx.getUpdate(psId).setTopic(magicBranch.topic);
- return true;
- }
- });
+ bu.addOp(changeId, new SetTopicOp(magicBranch.topic));
}
bu.addOp(
changeId,
@@ -3466,4 +3459,19 @@
b.append(")\n");
return b.toString();
}
+
+ private static class SetTopicOp implements BatchUpdateOp {
+
+ private final String topic;
+
+ public SetTopicOp(String topic) {
+ this.topic = topic;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) throws ValidationException {
+ ctx.getUpdate(ctx.getChange().currentPatchSetId()).setTopic(topic);
+ return true;
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 16a4e64..0baecf5 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -41,6 +41,7 @@
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.ApprovalsUtil;
@@ -72,6 +73,7 @@
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.RequestScopePropagator;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.util.Providers;
@@ -242,7 +244,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
- throws RestApiException, IOException, PermissionBackendException, ConfigInvalidException {
+ throws RestApiException, IOException, PermissionBackendException, ConfigInvalidException,
+ ValidationException {
notes = ctx.getNotes();
Change change = notes.getChange();
if (change == null || change.isClosed()) {
@@ -272,7 +275,11 @@
update.setHashtags(hashtags);
}
if (magicBranch.topic != null && !magicBranch.topic.equals(ctx.getChange().getTopic())) {
- update.setTopic(magicBranch.topic);
+ try {
+ update.setTopic(magicBranch.topic);
+ } catch (ValidationException ex) {
+ throw new BadRequestException(ex.getMessage());
+ }
}
if (magicBranch.removePrivate) {
change.setPrivate(false);
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index af0ab35..63f4e5d 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -66,6 +66,7 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
@@ -345,7 +346,11 @@
}
}
- public void setTopic(String topic) {
+ public void setTopic(String topic) throws ValidationException {
+
+ if (isIllegalTopic(topic)) {
+ throw new ValidationException("topic can't contain quotation marks.");
+ }
this.topic = Strings.nullToEmpty(topic);
}
@@ -791,6 +796,10 @@
sb.append('\n');
}
+ private static boolean isIllegalTopic(String topic) {
+ return (topic != null && topic.contains("\""));
+ }
+
private StringBuilder addIdent(StringBuilder sb, Account.Id accountId) {
PersonIdent ident = noteUtil.newIdent(accountId, when, serverIdent);
ChangeNoteUtil.appendIdentString(sb, ident.getName(), ident.getEmailAddress());
diff --git a/java/com/google/gerrit/server/restapi/change/SetTopicOp.java b/java/com/google/gerrit/server/restapi/change/SetTopicOp.java
index 331cbd9..9eff5c1 100644
--- a/java/com/google/gerrit/server/restapi/change/SetTopicOp.java
+++ b/java/com/google/gerrit/server/restapi/change/SetTopicOp.java
@@ -18,12 +18,14 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.extensions.api.changes.TopicInput;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.extensions.events.TopicEdited;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -49,7 +51,7 @@
}
@Override
- public boolean updateChange(ChangeContext ctx) {
+ public boolean updateChange(ChangeContext ctx) throws BadRequestException {
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
newTopicName = Strings.nullToEmpty(input.topic);
@@ -67,8 +69,11 @@
summary = String.format("Topic changed from %s to %s", oldTopicName, newTopicName);
}
change.setTopic(Strings.emptyToNull(newTopicName));
- update.setTopic(change.getTopic());
-
+ try {
+ update.setTopic(change.getTopic());
+ } catch (ValidationException ex) {
+ throw new BadRequestException(ex.getMessage());
+ }
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx, summary, ChangeMessagesUtil.TAG_SET_TOPIC);
cmUtil.addChangeMessage(update, cmsg);
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index 30fb580..33c3584 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -22,6 +22,7 @@
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -214,7 +215,7 @@
@Override
public PatchSet updateChangeImpl(ChangeContext ctx)
- throws NoSuchChangeException, ResourceConflictException, IOException {
+ throws NoSuchChangeException, ResourceConflictException, IOException, BadRequestException {
if (newCommit == null) {
checkState(!rebaseAlways, "RebaseAlways must never fast forward");
// otherwise, took the fast-forward option, nothing to do.
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java b/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
index 1d928a2..003580a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/TopicIT.java
@@ -58,4 +58,13 @@
assertThat(response.getEntityContent()).isEqualTo(")]}'\n\"t opic\"");
}
+
+ @Test
+ public void illegalTopics() throws Exception {
+ Result result = createChange();
+ char illegalChar = '"';
+ String endpoint = "/changes/" + result.getChangeId() + "/topic";
+ RestResponse response = adminRestSession.put(endpoint, "topic" + illegalChar);
+ response.assertBadRequest();
+ }
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index e7db79e..4ec52e3 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -57,6 +57,7 @@
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.util.time.TimeUtil;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.testing.TestChanges;
import com.google.inject.Inject;
import java.sql.Timestamp;
@@ -931,6 +932,10 @@
update.commit();
notes = newNotes(c);
assertThat(notes.getChange().getTopic()).isNull();
+
+ // check invalid topic
+ ChangeUpdate failingUpdate = newUpdate(c, changeOwner);
+ assertThrows(ValidationException.class, () -> failingUpdate.setTopic("\""));
}
@Test
