From 565fd784a62099837457a1df6c7fd0338c02f235 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Fri, 13 Sep 2024 12:51:43 -0400 Subject: [PATCH 1/6] fix: escape ampersand and question mark in ProxyCommand Fixes #479. --- src/main/kotlin/com/coder/gateway/util/Escape.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/kotlin/com/coder/gateway/util/Escape.kt b/src/main/kotlin/com/coder/gateway/util/Escape.kt index 8cb71a28f..05ce852b7 100644 --- a/src/main/kotlin/com/coder/gateway/util/Escape.kt +++ b/src/main/kotlin/com/coder/gateway/util/Escape.kt @@ -12,8 +12,10 @@ fun escape(s: String): String { if (s.contains("\n")) { throw Exception("argument cannot contain newlines") } - if (s.contains(" ") || s.contains("\t")) { - return "\"" + s.replace("\"", "\\\"") + "\"" + if (s.contains(" ") || s.contains("\t") || s.contains("&") || s.contains("?")) { + // See https://github.com/coder/jetbrains-coder/issues/479 + // Escape existing " and & + return "\"" + s.replace("\"", "\\\"").replace("&", "\\&").replace("?", "\\?") + "\"" } return s.replace("\"", "\\\"") } From 209077ccc0324305c52bf38023872e5c85a6ee5e Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Sep 2024 10:12:43 -0800 Subject: [PATCH 2/6] Update platform version Looks like the one we were using disappeared again. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index a4325041c..c6ab9e544 100644 --- a/gradle.properties +++ b/gradle.properties @@ -26,7 +26,7 @@ pluginUntilBuild=242.* # that exists, ideally the most recent one, for example # 233.15325-EAP-CANDIDATE-SNAPSHOT). platformType=GW -platformVersion=233.15325-EAP-CANDIDATE-SNAPSHOT +platformVersion=233.15619-EAP-CANDIDATE-SNAPSHOT instrumentationCompiler=242.19533-EAP-CANDIDATE-SNAPSHOT # Gateway does not have open sources. platformDownloadSources=true From ee23d22725f0d7118ede971fc5b471edb85d57dc Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Sep 2024 10:25:58 -0800 Subject: [PATCH 3/6] Rely on quotes for escaping Also add some notes on characters double quotes does not handle, in case we need to do something about them in the future. --- src/main/kotlin/com/coder/gateway/util/Escape.kt | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/com/coder/gateway/util/Escape.kt b/src/main/kotlin/com/coder/gateway/util/Escape.kt index 05ce852b7..594d5cf28 100644 --- a/src/main/kotlin/com/coder/gateway/util/Escape.kt +++ b/src/main/kotlin/com/coder/gateway/util/Escape.kt @@ -1,10 +1,18 @@ package com.coder.gateway.util +import com.intellij.icons.ExpUiIcons.Nodes.Exception + /** * Escape an argument to be used in the ProxyCommand of an SSH config. * - * Escaping happens by surrounding with double quotes if the argument contains - * whitespace and escaping any existing double quotes regardless of whitespace. + * Escaping happens by: + * 1. Surrounding with double quotes if the argument contains whitespace, ?, or + * & (to handle query parameters in URLs) as these characters have special + * meaning in shells. + * 2. Always escaping existing double quotes. + * + * Double quotes does not preserve the literal values of $, `, \, *, @, and ! + * (when history expansion is enabled); these are not currently handled. * * Throws if the argument is invalid. */ @@ -13,9 +21,7 @@ fun escape(s: String): String { throw Exception("argument cannot contain newlines") } if (s.contains(" ") || s.contains("\t") || s.contains("&") || s.contains("?")) { - // See https://github.com/coder/jetbrains-coder/issues/479 - // Escape existing " and & - return "\"" + s.replace("\"", "\\\"").replace("&", "\\&").replace("?", "\\?") + "\"" + return "\"" + s.replace("\"", "\\\"") + "\"" } return s.replace("\"", "\\\"") } From 57635e1ba4666da5dbbcca67c2aa86db1d2546cc Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Sep 2024 10:37:54 -0800 Subject: [PATCH 4/6] Add tests for escaping URLs --- src/test/fixtures/outputs/url.conf | 16 ++++++++++++++++ .../com/coder/gateway/cli/CoderCLIManagerTest.kt | 10 +++++++++- .../kotlin/com/coder/gateway/util/EscapeTest.kt | 4 ++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 src/test/fixtures/outputs/url.conf diff --git a/src/test/fixtures/outputs/url.conf b/src/test/fixtures/outputs/url.conf new file mode 100644 index 000000000..8854325c7 --- /dev/null +++ b/src/test/fixtures/outputs/url.conf @@ -0,0 +1,16 @@ +# --- START CODER JETBRAINS test.coder.invalid +Host coder-jetbrains--url--test.coder.invalid + ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --url "https://test.coder.invalid?foo=bar&baz=qux" ssh --stdio --usage-app=jetbrains url + ConnectTimeout 0 + StrictHostKeyChecking no + UserKnownHostsFile /dev/null + LogLevel ERROR + SetEnv CODER_SSH_SESSION_TYPE=JetBrains +Host coder-jetbrains--url--test.coder.invalid--bg + ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --url "https://test.coder.invalid?foo=bar&baz=qux" ssh --stdio --usage-app=disable url + ConnectTimeout 0 + StrictHostKeyChecking no + UserKnownHostsFile /dev/null + LogLevel ERROR + SetEnv CODER_SSH_SESSION_TYPE=JetBrains +# --- END CODER JETBRAINS test.coder.invalid diff --git a/src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt index 1cbe5747a..1baafe540 100644 --- a/src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt @@ -303,6 +303,7 @@ internal class CoderCLIManagerTest { val extraConfig: String = "", val env: Environment = Environment(), val sshLogDirectory: Path? = null, + val url: URL? = null ) @Test @@ -390,6 +391,13 @@ internal class CoderCLIManagerTest { "blank", sshLogDirectory = tmpdir.resolve("ssh-logs"), ), + SSHTest( + listOf("url"), + input = null, + output = "url", + remove = "blank", + url = URL("https://test.coder.invalid?foo=bar&baz=qux"), + ), ) val newlineRe = "\r?\n".toRegex() @@ -408,7 +416,7 @@ internal class CoderCLIManagerTest { env = it.env, ) - val ccm = CoderCLIManager(URL("https://test.coder.invalid"), settings) + val ccm = CoderCLIManager(it.url ?: URL("https://test.coder.invalid"), settings) // Input is the configuration that we start with, if any. if (it.input != null) { diff --git a/src/test/kotlin/com/coder/gateway/util/EscapeTest.kt b/src/test/kotlin/com/coder/gateway/util/EscapeTest.kt index 8da5232e5..3e8265874 100644 --- a/src/test/kotlin/com/coder/gateway/util/EscapeTest.kt +++ b/src/test/kotlin/com/coder/gateway/util/EscapeTest.kt @@ -15,6 +15,10 @@ internal class EscapeTest { """C:\echo "hello world"""" to """"C:\echo \"hello world\""""", """C:\"no"\"spaces"""" to """C:\\"no\"\\"spaces\"""", """"C:\Program Files\HeaderCommand.exe" --flag""" to """"\"C:\Program Files\HeaderCommand.exe\" --flag"""", + "https://coder.com" to """https://coder.com""", + "https://coder.com/?question" to """"https://coder.com/?question"""", + "https://coder.com/&ersand" to """"https://coder.com/&ersand"""", + "https://coder.com/?with&both" to """"https://coder.com/?with&both"""", ) tests.forEach { assertEquals(it.value, escape(it.key)) From 37c87975e5a4ec82da2dd4c573b12cb2189836c2 Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Sep 2024 10:54:38 -0800 Subject: [PATCH 5/6] Why did Intellij add this --- src/main/kotlin/com/coder/gateway/util/Escape.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/kotlin/com/coder/gateway/util/Escape.kt b/src/main/kotlin/com/coder/gateway/util/Escape.kt index 594d5cf28..af22bfe50 100644 --- a/src/main/kotlin/com/coder/gateway/util/Escape.kt +++ b/src/main/kotlin/com/coder/gateway/util/Escape.kt @@ -1,7 +1,5 @@ package com.coder.gateway.util -import com.intellij.icons.ExpUiIcons.Nodes.Exception - /** * Escape an argument to be used in the ProxyCommand of an SSH config. * From f3e8264d5a0968c46c0bf9a2cc427e981b5197bb Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Sep 2024 10:57:18 -0800 Subject: [PATCH 6/6] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf398b99b..cd56327f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ as "failed to retrieve IDEs". To aply this fix, you must add the connection again through the "Connect to Coder" flow or by using the dashboard link (the recent connections do not reconfigure SSH). +- When a proxy command argument (such as the URL) contains `?` and `&`, escape + it in the SSH config by using double quotes, as these characters have special + meanings in shells. ### Changed