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 c1e078d..d6c7a61 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.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;
}
}
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 fd1c3bf..18946e0 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.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;
}
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 5a4d9bb..8dfbf6e 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 @@
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 b80b749..82f0712 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 static JGitText get() {
/***/ 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 e402de0..954359e 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 @@ 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,
