From bb43782b57002ab9830551204e6e98decced7a7a Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Tue, 3 May 2016 20:27:41 -0400
Subject: [PATCH] ChangeNoteUtil: Handle PersonIdents with weird names

JGit recently gained the ability to sanitize the output of
PersonIdents with characters that interfere with parsing (<>\n). This
now happens automatically when using toExternalString(), but
ChangeNoteUtil isn't using the full string with timestamp, so it
didn't get the behavior for free. Call the sanitization method
manually instead.

Change-Id: I35eedc5f31e36b0eef609142738fbefe58d6df72
---
 .../gerrit/server/notedb/ChangeNoteUtil.java  |  9 ++--
 .../server/notedb/ChangeNotesParserTest.java  |  5 ++
 .../gerrit/server/notedb/ChangeNotesTest.java | 53 +++++++++++++++++++
 3 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index 8b112a326a..95b57254ec 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -539,9 +539,12 @@ public class ChangeNoteUtil {
     PersonIdent ident = newIdent(
         accountCache.get(c.getAuthor()).getAccount(),
         c.getWrittenOn(), serverIdent, anonymousCowardName);
-    String nameString = ident.getName() + " <" + ident.getEmailAddress()
-        + ">";
-    appendHeaderField(writer, AUTHOR, nameString);
+    StringBuilder name = new StringBuilder();
+    PersonIdent.appendSanitized(name, ident.getName());
+    name.append(" <");
+    PersonIdent.appendSanitized(name, ident.getEmailAddress());
+    name.append('>');
+    appendHeaderField(writer, AUTHOR, name.toString());
 
     String parent = c.getParentUuid();
     if (parent != null) {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
index d4d7d190ac..45405f4ca9 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -65,6 +65,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
         + "Patch-set: 1\n",
         new PersonIdent("Change Owner", "x@gerrit",
           serverIdent.getWhen(), serverIdent.getTimeZone())));
+    assertParseFails(writeCommit("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n",
+        new PersonIdent("Change\n\u1234<Owner>", "\n\nx<@>\u0002gerrit",
+          serverIdent.getWhen(), serverIdent.getTimeZone())));
   }
 
   @Test
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index c73d972eab..526ce47ed8 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -45,6 +45,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -1488,6 +1489,58 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
             revId, comment3));
   }
 
+  @Test
+  public void patchLineCommentNotesFormatWeirdUser() throws Exception {
+    Account account = new Account(new Account.Id(3), TimeUtil.nowTs());
+    account.setFullName("Weird\n\u0002<User>\n");
+    account.setPreferredEmail(" we\r\nird@ex>ample<.com");
+    accountCache.put(account);
+    IdentifiedUser user = userFactory.create(account.getId());
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, user);
+    String uuid = "uuid";
+    CommentRange range = new CommentRange(1, 1, 2, 1);
+    Timestamp time = TimeUtil.nowTs();
+    PatchSet.Id psId = c.currentPatchSetId();
+
+    PatchLineComment comment = newPublishedComment(psId, "file1",
+        uuid, range, range.getEndLine(), user, null, time, "comment",
+        (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234");
+    update.setPatchSetId(psId);
+    update.putComment(comment);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+
+    try (RevWalk walk = new RevWalk(repo)) {
+      ArrayList<Note> notesInTree =
+          Lists.newArrayList(notes.revisionNoteMap.noteMap.iterator());
+      Note note = Iterables.getOnlyElement(notesInTree);
+
+      byte[] bytes =
+          walk.getObjectReader().open(
+              note.getData(), Constants.OBJ_BLOB).getBytes();
+      String noteString = new String(bytes, UTF_8);
+      String timeStr = ChangeNoteUtil.formatTime(serverIdent, time);
+      assertThat(noteString).isEqualTo(
+          "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+          + "Patch-set: 1\n"
+          + "File: file1\n"
+          + "\n"
+          + "1:1-2:1\n"
+          + timeStr + "\n"
+          + "Author: Weird\u0002User <3@gerrit>\n"
+          + "UUID: uuid\n"
+          + "Bytes: 7\n"
+          + "comment\n"
+          + "\n");
+    }
+
+    assertThat(notes.getComments())
+        .isEqualTo(ImmutableMultimap.of(comment.getRevId(), comment));
+  }
+
   @Test
   public void patchLineCommentMultipleOnePatchsetOneFileBothSides()
       throws Exception {