diff --git a/CHANGELOG.md b/CHANGELOG.md index 56c81b2b..ee23d4d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ It follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Security +- Restrict HTTP redirects and deployment credentials to their original destination. Disable redirects in CLI v2.38.0 and newer. + - 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. diff --git a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt index 1d3a3770..2f7ef7dd 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt @@ -113,6 +113,7 @@ class CoderCLIManager( // Keep credential storage consistent with existing SSH commands until the next sign-in. private val useKeyring = context.settingsStore.useKeyring && usesTokenAuth + private val supportsRedirectFlag by lazy { tryVersion()?.let { it >= SemVer(2, 38, 0) } == true } private val downloader = createDownloadService() private val gpgVerifier = GPGVerifier(context) @@ -661,6 +662,7 @@ class CoderCLIManager( private fun globalConfigArgs(): List = listOf("--global-config", coderConfigPath.toString()) private fun workspaceAuthArgs(): List = buildList { + if (supportsRedirectFlag) add("--allow-redirects=false") val useKeyringAuth = shouldUseKeyringAuth() if (!useKeyringAuth) addAll(globalConfigArgs()) // Override inherited CODER_URL and CODER_USE_KEYRING for every authenticated command. diff --git a/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt b/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt index da5e2f21..21b3d69a 100644 --- a/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt +++ b/src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt @@ -4,6 +4,7 @@ import com.coder.toolbox.CoderToolboxContext import com.coder.toolbox.cli.ex.ResponseException import com.coder.toolbox.util.OS import com.coder.toolbox.util.SemVer +import com.coder.toolbox.util.hasSameOrigin import com.coder.toolbox.util.getHeaders import com.coder.toolbox.util.getOS import com.coder.toolbox.util.sha1 @@ -130,7 +131,7 @@ class CoderDownloadService( } private fun getRequestHeaders(): Map { - return if (context.settingsStore.headerCommand.isNullOrBlank()) { + return if (!remoteBinaryURL.hasSameOrigin(deploymentUrl) || context.settingsStore.headerCommand.isNullOrBlank()) { emptyMap() } else { getHeaders(deploymentUrl, context.settingsStore.headerCommand) diff --git a/src/main/kotlin/com/coder/toolbox/sdk/CoderHttpClientBuilder.kt b/src/main/kotlin/com/coder/toolbox/sdk/CoderHttpClientBuilder.kt index b14479db..34d46f3e 100644 --- a/src/main/kotlin/com/coder/toolbox/sdk/CoderHttpClientBuilder.kt +++ b/src/main/kotlin/com/coder/toolbox/sdk/CoderHttpClientBuilder.kt @@ -21,6 +21,8 @@ object CoderHttpClientBuilder { .sslSocketFactory(tlsContext.sslSocketFactory, tlsContext.trustManager) .hostnameVerifier(CoderHostnameVerifier(context.settingsStore.tls.altHostname)) .retryOnConnectionFailure(true) + .followRedirects(false) + .addInterceptor(Interceptors.sameOriginRedirects()) // Detect silent connection loss even when a workspace build produces no log messages. .pingInterval(30, TimeUnit.SECONDS) diff --git a/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt b/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt index f0bde3f8..188cddc3 100644 --- a/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt +++ b/src/main/kotlin/com/coder/toolbox/sdk/CoderRestClient.kt @@ -95,7 +95,7 @@ open class CoderRestClient( val interceptors = buildList { if (context.settingsStore.requiresTokenAuth) { - add(Interceptors.tokenAuth(requireSessionToken())) + add(Interceptors.tokenAuth(requireSessionToken(), url)) } add((Interceptors.userAgent(pluginVersion))) add(Interceptors.externalHeaders(context, url)) diff --git a/src/main/kotlin/com/coder/toolbox/sdk/interceptors/Interceptors.kt b/src/main/kotlin/com/coder/toolbox/sdk/interceptors/Interceptors.kt index 8d64cbeb..49702424 100644 --- a/src/main/kotlin/com/coder/toolbox/sdk/interceptors/Interceptors.kt +++ b/src/main/kotlin/com/coder/toolbox/sdk/interceptors/Interceptors.kt @@ -4,7 +4,10 @@ import com.coder.toolbox.CoderToolboxContext import com.coder.toolbox.util.getArch import com.coder.toolbox.util.getHeaders import com.coder.toolbox.util.getOS +import com.coder.toolbox.util.hasSameOrigin +import com.coder.toolbox.util.origin import okhttp3.Interceptor +import java.net.ProtocolException import java.net.URL const val CODER_SESSION_TOKEN_HEADER_NAME = "Coder-Session-Token" @@ -14,13 +17,46 @@ const val CODER_SESSION_TOKEN_HEADER_NAME = "Coder-Session-Token" */ object Interceptors { + private const val MAX_REDIRECTS = 20 + + /** + * Follows redirects only for GET and HEAD requests and only within the original scheme, host, and port. + * HTTP defines these methods as read-only, and OkHttp sends them without a request body. Checking each + * target prevents forwarding deployment credentials to another origin. Other methods are never replayed. + * + * The cap of [MAX_REDIRECTS] matches OkHttp 4.12.1's limit for follow-up requests and bounds redirect loops. + * The initial request does not count. A further redirect after the twentieth fails before the next request is sent. + */ + fun sameOriginRedirects(): Interceptor = Interceptor { chain -> + var request = chain.request() + val origin = request.url.origin() + var redirects = 0 + var response = chain.proceed(request) + while (response.isRedirect) { + val location = response.header("Location") ?: break + val target = request.url.resolve(location) + if (target == null || target.origin() != origin || target.username.isNotEmpty() || target.password.isNotEmpty()) { + response.close() + throw ProtocolException("Refused a redirect outside the request origin or to an invalid URL.") + } + if (request.method != "GET" && request.method != "HEAD") break + response.close() + if (++redirects > MAX_REDIRECTS) throw ProtocolException("Too many redirects.") + request = request.newBuilder().url(target).build() + response = chain.proceed(request) + } + response + } + /** * Creates a token authentication interceptor */ - fun tokenAuth(token: String): Interceptor { + fun tokenAuth(token: String, deploymentUrl: URL): Interceptor { return Interceptor { chain -> + val request = chain.request() + if (!request.url.hasSameOrigin(deploymentUrl)) return@Interceptor chain.proceed(request) chain.proceed( - chain.request().newBuilder() + request.newBuilder() .addHeader(CODER_SESSION_TOKEN_HEADER_NAME, token) .build() ) @@ -47,6 +83,7 @@ object Interceptors { val settings = context.settingsStore.readOnly() return Interceptor { chain -> var request = chain.request() + if (!request.url.hasSameOrigin(url)) return@Interceptor chain.proceed(request) val headers = getHeaders(url, settings.headerCommand) if (headers.isNotEmpty()) { val reqBuilder = request.newBuilder() @@ -63,4 +100,4 @@ object Interceptors { fun logging(context: CoderToolboxContext): Interceptor { return LoggingInterceptor(context) } -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt index 9c037fdd..72364b77 100644 --- a/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt +++ b/src/main/kotlin/com/coder/toolbox/util/URLExtensions.kt @@ -24,7 +24,9 @@ fun HttpUrl.origin(): String = newBuilder() .toString() .removeSuffix("/") -fun URL.hasSameOrigin(other: URL?): Boolean = origin()?.let { it == other?.origin() } ?: false +fun URL.hasSameOrigin(other: URL?): Boolean = toString().toHttpUrlOrNull()?.hasSameOrigin(other) == true + +fun HttpUrl.hasSameOrigin(other: URL?): Boolean = origin() == other?.origin() fun String.toBaseURL(): URL { val url = this.toURL() diff --git a/src/main/kotlin/com/coder/toolbox/views/CoderSetupWizardPage.kt b/src/main/kotlin/com/coder/toolbox/views/CoderSetupWizardPage.kt index 7b742298..55bab271 100644 --- a/src/main/kotlin/com/coder/toolbox/views/CoderSetupWizardPage.kt +++ b/src/main/kotlin/com/coder/toolbox/views/CoderSetupWizardPage.kt @@ -48,14 +48,14 @@ class CoderSetupWizardPage private constructor( override val fields: MutableStateFlow> = MutableStateFlow(emptyList()) override val actionButtons: MutableStateFlow> = MutableStateFlow(emptyList()) - fun pageForDisplay(): CoderSetupWizardPage? = when (model.currentStep()) { - WizardStep.CONNECT -> if (model.connectionOrigin == ConnectionOrigin.SAVED_SESSION) null else this - WizardStep.SIGNED_IN -> null + fun pageForDisplay(): CoderSetupWizardPage? = when (val step = model.currentStep()) { + is WizardStep.Connecting -> if (step.origin == ConnectionOrigin.SAVED_SESSION) null else this + WizardStep.SignedIn -> null else -> this } internal fun startSavedSessionRestore() { - if (model.connectionOrigin == ConnectionOrigin.SAVED_SESSION) connectStep.startConnection() + if (model.currentStep() == WizardStep.Connecting(ConnectionOrigin.SAVED_SESSION)) connectStep.startConnection() } override fun beforeShow() { @@ -77,7 +77,7 @@ class CoderSetupWizardPage private constructor( private fun displayStep(step: WizardStep) { when (step) { - WizardStep.URL_REQUEST -> { + WizardStep.UrlRequest -> { fields.update { listOf(deploymentUrlStep.panel) } @@ -90,7 +90,7 @@ class CoderSetupWizardPage private constructor( deploymentUrlStep.onVisible() } - WizardStep.TOKEN_REQUEST -> { + WizardStep.TokenRequest -> { fields.update { listOf(tokenStep.panel) } @@ -104,7 +104,7 @@ class CoderSetupWizardPage private constructor( tokenStep.onVisible() } - WizardStep.CONNECT -> { + is WizardStep.Connecting -> { fields.update { listOf(connectStep.panel) } @@ -119,7 +119,7 @@ class CoderSetupWizardPage private constructor( // Toolbox can bring this wizard back after Settings or during startup. // Ask for the workspace list when the completed wizard is actually shown. - WizardStep.SIGNED_IN -> context.envPageManager.showPluginEnvironmentsPage(false) + WizardStep.SignedIn -> context.envPageManager.showPluginEnvironmentsPage(false) } } @@ -130,14 +130,14 @@ class CoderSetupWizardPage private constructor( } private fun navigateBackFromConnect() { - if (model.connectionOrigin == ConnectionOrigin.FORM) { - model.goTo(if (context.settingsStore.requiresTokenAuth) WizardStep.TOKEN_REQUEST else WizardStep.URL_REQUEST) + val phase = model.currentStep() as? WizardStep.Connecting ?: return + if (phase.origin == ConnectionOrigin.FORM) { + model.goTo(if (context.settingsStore.requiresTokenAuth) WizardStep.TokenRequest else WizardStep.UrlRequest) return } - val revealForm = model.connectionOrigin == ConnectionOrigin.SAVED_SESSION + val revealForm = phase.origin == ConnectionOrigin.SAVED_SESSION model.clearFormData() model.goToFirst() - model.connectionOrigin = ConnectionOrigin.FORM if (revealForm) { context.cs.launch { if (!disposed) context.ui.showUiPageSuspending(this@CoderSetupWizardPage) @@ -188,14 +188,13 @@ class CoderSetupWizardPage private constructor( onConnect = onConnect, onTokenRefreshed = onTokenRefreshed, ).apply { - model.connectionOrigin = origin model.url = url when (credentials) { is Credentials.MTls -> Unit is Credentials.Token -> model.token = credentials.value is Credentials.OAuth -> model.oauthSession = credentials.session } - model.goTo(WizardStep.CONNECT) + model.goTo(WizardStep.Connecting(origin)) } } } diff --git a/src/main/kotlin/com/coder/toolbox/views/ConnectStep.kt b/src/main/kotlin/com/coder/toolbox/views/ConnectStep.kt index 14cad55c..75a79780 100644 --- a/src/main/kotlin/com/coder/toolbox/views/ConnectStep.kt +++ b/src/main/kotlin/com/coder/toolbox/views/ConnectStep.kt @@ -10,8 +10,8 @@ import com.coder.toolbox.util.origin import com.coder.toolbox.views.state.CoderOAuthSessionContext import com.coder.toolbox.views.state.ConnectionOrigin import com.coder.toolbox.views.state.WizardModel -import com.coder.toolbox.views.state.WizardStep.CONNECT -import com.coder.toolbox.views.state.WizardStep.SIGNED_IN +import com.coder.toolbox.views.state.WizardStep.Connecting +import com.coder.toolbox.views.state.WizardStep.SignedIn import com.jetbrains.toolbox.api.ui.components.LabelField import com.jetbrains.toolbox.api.ui.components.RowGroup import com.jetbrains.toolbox.api.ui.components.ValidationErrorField @@ -62,7 +62,7 @@ class ConnectStep( @Synchronized private fun prepareConnection(): Job? { - if (disposed || signInJob != null || model.currentStep() != CONNECT) return null + if (disposed || signInJob != null || model.currentStep() !is Connecting) return null errorField.textState.update { context.i18n.pnotr("") } if (context.settingsStore.requiresTokenAuth && model.isNotReadyForAuth()) { @@ -160,7 +160,7 @@ class ConnectStep( private fun reportConnectionFailure(job: Job, url: URL, ex: Exception) { job.ensureActive() if (signInJob !== job) return - if (model.currentStep() == SIGNED_IN) { + if (model.currentStep() == SignedIn) { context.logger.warn(ex, "Failed to update session credentials after sign-in") return } @@ -179,7 +179,8 @@ class ConnectStep( @Synchronized override fun onBack() { - if (disposed || model.currentStep() != CONNECT || model.connectionOrigin == ConnectionOrigin.SAVED_SESSION) return + val phase = model.currentStep() as? Connecting ?: return + if (disposed || phase.origin == ConnectionOrigin.SAVED_SESSION) return try { context.logger.info("Back button was pressed, cancelling in-progress connection setup...") signInJob?.cancel(CancellationException(USER_HIT_THE_BACK_BUTTON)) diff --git a/src/main/kotlin/com/coder/toolbox/views/DeploymentUrlStep.kt b/src/main/kotlin/com/coder/toolbox/views/DeploymentUrlStep.kt index ceac44b3..51a9f2b3 100644 --- a/src/main/kotlin/com/coder/toolbox/views/DeploymentUrlStep.kt +++ b/src/main/kotlin/com/coder/toolbox/views/DeploymentUrlStep.kt @@ -12,8 +12,8 @@ import com.coder.toolbox.util.toURL import com.coder.toolbox.util.validateStrictWebUrl import com.coder.toolbox.views.state.CoderOAuthSessionContext import com.coder.toolbox.views.state.WizardModel -import com.coder.toolbox.views.state.WizardStep.CONNECT -import com.coder.toolbox.views.state.WizardStep.TOKEN_REQUEST +import com.coder.toolbox.views.state.WizardStep.Connecting +import com.coder.toolbox.views.state.WizardStep.TokenRequest import com.jetbrains.toolbox.api.localization.LocalizableString import com.jetbrains.toolbox.api.ui.components.CheckboxField import com.jetbrains.toolbox.api.ui.components.FieldModifier @@ -90,7 +90,7 @@ class DeploymentUrlStep( } if (context.settingsStore.requiresMTlsAuth) { - model.goTo(CONNECT) + model.goTo(Connecting()) return true } if (context.settingsStore.requiresTokenAuth && context.settingsStore.preferOAuth2IfAvailable) { @@ -104,7 +104,7 @@ class DeploymentUrlStep( } } // if all else fails try the good old API token auth - model.goTo(TOKEN_REQUEST) + model.goTo(TokenRequest) return true } diff --git a/src/main/kotlin/com/coder/toolbox/views/TokenStep.kt b/src/main/kotlin/com/coder/toolbox/views/TokenStep.kt index 42b290e6..b56ab428 100644 --- a/src/main/kotlin/com/coder/toolbox/views/TokenStep.kt +++ b/src/main/kotlin/com/coder/toolbox/views/TokenStep.kt @@ -3,8 +3,8 @@ package com.coder.toolbox.views import com.coder.toolbox.CoderToolboxContext import com.coder.toolbox.util.withPath import com.coder.toolbox.views.state.WizardModel -import com.coder.toolbox.views.state.WizardStep.CONNECT -import com.coder.toolbox.views.state.WizardStep.URL_REQUEST +import com.coder.toolbox.views.state.WizardStep.Connecting +import com.coder.toolbox.views.state.WizardStep.UrlRequest import com.jetbrains.toolbox.api.localization.LocalizableString import com.jetbrains.toolbox.api.ui.components.FieldModifier import com.jetbrains.toolbox.api.ui.components.LinkField @@ -58,12 +58,12 @@ class TokenStep( } model.token = token - model.goTo(CONNECT) + model.goTo(Connecting()) return true } override fun onBack() { - model.goTo(URL_REQUEST) + model.goTo(UrlRequest) } private fun reportError(message: LocalizableString?) { diff --git a/src/main/kotlin/com/coder/toolbox/views/state/PageRouter.kt b/src/main/kotlin/com/coder/toolbox/views/state/PageRouter.kt index e428ba35..154b5dbb 100644 --- a/src/main/kotlin/com/coder/toolbox/views/state/PageRouter.kt +++ b/src/main/kotlin/com/coder/toolbox/views/state/PageRouter.kt @@ -57,6 +57,6 @@ class PageRouter { startSession() val wizard = activePage ?: return activePage = null - wizard.model.goTo(WizardStep.SIGNED_IN) + wizard.model.goTo(WizardStep.SignedIn) } } diff --git a/src/main/kotlin/com/coder/toolbox/views/state/WizardModel.kt b/src/main/kotlin/com/coder/toolbox/views/state/WizardModel.kt index 0251c13e..98f13787 100644 --- a/src/main/kotlin/com/coder/toolbox/views/state/WizardModel.kt +++ b/src/main/kotlin/com/coder/toolbox/views/state/WizardModel.kt @@ -12,14 +12,12 @@ import java.net.URL * cycles without leaking across wizard recreations. */ class WizardModel { - private val _step: MutableStateFlow = MutableStateFlow(WizardStep.URL_REQUEST) + private val _step: MutableStateFlow = MutableStateFlow(WizardStep.UrlRequest) val step: StateFlow = _step var url: URL? = null var token: String? = null var oauthSession: CoderOAuthSessionContext? = null - @Volatile - var connectionOrigin = ConnectionOrigin.FORM fun currentStep(): WizardStep = _step.value @@ -28,7 +26,7 @@ class WizardModel { } fun goToFirst() { - _step.value = WizardStep.URL_REQUEST + _step.value = WizardStep.UrlRequest } fun hasUrl(): Boolean = url != null @@ -44,8 +42,11 @@ class WizardModel { } } -enum class WizardStep { - URL_REQUEST, TOKEN_REQUEST, CONNECT, SIGNED_IN; +sealed interface WizardStep { + data object UrlRequest : WizardStep + data object TokenRequest : WizardStep + data class Connecting(val origin: ConnectionOrigin = ConnectionOrigin.FORM) : WizardStep + data object SignedIn : WizardStep } enum class ConnectionOrigin { diff --git a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt index 9725f768..eef4a809 100644 --- a/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt +++ b/src/test/kotlin/com/coder/toolbox/CoderRemoteProviderTest.kt @@ -27,6 +27,7 @@ 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.ConnectionOrigin import com.coder.toolbox.views.state.WizardStep import com.coder.toolbox.views.state.toSessionContext import com.jetbrains.toolbox.api.core.diagnostics.Logger @@ -729,7 +730,7 @@ class CoderRemoteProviderTest { assertNull(remoteProvider.getOverrideUiPage()) val restoringWizard = setupWizard(remoteProvider) assertEquals(restoringDeployment, restoringWizard.model.url) - assertEquals(WizardStep.CONNECT, restoringWizard.model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SAVED_SESSION), restoringWizard.model.currentStep()) val displayedWizard = slot() coEvery { mockUi.showUiPageSuspending(capture(displayedWizard)) } returns Unit @@ -738,7 +739,10 @@ class CoderRemoteProviderTest { // Toolbox has not requested the override page again since the link arrived. coVerify(exactly = 1) { mockUi.showUiPageSuspending(any()) } val replacementWizard = displayedWizard.captured - assertEquals(WizardStep.CONNECT, replacementWizard.model.currentStep()) + assertEquals( + WizardStep.Connecting(ConnectionOrigin.SUPPLIED_CREDENTIALS), + replacementWizard.model.currentStep(), + ) assertEquals(expectedOrigin, replacementWizard.model.url.toString()) assertEquals("link-token", replacementWizard.model.token) assertSame(replacementWizard, remoteProvider.getOverrideUiPage()) @@ -900,7 +904,7 @@ class CoderRemoteProviderTest { provider.handleUri(workspaceLink("https://CODER.example.com:443/different/path")) val page = provider.getOverrideUiPage() as CoderSetupWizardPage - assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SUPPLIED_CREDENTIALS), 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()) } @@ -1031,7 +1035,7 @@ class CoderRemoteProviderTest { assertNull(provider.getOverrideUiPage()) assertSame(existingWizard, setupWizard(provider)) - assertEquals(WizardStep.CONNECT, existingWizard.model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SAVED_SESSION), existingWizard.model.currentStep()) assertEquals("saved-token", existingWizard.model.token) verify(exactly = 0) { mockContext.popupPluginMainPage() } } @@ -1140,7 +1144,7 @@ class CoderRemoteProviderTest { val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage assertEquals("https://coder.example.com", page.model.url.toString()) - assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SUPPLIED_CREDENTIALS), page.model.currentStep()) assertNull(page.model.token) coVerify(exactly = 0) { mockUi.showYesNoPopup(any(), any(), any(), any()) } } @@ -1255,7 +1259,7 @@ class CoderRemoteProviderTest { remoteProvider.handleUri(workspaceLink("https://other.example.com")) assertSame(page, remoteProvider.getOverrideUiPage()) - assertEquals(WizardStep.URL_REQUEST, page.model.currentStep()) + assertEquals(WizardStep.UrlRequest, page.model.currentStep()) verify(exactly = 0) { mockContext.popupPluginMainPage() } } @@ -1286,7 +1290,7 @@ class CoderRemoteProviderTest { coVerify(exactly = 1) { mockUi.showYesNoPopup(any(), any(), any(), any()) } val page = remoteProvider.getOverrideUiPage() as CoderSetupWizardPage - assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SUPPLIED_CREDENTIALS), page.model.currentStep()) assertNull(page.model.token) } @@ -1375,7 +1379,7 @@ class CoderRemoteProviderTest { assertNull(remoteProvider.getOverrideUiPage()) val model = setupWizard(remoteProvider).model - assertEquals(WizardStep.CONNECT, model.currentStep()) + assertEquals(WizardStep.Connecting(ConnectionOrigin.SAVED_SESSION), model.currentStep()) assertEquals(expectedApiToken, model.token) assertEquals(if (expectOAuth) requireNotNull(savedOAuthSession).toSessionContext() else null, model.oauthSession) } @@ -1395,7 +1399,7 @@ class CoderRemoteProviderTest { val overridePage = provider.getOverrideUiPage() as CoderSetupWizardPage // then - assertEquals(WizardStep.URL_REQUEST, overridePage.model.currentStep()) + assertEquals(WizardStep.UrlRequest, overridePage.model.currentStep()) assertNull(overridePage.model.token) assertNull(overridePage.model.oauthSession) } @@ -1416,7 +1420,7 @@ class CoderRemoteProviderTest { val overridePage = provider.getOverrideUiPage() as CoderSetupWizardPage // then - assertEquals(WizardStep.URL_REQUEST, overridePage.model.currentStep()) + assertEquals(WizardStep.UrlRequest, overridePage.model.currentStep()) assertNull(overridePage.model.token) assertNull(overridePage.model.oauthSession) } diff --git a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt index 02adc5c1..9231bb3d 100644 --- a/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt +++ b/src/test/kotlin/com/coder/toolbox/cli/CoderCLIManagerTest.kt @@ -61,6 +61,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout +import org.junit.jupiter.api.Assumptions.assumeTrue import org.junit.jupiter.api.BeforeAll import org.junit.jupiter.api.assertDoesNotThrow import org.junit.jupiter.api.condition.EnabledOnOs @@ -486,12 +487,8 @@ internal class CoderCLIManagerTest { // download a working CLI and that it runs on each platform. @Test fun testDownloadRealCLI() { - var url = System.getenv("CODER_TOOLBOX_TEST_DEPLOYMENT") - if (url == "mock") { - return - } else if (url == null) { - url = "https://dev.coder.com" - } + val url = System.getenv("CODER_TOOLBOX_TEST_DEPLOYMENT") + assumeTrue(!url.isNullOrBlank() && url != "mock", "Set a deployment that serves its CLI without a cross-origin redirect.") val ccm = CoderCLIManager( context.copy( @@ -504,7 +501,7 @@ internal class CoderCLIManagerTest { context.logger ) ), - url.toURL(), + requireNotNull(url).toURL(), ) assertTrue(runBlocking { ccm.download(VERSION_FOR_PROGRESS_REPORTING, noOpTextProgress) }) @@ -734,15 +731,17 @@ internal class CoderCLIManagerTest { session.writeText("old-token") val loginArgs = cli.login("token") + assertEquals(version != "v2.29.0", "--allow-redirects=false" in loginArgs) 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 agent = workspace.latestBuild.resources.firstNotNullOf { it.agents?.firstOrNull() } 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") + assertEquals(version != "v2.29.0", "--allow-redirects=false" in Files.readString(config)) assertContains(cli.startWorkspace(address), "--use-keyring=true") assertContains(cli.logout(), "--url $url --use-keyring=true") } @@ -1207,7 +1206,7 @@ internal class CoderCLIManagerTest { val sessionId = SessionId.generate() val sessionPrefix = "client_session_id=$sessionId" val workspace = workspace("foo", agents = mapOf("agent" to UUID.randomUUID().toString())) - val agent = workspace.latestBuild.resources.single().agents!!.single() + val agent = workspace.latestBuild.resources.single().agents.orEmpty().single() ccm.configSsh( setOf(WorkspaceAddress.from(workspace, agent)), @@ -1231,8 +1230,8 @@ internal class CoderCLIManagerTest { verify(exactly = 1) { underlyingLogger.info("$sessionPrefix Finished configuring SSH config") } - // SSH setup no longer runs the CLI to detect features. - verify(exactly = 0) { + // SSH setup checks the binary once for redirect flag support. + verify(exactly = 1) { underlyingLogger.info(match { it.contains("${ccm.localBinaryPath} version --output json") }) } } @@ -1329,7 +1328,12 @@ internal class CoderCLIManagerTest { testDirectory.resolve("coder") } binaryPath.parent.toFile().mkdirs() - binaryPath.toFile().writeText(mkbin(script)) + val versionCommand = if (getOS() == OS.WINDOWS) { + """if "%1"=="version" (echo {"version":"2.38.0"} & exit /b 0)""" + } else { + """if [ "$1" = version ]; then echo '{"version":"2.38.0"}'; exit 0; fi""" + } + binaryPath.toFile().writeText(mkbin(versionCommand + System.lineSeparator() + script)) if (getOS() != OS.WINDOWS) { binaryPath.toFile().setExecutable(true) } diff --git a/src/test/kotlin/com/coder/toolbox/sdk/DeploymentRedirectTest.kt b/src/test/kotlin/com/coder/toolbox/sdk/DeploymentRedirectTest.kt new file mode 100644 index 00000000..c685becd --- /dev/null +++ b/src/test/kotlin/com/coder/toolbox/sdk/DeploymentRedirectTest.kt @@ -0,0 +1,92 @@ +package com.coder.toolbox.sdk + +import com.coder.toolbox.sdk.interceptors.Interceptors +import com.sun.net.httpserver.HttpServer +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody +import java.net.InetSocketAddress +import java.net.ProtocolException +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +class DeploymentRedirectTest { + @Test + fun `reads can follow a relative redirect but cannot leave their origin`() { + val first = HttpServer.create(InetSocketAddress("127.0.0.1", 0), 0) + val second = HttpServer.create(InetSocketAddress("127.0.0.1", 0), 0) + val visits = AtomicInteger() + val origin = "http://127.0.0.1:${first.address.port}" + val http = OkHttpClient.Builder().followRedirects(false).addInterceptor(Interceptors.sameOriginRedirects()).build() + first.createContext("/local") { exchange -> + exchange.responseHeaders.add("Location", "/result") + exchange.sendResponseHeaders(302, -1) + exchange.close() + } + first.createContext("/result") { exchange -> + exchange.sendResponseHeaders(204, -1) + exchange.close() + } + first.createContext("/away") { exchange -> + exchange.responseHeaders.add("Location", "http://127.0.0.1:${second.address.port}/") + exchange.sendResponseHeaders(307, -1) + exchange.close() + } + second.createContext("/") { exchange -> + visits.incrementAndGet() + exchange.sendResponseHeaders(204, -1) + exchange.close() + } + try { + first.start() + second.start() + val local = Request.Builder().url("$origin/local").build() + http.newCall(local).execute().use { assertEquals(204, it.code) } + http.newCall(local.newBuilder().post("".toRequestBody()).build()).execute().use { assertEquals(302, it.code) } + val away = Request.Builder().url("$origin/away").build() + assertFailsWith { http.newCall(away).execute().close() } + assertEquals(0, visits.get()) + } finally { + first.stop(0) + second.stop(0) + http.connectionPool.evictAll() + } + } + + @Test + fun `HEAD follows twenty redirects and stops before the twenty-first target`() { + val server = HttpServer.create(InetSocketAddress("127.0.0.1", 0), 0) + val visits = AtomicInteger() + server.createContext("/") { exchange -> + visits.incrementAndGet() + val remaining = exchange.requestURI.path.removePrefix("/").toInt() + when { + exchange.requestMethod != "HEAD" -> exchange.sendResponseHeaders(405, -1) + remaining == 0 -> exchange.sendResponseHeaders(204, -1) + else -> { + exchange.responseHeaders.add("Location", "/${remaining - 1}") + exchange.sendResponseHeaders(302, -1) + } + } + exchange.close() + } + val client = OkHttpClient.Builder().followRedirects(false).addInterceptor(Interceptors.sameOriginRedirects()).build() + try { + server.start() + val origin = "http://127.0.0.1:${server.address.port}" + val request = Request.Builder().url("$origin/20").head().build() + client.newCall(request).execute().use { assertEquals(204, it.code) } + assertEquals(21, visits.get()) + + visits.set(0) + val tooMany = request.newBuilder().url("$origin/21").build() + assertFailsWith { client.newCall(tooMany).execute().close() } + assertEquals(21, visits.get()) + } finally { + server.stop(0) + client.connectionPool.evictAll() + } + } +} diff --git a/src/test/kotlin/com/coder/toolbox/views/CoderSetupWizardPageTest.kt b/src/test/kotlin/com/coder/toolbox/views/CoderSetupWizardPageTest.kt index 5cf3adc2..4b177f39 100644 --- a/src/test/kotlin/com/coder/toolbox/views/CoderSetupWizardPageTest.kt +++ b/src/test/kotlin/com/coder/toolbox/views/CoderSetupWizardPageTest.kt @@ -57,7 +57,7 @@ class CoderSetupWizardPageTest { private val model = WizardModel().apply { url = this@CoderSetupWizardPageTest.url token = "test-token" - goTo(WizardStep.CONNECT) + goTo(WizardStep.Connecting()) } private val onConnect = mockk>(relaxed = true) @@ -104,15 +104,14 @@ class CoderSetupWizardPageTest { assertNull(page.pageForDisplay()) runCurrent() - assertEquals(WizardStep.URL_REQUEST, page.model.currentStep()) - assertEquals(ConnectionOrigin.FORM, page.model.connectionOrigin) + assertEquals(WizardStep.UrlRequest, page.model.currentStep()) assertSame(page, page.pageForDisplay()) coVerify(exactly = 1) { ui.showUiPageSuspending(page) } coVerify(exactly = 0) { onConnect.accept(any(), any()) } page.model.url = url page.model.token = "test-token" - page.model.goTo(WizardStep.CONNECT) + page.model.goTo(WizardStep.Connecting()) page.beforeShow() runCurrent() coVerify(exactly = 2) { cli.login("test-token") } @@ -151,7 +150,7 @@ class CoderSetupWizardPageTest { page.beforeShow() runCurrent() - assertEquals(WizardStep.SIGNED_IN, page.model.currentStep()) + assertEquals(WizardStep.SignedIn, page.model.currentStep()) verify(exactly = 1) { pages.showPluginEnvironmentsPage(false) } verify(exactly = 1) { log.warn(failure, any()) } verify(exactly = 0) { logger.showInfoPopup(any(), any()) } @@ -169,14 +168,14 @@ class CoderSetupWizardPageTest { justRun { anyConstructed().onVisible() } val page = CoderSetupWizardPage.deploymentUrlStep(context, mockk(), mockk()) page.model.url = url - page.model.goTo(WizardStep.TOKEN_REQUEST) + page.model.goTo(WizardStep.TokenRequest) page.beforeShow() runCurrent() page.actionButtons.value.first().run() runCurrent() - assertEquals(WizardStep.CONNECT, page.model.currentStep()) + assertEquals(WizardStep.Connecting(), page.model.currentStep()) verify(exactly = 1) { anyConstructed().onVisible() } page.dispose() @@ -192,7 +191,7 @@ class CoderSetupWizardPageTest { @EnumSource(ConnectionOrigin::class) @ParameterizedTest(name = "Repeated visibility during sign-in from {0}") fun `a second visibility callback during startup logs in to the CLI once`(origin: ConnectionOrigin) = runTest { - model.connectionOrigin = origin + model.goTo(WizardStep.Connecting(origin)) lateinit var step: ConnectStep val scheduler = StandardTestDispatcher(testScheduler) val dispatcher = object : CoroutineDispatcher() { @@ -208,7 +207,7 @@ class CoderSetupWizardPageTest { } } every { context.cs } returns CoroutineScope(backgroundScope.coroutineContext + dispatcher) - step = ConnectStep(context, model, { model.goTo(WizardStep.TOKEN_REQUEST) }, onConnect) + step = ConnectStep(context, model, { model.goTo(WizardStep.TokenRequest) }, onConnect) withMockCli { cli -> step.onVisible() runCurrent() @@ -222,7 +221,7 @@ class CoderSetupWizardPageTest { fun `certificate sign-in connects without a CLI login`() = runTest { every { context.cs } returns backgroundScope model.token = null - val step = ConnectStep(context, model, { model.goTo(WizardStep.URL_REQUEST) }, onConnect) + val step = ConnectStep(context, model, { model.goTo(WizardStep.UrlRequest) }, onConnect) withMockCli { cli -> val settings = spyk(context.settingsStore) every { settings.requiresTokenAuth } returns false @@ -240,7 +239,7 @@ class CoderSetupWizardPageTest { fun `switching from certificates to token authentication during setup reports the missing token`() = runTest { every { context.cs } returns backgroundScope model.token = null - val step = ConnectStep(context, model, { model.goTo(WizardStep.TOKEN_REQUEST) }, onConnect) + val step = ConnectStep(context, model, { model.goTo(WizardStep.TokenRequest) }, onConnect) val logger = context.logger withMockCli { cli -> val settings = spyk(context.settingsStore) @@ -254,7 +253,7 @@ class CoderSetupWizardPageTest { step.onVisible() runCurrent() - assertEquals(WizardStep.TOKEN_REQUEST, model.currentStep()) + assertEquals(WizardStep.TokenRequest, model.currentStep()) verify { logger.showInfoPopup(any(), match { it.contains("OAuth or API token is required") }) } coVerify(exactly = 0) { cli.login(any()) } coVerify(exactly = 0) { onConnect.accept(any(), any()) } @@ -284,7 +283,7 @@ class CoderSetupWizardPageTest { @Test fun `a retry waits for a cancelled login and ignores its late error`() = runTest { every { context.cs } returns backgroundScope - val step = ConnectStep(context, model, { model.goTo(WizardStep.TOKEN_REQUEST) }, onConnect) + val step = ConnectStep(context, model, { model.goTo(WizardStep.TokenRequest) }, onConnect) val allowCleanup = CompletableDeferred() withMockCli { cli -> coEvery { cli.login("test-token") } coAnswers { @@ -299,10 +298,10 @@ class CoderSetupWizardPageTest { step.onVisible() runCurrent() step.onBack() - assertEquals(WizardStep.TOKEN_REQUEST, model.currentStep()) + assertEquals(WizardStep.TokenRequest, model.currentStep()) // Retry while the cancelled CLI login is still waiting for cleanup. - model.goTo(WizardStep.CONNECT) + model.goTo(WizardStep.Connecting()) step.onVisible() runCurrent() coVerify(exactly = 1) { cli.login("test-token") } @@ -310,7 +309,7 @@ class CoderSetupWizardPageTest { allowCleanup.complete(Unit) runCurrent() - assertEquals(WizardStep.CONNECT, model.currentStep()) + assertEquals(WizardStep.Connecting(), model.currentStep()) coVerify(exactly = 2) { cli.login("test-token") } coVerify(exactly = 1) { onConnect.accept(any(), cli) } } finally { @@ -323,15 +322,15 @@ class CoderSetupWizardPageTest { @Test fun `a genuine CLI failure returns to token entry and allows a new sign-in`() = runTest { every { context.cs } returns backgroundScope - val step = ConnectStep(context, model, { model.goTo(WizardStep.TOKEN_REQUEST) }, onConnect) + val step = ConnectStep(context, model, { model.goTo(WizardStep.TokenRequest) }, onConnect) withMockCli { cli -> coEvery { cli.login("test-token") } throws IllegalStateException("Keychain is locked") andThen "Signed in" step.onVisible() runCurrent() - assertEquals(WizardStep.TOKEN_REQUEST, model.currentStep()) + assertEquals(WizardStep.TokenRequest, model.currentStep()) coVerify(exactly = 0) { onConnect.accept(any(), any()) } - model.goTo(WizardStep.CONNECT) + model.goTo(WizardStep.Connecting()) step.onVisible() runCurrent()