ReceivePack: Prevent pointing a branch to a non-commit object

Since commit c3b0dec509fe136c5417422f31898b5a4e2d5e02, Git has
disallowed writing a non-commit to refs/heads/* refs.  JGit still
allows that, which can put users in a bad situation.  For example,

	git push origin v1.0:master

pushes the tag object v1.0 to refs/heads/master, instead of the
intended commit object v1.0^{commit}.

Prevent that by validating that the target of the ref points to a
commit object when pushing to refs/heads/*.

Git performs the same check at a lower level (in the RefDatabase).  We
could do the same here, but for now let's start conservatively by
handling it in pushes first.

[jn: fleshed out commit message]

Change-Id: I8f98ae6d8acbcd5ef7553ec732bc096cb6eb7c4e
Signed-off-by: Yunjie Li <yunjieli@google.com>
Signed-off-by: Jonathan Nieder <jrn@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/AtomicPushTest.java

@@ -55,10 +55,9 @@
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
-import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.resolver.ReceivePackFactory;
 import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException;
@@ -73,8 +72,8 @@ public class AtomicPushTest {
 	private Object ctx = new Object();
 	private InMemoryRepository server;
 	private InMemoryRepository client;
-	private ObjectId obj1;
-	private ObjectId obj2;
+	private ObjectId commit1;
+	private ObjectId commit2;
 
 	@Before
 	public void setUp() throws Exception {
@@ -92,10 +91,11 @@ public ReceivePack create(Object req, Repository db)
 				});
 		uri = testProtocol.register(ctx, server);
 
-		try (ObjectInserter ins = client.newObjectInserter()) {
-			obj1 = ins.insert(Constants.OBJ_BLOB, Constants.encode("test"));
-			obj2 = ins.insert(Constants.OBJ_BLOB, Constants.encode("file"));
-			ins.flush();
+		try (TestRepository<?> clientRepo = new TestRepository<>(client)) {
+			commit1 = clientRepo.commit().noFiles().message("test commit 1")
+					.create();
+			commit2 = clientRepo.commit().noFiles().message("test commit 2")
+					.create();
 		}
 	}
 
@@ -149,13 +149,13 @@ public void pushAtomicDisabled() throws Exception {
 		List<RemoteRefUpdate> cmds = new ArrayList<>();
 		cmds.add(new RemoteRefUpdate(
 				null, null,
-				obj1, "refs/heads/one",
+				commit1, "refs/heads/one",
 				true /* force update */,
 				null /* no local tracking ref */,
 				ObjectId.zeroId()));
 		cmds.add(new RemoteRefUpdate(
 				null, null,
-				obj2, "refs/heads/two",
+				commit2, "refs/heads/two",
 				true /* force update */,
 				null /* no local tracking ref */,
 				ObjectId.zeroId()));
@@ -176,16 +176,16 @@ private List<RemoteRefUpdate> commands() throws IOException {
 		List<RemoteRefUpdate> cmds = new ArrayList<>();
 		cmds.add(new RemoteRefUpdate(
 				null, null,
-				obj1, "refs/heads/one",
+				commit1, "refs/heads/one",
 				true /* force update */,
 				null /* no local tracking ref */,
 				ObjectId.zeroId()));
 		cmds.add(new RemoteRefUpdate(
 				null, null,
-				obj2, "refs/heads/two",
+				commit2, "refs/heads/two",
 				true /* force update */,
 				null /* no local tracking ref */,
-				obj1));
+				commit1));
 		return cmds;
 	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushOptionsTest.java

