Fixed a bug that caused tabs in ~/.ssh/config to break parsing
PATCH 1/1 Fixed a bug that caused tabs in ~/.ssh/config to break parsing
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.
--- .../org/spearce/jgit/transport/OpenSshConfig.java | 14 +++----------- 1 files changed, 3 insertions(+), 11 deletions(-)
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +3 -11
@@ -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();
Signature · 1 line
1.5.3.8
PATCH 1/1 Add test for OpenSshConfig separator parsing
--- .../spearce/jgit/transport/OpenSshConfigTest.java | 22 ++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
Gilion Goudsmit <[email protected]> wrote Sat, Sep 20, 2008:
Quoted 6 lines
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 <[email protected]>
A small test for this bug and FWIW:
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +22 -0
@@ -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");
Signature · 5 lines
1.6.0.1.451.gc8d31 -- Jonas Fonseca
PATCH 1/1 Add test for OpenSshConfig separator parsing
--- .../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.
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +24 -0
@@ -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");
Signature · 4 lines
1.6.0.2.444.gf2494 -- Jonas Fonseca
Gilion Goudsmit <[email protected]> wrote Sat, Sep 20, 2008:
Quoted 7 lines
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 <[email protected]>---
With the following on top, the code will also pass the updated test for:
Host = somewhere
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +1 -1
@@ -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();
Signature · 1 line
Jonas Fonseca
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 Test and fix handling of quotes in ~/.ssh/config
Removal of quoting had an off-by-one error, and was not handled for the patterns used for the Host entry.
--- .../spearce/jgit/transport/OpenSshConfigTest.java | 26 ++++++++++++++++++++ .../org/spearce/jgit/transport/OpenSshConfig.java | 5 ++- 2 files changed, 29 insertions(+), 2 deletions(-)
Sverre Rabbelier <[email protected]> wrote Sun, Sep 21, 2008:
Quoted 1 line
Heya,
Allo,
Quoted 1 line
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.
Quoted 7 lines · Jonas Fonseca
Review
Quoted 2 lines
(quoted diff) +1
-1- 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 '='.
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +26 -0
@@ -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");
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +3 -2
@@ -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; }
Signature · 5 lines
1.6.0.2.444.gf2494 -- Jonas Fonseca
Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config
söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java config("Host\tfirst\n" + · line 131
Quoted 1 line
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +1
-0+ 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
Quoted 5 lines · Robin Rosenberg
söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java config("Host\tfirst\n" + · line 131
Quoted 1 line
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +1
-0+ 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.
Signature · 1 line
Shawn.
Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config
Shawn O. Pearce <[email protected]> wrote Mon, Sep 22, 2008:
Quoted 7 lines
Quoted 5 lines · Robin Rosenberg
söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java config("Host\tfirst\n" + · line 131
Quoted 1 line
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +1
-0+ 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.
Quoted 6 lines
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.
Signature · 1 line
Jonas Fonseca
Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config
Quoted 4 lines · Jonas Fonseca
Shawn O. Pearce <[email protected]> wrote Mon, Sep 22, 2008:
Quoted 3 lines
Quoted 2 lines · Robin Rosenberg
söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
Quoted 1 line
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +1
-0+ assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
...
Quoted 4 lines
Quoted 1 line
Quoted 1 line
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.
...
Quoted 3 lines
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.
Signature · 1 line
Shawn.
PATCH 1/1 Add tests for handling of parsing errors in OpenSshConfig
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.
--- .../spearce/jgit/transport/OpenSshConfigTest.java | 32 +++++++++++++++--- .../org/spearce/jgit/transport/OpenSshConfig.java | 34 +++++++++++++++---- 2 files changed, 53 insertions(+), 13 deletions(-)
Shawn O. Pearce <[email protected]> wrote Wed, Sep 24, 2008:
Quoted 5 lines
Quoted 3 lines · Jonas Fonseca
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.
Quoted 7 lines
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.
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +26 -6
@@ -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 {
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +27 -7
@@ -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; }
Signature · 4 lines
tg: (cf67dfc..) jf/opensshconfigquote (depends on: master) -- Jonas Fonseca
Re: [JGIT PATCH] Add tests for handling of parsing errors in OpenSshConfig
Den 9/25/2008, skrev "Jonas Fonseca" <[email protected]>:
Quoted 3 lines
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
RFC PATCH 1/1 Improve handling of parsing errors in OpenSshConfig
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.
--- .../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 <[email protected]> wrote Thu, Sep 25, 2008:
Quoted 11 lines
Den 9/25/2008, skrev "Jonas Fonseca" <[email protected]>:
Quoted 3 lines
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.
Quoted 5 lines
HostName=bad"
Port=hubbaEven 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.
org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java +39 -7
@@ -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 {
org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java +8 -2
@@ -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;
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +32 -19
123 lines not on this page · raw message
Signature · 4 lines
tg: (cf67dfc..) jf/opensshconfigquote (depends on: master) -- Jonas Fonseca
Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig
Quoted 11 lines · 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 <[email protected]>--- .../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. ;-)
Quoted 5 lines
org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java +0
-0
org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java line 98
Quoted 10 lines
org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java +6
-1@@ -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.
Quoted 5 lines
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +0
-0
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java public Host lookup(final String hostName) { · line 158
Quoted 10 lines
org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java +4
-0@@ -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
Signature · 1 line
Shawn.
Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig
In reply to Jonas FonsecaQuoted 3 lines · 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.
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.
Signature · 1 line
Jonas Fonseca
Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config
In reply to Jonas Fonsecatorsdagen den 25 september 2008 01.25.19 skrev Jonas Fonseca:
Quoted 2 lines
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
lördagen den 20 september 2008 22.33.41 skrev Gilion Goudsmit:
Quoted 4 lines
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