From 00e7a7456f272bd70b3907e106a6fcafdc2b015b Mon Sep 17 00:00:00 2001 From: Rene Luria Date: Wed, 8 Jul 2026 17:46:40 +0200 Subject: [PATCH] fix: address OAuth review findings (redirect_uri, binding, timeout, CRLF, DRY) --- .../parano/myice/auth/AndroidOAuthClient.kt | 23 ++++-- .../ch/parano/myice/auth/TokenParser.kt | 10 +++ .../parano/myice/auth/DesktopOAuthClient.kt | 57 +++++++------- .../ch/parano/myice/auth/IosOAuthClient.kt | 76 ++++++++++--------- 4 files changed, 97 insertions(+), 69 deletions(-) create mode 100644 composeApp/src/commonMain/kotlin/ch/parano/myice/auth/TokenParser.kt diff --git a/composeApp/src/androidMain/kotlin/ch/parano/myice/auth/AndroidOAuthClient.kt b/composeApp/src/androidMain/kotlin/ch/parano/myice/auth/AndroidOAuthClient.kt index 7746ef6..1f7be61 100644 --- a/composeApp/src/androidMain/kotlin/ch/parano/myice/auth/AndroidOAuthClient.kt +++ b/composeApp/src/androidMain/kotlin/ch/parano/myice/auth/AndroidOAuthClient.kt @@ -4,6 +4,8 @@ import android.content.Context import android.content.Intent import android.net.Uri import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.withTimeout class AndroidOAuthClient(private val context: Context) : OAuthClient { @@ -17,15 +19,20 @@ class AndroidOAuthClient(private val context: Context) : OAuthClient { } context.startActivity(intent) - val callbackUri = OAuthCallbackHolder.deferred?.await() ?: return null - - val fragment = callbackUri.fragment ?: return null - val params = fragment.split("&").associate { pair -> - val idx = pair.indexOf("=") - if (idx >= 0) pair.substring(0, idx) to pair.substring(idx + 1) - else pair to "" + return try { + withTimeout(TIMEOUT_MS) { + val callbackUri = OAuthCallbackHolder.deferred?.await() ?: return@withTimeout null + parseAccessTokenFromFragment(callbackUri.fragment ?: "") + } + } catch (e: TimeoutCancellationException) { + null + } finally { + OAuthCallbackHolder.deferred = null } - return params["access_token"] + } + + companion object { + private const val TIMEOUT_MS = 5L * 60 * 1000 } } diff --git a/composeApp/src/commonMain/kotlin/ch/parano/myice/auth/TokenParser.kt b/composeApp/src/commonMain/kotlin/ch/parano/myice/auth/TokenParser.kt new file mode 100644 index 0000000..7d7b169 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/ch/parano/myice/auth/TokenParser.kt @@ -0,0 +1,10 @@ +package ch.parano.myice.auth + +internal fun parseAccessTokenFromFragment(fragment: String): String? { + val params = fragment.split("&").associate { pair -> + val idx = pair.indexOf("=") + if (idx >= 0) pair.substring(0, idx) to pair.substring(idx + 1) + else pair to "" + } + return params["access_token"] +} diff --git a/composeApp/src/desktopMain/kotlin/ch/parano/myice/auth/DesktopOAuthClient.kt b/composeApp/src/desktopMain/kotlin/ch/parano/myice/auth/DesktopOAuthClient.kt index b4d8fd9..3bacdba 100644 --- a/composeApp/src/desktopMain/kotlin/ch/parano/myice/auth/DesktopOAuthClient.kt +++ b/composeApp/src/desktopMain/kotlin/ch/parano/myice/auth/DesktopOAuthClient.kt @@ -8,23 +8,23 @@ import java.io.PrintWriter import java.net.InetSocketAddress import java.net.ServerSocket import java.net.URI -import java.net.URLDecoder import java.net.URLEncoder import kotlin.concurrent.thread class DesktopOAuthClient : OAuthClient { - private var serverPort: Int = 0 - - override fun redirectUri(): String = "http://localhost:$serverPort/callback" + override fun redirectUri(): String = "myice://callback" override suspend fun authenticate(loginUrl: String): String? { val server = ServerSocket() - server.bind(InetSocketAddress(0)) - serverPort = server.localPort + server.bind(InetSocketAddress("127.0.0.1", 0)) + val serverPort = server.localPort - val redirectUri = "http://localhost:$serverPort/callback" - val fullLoginUrl = loginUrl + "&redirect_uri=" + URLEncoder.encode(redirectUri, "UTF-8") + val realRedirectUri = "http://localhost:$serverPort/callback" + val fullLoginUrl = loginUrl.replace( + "redirect_uri=myice://callback", + "redirect_uri=" + URLEncoder.encode(realRedirectUri, "UTF-8") + ) val deferred = CompletableDeferred() @@ -47,28 +47,22 @@ class DesktopOAuthClient : OAuthClient { """.trimIndent() - writer.println("HTTP/1.1 200 OK") - writer.println("Content-Type: text/html") - writer.println("Connection: close") - writer.println() - writer.println(html) + writer.print("HTTP/1.1 200 OK\r\n") + writer.print("Content-Type: text/html\r\n") + writer.print("Connection: close\r\n") + writer.print("\r\n") + writer.print(html) writer.flush() } else if (path.startsWith("/token")) { val query = path.substringAfter("?") - val params = query.split("&").associate { pair -> - val idx = pair.indexOf("=") - if (idx >= 0) { - pair.substring(0, idx) to URLDecoder.decode(pair.substring(idx + 1), "UTF-8") - } else pair to "" - } - val token = params["access_token"] + val token = parseAccessTokenFromFragment(query) deferred.complete(token) - writer.println("HTTP/1.1 200 OK") - writer.println("Content-Type: text/html") - writer.println("Connection: close") - writer.println() - writer.println("

Authentication complete. You can close this window.

") + writer.print("HTTP/1.1 200 OK\r\n") + writer.print("Content-Type: text/html\r\n") + writer.print("Connection: close\r\n") + writer.print("\r\n") + writer.print("

Authentication complete. You can close this window.

") writer.flush() socket.close() break @@ -82,8 +76,17 @@ class DesktopOAuthClient : OAuthClient { } } - Desktop.getDesktop().browse(URI(fullLoginUrl)) + try { + Desktop.getDesktop().browse(URI(fullLoginUrl)) + } catch (e: Exception) { + server.close() + return null + } - return deferred.await() + return try { + deferred.await() + } finally { + server.close() + } } } diff --git a/composeApp/src/iosMain/kotlin/ch/parano/myice/auth/IosOAuthClient.kt b/composeApp/src/iosMain/kotlin/ch/parano/myice/auth/IosOAuthClient.kt index 19a98a5..8adfcb0 100644 --- a/composeApp/src/iosMain/kotlin/ch/parano/myice/auth/IosOAuthClient.kt +++ b/composeApp/src/iosMain/kotlin/ch/parano/myice/auth/IosOAuthClient.kt @@ -1,6 +1,9 @@ package ch.parano.myice.auth import kotlinx.cinterop.ExperimentalForeignApi +import kotlinx.coroutines.TimeoutCancellationException +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.withTimeout import platform.AuthenticationServices.ASWebAuthenticationPresentationContextProvidingProtocol import platform.AuthenticationServices.ASWebAuthenticationSession import platform.Foundation.NSError @@ -10,53 +13,54 @@ import platform.UIKit.UIWindow import platform.UIKit.UIWindowScene import platform.darwin.NSObject import kotlin.coroutines.resume -import kotlin.coroutines.suspendCoroutine @OptIn(ExperimentalForeignApi::class) class IosOAuthClient : OAuthClient { override fun redirectUri(): String = "myice://callback" - override suspend fun authenticate(loginUrl: String): String? = suspendCoroutine { continuation -> - val nsUrl = NSURL.URLWithString(loginUrl) ?: run { - continuation.resume(null) - return@suspendCoroutine - } - - val session = ASWebAuthenticationSession( - nsUrl, - "myice", - completionHandler = { callbackURL: NSURL?, error: NSError? -> - if (error != null || callbackURL == null) { + override suspend fun authenticate(loginUrl: String): String? = try { + withTimeout(TIMEOUT_MS) { + suspendCancellableCoroutine { continuation -> + val nsUrl = NSURL.URLWithString(loginUrl) ?: run { continuation.resume(null) - } else { - val urlStr = callbackURL.absoluteString ?: "" - val fragmentIndex = urlStr.indexOf("#") - if (fragmentIndex < 0) { - continuation.resume(null) - } else { - val fragment = urlStr.substring(fragmentIndex + 1) - val params = fragment.split("&").associate { pair -> - val idx = pair.indexOf("=") - if (idx >= 0) pair.substring(0, idx) to pair.substring(idx + 1) - else pair to "" + return@suspendCancellableCoroutine + } + + val session = ASWebAuthenticationSession( + nsUrl, + "myice", + completionHandler = { callbackURL: NSURL?, error: NSError? -> + if (error != null || callbackURL == null) { + continuation.resume(null) + } else { + val urlStr = callbackURL.absoluteString ?: "" + val fragmentIndex = urlStr.indexOf("#") + if (fragmentIndex < 0) { + continuation.resume(null) + } else { + val fragment = urlStr.substring(fragmentIndex + 1) + continuation.resume(parseAccessTokenFromFragment(fragment)) + } } - continuation.resume(params["access_token"]) + } + ) + + session.presentationContextProvider = object : NSObject(), + ASWebAuthenticationPresentationContextProvidingProtocol { + override fun presentationAnchorForWebAuthenticationSession( + session: ASWebAuthenticationSession + ): UIWindow { + return keyWindow() } } - } - ) - session.presentationContextProvider = object : NSObject(), - ASWebAuthenticationPresentationContextProvidingProtocol { - override fun presentationAnchorForWebAuthenticationSession( - session: ASWebAuthenticationSession - ): UIWindow { - return keyWindow() + continuation.invokeOnCancellation { session.cancel() } + session.start() } } - - session.start() + } catch (e: TimeoutCancellationException) { + null } private fun keyWindow(): UIWindow { @@ -64,4 +68,8 @@ class IosOAuthClient : OAuthClient { val windowScene = scenes.firstOrNull { it is UIWindowScene } as? UIWindowScene return (windowScene?.windows?.firstOrNull() as? UIWindow) ?: UIWindow() } + + companion object { + private const val TIMEOUT_MS = 5L * 60 * 1000 + } }