Skip to content

Commit 1da515e

Browse files
code-ashercode-asher
authored
Prevent early variable expansion in header command (#364)
1 parent 71d3405 commit 1da515e

File tree

6 files changed

+74
-23
lines changed

6 files changed

+74
-23
lines changed

‎src/main/kotlin/com/coder/gateway/cli/CoderCLIManager.kt‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,15 @@ package com.coder.gateway.cli
33
import com.coder.gateway.cli.ex.MissingVersionException
44
import com.coder.gateway.cli.ex.ResponseException
55
import com.coder.gateway.cli.ex.SSHConfigFormatException
6-
import com.coder.gateway.settings.CoderSettings
76
import com.coder.gateway.services.CoderSettingsState
7+
import com.coder.gateway.settings.CoderSettings
88
import com.coder.gateway.util.CoderHostnameVerifier
99
import com.coder.gateway.util.InvalidVersionException
10-
import com.coder.gateway.util.SemVer
1110
import com.coder.gateway.util.OS
11+
import com.coder.gateway.util.SemVer
1212
import com.coder.gateway.util.coderSocketFactory
1313
import com.coder.gateway.util.escape
14+
import com.coder.gateway.util.escapeSubcommand
1415
import com.coder.gateway.util.getHeaders
1516
import com.coder.gateway.util.getOS
1617
import com.coder.gateway.util.safeHost
@@ -228,7 +229,7 @@ class CoderCLIManager(
228229
escape(localBinaryPath.toString()),
229230
"--global-config", escape(coderConfigPath.toString()),
230231
if (settings.headerCommand.isNotBlank()) "--header-command" else null,
231-
if (settings.headerCommand.isNotBlank()) escape(settings.headerCommand) else null,
232+
if (settings.headerCommand.isNotBlank()) escapeSubcommand(settings.headerCommand) else null,
232233
"ssh", "--stdio")
233234
val blockContent = workspaceNames.joinToString(
234235
System.lineSeparator(),

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

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
package com.coder.gateway.util
22

33
/**
4-
* Escape a command argument to be used in the ProxyCommand of an SSH
5-
* config. Surround with double quotes if the argument contains whitespace
6-
* and escape any existing double quotes.
4+
* Escape an argument to be used in the ProxyCommand of an SSH config.
5+
*
6+
* Escaping happens by surrounding with double quotes if the argument contains
7+
* whitespace and escaping any existing double quotes regardless of whitespace.
78
*
89
* Throws if the argument is invalid.
910
*/
@@ -15,4 +16,33 @@ fun escape(s: String): String {
1516
return "\"" + s.replace("\"", "\\\"") + "\""
1617
}
1718
return s.replace("\"", "\\\"")
18-
}
19+
}
20+
21+
/**
22+
* Escape an argument to be executed by the Coder binary such that expansions
23+
* happen in the binary and not in SSH.
24+
*
25+
* Escaping happens by wrapping in single quotes on Linux and escaping % on
26+
* Windows.
27+
*
28+
* Throws if the argument is invalid.
29+
*/
30+
fun escapeSubcommand(s: String): String {
31+
if (s.contains("\n")) {
32+
throw Exception("argument cannot contain newlines")
33+
}
34+
return if (getOS() == OS.WINDOWS) {
35+
// On Windows variables are in the format %VAR%. % is interpreted by
36+
// SSH as a special sequence and can be escaped with %%. Do not use
37+
// single quotes on Windows; they appear to only be used literally.
38+
return escape(s).replace("%", "%%")
39+
} else {
40+
// On *nix and similar systems variables are in the format $VAR. SSH
41+
// will expand these before executing the proxy command; we can prevent
42+
// this by using single quotes. You cannot escape single quotes inside
43+
// single quotes, so if there are existing quotes you end the current
44+
// quoted string, output an escaped quote, then start the quoted string
45+
// again.
46+
"'" + s.replace("'", "'\\''") + "'"
47+
}
48+
}

‎src/test/fixtures/outputs/header-command-windows.conf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# --- START CODER JETBRAINS test.coder.invalid
22
Host coder-jetbrains--header--test.coder.invalid
3-
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --header-command "C:\Program Files\My Header Command\\"also has quotes\"\HeaderCommand.exe" ssh --stdio header
3+
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --header-command "\"C:\Program Files\My Header Command\HeaderCommand.exe\" --url=\"%%CODER_URL%%\" --test=\"foo bar\"" ssh --stdio header
44
ConnectTimeout 0
55
StrictHostKeyChecking no
66
UserKnownHostsFile /dev/null

