Fix: User could get around restrictions by editing commit msg

The Gerrit server may enforce several restrictions on the
commit message (change-id needed, signed-off-by, etc).

The user could get around these restrictions by pushing a
commit and then editing the commit message using the UI.

Now the "Edit commit message"-feature is using the
validation code refactored out from ReceiveCommits, and
correct validation of the commit message is done before
Gerrit accepts the new commit.

Change-Id: I2fb13ddb1ea5aacf672101b1e4c1867543bd66e3

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java

@@ -28,18 +28,22 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.mail.CommitMessageEditedSender;
+import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.ssh.NoSshInfo;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
 
 import java.io.IOException;
 
@@ -63,6 +67,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
   private final String message;
 
   private final ChangeHooks hooks;
+  private final CommitValidators.Factory commitValidatorsFactory;
 
   private final GitRepositoryManager gitManager;
   private final PatchSetInfoFactory patchSetInfoFactory;
@@ -76,6 +81,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
       final CommitMessageEditedSender.Factory commitMessageEditedSenderFactory,
       @Assisted final PatchSet.Id patchSetId,
       @Assisted @Nullable final String message, final ChangeHooks hooks,
+      final CommitValidators.Factory commitValidatorsFactory,
       final GitRepositoryManager gitManager,
       final PatchSetInfoFactory patchSetInfoFactory,
       final GitReferenceUpdated replication,
@@ -89,6 +95,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
     this.patchSetId = patchSetId;
     this.message = message;
     this.hooks = hooks;
+    this.commitValidatorsFactory = commitValidatorsFactory;
     this.gitManager = gitManager;
 
     this.patchSetInfoFactory = patchSetInfoFactory;
@@ -109,10 +116,22 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
           "Not allowed to add new Patch Sets to: " + changeId.toString());
     }
 
-    ChangeUtil.editCommitMessage(patchSetId, currentUser, message, db,
+    final Repository git;
-        commitMessageEditedSenderFactory, hooks, gitManager, patchSetInfoFactory,
+    try {
-        replication, myIdent);
+      git = gitManager.openRepository(db.changes().get(changeId).getProject());
+    } catch (RepositoryNotFoundException e) {
+      throw new NoSuchChangeException(changeId, e);
+    }
+    try {
+      CommitValidators commitValidators =
+          commitValidatorsFactory.create(control.getRefControl(), new NoSshInfo(), git);
 
-    return changeDetailFactory.create(changeId).call();
+      ChangeUtil.editCommitMessage(patchSetId, control.getRefControl(), commitValidators, currentUser, message, db,
+          commitMessageEditedSenderFactory, hooks, git, patchSetInfoFactory, replication, myIdent);
+
+      return changeDetailFactory.create(changeId).call();
+    } finally {
+      git.close();
+    }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -27,15 +27,19 @@ import com.google.gerrit.reviewdb.client.TrackingId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.config.TrackingFooter;
 import com.google.gerrit.server.config.TrackingFooters;
+import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.mail.CommitMessageEditedSender;
+import com.google.gerrit.server.git.validators.CommitValidationException;
+import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.mail.RevertedSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.RefControl;
 import com.google.gerrit.server.util.IdGenerator;
 import com.google.gwtorm.server.AtomicUpdate;
 import com.google.gwtorm.server.OrmConcurrencyException;
@@ -54,6 +58,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.FooterLine;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
 import org.eclipse.jgit.util.Base64;
 import org.eclipse.jgit.util.ChangeIdUtil;
 import org.eclipse.jgit.util.NB;
@@ -305,9 +310,10 @@ public class ChangeUtil {
   }
 
   public static Change.Id editCommitMessage(final PatchSet.Id patchSetId,
+      final RefControl refControl, CommitValidators commitValidators,
       final IdentifiedUser user, final String message, final ReviewDb db,
       final CommitMessageEditedSender.Factory commitMessageEditedSenderFactory,
-      final ChangeHooks hooks, GitRepositoryManager gitManager,
+      final ChangeHooks hooks, Repository git,
       final PatchSetInfoFactory patchSetInfoFactory,
       final GitReferenceUpdated replication, PersonIdent myIdent)
       throws NoSuchChangeException, EmailException, OrmException,
@@ -323,128 +329,129 @@ public class ChangeUtil {
       throw new InvalidChangeOperationException("The commit message cannot be empty");
     }
 
-    final Repository git;
+    final RevWalk revWalk = new RevWalk(git);
     try {
-      git = gitManager.openRepository(db.changes().get(changeId).getProject());
+      RevCommit commit =
-    } catch (RepositoryNotFoundException e) {
+          revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
-      throw new NoSuchChangeException(changeId, e);
+      if (commit.getFullMessage().equals(message)) {
-    }
+        throw new InvalidChangeOperationException("New commit message cannot be same as existing commit message");
+      }
 
