BaseReceivePack: More validation during parseCommand

Change-Id: I25f3a5582a45dd0ec8f78f5daf74c2203797a184

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

@@ -13,17 +13,17 @@
  * conditions are met:
  *
  * - Redistributions of source code must retain the above copyright
- *	 notice, this list of conditions and the following disclaimer.
+ *   notice, this list of conditions and the following disclaimer.
  *
  * - Redistributions in binary form must reproduce the above
- *	 copyright notice, this list of conditions and the following
+ *   copyright notice, this list of conditions and the following
- *	 disclaimer in the documentation and/or other materials provided
+ *   disclaimer in the documentation and/or other materials provided
- *	 with the distribution.
+ *   with the distribution.
  *
  * - Neither the name of the Eclipse Foundation, Inc. nor the
- *	 names of its contributors may be used to endorse or promote
+ *   names of its contributors may be used to endorse or promote
- *	 products derived from this software without specific prior
+ *   products derived from this software without specific prior
- *	 written permission.
+ *   written permission.
  *
  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
  * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
@@ -43,7 +43,9 @@
 package org.eclipse.jgit.transport;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
+import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
@@ -57,14 +59,34 @@ public class BaseReceivePackTest {
 	}
 
 	@Test
-	public void parseCommand() {
+	public void parseCommand() throws Exception {
-		String input = "0000000000000000000000000000000000000000"
+		String o = "0000000000000000000000000000000000000000";
-				+ " deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"
+		String n = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
-				+ " refs/heads/master";
+		String r = "refs/heads/master";
-		ReceiveCommand cmd = BaseReceivePack.parseCommand(input);
+		ReceiveCommand cmd = BaseReceivePack.parseCommand(o + " " + n + " " + r);
 		assertEquals(ObjectId.zeroId(), cmd.getOldId());
 		assertEquals("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				cmd.getNewId().name());
 		assertEquals("refs/heads/master", cmd.getRefName());
+
+		assertParseCommandFails(null);
+		assertParseCommandFails("");
+		assertParseCommandFails(o.substring(35) + " " + n.substring(35)
+				+ " " + r + "\n");
+		assertParseCommandFails(o + " " + n + " " + r + "\n");
+		assertParseCommandFails(o + " " + n + " " + "refs^foo");
+		assertParseCommandFails(o + " " + n.substring(10) + " " + r);
+		assertParseCommandFails(o.substring(10) + " " + n + " " + r);
+		assertParseCommandFails("X" + o.substring(1) + " " + n + " " + r);
+		assertParseCommandFails(o + " " + "X" + n.substring(1) + " " + r);
+	}
+
+	private void assertParseCommandFails(String input) {
+		try {
+			BaseReceivePack.parseCommand(input);
+			fail();
+		} catch (PackProtocolException e) {
+			// Expected.
+		}
 	}
 }

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

@@ -114,10 +114,10 @@ public class PushCertificateParserTest {
 		ObjectId oldId = ObjectId.zeroId();
 		ObjectId newId =
 				ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
-		String rawLine =
+		String line = oldId.name() + " " + newId.name() + " refs/heads/master";
-				oldId.name() + " " + newId.name() + " refs/heads/master";
+		String rawLine = line + "\n";
-		ReceiveCommand cmd = BaseReceivePack.parseCommand(rawLine);
 
+		ReceiveCommand cmd = BaseReceivePack.parseCommand(line);
 		parser.addCommand(cmd, rawLine);
 		parser.addCommand(rawLine);
 		assertNull(parser.build());

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

@@ -1100,13 +1100,13 @@ public abstract class BaseReceivePack {
 				continue;
 			}
 
-			if (line.length() < 83) {
+			ReceiveCommand cmd;
-				final String m = JGitText.get().errorInvalidProtocolWantedOldNewRef;
+			try {
-				sendError(m);
+				cmd = parseCommand(line);
-				throw new PackProtocolException(m);
+			} catch (PackProtocolException e) {
+				sendError(e.getMessage());
+				throw e;
 			}
-
-			final ReceiveCommand cmd = parseCommand(line);
 			if (cmd.getRefName().equals(Constants.HEAD)) {
 				cmd.setResult(Result.REJECTED_CURRENT_BRANCH);
 			} else {
@@ -1129,10 +1129,26 @@ public abstract class BaseReceivePack {
 		return line;
 	}
 
-	static ReceiveCommand parseCommand(String line) {
+	static ReceiveCommand parseCommand(String line) throws PackProtocolException {
-		ObjectId oldId = ObjectId.fromString(line.substring(0, 40));
+          if (line == null || line.length() < 83) {
-		ObjectId newId = ObjectId.fromString(line.substring(41, 81));
+			throw new PackProtocolException(
+					JGitText.get().errorInvalidProtocolWantedOldNewRef);
+		}
+		String oldStr = line.substring(0, 40);
+		String newStr = line.substring(41, 81);
+		ObjectId oldId, newId;
+		try {
+			oldId = ObjectId.fromString(oldStr);
+			newId = ObjectId.fromString(newStr);
+		} catch (IllegalArgumentException e) {
+			throw new PackProtocolException(
+					JGitText.get().errorInvalidProtocolWantedOldNewRef, e);
+		}
 		String name = line.substring(82);
+		if (!Repository.isValidRefName(name)) {
+			throw new PackProtocolException(
+					JGitText.get().errorInvalidProtocolWantedOldNewRef);
+		}
 		return new ReceiveCommand(oldId, newId, name);
 	}
 

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

@@ -294,9 +294,11 @@ public class PushCertificateParser {
 	 * @param rawLine
 	 *            the exact line read from the wire that produced this
 	 *            command, including trailing newline if present.
+	 * @throws PackProtocolException
+	 *             if the raw line cannot be parsed to a command.
 	 * @since 4.0
 	 */
-	public void addCommand(String rawLine) {
+	public void addCommand(String rawLine) throws PackProtocolException {
 		commands.add(parseCommand(chomp(rawLine)));
 		rawCommands.append(rawLine);
 	}