diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransport.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransport.kt index 7139e8338..8cf75240c 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransport.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransport.kt @@ -1,14 +1,13 @@ package dev.obiente.nextcloudnative import dev.obiente.nextcloudnative.app.JvmNetworkRequestAttempt +import dev.obiente.nextcloudnative.app.NextcloudAuthenticatedRequestPolicy import dev.obiente.nextcloudnative.app.NextcloudSession import dev.obiente.nextcloudnative.app.copyBoundedNetworkResponseTo -import dev.obiente.nextcloudnative.app.executeCancellableJvmHttpCall +import dev.obiente.nextcloudnative.app.executeCancellableNextcloudAuthenticatedRequest import dev.obiente.nextcloudnative.app.isFullDetachedFileResponse -import dev.obiente.nextcloudnative.app.nextcloudBasicAuthorization import java.io.FileOutputStream import okhttp3.OkHttpClient -import okhttp3.Request internal suspend fun downloadAndroidDetachedFile( client: OkHttpClient, @@ -28,42 +27,35 @@ internal suspend fun downloadAndroidDetachedFile( require(maximumBytes > 0L) val started = System.nanoTime() val attempt = JvmNetworkRequestAttempt() - val request = Request.Builder() - .url(url) + val request = NextcloudAuthenticatedRequestPolicy(session, userAgent) + .requestBuilder(url) .get() .tag(JvmNetworkRequestAttempt::class.java, attempt) .header("Accept", accept) - .header("User-Agent", userAgent) - .header("Authorization", nextcloudBasicAuthorization(session)) .apply { requestHeaders.forEach { (name, value) -> header(name, value) } } .build() - val call = client.newCall(request) - return executeCancellableJvmHttpCall(client, call) { activeCall, shouldContinue -> - val response = try { - activeCall.execute() - } catch (failure: Throwable) { - onNetworkFailure(started, attempt, failure) - throw failure - } - response.use { - check(isFullDetachedFileResponse(response.code)) { failureMessage(response.code) } - val body = response.body - val contentLength = body.contentLength() - check(contentLength == -1L || contentLength <= maximumBytes) { limitMessage } - val copied = body.byteStream().copyBoundedNetworkResponseTo( - output = output, - maxBytes = maximumBytes, - onLimitExceeded = { error(limitMessage) }, - onNetworkReadFailure = { failure -> onNetworkFailure(started, attempt, failure) }, - shouldContinue = shouldContinue, - ) - val responseEtag = response.header("ETag") ?: response.header("OC-Etag") - validateResponseEtag(responseEtag) - AndroidDetachedDownload( - byteCount = copied, - mimeType = body.contentType()?.toString(), - etag = handoffEtag ?: responseEtag, - ) - } + return executeCancellableNextcloudAuthenticatedRequest( + client = client, + initialRequest = request, + onNetworkFailure = { failure -> onNetworkFailure(started, attempt, failure) }, + ) { response, shouldContinue -> + check(isFullDetachedFileResponse(response.code)) { failureMessage(response.code) } + val body = response.body + val contentLength = body.contentLength() + check(contentLength == -1L || contentLength <= maximumBytes) { limitMessage } + val copied = body.byteStream().copyBoundedNetworkResponseTo( + output = output, + maxBytes = maximumBytes, + onLimitExceeded = { error(limitMessage) }, + onNetworkReadFailure = { failure -> onNetworkFailure(started, attempt, failure) }, + shouldContinue = shouldContinue, + ) + val responseEtag = response.header("ETag") ?: response.header("OC-Etag") + validateResponseEtag(responseEtag) + AndroidDetachedDownload( + byteCount = copied, + mimeType = body.contentType()?.toString(), + etag = handoffEtag ?: responseEtag, + ) } } diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransportTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransportTest.kt index 5f07545dc..3643b5899 100644 --- a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransportTest.kt +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidDetachedDownloadTransportTest.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertTrue import mockwebserver3.MockResponse @@ -23,6 +24,12 @@ class AndroidDetachedDownloadTransportTest { @Test fun `detached download exposes and validates the response etag`() = runBlocking { MockWebServer().use { server -> + server.enqueue( + MockResponse.Builder() + .code(307) + .addHeader("Location", "/cloud/version-2.bin") + .build(), + ) server.enqueue( MockResponse.Builder() .code(200) @@ -36,8 +43,8 @@ class AndroidDetachedDownloadTransportTest { val result = FileOutputStream(destination).use { output -> downloadAndroidDetachedFile( client = OkHttpClient(), - session = NextcloudSession(server.url("/").toString(), "alice", "secret"), - url = server.url("/version.bin").toString(), + session = NextcloudSession(server.url("/cloud").toString(), "alice", "secret"), + url = server.url("/cloud/version.bin").toString(), output = output, maximumBytes = Long.MAX_VALUE, userAgent = "test", @@ -51,12 +58,54 @@ class AndroidDetachedDownloadTransportTest { assertEquals("\"listed-version\"", result.etag) assertEquals("historical", destination.readText()) + val initial = assertNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + val redirected = assertNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + assertEquals("Basic YWxpY2U6c2VjcmV0", initial.headers["Authorization"]) + assertEquals(initial.headers["Authorization"], redirected.headers["Authorization"]) + assertEquals("/cloud/version-2.bin", redirected.url.encodedPath) } finally { destination.delete() } } } + @Test + fun `detached download rejects another origin before sending credentials`() = runBlocking { + MockWebServer().use { accountServer -> + MockWebServer().use { unrelatedServer -> + accountServer.start() + unrelatedServer.start() + val destination = Files.createTempFile("ncn-detached-origin-", ".tmp").toFile() + try { + assertFailsWith { + FileOutputStream(destination).use { output -> + downloadAndroidDetachedFile( + client = OkHttpClient(), + session = NextcloudSession( + accountServer.url("/cloud").toString(), + "alice", + "secret", + ), + url = unrelatedServer.url("/capture.bin").toString(), + output = output, + maximumBytes = Long.MAX_VALUE, + userAgent = "test", + failureMessage = { status -> "HTTP $status" }, + limitMessage = "Too large", + onNetworkFailure = { _, _, _ -> }, + ) + } + } + + assertEquals(0, accountServer.requestCount) + assertEquals(0, unrelatedServer.requestCount) + } finally { + destination.delete() + } + } + } + } + @Test fun `coroutine cancellation cancels an in-flight detached download`() = runBlocking { MockWebServer().use { server -> diff --git a/changes/unreleased/106-detached-download-request-policy.md b/changes/unreleased/106-detached-download-request-policy.md new file mode 100644 index 000000000..0930b9468 --- /dev/null +++ b/changes/unreleased/106-detached-download-request-policy.md @@ -0,0 +1,7 @@ +category: fix +issue: 106 +pull: 432 +platforms: android, desktop +user-facing: yes + +Keep credentials for file exports and attachment downloads inside the configured Nextcloud account, including when the server redirects the download. diff --git a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownload.kt b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownload.kt index b1e0c8b47..a722d6989 100644 --- a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownload.kt +++ b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownload.kt @@ -2,7 +2,6 @@ package dev.obiente.nextcloudnative.app import java.io.FileOutputStream import okhttp3.OkHttpClient -import okhttp3.Request internal suspend fun downloadDesktopDetachedFile( client: OkHttpClient, @@ -22,38 +21,31 @@ internal suspend fun downloadDesktopDetachedFile( require(maximumBytes > 0L) val started = System.nanoTime() val attempt = JvmNetworkRequestAttempt() - val request = Request.Builder() - .url(url) + val request = NextcloudAuthenticatedRequestPolicy(session, userAgent) + .requestBuilder(url) .get() .tag(JvmNetworkRequestAttempt::class.java, attempt) .header("Accept", accept) - .header("User-Agent", userAgent) - .header("Authorization", nextcloudBasicAuthorization(session)) .apply { requestHeaders.forEach { (name, value) -> header(name, value) } } .build() - val call = client.newCall(request) - return executeCancellableJvmHttpCall(client, call) { activeCall, shouldContinue -> - val response = try { - activeCall.execute() - } catch (failure: Throwable) { - onNetworkFailure(started, attempt, failure) - throw failure - } - response.use { - check(isFullDetachedFileResponse(response.code)) { failureMessage(response.code) } - val body = response.body - val contentLength = body.contentLength() - check(contentLength == -1L || contentLength <= maximumBytes) { limitMessage } - val copied = body.byteStream().copyBoundedNetworkResponseTo( - output = output, - maxBytes = maximumBytes, - onLimitExceeded = { error(limitMessage) }, - onNetworkReadFailure = { failure -> onNetworkFailure(started, attempt, failure) }, - shouldContinue = shouldContinue, - ) - val responseEtag = response.header("ETag") - validateResponseEtag(responseEtag) - DesktopDetachedDownload(copied, handoffEtag ?: responseEtag) - } + return executeCancellableNextcloudAuthenticatedRequest( + client = client, + initialRequest = request, + onNetworkFailure = { failure -> onNetworkFailure(started, attempt, failure) }, + ) { response, shouldContinue -> + check(isFullDetachedFileResponse(response.code)) { failureMessage(response.code) } + val body = response.body + val contentLength = body.contentLength() + check(contentLength == -1L || contentLength <= maximumBytes) { limitMessage } + val copied = body.byteStream().copyBoundedNetworkResponseTo( + output = output, + maxBytes = maximumBytes, + onLimitExceeded = { error(limitMessage) }, + onNetworkReadFailure = { failure -> onNetworkFailure(started, attempt, failure) }, + shouldContinue = shouldContinue, + ) + val responseEtag = response.header("ETag") + validateResponseEtag(responseEtag) + DesktopDetachedDownload(copied, handoffEtag ?: responseEtag) } } diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownloadTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownloadTest.kt index 12e0cd0e0..1178e3121 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownloadTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/DesktopDetachedDownloadTest.kt @@ -12,6 +12,7 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeout import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertTrue import mockwebserver3.MockResponse @@ -20,8 +21,14 @@ import okhttp3.OkHttpClient class DesktopDetachedDownloadTest { @Test - fun `cancellation stops the desktop request and UTF-8 credentials are preserved`() = runBlocking { + fun `cancellation stops the redirected desktop request and UTF-8 credentials are preserved`() = runBlocking { MockWebServer().use { server -> + server.enqueue( + MockResponse.Builder() + .code(307) + .addHeader("Location", "/cloud/redirected.bin") + .build(), + ) server.enqueue( MockResponse.Builder() .code(200) @@ -31,14 +38,14 @@ class DesktopDetachedDownloadTest { ) server.start() val destination = Files.createTempFile("ncn-desktop-detached-cancel-", ".tmp").toFile() - val session = NextcloudSession(server.url("/").toString(), "alïce", "pässword") + val session = NextcloudSession(server.url("/cloud").toString(), "alïce", "pässword") try { val job = launch(Dispatchers.Default) { FileOutputStream(destination).use { output -> downloadDesktopDetachedFile( client = OkHttpClient(), session = session, - url = server.url("/large.bin").toString(), + url = server.url("/cloud/large.bin").toString(), output = output, maximumBytes = Long.MAX_VALUE, userAgent = "test", @@ -48,11 +55,13 @@ class DesktopDetachedDownloadTest { ) } } - val request = assertNotNull(server.takeRequest(5, TimeUnit.SECONDS)) + val initial = assertNotNull(server.takeRequest(5, TimeUnit.SECONDS)) + val redirected = assertNotNull(server.takeRequest(5, TimeUnit.SECONDS)) val expected = Base64.getEncoder().encodeToString( "alïce:pässword".toByteArray(StandardCharsets.UTF_8), ) - assertEquals("Basic $expected", request.headers["Authorization"]) + assertEquals("Basic $expected", initial.headers["Authorization"]) + assertEquals(initial.headers["Authorization"], redirected.headers["Authorization"]) withTimeout(2_000L) { job.cancelAndJoin() } @@ -62,4 +71,41 @@ class DesktopDetachedDownloadTest { } } } + + @Test + fun `desktop detached download rejects another origin before sending credentials`() = runBlocking { + MockWebServer().use { accountServer -> + MockWebServer().use { unrelatedServer -> + accountServer.start() + unrelatedServer.start() + val destination = Files.createTempFile("ncn-desktop-detached-origin-", ".tmp").toFile() + try { + assertFailsWith { + FileOutputStream(destination).use { output -> + downloadDesktopDetachedFile( + client = OkHttpClient(), + session = NextcloudSession( + accountServer.url("/cloud").toString(), + "alice", + "secret", + ), + url = unrelatedServer.url("/capture.bin").toString(), + output = output, + maximumBytes = Long.MAX_VALUE, + userAgent = "test", + failureMessage = { status -> "HTTP $status" }, + limitMessage = "Too large", + onNetworkFailure = { _, _, _ -> }, + ) + } + } + + assertEquals(0, accountServer.requestCount) + assertEquals(0, unrelatedServer.requestCount) + } finally { + destination.delete() + } + } + } + } } diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmCancellableHttp.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmCancellableHttp.kt index 7bf43cd13..35320920f 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmCancellableHttp.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmCancellableHttp.kt @@ -1,37 +1,54 @@ package dev.obiente.nextcloudnative.app -import java.nio.charset.StandardCharsets -import java.util.Base64 -import kotlinx.coroutines.Job -import kotlinx.coroutines.currentCoroutineContext +import java.util.concurrent.atomic.AtomicReference import kotlinx.coroutines.suspendCancellableCoroutine import okhttp3.Call import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response -/** Executes a blocking OkHttp call without allowing it to outlive its owning coroutine. */ -suspend fun executeCancellableJvmHttpCall( +/** Executes an account-bound request without allowing any redirected call to outlive its coroutine. */ +suspend fun executeCancellableNextcloudAuthenticatedRequest( client: OkHttpClient, - call: Call, - block: (Call, shouldContinue: () -> Boolean) -> T, + initialRequest: Request, + onNetworkFailure: (Throwable) -> Unit, + consume: (Response, shouldContinue: () -> Boolean) -> T, ): T { - val job = currentCoroutineContext()[Job] + val requestClient = client.newBuilder() + .followRedirects(false) + .followSslRedirects(false) + .build() return suspendCancellableCoroutine { continuation -> - continuation.invokeOnCancellation { call.cancel() } + val activeCall = AtomicReference(null) + continuation.invokeOnCancellation { activeCall.get()?.cancel() } val execute = Runnable { val result = runCatching { - block(call) { job?.isActive != false && !call.isCanceled() } + executeNextcloudAuthenticatedRequest( + client = requestClient, + initialRequest = initialRequest, + executeCall = { call -> + activeCall.set(call) + if (!continuation.isActive) call.cancel() + try { + call.execute() + } catch (failure: Throwable) { + onNetworkFailure(failure) + throw failure + } + }, + ) { response -> + consume(response) { + continuation.isActive && activeCall.get()?.isCanceled() == false + } + } } + activeCall.set(null) continuation.resumeWith(result) } - runCatching { client.dispatcher.executorService.execute(execute) } - .onFailure { failure -> continuation.resumeWith(Result.failure(failure)) } + runCatching { requestClient.dispatcher.executorService.execute(execute) } + .onFailure { failure -> + activeCall.set(null) + continuation.resumeWith(Result.failure(failure)) + } } } - -/** Nextcloud credentials are UTF-8 throughout the JVM transports. */ -fun nextcloudBasicAuthorization(session: NextcloudSession): String { - val encoded = Base64.getEncoder().encodeToString( - "${session.loginName}:${session.appPassword}".toByteArray(StandardCharsets.UTF_8), - ) - return "Basic $encoded" -}