UploadPack: Verify clients send only commits for shallow lines

If a client mistakenly tries to send a tag object as a shallow line
JGit blindly assumes this is a commit and tries to parse the tag
buffer using the commit parser. This can cause an obtuse error like:

  InvalidObjectIdException: Invalid id: t c0ff331234...

The "t" comes from the "object c0ff331234..." line of the tag tring
to be parsed as though it where the "tree" line of a commit.

Run any client supplied shallow lines through the RevWalk to lookup
the object types. Fail fast with a protocol exception if any of them
are non-commit.

Skip objects not known to this repository. This matches behavior
with git-core's upload-pack, which sliently skips over any shallow
line object named by the client but not known by the server.

Change-Id: Ic6c57a90a42813164ce65c2244705fc42e84d700

4 files changed, 35 insertions, 0 deletions

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 8ca3d9ae86..e2bed1873a 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -344,6 +344,7 @@ invalidPathReservedOnWindows=Invalid path (''{0}'' is reserved on Windows): {1}
 invalidReflogRevision=Invalid reflog revision: {0}
 invalidRefName=Invalid ref name: {0}
 invalidRemote=Invalid remote: {0}
+invalidShallowObject=invalid shallow object {0}, expected commit
 invalidStageForPath=Invalid stage {0} for path {1}
 invalidTagOption=Invalid tag option: {0}
 invalidTimeout=Invalid timeout: {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 c227884d58..d6cf6e6a7f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -403,6 +403,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String invalidReflogRevision;
 	/***/ public String invalidRefName;
 	/***/ public String invalidRemote;
+	/***/ public String invalidShallowObject;
 	/***/ public String invalidStageForPath;
 	/***/ public String invalidTagOption;
 	/***/ public String invalidTimeout;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
index afb208ecf9..1176d958b0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
@@ -1406,6 +1406,8 @@ public class RevWalk implements Iterable<RevCommit>, AutoCloseable {
 
 	/**
 	 * Assume additional commits are shallow (have no parents).
+	 * <p>
+	 * This method is a No-op if the collection is empty.
 	 *
 	 * @param ids
 	 *            commits that should be treated as shallow commits, in addition
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index e0d900db8c..101057fb4f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -735,6 +735,8 @@ public class UploadPack {
 			else
 				multiAck = MultiAck.OFF;
 
+			if (!clientShallowCommits.isEmpty())
+				verifyClientShallow();
 			if (depth != 0)
 				processShallow();
 			if (!clientShallowCommits.isEmpty())
@@ -820,6 +822,35 @@ public class UploadPack {
 		pckOut.end();
 	}
 
+	private void verifyClientShallow()
+			throws IOException, PackProtocolException {
+		AsyncRevObjectQueue q = walk.parseAny(clientShallowCommits, true);
+		try {
+			for (;;) {
+				try {
+					// Shallow objects named by the client must be commits.
+					RevObject o = q.next();
+					if (o == null) {
+						break;
+					}
+					if (!(o instanceof RevCommit)) {
+						throw new PackProtocolException(
+							MessageFormat.format(
+								JGitText.get().invalidShallowObject,
+								o.name()));
+					}
+				} catch (MissingObjectException notCommit) {
+					// shallow objects not known at the server are ignored
+					// by git-core upload-pack, match that behavior.
+					clientShallowCommits.remove(notCommit.getObjectId());
+					continue;
+				}
+			}
+		} finally {
+			q.release();
+		}
+	}
+
 	/**
 	 * Generate an advertisement of available refs and capabilities.
 	 *