Skip to content

Commit 81a659c

Browse files
authored
Merge commit from fork
* fix: prevent workspace identifiers from injecting SSH config Validate deployment-provided owner, workspace, and agent names before they reach CLI or SSH configuration sinks. Keep the validation regex slightly looser than initially recommended to match backend support for uppercase names and legacy agent underscores. Centralize workspace address construction and ProxyCommand quoting, escape SSH percent expansions, and add operand separators before workspace targets. Reject unsafe poll responses before publishing environments, scope managed SSH blocks by deployment origin, and safely migrate legacy blocks while ignoring injected early end markers. * fix: filter unsafe workspace and agent identifiers Initially I did a fast return when the json marshaller decoded invalid strings in the ws and agent name. The approach caused a regression because valid ws should still be rendered. * fix: restore workspace and agent terminology Revert accidental wsName and agentName substitutions in user-facing text, documentation, tests, and the workspace URI parameter. * refactor: improve namings and simplify usage * chore: fix method doc and test name * fix: report skipped workspaces directly from the rest client * chore: update CHANGELOG.md * chore: next version is 0.9.2 * fix: expand ~ and $HOME in ssh log dir and network info dir settings ProxyCommandBuilder now treats every argument as literal argv, so the SSH shell no longer expands these paths. Since binary and data directories already expand at the settings boundary, do the same for sshLogDirectory and networkInfoDir instead of relying on shell interpretation that no longer happens. * test: cover Windows backslash-before-quote escaping in ProxyCommandBuilder Demonstrates that a backslash immediately preceding an embedded or closing quote is doubled so it escapes itself rather than the quote, per review feedback on the Windows argument quoting logic.
1 parent 59c285b commit 81a659c

49 files changed

Lines changed: 950 additions & 216 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
### Fixed
66

77
- keep workspace polling alive when SSH config updates fail
8+
- improved validation and handling of workspace connection data
9+
- expand `~` and `$HOME` in the SSH log directory and network info directory settings, consistent with how the data
10+
and binary directories are already resolved
811

912
## 0.9.2-alpha2 - 2026-07-24
1013

‎gradle.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
version=0.9.2-alpha2
1+
version=0.9.2
22
group=com.coder.toolbox
33
name=coder-toolbox

‎src/main/kotlin/com/coder/toolbox/CoderRemoteEnvironment.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package com.coder.toolbox
33
import com.coder.toolbox.browser.browse
44
import com.coder.toolbox.cli.CoderCLIManager
55
import com.coder.toolbox.cli.SshCommandProcessHandle
6+
import com.coder.toolbox.cli.WorkspaceAddress
67
import com.coder.toolbox.models.WorkspaceAndAgentStatus
78
import com.coder.toolbox.sdk.CoderRestClient
89
import com.coder.toolbox.sdk.ex.APIResponseException
@@ -85,7 +86,7 @@ class CoderRemoteEnvironment(
8586
refreshAvailableActions()
8687
}
8788

88-
fun toWorkspaceAgentPairOrNull(): Pair<Workspace, WorkspaceAgent>? = agent?.let { Pair(workspace, it) }
89+
internal fun toWorkspaceAddressOrNull(): WorkspaceAddress? = agent?.let { WorkspaceAddress.from(workspace, it) }
8990

