OpenSshConfigFile: line comments and quoted strings

Bring our SSH config parser up-to-date with respect to changes in
OpenSSH. In particular, they fixed[1] the handling of line comments
such that #-characters inside strings are not considered. This means
that we have to parse strings with escaped quotes correctly.

[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Change-Id: Ifbd9014127e8d51e7c8792e237f3fc2a9a0719d2
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

3 files changed, 153 insertions, 44 deletions

diff --git a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/OpenSshConfigTest.java b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/OpenSshConfigTest.java
index 4399ad9be6..9a61cc4c55 100644
--- a/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/OpenSshConfigTest.java
+++ b/org.eclipse.jgit.ssh.jsch.test/tst/org/eclipse/jgit/transport/ssh/jsch/OpenSshConfigTest.java
@@ -138,7 +138,7 @@ public class OpenSshConfigTest extends RepositoryTestCase {
 		assertEquals(2222, osc.lookup("unquoted").getPort());
 		assertEquals(2222, osc.lookup("hosts").getPort());
 		assertEquals(" spaced\ttld ", osc.lookup("spaced").getHostName());
-		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
+		assertEquals("bad.tld", osc.lookup("bad").getHostName());
 	}
 
 	@Test
@@ -229,7 +229,7 @@ public class OpenSshConfigTest extends RepositoryTestCase {
 	@Test
 	public void testAlias_InheritPreferredAuthentications() throws Exception {
 		config("Host orcz\n" + "\tHostName repo.or.cz\n" + "\n" + "Host *\n"
-				+ "\tPreferredAuthentications publickey, hostbased\n");
+				+ "\tPreferredAuthentications 'publickey, hostbased'\n");
 		final Host h = osc.lookup("orcz");
 		assertNotNull(h);
 		assertEquals("publickey,hostbased", h.getPreferredAuthentications());
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFileTest.java
index 27bae3747c..11741b41aa 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFileTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFileTest.java
@@ -166,7 +166,38 @@ public class OpenSshConfigFileTest extends RepositoryTestCase {
 		assertPort(2222, lookup("unquoted"));
 		assertPort(2222, lookup("hosts"));
 		assertHost(" spaced\ttld ", lookup("spaced"));
-		assertHost("bad.tld\"", lookup("bad"));
+		assertHost("bad.tld", lookup("bad"));
+	}
+
+	@Test
+	public void testAdvancedParsing() throws Exception {
+		// Escaped quotes, and line comments
+		config("Host foo\n"
+				+ " HostName=\"foo\\\"d.tld\"\n"
+				+ " User= someone#foo\n"
+				+ "Host bar\n"
+				+ " User ' some one#two' # Comment\n"
+				+ " GlobalKnownHostsFile '/a folder/with spaces/hosts' '/other/more hosts' # Comment\n"
+				+ "Host foobar\n"
+				+ " User a\\ u\\ thor\n"
+				+ "Host backslash\n"
+				+ " User some\\one\\\\\\ foo\n"
+				+ "Host backslash_before_quote\n"
+				+ " User \\\"someone#\"el#se\" #Comment\n"
+				+ "Host backslash_in_quote\n"
+				+ " User 'some\\one\\\\\\ foo'\n");
+		assertHost("foo\"d.tld", lookup("foo"));
+		assertUser("someone#foo", lookup("foo"));
+		HostConfig c = lookup("bar");
+		assertUser(" some one#two", c);
+		assertArrayEquals(
+				new Object[] { "/a folder/with spaces/hosts",
+						"/other/more hosts" },
+				c.getValues("GlobalKnownHostsFile").toArray());
+		assertUser("a u thor", lookup("foobar"));
+		assertUser("some\\one\\ foo", lookup("backslash"));
+		assertUser("\"someone#el#se", lookup("backslash_before_quote"));
+		assertUser("some\\one\\\\ foo", lookup("backslash_in_quote"));
 	}
 
 	@Test
@@ -258,7 +289,7 @@ public class OpenSshConfigFileTest extends RepositoryTestCase {
 	@Test
 	public void testAlias_InheritPreferredAuthentications() throws Exception {
 		config("Host orcz\n" + "\tHostName repo.or.cz\n" + "\n" + "Host *\n"
-				+ "\tPreferredAuthentications publickey, hostbased\n");
+				+ "\tPreferredAuthentications 'publickey, hostbased'\n");
 		final HostConfig h = lookup("orcz");
 		assertNotNull(h);
 		assertEquals("publickey,hostbased",
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java
index 4d1864a92c..2dbb7859b9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/ssh/OpenSshConfigFile.java
@@ -230,27 +230,39 @@ public class OpenSshConfigFile implements SshConfigStore {
 
 		String line;
 		while ((line = reader.readLine()) != null) {
-			// OpenSsh ignores trailing comments on a line. Anything after the
-			// first # on a line is trimmed away (yes, even if the hash is
-			// inside quotes).
-			//
-			// See https://github.com/openssh/openssh-portable/commit/2bcbf679
-			int i = line.indexOf('#');
-			if (i >= 0) {
-				line = line.substring(0, i);
-			}
-			line = line.trim();
+			line = line.strip();
 			if (line.isEmpty()) {
 				continue;
 			}
 			String[] parts = line.split("[ \t]*[= \t]", 2); //$NON-NLS-1$
-			// Although the ssh-config man page doesn't say so, the openssh
-			// parser does allow quoted keywords.
-			String keyword = dequote(parts[0].trim());
+			String keyword = parts[0].strip();
+			if (keyword.isEmpty()) {
+				continue;
+			}
+			switch (keyword.charAt(0)) {
+			case '#':
+				continue;
+			case '"':
+				// Although the ssh-config man page doesn't say so, the openssh
+				// parser does allow quoted keywords.
+				List<String> dequoted = parseList(keyword);
+				keyword = dequoted.isEmpty() ? "" : dequoted.get(0); //$NON-NLS-1$
+				break;
+			default:
+				// Keywords never contain hashes, nor whitespace
+				int i = keyword.indexOf('#');
+				if (i >= 0) {
+					keyword = keyword.substring(0, i);
+				}
+				break;
+			}
+			if (keyword.isEmpty()) {
+				continue;
+			}
 			// man 5 ssh-config says lines had the format "keyword arguments",
 			// with no indication that arguments were optional. However, let's
 			// not crap out on missing arguments. See bug 444319.
-			String argValue = parts.length > 1 ? parts[1].trim() : ""; //$NON-NLS-1$
+			String argValue = parts.length > 1 ? parts[1].strip() : ""; //$NON-NLS-1$
 
 			if (StringUtils.equalsIgnoreCase(SshConstants.HOST, keyword)) {
 				current = new HostEntry(parseList(argValue));
@@ -262,7 +274,9 @@ public class OpenSshConfigFile implements SshConfigStore {
 				List<String> args = validate(keyword, parseList(argValue));
 				current.setValue(keyword, args);
 			} else if (!argValue.isEmpty()) {
-				argValue = validate(keyword, dequote(argValue));
+				List<String> args = parseList(argValue);
+				String arg = args.isEmpty() ? "" : args.get(0); //$NON-NLS-1$
+				argValue = validate(keyword, arg);
 				current.setValue(keyword, argValue);
 			}
 		}
@@ -273,6 +287,7 @@ public class OpenSshConfigFile implements SshConfigStore {
 	/**
 	 * Splits the argument into a list of whitespace-separated elements.
 	 * Elements containing whitespace must be quoted and will be de-quoted.
+	 * Backslash-escapes are handled for quotes and blanks.
 	 *
 	 * @param argument
 	 *            argument part of the configuration line as read from the
@@ -280,35 +295,105 @@ public class OpenSshConfigFile implements SshConfigStore {
 	 * @return a {@link List} of elements, possibly empty and possibly
 	 *         containing empty elements, but not containing {@code null}
 	 */
-	private List<String> parseList(String argument) {
+	private static List<String> parseList(String argument) {
 		List<String> result = new ArrayList<>(4);
 		int start = 0;
 		int length = argument.length();
 		while (start < length) {
 			// Skip whitespace
-			if (Character.isWhitespace(argument.charAt(start))) {
+			char ch = argument.charAt(start);
+			if (Character.isWhitespace(ch)) {
 				start++;
-				continue;
+			} else if (ch == '#') {
+				break; // Comment start
+			} else {
+				// Parse one token now.
+				start = parseToken(argument, start, length, result);
 			}
-			if (argument.charAt(start) == '"') {
-				int stop = argument.indexOf('"', ++start);
-				if (stop < start) {
-					// No closing double quote: skip
-					break;
+		}
+		return result;
+	}
+
+	/**
+	 * Parses a token up to the next whitespace not inside a string quoted by
+	 * single or double quotes. Inside a string, quotes can be escaped by
+	 * backslash characters. Outside of a string, "\ " can be used to include a
+	 * space in a token; inside a string "\ " is taken literally as '\' followed
+	 * by ' '.
+	 *
+	 * @param argument
+	 *            to parse the token out of
+	 * @param from
+	 *            index at the beginning of the token
+	 * @param to
+	 *            index one after the last character to look at
+	 * @param result
+	 *            a list collecting tokens to which the parsed token is added
+	 * @return the index after the token
+	 */
+	private static int parseToken(String argument, int from, int to,
+			List<String> result) {
+		StringBuilder b = new StringBuilder();
+		int i = from;
+		char quote = 0;
+		boolean escaped = false;
+		SCAN: while (i < to) {
+			char ch = argument.charAt(i);
+			switch (ch) {
+			case '"':
+			case '\'':
+				if (quote == 0) {
+					if (escaped) {
+						b.append(ch);
+					} else {
+						quote = ch;
+					}
+				} else if (!escaped && quote == ch) {
+					quote = 0;
+				} else {
+					b.append(ch);
 				}
-				result.add(argument.substring(start, stop));
-				start = stop + 1;
-			} else {
-				int stop = start + 1;
-				while (stop < length
-						&& !Character.isWhitespace(argument.charAt(stop))) {
-					stop++;
+				escaped = false;
+				break;
+			case '\\':
+				if (escaped) {
+					b.append(ch);
+				}
+				escaped = !escaped;
+				break;
+			case ' ':
+				if (quote == 0) {
+					if (escaped) {
+						b.append(ch);
+						escaped = false;
+					} else {
+						break SCAN;
+					}
+				} else {
+					if (escaped) {
+						b.append('\\');
+					}
+					b.append(ch);
+					escaped = false;
+				}
+				break;
+			default:
+				if (escaped) {
+					b.append('\\');
+				}
+				if (quote == 0 && Character.isWhitespace(ch)) {
+					break SCAN;
 				}
-				result.add(argument.substring(start, stop));
-				start = stop + 1;
+				b.append(ch);
+				escaped = false;
+				break;
 			}
+			i++;
 		}
-		return result;
+		if (b.length() > 0) {
+			result.add(b.toString());
+		}
+		return i;
 	}
 
 	/**
@@ -358,13 +443,6 @@ public class OpenSshConfigFile implements SshConfigStore {
 		return pattern.equals(name);
 	}
 
-	private static String dequote(String value) {
-		if (value.startsWith("\"") && value.endsWith("\"") //$NON-NLS-1$ //$NON-NLS-2$
-				&& value.length() > 1)
-			return value.substring(1, value.length() - 1);
-		return value;
-	}
-
 	private static String stripWhitespace(String value) {
 		final StringBuilder b = new StringBuilder();
 		int length = value.length();