-    try {
+      Date now = myIdent.getWhen();
-      final RevWalk revWalk = new RevWalk(git);
+      Change change = db.changes().get(changeId);
+      PersonIdent authorIdent =
+          user.newCommitterIdent(now, myIdent.getTimeZone());
+
+      CommitBuilder commitBuilder = new CommitBuilder();
+      commitBuilder.setTreeId(commit.getTree());
+      commitBuilder.setParentIds(commit.getParents());
+      commitBuilder.setAuthor(commit.getAuthorIdent());
+      commitBuilder.setCommitter(authorIdent);
+      commitBuilder.setMessage(message);
+
+      RevCommit newCommit;
+      final ObjectInserter oi = git.newObjectInserter();
       try {
-        RevCommit commit =
+        ObjectId id = oi.insert(commitBuilder);
-            revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
+        oi.flush();
-        if (commit.getFullMessage().equals(message)) {
+        newCommit = revWalk.parseCommit(id);
-          throw new InvalidChangeOperationException("New commit message cannot be same as existing commit message");
+      } finally {
+        oi.release();
+      }
+
+      final PatchSet originalPS = db.patchSets().get(patchSetId);
+      PatchSet.Id id = nextPatchSetId(git, change.currentPatchSetId());
+      final PatchSet newPatchSet = new PatchSet(id);
+      newPatchSet.setCreatedOn(new Timestamp(now.getTime()));
+      newPatchSet.setUploader(user.getAccountId());
+      newPatchSet.setRevision(new RevId(newCommit.name()));
+      newPatchSet.setDraft(originalPS.isDraft());
+
+      final PatchSetInfo info =
+          patchSetInfoFactory.get(newCommit, newPatchSet.getId());
+
+      CommitReceivedEvent commitReceivedEvent =
+          new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(),
+              newCommit.getId(), newPatchSet.getRefName()), refControl
+              .getProjectControl().getProject(), refControl.getRefName(),
+              newCommit, user);
+
+      try {
+        commitValidators.validateForReceiveCommits(commitReceivedEvent);
+      } catch (CommitValidationException e) {
+        throw new InvalidChangeOperationException(e.getMessage());
+      }
+
+      final RefUpdate ru = git.updateRef(newPatchSet.getRefName());
+      ru.setExpectedOldObjectId(ObjectId.zeroId());
+      ru.setNewObjectId(newCommit);
+      ru.disableRefLog();
+      if (ru.update(revWalk) != RefUpdate.Result.NEW) {
+        throw new IOException(String.format(
+            "Failed to create ref %s in %s: %s", newPatchSet.getRefName(),
+            change.getDest().getParentKey().get(), ru.getResult()));
+      }
+      replication.fire(change.getProject(), ru.getName());
+
+      db.changes().beginTransaction(change.getId());
+      try {
+        Change updatedChange = db.changes().get(change.getId());
+        if (updatedChange != null && updatedChange.getStatus().isOpen()) {
+          change = updatedChange;
+        } else {
+          throw new InvalidChangeOperationException(String.format(
+              "Change %s is closed", change.getId()));
         }
 
-        Date now = myIdent.getWhen();
+        ChangeUtil.insertAncestors(db, newPatchSet.getId(), commit);
-        Change change = db.changes().get(changeId);
+        db.patchSets().insert(Collections.singleton(newPatchSet));
-        PersonIdent authorIdent =
+        updatedChange =
-            user.newCommitterIdent(now, myIdent.getTimeZone());
+            db.changes().atomicUpdate(change.getId(), new AtomicUpdate<Change>() {
-
+              @Override
-        CommitBuilder commitBuilder = new CommitBuilder();
+              public Change update(Change change) {
-        commitBuilder.setTreeId(commit.getTree());
+                if (change.getStatus().isClosed()) {
-        commitBuilder.setParentIds(commit.getParents());
+                  return null;
-        commitBuilder.setAuthor(commit.getAuthorIdent());
-        commitBuilder.setCommitter(authorIdent);
-        commitBuilder.setMessage(message);
-
-        RevCommit newCommit;
-        final ObjectInserter oi = git.newObjectInserter();
-        try {
-          ObjectId id = oi.insert(commitBuilder);
-          oi.flush();
-          newCommit = revWalk.parseCommit(id);
-        } finally {
-          oi.release();
-        }
-
-        final PatchSet originalPS = db.patchSets().get(patchSetId);
-        PatchSet.Id id = nextPatchSetId(git, change.currentPatchSetId());
-        final PatchSet newPatchSet = new PatchSet(id);
-        newPatchSet.setCreatedOn(new Timestamp(now.getTime()));
-        newPatchSet.setUploader(user.getAccountId());
-        newPatchSet.setRevision(new RevId(newCommit.name()));
-        newPatchSet.setDraft(originalPS.isDraft());
-
-        final PatchSetInfo info =
-            patchSetInfoFactory.get(newCommit, newPatchSet.getId());
-
-        final RefUpdate ru = git.updateRef(newPatchSet.getRefName());
-        ru.setExpectedOldObjectId(ObjectId.zeroId());
-        ru.setNewObjectId(newCommit);
-        ru.disableRefLog();
-        if (ru.update(revWalk) != RefUpdate.Result.NEW) {
-          throw new IOException(String.format(
-              "Failed to create ref %s in %s: %s", newPatchSet.getRefName(),
-              change.getDest().getParentKey().get(), ru.getResult()));
-        }
-        replication.fire(change.getProject(), ru.getName());
-
-        db.changes().beginTransaction(change.getId());
-        try {
-          Change updatedChange = db.changes().get(change.getId());
-          if (updatedChange != null && updatedChange.getStatus().isOpen()) {
-            change = updatedChange;
-          } else {
-            throw new InvalidChangeOperationException(String.format(
-                "Change %s is closed", change.getId()));
-          }
-
-          ChangeUtil.insertAncestors(db, newPatchSet.getId(), commit);
-          db.patchSets().insert(Collections.singleton(newPatchSet));
-          updatedChange =
-              db.changes().atomicUpdate(change.getId(), new AtomicUpdate<Change>() {
-                @Override
-                public Change update(Change change) {
-                  if (change.getStatus().isClosed()) {
-                    return null;
-                  }
-                  if (!change.currentPatchSetId().equals(patchSetId)) {
-                    return null;
-                  }
-                  if (change.getStatus() != Change.Status.DRAFT) {
-                    change.setStatus(Change.Status.NEW);
-                  }
-                  change.setLastSha1MergeTested(null);
-                  change.setCurrentPatchSet(info);
-                  ChangeUtil.updated(change);
-                  return change;
                 }
-              });
+                if (!change.currentPatchSetId().equals(patchSetId)) {
-          if (updatedChange != null) {
+                  return null;
-            change = updatedChange;
+                }
-          } else {
+                if (change.getStatus() != Change.Status.DRAFT) {
-            throw new InvalidChangeOperationException(String.format(
+                  change.setStatus(Change.Status.NEW);
-                "Change %s was modified", change.getId()));
+                }
-          }
+                change.setLastSha1MergeTested(null);
+                change.setCurrentPatchSet(info);
+                ChangeUtil.updated(change);
+                return change;
+              }
+            });
+        if (updatedChange != null) {
+          change = updatedChange;
+        } else {
+          throw new InvalidChangeOperationException(String.format(
+              "Change %s was modified", change.getId()));
+        }
 
-          final ChangeMessage cmsg =
+        final ChangeMessage cmsg =
-              new ChangeMessage(new ChangeMessage.Key(changeId,
+            new ChangeMessage(new ChangeMessage.Key(changeId,
-                  ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId);
+                ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId);
-          final String msg = "Patch Set " + newPatchSet.getPatchSetId() + ": Commit message was updated";
+        final String msg = "Patch Set " + newPatchSet.getPatchSetId() + ": Commit message was updated";
-          cmsg.setMessage(msg);
+        cmsg.setMessage(msg);
-          db.changeMessages().insert(Collections.singleton(cmsg));
+        db.changeMessages().insert(Collections.singleton(cmsg));
-          db.commit();
+        db.commit();
 
           final CommitMessageEditedSender cm = commitMessageEditedSenderFactory.create(change);
           cm.setFrom(user.getAccountId());
           cm.setChangeMessage(cmsg);
           cm.send();
-        } finally {
-          db.rollback();
-        }
-
-        hooks.doPatchsetCreatedHook(change, newPatchSet, db);
-
-        return change.getId();
       } finally {
-        revWalk.release();
+        db.rollback();
       }
+
+      hooks.doPatchsetCreatedHook(change, newPatchSet, db);
+
+      return change.getId();
     } finally {
-      git.close();
+      revWalk.release();
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -58,7 +58,7 @@ public class CommitValidators {
   private static final FooterKey CHANGE_ID = new FooterKey("Change-Id");
 
   private static final Pattern NEW_PATCHSET = Pattern
-      .compile("^refs/changes/(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/new)?$");
+      .compile("^refs/changes/(?:[0-9][0-9])?(/[1-9][0-9]*){1,2}(?:/new)?$");
 
   public interface Factory {
     CommitValidators create(RefControl refControl, SshInfo sshInfo,

gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java

@@ -19,7 +19,7 @@ import com.jcraft.jsch.HostKey;
 import java.util.Collections;
 import java.util.List;
 
-class NoSshInfo implements SshInfo {
+public class NoSshInfo implements SshInfo {
   @Override
   public List<HostKey> getHostKeys() {
     return Collections.emptyList();