[JGIT PATCH] Add tests for handling of parsing errors in OpenSshConfig

From

Jonas Fonseca <fonseca@diku.dk>

Date

Sep 25, 2008, 08:39 UTC

Message-ID

<20080925083934.GB10273@diku.dk>

In-Reply-To

<20080924233104.GG3669@spearce.org>

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 .../spearce/jgit/transport/OpenSshConfigTest.java  |   32 +++++++++++++++---
 .../org/spearce/jgit/transport/OpenSshConfig.java  |   34 +++++++++++++++----
 2 files changed, 53 insertions(+), 13 deletions(-)

 Shawn O. Pearce <spearce@spearce.org> wrote Wed, Sep 24, 2008:
 > Jonas Fonseca <fonseca@diku.dk> wrote:
 > > I propose to simply remove these hosts from the host map and clear
 > > the current host list so that no values will be saved, effectively
 > > causing invalid hosts to result in the same as unknown hosts.
 > 
 > Yea, that seems quite reasonable.
 > 
 > If you want more debugging than that on your ~/.ssh/config file then
 > run OpenSSH tools on it.  Hell, I can't count the number of times
 > I've made typos in there and couldn't figure out why it was still
 > asking for a password, etc.  And that's just the OpenSSH command
 > line tools.

 Heh, so below makes JGit compatible with OpenSSH's silent treatment. I
 also fixed the .ssh/config parser to ignore negative port numbers.

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java
index ad6e79c..ccd7400 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java
@@ -114,11 +114,7 @@ config("Host \"good\"\n" +
 			" Port=\"2222\"\n" +
 			"Host \"spaced\"\n" +
 			"# Bad host name, but testing preservation of spaces\n" +
-			" HostName=\" spaced\ttld \"\n" +
-			"# Misbalanced quotes\n" +
-			"Host \"bad\"\n" +
-			"# OpenSSH doesn't allow this but ...\n" +
-			" HostName=bad.tld\"\n");
+			" HostName=\" spaced\ttld \"\n");
 		assertEquals("good.tld", osc.lookup("good").getHostName());
 		assertEquals("gooduser", osc.lookup("good").getUser());
 		assertEquals(6007, osc.lookup("good").getPort());
@@ -128,7 +124,31 @@ config("Host \"good\"\n" +
 		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());
+	}
+
+	public void testParsingErrors() throws Exception {
+		config("Host negativeportnumber\n" +
+		       "Port -200\n" +
+		       "Host badportnumber\n" +
+		       "Port twentytwo\n" +
+		       "Host badportquote\n" +
+		       "Port \"2222\n" +
+		       "Host badportquote2\n" +
+		       "Port 2222\"\n" +
+		       "# Misbalanced quotes\n" +
+		       "Host badly \"quoted\n" +
+		       "HostName=unknown\n");
+
+		assertEquals(DefaultSshSessionFactory.SSH_PORT,
+			     osc.lookup("negativeportnumber").getPort());
+		assertEquals(DefaultSshSessionFactory.SSH_PORT,
+			     osc.lookup("badportnumber").getPort());
+		assertEquals(DefaultSshSessionFactory.SSH_PORT,
+			     osc.lookup("badportquote").getPort());
+		assertEquals(DefaultSshSessionFactory.SSH_PORT,
+			     osc.lookup("badportquote2").getPort());
+		assertNotSame("unknown", osc.lookup("badly").getHostName());
+		assertNotSame("unknown", osc.lookup("\"quoted"));
 	}
 
 	public void testAlias_DoesNotMatch() throws Exception {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
index b08d5c6..6a3f2c1 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
@@ -46,6 +46,7 @@
 import java.io.InputStreamReader;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -175,6 +176,15 @@ public Host lookup(final String hostName) {
 				current.clear();
 				for (final String pattern : argValue.split("[ \t]")) {
 					final String name = dequote(pattern);
+					if (name == null) {
+						/* Prune all the current hosts. */
+						Iterator<Host> it = m.values().iterator();
+						while (it.hasNext())
+							if (current.contains(it.next()))
+								it.remove();
+						current.clear();
+						break;
+					}
 					Host c = m.get(name);
 					if (c == null) {
 						c = new Host();
@@ -192,17 +202,23 @@ public Host lookup(final String hostName) {
 				continue;
 			}
 
+			final String value = dequote(argValue);
+			if (value == null)
+				continue;
+
 			if ("HostName".equalsIgnoreCase(keyword)) {
 				for (final Host c : current)
 					if (c.hostName == null)
-						c.hostName = dequote(argValue);
+						c.hostName = value;
 			} else if ("User".equalsIgnoreCase(keyword)) {
 				for (final Host c : current)
 					if (c.user == null)
-						c.user = dequote(argValue);
+						c.user = value;
 			} else if ("Port".equalsIgnoreCase(keyword)) {
 				try {
-					final int port = Integer.parseInt(dequote(argValue));
+					final int port = Integer.parseInt(value);
+					if (port < 0)
+						continue;
 					for (final Host c : current)
 						if (c.port == 0)
 							c.port = port;
@@ -212,15 +228,15 @@ public Host lookup(final String hostName) {
 			} else if ("IdentityFile".equalsIgnoreCase(keyword)) {
 				for (final Host c : current)
 					if (c.identityFile == null)
-						c.identityFile = toFile(dequote(argValue));
+						c.identityFile = toFile(value);
 			} else if ("PreferredAuthentications".equalsIgnoreCase(keyword)) {
 				for (final Host c : current)
 					if (c.preferredAuthentications == null)
-						c.preferredAuthentications = nows(dequote(argValue));
+						c.preferredAuthentications = nows(value);
 			} else if ("BatchMode".equalsIgnoreCase(keyword)) {
 				for (final Host c : current)
 					if (c.batchMode == null)
-						c.batchMode = yesno(dequote(argValue));
+						c.batchMode = yesno(value);
 			}
 		}
 
@@ -243,8 +259,12 @@ private static boolean isHostMatch(final String pattern, final String name) {
 	}
 
 	private static String dequote(final String value) {
-		if (value.startsWith("\"") && value.endsWith("\""))
+		final boolean hasStart = value.startsWith("\"");
+		final boolean hasEnd = value.endsWith("\"");
+		if (hasStart && hasEnd)
 			return value.substring(1, value.length() - 1);
+		if (hasStart || hasEnd)
+			return null;
 		return value;
 	}
 

-- 
tg: (cf67dfc..) jf/opensshconfigquote (depends on: master)

-- 
Jonas Fonseca