9091
private fun refreshAvailableActions() {
9192
val actions = mutableListOf<ActionDescription>()
@@ -135,7 +136,7 @@ class CoderRemoteEnvironment(
135136
context.logger.debug("Starting $id... ")
136137
context.cs
137138
.launch(CoroutineName("Start Workspace Action CLI Runner") + Dispatchers.IO) {
138-
cli.startWorkspace(workspace.ownerName, workspace.name)
139+
cli.startWorkspace(WorkspaceAddress.from(workspace))
139140
workspaceRefreshTrigger.trySend(true)
140141
}
141142
// cli takes 15 seconds to move the workspace in queueing/starting state

‎src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import com.coder.toolbox.plugin.PluginManager
88
import com.coder.toolbox.sdk.CoderRestClient
99
import com.coder.toolbox.sdk.ex.APIResponseException
1010
import com.coder.toolbox.sdk.ex.OAuthTokenResponseException
11+
import com.coder.toolbox.sdk.v2.models.InvalidCoderIdentifierException
1112
import com.coder.toolbox.sdk.v2.models.WorkspaceStatus
1213
import com.coder.toolbox.util.CoderProtocolHandler
1314
import com.coder.toolbox.util.DialogUi
@@ -212,9 +213,13 @@ class CoderRemoteProvider(
212213
*/
213214
private fun configureSsh(cli: CoderCLIManager, resolvedEnvironments: List<CoderRemoteEnvironment>) {
214215
try {
215-
cli.configSsh(resolvedEnvironments.mapNotNull { it.toWorkspaceAgentPairOrNull() }.toSet())
216+
cli.configSsh(resolvedEnvironments.mapNotNull { it.toWorkspaceAddressOrNull() }.toSet())
216217
isSshConfigurationWarningShown = false
217218
} catch (ex: Exception) {
219+
// Identifier failures are security boundary violations, not recoverable file-system errors.
220+
// Let the outer poll handler reject the response before it publishes the environments.
221+
if (ex.hasInvalidCoderIdentifierCause()) throw ex
222+
218223
if (!isSshConfigurationWarningShown) {
219224
isSshConfigurationWarningShown = true
220225
val reason = ex.message?.takeIf { it.isNotBlank() } ?: ex.javaClass.simpleName
@@ -748,3 +753,6 @@ class CoderRemoteProvider(
748753
}
749754
}
750755
}
756+
757+
private fun Throwable.hasInvalidCoderIdentifierCause(): Boolean =
758+
this is InvalidCoderIdentifierException || cause is InvalidCoderIdentifierException

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

Lines changed: 71 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,9 @@ import com.coder.toolbox.cli.gpg.VerificationResult
1313
import com.coder.toolbox.cli.gpg.VerificationResult.Failed
1414
import com.coder.toolbox.cli.gpg.VerificationResult.Invalid
1515
import com.coder.toolbox.sdk.CoderHttpClientBuilder
16-
import com.coder.toolbox.sdk.v2.models.Workspace
17-
import com.coder.toolbox.sdk.v2.models.WorkspaceAgent
1816
import com.coder.toolbox.settings.SignatureFallbackStrategy.ALLOW
1917
import com.coder.toolbox.util.InvalidVersionException
2018
import com.coder.toolbox.util.SemVer
21-
import com.coder.toolbox.util.escape
22-
import com.coder.toolbox.util.escapeSubcommand
2319
import com.coder.toolbox.util.safeHost
2420
import com.squareup.moshi.Json
2521
import com.squareup.moshi.JsonClass
@@ -277,18 +273,19 @@ class CoderCLIManager(
277273
/**
278274
* Start a workspace. Throws if the command execution fails.
279275
*/
280-
fun startWorkspace(workspaceOwner: String, workspaceName: String, feats: Features = features): String {
276+
internal fun startWorkspace(wsAddress: WorkspaceAddress, feats: Features = features): String {
281277
val args = mutableListOf(
282278
"--global-config",
283279
coderConfigPath.toString(),
284280
"start",
285281
"--yes",
286-
"$workspaceOwner/$workspaceName"
287282
)
288283

289284
if (feats.buildReason) {
290285
args.addAll(listOf("--reason", "jetbrains_connection"))
291286
}
287+
args.add("--")
288+
args.add(wsAddress.ownerAndWsName)
292289

293290
return exec(*args.toTypedArray())
294291
}
@@ -298,12 +295,12 @@ class CoderCLIManager(
298295
*
299296
* This can take supported features for testing purposes only.
300297
*/
301-
fun configSsh(
302-
wsWithAgents: Set<Pair<Workspace, WorkspaceAgent>>,
298+
internal fun configSsh(
299+
workspaceAddresses: Set<WorkspaceAddress>,
303300
feats: Features = features,
304301
) {
305302
context.logger.info("Configuring SSH config at ${context.settingsStore.sshConfigPath}")
306-
writeSSHConfig(modifySSHConfig(readSSHConfig(), wsWithAgents, feats))
303+
writeSSHConfig(modifySSHConfig(readSSHConfig(), workspaceAddresses, feats))
307304
context.logger.info("Finished configuring SSH config")
308305
}
309306

@@ -326,34 +323,40 @@ class CoderCLIManager(
326323
*/
327324
private fun modifySSHConfig(
328325
contents: String?,
329-
wsWithAgents: Set<Pair<Workspace, WorkspaceAgent>>,
326+
workspaceAddresses: Set<WorkspaceAddress>,
330327
feats: Features,
331328
): String? {
332329
val host = deploymentURL.safeHost()
333330
val startBlock = "# --- START CODER JETBRAINS TOOLBOX $host"
334331
val endBlock = "# --- END CODER JETBRAINS TOOLBOX $host"
335-
val isRemoving = wsWithAgents.isEmpty()
332+
val isRemoving = workspaceAddresses.isEmpty()
336333
val baseArgs =
337334
listOfNotNull(
338-
escape(localBinaryPath.toString()),
335+
localBinaryPath.toString(),
339336
"--global-config",
340-
escape(coderConfigPath.toString()),
337+
coderConfigPath.toString(),
341338
// CODER_URL might be set, and it will override the URL file in
342339
// the config directory, so override that here to make sure we
343340
// always use the correct URL.
344341
"--url",
345-
escape(deploymentURL.toString()),
342+
deploymentURL.toString(),
346343
context.settingsStore.headerCommand?.takeIf { it.isNotBlank() }?.let { "--header-command" },
347-
context.settingsStore.headerCommand?.takeIf { it.isNotBlank() }?.let { escapeSubcommand(it) },
344+
context.settingsStore.headerCommand?.takeIf { it.isNotBlank() },
348345
"ssh",
349346
"--stdio",
350347
if (context.settingsStore.disableAutostart && feats.disableAutostart) "--disable-autostart" else null,
351-
"--network-info-dir ${escape(context.settingsStore.networkInfoDir)}"
348+
"--network-info-dir",
349+
context.settingsStore.networkInfoDir,
352350
)
353-
val proxyArgs = baseArgs + listOfNotNull(
354-
context.settingsStore.sshLogDirectory?.takeIf { it.isNotBlank() }?.let { "--log-dir ${escape(it)} -v" },
355-
if (feats.reportWorkspaceUsage) "--usage-app=jetbrains" else null,
356-
)
351+
val proxyArgs = buildList {
352+
addAll(baseArgs)
353+
context.settingsStore.sshLogDirectory?.takeIf { it.isNotBlank() }?.let {
354+
add("--log-dir")
355+
add(it)
356+
add("-v")
357+
}
358+
if (feats.reportWorkspaceUsage) add("--usage-app=jetbrains")
359+
}
357360
val extraConfig = context.settingsStore.sshConfigOptions
358361
?.takeIf { it.isNotBlank() }
359362
?.let { "\n" + it.prependIndent(" ") }
@@ -367,25 +370,38 @@ class CoderCLIManager(
367370
""".trimIndent()
368371

369372
val blockContent = if (context.settingsStore.isSshWildcardConfigEnabled && feats.wildcardSsh) {
373+
val hostnamePrefix = WorkspaceAddress.wildcardSshHostPrefix(deploymentURL.safeHost())
374+
val proxyCommand = ProxyCommandBuilder()
375+
.arguments(proxyArgs)
376+
.argument("--ssh-host-prefix")
377+
.argument("$hostnamePrefix--")
378+
.argument("--")
379+
.sshToken("%h")
380+
.render()
370381
startBlock + System.lineSeparator() +
371382
"""
372-
Host ${getHostnamePrefix(deploymentURL)}--*
373-
ProxyCommand ${proxyArgs.joinToString(" ")} --ssh-host-prefix ${getHostnamePrefix(deploymentURL)}-- %h
383+
Host $hostnamePrefix--*
384+
ProxyCommand $proxyCommand
374385
""".trimIndent()
375386
.plus("\n" + options.prependIndent(" "))
376387
.plus(extraConfig)
377388
.plus("\n")
378389
.replace("\n", System.lineSeparator()) +
379390
System.lineSeparator() + endBlock
380391
} else {
381-
wsWithAgents.joinToString(
392+
workspaceAddresses.joinToString(
382393
System.lineSeparator(),
383394
startBlock + System.lineSeparator(),
384395
System.lineSeparator() + endBlock,
385-
transform = {
396+
transform = { workspaceAddress ->
397+
val proxyCommand = ProxyCommandBuilder()
398+
.arguments(proxyArgs)
399+
.argument("--")
400+
.argument(workspaceAddress.ownerWsAndAgentName)
401+
.render()
386402
"""
387-
Host ${getHostname(deploymentURL, it.workspace(), it.agent())}
388-
ProxyCommand ${proxyArgs.joinToString(" ")} ${getWsByOwner(it.workspace(), it.agent())}
403+
Host ${getHostname(deploymentURL, workspaceAddress)}
404+
ProxyCommand $proxyCommand
389405
""".trimIndent()
390406
.plus("\n" + options.prependIndent(" "))
391407
.plus(extraConfig)
@@ -400,15 +416,14 @@ class CoderCLIManager(
400416
return blockContent + System.lineSeparator()
401417
}
402418

403-
val start = "(\\s*)$startBlock".toRegex().find(contents)
404-
val end = "$endBlock(\\s*)".toRegex().find(contents)
419+
val managedBlock = findManagedBlock(contents, startBlock, endBlock)
405420

406-
if (start == null && end == null && isRemoving) {
421+
if (managedBlock == null && isRemoving) {
407422
context.logger.info("No workspaces and no existing config blocks to remove")
408423
return null
409424
}
410425

411-
if (start == null && end == null) {
426+
if (managedBlock == null) {
412427
context.logger.info("Appending config block")
413428
val toAppend =
414429
if (contents.isEmpty()) {
@@ -422,15 +437,7 @@ class CoderCLIManager(
422437
return toAppend + System.lineSeparator()
423438
}
424439

425-
if (start == null) {
426-
throw SSHConfigFormatException("End block exists but no start block")
427-
}
428-
if (end == null) {
429-
throw SSHConfigFormatException("Start block exists but no end block")
430-
}
431-
if (start.range.first > end.range.first) {
432-
throw SSHConfigFormatException("Start block found after end block")
433-
}
440+
val (start, end) = managedBlock
434441

435442
if (isRemoving) {
436443
context.logger.info("No workspaces; removing config block")
@@ -454,6 +461,26 @@ class CoderCLIManager(
454461
).joinToString("")
455462
}
456463

464+
/**
465+
* Locate a managed block, using the final end marker so a rewrite also
466+
* removes content following an end marker injected by a vulnerable version.
467+
*/
468+
private fun findManagedBlock(contents: String, startMarker: String, endMarker: String): ManagedBlock? {
469+
val start = "(\\s*)${Regex.escape(startMarker)}".toRegex().find(contents)
470+
val allEnds = "${Regex.escape(endMarker)}(\\s*)".toRegex().findAll(contents).toList()
471+
472+
if (start == null && allEnds.isEmpty()) return null
473+
if (start == null) throw SSHConfigFormatException("End block exists but no start block")
474+
475+
val end = allEnds.lastOrNull { it.range.first > start.range.first }
476+
?: if (allEnds.isEmpty()) {
477+
throw SSHConfigFormatException("Start block exists but no end block")
478+
} else {
479+
throw SSHConfigFormatException("Start block found after end block")
480+
}
481+
return ManagedBlock(start, end)
482+
}
483+
457484
/**
458485
* Write the provided SSH config or do nothing if null.
459486
*/
@@ -563,24 +590,17 @@ class CoderCLIManager(
563590
}
564591
}
565592

566-
fun getHostname(url: URL, ws: Workspace, agent: WorkspaceAgent): String {
593+
internal fun getHostname(url: URL, workspaceAddress: WorkspaceAddress): String {
567594
return if (context.settingsStore.isSshWildcardConfigEnabled && features.wildcardSsh) {
568-
"${getHostnamePrefix(url)}--${ws.ownerName}--${ws.name}.${agent.name}"
595+
workspaceAddress.wildcardSshHostAlias(url.safeHost())
569596
} else {
570-
"coder-jetbrains-toolbox--${ws.ownerName}--${ws.name}.${agent.name}--${url.safeHost()}"
597+
workspaceAddress.sshHostAlias(url.safeHost())
571598
}
572599
}
573600

574601
companion object {
575-
private val tokenRegex = "--token [^ ]+".toRegex()
602+
private data class ManagedBlock(val start: MatchResult, val end: MatchResult)
576603

577-
private fun getHostnamePrefix(url: URL): String = "coder-jetbrains-toolbox-${url.safeHost()}"
578-
579-
private fun getWsByOwner(ws: Workspace, agent: WorkspaceAgent): String =
580-
"${ws.ownerName}/${ws.name}.${agent.name}"
581-
582-
private fun Pair<Workspace, WorkspaceAgent>.workspace() = this.first
583-
584-
private fun Pair<Workspace, WorkspaceAgent>.agent() = this.second
604+
private val tokenRegex = "--token [^ ]+".toRegex()
585605
}
586606
}

0 commit comments

Comments
 (0)