fix: address OAuth review findings (redirect_uri, binding, timeout, CRLF, DRY)

This commit is contained in:
2026-07-08 17:46:40 +02:00
parent 5dc206ae72
commit 00e7a7456f
4 changed files with 97 additions and 69 deletions
@@ -4,6 +4,8 @@ import android.content.Context
import android.content.Intent import android.content.Intent
import android.net.Uri import android.net.Uri
import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.TimeoutCancellationException
import kotlinx.coroutines.withTimeout
class AndroidOAuthClient(private val context: Context) : OAuthClient { class AndroidOAuthClient(private val context: Context) : OAuthClient {
@@ -17,15 +19,20 @@ class AndroidOAuthClient(private val context: Context) : OAuthClient {
} }
context.startActivity(intent) context.startActivity(intent)
val callbackUri = OAuthCallbackHolder.deferred?.await() ?: return null return try {
withTimeout(TIMEOUT_MS) {
val fragment = callbackUri.fragment ?: return null val callbackUri = OAuthCallbackHolder.deferred?.await() ?: return@withTimeout null
val params = fragment.split("&").associate { pair -> parseAccessTokenFromFragment(callbackUri.fragment ?: "")
val idx = pair.indexOf("=") }
if (idx >= 0) pair.substring(0, idx) to pair.substring(idx + 1) } catch (e: TimeoutCancellationException) {
else pair to "" null
} finally {
OAuthCallbackHolder.deferred = null
} }
return params["access_token"] }
companion object {
private const val TIMEOUT_MS = 5L * 60 * 1000
} }
} }
@@ -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"]
}
@@ -8,23 +8,23 @@ import java.io.PrintWriter
import java.net.InetSocketAddress import java.net.InetSocketAddress
import java.net.ServerSocket import java.net.ServerSocket
import java.net.URI import java.net.URI
import java.net.URLDecoder
import java.net.URLEncoder import java.net.URLEncoder
import kotlin.concurrent.thread import kotlin.concurrent.thread
class DesktopOAuthClient : OAuthClient { class DesktopOAuthClient : OAuthClient {
private var serverPort: Int = 0 override fun redirectUri(): String = "myice://callback"
override fun redirectUri(): String = "http://localhost:$serverPort/callback"
override suspend fun authenticate(loginUrl: String): String? { override suspend fun authenticate(loginUrl: String): String? {
val server = ServerSocket() val server = ServerSocket()
server.bind(InetSocketAddress(0)) server.bind(InetSocketAddress("127.0.0.1", 0))
serverPort = server.localPort val serverPort = server.localPort
val redirectUri = "http://localhost:$serverPort/callback" val realRedirectUri = "http://localhost:$serverPort/callback"
val fullLoginUrl = loginUrl + "&redirect_uri=" + URLEncoder.encode(redirectUri, "UTF-8") val fullLoginUrl = loginUrl.replace(
"redirect_uri=myice://callback",
"redirect_uri=" + URLEncoder.encode(realRedirectUri, "UTF-8")
)
val deferred = CompletableDeferred<String?>() val deferred = CompletableDeferred<String?>()
@@ -47,28 +47,22 @@ class DesktopOAuthClient : OAuthClient {
</script> </script>
</body></html> </body></html>
""".trimIndent() """.trimIndent()
writer.println("HTTP/1.1 200 OK") writer.print("HTTP/1.1 200 OK\r\n")
writer.println("Content-Type: text/html") writer.print("Content-Type: text/html\r\n")
writer.println("Connection: close") writer.print("Connection: close\r\n")
writer.println() writer.print("\r\n")
writer.println(html) writer.print(html)
writer.flush() writer.flush()
} else if (path.startsWith("/token")) { } else if (path.startsWith("/token")) {
val query = path.substringAfter("?") val query = path.substringAfter("?")
val params = query.split("&").associate { pair -> val token = parseAccessTokenFromFragment(query)
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"]
deferred.complete(token) deferred.complete(token)
writer.println("HTTP/1.1 200 OK") writer.print("HTTP/1.1 200 OK\r\n")
writer.println("Content-Type: text/html") writer.print("Content-Type: text/html\r\n")
writer.println("Connection: close") writer.print("Connection: close\r\n")
writer.println() writer.print("\r\n")
writer.println("<html><body><h1>Authentication complete. You can close this window.</h1></body></html>") writer.print("<html><body><h1>Authentication complete. You can close this window.</h1></body></html>")
writer.flush() writer.flush()
socket.close() socket.close()
break 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()
}
} }
} }
@@ -1,6 +1,9 @@
package ch.parano.myice.auth package ch.parano.myice.auth
import kotlinx.cinterop.ExperimentalForeignApi import kotlinx.cinterop.ExperimentalForeignApi
import kotlinx.coroutines.TimeoutCancellationException
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlinx.coroutines.withTimeout
import platform.AuthenticationServices.ASWebAuthenticationPresentationContextProvidingProtocol import platform.AuthenticationServices.ASWebAuthenticationPresentationContextProvidingProtocol
import platform.AuthenticationServices.ASWebAuthenticationSession import platform.AuthenticationServices.ASWebAuthenticationSession
import platform.Foundation.NSError import platform.Foundation.NSError
@@ -10,53 +13,54 @@ import platform.UIKit.UIWindow
import platform.UIKit.UIWindowScene import platform.UIKit.UIWindowScene
import platform.darwin.NSObject import platform.darwin.NSObject
import kotlin.coroutines.resume import kotlin.coroutines.resume
import kotlin.coroutines.suspendCoroutine
@OptIn(ExperimentalForeignApi::class) @OptIn(ExperimentalForeignApi::class)
class IosOAuthClient : OAuthClient { class IosOAuthClient : OAuthClient {
override fun redirectUri(): String = "myice://callback" override fun redirectUri(): String = "myice://callback"
override suspend fun authenticate(loginUrl: String): String? = suspendCoroutine { continuation -> override suspend fun authenticate(loginUrl: String): String? = try {
val nsUrl = NSURL.URLWithString(loginUrl) ?: run { withTimeout(TIMEOUT_MS) {
continuation.resume(null) suspendCancellableCoroutine { continuation ->
return@suspendCoroutine val nsUrl = NSURL.URLWithString(loginUrl) ?: run {
}
val session = ASWebAuthenticationSession(
nsUrl,
"myice",
completionHandler = { callbackURL: NSURL?, error: NSError? ->
if (error != null || callbackURL == null) {
continuation.resume(null) continuation.resume(null)
} else { return@suspendCancellableCoroutine
val urlStr = callbackURL.absoluteString ?: "" }
val fragmentIndex = urlStr.indexOf("#")
if (fragmentIndex < 0) { val session = ASWebAuthenticationSession(
continuation.resume(null) nsUrl,
} else { "myice",
val fragment = urlStr.substring(fragmentIndex + 1) completionHandler = { callbackURL: NSURL?, error: NSError? ->
val params = fragment.split("&").associate { pair -> if (error != null || callbackURL == null) {
val idx = pair.indexOf("=") continuation.resume(null)
if (idx >= 0) pair.substring(0, idx) to pair.substring(idx + 1) } else {
else pair to "" 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(), continuation.invokeOnCancellation { session.cancel() }
ASWebAuthenticationPresentationContextProvidingProtocol { session.start()
override fun presentationAnchorForWebAuthenticationSession(
session: ASWebAuthenticationSession
): UIWindow {
return keyWindow()
} }
} }
} catch (e: TimeoutCancellationException) {
session.start() null
} }
private fun keyWindow(): UIWindow { private fun keyWindow(): UIWindow {
@@ -64,4 +68,8 @@ class IosOAuthClient : OAuthClient {
val windowScene = scenes.firstOrNull { it is UIWindowScene } as? UIWindowScene val windowScene = scenes.firstOrNull { it is UIWindowScene } as? UIWindowScene
return (windowScene?.windows?.firstOrNull() as? UIWindow) ?: UIWindow() return (windowScene?.windows?.firstOrNull() as? UIWindow) ?: UIWindow()
} }
companion object {
private const val TIMEOUT_MS = 5L * 60 * 1000
}
} }