From 7290ccb3fd219874719a6013899e04c58afeb947 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Fri, 6 Nov 2026 23:52:29 +0200 Subject: [PATCH 1/6] Fix workspace link trust and credential isolation Two security weaknesses addressed in the VS Code extension also affect Toolbox, as identified during a review of those fixes. The Toolbox URI handler accepts deployment URLs from workspace links without a trust prompt, while the secret store retrieves API tokens by hostname alone. A crafted link can start a connection to an untrusted deployment, and a different scheme or port on the same host can reuse a saved token. With this change, the URI handler requests confirmation for a different deployment or a fresh connection. Links to the active origin retain the existing deployment URL and continue without a prompt. Cancellation preserves the current session, and URI logging omits tokens. The secret store now keeps API tokens under separate keys for each normalized scheme, host, and port. Logout clears only the matching origin. Legacy hostname-only entries remain untouched and are ignored, so existing API-token users sign in again. The remote provider also prevents delayed OAuth refreshes from updating a CLI session for another origin. Regression tests cover trust decisions, cancellation, URL normalization, separate token storage, and OAuth refreshes. The changelog records the security fixes and the sign-in requirement. --- CHANGELOG.md | 22 +- .../com/coder/toolbox/CoderRemoteProvider.kt | 41 ++- .../com/coder/toolbox/cli/CoderCLIManager.kt | 2 +- .../coder/toolbox/store/CoderSecretsStore.kt | 11 +- .../com/coder/toolbox/util/URLExtensions.kt | 14 + .../coder/toolbox/CoderRemoteProviderTest.kt | 241 ++++++++++++++++++ .../coder/toolbox/cli/CoderCLIManagerTest.kt | 149 +++++++++-- .../toolbox/store/CoderSecretsStoreTest.kt | 112 ++++++++ .../coder/toolbox/util/URLExtensionsTest.kt | 17 ++ 9 files changed, 568 insertions(+), 41 deletions(-) create mode 100644 src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ef52e67..9eebf579 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,22 +1,32 @@ # Coder Toolbox Plugin Changelog +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). + ## Unreleased -### Breaking changes +### Changed -- drop support for Coder server and CLI versions older than v2.29.0. Connecting to an older deployment, or with an - older CLI binary, now stops with a message asking to upgrade to v2.29.0 or newer +- Stream build status and provisioner output while updating, restarting, or stopping workspaces, falling back to + polling when the WebSocket connection is unavailable. -### Changed +### Removed -- stream build status and provisioner output while updating, restarting, or stopping workspaces, falling back to - polling when the WebSocket connection is unavailable +- **Breaking:** Support for Coder server and CLI versions older than v2.29.0. Connecting to an older deployment, or + with an older CLI binary, now stops with a message asking to upgrade to v2.29.0 or newer. ### Fixed - SSH connections being blocked while non-blocking startup scripts run or after startup scripts fail or time out. - Automatic connection not starting when a workspace agent becomes connectable after the initial workspace load. +### Security + +- Require confirmation before a workspace link connects to a different Coder deployment or starts a fresh connection. + Links to the currently connected deployment continue without a prompt. +- Store Toolbox API tokens separately for each deployment scheme, host, and port to prevent cross-deployment reuse. + Tokens saved by older plugin versions require one new sign-in because they were stored without an origin. +- Prevent delayed OAuth refreshes from updating CLI credentials for a different deployment. + ## 0.9.5 - 2026-10-30 ### Added diff --git a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt index 6f340913..b5aaebb0 100644 --- a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt +++ b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt @@ -16,6 +16,8 @@ import com.coder.toolbox.util.DialogUi import com.coder.toolbox.util.TOKEN import com.coder.toolbox.util.URL import com.coder.toolbox.util.WebUrlValidationResult.Invalid +import com.coder.toolbox.util.hasSameOrigin +import com.coder.toolbox.util.origin import com.coder.toolbox.util.owner import com.coder.toolbox.util.toQueryParameters import com.coder.toolbox.util.toURL @@ -507,24 +509,29 @@ class CoderRemoteProvider( context.logger.logAndShowInfo("URI will not be handled", "No query parameters were provided") return } - context.logger.info("Handling $uri...") val newUrl = resolveDeploymentUrl(params)?.toURL() ?: return + val newOrigin = requireNotNull(newUrl.origin()) { + "The deployment URL must have a valid HTTP(S) host and port." + } val newToken = if (context.settingsStore.requiresMTlsAuth) null else resolveToken(params) ?: return - if (sameUrl(newUrl, client?.url)) { + context.logger.info("Handling workspace link for $newOrigin") + val activeUrl = client?.url?.takeIf { newUrl.hasSameOrigin(it) } + if (activeUrl != null) { coderHeaderPage.isBusy.update { true } try { val activeSession = if (context.settingsStore.requiresTokenAuth) { newToken?.let { - refreshSession(newUrl, it) + refreshSession(activeUrl, it) } ?: (this.client!! to this.cli!!) } else { this.client!! to this.cli!! } - handleLink(params, newUrl, activeSession.first, activeSession.second) + handleLink(params, activeUrl, activeSession.first, activeSession.second) } finally { coderHeaderPage.isBusy.update { false } } } else { + if (!confirmDeployment(newOrigin, newToken != null)) return // Different URL - we need a new connection. Tear down any // in-flight wizard, install a fresh one on the router, and let // showPluginEnvironmentsPage() pull it through getOverrideUiPage. @@ -539,6 +546,8 @@ class CoderRemoteProvider( router.navigate(wizard) context.popupPluginMainPage() } + } catch (ex: CancellationException) { + throw ex } catch (ex: Exception) { val textError = if (ex is APIResponseException) { if (!ex.reason.isNullOrBlank()) { @@ -648,7 +657,21 @@ class CoderRemoteProvider( return token } - private fun sameUrl(first: URL, second: URL?): Boolean = first.toURI().normalize() == second?.toURI()?.normalize() + private suspend fun confirmDeployment(origin: String, hasToken: Boolean): Boolean { + val disclosure = if (hasToken) { + " This link contains a token that will be sent to this deployment to sign you in." + } else "" + context.ui.showWindowSuspending() + return context.ui.showYesNoPopup( + context.i18n.ptrl("Connect to a Coder deployment?"), + context.i18n.pnotr( + "$origin\n\nToolbox is not currently connected to this deployment.$disclosure " + + "Continue only if you trust it. Connecting can update your SSH configuration." + ), + context.i18n.ptrl("Trust and Continue"), + context.i18n.ptrl("Cancel"), + ) + } private suspend fun refreshSession(url: URL, token: String): Pair { context.logger.info( @@ -768,8 +791,12 @@ class CoderRemoteProvider( } ?: tokenCredentials } - private suspend fun onTokenRefreshed(url: URL, oauthSessionCtx: CoderOAuthSessionContext) { - oauthSessionCtx.tokenResponse?.accessToken?.let { cli?.login(it) } + internal suspend fun onTokenRefreshed(url: URL, oauthSessionCtx: CoderOAuthSessionContext) { + // A refresh may finish after the user has connected to another deployment. + val activeCli = cli?.takeIf { + client?.url?.hasSameOrigin(url) == true && it.deploymentURL.hasSameOrigin(url) + } + oauthSessionCtx.tokenResponse?.accessToken?.let { activeCli?.login(it) } context.secrets.storeOAuthFor(url.toString(), oauthSessionCtx) } diff --git a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt index 39a9a624..1d3a3770 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt @@ -106,7 +106,7 @@ suspend fun ensureCLI( class CoderCLIManager( private val context: CoderToolboxContext, // The URL of the deployment this CLI is for. - private val deploymentURL: URL, + internal val deploymentURL: URL, private val currentOs: OS? = getOS(), ) { internal val usesTokenAuth = context.settingsStore.requiresTokenAuth diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt index 29043b91..a90a7c53 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt @@ -1,6 +1,7 @@ package com.coder.toolbox.store import com.coder.toolbox.oauth.TokenEndpointAuthMethod +import com.coder.toolbox.util.origin import com.coder.toolbox.views.state.CoderOAuthSessionContext import com.coder.toolbox.views.state.StoredOAuthSession import com.jetbrains.toolbox.api.core.PluginSecretStore @@ -22,14 +23,18 @@ class CoderSecretsStore(private val store: PluginSecretStore) { ) val lastDeploymentURL: String = store["last-deployment-url"] ?: "" - fun apiTokenFor(url: URL): String? = store[url.host] + fun apiTokenFor(url: URL): String? = apiSessionKey(url)?.let { store[it] } fun storeApiTokenFor(url: URL, apiToken: String) { - store[url.host] = apiToken + val key = requireNotNull(apiSessionKey(url)) { "Session credentials require an HTTP(S) deployment URL" } + store[key] = apiToken } + private fun apiSessionKey(url: URL): String? = + url.origin()?.let { "api-session:$it" } + fun clearSessionFor(url: URL) { - store.clear(url.host) + apiSessionKey(url)?.let { store.clear(it) } listOf( OAUTH_CLIENT_ID_PREFIX, OAUTH_CLIENT_SECRET_PREFIX, OAUTH_REFRESH_TOKEN, OAUTH_TOKEN_AUTH_METHOD, OAUTH_TOKEN_ENDPOINT diff --git a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt index b5f271b7..47720971 100644 --- a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt +++ b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt @@ -2,6 +2,7 @@ package com.coder.toolbox.util import com.coder.toolbox.util.WebUrlValidationResult.Invalid import com.coder.toolbox.util.WebUrlValidationResult.Valid +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import java.net.IDN import java.net.URI import java.net.URL @@ -9,6 +10,19 @@ import java.net.URLDecoder fun String.toURL(): URL = URI.create(this).toURL() +/** Canonical HTTP(S) origin, without credentials or a path; default ports are omitted. */ +fun URL.origin(): String? = toString().toHttpUrlOrNull()?.newBuilder() + ?.username("") + ?.password("") + ?.encodedPath("/") + ?.query(null) + ?.fragment(null) + ?.build() + ?.toString() + ?.removeSuffix("/") + +fun URL.hasSameOrigin(other: URL?): Boolean = origin()?.let { it == other?.origin() } ?: false + fun String.toBaseURL(): URL { val url = this.toURL() val port = if (url.port != -1) ":${url.port}" else "" diff --git a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt index c8cf059d..2c56526b 100644 --- a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt +++ b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt @@ -16,9 +16,11 @@ import com.coder.toolbox.sdk.v2.models.WorkspaceResource import com.coder.toolbox.sdk.v2.models.WorkspaceStatus import com.coder.toolbox.session.SessionId import com.coder.toolbox.store.CoderSettingsStore +import com.coder.toolbox.util.CoderProtocolHandler import com.coder.toolbox.views.CoderSetupWizardPage import com.coder.toolbox.views.state.StoredOAuthSession import com.coder.toolbox.views.state.WizardStep +import com.coder.toolbox.views.state.toSessionContext import com.jetbrains.toolbox.api.core.diagnostics.Logger import com.jetbrains.toolbox.api.core.util.LoadableState import com.jetbrains.toolbox.api.localization.LocalizableStringFactory @@ -29,7 +31,9 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.slot +import io.mockk.spyk import io.mockk.verify +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -41,11 +45,13 @@ import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertTrue import java.io.FileNotFoundException import java.net.URI +import java.net.URLEncoder import java.util.UUID import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -56,6 +62,7 @@ class CoderRemoteProviderTest { private lateinit var mockClient: CoderRestClient private lateinit var mockCli: CoderCLIManager private lateinit var mockContext: CoderToolboxContext + private lateinit var mockUi: ToolboxUi private lateinit var mockLogger: CoderLogger private lateinit var underlyingLogger: Logger private lateinit var remoteProvider: CoderRemoteProvider @@ -65,6 +72,8 @@ class CoderRemoteProviderTest { mockClient = mockk(relaxed = true) mockCli = mockk(relaxed = true) mockContext = mockk(relaxed = true) + mockUi = mockk(relaxed = true) + every { mockContext.ui } returns mockUi underlyingLogger = mockk(relaxed = true) mockLogger = CoderLogger( underlyingLogger, @@ -77,6 +86,7 @@ class CoderRemoteProviderTest { every { mockContext.settingsStore } returns settingsStore every { mockContext.logger } returns mockLogger every { mockClient.url } returns URI("https://coder.example.com").toURL() + every { mockCli.deploymentURL } returns URI("https://coder.example.com").toURL() remoteProvider = CoderRemoteProvider(mockContext) } @@ -134,6 +144,69 @@ class CoderRemoteProviderTest { coVerify(exactly = 0) { mockCli.logout() } } + @Test + fun `OAuth refresh updates the CLI session for the active origin`() = runTest { + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + val url = URI("https://CODER.example.com:443/").toURL() + val stored = storedOAuthSession().toSessionContext() + val refreshed = stored.copy(tokenResponse = stored.tokenResponse!!.copy(accessToken = "refreshed-token")) + val secrets = mockContext.secrets + + remoteProvider.onTokenRefreshed(url, refreshed) + + coVerify(exactly = 1) { mockCli.login("refreshed-token") } + verify(exactly = 1) { secrets.storeOAuthFor(url.toString(), refreshed) } + } + + @Test + fun `late OAuth refresh never sends the token to another active origin`() = runTest { + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + val stored = storedOAuthSession().toSessionContext() + val refreshed = stored.copy(tokenResponse = stored.tokenResponse!!.copy(accessToken = "refreshed-token")) + val secrets = mockContext.secrets + + for (deployment in listOf( + "http://coder.example.com", + "https://coder.example.com:8443", + "https://other.example.com", + )) { + remoteProvider.onTokenRefreshed(URI(deployment).toURL(), refreshed) + verify(exactly = 1) { secrets.storeOAuthFor(deployment, refreshed) } + } + + coVerify(exactly = 0) { mockCli.login(any()) } + } + + @Test + fun `OAuth refresh after disconnection only saves credentials for its deployment`() = runTest { + setPrivateField(remoteProvider, "cli", mockCli) + val url = URI("https://coder.example.com").toURL() + val stored = storedOAuthSession().toSessionContext() + val refreshed = stored.copy(tokenResponse = stored.tokenResponse!!.copy(accessToken = "refreshed-token")) + val secrets = mockContext.secrets + + remoteProvider.onTokenRefreshed(url, refreshed) + + coVerify(exactly = 0) { mockCli.login(any()) } + verify(exactly = 1) { secrets.storeOAuthFor(url.toString(), refreshed) } + } + + @Test + fun `OAuth refresh checks the CLI origin while a connection is being replaced`() = runTest { + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + val url = URI("https://other.example.com").toURL() + every { mockClient.url } returns url + val stored = storedOAuthSession().toSessionContext() + val refreshed = stored.copy(tokenResponse = stored.tokenResponse!!.copy(accessToken = "refreshed-token")) + + remoteProvider.onTokenRefreshed(url, refreshed) + + coVerify(exactly = 0) { mockCli.login(any()) } + } + @Test fun `given an empty workspace list expect an empty list of environments`() = runTest { // given @@ -512,6 +585,7 @@ class CoderRemoteProviderTest { every { mockClient.url } returns URI("https://old.example.com").toURL() every { mockContext.settingsStore.requiresMTlsAuth } returns false every { mockContext.settingsStore.requiresTokenAuth } returns true + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns true setPrivateField(remoteProvider, "client", mockClient) setPrivateField(remoteProvider, "cli", mockCli) @@ -526,9 +600,176 @@ class CoderRemoteProviderTest { val overridePage = remoteProvider.getOverrideUiPage() assertNotNull(overridePage) assertTrue(overridePage is CoderSetupWizardPage) + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } verify { mockContext.popupPluginMainPage() } } + @Test + fun `links to the active origin continue without confirmation and preserve its URL`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + val activeUrl = URI("https://coder.example.com").toURL() + val provider = spyk(remoteProvider, recordPrivateCalls = true) + val handler = mockk(relaxed = true) + setPrivateField(provider, "client", mockClient) + setPrivateField(provider, "cli", mockCli) + setPrivateField(provider, "linkHandler", handler) + coEvery { provider["refreshSession"](activeUrl, "link-token") } returns (mockClient to mockCli) + + val targets = listOf( + "https://coder.example.com", + "https://CODER.example.com:443/", + "https://coder.example.com/different/path/", + ) + for (target in targets) { + provider.handleUri(workspaceLink(target)) + } + + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + coVerify(exactly = targets.size) { handler.handle(any(), activeUrl, mockClient, mockCli) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + verify(exactly = 0) { underlyingLogger.info(match { it.contains("link-token") }) } + assertNull(provider.getOverrideUiPage()) + } + + @Test + @OptIn(ExperimentalCoroutinesApi::class) + fun `different origins wait for confirmation and cancellation preserves the active session`() = runTest { + val settings = mockContext.settingsStore + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + + for (target in listOf( + "https://other.example.com", + "http://coder.example.com", + "https://coder.example.com:8443", + )) { + val decision = CompletableDeferred() + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } coAnswers { decision.await() } + val handling = async { remoteProvider.handleUri(workspaceLink(target)) } + runCurrent() + + assertFalse(handling.isCompleted) + assertNull(remoteProvider.getOverrideUiPage()) + verify(exactly = 0) { mockContext.popupPluginMainPage() } + coVerify(exactly = 0) { mockClient.initializeSession() } + coVerify(exactly = 0) { mockCli.login(any()) } + + decision.complete(false) + handling.await() + assertNull(remoteProvider.getOverrideUiPage()) + } + coVerify(exactly = 3) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 0) { mockContext.secrets.storeApiTokenFor(any(), any()) } + verify(exactly = 0) { settings.updateLastUsedUrl(any()) } + } + + @Test + @OptIn(ExperimentalCoroutinesApi::class) + fun `fresh link requires confirmation even when matching credentials are saved`() = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockContext.secrets.apiTokenFor(url) } returns "stored-token" + val decision = CompletableDeferred() + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } coAnswers { decision.await() } + + val handling = async { remoteProvider.handleUri(workspaceLink(url.toString())) } + runCurrent() + assertFalse(handling.isCompleted) + verify(exactly = 0) { mockContext.popupPluginMainPage() } + verify(exactly = 0) { mockContext.secrets.apiTokenFor(any()) } + + decision.complete(true) + handling.await() + + val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(url.toString(), page.model.url.toString()) + assertEquals("link-token", page.model.token) + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 1) { mockContext.popupPluginMainPage() } + } + + @Test + fun `declining a fresh link preserves the existing setup wizard`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockContext.secrets.apiTokenFor(any()) } returns null + val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + + remoteProvider.handleUri(workspaceLink("https://other.example.com")) + + assertSame(page, remoteProvider.getOverrideUiPage()) + assertEquals(WizardStep.URL_REQUEST, page.model.currentStep()) + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `dismissing the trust popup cancels handling without showing an error`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } throws CancellationException("Dismissed") + + assertFailsWith { + remoteProvider.handleUri(workspaceLink("https://other.example.com")) + } + + assertNull(remoteProvider.getOverrideUiPage()) + verify(exactly = 0) { underlyingLogger.error(any()) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `certificate authentication also requires trust for another deployment`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns true + every { mockContext.settingsStore.requiresTokenAuth } returns false + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns true + + remoteProvider.handleUri(URI("jetbrains://gateway/com.coder.toolbox?url=https://other.example.com")) + + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertNull(page.model.token) + } + + @Test + fun `trust prompt identifies the destination and token sign-in without exposing credentials`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns false + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + val i18n = mockContext.i18n + + remoteProvider.handleUri(workspaceLink("https://user:password@OTHER.example.com:8443/path")) + + verify { + i18n.pnotr(match { + it.startsWith("https://other.example.com:8443\n\n") && + it.contains("contains a token") && it.contains("SSH configuration") && + !it.contains("password") && !it.contains("link-token") + }) + } + } + + @Test + fun `links without a valid origin cannot start a connection`() = runTest { + remoteProvider.handleUri(workspaceLink("https://coder.example.com:99999")) + + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + verify { underlyingLogger.error(match { it.contains("valid HTTP(S) host and port") }) } + } + + private fun workspaceLink(url: String): URI = URI( + "jetbrains://gateway/com.coder.toolbox?url=${URLEncoder.encode(url, Charsets.UTF_8)}" + + "&token=link-token&workspace=dev" + ) + @Test fun `given Toolbox 3_5 provider page then header surface is available and account dropdown starts hidden`() { // Toolbox 3.5 hides the whole top section when this flag is false. diff --git a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt index 0c9c3094..c72cae1d 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt @@ -201,12 +201,15 @@ internal class CoderCLIManagerTest { context.logger, ) try { - val url = "https://coder.example.test".toURL() val ws = workspace("diagnostic-workspace", agents = mapOf("main" to UUID.randomUUID().toString())) val agent = ws.latestBuild.resources.flatMap { it.agents.orEmpty() }.first() for (keyring in listOf(true, false)) { settings.updateUseKeyring(keyring) - val cli = CoderCLIManager(context.copy(settingsStore = settings), url, OS.MAC) + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "https://coder.example.test".toURL(), + OS.MAC, + ) for (selectedAgent in listOf(agent, null)) { val output = root.resolve("bundle with spaces.zip") cli.supportBundle(WorkspaceAddress.from(ws, selectedAgent), output) @@ -214,7 +217,7 @@ internal class CoderCLIManagerTest { listOfNotNull( if (!keyring) "--global-config" else null, if (!keyring) cli.coderConfigPath.toString() else null, - "--url", url.toString(), "--use-keyring=$keyring", + "--url", "https://coder.example.test", "--use-keyring=$keyring", "support", "bundle", "--yes", "--output-file", output.toString(), "--", "${ws.ownerName}/${ws.name}", selectedAgent?.name, ), @@ -352,12 +355,10 @@ internal class CoderCLIManagerTest { Environment(), context.logger ) - val url = URL("http://localhost") - - val ccm = CoderCLIManager(context.copy(settingsStore = settings), url) - assertEquals(settings.binSource(url), ccm.remoteBinaryURL) - assertEquals(settings.dataDir(url), ccm.coderConfigPath.parent) - assertEquals(settings.binPath(url), ccm.localBinaryPath) + val ccm = CoderCLIManager(context.copy(settingsStore = settings), "http://localhost".toURL()) + assertEquals(settings.binSource("http://localhost".toURL()), ccm.remoteBinaryURL) + assertEquals(settings.dataDir("http://localhost".toURL()), ccm.coderConfigPath.parent) + assertEquals(settings.binPath("http://localhost".toURL()), ccm.localBinaryPath) } @Test @@ -420,7 +421,6 @@ internal class CoderCLIManagerTest { @Suppress("DEPRECATION") fun `test binaryDirectory fallback to BINARY_DESTINATION`() { val binDir = tmpdir.resolve("bin-dir-fallback") - val url = URL("http://localhost") // Using the deprecated BINARY_DIRECTORY key should still work // because binaryDestination falls back to it. @@ -432,7 +432,7 @@ internal class CoderCLIManagerTest { context.logger ) val expectedOldKey = binDir.resolve("localhost").resolve(settingsWithOldKey.defaultCliBinaryNameByOsAndArch) - assertEquals(expectedOldKey.toAbsolutePath(), settingsWithOldKey.binPath(url)) + assertEquals(expectedOldKey.toAbsolutePath(), settingsWithOldKey.binPath("http://localhost".toURL())) // BINARY_DESTINATION takes priority over BINARY_DIRECTORY. val overrideDir = tmpdir.resolve("bin-dest-override") @@ -446,7 +446,7 @@ internal class CoderCLIManagerTest { ) val expectedNewKey = overrideDir.resolve("localhost").resolve(settingsWithBothKeys.defaultCliBinaryNameByOsAndArch) - assertEquals(expectedNewKey.toAbsolutePath(), settingsWithBothKeys.binPath(url)) + assertEquals(expectedNewKey.toAbsolutePath(), settingsWithBothKeys.binPath("http://localhost".toURL())) } @Test @@ -631,7 +631,7 @@ internal class CoderCLIManagerTest { ) val ccm = CoderCLIManager( context.copy(settingsStore = settings), - URL("https://test.coder.com"), + "https://test.coder.com".toURL(), osOverride, ) @@ -660,7 +660,28 @@ internal class CoderCLIManagerTest { } } - private fun credentialCli(keyring: Boolean = true): Pair { + @Test + fun `HTTP login selects keyring by default and honors opt out across platforms`() = runBlocking { + for (os in listOf(OS.MAC, OS.WINDOWS, OS.LINUX, null)) { + for (setting in listOf(null, true, false)) { + val settings = credentialSettings(keyring = setting) + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "http://test.coder.com".toURL(), + os, + ) + + val args = cli.login("token") + val expectsKeyring = setting != false && CoderCLIManager.supportsKeyringStorage(os) + assertContains(args, "login --use-token-as-session http://test.coder.com") + assertContains(args, "--url http://test.coder.com") + assertContains(args, "--use-keyring=$expectsKeyring") + assertEquals(!expectsKeyring, args.contains("--global-config")) + } + } + } + + private fun credentialSettings(keyring: Boolean? = null): CoderSettingsStore { val root = tmpdir.resolve("credentials-${UUID.randomUUID()}") val binary = root.resolve(if (getOS() == OS.WINDOWS) "coder.bat" else "coder") writeExecutable( @@ -670,24 +691,94 @@ internal class CoderCLIManagerTest { mkbin("printf '%s' \"$*\"") } ) - val settings = CoderSettingsStore( + return CoderSettingsStore( pluginTestSettingsStore( BINARY_DESTINATION to binary.toString(), DATA_DIRECTORY to root.resolve("data").toString(), - USE_KEYRING to keyring.toString(), + *(keyring?.let { arrayOf(USE_KEYRING to it.toString()) } ?: emptyArray()), ), Environment(), underlyingLogger, ) - return CoderCLIManager( - context.copy(settingsStore = settings), - URL("https://test.coder.com"), - OS.MAC - ) to settings + } + + @Test + fun `HTTP uses the keyring across supported CLI versions`() = runBlocking { + for (version in listOf("v2.29.0", "v2.38.0", "v2.39.0")) { + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "http://test.coder.com".toURL(), + OS.MAC, + ) + val script = if (getOS() == OS.WINDOWS) { + "if \"%1\"==\"version\" (echo {\"version\":\"$version\"}) else (echo %*)" + } else { + "if [ \"${'$'}1\" = version ]; then printf '%s' '{\"version\":\"$version\"}'; " + + "else printf '%s' \"${'$'}*\"; fi" + } + writeExecutable(cli.localBinaryPath, mkbin(script)) + val session = cli.coderConfigPath.resolve("session").toFile() + session.parentFile.mkdirs() + session.writeText("old-token") + + val loginArgs = cli.login("token") + assertContains(loginArgs, "--url http://test.coder.com --use-keyring=true", message = version) + assertFalse(loginArgs.contains("--global-config"), version) + assertFalse(session.exists(), "Successful keyring login removes the plaintext token") + val workspace = workspace("dev", agents = mapOf("main" to UUID.randomUUID().toString())) + val agent = workspace.latestBuild.resources.first { !it.agents.isNullOrEmpty() }.agents!!.first() + val address = WorkspaceAddress.from(workspace, agent) + val config = cli.coderConfigPath.parent.resolve("ssh-config") + cli.configSsh(setOf(address), sshConfigPath = config.toString()) + assertContains(Files.readString(config), "--use-keyring=true", message = version) + assertContains(cli.startWorkspace(address), "--use-keyring=true", message = version) + assertContains(cli.logout(), "--url http://test.coder.com --use-keyring=true", message = version) + } + } + + @Test + fun `HTTPS uses the keyring across supported CLI versions`() = runBlocking { + for (version in listOf("v2.29.0", "v2.38.0", "v2.39.0")) { + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "https://test.coder.com".toURL(), + OS.MAC, + ) + val script = if (getOS() == OS.WINDOWS) { + "if \"%1\"==\"version\" (echo {\"version\":\"$version\"}) else (echo %*)" + } else { + "if [ \"${'$'}1\" = version ]; then printf '%s' '{\"version\":\"$version\"}'; " + + "else printf '%s' \"${'$'}*\"; fi" + } + writeExecutable(cli.localBinaryPath, mkbin(script)) + val session = cli.coderConfigPath.resolve("session").toFile() + session.parentFile.mkdirs() + session.writeText("old-token") + + val loginArgs = cli.login("token") + assertContains(loginArgs, "--url https://test.coder.com --use-keyring=true", message = version) + assertFalse(loginArgs.contains("--global-config"), version) + assertFalse(session.exists(), "Successful keyring login removes the plaintext token") + val workspace = workspace("dev", agents = mapOf("main" to UUID.randomUUID().toString())) + val agent = workspace.latestBuild.resources.first { !it.agents.isNullOrEmpty() }.agents!!.first() + val address = WorkspaceAddress.from(workspace, agent) + val config = cli.coderConfigPath.parent.resolve("ssh-config") + cli.configSsh(setOf(address), sshConfigPath = config.toString()) + assertContains(Files.readString(config), "--use-keyring=true", message = version) + assertContains(cli.startWorkspace(address), "--use-keyring=true", message = version) + assertContains(cli.logout(), "--url https://test.coder.com --use-keyring=true", message = version) + } } @Test fun `successful keyring login removes the old plaintext session`() = runBlocking { - val (cli, _) = credentialCli() + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "https://test.coder.com".toURL(), + OS.MAC, + ) val session = cli.coderConfigPath.resolve("session").toFile() session.parentFile.mkdirs() session.writeText("old-token") @@ -699,7 +790,12 @@ internal class CoderCLIManagerTest { @Test fun `failed keyring login preserves the previous plaintext session`() = runBlocking { - val (cli, _) = credentialCli() + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "https://test.coder.com".toURL(), + OS.MAC, + ) writeExecutable(cli.localBinaryPath, mkbin(if (getOS() == OS.WINDOWS) "exit /b 1" else "exit 1")) val session = cli.coderConfigPath.resolve("session").toFile() session.parentFile.mkdirs() @@ -712,7 +808,12 @@ internal class CoderCLIManagerTest { @Test fun `logout and token refresh retain active storage until the next sign in`() = runBlocking { - val (cli, settings) = credentialCli() + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + "https://test.coder.com".toURL(), + OS.MAC, + ) settings.updateUseKeyring(false) assertContains(cli.login("new-token"), "--use-keyring=true") diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt new file mode 100644 index 00000000..e7eb6914 --- /dev/null +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt @@ -0,0 +1,112 @@ +package com.coder.toolbox.store + +import com.coder.toolbox.util.toURL +import com.jetbrains.toolbox.api.core.PluginSecretStore +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull + +class CoderSecretsStoreTest { + private val values = mutableMapOf() + private val backing = object : PluginSecretStore { + override fun get(key: String): String? = values[key] + override fun set(key: String, value: String) { + values[key] = value + } + + override fun clear(key: String) { + values.remove(key) + } + } + private val secrets = CoderSecretsStore(backing) + + @Test + fun `saved API tokens are reused only for the same origin after restarting`() { + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "token") + val reopened = CoderSecretsStore(backing) + + for (url in listOf("https://coder.example.com/", "https://CODER.example.com:443/path")) { + assertEquals("token", reopened.apiTokenFor(url.toURL()), url) + } + for (url in listOf("http://coder.example.com", "https://coder.example.com:8443", "https://other.example.com")) { + assertNull(reopened.apiTokenFor(url.toURL()), url) + } + } + + @Test + fun `HTTP default ports IPv6 and international hostnames use canonical origins`() { + for ((original, equivalent) in listOf( + "http://coder.example.com" to "http://coder.example.com:80/", + "https://[::1]:8443" to "https://[0:0:0:0:0:0:0:1]:8443/", + "https://ほげ.example" to "https://xn--18j4d.example:443/", + )) { + secrets.storeApiTokenFor(original.toURL(), "token") + assertEquals("token", secrets.apiTokenFor(equivalent.toURL()), original) + } + } + + @Test + fun `hostname-only credentials are ignored and preserved`() { + values["coder.example.com"] = "old-token" + values["last-deployment-url"] = "https://coder.example.com" + + assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) + assertNull(secrets.apiTokenFor("http://coder.example.com".toURL())) + + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "new-token") + assertEquals("new-token", secrets.apiTokenFor("https://coder.example.com".toURL())) + assertEquals("old-token", values["coder.example.com"]) + + secrets.clearSessionFor("https://coder.example.com".toURL()) + assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) + assertEquals("old-token", values["coder.example.com"]) + } + + @Test + fun `sessions on the same host with different schemes and ports coexist`() { + secrets.storeApiTokenFor("http://coder.example.com".toURL(), "http-token") + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "https-token") + secrets.storeApiTokenFor("https://coder.example.com:8443".toURL(), "other-port-token") + val reopened = CoderSecretsStore(backing) + + assertEquals("http-token", reopened.apiTokenFor("http://coder.example.com".toURL())) + assertEquals("https-token", reopened.apiTokenFor("https://coder.example.com".toURL())) + assertEquals("other-port-token", reopened.apiTokenFor("https://coder.example.com:8443".toURL())) + + reopened.storeApiTokenFor("https://CODER.example.com:443/".toURL(), "refreshed-token") + + assertEquals("http-token", secrets.apiTokenFor("http://coder.example.com".toURL())) + assertEquals("refreshed-token", secrets.apiTokenFor("https://coder.example.com".toURL())) + assertEquals("other-port-token", secrets.apiTokenFor("https://coder.example.com:8443".toURL())) + } + + @Test + fun `logout removes only the matching origin session`() { + secrets.storeApiTokenFor("http://coder.example.com".toURL(), "http-token") + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "https-token") + secrets.storeApiTokenFor("https://coder.example.com:8443".toURL(), "other-port-token") + + secrets.clearSessionFor("https://CODER.example.com:443/path".toURL()) + + assertEquals("http-token", secrets.apiTokenFor("http://coder.example.com".toURL())) + assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) + assertEquals("other-port-token", secrets.apiTokenFor("https://coder.example.com:8443".toURL())) + } + + @Test + fun `URLs without a valid HTTP origin cannot read or store tokens`() { + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "existing-token") + + assertNull(secrets.apiTokenFor("file:///tmp/coder".toURL())) + assertNull(secrets.apiTokenFor("https://coder.example.com:99999".toURL())) + assertFailsWith { + secrets.storeApiTokenFor("file:///tmp/coder".toURL(), "invalid-token") + } + assertFailsWith { + secrets.storeApiTokenFor("https://coder.example.com:99999".toURL(), "invalid-token") + } + + assertEquals("existing-token", secrets.apiTokenFor("https://coder.example.com".toURL())) + } +} diff --git a/src/test/kotlin/com/coder/toolbox/util/URLExtensionsTest.kt b/src/test/kotlin/com/coder/toolbox/util/URLExtensionsTest.kt index 7bad0dcd..01e303fd 100644 --- a/src/test/kotlin/com/coder/toolbox/util/URLExtensionsTest.kt +++ b/src/test/kotlin/com/coder/toolbox/util/URLExtensionsTest.kt @@ -5,8 +5,25 @@ import java.net.URL import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue internal class URLExtensionsTest { + @Test + fun `origins compare scheme canonical hostname and effective port`() { + val url = "https://coder.example.com".toURL() + assertTrue(url.hasSameOrigin("https://CODER.example.com:443/path".toURL())) + assertFalse(url.hasSameOrigin("http://coder.example.com".toURL())) + assertFalse(url.hasSameOrigin("https://coder.example.com:8443".toURL())) + assertFalse(url.hasSameOrigin("https://other.example.com".toURL())) + assertFalse(url.hasSameOrigin(null)) + assertFalse("file:///tmp".toURL().hasSameOrigin("file:///tmp".toURL())) + assertEquals( + "https://coder.example.com", + "https://user:password@CODER.example.com:443/path?q=1#fragment".toURL().origin() + ) + } + @Test fun testToURL() { assertEquals( From ab47dc441d4a150cf00420d3901e9869b63a5002 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Sat, 7 Nov 2026 00:47:47 +0200 Subject: [PATCH 2/6] Revert unrelated changelog formatting changes The changelog mixes the security fixes with unrelated changes to existing Unreleased entries and an added format introduction, which expands the scope of the security review. This commit restores the original headings, wording, and formatting and removes the introduction. The Security section remains intact so the broader changelog cleanup can be reviewed separately. --- CHANGELOG.md | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eebf579..177916b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,18 +1,16 @@ # Coder Toolbox Plugin Changelog -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - ## Unreleased -### Changed +### Breaking changes -- Stream build status and provisioner output while updating, restarting, or stopping workspaces, falling back to - polling when the WebSocket connection is unavailable. +- drop support for Coder server and CLI versions older than v2.29.0. Connecting to an older deployment, or with an + older CLI binary, now stops with a message asking to upgrade to v2.29.0 or newer -### Removed +### Changed -- **Breaking:** Support for Coder server and CLI versions older than v2.29.0. Connecting to an older deployment, or - with an older CLI binary, now stops with a message asking to upgrade to v2.29.0 or newer. +- stream build status and provisioner output while updating, restarting, or stopping workspaces, falling back to + polling when the WebSocket connection is unavailable ### Fixed From 2df2d968ceda334dca0aebc8482c33c04c8d5a24 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Sat, 7 Nov 2026 01:01:35 +0200 Subject: [PATCH 3/6] Parameterize CLI keyring tests across URLs and versions The CLI manager tests duplicate HTTP and HTTPS keyring checks and loop over CLI versions inside each test. A failure stops the remaining versions in that loop, and the results are reported as only two tests. This commit replaces the duplicated checks with six named JUnit cases. Each URL and version pair runs independently, while the test data keeps explicit URL literals. The build includes the parameterized test module aligned with the existing JUnit version. --- build.gradle.kts | 1 + gradle/libs.versions.toml | 1 + .../coder/toolbox/cli/CoderCLIManagerTest.kt | 108 +++++++----------- 3 files changed, 44 insertions(+), 66 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index f7825c0f..0218a2ba 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -46,6 +46,7 @@ dependencies { implementation(libs.bundles.bouncycastle) testImplementation(kotlin("test")) + testImplementation(libs.junit.jupiter.params) testImplementation(libs.coroutines.test) testImplementation(libs.mokk) testImplementation(libs.bundles.toolbox.plugin.api) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 6c992edb..c777f8bb 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -35,6 +35,7 @@ retrofit = { module = "com.squareup.retrofit2:retrofit", version.ref = "retrofit retrofit-moshi = { module = "com.squareup.retrofit2:converter-moshi", version.ref = "retrofit" } plugin-structure = { module = "org.jetbrains.intellij.plugins:structure-toolbox", version.ref = "plugin-structure" } mokk = { module = "io.mockk:mockk", version.ref = "mockk" } +junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params" } marketplace-client = { module = "org.jetbrains.intellij:plugin-repository-rest-client", version.ref = "marketplace-client" } bouncycastle-bcpg = { module = "org.bouncycastle:bcpg-jdk18on", version.ref = "bouncycastle" } bouncycastle-bcprov = { module = "org.bouncycastle:bcprov-jdk18on", version.ref = "bouncycastle" } diff --git a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt index c72cae1d..02adc5c1 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt @@ -67,6 +67,8 @@ 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.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource import java.net.HttpURLConnection import java.net.InetSocketAddress import java.net.Proxy @@ -701,74 +703,48 @@ internal class CoderCLIManagerTest { ) } - @Test - fun `HTTP uses the keyring across supported CLI versions`() = runBlocking { - for (version in listOf("v2.29.0", "v2.38.0", "v2.39.0")) { - val settings = credentialSettings() - val cli = CoderCLIManager( - context.copy(settingsStore = settings), - "http://test.coder.com".toURL(), - OS.MAC, - ) - val script = if (getOS() == OS.WINDOWS) { - "if \"%1\"==\"version\" (echo {\"version\":\"$version\"}) else (echo %*)" - } else { - "if [ \"${'$'}1\" = version ]; then printf '%s' '{\"version\":\"$version\"}'; " + - "else printf '%s' \"${'$'}*\"; fi" - } - writeExecutable(cli.localBinaryPath, mkbin(script)) - val session = cli.coderConfigPath.resolve("session").toFile() - session.parentFile.mkdirs() - session.writeText("old-token") - - val loginArgs = cli.login("token") - assertContains(loginArgs, "--url http://test.coder.com --use-keyring=true", message = version) - assertFalse(loginArgs.contains("--global-config"), version) - assertFalse(session.exists(), "Successful keyring login removes the plaintext token") - val workspace = workspace("dev", agents = mapOf("main" to UUID.randomUUID().toString())) - val agent = workspace.latestBuild.resources.first { !it.agents.isNullOrEmpty() }.agents!!.first() - val address = WorkspaceAddress.from(workspace, agent) - val config = cli.coderConfigPath.parent.resolve("ssh-config") - cli.configSsh(setOf(address), sshConfigPath = config.toString()) - assertContains(Files.readString(config), "--use-keyring=true", message = version) - assertContains(cli.startWorkspace(address), "--use-keyring=true", message = version) - assertContains(cli.logout(), "--url http://test.coder.com --use-keyring=true", message = version) + @CsvSource( + "http://test.coder.com, v2.29.0", + "http://test.coder.com, v2.38.0", + "http://test.coder.com, v2.39.0", + "https://test.coder.com, v2.29.0", + "https://test.coder.com, v2.38.0", + "https://test.coder.com, v2.39.0", + ) + @ParameterizedTest(name = "{0} uses the keyring with CLI {1}") + fun `HTTP and HTTPS use the keyring across supported CLI versions`( + url: String, + version: String, + ) = runBlocking { + val settings = credentialSettings() + val cli = CoderCLIManager( + context.copy(settingsStore = settings), + url.toURL(), + OS.MAC, + ) + val script = if (getOS() == OS.WINDOWS) { + "if \"%1\"==\"version\" (echo {\"version\":\"$version\"}) else (echo %*)" + } else { + "if [ \"${'$'}1\" = version ]; then printf '%s' '{\"version\":\"$version\"}'; " + + "else printf '%s' \"${'$'}*\"; fi" } - } + writeExecutable(cli.localBinaryPath, mkbin(script)) + val session = cli.coderConfigPath.resolve("session").toFile() + session.parentFile.mkdirs() + session.writeText("old-token") - @Test - fun `HTTPS uses the keyring across supported CLI versions`() = runBlocking { - for (version in listOf("v2.29.0", "v2.38.0", "v2.39.0")) { - val settings = credentialSettings() - val cli = CoderCLIManager( - context.copy(settingsStore = settings), - "https://test.coder.com".toURL(), - OS.MAC, - ) - val script = if (getOS() == OS.WINDOWS) { - "if \"%1\"==\"version\" (echo {\"version\":\"$version\"}) else (echo %*)" - } else { - "if [ \"${'$'}1\" = version ]; then printf '%s' '{\"version\":\"$version\"}'; " + - "else printf '%s' \"${'$'}*\"; fi" - } - writeExecutable(cli.localBinaryPath, mkbin(script)) - val session = cli.coderConfigPath.resolve("session").toFile() - session.parentFile.mkdirs() - session.writeText("old-token") - - val loginArgs = cli.login("token") - assertContains(loginArgs, "--url https://test.coder.com --use-keyring=true", message = version) - assertFalse(loginArgs.contains("--global-config"), version) - assertFalse(session.exists(), "Successful keyring login removes the plaintext token") - val workspace = workspace("dev", agents = mapOf("main" to UUID.randomUUID().toString())) - val agent = workspace.latestBuild.resources.first { !it.agents.isNullOrEmpty() }.agents!!.first() - val address = WorkspaceAddress.from(workspace, agent) - val config = cli.coderConfigPath.parent.resolve("ssh-config") - cli.configSsh(setOf(address), sshConfigPath = config.toString()) - assertContains(Files.readString(config), "--use-keyring=true", message = version) - assertContains(cli.startWorkspace(address), "--use-keyring=true", message = version) - assertContains(cli.logout(), "--url https://test.coder.com --use-keyring=true", message = version) - } + val loginArgs = cli.login("token") + assertContains(loginArgs, "--url $url --use-keyring=true") + assertFalse(loginArgs.contains("--global-config")) + assertFalse(session.exists(), "Successful keyring login removes the plaintext token") + val workspace = workspace("dev", agents = mapOf("main" to UUID.randomUUID().toString())) + val agent = workspace.latestBuild.resources.first { !it.agents.isNullOrEmpty() }.agents!!.first() + val address = WorkspaceAddress.from(workspace, agent) + val config = cli.coderConfigPath.parent.resolve("ssh-config") + cli.configSsh(setOf(address), sshConfigPath = config.toString()) + assertContains(Files.readString(config), "--use-keyring=true") + assertContains(cli.startWorkspace(address), "--use-keyring=true") + assertContains(cli.logout(), "--url $url --use-keyring=true") } @Test From a3a69e196e75f2d3c0fa53c8f1707e8ad6232a93 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Tue, 10 Nov 2026 00:53:51 +0200 Subject: [PATCH 4/6] Remember trusted deployments and confirm account changes The Toolbox link handler only recognizes the active deployment, so it asks users to trust a familiar deployment again after a restart. A link to the active deployment can also replace the session without checking which account owns the new token. The secrets store now remembers successful sign-ins. The link handler selects the deployment by scheme, host, and port and compares the account behind the link token with the active or saved account. Toolbox asks for confirmation when the deployment is unknown, the accounts differ, or it cannot verify the saved account. --- CHANGELOG.md | 5 +- .../com/coder/toolbox/CoderRemoteProvider.kt | 361 +++++++++------- .../coder/toolbox/store/CoderSecretsStore.kt | 55 ++- .../com/coder/toolbox/util/URLExtensions.kt | 21 +- .../coder/toolbox/CoderRemoteProviderTest.kt | 406 ++++++++++++++++-- .../toolbox/store/CoderSecretsStoreTest.kt | 99 +++++ 6 files changed, 756 insertions(+), 191 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 177916b2..3f69ca14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,9 @@ ### Security -- Require confirmation before a workspace link connects to a different Coder deployment or starts a fresh connection. - Links to the currently connected deployment continue without a prompt. +- Ask for confirmation before a workspace link opens an unknown Coder deployment. Links select the deployment by + scheme, host, and port. For a known deployment, Toolbox asks before using a link token if the accounts differ or + it cannot verify the saved account. - Store Toolbox API tokens separately for each deployment scheme, host, and port to prevent cross-deployment reuse. Tokens saved by older plugin versions require one new sign-in because they were stored without an origin. - Prevent delayed OAuth refreshes from updating CLI credentials for a different deployment. diff --git a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt index b5aaebb0..e7610993 100644 --- a/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt +++ b/src/main/kotlin/com/coder/toolbox/CoderRemoteProvider.kt @@ -9,6 +9,7 @@ import com.coder.toolbox.sdk.CoderRestClient import com.coder.toolbox.sdk.ex.APIResponseException import com.coder.toolbox.sdk.ex.OAuthTokenResponseException import com.coder.toolbox.sdk.v2.models.InvalidCoderIdentifierException +import com.coder.toolbox.sdk.v2.models.User import com.coder.toolbox.sdk.v2.models.WorkspaceStatus import com.coder.toolbox.session.SessionId import com.coder.toolbox.util.CoderProtocolHandler @@ -50,6 +51,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.update @@ -57,6 +59,7 @@ import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.selects.onTimeout import kotlinx.coroutines.selects.select +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import java.net.URI import java.net.URL import java.nio.file.Path @@ -498,74 +501,26 @@ class CoderRemoteProvider( */ override suspend fun handleUri(uri: URI) { try { - if (uri.toString().startsWith("jetbrains://gateway/com.coder.toolbox/auth")) { - handleOAuthUri(uri) - return - } - val params = uri.toQueryParameters() - if (params.isEmpty()) { - // probably a plugin installation scenario - context.logger.logAndShowInfo("URI will not be handled", "No query parameters were provided") - return - } - val newUrl = resolveDeploymentUrl(params)?.toURL() ?: return - val newOrigin = requireNotNull(newUrl.origin()) { - "The deployment URL must have a valid HTTP(S) host and port." - } - val newToken = if (context.settingsStore.requiresMTlsAuth) null else resolveToken(params) ?: return - context.logger.info("Handling workspace link for $newOrigin") - val activeUrl = client?.url?.takeIf { newUrl.hasSameOrigin(it) } - if (activeUrl != null) { - coderHeaderPage.isBusy.update { true } - try { - val activeSession = if (context.settingsStore.requiresTokenAuth) { - newToken?.let { - refreshSession(activeUrl, it) - } ?: (this.client!! to this.cli!!) - } else { - this.client!! to this.cli!! - } - handleLink(params, activeUrl, activeSession.first, activeSession.second) - } finally { - coderHeaderPage.isBusy.update { false } - } - } else { - if (!confirmDeployment(newOrigin, newToken != null)) return - // Different URL - we need a new connection. Tear down any - // in-flight wizard, install a fresh one on the router, and let - // showPluginEnvironmentsPage() pull it through getOverrideUiPage. - val credentials = newToken?.let { Credentials.Token(it) } ?: Credentials.MTls - val wizard = CoderSetupWizardPage.connectStep( - context, settingsPage, - url = newUrl, - credentials = credentials, - onConnect = onConnect.andThen(deferredLinkHandler(params, newUrl)), - onTokenRefreshed = ::onTokenRefreshed, - ) - router.navigate(wizard) - context.popupPluginMainPage() + when { + uri.toString().startsWith("jetbrains://gateway/com.coder.toolbox/auth") -> handleOAuthUri(params) + // An empty query can indicate plugin installation. + params.isEmpty() -> + context.logger.logAndShowInfo("URI will not be handled", "No query parameters were provided") + + else -> handleWorkspaceUri(params) } } catch (ex: CancellationException) { throw ex } catch (ex: Exception) { - val textError = if (ex is APIResponseException) { - if (!ex.reason.isNullOrBlank()) { - ex.reason - } else ex.message - } else ex.message - context.logger.logAndShowError( - "Error encountered while handling Coder URI", - textError ?: "" - ) + val reason = (ex as? APIResponseException)?.reason?.takeIf { it.isNotBlank() } + context.logger.logAndShowError("Error encountered while handling Coder URI", reason ?: ex.message.orEmpty()) } finally { firstRun = false } } - private suspend fun handleOAuthUri(uri: URI) { - val params = uri.toQueryParameters() - + private suspend fun handleOAuthUri(params: Map) { // RFC 6749 §4.1.2.1 (also covers RFC 7636 §4.4.1 PKCE errors): the authorization // server redirects back with `error` when authorization fails (e.g. access_denied, // invalid_request, unsupported_response_type, server_error, ...). @@ -634,7 +589,35 @@ class CoderRemoteProvider( } } - private suspend fun resolveDeploymentUrl(params: Map): String? { + internal suspend fun onTokenRefreshed(url: URL, oauthSessionCtx: CoderOAuthSessionContext) { + // A refresh may finish after the user has connected to another deployment. + val activeCli = cli?.takeIf { + client?.url?.hasSameOrigin(url) == true && it.deploymentURL.hasSameOrigin(url) + } + oauthSessionCtx.tokenResponse?.accessToken?.let { activeCli?.login(it) } + context.secrets.storeOAuthFor(url.toString(), oauthSessionCtx) + } + + private suspend fun handleWorkspaceUri(params: Map) { + val deployment = resolveDeploymentParameters(params) ?: return + context.logger.info("Handling workspace link for ${deployment.url}") + val activeClient = client?.takeIf { it.url.hasSameOrigin(deployment.url) } + if (!confirmDeploymentSignIn(deployment, activeClient)) return + + if (activeClient != null) { + openLinkInActiveSession(params, activeClient, deployment.token) + } else { + connectFromLink(params, deployment.url, deployment.token) + } + } + + private suspend fun resolveDeploymentParameters(params: Map): ResolvedDeploymentParameters? { + val url = resolveDeploymentUrl(params) ?: return null + val token = if (context.settingsStore.requiresMTlsAuth) null else resolveToken(params) ?: return null + return ResolvedDeploymentParameters(url, token) + } + + private suspend fun resolveDeploymentUrl(params: Map): URL? { val deploymentURL = params.url() ?: askUrl() if (deploymentURL.isNullOrBlank()) { context.logger.logAndShowError(CAN_T_HANDLE_URI_TITLE, "Query parameter \"${URL}\" is missing from URI") @@ -645,7 +628,19 @@ class CoderRemoteProvider( context.logger.logAndShowError(CAN_T_HANDLE_URI_TITLE, "\"$URL\" is invalid: ${validationResult.reason}") return null } - return deploymentURL + // Workspace links select a deployment by scheme, host, and port. + val origin = requireNotNull(deploymentURL.toHttpUrlOrNull()?.origin()) { + "The deployment URL must have a valid HTTP(S) host and port." + } + return origin.toURL() + } + + private suspend fun askUrl(): String? { + context.popupPluginMainPage() + return dialogUi.ask( + context.i18n.ptrl("Deployment URL"), + context.i18n.ptrl("Enter the full URL of your Coder deployment") + ) } private fun resolveToken(params: Map): String? { @@ -657,6 +652,17 @@ class CoderRemoteProvider( return token } + private suspend fun confirmDeploymentSignIn( + deployment: ResolvedDeploymentParameters, + activeClient: CoderRestClient?, + ): Boolean { + if (activeClient == null && !context.secrets.isUrlTrusted(deployment.url)) { + return confirmDeployment(deployment.url.toString(), deployment.token != null) + } + val token = deployment.token ?: return true + return confirmTokenSignIn(activeClient?.url ?: deployment.url, token, activeClient?.me) + } + private suspend fun confirmDeployment(origin: String, hasToken: Boolean): Boolean { val disclosure = if (hasToken) { " This link contains a token that will be sent to this deployment to sign you in." @@ -665,7 +671,7 @@ class CoderRemoteProvider( return context.ui.showYesNoPopup( context.i18n.ptrl("Connect to a Coder deployment?"), context.i18n.pnotr( - "$origin\n\nToolbox is not currently connected to this deployment.$disclosure " + + "$origin\n\nYou have not signed in to this deployment in Toolbox before.$disclosure " + "Continue only if you trust it. Connecting can update your SSH configuration." ), context.i18n.ptrl("Trust and Continue"), @@ -673,22 +679,92 @@ class CoderRemoteProvider( ) } + private suspend fun confirmTokenSignIn(url: URL, token: String, activeUser: User?): Boolean { + val linkUser = requireNotNull(userForToken(url, token)) { "The token in the link is invalid or expired." } + val previousUser = activeUser ?: (autoSetupCredentials(url) as? Credentials.Token)?.let { saved -> + if (saved.value == token) linkUser else userForToken(url, saved.value) + } + if (linkUser.id == previousUser?.id) return true + val previousSession = previousUser?.let { "your saved session for \"${it.username}\"" } ?: "your saved session" + context.ui.showWindowSuspending() + return context.ui.showYesNoPopup( + context.i18n.ptrl("Sign in with the token from the link?"), + context.i18n.pnotr( + "${url.origin()}\n\nThis link signs you in as \"${linkUser.username}\", " + + "replacing $previousSession. Continue only if you intended to use this account." + ), + context.i18n.ptrl("Sign In"), + context.i18n.ptrl("Cancel"), + ) + } + + private suspend fun userForToken(url: URL, token: String): User? { + val candidate = CoderRestClient(context, url, token, pluginVersion = PluginManager.pluginInfo.version) + return try { + candidate.me() + } catch (ex: APIResponseException) { + if (!ex.isUnauthorized) throw ex + null + } finally { + candidate.close() + } + } + + private fun autoSetupCredentials(url: URL): Credentials? { + if (context.settingsStore.requiresMTlsAuth) return Credentials.MTls + + val tokenCredentials = context.secrets.apiTokenFor(url) + ?.takeIf { it.isNotBlank() } + ?.let { Credentials.Token(it) } + + if (!context.settingsStore.preferOAuth2IfAvailable) return tokenCredentials + + return context.secrets.oauthSessionFor(url.toString())?.let { + Credentials.OAuth(it.toSessionContext()) + } ?: tokenCredentials + } + + private suspend fun openLinkInActiveSession( + params: Map, + activeClient: CoderRestClient, + token: String?, + ) { + coderHeaderPage.isBusy.update { true } + try { + val (sessionClient, sessionCli) = if (context.settingsStore.requiresTokenAuth && token != null) { + refreshSession(activeClient.url, token) + } else { + activeClient to checkNotNull(cli) { "The active Coder connection has no CLI." } + } + handleLink(params, activeClient.url, sessionClient, sessionCli) + } finally { + coderHeaderPage.isBusy.update { false } + } + } + private suspend fun refreshSession(url: URL, token: String): Pair { context.logger.info( lastEnvironments.currentSessionIds(), - "Stopping workspace polling and re-initializing the http client and cli with a new token", + "Preparing the HTTP client and CLI with the token from the workspace link", ) - softClose() val newRestClient = CoderRestClient( context, url, token, null, PluginManager.pluginInfo.version, - ).apply { initializeSession() } - val newCli = CoderCLIManager(context, url).apply { - login(token) + ) + val newCli = try { + newRestClient.initializeSession() + CoderCLIManager(context, url).apply { + login(token) + storeSession(newRestClient) + } + } catch (ex: Exception) { + newRestClient.close() + throw ex } + softClose() this.client = newRestClient this.cli = newCli lastEnvironments.forEach { it.updateClientAndCli(newRestClient, newCli) } @@ -704,12 +780,73 @@ class CoderRemoteProvider( return newRestClient to newCli } - private suspend fun askUrl(): String? { - context.popupPluginMainPage() - return dialogUi.ask( - context.i18n.ptrl("Deployment URL"), - context.i18n.ptrl("Enter the full URL of your Coder deployment") + private fun storeSession(client: CoderRestClient) { + context.settingsStore.updateLastUsedUrl(client.url) + if (context.settingsStore.requiresTokenAuth) { + client.token?.let { context.secrets.storeApiTokenFor(client.url, it) } + } + context.secrets.trustDeployment(client.url) + context.logger.info("Deployment session was stored and will be available for automatic connection") + } + + /** + * Applies the appropriate workspace filter for the URI then delegates to [linkHandler]. + * Scopes the filter to the target workspace when it is owned by a different user so the poll + * fetches and registers it before the IDE launch runs. For own workspaces, resets to the + * default filter, clearing any leftover non-owned filter from a previous URI. + */ + private suspend fun handleLink( + params: Map, + url: URL, + client: CoderRestClient, + cli: CoderCLIManager, + ) { + val uriOwner = params.owner() + val uriWorkspace = params.workspace() + if (!uriOwner.isNullOrBlank() && !uriWorkspace.isNullOrBlank() && uriOwner != client.me.username) { + coderHeaderPage.setFilter("owner:$uriOwner name:$uriWorkspace") + } else { + coderHeaderPage.resetFilter() + } + linkHandler.handle(params, url, client, cli) + } + + private fun connectFromLink(params: Map, url: URL, token: String?) { + val credentials = token?.let { Credentials.Token(it) } ?: Credentials.MTls + val wizard = CoderSetupWizardPage.connectStep( + context, settingsPage, + url = url, + credentials = credentials, + onConnect = onConnect.andThen(deferredLinkHandler(params, url)), + onTokenRefreshed = ::onTokenRefreshed, ) + // The router tears down any in-flight wizard and installs the new one before + // showPluginEnvironmentsPage() asks getOverrideUiPage() for the page to display. + router.navigate(wizard) + context.popupPluginMainPage() + } + + /** Returns a [SuspendBiConsumer] that handles the given link parameters in a background coroutine. */ + private fun deferredLinkHandler( + params: Map, + deploymentUrl: URL, + ): SuspendBiConsumer = SuspendBiConsumer { client, cli -> + context.cs.launch(CoroutineName("Deferred Link Handler")) { + coderHeaderPage.isBusy.update { true } + try { + handleLink(params, deploymentUrl, client, cli) + } catch (ex: Exception) { + if (ex is kotlinx.coroutines.CancellationException) { + ensureActive() + } + context.logger.logAndShowError( + "Error handling deferred link", + ex.message ?: "" + ) + } finally { + coderHeaderPage.isBusy.update { false } + } + } } /** @@ -777,41 +914,10 @@ class CoderRemoteProvider( fun canAutoLogin(): Boolean = autoSetupCredentials(context.deploymentUrl) != null - private fun autoSetupCredentials(url: URL): Credentials? { - if (context.settingsStore.requiresMTlsAuth) return Credentials.MTls - - val tokenCredentials = context.secrets.apiTokenFor(url) - ?.takeIf { it.isNotBlank() } - ?.let { Credentials.Token(it) } - - if (!context.settingsStore.preferOAuth2IfAvailable) return tokenCredentials - - return context.secrets.oauthSessionFor(url.toString())?.let { - Credentials.OAuth(it.toSessionContext()) - } ?: tokenCredentials - } - - internal suspend fun onTokenRefreshed(url: URL, oauthSessionCtx: CoderOAuthSessionContext) { - // A refresh may finish after the user has connected to another deployment. - val activeCli = cli?.takeIf { - client?.url?.hasSameOrigin(url) == true && it.deploymentURL.hasSameOrigin(url) - } - oauthSessionCtx.tokenResponse?.accessToken?.let { activeCli?.login(it) } - context.secrets.storeOAuthFor(url.toString(), oauthSessionCtx) - } - private val onConnect: SuspendBiConsumer = SuspendBiConsumer { client, cli -> // Store the URL and token for use next time. + storeSession(client) close() - context.settingsStore.updateLastUsedUrl(client.url) - if (context.settingsStore.requiresTokenAuth) { - if (client.token != null) { - context.secrets.storeApiTokenFor(client.url, client.token) - } - context.logger.info("Deployment URL and token were stored and will be available for automatic connection") - } else { - context.logger.info("Deployment URL was stored and will be available for automatic connection") - } this.client = client this.cli = cli lastEnvironments.forEach { it.updateClientAndCli(client, cli) } @@ -833,48 +939,6 @@ class CoderRemoteProvider( context.logger.info("Workspace poll job with name ${pollJob.toString()} was created") } - /** - * Applies the appropriate workspace filter for the URI then delegates to [linkHandler]. - * Scopes the filter to the target workspace when it is owned by a different user so the poll - * fetches and registers it before the IDE launch runs. For own workspaces, resets to the - * default filter, clearing any leftover non-owned filter from a previous URI. - */ - private suspend fun handleLink( - params: Map, - url: URL, - client: CoderRestClient, - cli: CoderCLIManager, - ) { - val uriOwner = params.owner() - val uriWorkspace = params.workspace() - if (!uriOwner.isNullOrBlank() && !uriWorkspace.isNullOrBlank() && uriOwner != client.me.username) { - coderHeaderPage.setFilter("owner:$uriOwner name:$uriWorkspace") - } else { - coderHeaderPage.resetFilter() - } - linkHandler.handle(params, url, client, cli) - } - - /** Returns a [SuspendBiConsumer] that handles the given link parameters in a background coroutine. */ - private fun deferredLinkHandler( - params: Map, - deploymentUrl: URL, - ): SuspendBiConsumer = SuspendBiConsumer { client, cli -> - context.cs.launch(CoroutineName("Deferred Link Handler")) { - coderHeaderPage.isBusy.update { true } - try { - handleLink(params, deploymentUrl, client, cli) - } catch (ex: Exception) { - context.logger.logAndShowError( - "Error handling deferred link", - ex.message ?: "" - ) - } finally { - coderHeaderPage.isBusy.update { false } - } - } - } - private fun MutableStateFlow>>.showLoadingMessage() { this.update { LoadableState.Loading @@ -884,3 +948,10 @@ class CoderRemoteProvider( private fun Throwable.hasInvalidCoderIdentifierCause(): Boolean = this is InvalidCoderIdentifierException || cause is InvalidCoderIdentifierException + +private data class ResolvedDeploymentParameters( + val url: URL, + val token: String?, +) { + override fun toString(): String = "ResolvedDeploymentParameters(url=$url)" +} diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt index a90a7c53..67532270 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt @@ -5,6 +5,7 @@ import com.coder.toolbox.util.origin import com.coder.toolbox.views.state.CoderOAuthSessionContext import com.coder.toolbox.views.state.StoredOAuthSession import com.jetbrains.toolbox.api.core.PluginSecretStore +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import java.net.URL private const val OAUTH_CLIENT_ID_PREFIX = "oauth-client-id" @@ -33,8 +34,39 @@ class CoderSecretsStore(private val store: PluginSecretStore) { private fun apiSessionKey(url: URL): String? = url.origin()?.let { "api-session:$it" } + fun isUrlTrusted(url: URL): Boolean { + val requested = url.toString().toHttpUrlOrNull() ?: return false + val origin = requested.origin() + val saved = store["deployment-url:$origin"] + return if (saved != null) { + saved.toHttpUrlOrNull() == requested + } else { + // Older origin-bound tokens have no record of a deployment path. + requested.toString() == "$origin/" && !apiTokenFor(url).isNullOrBlank() + } + } + + fun trustDeployment(url: URL) { + val address = requireNotNull(url.toString().toHttpUrlOrNull()) { "Deployment trust requires an HTTP(S) URL" } + val key = "deployment-url:${address.origin()}" + if (store[key]?.toHttpUrlOrNull() != address) store[key] = url.toString() + } + fun clearSessionFor(url: URL) { apiSessionKey(url)?.let { store.clear(it) } + url.origin()?.let { origin -> + val key = "deployment-url:$origin" + val signedInAt = store[key] + val savedOrigin = signedInAt?.toHttpUrlOrNull()?.origin() + if (signedInAt != null && signedInAt != url.toString() && savedOrigin == origin) { + clearOAuthSession(signedInAt) + } + store.clear(key) + } + clearOAuthSession(url.toString()) + } + + private fun clearOAuthSession(url: String) { listOf( OAUTH_CLIENT_ID_PREFIX, OAUTH_CLIENT_SECRET_PREFIX, OAUTH_REFRESH_TOKEN, OAUTH_TOKEN_AUTH_METHOD, OAUTH_TOKEN_ENDPOINT @@ -44,6 +76,18 @@ class CoderSecretsStore(private val store: PluginSecretStore) { } fun oauthSessionFor(url: String): StoredOAuthSession? { + val address = oauthStorageAddress(url) + return readOAuthSession(address) ?: if (address != url) readOAuthSession(url) else null + } + + private fun oauthStorageAddress(url: String): String { + // Existing OAuth keys use the exact spelling of the URL used to sign in. + val requested = url.toHttpUrlOrNull() ?: return url + val saved = store["deployment-url:${requested.origin()}"] + return saved?.takeIf { it.toHttpUrlOrNull() == requested } ?: url + } + + private fun readOAuthSession(url: String): StoredOAuthSession? { val clientId = store["$OAUTH_CLIENT_ID_PREFIX-$url"] val clientSecret = store["$OAUTH_CLIENT_SECRET_PREFIX-$url"] val refreshToken = store["$OAUTH_REFRESH_TOKEN-$url"] @@ -64,11 +108,12 @@ class CoderSecretsStore(private val store: PluginSecretStore) { fun storeOAuthFor(url: String, oAuthSessionCtx: CoderOAuthSessionContext) { oAuthSessionCtx.tokenResponse?.refreshToken?.let { refreshToken -> - store["$OAUTH_CLIENT_ID_PREFIX-$url"] = oAuthSessionCtx.clientId - store["$OAUTH_CLIENT_SECRET_PREFIX-$url"] = oAuthSessionCtx.clientSecret - store["$OAUTH_REFRESH_TOKEN-$url"] = refreshToken - store["$OAUTH_TOKEN_AUTH_METHOD-$url"] = oAuthSessionCtx.tokenAuthMethod.name - store["$OAUTH_TOKEN_ENDPOINT-$url"] = oAuthSessionCtx.tokenEndpoint + val address = oauthStorageAddress(url) + store["$OAUTH_CLIENT_ID_PREFIX-$address"] = oAuthSessionCtx.clientId + store["$OAUTH_CLIENT_SECRET_PREFIX-$address"] = oAuthSessionCtx.clientSecret + store["$OAUTH_REFRESH_TOKEN-$address"] = refreshToken + store["$OAUTH_TOKEN_AUTH_METHOD-$address"] = oAuthSessionCtx.tokenAuthMethod.name + store["$OAUTH_TOKEN_ENDPOINT-$address"] = oAuthSessionCtx.tokenEndpoint } } } diff --git a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt index 47720971..9c037fdd 100644 --- a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt +++ b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt @@ -2,6 +2,7 @@ package com.coder.toolbox.util import com.coder.toolbox.util.WebUrlValidationResult.Invalid import com.coder.toolbox.util.WebUrlValidationResult.Valid +import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import java.net.IDN import java.net.URI @@ -11,15 +12,17 @@ import java.net.URLDecoder fun String.toURL(): URL = URI.create(this).toURL() /** Canonical HTTP(S) origin, without credentials or a path; default ports are omitted. */ -fun URL.origin(): String? = toString().toHttpUrlOrNull()?.newBuilder() - ?.username("") - ?.password("") - ?.encodedPath("/") - ?.query(null) - ?.fragment(null) - ?.build() - ?.toString() - ?.removeSuffix("/") +fun URL.origin(): String? = toString().toHttpUrlOrNull()?.origin() + +fun HttpUrl.origin(): String = newBuilder() + .username("") + .password("") + .encodedPath("/") + .query(null) + .fragment(null) + .build() + .toString() + .removeSuffix("/") fun URL.hasSameOrigin(other: URL?): Boolean = origin()?.let { it == other?.origin() } ?: false diff --git a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt index 2c56526b..bbac7437 100644 --- a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt +++ b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt @@ -15,9 +15,13 @@ import com.coder.toolbox.sdk.v2.models.WorkspaceBuild import com.coder.toolbox.sdk.v2.models.WorkspaceResource import com.coder.toolbox.sdk.v2.models.WorkspaceStatus import com.coder.toolbox.session.SessionId +import com.coder.toolbox.settings.Environment import com.coder.toolbox.store.CoderSettingsStore import com.coder.toolbox.util.CoderProtocolHandler +import com.coder.toolbox.util.pluginTestSettingsStore import com.coder.toolbox.views.CoderSetupWizardPage +import com.coder.toolbox.views.SuspendBiConsumer +import com.coder.toolbox.views.state.PageRouter import com.coder.toolbox.views.state.StoredOAuthSession import com.coder.toolbox.views.state.WizardStep import com.coder.toolbox.views.state.toSessionContext @@ -25,6 +29,7 @@ import com.jetbrains.toolbox.api.core.diagnostics.Logger import com.jetbrains.toolbox.api.core.util.LoadableState import com.jetbrains.toolbox.api.localization.LocalizableStringFactory import com.jetbrains.toolbox.api.ui.ToolboxUi +import com.sun.net.httpserver.HttpServer import io.mockk.clearAllMocks import io.mockk.coEvery import io.mockk.coVerify @@ -33,6 +38,7 @@ import io.mockk.mockk import io.mockk.slot import io.mockk.spyk import io.mockk.verify +import io.mockk.verifyOrder import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope @@ -43,10 +49,18 @@ import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.CsvSource +import org.junit.jupiter.params.provider.NullSource +import org.junit.jupiter.params.provider.ValueSource import java.io.FileNotFoundException +import java.net.InetSocketAddress +import java.net.Proxy import java.net.URI +import java.net.URL import java.net.URLEncoder import java.util.UUID +import java.util.concurrent.ConcurrentLinkedQueue import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test @@ -85,7 +99,9 @@ class CoderRemoteProviderTest { val settingsStore = mockk(relaxed = true) every { mockContext.settingsStore } returns settingsStore every { mockContext.logger } returns mockLogger + every { mockContext.secrets.isUrlTrusted(any()) } returns false every { mockClient.url } returns URI("https://coder.example.com").toURL() + every { mockClient.me } returns DataGen.user() every { mockCli.deploymentURL } returns URI("https://coder.example.com").toURL() remoteProvider = CoderRemoteProvider(mockContext) } @@ -579,33 +595,56 @@ class CoderRemoteProviderTest { } @Test - fun `given connected client when URI targets different deployment then scheduled wizard overrides client`() = - runTest { - // given - every { mockClient.url } returns URI("https://old.example.com").toURL() - every { mockContext.settingsStore.requiresMTlsAuth } returns false - every { mockContext.settingsStore.requiresTokenAuth } returns true - coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns true - setPrivateField(remoteProvider, "client", mockClient) - setPrivateField(remoteProvider, "cli", mockCli) + fun `an empty query still routes OAuth callbacks to the OAuth handler`() = runTest { + remoteProvider.handleUri(URI("jetbrains://gateway/com.coder.toolbox")) + verify { underlyingLogger.info("No query parameters were provided") } - assertNull(remoteProvider.getOverrideUiPage()) + remoteProvider.handleUri(URI("jetbrains://gateway/com.coder.toolbox/auth")) + verify { underlyingLogger.error("OAuth2 callback arrived but the setup wizard is no longer active") } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } - // when - remoteProvider.handleUri( - URI("jetbrains://gateway/com.coder.toolbox?url=https%3A%2F%2Fnew.example.com&token=new-token") - ) + @ValueSource( + strings = [ + "https://coder2.example.com/", + "https://coder2.example.com/dashboard?view=recent#workspaces", + ] + ) + @ParameterizedTest + fun `accepted deployment switch replaces the wizard before Toolbox requests its page`(target: String) = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns true + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + assertNull(remoteProvider.getOverrideUiPage()) - // then - val overridePage = remoteProvider.getOverrideUiPage() - assertNotNull(overridePage) - assertTrue(overridePage is CoderSetupWizardPage) - coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } - verify { mockContext.popupPluginMainPage() } + val previousWizard = mockk(relaxed = true) + val router = PageRouter() + router.navigate(previousWizard) + setPrivateField(remoteProvider, "router", router) + var displayedWizard: CoderSetupWizardPage? = null + every { mockContext.popupPluginMainPage() } answers { + displayedWizard = remoteProvider.getOverrideUiPage() as? CoderSetupWizardPage } + remoteProvider.handleUri(workspaceLink(target)) + + val wizard = assertNotNull(displayedWizard) + assertEquals(WizardStep.CONNECT, wizard.model.currentStep()) + assertEquals("https://coder2.example.com", wizard.model.url.toString()) + assertEquals("link-token", wizard.model.token) + assertSame(wizard, remoteProvider.getOverrideUiPage()) + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verifyOrder { + previousWizard.dispose() + mockContext.popupPluginMainPage() + } + } + @Test fun `links to the active origin continue without confirmation and preserve its URL`() = runTest { + val secrets = mockContext.secrets every { mockContext.settingsStore.requiresMTlsAuth } returns false every { mockContext.settingsStore.requiresTokenAuth } returns true val activeUrl = URI("https://coder.example.com").toURL() @@ -614,6 +653,7 @@ class CoderRemoteProviderTest { setPrivateField(provider, "client", mockClient) setPrivateField(provider, "cli", mockCli) setPrivateField(provider, "linkHandler", handler) + coEvery { provider["userForToken"](activeUrl, "link-token") } returns mockClient.me coEvery { provider["refreshSession"](activeUrl, "link-token") } returns (mockClient to mockCli) val targets = listOf( @@ -627,6 +667,7 @@ class CoderRemoteProviderTest { coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } coVerify(exactly = targets.size) { handler.handle(any(), activeUrl, mockClient, mockCli) } + verify(exactly = 0) { secrets.isUrlTrusted(any()) } verify(exactly = 0) { mockContext.popupPluginMainPage() } verify(exactly = 0) { underlyingLogger.info(match { it.contains("link-token") }) } assertNull(provider.getOverrideUiPage()) @@ -636,6 +677,7 @@ class CoderRemoteProviderTest { @OptIn(ExperimentalCoroutinesApi::class) fun `different origins wait for confirmation and cancellation preserves the active session`() = runTest { val settings = mockContext.settingsStore + val secrets = mockContext.secrets every { mockContext.settingsStore.requiresMTlsAuth } returns false every { mockContext.settingsStore.requiresTokenAuth } returns true setPrivateField(remoteProvider, "client", mockClient) @@ -662,35 +704,339 @@ class CoderRemoteProviderTest { assertNull(remoteProvider.getOverrideUiPage()) } coVerify(exactly = 3) { mockUi.showYesNoPopup(any(), any(), any(), any()) } - verify(exactly = 0) { mockContext.secrets.storeApiTokenFor(any(), any()) } + verify(exactly = 0) { secrets.storeApiTokenFor(any(), any()) } verify(exactly = 0) { settings.updateLastUsedUrl(any()) } } + @CsvSource("saved-token, 1", "link-token, 0") + @ParameterizedTest(name = "Reopen a known deployment with {0} saved") + fun `known deployment reopens at its origin without confirmation for the same account`( + savedToken: String, + savedTokenLookups: Int, + ) = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockContext.secrets.isUrlTrusted(url) } returns true + every { mockContext.secrets.apiTokenFor(url) } returns savedToken + val provider = spyk(remoteProvider, recordPrivateCalls = true) + val user = DataGen.user() + coEvery { provider["userForToken"](url, "link-token") } returns user + coEvery { provider["userForToken"](url, "saved-token") } returns user + + provider.handleUri(workspaceLink("https://CODER.example.com:443/different/path")) + + val page = provider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(url.toString(), page.model.url.toString()) + assertEquals("link-token", page.model.token) + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + coVerify(exactly = 1) { provider["userForToken"](url, "link-token") } + coVerify(exactly = savedTokenLookups) { provider["userForToken"](url, "saved-token") } + verify(exactly = 1) { mockContext.popupPluginMainPage() } + } + + @Test + fun `link to another known deployment compares that deployment's saved account`() = runTest { + val target = URI("https://other.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockClient.me } returns DataGen.user().copy(username = "active-user") + every { mockContext.secrets.isUrlTrusted(target) } returns true + every { mockContext.secrets.apiTokenFor(target) } returns "saved-token" + val provider = spyk(remoteProvider, recordPrivateCalls = true) + setPrivateField(provider, "client", mockClient) + setPrivateField(provider, "cli", mockCli) + val savedUser = DataGen.user().copy(username = "saved-user") + coEvery { provider["userForToken"](target, "link-token") } returns savedUser + coEvery { provider["userForToken"](target, "saved-token") } returns savedUser + + provider.handleUri(workspaceLink("https://other.example.com")) + + val page = provider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals(target.toString(), page.model.url.toString()) + assertEquals("link-token", page.model.token) + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 0) { mockClient.close() } + } + @Test @OptIn(ExperimentalCoroutinesApi::class) - fun `fresh link requires confirmation even when matching credentials are saved`() = runTest { + fun `account change waits for confirmation before modifying the active session`() = runTest { val url = URI("https://coder.example.com").toURL() + val secrets = mockContext.secrets every { mockContext.settingsStore.requiresMTlsAuth } returns false every { mockContext.settingsStore.requiresTokenAuth } returns true - every { mockContext.secrets.apiTokenFor(url) } returns "stored-token" + val provider = spyk(remoteProvider, recordPrivateCalls = true) + setPrivateField(provider, "client", mockClient) + setPrivateField(provider, "cli", mockCli) + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user().copy(username = "other-user") val decision = CompletableDeferred() coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } coAnswers { decision.await() } - val handling = async { remoteProvider.handleUri(workspaceLink(url.toString())) } + val handling = async { provider.handleUri(workspaceLink("https://coder.example.com")) } runCurrent() + assertFalse(handling.isCompleted) - verify(exactly = 0) { mockContext.popupPluginMainPage() } - verify(exactly = 0) { mockContext.secrets.apiTokenFor(any()) } + assertNull(provider.getOverrideUiPage()) + verify(exactly = 0) { mockClient.close() } + coVerify(exactly = 0) { mockCli.login(any()) } + verify { + mockContext.i18n.pnotr(match { + it.contains("other-user") && it.contains("tester") && !it.contains("link-token") + }) + } - decision.complete(true) + decision.complete(false) handling.await() + coVerify(exactly = 0) { provider["refreshSession"](any(), any()) } + verify(exactly = 0) { secrets.storeApiTokenFor(any(), any()) } + verify(exactly = 0) { secrets.trustDeployment(any()) } + assertNull(provider.getOverrideUiPage()) + } + + @Test + fun `accepting an account change uses the link token`() = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockContext.secrets.isUrlTrusted(url) } returns true + every { mockContext.secrets.apiTokenFor(url) } returns "saved-token" + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns true + val provider = spyk(remoteProvider, recordPrivateCalls = true) + // Separate accounts can have the same username after a name is reused. + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user() + coEvery { provider["userForToken"](url, "saved-token") } returns DataGen.user() + + provider.handleUri(workspaceLink("https://coder.example.com")) + + val page = provider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals("link-token", page.model.token) + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + } + + @NullSource + @ValueSource(strings = ["expired-token"]) + @ParameterizedTest(name = "Confirm sign-in when the saved token is {0}") + fun `a missing or expired saved token requires sign-in confirmation`(savedToken: String?) = runTest { + val url = URI("https://coder.example.com").toURL() + val i18n = mockContext.i18n + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.secrets.isUrlTrusted(url) } returns true + every { mockContext.secrets.apiTokenFor(url) } returns savedToken + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + val provider = spyk(remoteProvider, recordPrivateCalls = true) + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user() + coEvery { provider["userForToken"](url, "expired-token") } returns null + + provider.handleUri(workspaceLink("https://coder.example.com")) + + verify { i18n.ptrl("Sign in with the token from the link?") } + verify(exactly = 0) { i18n.ptrl("Connect to a Coder deployment?") } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `declining an account change preserves an in-flight auto-connect wizard`() = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.deploymentUrl } returns url + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + every { mockContext.settingsStore.preferOAuth2IfAvailable } returns false + every { mockContext.secrets.apiTokenFor(url) } returns "saved-token" + every { mockContext.secrets.isUrlTrusted(url) } returns true + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + val provider = spyk(remoteProvider, recordPrivateCalls = true) + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user().copy(username = "other-user") + coEvery { provider["userForToken"](url, "saved-token") } returns DataGen.user().copy(username = "saved-user") + val existingWizard = provider.getOverrideUiPage() as CoderSetupWizardPage + + provider.handleUri(workspaceLink("https://coder.example.com")) + + assertSame(existingWizard, provider.getOverrideUiPage()) + assertEquals(WizardStep.CONNECT, existingWizard.model.currentStep()) + assertEquals("saved-token", existingWizard.model.token) + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @CsvSource( + "tester, 200, false, true", + "other-user, 200, true, false", + "tester, 401, false, false", + ) + @ParameterizedTest(name = "Link token for {0} with HTTP {1}: prompt={2}, refresh={3}") + fun `link tokens are validated before changing the active session`( + username: String, + responseCode: Int, + expectsPrompt: Boolean, + expectsRefresh: Boolean, + ) = runTest { + val server = HttpServer.create(InetSocketAddress("127.0.0.1", 0), 0) + val url = URI("http://127.0.0.1:${server.address.port}").toURL() + val requests = ConcurrentLinkedQueue() + val userId = if (username == mockClient.me.username) mockClient.me.id else UUID.randomUUID() + server.createContext("/api/v2/users/me") { exchange -> + requests.add(exchange.requestHeaders.getFirst("Coder-Session-Token")) + val body = if (responseCode == 200) { + """{"id":"$userId","username":"$username"}""" + } else { + """{"message":"Unauthorized","detail":"Invalid token"}""" + }.toByteArray() + exchange.responseHeaders.add("Content-Type", "application/json") + exchange.sendResponseHeaders(responseCode, body.size.toLong()) + exchange.responseBody.use { it.write(body) } + exchange.close() + } + server.start() + val secrets = mockContext.secrets + every { mockContext.settingsStore } returns CoderSettingsStore( + pluginTestSettingsStore(), Environment(), underlyingLogger, + ) + every { mockContext.proxySettings.getProxy() } returns Proxy.NO_PROXY + every { mockContext.proxySettings.getProxyAuth() } returns null + every { mockClient.url } returns url + every { secrets.apiTokenFor(url) } returns "saved-token" + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + val provider = spyk(remoteProvider, recordPrivateCalls = true) + val handler = mockk(relaxed = true) + setPrivateField(provider, "client", mockClient) + setPrivateField(provider, "cli", mockCli) + setPrivateField(provider, "linkHandler", handler) + coEvery { provider["refreshSession"](url, "link-token") } returns (mockClient to mockCli) + + try { + provider.handleUri(workspaceLink(url.toString())) + + assertEquals(listOf("link-token"), requests.toList()) + coVerify(exactly = if (expectsPrompt) 1 else 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + coVerify(exactly = if (expectsRefresh) 1 else 0) { provider["refreshSession"](url, "link-token") } + coVerify(exactly = if (expectsRefresh) 1 else 0) { handler.handle(any(), url, mockClient, mockCli) } + verify(exactly = 0) { secrets.apiTokenFor(any()) } + verify(exactly = 0) { secrets.storeApiTokenFor(any(), any()) } + verify(exactly = 0) { secrets.trustDeployment(any()) } + verify(exactly = 0) { mockClient.close() } + assertNull(provider.getOverrideUiPage()) + } finally { + server.stop(0) + } + } + + @CsvSource( + "true, jetbrains://gateway/com.coder.toolbox?url=https://coder.example.com", + "false, jetbrains://gateway/com.coder.toolbox?url=https://coder.example.com&token=", + "false, jetbrains://gateway/com.coder.toolbox?url=https://other.example.com&token=%20", + ) + @ParameterizedTest(name = "Reject {1} when connected={0}") + fun `token authentication requires a nonblank token in every workspace link`( + connected: Boolean, + link: String, + ) = runTest { + val secrets = mockContext.secrets + every { mockContext.settingsStore.requiresMTlsAuth } returns false + every { mockContext.settingsStore.requiresTokenAuth } returns true + val savedUrl = URI("https://coder.example.com").toURL() + every { secrets.isUrlTrusted(savedUrl) } returns true + every { secrets.apiTokenFor(savedUrl) } returns "saved-token" + if (connected) { + setPrivateField(remoteProvider, "client", mockClient) + setPrivateField(remoteProvider, "cli", mockCli) + } + + remoteProvider.handleUri(URI(link)) + + verify { + underlyingLogger.error(match { it.contains("Query parameter \"token\" is missing from URI") }) + } + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 0) { secrets.isUrlTrusted(any()) } + verify(exactly = 0) { secrets.apiTokenFor(any()) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `known certificate deployment reopens without a trust prompt`() = runTest { + every { mockContext.settingsStore.requiresMTlsAuth } returns true + every { mockContext.settingsStore.requiresTokenAuth } returns false + every { mockContext.secrets.isUrlTrusted(URI("https://coder.example.com").toURL()) } returns true + + remoteProvider.handleUri(URI("jetbrains://gateway/com.coder.toolbox?url=https://coder.example.com/path")) + val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage + assertEquals("https://coder.example.com", page.model.url.toString()) assertEquals(WizardStep.CONNECT, page.model.currentStep()) - assertEquals(url.toString(), page.model.url.toString()) - assertEquals("link-token", page.model.token) + assertNull(page.model.token) + coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + } + + @Test + fun `a saved API token cannot identify the account when Toolbox would use OAuth`() = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.preferOAuth2IfAvailable } returns true + every { mockContext.secrets.isUrlTrusted(url) } returns true + every { mockContext.secrets.oauthSessionFor(url.toString()) } returns storedOAuthSession() + every { mockContext.secrets.apiTokenFor(url) } returns "link-token" + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + val provider = spyk(remoteProvider, recordPrivateCalls = true) + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user() + + provider.handleUri(workspaceLink("https://coder.example.com")) + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } - verify(exactly = 1) { mockContext.popupPluginMainPage() } + coVerify(exactly = 1) { provider["userForToken"](url, any()) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `dismissing account confirmation preserves the active session without an error popup`() = runTest { + val url = URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + val provider = spyk(remoteProvider, recordPrivateCalls = true) + setPrivateField(provider, "client", mockClient) + setPrivateField(provider, "cli", mockCli) + coEvery { provider["userForToken"](url, "link-token") } returns DataGen.user().copy(username = "other-user") + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } throws CancellationException("Dismissed") + + assertFailsWith { + provider.handleUri(workspaceLink("https://coder.example.com")) + } + + coVerify(exactly = 0) { provider["refreshSession"](any(), any()) } + verify(exactly = 0) { underlyingLogger.error(any()) } + verify(exactly = 0) { mockClient.close() } + assertNull(provider.getOverrideUiPage()) + } + + @Test + fun `default or last-used URL alone does not establish deployment trust`() = runTest { + val secrets = mockContext.secrets + every { mockContext.deploymentUrl } returns URI("https://coder.example.com").toURL() + every { mockContext.settingsStore.requiresMTlsAuth } returns false + coEvery { mockUi.showYesNoPopup(any(), any(), any(), any()) } returns false + + remoteProvider.handleUri(workspaceLink("https://coder.example.com")) + + coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } + verify(exactly = 0) { secrets.trustDeployment(any()) } + verify(exactly = 0) { mockContext.popupPluginMainPage() } + } + + @Test + fun `successful connection remembers the deployment URL and token`() = runTest { + val url = URI("https://coder.example.com").toURL() + val secrets = mockContext.secrets + val settings = CoderSettingsStore(pluginTestSettingsStore(), Environment(), underlyingLogger) + every { mockContext.settingsStore } returns settings + every { mockContext.cs } returns backgroundScope + every { mockClient.token } returns "saved-token" + val callback = remoteProvider.javaClass.getDeclaredField("onConnect").apply { isAccessible = true } + .get(remoteProvider) as SuspendBiConsumer + + callback.accept(mockClient, mockCli) + + verify { secrets.storeApiTokenFor(url, "saved-token") } + verify { secrets.trustDeployment(url) } + assertEquals("https://coder.example.com", settings.lastDeploymentURL) } @Test diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt index e7eb6914..1c71aaf5 100644 --- a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt @@ -1,11 +1,16 @@ package com.coder.toolbox.store +import com.coder.toolbox.oauth.TokenEndpointAuthMethod import com.coder.toolbox.util.toURL +import com.coder.toolbox.views.state.StoredOAuthSession +import com.coder.toolbox.views.state.toSessionContext import com.jetbrains.toolbox.api.core.PluginSecretStore import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse import kotlin.test.assertNull +import kotlin.test.assertTrue class CoderSecretsStoreTest { private val values = mutableMapOf() @@ -53,6 +58,7 @@ class CoderSecretsStoreTest { assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) assertNull(secrets.apiTokenFor("http://coder.example.com".toURL())) + assertFalse(secrets.isUrlTrusted("https://coder.example.com".toURL())) secrets.storeApiTokenFor("https://coder.example.com".toURL(), "new-token") assertEquals("new-token", secrets.apiTokenFor("https://coder.example.com".toURL())) @@ -109,4 +115,97 @@ class CoderSecretsStoreTest { assertEquals("existing-token", secrets.apiTokenFor("https://coder.example.com".toURL())) } + + @Test + fun `signing in trusts only that deployment origin across restarts`() { + secrets.trustDeployment("https://coder1.example.com/".toURL()) + val reopened = CoderSecretsStore(backing) + + assertTrue(reopened.isUrlTrusted("https://CODER1.example.com:443".toURL())) + assertFalse(reopened.isUrlTrusted("https://coder2.example.com/".toURL())) + assertFalse(reopened.isUrlTrusted("http://coder1.example.com/".toURL())) + assertFalse(reopened.isUrlTrusted("https://coder1.example.com:8443/".toURL())) + } + + @Test + fun `an origin-bound token establishes trust only for its origin`() { + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "saved-token") + + assertTrue(secrets.isUrlTrusted("https://coder.example.com/".toURL())) + assertFalse(secrets.isUrlTrusted("https://coder.example.com/other/".toURL())) + assertFalse(secrets.isUrlTrusted("http://coder.example.com/".toURL())) + } + + @Test + fun `a sign-in at a configured base path does not silently trust the root`() { + secrets.storeApiTokenFor("https://coder.example.com/team/".toURL(), "token") + secrets.trustDeployment("https://coder.example.com/team/".toURL()) + + assertTrue(secrets.isUrlTrusted("https://coder.example.com/team/".toURL())) + assertFalse(secrets.isUrlTrusted("https://coder.example.com/".toURL())) + } + + @Test + fun `equivalent root URLs find and clear the OAuth session saved at sign-in`() { + val signedInAt = "https://CODER.example.com:443/" + secrets.trustDeployment(signedInAt.toURL()) + val session = StoredOAuthSession( + clientId = "client-id", + clientSecret = "client-secret", + refreshToken = "refresh-token", + tokenAuthMethod = TokenEndpointAuthMethod.CLIENT_SECRET_BASIC, + tokenEndpoint = "https://coder.example.com/oauth/token", + ) + secrets.storeOAuthFor(signedInAt, session.toSessionContext()) + + val reopened = CoderSecretsStore(backing) + assertEquals(session, reopened.oauthSessionFor("https://coder.example.com")) + assertNull(reopened.oauthSessionFor("https://other.example.com")) + assertNull(reopened.oauthSessionFor("https://coder.example.com/other/")) + + reopened.trustDeployment("https://coder.example.com".toURL()) + val refreshed = session.copy(refreshToken = "refreshed-token") + reopened.storeOAuthFor("https://coder.example.com", refreshed.toSessionContext()) + assertEquals(refreshed, reopened.oauthSessionFor(signedInAt)) + + reopened.clearSessionFor("https://coder.example.com".toURL()) + assertNull(reopened.oauthSessionFor(signedInAt)) + } + + @Test + fun `certificate and OAuth deployments can be remembered without an API token`() { + secrets.trustDeployment("https://coder.example.com".toURL()) + + val reopened = CoderSecretsStore(backing) + + assertTrue(reopened.isUrlTrusted("https://coder.example.com".toURL())) + assertNull(reopened.apiTokenFor("https://coder.example.com".toURL())) + } + + @Test + fun `logout forgets only the matching origin's trusted URL`() { + secrets.trustDeployment("http://coder.example.com".toURL()) + secrets.trustDeployment("https://coder.example.com".toURL()) + secrets.storeApiTokenFor("https://coder.example.com".toURL(), "token") + secrets.trustDeployment("https://coder.example.com:8443".toURL()) + values["coder.example.com"] = "legacy-token" + + secrets.clearSessionFor("https://CODER.example.com:443/".toURL()) + + val reopened = CoderSecretsStore(backing) + assertFalse(reopened.isUrlTrusted("https://coder.example.com".toURL())) + assertNull(reopened.apiTokenFor("https://coder.example.com".toURL())) + for (url in listOf("http://coder.example.com", "https://coder.example.com:8443")) { + assertTrue(reopened.isUrlTrusted(url.toURL()), url) + } + assertEquals("legacy-token", values["coder.example.com"]) + } + + @Test + fun `deployment trust requires a valid HTTP origin`() { + assertFalse(secrets.isUrlTrusted("file:///tmp/coder".toURL())) + assertFailsWith { + secrets.trustDeployment("file:///tmp/coder".toURL()) + } + } } From 9a5f2c3d1beb8d0815fa971d884b9bbfb362e341 Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Tue, 10 Nov 2026 22:49:05 +0200 Subject: [PATCH 5/6] Clear legacy hostname-only API tokens Older Toolbox versions store API tokens under the deployment hostname. The secret store now uses the deployment origin, so the old token stays in the keychain after sign-in or logout. The secret store now clears the hostname entry after it saves a replacement token or when the user signs out. A failed write preserves the old credential, and cleanup preserves the last deployment URL when its key matches the hostname. --- .../coder/toolbox/store/CoderSecretsStore.kt | 19 +++++++- .../toolbox/store/CoderSecretsStoreTest.kt | 43 ++++++++++++++++--- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt index 67532270..f377b0a4 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt @@ -13,6 +13,7 @@ private const val OAUTH_CLIENT_SECRET_PREFIX = "oauth-client-secret" private const val OAUTH_REFRESH_TOKEN = "oauth-refresh-token" private const val OAUTH_TOKEN_AUTH_METHOD = "oauth-token-auth-method" private const val OAUTH_TOKEN_ENDPOINT = "oauth-token-endpoint" +private const val LAST_DEPLOYMENT_URL_KEY = "last-deployment-url" /** * Provides Coder secrets backed by the secrets store service. @@ -22,18 +23,29 @@ class CoderSecretsStore(private val store: PluginSecretStore) { message = "The URL is now stored the JSON backed settings store. Use CoderSettingsStore#lastDeploymentURL", replaceWith = ReplaceWith("context.settingsStore.lastDeploymentURL") ) - val lastDeploymentURL: String = store["last-deployment-url"] ?: "" + val lastDeploymentURL: String = store[LAST_DEPLOYMENT_URL_KEY] ?: "" fun apiTokenFor(url: URL): String? = apiSessionKey(url)?.let { store[it] } fun storeApiTokenFor(url: URL, apiToken: String) { val key = requireNotNull(apiSessionKey(url)) { "Session credentials require an HTTP(S) deployment URL" } store[key] = apiToken + clearLegacyApiToken(url) } private fun apiSessionKey(url: URL): String? = url.origin()?.let { "api-session:$it" } + private fun clearLegacyApiToken(url: URL) { + if (url.host.isEmpty()) return + // Older versions used URL.host verbatim. A link can also supply the host in lowercase. + for (host in setOf(url.host, url.host.lowercase())) { + // A single-label hostname can share the key used for the last deployment URL. + if (host == LAST_DEPLOYMENT_URL_KEY && store[host]?.toHttpUrlOrNull() != null) continue + store.clear(host) + } + } + fun isUrlTrusted(url: URL): Boolean { val requested = url.toString().toHttpUrlOrNull() ?: return false val origin = requested.origin() @@ -53,7 +65,10 @@ class CoderSecretsStore(private val store: PluginSecretStore) { } fun clearSessionFor(url: URL) { - apiSessionKey(url)?.let { store.clear(it) } + apiSessionKey(url)?.let { + store.clear(it) + clearLegacyApiToken(url) + } url.origin()?.let { origin -> val key = "deployment-url:$origin" val signedInAt = store[key] diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt index 1c71aaf5..18b8409d 100644 --- a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt @@ -52,23 +52,56 @@ class CoderSecretsStoreTest { } @Test - fun `hostname-only credentials are ignored and preserved`() { + fun `hostname-only credentials are ignored until a new token is saved`() { values["coder.example.com"] = "old-token" values["last-deployment-url"] = "https://coder.example.com" assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) assertNull(secrets.apiTokenFor("http://coder.example.com".toURL())) assertFalse(secrets.isUrlTrusted("https://coder.example.com".toURL())) + assertEquals("old-token", values["coder.example.com"]) secrets.storeApiTokenFor("https://coder.example.com".toURL(), "new-token") assertEquals("new-token", secrets.apiTokenFor("https://coder.example.com".toURL())) - assertEquals("old-token", values["coder.example.com"]) + assertNull(values["coder.example.com"]) + assertEquals("https://coder.example.com", values["last-deployment-url"]) + } - secrets.clearSessionFor("https://coder.example.com".toURL()) - assertNull(secrets.apiTokenFor("https://coder.example.com".toURL())) + @Test + fun `a failed replacement write preserves the hostname-only credential`() { + values["coder.example.com"] = "old-token" + val failingStore = object : PluginSecretStore by backing { + override fun set(key: String, value: String) = throw IllegalStateException("Keychain is locked") + } + + assertFailsWith { + CoderSecretsStore(failingStore).storeApiTokenFor("https://coder.example.com".toURL(), "new-token") + } assertEquals("old-token", values["coder.example.com"]) } + @Test + fun `logout clears a legacy token even without an origin-bound token`() { + values["CODER.example.com"] = "old-token" + values["other.example.com"] = "other-token" + + secrets.clearSessionFor("https://CODER.example.com:8443".toURL()) + + assertNull(values["CODER.example.com"]) + assertEquals("other-token", values["other.example.com"]) + } + + @Test + fun `a hostname matching the last deployment key preserves the stored URL`() { + values["last-deployment-url"] = "https://coder.example.com" + val url = "https://last-deployment-url".toURL() + + secrets.storeApiTokenFor(url, "new-token") + secrets.clearSessionFor(url) + + assertEquals("https://coder.example.com", values["last-deployment-url"]) + } + @Test fun `sessions on the same host with different schemes and ports coexist`() { secrets.storeApiTokenFor("http://coder.example.com".toURL(), "http-token") @@ -198,7 +231,7 @@ class CoderSecretsStoreTest { for (url in listOf("http://coder.example.com", "https://coder.example.com:8443")) { assertTrue(reopened.isUrlTrusted(url.toURL()), url) } - assertEquals("legacy-token", values["coder.example.com"]) + assertNull(values["coder.example.com"]) } @Test From b51d36b321e359c2473b01a60b61adc99991a7cc Mon Sep 17 00:00:00 2001 From: Faur Ioan-Aurel Date: Tue, 10 Nov 2026 23:19:41 +0200 Subject: [PATCH 6/6] Store OAuth sessions by deployment origin The Toolbox secret store keys OAuth credentials by deployment URL. A change in the path or trailing slash can leave the same deployment with separate sessions. The secret store now uses the scheme, host, and effective port as the key. On first use, it moves credentials found under a legacy URL to the origin key. If that write fails, the legacy session remains available. Logout clears the origin key and the legacy keys for the supplied URL and root spelling. --- .../coder/toolbox/store/CoderSecretsStore.kt | 76 +++++---- .../toolbox/store/CoderSecretsStoreTest.kt | 148 ++++++++++++++++-- 2 files changed, 183 insertions(+), 41 deletions(-) diff --git a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt index f377b0a4..192f6bdc 100644 --- a/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt +++ b/src/main/kotlin/com/coder/toolbox/store/CoderSecretsStore.kt @@ -64,21 +64,17 @@ class CoderSecretsStore(private val store: PluginSecretStore) { if (store[key]?.toHttpUrlOrNull() != address) store[key] = url.toString() } + @Synchronized fun clearSessionFor(url: URL) { apiSessionKey(url)?.let { store.clear(it) clearLegacyApiToken(url) } url.origin()?.let { origin -> - val key = "deployment-url:$origin" - val signedInAt = store[key] - val savedOrigin = signedInAt?.toHttpUrlOrNull()?.origin() - if (signedInAt != null && signedInAt != url.toString() && savedOrigin == origin) { - clearOAuthSession(signedInAt) - } - store.clear(key) + clearOAuthSession(origin) + legacyOAuthAddresses(url.toString(), origin).forEach(::clearOAuthSession) + store.clear("deployment-url:$origin") } - clearOAuthSession(url.toString()) } private fun clearOAuthSession(url: String) { @@ -90,25 +86,31 @@ class CoderSecretsStore(private val store: PluginSecretStore) { } } - fun oauthSessionFor(url: String): StoredOAuthSession? { - val address = oauthStorageAddress(url) - return readOAuthSession(address) ?: if (address != url) readOAuthSession(url) else null - } + // Older versions used the URL exactly as entered, including its path or trailing slash. + private fun legacyOAuthAddresses(url: String, origin: String): Set = + setOf(url, "$origin/") - origin - private fun oauthStorageAddress(url: String): String { - // Existing OAuth keys use the exact spelling of the URL used to sign in. - val requested = url.toHttpUrlOrNull() ?: return url - val saved = store["deployment-url:${requested.origin()}"] - return saved?.takeIf { it.toHttpUrlOrNull() == requested } ?: url + @Synchronized + fun oauthSessionFor(url: String): StoredOAuthSession? { + val origin = url.toHttpUrlOrNull()?.origin() ?: return null + readOAuthSession(origin)?.let { return it } + val (legacyUrl, legacy) = legacyOAuthAddresses(url, origin) + .firstNotNullOfOrNull { address -> readOAuthSession(address)?.let { address to it } } ?: return null + // Move the session on first use so later lookups and logout need only the origin. + runCatching { + writeOAuthSession(origin, legacy) + clearOAuthSession(legacyUrl) + } + return legacy } private fun readOAuthSession(url: String): StoredOAuthSession? { - val clientId = store["$OAUTH_CLIENT_ID_PREFIX-$url"] + val clientId = store["$OAUTH_CLIENT_ID_PREFIX-$url"] ?: return null val clientSecret = store["$OAUTH_CLIENT_SECRET_PREFIX-$url"] val refreshToken = store["$OAUTH_REFRESH_TOKEN-$url"] val tokenAuthMethod = store["$OAUTH_TOKEN_AUTH_METHOD-$url"] val tokenEndpoint = store["$OAUTH_TOKEN_ENDPOINT-$url"] - if (clientId == null || clientSecret == null || refreshToken == null || tokenAuthMethod == null || tokenEndpoint == null) { + if (clientSecret == null || refreshToken == null || tokenAuthMethod == null || tokenEndpoint == null) { return null } @@ -121,14 +123,34 @@ class CoderSecretsStore(private val store: PluginSecretStore) { ) } - fun storeOAuthFor(url: String, oAuthSessionCtx: CoderOAuthSessionContext) { - oAuthSessionCtx.tokenResponse?.refreshToken?.let { refreshToken -> - val address = oauthStorageAddress(url) - store["$OAUTH_CLIENT_ID_PREFIX-$address"] = oAuthSessionCtx.clientId - store["$OAUTH_CLIENT_SECRET_PREFIX-$address"] = oAuthSessionCtx.clientSecret - store["$OAUTH_REFRESH_TOKEN-$address"] = refreshToken - store["$OAUTH_TOKEN_AUTH_METHOD-$address"] = oAuthSessionCtx.tokenAuthMethod.name - store["$OAUTH_TOKEN_ENDPOINT-$address"] = oAuthSessionCtx.tokenEndpoint + private fun writeOAuthSession(origin: String, session: StoredOAuthSession) { + if (readOAuthSession(origin)?.copy(refreshToken = session.refreshToken) == session) { + store["$OAUTH_REFRESH_TOKEN-$origin"] = session.refreshToken + return } + // Publish a new registration's client ID last so partial writes cannot be read as a session. + store.clear("$OAUTH_CLIENT_ID_PREFIX-$origin") + store["$OAUTH_CLIENT_SECRET_PREFIX-$origin"] = session.clientSecret + store["$OAUTH_REFRESH_TOKEN-$origin"] = session.refreshToken + store["$OAUTH_TOKEN_AUTH_METHOD-$origin"] = session.tokenAuthMethod.name + store["$OAUTH_TOKEN_ENDPOINT-$origin"] = session.tokenEndpoint + store["$OAUTH_CLIENT_ID_PREFIX-$origin"] = session.clientId + } + + @Synchronized + fun storeOAuthFor(url: String, oAuthSessionCtx: CoderOAuthSessionContext) { + val refreshToken = oAuthSessionCtx.tokenResponse?.refreshToken ?: return + val origin = + requireNotNull(url.toHttpUrlOrNull()?.origin()) { "OAuth credentials require an HTTP(S) deployment URL" } + writeOAuthSession( + origin, StoredOAuthSession( + clientId = oAuthSessionCtx.clientId, + clientSecret = oAuthSessionCtx.clientSecret, + refreshToken = refreshToken, + tokenAuthMethod = oAuthSessionCtx.tokenAuthMethod, + tokenEndpoint = oAuthSessionCtx.tokenEndpoint, + ) + ) + legacyOAuthAddresses(url, origin).forEach(::clearOAuthSession) } } diff --git a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt index 18b8409d..a257f82c 100644 --- a/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt +++ b/src/test/kotlin/com/coder/toolbox/store/CoderSecretsStoreTest.kt @@ -179,32 +179,152 @@ class CoderSecretsStoreTest { } @Test - fun `equivalent root URLs find and clear the OAuth session saved at sign-in`() { - val signedInAt = "https://CODER.example.com:443/" - secrets.trustDeployment(signedInAt.toURL()) - val session = StoredOAuthSession( - clientId = "client-id", - clientSecret = "client-secret", - refreshToken = "refresh-token", - tokenAuthMethod = TokenEndpointAuthMethod.CLIENT_SECRET_BASIC, - tokenEndpoint = "https://coder.example.com/oauth/token", - ) + fun `OAuth sessions use the normalized origin across restarts and refreshes`() { + val signedInAt = "https://CODER.example.com:443/team/" + val session = oauthSession() secrets.storeOAuthFor(signedInAt, session.toSessionContext()) + assertEquals("refresh-token", values["oauth-refresh-token-https://coder.example.com"]) + assertFalse(values.keys.any { it.endsWith(signedInAt) }) val reopened = CoderSecretsStore(backing) assertEquals(session, reopened.oauthSessionFor("https://coder.example.com")) - assertNull(reopened.oauthSessionFor("https://other.example.com")) - assertNull(reopened.oauthSessionFor("https://coder.example.com/other/")) + assertEquals(session, reopened.oauthSessionFor("https://coder.example.com/")) + assertEquals(session, reopened.oauthSessionFor("https://coder.example.com/other/")) + for (url in listOf("http://coder.example.com", "https://coder.example.com:8443", "https://other.example.com")) { + assertNull(reopened.oauthSessionFor(url), url) + } - reopened.trustDeployment("https://coder.example.com".toURL()) val refreshed = session.copy(refreshToken = "refreshed-token") reopened.storeOAuthFor("https://coder.example.com", refreshed.toSessionContext()) assertEquals(refreshed, reopened.oauthSessionFor(signedInAt)) - reopened.clearSessionFor("https://coder.example.com".toURL()) + reopened.clearSessionFor("https://coder.example.com/path".toURL()) assertNull(reopened.oauthSessionFor(signedInAt)) } + private fun oauthSession() = StoredOAuthSession( + clientId = "client-id", + clientSecret = "client-secret", + refreshToken = "refresh-token", + tokenAuthMethod = TokenEndpointAuthMethod.CLIENT_SECRET_BASIC, + tokenEndpoint = "https://coder.example.com/team/oauth/token", + ) + + @Test + fun `first lookup moves a legacy OAuth session from its exact URL to the origin`() { + val oldUrl = "https://CODER.example.com:443/team/" + val session = oauthSession() + seedLegacyOAuth(oldUrl, session) + + assertEquals(session, secrets.oauthSessionFor(oldUrl)) + + assertFalse(values.keys.any { it.endsWith(oldUrl) }) + assertEquals(session, CoderSecretsStore(backing).oauthSessionFor("https://coder.example.com/other")) + secrets.clearSessionFor("https://coder.example.com".toURL()) + assertNull(secrets.oauthSessionFor(oldUrl)) + } + + private fun seedLegacyOAuth(url: String, session: StoredOAuthSession) { + values["oauth-client-id-$url"] = session.clientId + values["oauth-client-secret-$url"] = session.clientSecret + values["oauth-refresh-token-$url"] = session.refreshToken + values["oauth-token-auth-method-$url"] = session.tokenAuthMethod.name + values["oauth-token-endpoint-$url"] = session.tokenEndpoint + } + + @Test + fun `remembering another base path leaves the OAuth session in place`() { + val session = oauthSession() + secrets.storeOAuthFor("https://coder.example.com/team/", session.toSessionContext()) + secrets.trustDeployment("https://coder.example.com/team/".toURL()) + + secrets.trustDeployment("https://coder.example.com/other".toURL()) + + assertEquals(session, CoderSecretsStore(backing).oauthSessionFor("https://coder.example.com/other")) + } + + @Test + fun `logout clears canonical and legacy OAuth keys without touching another origin`() { + val origin = "https://coder.example.com" + val other = "https://coder.example.com:8443" + val session = oauthSession() + seedLegacyOAuth("$origin/", session) + assertEquals(session, secrets.oauthSessionFor(origin)) + seedLegacyOAuth(origin, session.copy(refreshToken = "canonical")) + assertEquals("canonical", secrets.oauthSessionFor("$origin/")?.refreshToken) + secrets.storeOAuthFor(other, session.toSessionContext()) + values["deployment-url:$origin"] = other + + secrets.clearSessionFor("$origin/team".toURL()) + + assertTrue(values.keys.all { it.endsWith(other) }) + assertEquals(session, secrets.oauthSessionFor(other)) + assertNull(secrets.oauthSessionFor(origin)) + } + + @Test + fun `failed migration leaves the legacy session available for retry`() { + val url = "https://coder.example.com/" + val session = oauthSession() + seedLegacyOAuth(url, session) + val failingStore = object : PluginSecretStore by backing { + override fun set(key: String, value: String) { + if (key == "oauth-token-endpoint-https://coder.example.com") error("Keychain is locked") + backing[key] = value + } + } + val refreshed = session.copy(refreshToken = "rotated") + assertFailsWith { + CoderSecretsStore(failingStore).storeOAuthFor(url, refreshed.toSessionContext()) + } + + assertEquals(session, secrets.oauthSessionFor(url)) + secrets.storeOAuthFor(url, refreshed.toSessionContext()) + assertEquals(refreshed, secrets.oauthSessionFor(url)) + } + + @Test + fun `lookup returns the legacy OAuth session when moving it fails`() { + val url = "https://coder.example.com/" + val session = oauthSession() + seedLegacyOAuth(url, session) + val failingStore = object : PluginSecretStore by backing { + override fun set(key: String, value: String) = error("Keychain is locked") + } + + assertEquals(session, CoderSecretsStore(failingStore).oauthSessionFor(url)) + assertEquals(session.refreshToken, values["oauth-refresh-token-$url"]) + } + + @Test + fun `failed refresh token write preserves the current OAuth registration`() { + val url = "https://coder.example.com" + val session = oauthSession() + secrets.storeOAuthFor(url, session.toSessionContext()) + val failingStore = object : PluginSecretStore by backing { + override fun set(key: String, value: String) = error("Keychain is locked") + } + + assertFailsWith { + CoderSecretsStore(failingStore).storeOAuthFor( + url, + session.copy(refreshToken = "rotated").toSessionContext() + ) + } + assertEquals(session, secrets.oauthSessionFor(url)) + } + + @Test + fun `invalid origins cannot read or store OAuth credentials`() { + for (url in listOf("file:///tmp/coder", "https://coder.example.com:99999")) { + assertNull(secrets.oauthSessionFor(url)) + assertFailsWith { + secrets.storeOAuthFor(url, oauthSession().toSessionContext()) + } + } + assertTrue(values.isEmpty()) + } + @Test fun `certificate and OAuth deployments can be remembered without an API token`() { secrets.trustDeployment("https://coder.example.com".toURL())