[JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing

[JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing

[Gilion Goudsmit]

Having only tab-characters separating a key and value in the
users ~/.ssh/config would cause the config-parser to fail with
a "String index out of range: -1" exception. Also simplified
the line parsing code my using a two argument split.

Signed-off-by: Gilion Goudsmit <ggoudsmit@shebang.nl>
---
 .../org/spearce/jgit/transport/OpenSshConfig.java  |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

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 df38e18..5bfcf5f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
@@ -167,17 +167,9 @@ while ((line = br.readLine()) != null) {
 			if (line.length() == 0 || line.startsWith("#"))
 				continue;
 
-			final int sp = line.indexOf(' ');
-			final int eq = line.indexOf('=');
-			final int splitAt;
-			if (sp >= 0 && eq >= 0)
-				splitAt = Math.min(sp, eq);
-			else if (sp < 0)
-				splitAt = eq;
-			else
-				splitAt = sp;
-			final String keyword = line.substring(0, splitAt).trim();
-			final String argValue = line.substring(splitAt + 1).trim();
+			final String[] parts = line.split("[ \t=]", 2);
+			final String keyword = parts[0].trim();
+			final String argValue = parts[1].trim();
 
 			if ("Host".equalsIgnoreCase(keyword)) {
 				current.clear();
-- 
1.5.3.8

[JGIT PATCH] Add test for OpenSshConfig separator parsing

[Jonas Fonseca]

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 .../spearce/jgit/transport/OpenSshConfigTest.java  |   22 ++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

 Gilion Goudsmit <ggoudsmit@shebang.nl> wrote Sat, Sep 20, 2008:
 > Having only tab-characters separating a key and value in the
 > users ~/.ssh/config would cause the config-parser to fail with
 > a "String index out of range: -1" exception. Also simplified
 > the line parsing code my using a two argument split.
 > 
 > Signed-off-by: Gilion Goudsmit <ggoudsmit@shebang.nl>

 A small test for this bug and FWIW:

 Tested-by: Jonas Fonseca <fonseca@diku.dk>

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 959b6b7..927c350 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
@@ -81,6 +81,28 @@ public void testNoConfig() {
 		assertNull(h.getIdentityFile());
 	}
 
+	public void testSeparatorParsing() throws Exception {
+		config("Host\tfirst\n" +
+		       "\tHostName\tfirst.tld\n" +
+		       "\n" +
+		       "Host second\n" +
+		       " HostName\tsecond.tld\n" +
+		       "Host=third\n" +
+		       "HostName=third.tld\n\n\n" +
+		       "\t Host = fourth\n\n\n" +
+		       " \t HostName\t=fourth.tld\n" +
+		       "Host\t =     last\n" +
+		       "HostName  \t    last.tld");
+		assertNotNull(osc.lookup("first"));
+		assertEquals("first.tld", osc.lookup("first").getHostName());
+		assertNotNull(osc.lookup("second"));
+		assertEquals("second.tld", osc.lookup("second").getHostName());
+		assertNotNull(osc.lookup("third"));
+		assertEquals("third.tld", osc.lookup("third").getHostName());
+		assertNotNull(osc.lookup("last"));
+		assertEquals("last.tld", osc.lookup("last").getHostName());
+	}
+
 	public void testAlias_DoesNotMatch() throws Exception {
 		config("Host orcz\n" + "\tHostName repo.or.cz\n");
 		final Host h = osc.lookup("repo.or.cz");
-- 
1.6.0.1.451.gc8d31


-- 
Jonas Fonseca

Re: [JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing

[Robin Rosenberg]

lördagen den 20 september 2008 22.33.41 skrev Gilion Goudsmit:
> Having only tab-characters separating a key and value in the
> users ~/.ssh/config would cause the config-parser to fail with
> a "String index out of range: -1" exception. Also simplified
> the line parsing code my using a two argument split.

Thank you and welcome to the club!

-- robin

Re: [JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing

[Jonas Fonseca]

Gilion Goudsmit <ggoudsmit@shebang.nl> wrote Sat, Sep 20, 2008:
> Having only tab-characters separating a key and value in the
> users ~/.ssh/config would cause the config-parser to fail with
> a "String index out of range: -1" exception. Also simplified
> the line parsing code my using a two argument split.
> 
> Signed-off-by: Gilion Goudsmit <ggoudsmit@shebang.nl>
> ---

With the following on top, the code will also pass the updated test for:

	Host	= somewhere

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 5bfcf5f..a9c6c12 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
@@ -167,7 +167,7 @@ public Host lookup(final String hostName) {
 			if (line.length() == 0 || line.startsWith("#"))
 				continue;
 
-			final String[] parts = line.split("[ \t=]", 2);
+			final String[] parts = line.split("[ \t]*[= \t]", 2);
 			final String keyword = parts[0].trim();
 			final String argValue = parts[1].trim();

-- 
Jonas Fonseca

[JGIT PATCH] Add test for OpenSshConfig separator parsing

[Jonas Fonseca]

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 .../spearce/jgit/transport/OpenSshConfigTest.java  |   24 ++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

 I sent the previous patch a bit too early. This one has the assertions.

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 959b6b7..8c1133d 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
@@ -81,6 +81,30 @@ public void testNoConfig() {
 		assertNull(h.getIdentityFile());
 	}
 
+	public void testSeparatorParsing() throws Exception {
+		config("Host\tfirst\n" +
+		       "\tHostName\tfirst.tld\n" +
+		       "\n" +
+		       "Host second\n" +
+		       " HostName\tsecond.tld\n" +
+		       "Host=third\n" +
+		       "HostName=third.tld\n\n\n" +
+		       "\t Host = fourth\n\n\n" +
+		       " \t HostName\t=fourth.tld\n" +
+		       "Host\t =     last\n" +
+		       "HostName  \t    last.tld");
+		assertNotNull(osc.lookup("first"));
+		assertEquals("first.tld", osc.lookup("first").getHostName());
+		assertNotNull(osc.lookup("second"));
+		assertEquals("second.tld", osc.lookup("second").getHostName());
+		assertNotNull(osc.lookup("third"));
+		assertEquals("third.tld", osc.lookup("third").getHostName());
+		assertNotNull(osc.lookup("fourth"));
+		assertEquals("fourth.tld", osc.lookup("fourth").getHostName());
+		assertNotNull(osc.lookup("last"));
+		assertEquals("last.tld", osc.lookup("last").getHostName());
+	}
+
 	public void testAlias_DoesNotMatch() throws Exception {
 		config("Host orcz\n" + "\tHostName repo.or.cz\n");
 		final Host h = osc.lookup("repo.or.cz");
-- 
1.6.0.2.444.gf2494

-- 
Jonas Fonseca

[JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Jonas Fonseca]

Removal of quoting had an off-by-one error, and was not handled for the
patterns used for the Host entry.

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

 Sverre Rabbelier <alturin@gmail.com> wrote Sun, Sep 21, 2008:
 > Heya,
 
 Allo, 
 
 > I'm not involved with JGit, so feel free to discared this mail :).
 
 Thanks, it made me dig a bit further, and find a bug.
 
 > On Sun, Sep 21, 2008 at 00:29, Jonas Fonseca <fonseca@diku.dk> wrote:
 > > -                       final String[] parts = line.split("[ \t=]", 2);
 > > +                       final String[] parts = line.split("[ \t]*[= \t]", 2);
 > 
 > Unless I'm guessing the purpose of this split wrong, wouldn't it be
 > even better to go for "[ \t]*=[ \t]+", or something like that (e.g.,
 > to allow for multiple tabs/spaces at the end as well).
 
 The code using the split result trims tabs and spaces at the end, so the
 main purpose of the regex is to find something that splits. This
 something can be tabs/spaces or _optionally_ one '='. 

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 8c1133d..ad6e79c 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
@@ -105,6 +105,32 @@ config("Host\tfirst\n" +
 		assertEquals("last.tld", osc.lookup("last").getHostName());
 	}
 
+	public void testQuoteParsing() throws Exception {
+		config("Host \"good\"\n" +
+			" HostName=\"good.tld\"\n" +
+			" Port=\"6007\"\n" +
+			" User=\"gooduser\"\n" +
+			"Host multiple unquoted and \"quoted\" \"hosts\"\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");
+		assertEquals("good.tld", osc.lookup("good").getHostName());
+		assertEquals("gooduser", osc.lookup("good").getUser());
+		assertEquals(6007, osc.lookup("good").getPort());
+		assertEquals(2222, osc.lookup("multiple").getPort());
+		assertEquals(2222, osc.lookup("quoted").getPort());
+		assertEquals(2222, osc.lookup("and").getPort());
+		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 testAlias_DoesNotMatch() throws Exception {
 		config("Host orcz\n" + "\tHostName repo.or.cz\n");
 		final Host h = osc.lookup("repo.or.cz");
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 a9c6c12..b08d5c6 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
@@ -173,7 +173,8 @@ public Host lookup(final String hostName) {
 
 			if ("Host".equalsIgnoreCase(keyword)) {
 				current.clear();
-				for (final String name : argValue.split("[ \t]")) {
+				for (final String pattern : argValue.split("[ \t]")) {
+					final String name = dequote(pattern);
 					Host c = m.get(name);
 					if (c == null) {
 						c = new Host();
@@ -243,7 +244,7 @@ private static boolean isHostMatch(final String pattern, final String name) {
 
 	private static String dequote(final String value) {
 		if (value.startsWith("\"") && value.endsWith("\""))
-			return value.substring(1, value.length() - 2);
+			return value.substring(1, value.length() - 1);
 		return value;
 	}
 
-- 
1.6.0.2.444.gf2494


-- 
Jonas Fonseca

Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Robin Rosenberg]

söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
> +		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
This one is really (as you noted) bad so we shouldn't allow it at all. A new 
subclass of TransportExcpeption should be thrown to indicate a serious
configuration problem when attempting to use the option.

-- robin

Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
> > +		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
> This one is really (as you noted) bad so we shouldn't allow it at all. A new 
> subclass of TransportExcpeption should be thrown to indicate a serious
> configuration problem when attempting to use the option.

Probably so.

But then we need to mark that the Host is invalid, because we
are serving requests from a cache, not from the file itself.
And TransportException isn't something that the SshSessionFactory
knows about.  Probably better to use a a subclass of IOException.

-- 
Shawn.

Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Jonas Fonseca]

Shawn O. Pearce <spearce@spearce.org> wrote Mon, Sep 22, 2008:
> Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
> > > +		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
> > This one is really (as you noted) bad so we shouldn't allow it at all. A new 
> > subclass of TransportExcpeption should be thrown to indicate a serious
> > configuration problem when attempting to use the option.
>
> Probably so.
> 
> But then we need to mark that the Host is invalid, because we
> are serving requests from a cache, not from the file itself.
> And TransportException isn't something that the SshSessionFactory
> knows about.  Probably better to use a a subclass of IOException.

Sorry, for not following up on this. I was trying to cook up a patch for
this today. Now, it is somehow sad that testing "forces" us to waste
time on these stupid corner cases. ;-) On the other hand, this problem
might exist (.git/config) or turn up again, so it would be good to have
a design principle.

Using exceptions seems a bit harsh, since the quote is not really fatal
in anyway. Also, for badly formatted Port values the value is simply
ignored. So for bad quoting encountered during non-Host values, I think
it is fair to just ignore the value. For Host values it is a bit more
non-obvious to me. In terms of invalidating hosts, the API ensures that
a lookup will always return a host, so invalid hosts should not return
null. 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.

-- 
Jonas Fonseca

Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Shawn O. Pearce]

Jonas Fonseca <fonseca@diku.dk> wrote:
> Shawn O. Pearce <spearce@spearce.org> wrote Mon, Sep 22, 2008:
> > Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > > söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
> > > > +		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
...
> > > This one is really (as you noted) bad so we shouldn't allow it at all.
> 
> Using exceptions seems a bit harsh, since the quote is not really fatal
> in anyway.
...
> 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.

-- 
Shawn.

Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

[Robin Rosenberg]

torsdagen den 25 september 2008 01.25.19 skrev Jonas Fonseca:
> Using exceptions seems a bit harsh, since the quote is not really fatal
> in anyway. Also, for badly formatted Port values the value is simply ignored.

For this case OpenSSH complains about a missing parameter. It does not
even attempt to look up the host. For malformed port numbers I get "Bad number"
(and no attempt to connect. I think an exception is the right way to solve the
problem si nce the caller doesn't have to do the error checking. and the error
is more obvious to the user. OpenSSH also gives the line number. Maybe we
should too. 

The OpenSSH i'm referencing is openssh-5.1p1-1mdv2009.0

-- robin

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

[Jonas Fonseca]

Badly quoted entries are now ignored similar to how bad port number are
currently ignored. A check for negative port numbers is now performed
so that they also will be ignored.

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

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

[Robin Rosenberg]

Den 9/25/2008, skrev "Jonas Fonseca" <fonseca@diku.dk>:

>Badly quoted entries are now ignored similar to how bad port number are
>currently ignored. A check for negative port numbers is now performed
>so that they also will be ignored.

Nooo. We should really give some feedback on bad entries. OpenSSH
(version 5.1) complains and refuses to connect if I give it a bad port
number or hostname. It complains about "missing parameter" and bad
port number for these cases. OpenSSH doesn't simply ignore them.

HostName=bad"
Port=hubba

Even if openssh would ignore them I'd think it would be bad if we did,
unless there was some serious compatibility issue here.

-- robin

[JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig

[Jonas Fonseca]

Badly quoted entries are now ignored similar to how bad port number are
currently ignored. A check for negative port numbers is now performed
so that they also will be ignored.

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>

---
 .../spearce/jgit/transport/OpenSshConfigTest.java  |   46 +++++++++++++++---
 .../jgit/transport/DefaultSshSessionFactory.java   |   10 +++-
 .../org/spearce/jgit/transport/OpenSshConfig.java  |   51 ++++++++++++-------
 3 files changed, 79 insertions(+), 28 deletions(-)

 Robin Rosenberg <robin.rosenberg@dewire.com> wrote Thu, Sep 25, 2008:
 > Den 9/25/2008, skrev "Jonas Fonseca" <fonseca@diku.dk>:
 > 
 > >Badly quoted entries are now ignored similar to how bad port number are
 > >currently ignored. A check for negative port numbers is now performed
 > >so that they also will be ignored.
 > 
 > Nooo. We should really give some feedback on bad entries. OpenSSH
 > (version 5.1) complains and refuses to connect if I give it a bad port
 > number or hostname. It complains about "missing parameter" and bad
 > port number for these cases. OpenSSH doesn't simply ignore them.
 > 
 > HostName=bad"
 > Port=hubba
 > 
 > Even if openssh would ignore them I'd think it would be bad if we did,
 > unless there was some serious compatibility issue here.

 Maybe something like this? It adds OpenSshConfigException which will
 "contain" either a NumberFormatException or a ParseException.

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..6b7bb4b 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
@@ -42,6 +42,7 @@
 import java.io.IOException;
 import java.io.OutputStreamWriter;
 
+import org.spearce.jgit.errors.OpenSshConfigException;
 import org.spearce.jgit.lib.RepositoryTestCase;
 import org.spearce.jgit.transport.OpenSshConfig.Host;
 
@@ -72,7 +73,18 @@ private void config(final String data) throws IOException {
 		fw.close();
 	}
 
-	public void testNoConfig() {
+	private boolean parseError(String config) throws Exception {
+		config(config);
+		OpenSshConfigException osce = null;
+		try {
+			osc.lookup("something");
+		} catch (OpenSshConfigException cause) {
+			return true;
+		}
+		return false;
+	}
+
+	public void testNoConfig() throws Exception {
 		final Host h = osc.lookup("repo.or.cz");
 		assertNotNull(h);
 		assertEquals("repo.or.cz", h.getHostName());
@@ -114,11 +126,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 +136,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 testNegativePortNumber() throws Exception {
+		assertTrue(parseError("Host negativeportnumber\n" +
+			   "Port -200\n"));
+	}
+
+	public void testBadPortNumber() throws Exception {
+		assertTrue(parseError("Host badportnumber\n" +
+		       "Port twentytwo\n"));
+	}
+
+	public void testBadlyQuotedPortNumber() throws Exception {
+		assertTrue(parseError("Host badportquote\n" +
+		       "Port \"2222\n"));
+	}
+
+	public void testBadlyQuotedPortNumber2() throws Exception {
+		assertTrue(parseError("Host badportquote2\n" +
+		       "Port 2222\"\n"));
+	}
+
+	public void testBadlyQuotedHost() throws Exception {
+		assertTrue(parseError("Host badly \"quoted\n" +
+		       "HostName=unknown\n"));
 	}
 
 	public void testAlias_DoesNotMatch() throws Exception {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
index 89beab7..123a9b5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
@@ -65,6 +65,7 @@
 import com.jcraft.jsch.Session;
 import com.jcraft.jsch.UIKeyboardInteractive;
 import com.jcraft.jsch.UserInfo;
+import org.spearce.jgit.errors.OpenSshConfigException;
 
 /**
  * Loads known hosts and private keys from <code>$HOME/.ssh</code>.
@@ -89,7 +90,12 @@
 	@Override
 	public synchronized Session getSession(String user, String pass,
 			String host, int port) throws JSchException {
-		final OpenSshConfig.Host hc = getConfig().lookup(host);
+		final OpenSshConfig.Host hc;
+		try {
+			hc = getConfig().lookup(host);
+		} catch (OpenSshConfigException osce) {
+			throw new JSchException(osce.getMessage());
+		}
 		host = hc.getHostName();
 		if (port <= 0)
 			port = hc.getPort();
@@ -128,7 +134,7 @@ private JSch getUserJSch() throws JSchException {
 		return userJSch;
 	}
 
-	private OpenSshConfig getConfig() {
+	private OpenSshConfig getConfig() throws OpenSshConfigException {
 		if (config == null)
 			config = OpenSshConfig.get();
 		return config;
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..95a37f5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
@@ -44,13 +44,16 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.text.ParseException;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
 import org.spearce.jgit.errors.InvalidPatternException;
+import org.spearce.jgit.errors.OpenSshConfigException;
 import org.spearce.jgit.fnmatch.FileNameMatcher;
 import org.spearce.jgit.util.FS;
 
@@ -72,7 +75,7 @@
 	 *
 	 * @return a caching reader of the user's configuration file.
 	 */
-	public static OpenSshConfig get() {
+	public static OpenSshConfig get() throws OpenSshConfigException {
 		File home = FS.userHome();
 		if (home == null)
 			home = new File(".").getAbsoluteFile();
@@ -110,7 +113,7 @@ protected OpenSshConfig(final File h, final File cfg) {
 	 *            configuration file.
 	 * @return r configuration for the requested name. Never null.
 	 */
-	public Host lookup(final String hostName) {
+	public Host lookup(final String hostName) throws OpenSshConfigException {
 		final Map<String, Host> cache = refresh();
 		Host h = cache.get(hostName);
 		if (h == null)
@@ -136,7 +139,7 @@ public Host lookup(final String hostName) {
 		return h;
 	}
 
-	private synchronized Map<String, Host> refresh() {
+	private synchronized Map<String, Host> refresh() throws OpenSshConfigException {
 		final long mtime = configFile.lastModified();
 		if (mtime != lastModified) {
 			try {
@@ -146,6 +149,10 @@ public Host lookup(final String hostName) {
 				} finally {
 					in.close();
 				}
+			} catch (NumberFormatException nfe) {
+				throw new OpenSshConfigException("Parse error", nfe);
+			} catch (ParseException pe) {
+				throw new OpenSshConfigException("Parse error", pe);
 			} catch (FileNotFoundException none) {
 				hosts = Collections.emptyMap();
 			} catch (IOException err) {
@@ -156,7 +163,7 @@ public Host lookup(final String hostName) {
 		return hosts;
 	}
 
-	private Map<String, Host> parse(final InputStream in) throws IOException {
+	private Map<String, Host> parse(final InputStream in) throws IOException, ParseException {
 		final Map<String, Host> m = new LinkedHashMap<String, Host>();
 		final BufferedReader br = new BufferedReader(new InputStreamReader(in));
 		final List<Host> current = new ArrayList<Host>(4);
@@ -192,35 +199,37 @@ 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));
-					for (final Host c : current)
-						if (c.port == 0)
-							c.port = port;
-				} catch (NumberFormatException nfe) {
-					// Bad port number. Don't set it.
-				}
+				final int port = Integer.parseInt(value);
+				if (port < 0)
+					throw new NumberFormatException("Negative port number");
+				for (final Host c : current)
+					if (c.port == 0)
+						c.port = port;
 			} 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);
 			}
 		}
 
@@ -242,9 +251,13 @@ private static boolean isHostMatch(final String pattern, final String name) {
 		return fn.isMatch();
 	}
 
-	private static String dequote(final String value) {
-		if (value.startsWith("\"") && value.endsWith("\""))
+	private static String dequote(final String value) throws ParseException {
+		final boolean hasStart = value.startsWith("\"");
+		final boolean hasEnd = value.endsWith("\"");
+		if (hasStart && hasEnd)
 			return value.substring(1, value.length() - 1);
+		if (hasStart || hasEnd)
+			throw new ParseException("Quote mismatch", value.indexOf("\""));
 		return value;
 	}
 
-- 
tg: (cf67dfc..) jf/opensshconfigquote (depends on: master)

-- 
Jonas Fonseca

Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig

[Jonas Fonseca]

On Thu, Sep 25, 2008 at 15:29, Jonas Fonseca <fonseca@diku.dk> wrote:
> Badly quoted entries are now ignored similar to how bad port number are
> currently ignored. A check for negative port numbers is now performed
> so that they also will be ignored.

Sorry for spamming this list today. The commit message should of
course read something like:

Throw an OpenSshConfigException when parsing meet badly quoted entries
and port numbers, which do not contain numbers or are negative.

-- 
Jonas Fonseca

Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig

[Shawn O. Pearce]

Jonas Fonseca <fonseca@diku.dk> wrote:
> Badly quoted entries are now ignored similar to how bad port number are
> currently ignored. A check for negative port numbers is now performed
> so that they also will be ignored.
> 
> Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
> 
> ---
>  .../spearce/jgit/transport/OpenSshConfigTest.java  |   46 +++++++++++++++---
>  .../jgit/transport/DefaultSshSessionFactory.java   |   10 +++-
>  .../org/spearce/jgit/transport/OpenSshConfig.java  |   51 ++++++++++++-------
>  3 files changed, 79 insertions(+), 28 deletions(-)

Well, at first glance the new OpenSshConfigException is missing
from the patch.  We need that class to compile correctly.  ;-)
 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
> index 89beab7..123a9b5 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
> @@ -89,7 +90,12 @@
>  	@Override
>  	public synchronized Session getSession(String user, String pass,
>  			String host, int port) throws JSchException {
> -		final OpenSshConfig.Host hc = getConfig().lookup(host);
> +		final OpenSshConfig.Host hc;
> +		try {
> +			hc = getConfig().lookup(host);
> +		} catch (OpenSshConfigException osce) {
> +			throw new JSchException(osce.getMessage());
> +		}

I would perfer to chain the OpenSshConfigException as the cause of
the JSchException.  That way the caller has a chance to give us a
complete stack trace, including the cause of the OSCE being the
inner NumberFormatException or ParseException.

Robin or I will need to edit the EclipseSshSessionFactory to add
this same sort of try/catch.  To keep the tree buildable we'll
want to squash that into your patch.  Yea, sorry, this is where
the egit+jgit within one repository is going to bite us.

> 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..95a37f5 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
> @@ -146,6 +149,10 @@ public Host lookup(final String hostName) {
>  				} finally {
>  					in.close();
>  				}
> +			} catch (NumberFormatException nfe) {
> +				throw new OpenSshConfigException("Parse error", nfe);
> +			} catch (ParseException pe) {
> +				throw new OpenSshConfigException("Parse error", pe);
>  			} catch (FileNotFoundException none) {
>  				hosts = Collections.emptyMap();
>  			} catch (IOException err) {

If we are really going to this level of effort, can we please have
the file path and line number of the invalid line in the exception?

We rarely parse the ~/.ssh/config.  In general its parsed only once
during startup.  Going through a bit more work at parse time to get
more accurate error messages is acceptable.

I think it may be a good idea to have the exception thrown only
when a bad Host block is accessed, or if we try to access an unknown
host and there is an unreadable Host block.

So I'm thinking more like we stuff an OSCE instance into the Host
block if the host has a bad entry, and during get() or lookup()
we test for the exception and rethrow the exception.

E.g. lets say my config file is this:

	$ cat ~/.ssh/config
	Host work
		Hostname internal.google.com"
		Port -1

	Host orcz
		Hostname repo.or.cz
		User spearce
		Port 22

then:

  jgit fetch orcz:foo.git;        # works without error
  jgit fetch work:foo.git;        # throws OSCE due to bad Hostname
  jgit fetch kernel.org:foo.git;  # works as no Host matched

However if my config was more bogus, e.g.:

	$ cat ~/.ssh/config
	Host work"
		User sop

	Host orcz
		Hostname repo.or.cz
		User spearce
		Port 22

then:

  jgit fetch orcz:foo.git;        # still works without error
  jgit fetch work:foo.git;        # fails due to bad Host header
  jgit fetch kernel.org:foo.git;  # fails due to bad Host header

-- 
Shawn.