From 15d5ac0e705609dde13fcf7bc148e7b112370312 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Fri, 30 Nov 2012 16:12:16 +0100 Subject: [PATCH] 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 --- .../EditCommitMessageHandler.java | 27 ++- .../com/google/gerrit/server/ChangeUtil.java | 223 +++++++++--------- .../git/validators/CommitValidators.java | 2 +- .../google/gerrit/server/ssh/NoSshInfo.java | 2 +- 4 files changed, 140 insertions(+), 114 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java index 1ac6912309..c38bda72a4 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java +++ b/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 { 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 { 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 { 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 { "Not allowed to add new Patch Sets to: " + changeId.toString()); } - ChangeUtil.editCommitMessage(patchSetId, currentUser, message, db, - commitMessageEditedSenderFactory, hooks, gitManager, patchSetInfoFactory, - replication, myIdent); + final Repository git; + try { + 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(); + } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 1f53cbda1e..d20da3c58c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/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()); - } catch (RepositoryNotFoundException e) { - throw new NoSuchChangeException(changeId, e); - } + RevCommit commit = + revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); + if (commit.getFullMessage().equals(message)) { + throw new InvalidChangeOperationException("New commit message cannot be same as existing commit message"); + } - try { - final RevWalk revWalk = new RevWalk(git); + Date now = myIdent.getWhen(); + 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 = - revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); - if (commit.getFullMessage().equals(message)) { - throw new InvalidChangeOperationException("New commit message cannot be same as existing commit message"); + 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()); + + 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(); - 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 { - 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() { - @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; + ChangeUtil.insertAncestors(db, newPatchSet.getId(), commit); + db.patchSets().insert(Collections.singleton(newPatchSet)); + updatedChange = + db.changes().atomicUpdate(change.getId(), new AtomicUpdate() { + @Override + public Change update(Change change) { + if (change.getStatus().isClosed()) { + return null; } - }); - if (updatedChange != null) { - change = updatedChange; - } else { - throw new InvalidChangeOperationException(String.format( - "Change %s was modified", change.getId())); - } + 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 (updatedChange != null) { + change = updatedChange; + } else { + throw new InvalidChangeOperationException(String.format( + "Change %s was modified", change.getId())); + } - final ChangeMessage cmsg = - new ChangeMessage(new ChangeMessage.Key(changeId, - ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId); - final String msg = "Patch Set " + newPatchSet.getPatchSetId() + ": Commit message was updated"; - cmsg.setMessage(msg); - db.changeMessages().insert(Collections.singleton(cmsg)); - db.commit(); + final ChangeMessage cmsg = + new ChangeMessage(new ChangeMessage.Key(changeId, + ChangeUtil.messageUUID(db)), user.getAccountId(), patchSetId); + final String msg = "Patch Set " + newPatchSet.getPatchSetId() + ": Commit message was updated"; + cmsg.setMessage(msg); + db.changeMessages().insert(Collections.singleton(cmsg)); + 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(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 2e0c16b515..5e2f30b636 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/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, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java b/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java index 519b922e7d..6ceec2d446 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ssh/NoSshInfo.java +++ b/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 getHostKeys() { return Collections.emptyList();