From ea9231b39df100fa467282150ccef2994f1ef335 Mon Sep 17 00:00:00 2001 From: Yunjie Li Date: Thu, 15 Aug 2019 10:39:25 -0700 Subject: [PATCH] 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 Signed-off-by: Jonathan Nieder --- .../jgit/transport/AtomicPushTest.java | 26 ++++++++--------- .../jgit/transport/PushOptionsTest.java | 22 +++++++-------- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../jgit/transport/BaseReceivePack.java | 28 +++++++++++++------ 5 files changed, 45 insertions(+), 33 deletions(-) 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 c1e078d10d..d6c7a6199d 100644 --- 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 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 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 fd1c3bf8b8..18946e0d50 100644 --- 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 commands(boolean atomicSafe) throws IOException { List 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 5a4d9bb990..8dfbf6efbf 100644 --- 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 b80b7498b1..82f07126e1 100644 --- 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 e402de0158..954359ebaa 100644 --- 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, -- 2.39.5