{"thread":{"id":"15606","subject":"[JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing","startedAt":"2008-09-20T20:33:41Z","lastAt":"2008-09-25T15:16:32Z","messageCount":16,"participants":["Gilion Goudsmit","Jonas Fonseca","Robin Rosenberg","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"91195","messageId":"12219428213749-git-send-email-ggoudsmit@shebang.nl","threadId":"15606","inReplyTo":null,"subject":"[JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing","fromName":"Gilion Goudsmit","fromEmail":"ggoudsmit@shebang.nl","sentAt":"2008-09-20T20:33:41Z","receivedAt":"2008-09-20T20:33:41Z","isPatch":true,"sender":{"key":"ggoudsmit@shebang.nl","avatar":null},"body":"Having only tab-characters separating a key and value in the\nusers ~/.ssh/config would cause the config-parser to fail with\na \"String index out of range: -1\" exception. Also simplified\nthe line parsing code my using a two argument split.\n\nSigned-off-by: Gilion Goudsmit \n---\n .../org/spearce/jgit/transport/OpenSshConfig.java | 14 +++-----------\n 1 files changed, 3 insertions(+), 11 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\nindex df38e18..5bfcf5f 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n@@ -167,17 +167,9 @@ while ((line = br.readLine()) != null) {\n \t\t\tif (line.length() == 0 || line.startsWith(\"#\"))\n \t\t\t\tcontinue;\n \n-\t\t\tfinal int sp = line.indexOf(' ');\n-\t\t\tfinal int eq = line.indexOf('=');\n-\t\t\tfinal int splitAt;\n-\t\t\tif (sp >= 0 && eq >= 0)\n-\t\t\t\tsplitAt = Math.min(sp, eq);\n-\t\t\telse if (sp < 0)\n-\t\t\t\tsplitAt = eq;\n-\t\t\telse\n-\t\t\t\tsplitAt = sp;\n-\t\t\tfinal String keyword = line.substring(0, splitAt).trim();\n-\t\t\tfinal String argValue = line.substring(splitAt + 1).trim();\n+\t\t\tfinal String[] parts = line.split(\"[ \\t=]\", 2);\n+\t\t\tfinal String keyword = parts[0].trim();\n+\t\t\tfinal String argValue = parts[1].trim();\n \n \t\t\tif (\"Host\".equalsIgnoreCase(keyword)) {\n \t\t\t\tcurrent.clear();\n-- \n1.5.3.8\n"},{"id":"91198","messageId":"20080920214808.GA7426@diku.dk","threadId":"15606","inReplyTo":"12219428213749-git-send-email-ggoudsmit@shebang.nl","subject":"[JGIT PATCH] Add test for OpenSshConfig separator parsing","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-20T21:48:08Z","receivedAt":"2008-09-20T21:48:08Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Signed-off-by: Jonas Fonseca \n---\n .../spearce/jgit/transport/OpenSshConfigTest.java | 22 ++++++++++++++++++++\n 1 files changed, 22 insertions(+), 0 deletions(-)\n\n Gilion Goudsmit wrote Sat, Sep 20, 2008:\n > Having only tab-characters separating a key and value in the\n > users ~/.ssh/config would cause the config-parser to fail with\n > a \"String index out of range: -1\" exception. Also simplified\n > the line parsing code my using a two argument split.\n > \n > Signed-off-by: Gilion Goudsmit \n\n A small test for this bug and FWIW:\n\n Tested-by: Jonas Fonseca \n\ndiff --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\nindex 959b6b7..927c350 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n@@ -81,6 +81,28 @@ public void testNoConfig() {\n \t\tassertNull(h.getIdentityFile());\n \t}\n \n+\tpublic void testSeparatorParsing() throws Exception {\n+\t\tconfig(\"Host\\tfirst\\n\" +\n+\t\t \"\\tHostName\\tfirst.tld\\n\" +\n+\t\t \"\\n\" +\n+\t\t \"Host second\\n\" +\n+\t\t \" HostName\\tsecond.tld\\n\" +\n+\t\t \"Host=third\\n\" +\n+\t\t \"HostName=third.tld\\n\\n\\n\" +\n+\t\t \"\\t Host = fourth\\n\\n\\n\" +\n+\t\t \" \\t HostName\\t=fourth.tld\\n\" +\n+\t\t \"Host\\t = last\\n\" +\n+\t\t \"HostName \\t last.tld\");\n+\t\tassertNotNull(osc.lookup(\"first\"));\n+\t\tassertEquals(\"first.tld\", osc.lookup(\"first\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"second\"));\n+\t\tassertEquals(\"second.tld\", osc.lookup(\"second\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"third\"));\n+\t\tassertEquals(\"third.tld\", osc.lookup(\"third\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"last\"));\n+\t\tassertEquals(\"last.tld\", osc.lookup(\"last\").getHostName());\n+\t}\n+\n \tpublic void testAlias_DoesNotMatch() throws Exception {\n \t\tconfig(\"Host orcz\\n\" + \"\\tHostName repo.or.cz\\n\");\n \t\tfinal Host h = osc.lookup(\"repo.or.cz\");\n-- \n1.6.0.1.451.gc8d31\n\n\n-- \nJonas Fonseca\n"},{"id":"91201","messageId":"200809210018.29724.robin.rosenberg.lists@dewire.com","threadId":"15606","inReplyTo":"12219428213749-git-send-email-ggoudsmit@shebang.nl","subject":"Re: [JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-09-20T22:18:29Z","receivedAt":"2008-09-20T22:18:29Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördagen den 20 september 2008 22.33.41 skrev Gilion Goudsmit:\n> Having only tab-characters separating a key and value in the\n> users ~/.ssh/config would cause the config-parser to fail with\n> a \"String index out of range: -1\" exception. Also simplified\n> the line parsing code my using a two argument split.\n\nThank you and welcome to the club!\n\n-- robin\n"},{"id":"91206","messageId":"20080920222958.GA18981@diku.dk","threadId":"15606","inReplyTo":"12219428213749-git-send-email-ggoudsmit@shebang.nl","subject":"Re: [JGIT PATCH] Fixed a bug that caused tabs in ~/.ssh/config to break parsing","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-20T22:29:58Z","receivedAt":"2008-09-20T22:29:58Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Gilion Goudsmit wrote Sat, Sep 20, 2008:\n> Having only tab-characters separating a key and value in the\n> users ~/.ssh/config would cause the config-parser to fail with\n> a \"String index out of range: -1\" exception. Also simplified\n> the line parsing code my using a two argument split.\n> \n> Signed-off-by: Gilion Goudsmit \n> ---\n\nWith the following on top, the code will also pass the updated test for:\n\n\tHost\t= somewhere\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\nindex 5bfcf5f..a9c6c12 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n@@ -167,7 +167,7 @@ public Host lookup(final String hostName) {\n \t\t\tif (line.length() == 0 || line.startsWith(\"#\"))\n \t\t\t\tcontinue;\n \n-\t\t\tfinal String[] parts = line.split(\"[ \\t=]\", 2);\n+\t\t\tfinal String[] parts = line.split(\"[ \\t]*[= \\t]\", 2);\n \t\t\tfinal String keyword = parts[0].trim();\n \t\t\tfinal String argValue = parts[1].trim();\n\n-- \nJonas Fonseca\n"},{"id":"91207","messageId":"20080920223340.GB18981@diku.dk","threadId":"15606","inReplyTo":"20080920214808.GA7426@diku.dk","subject":"[JGIT PATCH] Add test for OpenSshConfig separator parsing","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-20T22:33:40Z","receivedAt":"2008-09-20T22:33:40Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Signed-off-by: Jonas Fonseca \n---\n .../spearce/jgit/transport/OpenSshConfigTest.java | 24 ++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\n I sent the previous patch a bit too early. This one has the assertions.\n\ndiff --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\nindex 959b6b7..8c1133d 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n@@ -81,6 +81,30 @@ public void testNoConfig() {\n \t\tassertNull(h.getIdentityFile());\n \t}\n \n+\tpublic void testSeparatorParsing() throws Exception {\n+\t\tconfig(\"Host\\tfirst\\n\" +\n+\t\t \"\\tHostName\\tfirst.tld\\n\" +\n+\t\t \"\\n\" +\n+\t\t \"Host second\\n\" +\n+\t\t \" HostName\\tsecond.tld\\n\" +\n+\t\t \"Host=third\\n\" +\n+\t\t \"HostName=third.tld\\n\\n\\n\" +\n+\t\t \"\\t Host = fourth\\n\\n\\n\" +\n+\t\t \" \\t HostName\\t=fourth.tld\\n\" +\n+\t\t \"Host\\t = last\\n\" +\n+\t\t \"HostName \\t last.tld\");\n+\t\tassertNotNull(osc.lookup(\"first\"));\n+\t\tassertEquals(\"first.tld\", osc.lookup(\"first\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"second\"));\n+\t\tassertEquals(\"second.tld\", osc.lookup(\"second\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"third\"));\n+\t\tassertEquals(\"third.tld\", osc.lookup(\"third\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"fourth\"));\n+\t\tassertEquals(\"fourth.tld\", osc.lookup(\"fourth\").getHostName());\n+\t\tassertNotNull(osc.lookup(\"last\"));\n+\t\tassertEquals(\"last.tld\", osc.lookup(\"last\").getHostName());\n+\t}\n+\n \tpublic void testAlias_DoesNotMatch() throws Exception {\n \t\tconfig(\"Host orcz\\n\" + \"\\tHostName repo.or.cz\\n\");\n \t\tfinal Host h = osc.lookup(\"repo.or.cz\");\n-- \n1.6.0.2.444.gf2494\n\n-- \nJonas Fonseca\n"},{"id":"91221","messageId":"20080921112519.GA24200@diku.dk","threadId":"15606","inReplyTo":"bd6139dc0809201819o5d6eb5b1r7bf0e46702c711d7@mail.gmail.com","subject":"[JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-21T11:25:19Z","receivedAt":"2008-09-21T11:25:19Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Removal of quoting had an off-by-one error, and was not handled for the\npatterns used for the Host entry.\n\nSigned-off-by: Jonas Fonseca \n---\n .../spearce/jgit/transport/OpenSshConfigTest.java | 26 ++++++++++++++++++++\n .../org/spearce/jgit/transport/OpenSshConfig.java | 5 ++-\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\n Sverre Rabbelier wrote Sun, Sep 21, 2008:\n > Heya,\n \n Allo, \n \n > I'm not involved with JGit, so feel free to discared this mail :).\n \n Thanks, it made me dig a bit further, and find a bug.\n \n > On Sun, Sep 21, 2008 at 00:29, Jonas Fonseca wrote:\n > > - final String[] parts = line.split(\"[ \\t=]\", 2);\n > > + final String[] parts = line.split(\"[ \\t]*[= \\t]\", 2);\n > \n > Unless I'm guessing the purpose of this split wrong, wouldn't it be\n > even better to go for \"[ \\t]*=[ \\t]+\", or something like that (e.g.,\n > to allow for multiple tabs/spaces at the end as well).\n \n The code using the split result trims tabs and spaces at the end, so the\n main purpose of the regex is to find something that splits. This\n something can be tabs/spaces or _optionally_ one '='. \n\ndiff --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\nindex 8c1133d..ad6e79c 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n@@ -105,6 +105,32 @@ config(\"Host\\tfirst\\n\" +\n \t\tassertEquals(\"last.tld\", osc.lookup(\"last\").getHostName());\n \t}\n \n+\tpublic void testQuoteParsing() throws Exception {\n+\t\tconfig(\"Host \\\"good\\\"\\n\" +\n+\t\t\t\" HostName=\\\"good.tld\\\"\\n\" +\n+\t\t\t\" Port=\\\"6007\\\"\\n\" +\n+\t\t\t\" User=\\\"gooduser\\\"\\n\" +\n+\t\t\t\"Host multiple unquoted and \\\"quoted\\\" \\\"hosts\\\"\\n\" +\n+\t\t\t\" Port=\\\"2222\\\"\\n\" +\n+\t\t\t\"Host \\\"spaced\\\"\\n\" +\n+\t\t\t\"# Bad host name, but testing preservation of spaces\\n\" +\n+\t\t\t\" HostName=\\\" spaced\\ttld \\\"\\n\" +\n+\t\t\t\"# Misbalanced quotes\\n\" +\n+\t\t\t\"Host \\\"bad\\\"\\n\" +\n+\t\t\t\"# OpenSSH doesn't allow this but ...\\n\" +\n+\t\t\t\" HostName=bad.tld\\\"\\n\");\n+\t\tassertEquals(\"good.tld\", osc.lookup(\"good\").getHostName());\n+\t\tassertEquals(\"gooduser\", osc.lookup(\"good\").getUser());\n+\t\tassertEquals(6007, osc.lookup(\"good\").getPort());\n+\t\tassertEquals(2222, osc.lookup(\"multiple\").getPort());\n+\t\tassertEquals(2222, osc.lookup(\"quoted\").getPort());\n+\t\tassertEquals(2222, osc.lookup(\"and\").getPort());\n+\t\tassertEquals(2222, osc.lookup(\"unquoted\").getPort());\n+\t\tassertEquals(2222, osc.lookup(\"hosts\").getPort());\n+\t\tassertEquals(\" spaced\\ttld \", osc.lookup(\"spaced\").getHostName());\n+\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n+\t}\n+\n \tpublic void testAlias_DoesNotMatch() throws Exception {\n \t\tconfig(\"Host orcz\\n\" + \"\\tHostName repo.or.cz\\n\");\n \t\tfinal Host h = osc.lookup(\"repo.or.cz\");\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\nindex a9c6c12..b08d5c6 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n@@ -173,7 +173,8 @@ public Host lookup(final String hostName) {\n \n \t\t\tif (\"Host\".equalsIgnoreCase(keyword)) {\n \t\t\t\tcurrent.clear();\n-\t\t\t\tfor (final String name : argValue.split(\"[ \\t]\")) {\n+\t\t\t\tfor (final String pattern : argValue.split(\"[ \\t]\")) {\n+\t\t\t\t\tfinal String name = dequote(pattern);\n \t\t\t\t\tHost c = m.get(name);\n \t\t\t\t\tif (c == null) {\n \t\t\t\t\t\tc = new Host();\n@@ -243,7 +244,7 @@ private static boolean isHostMatch(final String pattern, final String name) {\n \n \tprivate static String dequote(final String value) {\n \t\tif (value.startsWith(\"\\\"\") && value.endsWith(\"\\\"\"))\n-\t\t\treturn value.substring(1, value.length() - 2);\n+\t\t\treturn value.substring(1, value.length() - 1);\n \t\treturn value;\n \t}\n \n-- \n1.6.0.2.444.gf2494\n\n\n-- \nJonas Fonseca\n"},{"id":"91341","messageId":"200809222242.29165.robin.rosenberg.lists@dewire.com","threadId":"15606","inReplyTo":"20080921112519.GA24200@diku.dk","subject":"Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-09-22T20:42:29Z","receivedAt":"2008-09-22T20:42:29Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:\n> +\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\nThis one is really (as you noted) bad so we shouldn't allow it at all. A new \nsubclass of TransportExcpeption should be thrown to indicate a serious\nconfiguration problem when attempting to use the option.\n\n-- robin\n"},{"id":"91342","messageId":"20080922210734.GE3669@spearce.org","threadId":"15606","inReplyTo":"200809222242.29165.robin.rosenberg.lists@dewire.com","subject":"Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-22T21:07:34Z","receivedAt":"2008-09-22T21:07:34Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg wrote:\n> söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:\n> > +\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n> This one is really (as you noted) bad so we shouldn't allow it at all. A new \n> subclass of TransportExcpeption should be thrown to indicate a serious\n> configuration problem when attempting to use the option.\n\nProbably so.\n\nBut then we need to mark that the Host is invalid, because we\nare serving requests from a cache, not from the file itself.\nAnd TransportException isn't something that the SshSessionFactory\nknows about. Probably better to use a a subclass of IOException.\n\n-- \nShawn.\n"},{"id":"91542","messageId":"20080924232519.GA15318@diku.dk","threadId":"15606","inReplyTo":"20080922210734.GE3669@spearce.org","subject":"Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-24T23:25:19Z","receivedAt":"2008-09-24T23:25:19Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Shawn O. Pearce wrote Mon, Sep 22, 2008:\n> Robin Rosenberg wrote:\n> > söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:\n> > > +\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n> > This one is really (as you noted) bad so we shouldn't allow it at all. A new \n> > subclass of TransportExcpeption should be thrown to indicate a serious\n> > configuration problem when attempting to use the option.\n>\n> Probably so.\n> \n> But then we need to mark that the Host is invalid, because we\n> are serving requests from a cache, not from the file itself.\n> And TransportException isn't something that the SshSessionFactory\n> knows about. Probably better to use a a subclass of IOException.\n\nSorry, for not following up on this. I was trying to cook up a patch for\nthis today. Now, it is somehow sad that testing \"forces\" us to waste\ntime on these stupid corner cases. ;-) On the other hand, this problem\nmight exist (.git/config) or turn up again, so it would be good to have\na design principle.\n\nUsing exceptions seems a bit harsh, since the quote is not really fatal\nin anyway. Also, for badly formatted Port values the value is simply\nignored. So for bad quoting encountered during non-Host values, I think\nit is fair to just ignore the value. For Host values it is a bit more\nnon-obvious to me. In terms of invalidating hosts, the API ensures that\na lookup will always return a host, so invalid hosts should not return\nnull. I propose to simply remove these hosts from the host map and clear\nthe current host list so that no values will be saved, effectively\ncausing invalid hosts to result in the same as unknown hosts.\n\n-- \nJonas Fonseca\n"},{"id":"91543","messageId":"20080924233104.GG3669@spearce.org","threadId":"15606","inReplyTo":"20080924232519.GA15318@diku.dk","subject":"Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-24T23:31:04Z","receivedAt":"2008-09-24T23:31:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jonas Fonseca wrote:\n> Shawn O. Pearce wrote Mon, Sep 22, 2008:\n> > Robin Rosenberg wrote:\n> > > söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:\n> > > > +\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n...\n> > > This one is really (as you noted) bad so we shouldn't allow it at all.\n> \n> Using exceptions seems a bit harsh, since the quote is not really fatal\n> in anyway.\n...\n> I propose to simply remove these hosts from the host map and clear\n> the current host list so that no values will be saved, effectively\n> causing invalid hosts to result in the same as unknown hosts.\n\nYea, that seems quite reasonable.\n\nIf you want more debugging than that on your ~/.ssh/config file then\nrun OpenSSH tools on it. Hell, I can't count the number of times\nI've made typos in there and couldn't figure out why it was still\nasking for a password, etc. And that's just the OpenSSH command\nline tools.\n\n-- \nShawn.\n"},{"id":"91609","messageId":"200809250826.59765.robin.rosenberg.lists@dewire.com","threadId":"15606","inReplyTo":"20080924232519.GA15318@diku.dk","subject":"Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2008-09-25T06:26:56Z","receivedAt":"2008-09-25T06:26:56Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"torsdagen den 25 september 2008 01.25.19 skrev Jonas Fonseca:\n> Using exceptions seems a bit harsh, since the quote is not really fatal\n> in anyway. Also, for badly formatted Port values the value is simply ignored.\n\nFor this case OpenSSH complains about a missing parameter. It does not\neven attempt to look up the host. For malformed port numbers I get \"Bad number\"\n(and no attempt to connect. I think an exception is the right way to solve the\nproblem si nce the caller doesn't have to do the error checking. and the error\nis more obvious to the user. OpenSSH also gives the line number. Maybe we\nshould too. \n\nThe OpenSSH i'm referencing is openssh-5.1p1-1mdv2009.0\n\n-- robin\n"},{"id":"91565","messageId":"20080925083934.GB10273@diku.dk","threadId":"15606","inReplyTo":"20080924233104.GG3669@spearce.org","subject":"[JGIT PATCH] Add tests for handling of parsing errors in OpenSshConfig","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-25T08:39:34Z","receivedAt":"2008-09-25T08:39:34Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Badly quoted entries are now ignored similar to how bad port number are\ncurrently ignored. A check for negative port numbers is now performed\nso that they also will be ignored.\n\nSigned-off-by: Jonas Fonseca \n---\n .../spearce/jgit/transport/OpenSshConfigTest.java | 32 +++++++++++++++---\n .../org/spearce/jgit/transport/OpenSshConfig.java | 34 +++++++++++++++----\n 2 files changed, 53 insertions(+), 13 deletions(-)\n\n Shawn O. Pearce wrote Wed, Sep 24, 2008:\n > Jonas Fonseca wrote:\n > > I propose to simply remove these hosts from the host map and clear\n > > the current host list so that no values will be saved, effectively\n > > causing invalid hosts to result in the same as unknown hosts.\n > \n > Yea, that seems quite reasonable.\n > \n > If you want more debugging than that on your ~/.ssh/config file then\n > run OpenSSH tools on it. Hell, I can't count the number of times\n > I've made typos in there and couldn't figure out why it was still\n > asking for a password, etc. And that's just the OpenSSH command\n > line tools.\n\n Heh, so below makes JGit compatible with OpenSSH's silent treatment. I\n also fixed the .ssh/config parser to ignore negative port numbers.\n\ndiff --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\nindex ad6e79c..ccd7400 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n@@ -114,11 +114,7 @@ config(\"Host \\\"good\\\"\\n\" +\n \t\t\t\" Port=\\\"2222\\\"\\n\" +\n \t\t\t\"Host \\\"spaced\\\"\\n\" +\n \t\t\t\"# Bad host name, but testing preservation of spaces\\n\" +\n-\t\t\t\" HostName=\\\" spaced\\ttld \\\"\\n\" +\n-\t\t\t\"# Misbalanced quotes\\n\" +\n-\t\t\t\"Host \\\"bad\\\"\\n\" +\n-\t\t\t\"# OpenSSH doesn't allow this but ...\\n\" +\n-\t\t\t\" HostName=bad.tld\\\"\\n\");\n+\t\t\t\" HostName=\\\" spaced\\ttld \\\"\\n\");\n \t\tassertEquals(\"good.tld\", osc.lookup(\"good\").getHostName());\n \t\tassertEquals(\"gooduser\", osc.lookup(\"good\").getUser());\n \t\tassertEquals(6007, osc.lookup(\"good\").getPort());\n@@ -128,7 +124,31 @@ config(\"Host \\\"good\\\"\\n\" +\n \t\tassertEquals(2222, osc.lookup(\"unquoted\").getPort());\n \t\tassertEquals(2222, osc.lookup(\"hosts\").getPort());\n \t\tassertEquals(\" spaced\\ttld \", osc.lookup(\"spaced\").getHostName());\n-\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n+\t}\n+\n+\tpublic void testParsingErrors() throws Exception {\n+\t\tconfig(\"Host negativeportnumber\\n\" +\n+\t\t \"Port -200\\n\" +\n+\t\t \"Host badportnumber\\n\" +\n+\t\t \"Port twentytwo\\n\" +\n+\t\t \"Host badportquote\\n\" +\n+\t\t \"Port \\\"2222\\n\" +\n+\t\t \"Host badportquote2\\n\" +\n+\t\t \"Port 2222\\\"\\n\" +\n+\t\t \"# Misbalanced quotes\\n\" +\n+\t\t \"Host badly \\\"quoted\\n\" +\n+\t\t \"HostName=unknown\\n\");\n+\n+\t\tassertEquals(DefaultSshSessionFactory.SSH_PORT,\n+\t\t\t osc.lookup(\"negativeportnumber\").getPort());\n+\t\tassertEquals(DefaultSshSessionFactory.SSH_PORT,\n+\t\t\t osc.lookup(\"badportnumber\").getPort());\n+\t\tassertEquals(DefaultSshSessionFactory.SSH_PORT,\n+\t\t\t osc.lookup(\"badportquote\").getPort());\n+\t\tassertEquals(DefaultSshSessionFactory.SSH_PORT,\n+\t\t\t osc.lookup(\"badportquote2\").getPort());\n+\t\tassertNotSame(\"unknown\", osc.lookup(\"badly\").getHostName());\n+\t\tassertNotSame(\"unknown\", osc.lookup(\"\\\"quoted\"));\n \t}\n \n \tpublic void testAlias_DoesNotMatch() throws Exception {\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\nindex b08d5c6..6a3f2c1 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n@@ -46,6 +46,7 @@\n import java.io.InputStreamReader;\n import java.util.ArrayList;\n import java.util.Collections;\n+import java.util.Iterator;\n import java.util.LinkedHashMap;\n import java.util.List;\n import java.util.Map;\n@@ -175,6 +176,15 @@ public Host lookup(final String hostName) {\n \t\t\t\tcurrent.clear();\n \t\t\t\tfor (final String pattern : argValue.split(\"[ \\t]\")) {\n \t\t\t\t\tfinal String name = dequote(pattern);\n+\t\t\t\t\tif (name == null) {\n+\t\t\t\t\t\t/* Prune all the current hosts. */\n+\t\t\t\t\t\tIterator it = m.values().iterator();\n+\t\t\t\t\t\twhile (it.hasNext())\n+\t\t\t\t\t\t\tif (current.contains(it.next()))\n+\t\t\t\t\t\t\t\tit.remove();\n+\t\t\t\t\t\tcurrent.clear();\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\t}\n \t\t\t\t\tHost c = m.get(name);\n \t\t\t\t\tif (c == null) {\n \t\t\t\t\t\tc = new Host();\n@@ -192,17 +202,23 @@ public Host lookup(final String hostName) {\n \t\t\t\tcontinue;\n \t\t\t}\n \n+\t\t\tfinal String value = dequote(argValue);\n+\t\t\tif (value == null)\n+\t\t\t\tcontinue;\n+\n \t\t\tif (\"HostName\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.hostName == null)\n-\t\t\t\t\t\tc.hostName = dequote(argValue);\n+\t\t\t\t\t\tc.hostName = value;\n \t\t\t} else if (\"User\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.user == null)\n-\t\t\t\t\t\tc.user = dequote(argValue);\n+\t\t\t\t\t\tc.user = value;\n \t\t\t} else if (\"Port\".equalsIgnoreCase(keyword)) {\n \t\t\t\ttry {\n-\t\t\t\t\tfinal int port = Integer.parseInt(dequote(argValue));\n+\t\t\t\t\tfinal int port = Integer.parseInt(value);\n+\t\t\t\t\tif (port < 0)\n+\t\t\t\t\t\tcontinue;\n \t\t\t\t\tfor (final Host c : current)\n \t\t\t\t\t\tif (c.port == 0)\n \t\t\t\t\t\t\tc.port = port;\n@@ -212,15 +228,15 @@ public Host lookup(final String hostName) {\n \t\t\t} else if (\"IdentityFile\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.identityFile == null)\n-\t\t\t\t\t\tc.identityFile = toFile(dequote(argValue));\n+\t\t\t\t\t\tc.identityFile = toFile(value);\n \t\t\t} else if (\"PreferredAuthentications\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.preferredAuthentications == null)\n-\t\t\t\t\t\tc.preferredAuthentications = nows(dequote(argValue));\n+\t\t\t\t\t\tc.preferredAuthentications = nows(value);\n \t\t\t} else if (\"BatchMode\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.batchMode == null)\n-\t\t\t\t\t\tc.batchMode = yesno(dequote(argValue));\n+\t\t\t\t\t\tc.batchMode = yesno(value);\n \t\t\t}\n \t\t}\n \n@@ -243,8 +259,12 @@ private static boolean isHostMatch(final String pattern, final String name) {\n \t}\n \n \tprivate static String dequote(final String value) {\n-\t\tif (value.startsWith(\"\\\"\") && value.endsWith(\"\\\"\"))\n+\t\tfinal boolean hasStart = value.startsWith(\"\\\"\");\n+\t\tfinal boolean hasEnd = value.endsWith(\"\\\"\");\n+\t\tif (hasStart && hasEnd)\n \t\t\treturn value.substring(1, value.length() - 1);\n+\t\tif (hasStart || hasEnd)\n+\t\t\treturn null;\n \t\treturn value;\n \t}\n \n-- \ntg: (cf67dfc..) jf/opensshconfigquote (depends on: master)\n\n-- \nJonas Fonseca\n"},{"id":"91579","messageId":"tQAHDMR4.1222342394.9002900.roro@localhost","threadId":"15606","inReplyTo":"20080925083934.GB10273@diku.dk","subject":"Re: [JGIT PATCH] Add tests for handling of parsing errors in OpenSshConfig","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-09-25T11:33:14Z","receivedAt":"2008-09-25T11:33:14Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Den 9/25/2008, skrev \"Jonas Fonseca\" :\n\n>Badly quoted entries are now ignored similar to how bad port number are\n>currently ignored. A check for negative port numbers is now performed\n>so that they also will be ignored.\n\nNooo. We should really give some feedback on bad entries. OpenSSH\n(version 5.1) complains and refuses to connect if I give it a bad port\nnumber or hostname. It complains about \"missing parameter\" and bad\nport number for these cases. OpenSSH doesn't simply ignore them.\n\nHostName=bad\"\nPort=hubba\n\nEven if openssh would ignore them I'd think it would be bad if we did,\nunless there was some serious compatibility issue here.\n\n-- robin\n"},{"id":"91592","messageId":"20080925132937.GA16151@diku.dk","threadId":"15606","inReplyTo":"tQAHDMR4.1222342394.9002900.roro@localhost","subject":"[JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2008-09-25T13:29:37Z","receivedAt":"2008-09-25T13:29:37Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Badly quoted entries are now ignored similar to how bad port number are\ncurrently ignored. A check for negative port numbers is now performed\nso that they also will be ignored.\n\nSigned-off-by: Jonas Fonseca \n\n---\n .../spearce/jgit/transport/OpenSshConfigTest.java | 46 +++++++++++++++---\n .../jgit/transport/DefaultSshSessionFactory.java | 10 +++-\n .../org/spearce/jgit/transport/OpenSshConfig.java | 51 ++++++++++++-------\n 3 files changed, 79 insertions(+), 28 deletions(-)\n\n Robin Rosenberg wrote Thu, Sep 25, 2008:\n > Den 9/25/2008, skrev \"Jonas Fonseca\" :\n > \n > >Badly quoted entries are now ignored similar to how bad port number are\n > >currently ignored. A check for negative port numbers is now performed\n > >so that they also will be ignored.\n > \n > Nooo. We should really give some feedback on bad entries. OpenSSH\n > (version 5.1) complains and refuses to connect if I give it a bad port\n > number or hostname. It complains about \"missing parameter\" and bad\n > port number for these cases. OpenSSH doesn't simply ignore them.\n > \n > HostName=bad\"\n > Port=hubba\n > \n > Even if openssh would ignore them I'd think it would be bad if we did,\n > unless there was some serious compatibility issue here.\n\n Maybe something like this? It adds OpenSshConfigException which will\n \"contain\" either a NumberFormatException or a ParseException.\n\ndiff --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\nindex ad6e79c..6b7bb4b 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/OpenSshConfigTest.java\n@@ -42,6 +42,7 @@\n import java.io.IOException;\n import java.io.OutputStreamWriter;\n \n+import org.spearce.jgit.errors.OpenSshConfigException;\n import org.spearce.jgit.lib.RepositoryTestCase;\n import org.spearce.jgit.transport.OpenSshConfig.Host;\n \n@@ -72,7 +73,18 @@ private void config(final String data) throws IOException {\n \t\tfw.close();\n \t}\n \n-\tpublic void testNoConfig() {\n+\tprivate boolean parseError(String config) throws Exception {\n+\t\tconfig(config);\n+\t\tOpenSshConfigException osce = null;\n+\t\ttry {\n+\t\t\tosc.lookup(\"something\");\n+\t\t} catch (OpenSshConfigException cause) {\n+\t\t\treturn true;\n+\t\t}\n+\t\treturn false;\n+\t}\n+\n+\tpublic void testNoConfig() throws Exception {\n \t\tfinal Host h = osc.lookup(\"repo.or.cz\");\n \t\tassertNotNull(h);\n \t\tassertEquals(\"repo.or.cz\", h.getHostName());\n@@ -114,11 +126,7 @@ config(\"Host \\\"good\\\"\\n\" +\n \t\t\t\" Port=\\\"2222\\\"\\n\" +\n \t\t\t\"Host \\\"spaced\\\"\\n\" +\n \t\t\t\"# Bad host name, but testing preservation of spaces\\n\" +\n-\t\t\t\" HostName=\\\" spaced\\ttld \\\"\\n\" +\n-\t\t\t\"# Misbalanced quotes\\n\" +\n-\t\t\t\"Host \\\"bad\\\"\\n\" +\n-\t\t\t\"# OpenSSH doesn't allow this but ...\\n\" +\n-\t\t\t\" HostName=bad.tld\\\"\\n\");\n+\t\t\t\" HostName=\\\" spaced\\ttld \\\"\\n\");\n \t\tassertEquals(\"good.tld\", osc.lookup(\"good\").getHostName());\n \t\tassertEquals(\"gooduser\", osc.lookup(\"good\").getUser());\n \t\tassertEquals(6007, osc.lookup(\"good\").getPort());\n@@ -128,7 +136,31 @@ config(\"Host \\\"good\\\"\\n\" +\n \t\tassertEquals(2222, osc.lookup(\"unquoted\").getPort());\n \t\tassertEquals(2222, osc.lookup(\"hosts\").getPort());\n \t\tassertEquals(\" spaced\\ttld \", osc.lookup(\"spaced\").getHostName());\n-\t\tassertEquals(\"bad.tld\\\"\", osc.lookup(\"bad\").getHostName());\n+\t}\n+\n+\tpublic void testNegativePortNumber() throws Exception {\n+\t\tassertTrue(parseError(\"Host negativeportnumber\\n\" +\n+\t\t\t \"Port -200\\n\"));\n+\t}\n+\n+\tpublic void testBadPortNumber() throws Exception {\n+\t\tassertTrue(parseError(\"Host badportnumber\\n\" +\n+\t\t \"Port twentytwo\\n\"));\n+\t}\n+\n+\tpublic void testBadlyQuotedPortNumber() throws Exception {\n+\t\tassertTrue(parseError(\"Host badportquote\\n\" +\n+\t\t \"Port \\\"2222\\n\"));\n+\t}\n+\n+\tpublic void testBadlyQuotedPortNumber2() throws Exception {\n+\t\tassertTrue(parseError(\"Host badportquote2\\n\" +\n+\t\t \"Port 2222\\\"\\n\"));\n+\t}\n+\n+\tpublic void testBadlyQuotedHost() throws Exception {\n+\t\tassertTrue(parseError(\"Host badly \\\"quoted\\n\" +\n+\t\t \"HostName=unknown\\n\"));\n \t}\n \n \tpublic void testAlias_DoesNotMatch() throws Exception {\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\nindex 89beab7..123a9b5 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n@@ -65,6 +65,7 @@\n import com.jcraft.jsch.Session;\n import com.jcraft.jsch.UIKeyboardInteractive;\n import com.jcraft.jsch.UserInfo;\n+import org.spearce.jgit.errors.OpenSshConfigException;\n \n /**\n * Loads known hosts and private keys from $HOME/.ssh.\n@@ -89,7 +90,12 @@\n \t@Override\n \tpublic synchronized Session getSession(String user, String pass,\n \t\t\tString host, int port) throws JSchException {\n-\t\tfinal OpenSshConfig.Host hc = getConfig().lookup(host);\n+\t\tfinal OpenSshConfig.Host hc;\n+\t\ttry {\n+\t\t\thc = getConfig().lookup(host);\n+\t\t} catch (OpenSshConfigException osce) {\n+\t\t\tthrow new JSchException(osce.getMessage());\n+\t\t}\n \t\thost = hc.getHostName();\n \t\tif (port <= 0)\n \t\t\tport = hc.getPort();\n@@ -128,7 +134,7 @@ private JSch getUserJSch() throws JSchException {\n \t\treturn userJSch;\n \t}\n \n-\tprivate OpenSshConfig getConfig() {\n+\tprivate OpenSshConfig getConfig() throws OpenSshConfigException {\n \t\tif (config == null)\n \t\t\tconfig = OpenSshConfig.get();\n \t\treturn config;\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\nindex b08d5c6..95a37f5 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n@@ -44,13 +44,16 @@\n import java.io.IOException;\n import java.io.InputStream;\n import java.io.InputStreamReader;\n+import java.text.ParseException;\n import java.util.ArrayList;\n import java.util.Collections;\n+import java.util.Iterator;\n import java.util.LinkedHashMap;\n import java.util.List;\n import java.util.Map;\n \n import org.spearce.jgit.errors.InvalidPatternException;\n+import org.spearce.jgit.errors.OpenSshConfigException;\n import org.spearce.jgit.fnmatch.FileNameMatcher;\n import org.spearce.jgit.util.FS;\n \n@@ -72,7 +75,7 @@\n \t *\n \t * @return a caching reader of the user's configuration file.\n \t */\n-\tpublic static OpenSshConfig get() {\n+\tpublic static OpenSshConfig get() throws OpenSshConfigException {\n \t\tFile home = FS.userHome();\n \t\tif (home == null)\n \t\t\thome = new File(\".\").getAbsoluteFile();\n@@ -110,7 +113,7 @@ protected OpenSshConfig(final File h, final File cfg) {\n \t * configuration file.\n \t * @return r configuration for the requested name. Never null.\n \t */\n-\tpublic Host lookup(final String hostName) {\n+\tpublic Host lookup(final String hostName) throws OpenSshConfigException {\n \t\tfinal Map cache = refresh();\n \t\tHost h = cache.get(hostName);\n \t\tif (h == null)\n@@ -136,7 +139,7 @@ public Host lookup(final String hostName) {\n \t\treturn h;\n \t}\n \n-\tprivate synchronized Map refresh() {\n+\tprivate synchronized Map refresh() throws OpenSshConfigException {\n \t\tfinal long mtime = configFile.lastModified();\n \t\tif (mtime != lastModified) {\n \t\t\ttry {\n@@ -146,6 +149,10 @@ public Host lookup(final String hostName) {\n \t\t\t\t} finally {\n \t\t\t\t\tin.close();\n \t\t\t\t}\n+\t\t\t} catch (NumberFormatException nfe) {\n+\t\t\t\tthrow new OpenSshConfigException(\"Parse error\", nfe);\n+\t\t\t} catch (ParseException pe) {\n+\t\t\t\tthrow new OpenSshConfigException(\"Parse error\", pe);\n \t\t\t} catch (FileNotFoundException none) {\n \t\t\t\thosts = Collections.emptyMap();\n \t\t\t} catch (IOException err) {\n@@ -156,7 +163,7 @@ public Host lookup(final String hostName) {\n \t\treturn hosts;\n \t}\n \n-\tprivate Map parse(final InputStream in) throws IOException {\n+\tprivate Map parse(final InputStream in) throws IOException, ParseException {\n \t\tfinal Map m = new LinkedHashMap();\n \t\tfinal BufferedReader br = new BufferedReader(new InputStreamReader(in));\n \t\tfinal List current = new ArrayList(4);\n@@ -192,35 +199,37 @@ public Host lookup(final String hostName) {\n \t\t\t\tcontinue;\n \t\t\t}\n \n+\t\t\tfinal String value = dequote(argValue);\n+\t\t\tif (value == null)\n+\t\t\t\tcontinue;\n+\n \t\t\tif (\"HostName\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.hostName == null)\n-\t\t\t\t\t\tc.hostName = dequote(argValue);\n+\t\t\t\t\t\tc.hostName = value;\n \t\t\t} else if (\"User\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.user == null)\n-\t\t\t\t\t\tc.user = dequote(argValue);\n+\t\t\t\t\t\tc.user = value;\n \t\t\t} else if (\"Port\".equalsIgnoreCase(keyword)) {\n-\t\t\t\ttry {\n-\t\t\t\t\tfinal int port = Integer.parseInt(dequote(argValue));\n-\t\t\t\t\tfor (final Host c : current)\n-\t\t\t\t\t\tif (c.port == 0)\n-\t\t\t\t\t\t\tc.port = port;\n-\t\t\t\t} catch (NumberFormatException nfe) {\n-\t\t\t\t\t// Bad port number. Don't set it.\n-\t\t\t\t}\n+\t\t\t\tfinal int port = Integer.parseInt(value);\n+\t\t\t\tif (port < 0)\n+\t\t\t\t\tthrow new NumberFormatException(\"Negative port number\");\n+\t\t\t\tfor (final Host c : current)\n+\t\t\t\t\tif (c.port == 0)\n+\t\t\t\t\t\tc.port = port;\n \t\t\t} else if (\"IdentityFile\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.identityFile == null)\n-\t\t\t\t\t\tc.identityFile = toFile(dequote(argValue));\n+\t\t\t\t\t\tc.identityFile = toFile(value);\n \t\t\t} else if (\"PreferredAuthentications\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.preferredAuthentications == null)\n-\t\t\t\t\t\tc.preferredAuthentications = nows(dequote(argValue));\n+\t\t\t\t\t\tc.preferredAuthentications = nows(value);\n \t\t\t} else if (\"BatchMode\".equalsIgnoreCase(keyword)) {\n \t\t\t\tfor (final Host c : current)\n \t\t\t\t\tif (c.batchMode == null)\n-\t\t\t\t\t\tc.batchMode = yesno(dequote(argValue));\n+\t\t\t\t\t\tc.batchMode = yesno(value);\n \t\t\t}\n \t\t}\n \n@@ -242,9 +251,13 @@ private static boolean isHostMatch(final String pattern, final String name) {\n \t\treturn fn.isMatch();\n \t}\n \n-\tprivate static String dequote(final String value) {\n-\t\tif (value.startsWith(\"\\\"\") && value.endsWith(\"\\\"\"))\n+\tprivate static String dequote(final String value) throws ParseException {\n+\t\tfinal boolean hasStart = value.startsWith(\"\\\"\");\n+\t\tfinal boolean hasEnd = value.endsWith(\"\\\"\");\n+\t\tif (hasStart && hasEnd)\n \t\t\treturn value.substring(1, value.length() - 1);\n+\t\tif (hasStart || hasEnd)\n+\t\t\tthrow new ParseException(\"Quote mismatch\", value.indexOf(\"\\\"\"));\n \t\treturn value;\n \t}\n \n-- \ntg: (cf67dfc..) jf/opensshconfigquote (depends on: master)\n\n-- \nJonas Fonseca\n"},{"id":"91597","messageId":"2c6b72b30809250730kf643fb3p9881e82b998ca100@mail.gmail.com","threadId":"15606","inReplyTo":"20080925132937.GA16151@diku.dk","subject":"Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig","fromName":"Jonas Fonseca","fromEmail":"jonas.fonseca@gmail.com","sentAt":"2008-09-25T14:30:19Z","receivedAt":"2008-09-25T14:30:19Z","isPatch":true,"sender":{"key":"jonas.fonseca@gmail.com","avatar":"https://gravatar.com/avatar/9b7fa23cce50269e5d164312b6ac5ae818a180f837b38f28f3bdf689dd7f96cd?d=mp&s=160"},"body":"On Thu, Sep 25, 2008 at 15:29, Jonas Fonseca wrote:\n> Badly quoted entries are now ignored similar to how bad port number are\n> currently ignored. A check for negative port numbers is now performed\n> so that they also will be ignored.\n\nSorry for spamming this list today. The commit message should of\ncourse read something like:\n\nThrow an OpenSshConfigException when parsing meet badly quoted entries\nand port numbers, which do not contain numbers or are negative.\n\n-- \nJonas Fonseca\n"},{"id":"91606","messageId":"20080925151632.GN3669@spearce.org","threadId":"15606","inReplyTo":"20080925132937.GA16151@diku.dk","subject":"Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-25T15:16:32Z","receivedAt":"2008-09-25T15:16:32Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jonas Fonseca wrote:\n> Badly quoted entries are now ignored similar to how bad port number are\n> currently ignored. A check for negative port numbers is now performed\n> so that they also will be ignored.\n> \n> Signed-off-by: Jonas Fonseca \n> \n> ---\n> .../spearce/jgit/transport/OpenSshConfigTest.java | 46 +++++++++++++++---\n> .../jgit/transport/DefaultSshSessionFactory.java | 10 +++-\n> .../org/spearce/jgit/transport/OpenSshConfig.java | 51 ++++++++++++-------\n> 3 files changed, 79 insertions(+), 28 deletions(-)\n\nWell, at first glance the new OpenSshConfigException is missing\nfrom the patch. We need that class to compile correctly. ;-)\n \n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n> index 89beab7..123a9b5 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n> @@ -89,7 +90,12 @@\n> \t@Override\n> \tpublic synchronized Session getSession(String user, String pass,\n> \t\t\tString host, int port) throws JSchException {\n> -\t\tfinal OpenSshConfig.Host hc = getConfig().lookup(host);\n> +\t\tfinal OpenSshConfig.Host hc;\n> +\t\ttry {\n> +\t\t\thc = getConfig().lookup(host);\n> +\t\t} catch (OpenSshConfigException osce) {\n> +\t\t\tthrow new JSchException(osce.getMessage());\n> +\t\t}\n\nI would perfer to chain the OpenSshConfigException as the cause of\nthe JSchException. That way the caller has a chance to give us a\ncomplete stack trace, including the cause of the OSCE being the\ninner NumberFormatException or ParseException.\n\nRobin or I will need to edit the EclipseSshSessionFactory to add\nthis same sort of try/catch. To keep the tree buildable we'll\nwant to squash that into your patch. Yea, sorry, this is where\nthe egit+jgit within one repository is going to bite us.\n\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n> index b08d5c6..95a37f5 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java\n> @@ -146,6 +149,10 @@ public Host lookup(final String hostName) {\n> \t\t\t\t} finally {\n> \t\t\t\t\tin.close();\n> \t\t\t\t}\n> +\t\t\t} catch (NumberFormatException nfe) {\n> +\t\t\t\tthrow new OpenSshConfigException(\"Parse error\", nfe);\n> +\t\t\t} catch (ParseException pe) {\n> +\t\t\t\tthrow new OpenSshConfigException(\"Parse error\", pe);\n> \t\t\t} catch (FileNotFoundException none) {\n> \t\t\t\thosts = Collections.emptyMap();\n> \t\t\t} catch (IOException err) {\n\nIf we are really going to this level of effort, can we please have\nthe file path and line number of the invalid line in the exception?\n\nWe rarely parse the ~/.ssh/config. In general its parsed only once\nduring startup. Going through a bit more work at parse time to get\nmore accurate error messages is acceptable.\n\nI think it may be a good idea to have the exception thrown only\nwhen a bad Host block is accessed, or if we try to access an unknown\nhost and there is an unreadable Host block.\n\nSo I'm thinking more like we stuff an OSCE instance into the Host\nblock if the host has a bad entry, and during get() or lookup()\nwe test for the exception and rethrow the exception.\n\nE.g. lets say my config file is this:\n\n\t$ cat ~/.ssh/config\n\tHost work\n\t\tHostname internal.google.com\"\n\t\tPort -1\n\n\tHost orcz\n\t\tHostname repo.or.cz\n\t\tUser spearce\n\t\tPort 22\n\nthen:\n\n jgit fetch orcz:foo.git; # works without error\n jgit fetch work:foo.git; # throws OSCE due to bad Hostname\n jgit fetch kernel.org:foo.git; # works as no Host matched\n\nHowever if my config was more bogus, e.g.:\n\n\t$ cat ~/.ssh/config\n\tHost work\"\n\t\tUser sop\n\n\tHost orcz\n\t\tHostname repo.or.cz\n\t\tUser spearce\n\t\tPort 22\n\nthen:\n\n jgit fetch orcz:foo.git; # still works without error\n jgit fetch work:foo.git; # fails due to bad Host header\n jgit fetch kernel.org:foo.git; # fails due to bad Host header\n\n-- \nShawn.\n"}]}