fix: escape ampersand and question mark in ProxyCommand (#480)

* fix: escape ampersand and question mark in ProxyCommand

Also add some notes on characters double quotes does not handle, in case
we need to do something about them in the future.

Fixes #479.

* Update platform version

Looks like the one we were using disappeared again.

* Add tests for escaping URLs

* Add changelog entry

---------

Co-authored-by: Asher <[email protected]>

Author: kylecarbs

CHANGELOG.md

@@ -4,6 +4,12 @@
 
 ## Unreleased
 
+### Fixed
+
+- 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.
+
 ## 2.14.0 - 2024-08-30
 
 ### Fixed

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

src/main/kotlin/com/coder/gateway/util/Escape.kt

@@ -3,16 +3,22 @@ package com.coder.gateway.util
 /**
  * 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.
  */
 fun escape(s: String): String {
     if (s.contains("\n")) {
         throw Exception("argument cannot contain newlines")
     }
-    if (s.contains(" ") || s.contains("\t")) {
+    if (s.contains(" ") || s.contains("\t") || s.contains("&") || s.contains("?")) {
         return "\"" + s.replace("\"", "\\\"") + "\""
     }
     return s.replace("\"", "\\\"")

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

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) {

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/&ampersand" to """"https://coder.com/&ampersand"""",
+                "https://coder.com/?with&both" to """"https://coder.com/?with&both"""",
             )
         tests.forEach {
             assertEquals(it.value, escape(it.key))