FirstWant: tighten first-want line validation

First-want line parsing accepts lines with an optional whitespace, when
the spec is strict requiring a white space.

Validate the line enforcing that there is a white space between oid and
capabilities list.

Change-Id: I45ada67030e0720f9b402c298be18c7518c799b1
Signed-off-by: Ivan Frade <ifrade@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java

@@ -44,17 +44,20 @@ package org.eclipse.jgit.internal.transport.parser;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
+import org.eclipse.jgit.errors.PackProtocolException;
 import org.junit.Test;
 
 public class FirstWantTest {
 
 	@Test
-	public void testFirstWantWithOptions() {
+	public void testFirstWantWithOptions() throws PackProtocolException {
 		String line = "want b9d4d1eb2f93058814480eae9e1b67550f46ee38 "
 				+ "no-progress include-tag ofs-delta agent=JGit/unknown";
 
@@ -69,7 +72,7 @@ public class FirstWantTest {
 	}
 
 	@Test
-	public void testFirstWantWithoutOptions() {
+	public void testFirstWantWithoutOptions() throws PackProtocolException {
 		String line = "want b9d4d1eb2f93058814480eae9e1b67550f46ee38";
 
 		FirstWant r = FirstWant.fromLine(line);
@@ -77,4 +80,42 @@ public class FirstWantTest {
 				r.getLine());
 		assertTrue(r.getCapabilities().isEmpty());
 	}
+
+	private String makeFirstWantLine(String capability) {
+		return String.format("want b9d4d1eb2f93058814480eae9e1b67550f46ee38 %s", capability);
+	}
+
+	@Test
+	public void testFirstWantNoWhitespace() {
+		try {
+			FirstWant.fromLine(
+					"want b9d4d1eb2f93058814480eae9e1b67550f400000capability");
+			fail("Accepting first want line without SP between oid and first capability");
+		} catch (PackProtocolException e) {
+			// pass
+		}
+	}
+
+	@Test
+	public void testFirstWantOnlyWhitespace() throws PackProtocolException {
+		FirstWant r = FirstWant
+				.fromLine("want b9d4d1eb2f93058814480eae9e1b67550f46ee38 ");
+		assertEquals("want b9d4d1eb2f93058814480eae9e1b67550f46ee38",
+				r.getLine());
+	}
+
+	@Test
+	public void testFirstWantValidCapabilityNames()
+			throws PackProtocolException {
+		List<String> validNames = Arrays.asList(
+				"c", "cap", "C", "CAP", "1", "1cap", "cap-64k_test",
+				"-", "-cap",
+				"_", "_cap", "agent=pack.age/Version");
+
+		for (String capability: validNames) {
+			FirstWant r = FirstWant.fromLine(makeFirstWantLine(capability));
+			assertEquals(r.getCapabilities().size(), 1);
+			assertTrue(r.getCapabilities().contains(capability));
+		}
+	}
 }

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -771,6 +771,7 @@ URINotSupported=URI not supported: {0}
 userConfigFileInvalid=User config file {0} invalid {1}
 validatingGitModules=Validating .gitmodules files
 walkFailure=Walk failure.
+wantNoSpaceWithCapabilities=No space between oid and first capability in first want line
 wantNotValid=want {0} not valid
 weeksAgo={0} weeks ago
 windowSizeMustBeLesserThanLimit=Window size must be < limit

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -832,6 +832,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String userConfigFileInvalid;
 	/***/ public String validatingGitModules;
 	/***/ public String walkFailure;
+	/***/ public String wantNoSpaceWithCapabilities;
 	/***/ public String wantNotValid;
 	/***/ public String weeksAgo;
 	/***/ public String windowSizeMustBeLesserThanLimit;

org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java

@@ -42,10 +42,14 @@
  */
 package org.eclipse.jgit.internal.transport.parser;
 
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 
+import org.eclipse.jgit.errors.PackProtocolException;
+import org.eclipse.jgit.internal.JGitText;
+
 /**
  * In the pack negotiation phase (protocol v0/v1), the client sends a list of
  * wants. The first "want" line is special, as it (can) have a list of
@@ -74,20 +78,21 @@ public class FirstWant {
 	 * @param line
 	 *            line from the client.
 	 * @return an instance of FirstWant
+	 * @throws PackProtocolException
+	 *             if the line doesn't follow the protocol format.
 	 */
-	public static FirstWant fromLine(String line) {
+	public static FirstWant fromLine(String line) throws PackProtocolException {
 		String wantLine;
 		Set<String> capabilities;
 
 		if (line.length() > 45) {
-			final HashSet<String> opts = new HashSet<>();
 			String opt = line.substring(45);
-			if (opt.startsWith(" ")) { //$NON-NLS-1$
+			if (!opt.startsWith(" ")) { //$NON-NLS-1$
-				opt = opt.substring(1);
+				throw new PackProtocolException(JGitText.get().wantNoSpaceWithCapabilities);
-			}
-			for (String c : opt.split(" ")) { //$NON-NLS-1$
-				opts.add(c);
 			}
+			opt = opt.substring(1);
+			HashSet<String> opts = new HashSet<>(
+					Arrays.asList(opt.split(" "))); //$NON-NLS-1$
 			wantLine = line.substring(0, 45);
 			capabilities = Collections.unmodifiableSet(opts);
 		} else {

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

@@ -69,6 +69,7 @@ import java.io.EOFException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.io.UncheckedIOException;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -193,7 +194,11 @@ public class UploadPack {
 		 *            line from the client.
 		 */
 		public FirstLine(String line) {
-			firstWant = FirstWant.fromLine(line);
+			try {
+				firstWant = FirstWant.fromLine(line);
+			} catch (PackProtocolException e) {
+				throw new UncheckedIOException(e);
+			}
 		}
 
 		/** @return non-capabilities part of the line. */