@@ -62,10 +62,9 @@
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.RepositoryTestCase;
-import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -79,8 +78,8 @@ public class PushOptionsTest extends RepositoryTestCase {
 	private Object ctx = new Object();
 	private InMemoryRepository server;
 	private InMemoryRepository client;
-	private ObjectId obj1;
-	private ObjectId obj2;
+	private ObjectId commit1;
+	private ObjectId commit2;
 	private ReceivePack receivePack;
 
 	@Override
@@ -101,10 +100,11 @@ public void setUp() throws Exception {
 
 		uri = testProtocol.register(ctx, server);
 
-		try (ObjectInserter ins = client.newObjectInserter()) {
-			obj1 = ins.insert(Constants.OBJ_BLOB, Constants.encode("test"));
-			obj2 = ins.insert(Constants.OBJ_BLOB, Constants.encode("file"));
-			ins.flush();
+		try (TestRepository<?> clientRepo = new TestRepository<>(client)) {
+			commit1 = clientRepo.commit().noFiles().message("test commit 1")
+					.create();
+			commit2 = clientRepo.commit().noFiles().message("test commit 2")
+					.create();
 		}
 	}
 
@@ -121,12 +121,12 @@ private static InMemoryRepository newRepo(String name) {
 	private List<RemoteRefUpdate> commands(boolean atomicSafe)
 			throws IOException {
 		List<RemoteRefUpdate> cmds = new ArrayList<>();
-		cmds.add(new RemoteRefUpdate(null, null, obj1, "refs/heads/one",
+		cmds.add(new RemoteRefUpdate(null, null, commit1, "refs/heads/one",
 				true /* force update */, null /* no local tracking ref */,
 				ObjectId.zeroId()));
-		cmds.add(new RemoteRefUpdate(null, null, obj2, "refs/heads/two",
+		cmds.add(new RemoteRefUpdate(null, null, commit2, "refs/heads/two",
 				true /* force update */, null /* no local tracking ref */,
-				atomicSafe ? ObjectId.zeroId() : obj1));
+				atomicSafe ? ObjectId.zeroId() : commit1));
 		return cmds;
 	}
 

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -498,6 +498,7 @@ noHMACsupport=No {0} support: {1}
 noMergeBase=No merge base could be determined. Reason={0}. {1}
 noMergeHeadSpecified=No merge head specified
 nonBareLinkFilesNotSupported=Link files are not supported with nonbare repos
+nonCommitToHeads=Cannot point a branch to a non-commit object
 noPathAttributesFound=No Attributes found for {0}.
 noSuchRef=no such ref
 noSuchSubmodule=no such submodule {0}

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -559,6 +559,7 @@ public static JGitText get() {
 	/***/ public String noMergeBase;
 	/***/ public String noMergeHeadSpecified;
 	/***/ public String nonBareLinkFilesNotSupported;
+	/***/ public String nonCommitToHeads;
 	/***/ public String noPathAttributesFound;
 	/***/ public String noSuchRef;
 	/***/ public String noSuchSubmodule;

org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java

@@ -1613,6 +1613,24 @@ protected void validateCommands() {
 			if (cmd.getResult() != Result.NOT_ATTEMPTED)
 				continue;
 
+			RevObject newObj = null;
+			if (cmd.getType() == ReceiveCommand.Type.CREATE
+					|| cmd.getType() == ReceiveCommand.Type.UPDATE) {
+				try {
+					newObj = walk.parseAny(cmd.getNewId());
+				} catch (IOException e) {
+					cmd.setResult(Result.REJECTED_MISSING_OBJECT,
+							cmd.getNewId().name());
+					continue;
+				}
+				if (cmd.getRefName().startsWith(Constants.R_HEADS)
+						&& !(newObj instanceof RevCommit)) {
+					cmd.setResult(Result.REJECTED_OTHER_REASON,
+							JGitText.get().nonCommitToHeads);
+					continue;
+				}
+			}
+
 			if (cmd.getType() == ReceiveCommand.Type.DELETE) {
 				if (!isAllowDeletes()) {
 					// Deletes are not supported on this repository.
@@ -1694,7 +1712,7 @@ protected void validateCommands() {
 
 				// Is this possibly a non-fast-forward style update?
 				//
-				RevObject oldObj, newObj;
+				RevObject oldObj;
 				try {
 					oldObj = walk.parseAny(cmd.getOldId());
 				} catch (IOException e) {
@@ -1703,14 +1721,6 @@ protected void validateCommands() {
 					continue;
 				}
 
-				try {
-					newObj = walk.parseAny(cmd.getNewId());
-				} catch (IOException e) {
-					cmd.setResult(Result.REJECTED_MISSING_OBJECT, cmd
-							.getNewId().name());
-					continue;
-				}
-
 				if (oldObj instanceof RevCommit && newObj instanceof RevCommit) {
 					try {
 						if (walk.isMergedInto((RevCommit) oldObj,