‎src/test/fixtures/outputs/header-command.conf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# --- START CODER JETBRAINS test.coder.invalid
22
Host coder-jetbrains--header--test.coder.invalid
3-
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --header-command "my-header-command \"test\"" ssh --stdio header
3+
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config --header-command 'my-header-command --url="$CODER_URL" --test="foo bar" --literal='\''$CODER_URL'\''' ssh --stdio header
44
ConnectTimeout 0
55
StrictHostKeyChecking no
66
UserKnownHostsFile /dev/null

‎src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,6 @@ package com.coder.gateway.cli
33
import com.coder.gateway.cli.ex.MissingVersionException
44
import com.coder.gateway.cli.ex.ResponseException
55
import com.coder.gateway.cli.ex.SSHConfigFormatException
6-
import kotlin.test.Test
7-
import kotlin.test.assertContains
8-
import kotlin.test.assertEquals
9-
import kotlin.test.assertFailsWith
10-
import kotlin.test.assertFalse
11-
import kotlin.test.assertNotEquals
12-
import kotlin.test.assertTrue
13-
import org.junit.jupiter.api.assertDoesNotThrow
14-
import org.junit.jupiter.api.BeforeAll
15-
166
import com.coder.gateway.services.CoderSettingsState
177
import com.coder.gateway.settings.CoderSettings
188
import com.coder.gateway.util.InvalidVersionException
@@ -24,13 +14,22 @@ import com.coder.gateway.util.sha1
2414
import com.coder.gateway.util.toURL
2515
import com.google.gson.JsonSyntaxException
2616
import com.sun.net.httpserver.HttpServer
17+
import org.junit.jupiter.api.BeforeAll
18+
import org.junit.jupiter.api.assertDoesNotThrow
19+
import org.zeroturnaround.exec.InvalidExitValueException
20+
import org.zeroturnaround.exec.ProcessInitException
2721
import java.net.HttpURLConnection
2822
import java.net.InetSocketAddress
2923
import java.net.URL
3024
import java.nio.file.AccessDeniedException
3125
import java.nio.file.Path
32-
import org.zeroturnaround.exec.InvalidExitValueException
33-
import org.zeroturnaround.exec.ProcessInitException
26+
import kotlin.test.Test
27+
import kotlin.test.assertContains
28+
import kotlin.test.assertEquals
29+
import kotlin.test.assertFailsWith
30+
import kotlin.test.assertFalse
31+
import kotlin.test.assertNotEquals
32+
import kotlin.test.assertTrue
3433

3534
internal class CoderCLIManagerTest {
3635
private fun mkbin(version: String): String {
@@ -253,8 +252,11 @@ internal class CoderCLIManagerTest {
253252
SSHTest(listOf("foo-bar"), "no-blocks", "append-no-blocks", "no-blocks", null),
254253
SSHTest(listOf("foo-bar"), "no-related-blocks", "append-no-related-blocks", "no-related-blocks", null),
255254
SSHTest(listOf("foo-bar"), "no-newline", "append-no-newline", "no-blocks", null),
256-
SSHTest(listOf("header"), null, "header-command", "blank", "my-header-command \"test\""),
257-
SSHTest(listOf("header"), null, "header-command-windows", "blank", """C:\Program Files\My Header Command\"also has quotes"\HeaderCommand.exe"""),
255+
if (getOS() == OS.WINDOWS) {
256+
SSHTest(listOf("header"), null, "header-command-windows", "blank", """"C:\Program Files\My Header Command\HeaderCommand.exe" --url="%CODER_URL%" --test="foo bar"""")
257+
} else {
258+
SSHTest(listOf("header"), null, "header-command", "blank", "my-header-command --url=\"\$CODER_URL\" --test=\"foo bar\" --literal='\$CODER_URL'")
259+
}
258260
)
259261

260262
val newlineRe = "\r?\n".toRegex()

‎src/test/kotlin/com/coder/gateway/util/EscapeTest.kt‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,22 @@ internal class EscapeTest {
1919
assertEquals(it.value, escape(it.key))
2020
}
2121
}
22+
23+
@Test
24+
fun testEscapeSubcommand() {
25+
val tests = if (getOS() == OS.WINDOWS) {
26+
mapOf(
27+
"auth.exe --url=%CODER_URL%" to "\"auth.exe --url=%%CODER_URL%%\"",
28+
"\"my auth.exe\" --url=%CODER_URL%" to "\"\\\"my auth.exe\\\" --url=%%CODER_URL%%\"",
29+
)
30+
} else {
31+
mapOf(
32+
"auth --url=\$CODER_URL" to "'auth --url=\$CODER_URL'",
33+
"'my auth program' --url=\$CODER_URL" to "''\\''my auth program'\\'' --url=\$CODER_URL'",
34+
)
35+
}
36+
tests.forEach {
37+
assertEquals(it.value, escapeSubcommand(it.key))
38+
}
39+
}
2240
}

0 commit comments

Comments
 (0)