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>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/AtomicPushTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/AtomicPushTest.java
index c1e078d10dce3a2bfcdb501cdfe9dc90f0a77c68..d6c7a6199dff6cd348ee69078b42e955f6e35828 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/AtomicPushTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/AtomicPushTest.java
@@ -55,10 +55,9 @@ import org.eclipse.jgit.errors.TransportException;
 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 class AtomicPushTest {
                                });
                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 class AtomicPushTest {
                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 @@ public class AtomicPushTest {
                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;
        }
 }

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushOptionsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushOptionsTest.java
index fd1c3bf8b837130c743ba27a29dca60bf242f8eb..18946e0d50f086c0b9913c2d5c24e4da1cec193c 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushOptionsTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushOptionsTest.java
@@ -62,10 +62,9 @@ import org.eclipse.jgit.api.errors.TransportException;
 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 class PushOptionsTest extends RepositoryTestCase {
 
                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 @@ public class PushOptionsTest extends RepositoryTestCase {
        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;
        }
 

diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 5a4d9bb9906dc7c34ad6515929e08aa9bdf1fa94..8dfbf6efbf8eb1ea04597d8ddab6ec793391ac62 100644 (file)
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/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}

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index b80b7498b161934157936455b53fc81f255e4bea..82f07126e12029d91962f296f4052617d6ae9115 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -559,6 +559,7 @@ public class JGitText extends TranslationBundle {
        /***/ public String noMergeBase;
        /***/ public String noMergeHeadSpecified;
        /***/ public String nonBareLinkFilesNotSupported;
+       /***/ public String nonCommitToHeads;
        /***/ public String noPathAttributesFound;
        /***/ public String noSuchRef;
        /***/ public String noSuchSubmodule;

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
index e402de015823442af6bb44849693863e96356af2..954359ebaa713fef5fbe540b960716dce11e66d4 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
@@ -1613,6 +1613,24 @@ public abstract class BaseReceivePack {
                        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 @@ public abstract class BaseReceivePack {
 
                                // 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 @@ public abstract class BaseReceivePack {
                                        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,