diff --git a/CHANGELOG.md b/CHANGELOG.md index 50ac8107..59c8b32a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ ### Changed +- store CLI session tokens in the OS keyring by default on supported platforms; set `useKeyring` to `false` to opt out +- share the same session token between REST and CLI authentication, passing it through the environment and sanitizing logs +- remove stored credentials on explicit logout, and remove old plaintext session files after successful keyring login +- bound credential and header commands to 60 seconds while preserving unbounded workspace starts and complete progress lines + - upgraded the Toolbox plugin API, dropping support for Toolbox versions older than 3.7.2 - display live workspace start output from the Coder CLI and poll build status and provisioner output while updating, restarting, or stopping workspaces diff --git a/README.md b/README.md index 75dca6c0..0ba43fdf 100644 --- a/README.md +++ b/README.md @@ -540,13 +540,18 @@ explicit entry per resolved workspace/agent using as-is) or a base directory (the CLI is placed under a host-specific subdirectory). If blank, the data directory is used. Supports `~` and `$HOME` expansion. -- `Data directory` directory where deployment-specific data such as session tokens and CLI binaries are stored. Each - deployment gets a host-specific subdirectory (e.g. `coder.example.com`). Supports `~` and `$HOME` - expansion. +- `Data directory` directory where deployment-specific data such as session tokens and CLI binaries + are stored. Each deployment gets a host-specific subdirectory (e.g. `coder.example.com`). Supports `~` and `$HOME` + expansion. When keyring-backed CLI storage is enabled, the session token is no longer persisted in this directory. - `Header command` command that outputs additional HTTP headers. Each line of output must be in the format key=value. When this setting is left blank, the `CODER_HEADER_COMMAND` environment variable is used instead, if set. +- `Store CLI session in OS keyring when supported (CLI >= 2.29.0)` is enabled by default on macOS and Windows. + An unset or `true` `useKeyring` setting uses the OS keyring; `false` opts out and stores the CLI session in the + plugin's deployment-specific data directory. Linux and older CLIs use file storage. Changes apply on the next + sign-in or plugin restart. + - `lastDeploymentURL` the last Coder deployment URL that Coder Toolbox successfully authenticated to. - `workspaceViewUrl` specifies the dashboard page full URL where users can view details about a workspace. Helpful for @@ -576,6 +581,40 @@ Once the binary location is resolved: 3. If **downloads are disabled** and the CLI exists but its version does not match, the stale CLI is used with a warning. If no CLI exists at all, an error is raised. +#### How keyring-backed CLI login works + +Toolbox passes session tokens to `coder login` through `CODER_SESSION_TOKEN` and uses `--use-token-as-session` so +REST requests and CLI commands share the same token. The token is not passed in command-line arguments. + +On macOS and Windows with Coder CLI `2.29.0` or newer, Toolbox enables the OS keyring by default. It passes +`--use-keyring=true` and the deployment URL, and omits the plugin-specific `--global-config` directory. After a +successful keyring login, Toolbox removes any old plaintext session file from that deployment's plugin data directory. +A keyring write failure is reported as a login failure; Toolbox does not retry the write using file storage. + +To opt out, uncheck **Store CLI session in OS keyring when supported (CLI >= 2.29.0)** or set `useKeyring` to `false`. +Toolbox then uses its deployment-specific `--global-config` directory and passes `--use-keyring=false` to CLIs that +support the flag. Linux also uses this file storage. On older CLIs, Toolbox omits the unsupported flag and warns at +login on macOS and Windows when the requested keyring storage is unavailable. Certificate-based authentication +continues to use the plugin-specific CLI configuration without storing a session token. + +Saving the setting does not change the current session's credential backend or rewrite its SSH commands to use a +new backend. The change takes effect on the next sign-in or plugin restart, when Toolbox stores the token and +regenerates SSH configuration. Workspace starts, SSH connections, support bundles, token refreshes, and logout use +the active session's storage choice consistently. + +The OS keyring entry is shared with the Coder CLI and VS Code extension for the same deployment. Explicitly logging +out of Toolbox runs `coder logout --yes`, which revokes the session and removes its stored credential, and clears +Toolbox's saved API token and OAuth credentials. Other clients using that shared session will need to sign in again. +If CLI cleanup fails, Toolbox still signs out and displays a warning. Closing the plugin alone preserves credentials. + +Credential commands and header commands have a 60-second timeout. Workspace starts and SSH sessions are not subject +to that limit. Timeout and cancellation cleanup is best-effort for child processes: a command that forks and exits +before its children are observed can leave orphan processes running. Workspace start progress is reported one +complete line at a time, including carriage-return updates. + +Toolbox retains its own credentials in the Toolbox secret store for automatic sign-in. Importing a token created by +an independent terminal login is tracked separately in DEVEX-403. + ### TLS settings The following options control the secure communication behavior of the plugin with Coder deployment and its available @@ -628,6 +667,10 @@ support, may trigger regeneration of SSH configurations. > [!IMPORTANT] > Token authentication is required when TLS certificates are not configured. +When the plugin logs the Coder CLI in with a session token, it passes that token through the +`CODER_SESSION_TOKEN` environment variable instead of `--token`. This reduces the chances of the token showing up in +process listings, shell history, or command-line audit logs. + ## Releasing 1. Check that the changelog lists all the important changes. diff --git a/build.gradle.kts b/build.gradle.kts index 4b20c668..80a2fdda 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -38,7 +38,6 @@ dependencies { compileOnly(libs.bundles.serialization) compileOnly(libs.coroutines.core) implementation(libs.okhttp) - implementation(libs.exec) implementation(libs.moshi) ksp(libs.moshi.codegen) implementation(libs.retrofit) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 4b1ae565..c2230798 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -7,7 +7,6 @@ okhttp = "4.12.0" dependency-license-report = "3.1.4" marketplace-client = "2.0.51" gradle-wrapper = "0.16.0" -exec = "1.13.0" moshi = "1.15.2" ksp = "2.3.6" retrofit = "3.0.0" @@ -29,7 +28,6 @@ serialization-core = { module = "org.jetbrains.kotlinx:kotlinx-serialization-cor serialization-json = { module = "org.jetbrains.kotlinx:kotlinx-serialization-json", version.ref = "serialization" } serialization-json-okio = { module = "org.jetbrains.kotlinx:kotlinx-serialization-json-okio", version.ref = "serialization" } okhttp = { module = "com.squareup.okhttp3:okhttp", version.ref = "okhttp" } -exec = { module = "org.zeroturnaround:zt-exec", version.ref = "exec" } moshi = { module = "com.squareup.moshi:moshi", version.ref = "moshi" } moshi-codegen = { module = "com.squareup.moshi:moshi-kotlin-codegen", version.ref = "moshi" } retrofit = { module = "com.squareup.retrofit2:retrofit", version.ref = "retrofit" } diff --git a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt index 056e4e73..e1cd454f 100644 --- a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt +++ b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt @@ -46,6 +46,7 @@ import com.jetbrains.toolbox.api.ui.components.UiPage import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -115,8 +116,10 @@ class CoderRemoteProvider( private val linkHandler = CoderProtocolHandler(context, IdeFeedManager(context), workspaceRefreshTrigger, environments) private val accountDropdownField = dropDownFactory(context.i18n.pnotr("")) { - logout() - context.envPageManager.showPluginEnvironmentsPage(false) + context.cs.launch(CoroutineName("Logout")) { + logout() + context.envPageManager.showPluginEnvironmentsPage(false) + } }.apply { visibility.update { false } } @@ -350,15 +353,32 @@ class CoderRemoteProvider( }.sortedBy { it.id } } - /** - * Stop polling, clear the client and environments, then go back to the - * first page. - */ - private fun logout() { + /** Sign out explicitly; provider shutdown alone must preserve credentials. */ + internal suspend fun logout() { val sessionIds = lastEnvironments.currentSessionIds() - context.logger.info(sessionIds, "Logging out ${client?.me?.username}...") - close() - context.logger.info(sessionIds, "User ${client?.me?.username} logged out successfully") + val activeClient = client + val activeCli = cli + context.logger.info(sessionIds, "Logging out ${activeClient?.me?.username}...") + try { + pollJob?.cancelAndJoin() + activeClient?.close() + if (activeClient != null) { + context.secrets.clearSessionFor(activeClient.url) + if (activeCli?.usesTokenAuth == true) activeCli.logout() + } + } catch (ex: CancellationException) { + throw ex + } catch (ex: Exception) { + context.logger.logAndShowWarning( + sessionIds, + "CLI logout failed", + "Toolbox has signed out, but the CLI credential may remain stored. " + + "Sign in and try logging out again to remove it.", + ex, + ) + } finally { + close() + } } /** diff --git a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt index 240ac88e..e11b7f1a 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt @@ -16,8 +16,12 @@ import com.coder.toolbox.sdk.CoderHttpClientBuilder import com.coder.toolbox.session.SessionId import com.coder.toolbox.settings.SignatureFallbackStrategy.ALLOW import com.coder.toolbox.util.InvalidVersionException +import com.coder.toolbox.util.OS import com.coder.toolbox.util.SemVer +import com.coder.toolbox.util.getOS +import com.coder.toolbox.util.runProcess import com.coder.toolbox.util.safeHost +import com.coder.toolbox.util.sanitizeSecrets import com.squareup.moshi.Json import com.squareup.moshi.JsonClass import com.squareup.moshi.JsonDataException @@ -25,7 +29,6 @@ import com.squareup.moshi.Moshi import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.runInterruptible import kotlinx.coroutines.withContext -import org.zeroturnaround.exec.ProcessExecutor import retrofit2.Retrofit import java.io.EOFException import java.io.FileNotFoundException @@ -102,6 +105,7 @@ data class Features( val reportWorkspaceUsage: Boolean = false, val wildcardSsh: Boolean = false, val buildReason: Boolean = false, + val keyringAuth: Boolean = false, ) /** @@ -110,8 +114,14 @@ data class Features( class CoderCLIManager( private val context: CoderToolboxContext, // The URL of the deployment this CLI is for. - private val deploymentURL: URL + private val deploymentURL: URL, + private val currentOs: OS? = getOS(), ) { + internal val usesTokenAuth = context.settingsStore.requiresTokenAuth + + // Keep credential storage consistent with existing SSH commands until the next sign-in. + private val useKeyring = context.settingsStore.useKeyring && usesTokenAuth + private var keyringFallbackWarningShown = false private val downloader = createDownloadService() private val gpgVerifier = GPGVerifier(context) @@ -257,19 +267,34 @@ class CoderCLIManager( } } - /** - * Use the provided token to initializeSession the CLI. - */ - fun login(token: String): String { - context.logger.info("Storing CLI credentials in $coderConfigPath") - return exec( - "login", - deploymentURL.toString(), - "--token", - token, - "--global-config", - coderConfigPath.toString(), - ) + /** Persist the same token used by the REST client, without exposing it in process arguments. */ + suspend fun login(token: String, feats: Features = features): String { + if (useKeyring && supportsKeyringStorage(currentOs) && !feats.keyringAuth && !keyringFallbackWarningShown) { + keyringFallbackWarningShown = true + context.logger.logAndShowWarning( + "Keyring storage unavailable", + "Coder CLI 2.29.0 or newer is required for OS keyring storage. " + + "The CLI session will be stored in the plugin's data directory.", + ) + } + return runInterruptible(Dispatchers.IO) { + val output = exec( + *workspaceAuthArgs(feats).toTypedArray(), + "login", "--use-token-as-session", deploymentURL.toString(), + env = mapOf(CODER_SESSION_TOKEN_ENV_VAR to token), + timeoutMillis = CREDENTIAL_TIMEOUT_MILLIS, + ) + if (shouldUseKeyringAuth(feats)) { + // Remove the plaintext copy only after the keyring write succeeds. + Files.deleteIfExists(coderConfigPath.resolve("session")) + } + output + } + } + + /** Revoke the CLI session and remove its persisted credential. The caller handles failures. */ + internal suspend fun logout(feats: Features = features) = runInterruptible(Dispatchers.IO) { + exec(*workspaceAuthArgs(feats).toTypedArray(), "logout", "--yes", timeoutMillis = CREDENTIAL_TIMEOUT_MILLIS) } /** @@ -281,8 +306,7 @@ class CoderCLIManager( showTextProgress: (String) -> Unit = {}, ): String { val args = mutableListOf( - "--global-config", - coderConfigPath.toString(), + *workspaceAuthArgs(feats).toTypedArray(), "start", "--yes", ) @@ -348,13 +372,7 @@ class CoderCLIManager( val baseArgs = listOfNotNull( localBinaryPath.toString(), - "--global-config", - coderConfigPath.toString(), - // CODER_URL might be set, and it will override the URL file in - // the config directory, so override that here to make sure we - // always use the correct URL. - "--url", - deploymentURL.toString(), + *workspaceAuthArgs(feats).toTypedArray(), context.settingsStore.headerCommand?.takeIf { it.isNotBlank() }?.let { "--header-command" }, context.settingsStore.headerCommand?.takeIf { it.isNotBlank() }, "ssh", @@ -519,7 +537,7 @@ class CoderCLIManager( * Throws if it could not be determined. */ fun version(): SemVer { - val raw = exec("version", "--output", "json") + val raw = exec("version", "--output", "json", timeoutMillis = CREDENTIAL_TIMEOUT_MILLIS) try { val json = Moshi.Builder().build().adapter(Version::class.java).fromJson(raw) if (json?.version == null || json.version.isBlank()) { @@ -578,25 +596,21 @@ class CoderCLIManager( private fun exec( vararg args: String, + env: Map = emptyMap(), + timeoutMillis: Long? = null, showTextProgress: ((String) -> Unit)? = null, ): String { - val processExecutor = - ProcessExecutor() - .command(localBinaryPath.toString(), *args) - .environment("CODER_HEADER_COMMAND", context.settingsStore.headerCommand) - .exitValues(0) - - showTextProgress?.let { reportProgress -> - processExecutor.redirectOutput(reportProgress::invoke) + val command = listOf(localBinaryPath.toString(), *args) + val processEnv = buildMap { + context.settingsStore.headerCommand?.let { put("CODER_HEADER_COMMAND", it) } + putAll(env) } - - val stdout = - processExecutor - .readOutput(true) - .execute() - .outputUTF8() - val redactedArgs = listOf(*args).joinToString(" ").replace(tokenRegex, "--token ") - context.logger.info("`$localBinaryPath $redactedArgs`: $stdout") + val stdout = runProcess( + command, environment = processEnv, timeoutMillis = timeoutMillis, onOutputLine = showTextProgress, + ).stdout + val sanitizedArgs = listOf(*args).joinToString(" ").sanitizeSecrets() + val sanitizedStdout = stdout.sanitizeSecrets(env[CODER_SESSION_TOKEN_ENV_VAR]) + context.logger.info("`$localBinaryPath $sanitizedArgs`: $sanitizedStdout") return stdout } @@ -604,8 +618,7 @@ class CoderCLIManager( internal suspend fun supportBundle(address: WorkspaceAddress, outputFile: Path) { val command = listOfNotNull( localBinaryPath.toString(), - "--global-config", coderConfigPath.toString(), - "--url", deploymentURL.toString(), + *workspaceAuthArgs(features).toTypedArray(), "support", "bundle", "--yes", "--output-file", outputFile.toAbsolutePath().toString(), "--", address.ownerAndWsName, address.agentName, ) @@ -646,6 +659,7 @@ class CoderCLIManager( reportWorkspaceUsage = version >= SemVer(2, 13, 0), wildcardSsh = version >= SemVer(2, 19, 0), buildReason = version >= SemVer(2, 25, 0), + keyringAuth = version >= SemVer(2, 29, 0), ) } } @@ -661,6 +675,22 @@ class CoderCLIManager( companion object { private data class ManagedBlock(val start: MatchResult, val end: MatchResult) - private val tokenRegex = "--token [^ ]+".toRegex() + internal const val CREDENTIAL_TIMEOUT_MILLIS = 60_000L + private const val CODER_SESSION_TOKEN_ENV_VAR = "CODER_SESSION_TOKEN" + + internal fun supportsKeyringStorage(os: OS?): Boolean = os == OS.MAC || os == OS.WINDOWS } + + private fun globalConfigArgs(): List = listOf("--global-config", coderConfigPath.toString()) + + private fun workspaceAuthArgs(feats: Features): List = buildList { + if (!shouldUseKeyringAuth(feats)) addAll(globalConfigArgs()) + // Override inherited CODER_URL and CODER_USE_KEYRING for every authenticated command. + addAll(listOf("--url", deploymentURL.toString())) + if (feats.keyringAuth) add("--use-keyring=${shouldUseKeyringAuth(feats)}") + } + + private fun shouldUseKeyringAuth(feats: Features): Boolean = + useKeyring && feats.keyringAuth && supportsKeyringStorage(currentOs) + } diff --git a/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt b/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt index dc84879d..b70fcc91 100644 --- a/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt +++ b/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt @@ -26,6 +26,7 @@ import com.coder.toolbox.sdk.v2.models.WorkspaceBuild import com.coder.toolbox.sdk.v2.models.WorkspaceBuildReason import com.coder.toolbox.sdk.v2.models.WorkspaceTransition import com.coder.toolbox.util.ReloadableTlsContext +import com.coder.toolbox.util.runProcess import com.coder.toolbox.views.state.CoderOAuthSessionContext import com.coder.toolbox.views.state.hasRefreshToken import com.squareup.moshi.Moshi @@ -34,7 +35,6 @@ import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import okhttp3.OkHttpClient -import org.zeroturnaround.exec.ProcessExecutor import retrofit2.Response import retrofit2.Retrofit import retrofit2.converter.moshi.MoshiConverterFactory @@ -489,18 +489,17 @@ open class CoderRestClient( if (command.isNullOrBlank()) return@withContext false return@withContext try { - val result = ProcessExecutor() - .command(command.split(" ").toList()) - .exitValueAny() - .readOutput(true) - .execute() + val result = runProcess( + command.split(" ").filter { it.isNotBlank() }, + expectedExitCodes = Int.MIN_VALUE..Int.MAX_VALUE, + ) if (tlsContext.reload()) { context.logger.info("Certificate refresh successful. Reloading TLS and evicting pool.") // forces OkHttp to close the broken HTTP/2 connection. httpClient.connectionPool.evictAll() return@withContext true } else { - context.logger.error("Refresh command failed with code ${result.exitValue}") + context.logger.error("Refresh command failed with code ${result.exitCode}") false } } catch (ex: Exception) { diff --git a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt index 80f799f1..c6928e0c 100644 --- a/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt +++ b/src/main/kotlin/com/coder/toolbox/settings/ReadOnlyCoderSettings.kt @@ -70,7 +70,8 @@ interface ReadOnlyCoderSettings { /** * Where to save plugin data like the Coder binary (if not configured with - * binaryDestination) and the deployment URL and session token. + * binaryDestination) and the deployment URL. When keyring storage is not + * enabled, this also stores the CLI session token. */ val dataDirectory: String? @@ -98,6 +99,14 @@ interface ReadOnlyCoderSettings { */ val headerCommand: String? + /** + * Whether CLI login should allow the Coder CLI to persist the session in + * the operating system keyring when supported. This only takes effect on + * macOS and Windows. Defaults to true when unset; false opts out. + * Changes apply on the next sign-in or plugin restart. + */ + val useKeyring: Boolean + /** * Optional TLS settings */ diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt index f45a6dc3..29043b91 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt @@ -28,6 +28,16 @@ class CoderSecretsStore(private val store: PluginSecretStore) { store[url.host] = apiToken } + fun clearSessionFor(url: URL) { + store.clear(url.host) + listOf( + OAUTH_CLIENT_ID_PREFIX, OAUTH_CLIENT_SECRET_PREFIX, OAUTH_REFRESH_TOKEN, + OAUTH_TOKEN_AUTH_METHOD, OAUTH_TOKEN_ENDPOINT + ).forEach { prefix -> + store.clear("$prefix-$url") + } + } + fun oauthSessionFor(url: String): StoredOAuthSession? { val clientId = store["$OAUTH_CLIENT_ID_PREFIX-$url"] val clientSecret = store["$OAUTH_CLIENT_SECRET_PREFIX-$url"] diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt index e91a02bd..83e08323 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSettingsStore.kt @@ -57,6 +57,7 @@ class CoderSettingsStore( override val headerCommand: String? get() = store[HEADER_COMMAND].takeUnless { it.isNullOrEmpty() } ?: env.get(CODER_HEADER_COMMAND).takeUnless { it.isEmpty() } + override val useKeyring: Boolean get() = store[USE_KEYRING]?.toBooleanStrictOrNull() ?: true override val tls: ReadOnlyTLSSettings get() = TLSSettings( certPath = store[TLS_CERT_PATH], @@ -216,6 +217,10 @@ class CoderSettingsStore( store[HEADER_COMMAND] = cmd } + fun updateUseKeyring(useKeyring: Boolean) { + store[USE_KEYRING] = useKeyring.toString() + } + fun updateCertPath(path: String) { store[TLS_CERT_PATH] = path } diff --git a/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt b/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt index 2a95b347..df7d9b95 100644 --- a/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt +++ b/src/main/kotlin/com/coder/toolbox/store/StoreKeys.kt @@ -29,6 +29,8 @@ internal const val ENABLE_DOWNLOADS = "enableDownloads" internal const val HEADER_COMMAND = "headerCommand" +internal const val USE_KEYRING = "useKeyring" + internal const val TLS_CERT_PATH = "tlsCertPath" internal const val TLS_KEY_PATH = "tlsKeyPath" diff --git a/src/main/kotlin/com/coder/toolbox/util/Error.kt b/src/main/kotlin/com/coder/toolbox/util/Error.kt index 752aa074..029b0868 100644 --- a/src/main/kotlin/com/coder/toolbox/util/Error.kt +++ b/src/main/kotlin/com/coder/toolbox/util/Error.kt @@ -2,7 +2,6 @@ package com.coder.toolbox.util import com.coder.toolbox.cli.ex.ResponseException import com.coder.toolbox.sdk.ex.APIResponseException -import org.zeroturnaround.exec.InvalidExitValueException import java.net.ConnectException import java.net.UnknownHostException @@ -17,7 +16,7 @@ fun Throwable.prettify(): String { is FileSystemException -> fileSystemFailed(this.file.toString()) is java.nio.file.FileSystemException -> fileSystemFailed(this.file) is UnknownHostException -> "Unknown host $reason" - is InvalidExitValueException -> "CLI exited unexpectedly with ${this.exitValue}." + is ProcessExitException -> "CLI exited unexpectedly with ${this.result.exitCode}." is APIResponseException -> { if (this.isUnauthorized) { "Authorization failed" diff --git a/src/main/kotlin/com/coder/toolbox/util/Headers.kt b/src/main/kotlin/com/coder/toolbox/util/Headers.kt index 524c363d..baed0c36 100644 --- a/src/main/kotlin/com/coder/toolbox/util/Headers.kt +++ b/src/main/kotlin/com/coder/toolbox/util/Headers.kt @@ -1,7 +1,5 @@ package com.coder.toolbox.util -import org.zeroturnaround.exec.ProcessExecutor -import java.io.OutputStream import java.net.URL private val newlineRegex = "\r?\n".toRegex() @@ -20,16 +18,12 @@ fun getHeaders( else -> Pair("sh", "-c") } val output = - ProcessExecutor() - .command(shell, caller, headerCommand) - .environment("CODER_URL", url.toString()) - // By default stderr is in the output, but we want to ignore it. stderr - // will still be included in the exception if something goes wrong. - .redirectError(OutputStream.nullOutputStream()) - .exitValues(0) - .readOutput(true) - .execute() - .outputUTF8() + runProcess( + listOf(shell, caller, headerCommand), + environment = mapOf("CODER_URL" to url.toString()), + timeoutMillis = 60_000, + stderrMode = ProcessStderrMode.DISCARD_ON_SUCCESS, + ).stdout // The Coder CLI will allow no output, but not blank lines. Possibly we // should skip blank lines, but it is better to have parity so commands will diff --git a/src/main/kotlin/com/coder/toolbox/util/ProcessRunner.kt b/src/main/kotlin/com/coder/toolbox/util/ProcessRunner.kt new file mode 100644 index 00000000..ee0f58a9 --- /dev/null +++ b/src/main/kotlin/com/coder/toolbox/util/ProcessRunner.kt @@ -0,0 +1,230 @@ +package com.coder.toolbox.util + +import java.io.IOException +import java.io.InputStream +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.ExecutionException +import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException +import java.util.concurrent.atomic.AtomicReference +import kotlin.concurrent.thread + +private const val PROCESS_POLL_MILLIS = 100L +private const val PROCESS_CLEANUP_MILLIS = 1000L + +data class ProcessResult( + val command: List, + val exitCode: Int, + val stdout: String, + val stderr: String, +) + +sealed class ProcessRunnerException( + message: String, + cause: Throwable? = null, +) : RuntimeException(message.sanitizeSecrets(), cause) + +enum class ProcessStderrMode { + CAPTURE, + DISCARD_ON_SUCCESS, +} + +class ProcessExecutionException( + message: String, + cause: Throwable? = null +) : ProcessRunnerException(message, cause) + +class ProcessTimeoutException(command: List, timeoutMillis: Long) : + ProcessRunnerException("Process timed out after $timeoutMillis ms: $command") + +class ProcessExitException( + val result: ProcessResult, + private val expectedExitCodes: IntRange, +) : ProcessRunnerException( + buildString { + append("Unexpected exit value: ${result.exitCode}, allowed exit values: $expectedExitCodes") + append(", executed command ${result.command}") + if (result.stdout.isNotBlank()) { + append(", stdout was ${result.stdout.length} bytes:\n${result.stdout}") + } + if (result.stderr.isNotBlank()) { + append(", stderr was ${result.stderr.length} bytes:\n${result.stderr}") + } + }.sanitizeSecrets() +) + +/** + * Runs a non-interactive process, capturing both output streams. Only exit code 0 is accepted by default. + * [timeoutMillis] bounds the wait for a command and its output; cleanup has a bounded grace period. + * Null leaves workspace starts unbounded. Descendant cleanup is best-effort: observed handles are retained, + * but a child that forks and reparents before observation can escape cleanup. + * [onOutputLine] receives complete, sanitized stdout/stderr lines, including carriage-return progress updates + * and a final unterminated line. The captured output retains its original line endings. + * [ProcessStderrMode.DISCARD_ON_SUCCESS] discards stderr only for successful commands. + */ +fun runProcess( + command: List, + environment: Map = emptyMap(), + expectedExitCodes: IntRange = 0..0, + stderrMode: ProcessStderrMode = ProcessStderrMode.CAPTURE, + timeoutMillis: Long? = null, + onOutputLine: ((String) -> Unit)? = null, +): ProcessResult { + require(timeoutMillis == null || timeoutMillis > 0) { "Process timeout must be positive" } + val sessionToken = environment["CODER_SESSION_TOKEN"] + val safeCommand = command.mapIndexed { index, arg -> + if (index > 0 && command[index - 1] == "--token") "" else arg.sanitizeSecrets(sessionToken) + } + val process = try { + ProcessBuilder(command).apply { environment().putAll(environment) }.start() + } catch (ex: IOException) { + throw ProcessExecutionException( + "Failed to start process $safeCommand: ${ex.message.orEmpty().sanitizeSecrets(sessionToken)}" + ) + } + val deadline = timeoutMillis?.let { System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(it) } + val stdout = StringBuilder() + val stderr = StringBuilder() + val readerFailure = AtomicReference() + val callbackLock = Any() + val readers = mutableListOf() + val descendants = ConcurrentHashMap.newKeySet() + var completed = false + + fun readOutput(stream: InputStream, output: StringBuilder, name: String): Thread = + thread(name = name, isDaemon = true) { + try { + captureOutput(stream, output, onOutputLine?.let { report -> + { line -> synchronized(callbackLock) { report(line.sanitizeSecrets(sessionToken)) } } + }) + } catch (ex: Exception) { + readerFailure.compareAndSet(null, ex) + terminateProcesses(process, descendants) + } + } + + try { + // An unexpected prompt must see EOF rather than waiting for input forever. + process.outputStream.close() + readers += readOutput(process.inputStream, stdout, "process-stdout-reader") + readers += readOutput(process.errorStream, stderr, "process-stderr-reader") + val pollNanos = TimeUnit.MILLISECONDS.toNanos(PROCESS_POLL_MILLIS) + while (process.isAlive) { + recordDescendants(process.toHandle(), descendants) + val remaining = deadline?.minus(System.nanoTime()) ?: pollNanos + if (timeoutMillis != null && remaining <= 0) throw ProcessTimeoutException(safeCommand, timeoutMillis) + if (process.waitFor(minOf(pollNanos, remaining), TimeUnit.NANOSECONDS)) break + } + for (reader in readers) { + if (deadline == null) { + reader.join() + } else { + val remaining = deadline - System.nanoTime() + if (remaining > 0) TimeUnit.NANOSECONDS.timedJoin(reader, remaining) + if (reader.isAlive) throw ProcessTimeoutException(safeCommand, timeoutMillis) + } + } + readerFailure.get()?.let { + throw ProcessExecutionException( + "Failed to read output for $safeCommand: ${it.message.orEmpty().sanitizeSecrets(sessionToken)}" + ) + } + val result = ProcessResult(safeCommand, process.exitValue(), stdout.toString(), stderr.toString()) + if (result.exitCode !in expectedExitCodes) { + throw ProcessExitException( + result.copy( + stdout = result.stdout.sanitizeSecrets(sessionToken), + stderr = result.stderr.sanitizeSecrets(sessionToken), + ), + expectedExitCodes, + ) + } + completed = true + return if (stderrMode == ProcessStderrMode.DISCARD_ON_SUCCESS) result.copy(stderr = "") else result + } catch (ex: InterruptedException) { + Thread.currentThread().interrupt() + // runInterruptible translates this to coroutine cancellation after cleanup. + throw ex + } finally { + val interrupted = Thread.interrupted() + try { + if (!completed) { + terminateProcesses(process, descendants) + val cleanupDeadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(PROCESS_CLEANUP_MILLIS) + process.waitFor(PROCESS_CLEANUP_MILLIS, TimeUnit.MILLISECONDS) + awaitDescendants(descendants, cleanupDeadline) + } + readers.forEach { it.join(PROCESS_CLEANUP_MILLIS) } + } catch (_: InterruptedException) { + Thread.currentThread().interrupt() + } finally { + if (interrupted) Thread.currentThread().interrupt() + } + } +} + +private fun recordDescendants(parent: ProcessHandle, observed: MutableSet) { + try { + parent.descendants().use { children -> children.forEach { observed.add(it) } } + } catch (_: RuntimeException) { + // Some environments restrict process enumeration. Cleanup must still stop the direct process. + } +} + +private fun terminateProcesses(process: Process, observed: MutableSet) { + try { + recordDescendants(process.toHandle(), observed) + // Previously observed children remain addressable after reparenting, and can have children of their own. + observed.toList().forEach { recordDescendants(it, observed) } + observed.forEach { + try { + it.destroyForcibly() + } catch (_: RuntimeException) { + // Best-effort cleanup of descendants must not prevent stopping the original process. + } + } + } finally { + process.destroyForcibly() + } +} + +private fun awaitDescendants(observed: Set, deadline: Long) { + for (child in observed) { + val remaining = deadline - System.nanoTime() + if (remaining <= 0) return + try { + child.onExit().get(remaining, TimeUnit.NANOSECONDS) + } catch (_: TimeoutException) { + return + } catch (_: ExecutionException) { + // Process monitoring can fail independently of the termination signal. + } + } +} + +private fun captureOutput(stream: InputStream, output: StringBuilder, onOutputLine: ((String) -> Unit)?) { + stream.bufferedReader(Charsets.UTF_8).use { reader -> + val buffer = CharArray(DEFAULT_BUFFER_SIZE) + val line = StringBuilder() + fun reportLine() { + if (line.isNotEmpty()) { + onOutputLine?.invoke(line.toString()) + line.setLength(0) + } + } + while (true) { + val count = reader.read(buffer) + if (count < 0) break + output.append(buffer, 0, count) + if (onOutputLine != null) { + for (i in 0 until count) { + when (val char = buffer[i]) { + '\r', '\n' -> reportLine() + else -> line.append(char) + } + } + } + } + reportLine() + } +} diff --git a/src/main/kotlin/com/coder/toolbox/util/SecretSanitizer.kt b/src/main/kotlin/com/coder/toolbox/util/SecretSanitizer.kt new file mode 100644 index 00000000..96a2f9a1 --- /dev/null +++ b/src/main/kotlin/com/coder/toolbox/util/SecretSanitizer.kt @@ -0,0 +1,15 @@ +package com.coder.toolbox.util + +private val sensitivePatterns = listOf( + Regex("""(CODER_SESSION_TOKEN=)([^,\s}]+)"""), + Regex("""(Coder-Session-Token\s*[:=]\s*)([^\s,]+)""", RegexOption.IGNORE_CASE), + Regex("""(--token(?:\s+|=))(\S+)"""), + Regex("""([?&]token=)([^&\s]+)""", RegexOption.IGNORE_CASE), +) + +fun String.sanitizeSecrets(sessionToken: String? = null): String { + val redacted = if (sessionToken.isNullOrEmpty()) this else replace(sessionToken, "") + return sensitivePatterns.fold(redacted) { acc, regex -> + acc.replace(regex, "$1") + } +} diff --git a/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt b/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt index 1d830b29..1fddc9c6 100644 --- a/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt +++ b/src/main/kotlin/com/coder/toolbox/views/CoderSettingsPage.kt @@ -5,8 +5,10 @@ import com.coder.toolbox.settings.HttpLoggingVerbosity.BASIC import com.coder.toolbox.settings.HttpLoggingVerbosity.BODY import com.coder.toolbox.settings.HttpLoggingVerbosity.HEADERS import com.coder.toolbox.settings.HttpLoggingVerbosity.NONE +import com.coder.toolbox.util.OS import com.coder.toolbox.util.canCreateDirectory import com.coder.toolbox.util.expand +import com.coder.toolbox.util.getOS import com.jetbrains.toolbox.api.ui.actions.RunnableActionDescription import com.jetbrains.toolbox.api.ui.components.CheckboxField import com.jetbrains.toolbox.api.ui.components.ComboBoxField @@ -43,6 +45,7 @@ class CoderSettingsPage( ) : CoderPage(MutableStateFlow(context.i18n.ptrl("Coder Settings")), false) { private val settings = context.settingsStore.readOnly() + private val shouldShowKeyringField = getOS() == OS.MAC || getOS() == OS.WINDOWS private val preferOAuth2IfAvailableField = CheckboxField( context.settingsStore.preferOAuth2IfAvailable, @@ -85,6 +88,10 @@ class CoderSettingsPage( settings.headerCommand ?: "", TextType.General ) + private val useKeyringField = CheckboxField( + settings.useKeyring, + context.i18n.ptrl("Store CLI session in OS keyring when supported (CLI >= 2.29.0)") + ) private val tlsCertPathField = TextField( context.i18n.ptrl("TLS cert path"), @@ -196,8 +203,9 @@ class CoderSettingsPage( SectionField( "Security & Authentication", false, - listOf( + listOfNotNull( preferOAuth2IfAvailableField, + useKeyringField.takeIf { shouldShowKeyringField }, headerCommandField, tlsCertPathField, tlsKeyPathField, @@ -245,6 +253,8 @@ class CoderSettingsPage( updateSignatureFallbackStrategy(signatureFallbackStrategyField.checkedState.value) updateHttpClientLogLevel(httpLoggingField.selectedValueState.value) updateHeaderCommand(headerCommandField.contentState.value) + // The active CLI keeps its credential backend until the next sign-in. + if (shouldShowKeyringField) updateUseKeyring(useKeyringField.checkedState.value) updatePreferAuthViaOAuth2(preferOAuth2IfAvailableField.checkedState.value) updateCertPath(tlsCertPathField.contentState.value) updateKeyPath(tlsKeyPathField.contentState.value) @@ -306,6 +316,10 @@ class CoderSettingsPage( settings.headerCommand ?: "" } + useKeyringField.checkedState.update { + settings.useKeyring + } + preferOAuth2IfAvailableField.checkedState.update { settings.preferOAuth2IfAvailable } diff --git a/src/main/resources/localization/defaultMessages.po b/src/main/resources/localization/defaultMessages.po index df52c395..3b264228 100644 --- a/src/main/resources/localization/defaultMessages.po +++ b/src/main/resources/localization/defaultMessages.po @@ -85,6 +85,9 @@ msgstr "" msgid "Header command" msgstr "" +msgid "Store CLI session in OS keyring when supported (CLI >= 2.29.0)" +msgstr "" + msgid "TLS cert path" msgstr "" diff --git a/src/test/java/com/coder/toolbox/util/ProcessRunnerTestCommand.java b/src/test/java/com/coder/toolbox/util/ProcessRunnerTestCommand.java new file mode 100644 index 00000000..2f565092 --- /dev/null +++ b/src/test/java/com/coder/toolbox/util/ProcessRunnerTestCommand.java @@ -0,0 +1,34 @@ +package com.coder.toolbox.util; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +/** + * Test-only command that forks a child and can exit before that child. + */ +public final class ProcessRunnerTestCommand { + public static void main(String[] args) throws Exception { + if (args[0].equals("child")) { + Thread.sleep(30_000); + return; + } + String java = Path.of(System.getProperty("java.home"), "bin", + System.getProperty("os.name").startsWith("Windows") ? "java.exe" : "java").toString(); + ProcessBuilder builder = new ProcessBuilder(List.of( + java, "-cp", System.getProperty("java.class.path"), ProcessRunnerTestCommand.class.getName(), "child" + )).inheritIO(); + if (args[0].equals("detached-output")) { + builder.redirectOutput(ProcessBuilder.Redirect.DISCARD).redirectError(ProcessBuilder.Redirect.DISCARD); + } + Process child = builder.start(); + Files.writeString(Path.of(args[1]), ProcessHandle.current().pid() + " " + child.pid()); + System.out.println("ready"); + if (args[0].equals("keep-parent")) { + Thread.sleep(30_000); + } else { + // The test releases the parent only after the output reader has seen its readiness line. + while (!Files.exists(Path.of(args[2]))) Thread.sleep(10); + } + } +} diff --git a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt index 57d14851..c1fad6e8 100644 --- a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt +++ b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt @@ -80,6 +80,55 @@ class CoderRemoteProviderTest { clearAllMocks() } + @Test + fun `explicit logout clears saved credentials and CLI session`() = runTest { + every { mockCli.usesTokenAuth } returns true + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + + remoteProvider.logout() + + verify { mockContext.secrets.clearSessionFor(match { it.toString() == "https://coder.example.com" }) } + coVerify(exactly = 1) { mockCli.logout(any()) } + assertFalse(remoteProvider.getAccountDropDown().visibility.value) + } + + @Test + fun `CLI logout failure still clears Toolbox session and warns`() = runTest { + every { mockCli.usesTokenAuth } returns true + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + coEvery { mockCli.logout(any()) } throws IllegalStateException("keyring unavailable") + + remoteProvider.logout() + + verify { mockContext.secrets.clearSessionFor(match { it.toString() == "https://coder.example.com" }) } + verify { underlyingLogger.warn(any(), match { it.contains("CLI credential may remain") }) } + assertFalse(remoteProvider.getAccountDropDown().visibility.value) + } + + @Test + fun `provider shutdown preserves credentials`() { + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + + remoteProvider.close() + + coVerify(exactly = 0) { mockCli.logout(any()) } + verify(exactly = 0) { mockContext.secrets.clearSessionFor(any()) } + } + + @Test + fun `certificate logout does not remove a shared token session`() = runTest { + every { mockCli.usesTokenAuth } returns false + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + + remoteProvider.logout() + + coVerify(exactly = 0) { mockCli.logout(any()) } + } + @Test fun `given an empty workspace list expect an empty list of environments`() = runTest { // given diff --git a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt index cce5dc20..21e93a93 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt @@ -25,8 +25,11 @@ import com.coder.toolbox.store.NETWORK_INFO_DIR import com.coder.toolbox.store.SSH_CONFIG_OPTIONS import com.coder.toolbox.store.SSH_CONFIG_PATH import com.coder.toolbox.store.SSH_LOG_DIR +import com.coder.toolbox.store.USE_KEYRING import com.coder.toolbox.util.InvalidVersionException import com.coder.toolbox.util.OS +import com.coder.toolbox.util.ProcessExecutionException +import com.coder.toolbox.util.ProcessExitException import com.coder.toolbox.util.SemVer import com.coder.toolbox.util.getOS import com.coder.toolbox.util.pluginTestSettingsStore @@ -62,8 +65,6 @@ import org.junit.jupiter.api.condition.EnabledOnOs import org.junit.jupiter.api.condition.OS.LINUX import org.junit.jupiter.api.condition.OS.MAC import org.junit.jupiter.api.io.TempDir -import org.zeroturnaround.exec.InvalidExitValueException -import org.zeroturnaround.exec.ProcessInitException import java.net.HttpURLConnection import java.net.InetSocketAddress import java.net.Proxy @@ -135,6 +136,7 @@ internal class CoderCLIManagerTest { val binary = root.resolve("coder") binary.toFile().writeText( """#!/bin/sh + |if [ "${'$'}1" = version ]; then printf '{"version":"2.29.0"}'; exit 0; fi |for arg in "${'$'}@"; do | if [ "${'$'}previous" = "--output-file" ]; then output="${'$'}arg"; fi | previous="${'$'}arg" @@ -156,21 +158,26 @@ internal class CoderCLIManagerTest { ) try { val url = "https://coder.example.test".toURL() - val cli = CoderCLIManager(context.copy(settingsStore = settings), url) val ws = workspace("diagnostic-workspace", agents = mapOf("main" to UUID.randomUUID().toString())) val agent = ws.latestBuild.resources.flatMap { it.agents.orEmpty() }.first() - for (selectedAgent in listOf(agent, null)) { - val output = root.resolve("bundle with spaces.zip") - cli.supportBundle(WorkspaceAddress.from(ws, selectedAgent), output) - assertEquals( - listOfNotNull( - "--global-config", cli.coderConfigPath.toString(), "--url", url.toString(), - "support", "bundle", "--yes", "--output-file", output.toString(), - "--", "${ws.ownerName}/${ws.name}", selectedAgent?.name, - ), - output.toFile().readLines(), - ) - assertEquals("custom header", root.resolve("bundle with spaces.zip.header").toFile().readText()) + for (keyring in listOf(true, false)) { + settings.updateUseKeyring(keyring) + val cli = CoderCLIManager(context.copy(settingsStore = settings), url, OS.MAC) + for (selectedAgent in listOf(agent, null)) { + val output = root.resolve("bundle with spaces.zip") + cli.supportBundle(WorkspaceAddress.from(ws, selectedAgent), output) + assertEquals( + listOfNotNull( + if (!keyring) "--global-config" else null, + if (!keyring) cli.coderConfigPath.toString() else null, + "--url", url.toString(), "--use-keyring=$keyring", + "support", "bundle", "--yes", "--output-file", output.toString(), + "--", "${ws.ownerName}/${ws.name}", selectedAgent?.name, + ), + output.toFile().readLines(), + ) + assertEquals("custom header", root.resolve("bundle with spaces.zip.header").toFile().readText()) + } } } finally { root.toFile().deleteRecursively() @@ -187,13 +194,13 @@ internal class CoderCLIManagerTest { mockk(), mockk(), mockk(), - mockk(), + CoroutineScope(Dispatchers.Unconfined), underlyingLogger, mockk(relaxed = true), CoderSettingsStore( pluginTestSettingsStore(), Environment(), - mockk(relaxed = true) + underlyingLogger ), mockk(), object : ToolboxProxySettings { @@ -224,6 +231,14 @@ internal class CoderCLIManagerTest { listOf("#!/bin/sh", str) }.joinToString(System.lineSeparator()) + private fun writeExecutable(path: Path, contents: String) { + path.parent.toFile().mkdirs() + path.toFile().writeText(contents) + if (getOS() != OS.WINDOWS) { + path.toFile().setExecutable(true) + } + } + /** * Return the contents of a script that outputs JSON containing the version. */ @@ -454,8 +469,8 @@ internal class CoderCLIManagerTest { // Make sure login failures propagate. assertFailsWith( - exceptionClass = InvalidExitValueException::class, - block = { ccm.login("jetbrains-ci-test") }, + exceptionClass = ProcessExitException::class, + block = { runBlocking { ccm.login("jetbrains-ci-test") } }, ) } @@ -524,9 +539,314 @@ internal class CoderCLIManagerTest { ) assertFailsWith( - exceptionClass = ProcessInitException::class, - block = { ccm.login("fake-token") }, + exceptionClass = ProcessExecutionException::class, + block = { runBlocking { ccm.login("fake-token") } }, + ) + } + + private fun assertLoginUsesExpectedArgs( + extraSettings: Array>, + expectedArgsSubstring: String, + expectGlobalConfig: Boolean, + feats: Features = Features(), + osOverride: OS? = getOS(), + ) { + val binaryFile = tmpdir.resolve("login-env-${UUID.randomUUID()}") + .resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") + val argsFile = binaryFile.parent.resolve("argv.txt") + val envFile = binaryFile.parent.resolve("env.txt") + val token = "super-secret-token" + val stdout = "login ok" + val script = if (getOS() == OS.WINDOWS) { + mkbin( + """ + echo %* > "${argsFile.toAbsolutePath()}" + echo %CODER_SESSION_TOKEN% > "${envFile.toAbsolutePath()}" + echo $stdout + """.trimIndent() + ) + } else { + mkbin( + """ + printf '%s\n' "$*" > '${argsFile.toAbsolutePath()}' + printf '%s\n' "${'$'}CODER_SESSION_TOKEN" > '${envFile.toAbsolutePath()}' + printf '$stdout\n' + """.trimIndent() + ) + } + writeExecutable(binaryFile, script) + + val settings = CoderSettingsStore( + pluginTestSettingsStore( + BINARY_DESTINATION to binaryFile.toString(), + DATA_DIRECTORY to binaryFile.parent.resolve("data").toString(), + ENABLE_DOWNLOADS to "false", + *extraSettings, + ), + Environment(), + underlyingLogger + ) + val ccm = CoderCLIManager( + context.copy(settingsStore = settings), + URL("https://test.coder.com"), + osOverride, + ) + + assertEquals(stdout, runBlocking { ccm.login(token, feats) }.trim()) + val args = argsFile.toFile().readText() + assertContains(args, expectedArgsSubstring) + assertEquals(expectGlobalConfig, args.contains("--global-config")) + assertContains(args, "--url https://test.coder.com") + if (feats.keyringAuth) { + assertContains(args, "--use-keyring=${!expectGlobalConfig}") + } else { + assertFalse(args.contains("--use-keyring")) + } + assertFalse(args.contains("--token")) + assertFalse(args.contains(token)) + assertEquals(token, envFile.toFile().readText().trim()) + } + + @Test + fun `login selects keyring by default and honors opt out across platforms and CLI versions`() { + for (os in listOf(OS.MAC, OS.WINDOWS, OS.LINUX, null)) { + for (setting in listOf(null, "true", "false")) { + for (supported in listOf(false, true)) { + assertLoginUsesExpectedArgs( + extraSettings = setting?.let { arrayOf(USE_KEYRING to it) } ?: emptyArray(), + expectedArgsSubstring = "login --use-token-as-session https://test.coder.com", + expectGlobalConfig = setting == "false" || !supported || !CoderCLIManager.supportsKeyringStorage( + os + ), + feats = Features(keyringAuth = supported), + osOverride = os, + ) + } + } + } + } + + private fun credentialCli(keyring: Boolean = true): Pair { + val root = tmpdir.resolve("credentials-${UUID.randomUUID()}") + val binary = root.resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") + writeExecutable( + binary, if (getOS() == OS.WINDOWS) { + mkbin("echo %*") + } else { + mkbin("printf '%s' \"$*\"") + } + ) + val settings = CoderSettingsStore( + pluginTestSettingsStore( + BINARY_DESTINATION to binary.toString(), + DATA_DIRECTORY to root.resolve("data").toString(), + USE_KEYRING to keyring.toString(), + ), + Environment(), underlyingLogger, + ) + return CoderCLIManager( + context.copy(settingsStore = settings), + URL("https://test.coder.com"), + OS.MAC + ) to settings + } + + @Test + fun `successful keyring login removes the old plaintext session`() = runBlocking { + val (cli, _) = credentialCli() + val session = cli.coderConfigPath.resolve("session").toFile() + session.parentFile.mkdirs() + session.writeText("old-token") + + cli.login("new-token", Features(keyringAuth = true)) + + assertFalse(session.exists()) + } + + @Test + fun `failed keyring login preserves the previous plaintext session`() = runBlocking { + val (cli, _) = credentialCli() + writeExecutable(cli.localBinaryPath, mkbin(if (getOS() == OS.WINDOWS) "exit /b 1" else "exit 1")) + val session = cli.coderConfigPath.resolve("session").toFile() + session.parentFile.mkdirs() + session.writeText("old-token") + + assertFailsWith { cli.login("new-token", Features(keyringAuth = true)) } + + assertEquals("old-token", session.readText()) + } + + @Test + fun `logout and token refresh retain active storage until the next sign in`() = runBlocking { + val (cli, settings) = credentialCli() + settings.updateUseKeyring(false) + val feats = Features(keyringAuth = true) + + assertContains(cli.login("new-token", feats), "--use-keyring=true") + val args = cli.logout(feats) + assertContains(args, "--url https://test.coder.com --use-keyring=true logout --yes") + assertFalse(args.contains("--global-config")) + + val nextCli = CoderCLIManager(context.copy(settingsStore = settings), URL("https://test.coder.com"), OS.MAC) + val nextArgs = nextCli.login("next-token", feats) + assertContains(nextArgs, "--use-keyring=false") + assertContains(nextArgs, "--global-config") + assertContains(nextCli.logout(feats), "--use-keyring=false logout --yes") + } + + @Test + fun `unsupported CLI warns once per session`() = runBlocking { + val (cli, _) = credentialCli() + io.mockk.clearMocks(underlyingLogger, answers = false) + + cli.login("token", Features()) + cli.login("refreshed-token", Features()) + + verify(exactly = 1) { underlyingLogger.warn(match { it.contains("2.29.0 or newer is required") }) } + } + + @Test + fun `start workspace only uses url auth when keyring is enabled and runtime supports it`() { + val binaryFile = tmpdir.resolve("start-workspace-${UUID.randomUUID()}") + .resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") + val argsFile = binaryFile.parent.resolve("argv.txt") + val stdout = "start ok" + val script = if (getOS() == OS.WINDOWS) { + mkbin( + """ + echo %* > "${argsFile.toAbsolutePath()}" + echo $stdout + """.trimIndent() + ) + } else { + mkbin( + """ + printf '%s\n' "$*" > '${argsFile.toAbsolutePath()}' + printf '$stdout\n' + """.trimIndent() + ) + } + writeExecutable(binaryFile, script) + + val settings = CoderSettingsStore( + pluginTestSettingsStore( + BINARY_DESTINATION to binaryFile.toString(), + ENABLE_DOWNLOADS to "false", + USE_KEYRING to "true", + ), + Environment(), + underlyingLogger + ) + val ccm = CoderCLIManager(context.copy(settingsStore = settings), URL("https://test.coder.com")) + + val keyringSupported = CoderCLIManager.supportsKeyringStorage(getOS()) + assertEquals( + stdout, + ccm.startWorkspace( + WorkspaceAddress.from(workspace("dev").copy(ownerName = "alice")), + Features(keyringAuth = true) + ).trim() + ) + val args = argsFile.toFile().readText() + if (keyringSupported) { + assertContains(args, "--url https://test.coder.com --use-keyring=true start --yes -- alice/dev") + assertFalse(args.contains("--global-config")) + } else { + assertContains(args, "--global-config") + } + } + + @Test + fun `start workspace keeps global config when keyring is enabled but CLI does not support it`() { + val binaryFile = tmpdir.resolve("start-workspace-${UUID.randomUUID()}") + .resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") + val argsFile = binaryFile.parent.resolve("argv.txt") + val stdout = "start ok" + val script = if (getOS() == OS.WINDOWS) { + mkbin( + """ + echo %* > "${argsFile.toAbsolutePath()}" + echo $stdout + """.trimIndent() + ) + } else { + mkbin( + """ + printf '%s\n' "$*" > '${argsFile.toAbsolutePath()}' + printf '$stdout\n' + """.trimIndent() + ) + } + writeExecutable(binaryFile, script) + + val settings = CoderSettingsStore( + pluginTestSettingsStore( + BINARY_DESTINATION to binaryFile.toString(), + ENABLE_DOWNLOADS to "false", + USE_KEYRING to "true", + ), + Environment(), + underlyingLogger + ) + val ccm = CoderCLIManager(context.copy(settingsStore = settings), URL("https://test.coder.com")) + + assertEquals( + stdout, + ccm.startWorkspace(WorkspaceAddress.from(workspace("dev").copy(ownerName = "alice")), Features()).trim() ) + val args = argsFile.toFile().readText() + assertContains(args, "--global-config") + assertFalse(args.contains("--url https://test.coder.com --use-keyring=true start --yes -- alice/dev")) + } + + @Test + fun `start workspace keeps global config on linux even when keyring is enabled and CLI supports it`() { + val binaryFile = tmpdir.resolve("start-workspace-${UUID.randomUUID()}") + .resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") + val argsFile = binaryFile.parent.resolve("argv.txt") + val stdout = "start ok" + val script = if (getOS() == OS.WINDOWS) { + mkbin( + """ + echo %* > "${argsFile.toAbsolutePath()}" + echo $stdout + """.trimIndent() + ) + } else { + mkbin( + """ + printf '%s\n' "$*" > '${argsFile.toAbsolutePath()}' + printf '$stdout\n' + """.trimIndent() + ) + } + writeExecutable(binaryFile, script) + + val settings = CoderSettingsStore( + pluginTestSettingsStore( + BINARY_DESTINATION to binaryFile.toString(), + ENABLE_DOWNLOADS to "false", + USE_KEYRING to "true", + ), + Environment(), + underlyingLogger + ) + val ccm = CoderCLIManager( + context.copy(settingsStore = settings), + URL("https://test.coder.com"), + OS.LINUX, + ) + + assertEquals( + stdout, + ccm.startWorkspace( + WorkspaceAddress.from(workspace("dev").copy(ownerName = "alice")), + Features(keyringAuth = true) + ).trim() + ) + val args = argsFile.toFile().readText() + assertContains(args, "--global-config") + assertFalse(args.contains("--url https://test.coder.com --use-keyring=true start --yes -- alice/dev")) } @Test @@ -997,6 +1317,101 @@ internal class CoderCLIManagerTest { } } + @Test + fun `ssh config only uses url auth when keyring is enabled and runtime supports it`() { + val workspace = workspace("foo", agents = mapOf("agent1" to UUID.randomUUID().toString())) + val settings = CoderSettingsStore( + pluginTestSettingsStore( + USE_KEYRING to "true", + SSH_CONFIG_PATH to tmpdir.resolve("keyring-ssh.conf").toString(), + DATA_DIRECTORY to tmpdir.resolve("keyring-ssh-data").toString(), + NETWORK_INFO_DIR to tmpdir.resolve("keyring-network-info").toString(), + ), + Environment(), + context.logger + ) + val ccm = CoderCLIManager( + context.copy(settingsStore = settings), + URI.create("https://test.coder.invalid").toURL() + ) + + ccm.configSsh( + workspace.latestBuild.resources + .filter { it.agents != null } + .flatMap { resource -> resource.agents!!.map { agent -> WorkspaceAddress.from(workspace, agent) } } + .toSet(), + feats = Features(reportWorkspaceUsage = true, keyringAuth = true), + ) + + val keyringSupported = CoderCLIManager.supportsKeyringStorage(getOS()) + val sshConfig = Path.of(settings.sshConfigPath).toFile().readText() + assertContains(sshConfig, "--url https://test.coder.invalid") + assertEquals(!keyringSupported, sshConfig.contains("--global-config")) + } + + @Test + fun `ssh config keeps global config when keyring is enabled but CLI does not support it`() { + val workspace = workspace("foo", agents = mapOf("agent1" to UUID.randomUUID().toString())) + val settings = CoderSettingsStore( + pluginTestSettingsStore( + USE_KEYRING to "true", + SSH_CONFIG_PATH to tmpdir.resolve("keyring-ssh-unsupported.conf").toString(), + DATA_DIRECTORY to tmpdir.resolve("keyring-ssh-unsupported-data").toString(), + NETWORK_INFO_DIR to tmpdir.resolve("keyring-ssh-unsupported-network-info").toString(), + ), + Environment(), + context.logger + ) + val ccm = CoderCLIManager( + context.copy(settingsStore = settings), + URI.create("https://test.coder.invalid").toURL() + ) + + ccm.configSsh( + workspace.latestBuild.resources + .filter { it.agents != null } + .flatMap { resource -> resource.agents!!.map { agent -> WorkspaceAddress.from(workspace, agent) } } + .toSet(), + feats = Features(reportWorkspaceUsage = true), + ) + + val sshConfig = Path.of(settings.sshConfigPath).toFile().readText() + assertContains(sshConfig, "--global-config") + assertContains(sshConfig, "--url https://test.coder.invalid") + } + + @Test + fun `ssh config keeps global config on linux even when keyring is enabled and CLI supports it`() { + val workspace = workspace("foo", agents = mapOf("agent1" to UUID.randomUUID().toString())) + val settings = CoderSettingsStore( + pluginTestSettingsStore( + USE_KEYRING to "true", + SSH_CONFIG_PATH to tmpdir.resolve("keyring-linux-ssh.conf").toString(), + DATA_DIRECTORY to tmpdir.resolve("keyring-linux-ssh-data").toString(), + NETWORK_INFO_DIR to tmpdir.resolve("keyring-linux-network-info").toString(), + ), + Environment(), + context.logger + ) + val ccm = CoderCLIManager( + context.copy(settingsStore = settings), + URI.create("https://test.coder.invalid").toURL(), + OS.LINUX, + ) + + ccm.configSsh( + workspace.latestBuild.resources + .filter { it.agents != null } + .flatMap { resource -> resource.agents!!.map { agent -> WorkspaceAddress.from(workspace, agent) } } + .toSet(), + feats = Features(reportWorkspaceUsage = true, keyringAuth = true), + ) + + val sshConfig = Path.of(settings.sshConfigPath).toFile().readText() + assertContains(sshConfig, "--global-config") + assertContains(sshConfig, "--url https://test.coder.invalid") + } + @Test fun testMalformedHeader() { val tests = @@ -1122,14 +1537,14 @@ internal class CoderCLIManagerTest { fun testFailVersionParse() { val tests = mapOf( - null to ProcessInitException::class, + null to ProcessExecutionException::class, echo("""{"foo": true, "baz": 1}""") to MissingVersionException::class, echo("""{"version": ""}""") to MissingVersionException::class, echo("""v0.0.1""") to JsonEncodingException::class, echo("""{"version: """) to JsonEncodingException::class, echo("""{"version": "invalid"}""") to InvalidVersionException::class, exit(0) to MissingVersionException::class, - exit(1) to InvalidExitValueException::class, + exit(1) to ProcessExitException::class, ) val ccm = CoderCLIManager( @@ -1243,7 +1658,7 @@ internal class CoderCLIManagerTest { fun testFeatures() { val tests = listOf( - Pair("2.5.0", Features(true)), + Pair("2.5.0", Features(disableAutostart = true)), Pair("2.13.0", Features(disableAutostart = true, reportWorkspaceUsage = true)), Pair( "2.25.0", @@ -1259,11 +1674,22 @@ internal class CoderCLIManagerTest { disableAutostart = true, reportWorkspaceUsage = true, wildcardSsh = true, - buildReason = true + buildReason = true, + keyringAuth = true, + ) + ), + Pair( + "2.29.0", + Features( + disableAutostart = true, + reportWorkspaceUsage = true, + wildcardSsh = true, + buildReason = true, + keyringAuth = true, ) ), - Pair("2.4.9", Features(false)), - Pair("1.0.1", Features(false)), + Pair("2.4.9", Features()), + Pair("1.0.1", Features()), ) tests.forEach { @@ -1292,6 +1718,14 @@ internal class CoderCLIManagerTest { } } + @Test + fun `keyring storage is only supported on mac and windows`() { + assertTrue(CoderCLIManager.supportsKeyringStorage(OS.MAC)) + assertTrue(CoderCLIManager.supportsKeyringStorage(OS.WINDOWS)) + assertFalse(CoderCLIManager.supportsKeyringStorage(OS.LINUX)) + assertFalse(CoderCLIManager.supportsKeyringStorage(null)) + } + companion object { private val tmpdir: Path = Path.of(System.getProperty("java.io.tmpdir")).resolve("coder-toolbox-test/cli-manager") diff --git a/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt b/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt index c78473da..d0aa5ed9 100644 --- a/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt +++ b/src/test/kotlin/com/coder/toolbox/settings/CoderSettingsTest.kt @@ -13,6 +13,7 @@ import com.coder.toolbox.store.TLS_ALTERNATE_HOSTNAME import com.coder.toolbox.store.TLS_CA_PATH import com.coder.toolbox.store.TLS_CERT_PATH import com.coder.toolbox.store.TLS_KEY_PATH +import com.coder.toolbox.store.USE_KEYRING import com.coder.toolbox.util.OS import com.coder.toolbox.util.getOS import com.coder.toolbox.util.pluginTestSettingsStore @@ -309,6 +310,7 @@ internal class CoderSettingsTest { val settings = CoderSettingsStore(pluginTestSettingsStore(), Environment(), logger) assertEquals(true, settings.readOnly().enableDownloads) assertEquals(null, settings.readOnly().headerCommand) + assertEquals(true, settings.readOnly().useKeyring) assertEquals(null, settings.readOnly().tls.certPath) assertEquals(null, settings.readOnly().tls.keyPath) assertEquals(null, settings.readOnly().tls.caPath) @@ -341,6 +343,7 @@ internal class CoderSettingsTest { pluginTestSettingsStore( ENABLE_DOWNLOADS to false.toString(), HEADER_COMMAND to "test header", + USE_KEYRING to true.toString(), TLS_CERT_PATH to "tls cert path", TLS_KEY_PATH to "tls key path", TLS_CA_PATH to "tls ca path", @@ -354,6 +357,7 @@ internal class CoderSettingsTest { assertEquals(false, settings.readOnly().enableDownloads) assertEquals("test header", settings.readOnly().headerCommand) + assertEquals(true, settings.readOnly().useKeyring) assertEquals("tls cert path", settings.readOnly().tls.certPath) assertEquals("tls key path", settings.readOnly().tls.keyPath) assertEquals("tls ca path", settings.readOnly().tls.caPath) diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt index 0eca47b8..8dab0160 100644 --- a/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSettingsStoreTest.kt @@ -37,6 +37,15 @@ class CoderSettingsStoreTest { System.setProperty("os.arch", originalOsArch) } + @Test + fun `keyring is enabled unless explicitly disabled`() { + assertTrue(store.useKeyring) + store.updateUseKeyring(false) + assertEquals(false, store.useKeyring) + store.updateUseKeyring(true) + assertTrue(store.useKeyring) + } + @Test fun `Default CLI and signature for Windows AMD64`() = assertBinaryAndSignature("Windows 10", "amd64", "coder-windows-amd64.exe", "coder-windows-amd64.exe.asc") diff --git a/src/test/kotlin/com/coder/toolbox/util/ErrorTest.kt b/src/test/kotlin/com/coder/toolbox/util/ErrorTest.kt new file mode 100644 index 00000000..4bfbb061 --- /dev/null +++ b/src/test/kotlin/com/coder/toolbox/util/ErrorTest.kt @@ -0,0 +1,48 @@ +package com.coder.toolbox.util + +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertFalse + +internal class ErrorTest { + @Test + fun `sanitizeSecrets redacts common token forms`() { + val raw = """ + env={CODER_SESSION_TOKEN=super-secret-token} + header=Coder-Session-Token: super-secret-token + header=Coder-Session-Token=super-secret-token + header=coder-session-token = super-secret-token + argv=--token super-secret-token + argv=--token=super-secret-token + uri=https://coder.example.com?token=super-secret-token + """.trimIndent() + + val sanitized = raw.sanitizeSecrets() + + assertContains(sanitized, "CODER_SESSION_TOKEN=") + assertContains(sanitized, "Coder-Session-Token: ") + assertContains(sanitized, "Coder-Session-Token=") + assertContains(sanitized, "coder-session-token = ") + assertContains(sanitized, "--token ") + assertContains(sanitized, "--token=") + assertContains(sanitized, "?token=") + assertFalse(sanitized.contains("super-secret-token")) + } + + @Test + fun `process exit exception redacts output in message`() { + val ex = ProcessExitException( + ProcessResult( + command = listOf("coder", "login"), + exitCode = 1, + stdout = "CODER_SESSION_TOKEN=super-secret-token", + stderr = "Coder-Session-Token: super-secret-token", + ), + expectedExitCodes = 0..0, + ) + + assertContains(ex.message.orEmpty(), "CODER_SESSION_TOKEN=") + assertContains(ex.message.orEmpty(), "Coder-Session-Token: ") + assertFalse(ex.message.orEmpty().contains("super-secret-token")) + } +} diff --git a/src/test/kotlin/com/coder/toolbox/util/HeadersTest.kt b/src/test/kotlin/com/coder/toolbox/util/HeadersTest.kt index c3b75d2b..436c5d54 100644 --- a/src/test/kotlin/com/coder/toolbox/util/HeadersTest.kt +++ b/src/test/kotlin/com/coder/toolbox/util/HeadersTest.kt @@ -5,8 +5,27 @@ import kotlin.test.Test import kotlin.test.assertContains import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse internal class HeadersTest { + @Test + fun `failing header command redacts equals separated session tokens`() { + val command = if (getOS() == OS.WINDOWS) { + "echo Coder-Session-Token=header-secret&&echo coder-session-token=stderr-secret>&2&&exit /b 1" + } else { + "printf 'Coder-Session-Token=header-secret\\n'; " + + "printf 'coder-session-token=stderr-secret\\n' >&2; exit 1" + } + val error = assertFailsWith { + getHeaders(URL("http://localhost"), command) + } + + assertContains(error.result.stdout, "Coder-Session-Token=") + assertContains(error.result.stderr, "coder-session-token=") + assertFalse(error.message.orEmpty().contains("header-secret")) + assertFalse(error.message.orEmpty().contains("stderr-secret")) + } + @Test @IgnoreOnWindows fun testGetHeadersOK() { @@ -104,7 +123,7 @@ internal class HeadersTest { "echo foo foo=bar" to "Header name cannot contain spaces, got \"foo foo\"", "echo foo=bar " to "Header name cannot contain spaces, got \" foo\"", "exit /b 1" to "Unexpected exit value: 1", - // "foobar" appears in the InvalidExitValueException message as part of the command string. + // "foobar" appears in the process error message as part of the command string. "echo foobar>&2&&exit /b 1" to "foobar", // echo. outputs a bare CRLF; a blank line anywhere in the output is an error. "echo foo=bar&&echo." to "Blank lines are not allowed", diff --git a/src/test/kotlin/com/coder/toolbox/util/ProcessRunnerTest.kt b/src/test/kotlin/com/coder/toolbox/util/ProcessRunnerTest.kt new file mode 100644 index 00000000..9fd36438 --- /dev/null +++ b/src/test/kotlin/com/coder/toolbox/util/ProcessRunnerTest.kt @@ -0,0 +1,365 @@ +package com.coder.toolbox.util + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.runInterruptible +import kotlinx.coroutines.withTimeout +import java.nio.file.Files +import java.nio.file.Path +import java.util.concurrent.TimeUnit +import kotlin.test.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse + +internal class ProcessRunnerTest { + @Test + fun `timeout terminates an observed child after its parent exits`() { + withChildCommand("exit-parent") { command, releaseParent, handles -> + assertFailsWith { + runProcess(command, timeoutMillis = 5000, onOutputLine = { + // Keep the parent alive across several observation intervals before allowing it to exit. + if (it == "ready") Thread.sleep(500) + releaseParent(it) + }) + } + handles().forEach { assertFalse(it.isAlive, "Process ${it.pid()} survived timeout") } + } + } + + @Test + fun `timeout terminates a parent and its running child`() { + withChildCommand("keep-parent") { command, _, handles -> + assertFailsWith { + runProcess(command, timeoutMillis = 2000) + } + handles().forEach { assertFalse(it.isAlive, "Process ${it.pid()} survived timeout") } + } + } + + @Test + fun `successful command can leave a background child with redirected output`() { + withChildCommand("detached-output") { command, releaseParent, handles -> + val result = runProcess(command, timeoutMillis = 5000, onOutputLine = { releaseParent(it) }) + assertEquals(0, result.exitCode) + assertEquals("ready", result.stdout.trim()) + assertEquals(1, handles().count { it.isAlive }) + } + } + + @Test + fun `cancellation terminates both parent and child`() = runBlocking { + val ready = CompletableDeferred() + withChildCommand("keep-parent") { command, _, handles -> + val job = launch(Dispatchers.Default) { + runInterruptible { + runProcess(command, onOutputLine = { if (it == "ready") ready.complete(Unit) }) + } + } + try { + withTimeout(5000) { ready.await() } + withTimeout(5000) { job.cancelAndJoin() } + handles().forEach { assertFalse(it.isAlive, "Process ${it.pid()} survived cancellation") } + } finally { + job.cancelAndJoin() + } + } + } + + @Test + fun `missing command raises an execution error`() { + val error = assertFailsWith { + runProcess(listOf("coder-review-command-that-does-not-exist"), timeoutMillis = 5000) + } + assertContains(error.message.orEmpty(), "Failed to start process") + } + + @Test + fun `reader failure terminates both parent and child`() { + withChildCommand("keep-parent") { command, _, handles -> + val error = assertFailsWith { + runProcess(command, timeoutMillis = 5000, onOutputLine = { error("callback failed") }) + } + assertContains(error.message.orEmpty(), "callback failed") + handles().forEach { assertFalse(it.isAlive, "Process ${it.pid()} survived reader failure") } + } + } + + @Test + @IgnoreOnWindows + fun `cancellation terminates a running process after delivering live progress`() = runBlocking { + val pid = CompletableDeferred() + val job = launch(Dispatchers.Default) { + runInterruptible { + runProcess(listOf("sh", "-c", "echo $$; exec sleep 30"), onOutputLine = { pid.complete(it.toLong()) }) + } + } + var process: ProcessHandle? = null + try { + process = ProcessHandle.of(withTimeout(5000) { pid.await() }).orElseThrow() + withTimeout(5000) { job.cancelAndJoin() } + assertFalse(process.isAlive) + } finally { + job.cancelAndJoin() + process?.destroyForcibly() + } + } + + @Test + @IgnoreOnWindows + fun `runProcess closes stdin so unexpected prompts see EOF`() { + val result = runProcess(listOf("sh", "-c", "read value || printf eof"), timeoutMillis = 2000) + assertEquals("eof", result.stdout) + } + + @Test + @IgnoreOnWindows + fun `runProcess streams complete lines and retains original output`() { + val lines = mutableListOf() + val text = "x".repeat(8193) + val result = runProcess( + listOf("sh", "-c", "printf '%s' '$text'; printf ' done\\r\\nsecond\\rfinal'"), + onOutputLine = lines::add, + ) + assertEquals(listOf("$text done", "second", "final"), lines) + assertEquals("$text done\r\nsecond\rfinal", result.stdout) + } + + @Test + @IgnoreOnWindows + fun `runProcess terminates the process on timeout`() { + val pidFile = java.nio.file.Files.createTempFile("coder-process-timeout", ".pid") + try { + assertFailsWith { + runProcess( + listOf("sh", "-c", "echo $$ > '$pidFile'; exec sleep 30"), + timeoutMillis = 1000, + ) + } + val pid = pidFile.toFile().readText().trim().toLong() + assertFalse(ProcessHandle.of(pid).map { it.isAlive }.orElse(false)) + } finally { + java.nio.file.Files.deleteIfExists(pidFile) + } + } + + @Test + @IgnoreOnWindows + fun `runProcess reports reader callback failure`() { + val error = assertFailsWith { + runProcess(listOf("sh", "-c", "printf 'progress\\n'"), onOutputLine = { error("callback failed") }) + } + assertContains(error.message.orEmpty(), "callback failed") + } + + @Test + @IgnoreOnWindows + fun `runProcess redacts unlabeled token output in progress and failures`() { + val lines = mutableListOf() + val error = assertFailsWith { + runProcess( + listOf("sh", "-c", "printf '%s\\n' \"${'$'}CODER_SESSION_TOKEN\"; exit 1"), + environment = mapOf("CODER_SESSION_TOKEN" to "secret-value"), + onOutputLine = lines::add, + ) + } + assertEquals(listOf(""), lines) + assertFalse(error.message.orEmpty().contains("secret-value")) + } + + @Test + @IgnoreOnWindows + fun `runProcess captures stdout and stderr`() { + val result = runProcess(listOf("sh", "-c", "printf hello && printf problem >&2")) + + assertEquals(0, result.exitCode) + assertEquals("hello", result.stdout) + assertEquals("problem", result.stderr) + } + + @Test + @IgnoreOnWindows + fun `runProcess can discard stderr on success`() { + val result = runProcess( + listOf("sh", "-c", "printf hello && printf problem >&2"), + stderrMode = ProcessStderrMode.DISCARD_ON_SUCCESS, + ) + + assertEquals(0, result.exitCode) + assertEquals("hello", result.stdout) + assertEquals("", result.stderr) + } + + @Test + @IgnoreOnWindows + fun `runProcess includes stderr on failure when discarding stderr on success`() { + val ex = assertFailsWith { + runProcess( + listOf("sh", "-c", "printf problem >&2; exit 5"), + stderrMode = ProcessStderrMode.DISCARD_ON_SUCCESS, + ) + } + + assertEquals(5, ex.result.exitCode) + assertEquals("problem", ex.result.stderr) + assertContains(ex.message.orEmpty(), "problem") + } + + @Test + @IgnoreOnUnix + fun `runProcess captures stdout and stderr on windows`() { + val result = runProcess(listOf("cmd.exe", "/c", "echo hello&&echo problem 1>&2")) + + assertEquals(0, result.exitCode) + assertContains(result.stdout, "hello") + assertContains(result.stderr, "problem") + } + + @Test + @IgnoreOnUnix + fun `runProcess can discard stderr on success on windows`() { + val result = runProcess( + listOf("cmd.exe", "/c", "echo hello&&echo problem 1>&2"), + stderrMode = ProcessStderrMode.DISCARD_ON_SUCCESS, + ) + + assertEquals(0, result.exitCode) + assertContains(result.stdout, "hello") + assertEquals("", result.stderr) + } + + @Test + @IgnoreOnUnix + fun `runProcess includes stderr on failure when discarding stderr on success on windows`() { + val ex = assertFailsWith { + runProcess( + listOf("cmd.exe", "/c", "echo problem 1>&2&&exit /b 5"), + stderrMode = ProcessStderrMode.DISCARD_ON_SUCCESS, + ) + } + + assertEquals(5, ex.result.exitCode) + assertContains(ex.result.stderr, "problem") + assertContains(ex.message.orEmpty(), "problem") + } + + @Test + @IgnoreOnUnix + fun `runProcess passes environment to child process on windows`() { + val result = runProcess( + listOf("cmd.exe", "/c", "echo token=%CODER_SESSION_TOKEN%"), + mapOf("CODER_SESSION_TOKEN" to "token"), + ) + + assertEquals("token=token", result.stdout.trim()) + } + + @Test + @IgnoreOnUnix + fun `runProcess accepts configured non-zero exit code on windows`() { + val result = runProcess( + listOf("cmd.exe", "/c", "echo expected failure&&exit /b 7"), + expectedExitCodes = 0..7, + ) + + assertEquals(7, result.exitCode) + assertContains(result.stdout, "expected failure") + } + + @Test + @IgnoreOnUnix + fun `runProcess redacts labeled token values in failure messages on windows`() { + val ex = assertFailsWith { + runProcess( + listOf("cmd.exe", "/c", "echo CODER_SESSION_TOKEN=%CODER_SESSION_TOKEN% 1>&2&&exit /b 7"), + mapOf("CODER_SESSION_TOKEN" to "super-secret-token"), + ) + } + + assertContains(ex.message.orEmpty(), "CODER_SESSION_TOKEN=") + assertFalse(ex.message.orEmpty().contains("super-secret-token")) + } + + @Test + fun `runProcess passes environment to child process`() { + val result = + if (getOS() == OS.WINDOWS) { + runProcess( + listOf("cmd.exe", "/c", "echo %CODER_SESSION_TOKEN%"), + mapOf("CODER_SESSION_TOKEN" to "token"), + ) + } else { + runProcess( + listOf("sh", "-c", "printf %s \"${'$'}CODER_SESSION_TOKEN\""), + mapOf("CODER_SESSION_TOKEN" to "token"), + ) + } + + assertEquals("token", result.stdout.trim()) + } + + @Test + fun `runProcess throws sanitized exception on unexpected exit`() { + val ex = + if (getOS() == OS.WINDOWS) { + assertFailsWith { + runProcess( + listOf("cmd.exe", "/c", "echo CODER_SESSION_TOKEN=%CODER_SESSION_TOKEN% 1>&2&&exit /b 7"), + mapOf("CODER_SESSION_TOKEN" to "super-secret-token"), + ) + } + } else { + assertFailsWith { + runProcess( + listOf("sh", "-c", "printf 'CODER_SESSION_TOKEN=%s' \"${'$'}CODER_SESSION_TOKEN\" >&2; exit 7"), + mapOf("CODER_SESSION_TOKEN" to "super-secret-token"), + ) + } + } + + assertEquals(7, ex.result.exitCode) + assertFalse(ex.message.orEmpty().contains("super-secret-token")) + } + + private inline fun withChildCommand( + mode: String, + test: (List, (String) -> Unit, () -> List) -> Unit, + ) { + val directory = Files.createTempDirectory("coder process tree ") + val pidFile = directory.resolve("processes.pid") + val releaseFile = directory.resolve("release parent") + val handles = { + Files.readString(pidFile).trim().split(" ").mapNotNull { + ProcessHandle.of(it.toLong()).orElse(null) + } + } + val command = listOf( + Path.of(System.getProperty("java.home"), "bin", if (getOS() == OS.WINDOWS) "java.exe" else "java") + .toString(), + "-cp", + Path.of(ProcessRunnerTestCommand::class.java.protectionDomain.codeSource.location.toURI()).toString(), + ProcessRunnerTestCommand::class.java.name, mode, pidFile.toString(), releaseFile.toString(), + ) + try { + test(command, { if (it == "ready") Files.writeString(releaseFile, "exit") }, handles) + } finally { + if (Files.exists(pidFile)) { + stopTestProcesses(handles()) + } + Files.deleteIfExists(pidFile) + Files.deleteIfExists(releaseFile) + Files.deleteIfExists(directory) + } + } + + private fun stopTestProcesses(handles: List) { + handles.filter { it.isAlive }.forEach { + it.destroyForcibly() + it.onExit().get(5, TimeUnit.SECONDS) + } + } +}