diff --git a/.github/workflows/dev-release.yml b/.github/workflows/dev-release.yml index 3c16f13..0f49213 100644 --- a/.github/workflows/dev-release.yml +++ b/.github/workflows/dev-release.yml @@ -60,9 +60,10 @@ jobs: - name: Make gradlew executable run: chmod +x sheaf/gradlew - - name: Run unit tests + - name: Run unit tests (app + wear) working-directory: sheaf - run: ./gradlew :app:test + # The wear module ships in this workflow too, so run its tests. + run: ./gradlew :app:test :wear:test - name: Upload test reports if: always() @@ -72,6 +73,8 @@ jobs: path: | sheaf/app/build/reports/tests/ sheaf/app/build/test-results/ + sheaf/wear/build/reports/tests/ + sheaf/wear/build/test-results/ if-no-files-found: ignore - name: Build release APK diff --git a/.github/workflows/tagged-release.yml b/.github/workflows/tagged-release.yml index 9908fd6..1710be9 100644 --- a/.github/workflows/tagged-release.yml +++ b/.github/workflows/tagged-release.yml @@ -93,9 +93,12 @@ jobs: - name: Make gradlew executable run: chmod +x sheaf/gradlew - - name: Run unit tests + - name: Run unit tests (app + wear) working-directory: sheaf - run: ./gradlew :app:test + # This workflow builds and ships the wear module, so its tests belong + # here too. It used to run :app:test alone, so every tagged wear release + # went out without its tests ever having run. + run: ./gradlew :app:test :wear:test - name: Upload test reports if: always() @@ -105,6 +108,8 @@ jobs: path: | sheaf/app/build/reports/tests/ sheaf/app/build/test-results/ + sheaf/wear/build/reports/tests/ + sheaf/wear/build/test-results/ if-no-files-found: ignore - name: Install cosign diff --git a/CHANGELOG.md b/CHANGELOG.md index cc67475..e6040e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,14 @@ uses semantic versioning (`MAJOR.MINOR.PATCH`). switch. A failed group-membership load no longer lets Save wipe the group's members. +- **Self-hosted instances behind a path now work.** A server URL like + `example.org/sheaf` had the path silently dropped, so every request went to + the domain root instead. Only instances at the root of a domain worked. + +- **A server URL that cannot work is rejected up front.** The app used to invite + you to type `http://` for a plaintext server, accept it, and then fail every + request with an unhelpful network error, because Android blocks unencrypted + connections. It now says so when you enter the address. - **"Resend Email" works after registering.** The resend request went out without your credentials and failed, and the failure was hidden, so it looked like the email had been sent again. It now sends, confirms when it does, and diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/BaseUrlInterceptor.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/BaseUrlInterceptor.kt index f83bbb1..3c3a8fc 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/BaseUrlInterceptor.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/BaseUrlInterceptor.kt @@ -3,6 +3,7 @@ package systems.lupine.sheaf.data.api import systems.lupine.sheaf.data.repository.PreferencesRepository import kotlinx.coroutines.flow.firstOrNull import kotlinx.coroutines.runBlocking +import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.Interceptor import okhttp3.Response @@ -25,13 +26,28 @@ class BaseUrlInterceptor @Inject constructor( ?.toHttpUrlOrNull() ?: return chain.proceed(original) - val newUrl = original.url.newBuilder() - .scheme(baseUrl.scheme) - .host(baseUrl.host) - .port(baseUrl.port) - .build() - - val newRequest = original.newBuilder().url(newUrl).build() + val newRequest = original.newBuilder().url(applyBaseUrl(original.url, baseUrl)).build() return chain.proceed(newRequest) } } + +/** + * Rewrite Retrofit's placeholder URL onto the configured instance. + * + * Endpoint paths are absolute ("/v1/members"), so the original rewrite only + * swapped scheme/host/port. That silently dropped any path prefix on the base + * URL: an instance hosted at https://example.org/sheaf/ had every request sent + * to https://example.org/v1/... instead, i.e. at whatever lives on the domain + * root. Carry the base URL's path prefix through. The query string rides along + * on the original builder untouched. + */ +internal fun applyBaseUrl(original: HttpUrl, base: HttpUrl): HttpUrl { + // HttpUrl always reports at least "/", so an origin-root base contributes "". + val prefix = base.encodedPath.trimEnd('/') + return original.newBuilder() + .scheme(base.scheme) + .host(base.host) + .port(base.port) + .encodedPath(prefix + original.encodedPath) + .build() +} diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/repository/PreferencesRepository.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/repository/PreferencesRepository.kt index fe57d72..2a63cfb 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/repository/PreferencesRepository.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/repository/PreferencesRepository.kt @@ -10,6 +10,7 @@ import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import androidx.datastore.preferences.preferencesDataStore import dagger.hilt.android.qualifiers.ApplicationContext +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map @@ -270,3 +271,37 @@ internal fun normalizeBaseUrl(input: String): String { else -> "https://$trimmed" } } + +// Hosts the debug network-security-config permits cleartext to. Everything else +// is TLS-only, on every build type, because targetSdk >= 28 means the platform +// default is cleartextTrafficPermitted="false". +private val CLEARTEXT_HOSTS = setOf("localhost", "127.0.0.1", "::1", "10.0.2.2") + +/** + * Why a user-typed server URL cannot work, or null if it can. + * + * The app used to accept any http:// URL and tell the user to type the prefix + * for "a plaintext server". Release builds then blocked the request at the + * platform level and surfaced it as a generic network error, so the affordance + * we advertised only ever worked in debug, against loopback. Say no up front + * instead of saving an address that is guaranteed to fail. + * + * @param cleartextPermitted true on builds that ship a network-security-config + * allowing cleartext (debug). Pass BuildConfig.DEBUG. + */ +internal fun baseUrlError(input: String, cleartextPermitted: Boolean): String? { + val trimmed = input.trim().trimEnd('/') + if (trimmed.isEmpty()) return null + // A bare scheme survives normalizeBaseUrl as nonsense ("https://" -> the + // scheme check fails on the trailing-slash-stripped "https:", so it gets a + // second https:// glued on and then parses). Reject it here. + if (trimmed.equals("http:", ignoreCase = true) || trimmed.equals("https:", ignoreCase = true)) { + return "That doesn't look like a server address." + } + val url = normalizeBaseUrl(trimmed).toHttpUrlOrNull() + ?: return "That doesn't look like a server address." + if (url.scheme != "http") return null + if (cleartextPermitted && url.host in CLEARTEXT_HOSTS) return null + return "This server must use https://. Android blocks unencrypted connections, " + + "so an http:// address would fail every request." +} diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/LoginScreen.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/LoginScreen.kt index cfffc67..d8c490c 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/LoginScreen.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/LoginScreen.kt @@ -28,8 +28,10 @@ import androidx.compose.foundation.Image import androidx.compose.foundation.shape.CircleShape import androidx.compose.ui.res.painterResource import androidx.compose.ui.unit.dp +import systems.lupine.sheaf.BuildConfig import systems.lupine.sheaf.R import androidx.hilt.navigation.compose.hiltViewModel +import systems.lupine.sheaf.data.repository.baseUrlError import systems.lupine.sheaf.ui.components.ErrorBanner @Composable @@ -49,6 +51,7 @@ fun LoginScreen( var step by remember { mutableStateOf(if (savedBaseUrl.isBlank()) "url" else "auth") } var urlDraft by remember(savedBaseUrl) { mutableStateOf(savedBaseUrl) } + var urlError by remember { mutableStateOf(null) } var email by remember { mutableStateOf("") } var password by remember { mutableStateOf("") } var inviteCode by remember { mutableStateOf("") } @@ -128,16 +131,21 @@ fun LoginScreen( when (currentStep) { "url" -> ServerUrlStep( urlDraft = urlDraft, - onUrlChange = { urlDraft = it }, + onUrlChange = { urlDraft = it; urlError = null }, + error = urlError, onContinue = { // Default to the hosted instance when the user - // leaves the field blank — most users land there + // leaves the field blank - most users land there // anyway, and the placeholder alone reads as an // example rather than a "press Continue to use // this" hint. val resolved = urlDraft.trim().ifBlank { DEFAULT_HOSTED_INSTANCE } - viewModel.saveBaseUrl(resolved) - step = "auth" + val problem = baseUrlError(resolved, BuildConfig.DEBUG) + urlError = problem + if (problem == null) { + viewModel.saveBaseUrl(resolved) + step = "auth" + } }, ) "auth" -> AuthStep( @@ -255,6 +263,7 @@ private fun CfAccessDialog( private fun ServerUrlStep( urlDraft: String, onUrlChange: (String) -> Unit, + error: String?, onContinue: () -> Unit, ) { Column(verticalArrangement = Arrangement.spacedBy(16.dp)) { @@ -281,9 +290,11 @@ private fun ServerUrlStep( keyboardActions = KeyboardActions(onDone = { onContinue() }), modifier = Modifier.fillMaxWidth(), ) + if (error != null) ErrorBanner(error) Text( "Leave blank to use $DEFAULT_HOSTED_INSTANCE. https:// is added " + - "automatically; use http:// explicitly only for plaintext servers.", + "automatically. Servers must use https; a path is fine " + + "(e.g. example.org/sheaf).", style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, ) diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt index 51a7dcd..c2f688f 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt @@ -75,6 +75,8 @@ import systems.lupine.sheaf.ui.components.COMMON_ZONES import systems.lupine.sheaf.ui.components.TZ_AUTO import systems.lupine.sheaf.ui.components.allTimeZoneIds import systems.lupine.sheaf.ui.components.friendlyZoneLabel +import systems.lupine.sheaf.BuildConfig +import systems.lupine.sheaf.data.repository.baseUrlError import systems.lupine.sheaf.ui.auth.AuthViewModel import systems.lupine.sheaf.ui.components.ErrorBanner import systems.lupine.sheaf.ui.components.SheafTopAppBar @@ -678,13 +680,14 @@ fun ServerSettingsScreen( ) { val savedBaseUrl by viewModel.baseUrl.collectAsState() var urlDraft by remember(savedBaseUrl) { mutableStateOf(savedBaseUrl) } + var urlError by remember { mutableStateOf(null) } var showUrlDialog by remember { mutableStateOf(false) } CategoryScaffold(title = "Server", onNavigateUp = onNavigateUp) { SettingItem( icon = Icons.Outlined.Storage, title = "API Server", subtitle = savedBaseUrl.ifBlank { "Not configured" }, - onClick = { urlDraft = savedBaseUrl; showUrlDialog = true }, + onClick = { urlDraft = savedBaseUrl; urlError = null; showUrlDialog = true }, ) } if (showUrlDialog) { @@ -695,22 +698,32 @@ fun ServerSettingsScreen( Column(verticalArrangement = Arrangement.spacedBy(8.dp)) { OutlinedTextField( value = urlDraft, - onValueChange = { urlDraft = it }, + onValueChange = { urlDraft = it; urlError = null }, label = { Text("Base URL or domain") }, placeholder = { Text("app.sheaf.sh") }, singleLine = true, + isError = urlError != null, + supportingText = urlError?.let { { Text(it) } }, modifier = Modifier.fillMaxWidth(), ) Text( - "https:// is added automatically. To use plaintext (e.g. a local dev server), " + - "type the http:// prefix explicitly.", + "https:// is added automatically. Servers must use https; a path is " + + "fine (e.g. example.org/sheaf).", style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, ) } }, confirmButton = { - TextButton(onClick = { viewModel.saveBaseUrl(urlDraft.trim()); showUrlDialog = false }) { + TextButton(onClick = { + val candidate = urlDraft.trim() + val problem = baseUrlError(candidate, BuildConfig.DEBUG) + urlError = problem + if (problem == null) { + viewModel.saveBaseUrl(candidate) + showUrlDialog = false + } + }) { Text("Save") } }, diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/ApplyBaseUrlTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/ApplyBaseUrlTest.kt new file mode 100644 index 0000000..d781417 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/ApplyBaseUrlTest.kt @@ -0,0 +1,71 @@ +package systems.lupine.sheaf.data.api + +import okhttp3.HttpUrl.Companion.toHttpUrl +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * Retrofit is configured with the placeholder base http://localhost/ and every + * endpoint path is absolute, so these mirror what the interceptor actually sees. + */ +class ApplyBaseUrlTest { + + private fun rewrite(request: String, base: String): String = + applyBaseUrl(request.toHttpUrl(), base.toHttpUrl()).toString() + + @Test fun `origin-root base swaps scheme host and port`() { + assertEquals( + "https://app.sheaf.sh/v1/members", + rewrite("http://localhost/v1/members", "https://app.sheaf.sh"), + ) + } + + @Test fun `base with a path prefix keeps the prefix`() { + assertEquals( + "https://example.org/sheaf/v1/members", + rewrite("http://localhost/v1/members", "https://example.org/sheaf"), + ) + } + + @Test fun `trailing slash on the prefix is not doubled`() { + assertEquals( + "https://example.org/sheaf/v1/members", + rewrite("http://localhost/v1/members", "https://example.org/sheaf/"), + ) + } + + @Test fun `nested path prefix is preserved in order`() { + assertEquals( + "https://example.org/apps/sheaf/v1/fronts/current", + rewrite("http://localhost/v1/fronts/current", "https://example.org/apps/sheaf"), + ) + } + + @Test fun `non-default port is carried over`() { + assertEquals( + "https://example.org:8443/sheaf/v1/members", + rewrite("http://localhost/v1/members", "https://example.org:8443/sheaf"), + ) + } + + @Test fun `query params survive the rewrite`() { + assertEquals( + "https://example.org/sheaf/v1/export?format=openplural", + rewrite("http://localhost/v1/export?format=openplural", "https://example.org/sheaf"), + ) + } + + @Test fun `encoded path segments are not double-encoded`() { + assertEquals( + "https://example.org/sheaf/v1/members/a%20b", + rewrite("http://localhost/v1/members/a%20b", "https://example.org/sheaf"), + ) + } + + @Test fun `cleartext loopback base is left as http`() { + assertEquals( + "http://10.0.2.2:8000/v1/members", + rewrite("http://localhost/v1/members", "http://10.0.2.2:8000"), + ) + } +} diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/repository/BaseUrlErrorTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/repository/BaseUrlErrorTest.kt new file mode 100644 index 0000000..2200fdd --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/repository/BaseUrlErrorTest.kt @@ -0,0 +1,53 @@ +package systems.lupine.sheaf.data.repository + +import kotlin.test.Test +import kotlin.test.assertNotNull +import kotlin.test.assertNull + +class BaseUrlErrorTest { + + // Release builds: no network-security-config, so the platform default + // (cleartext blocked) applies to every host. + @Test fun `release rejects cleartext to a real host`() { + assertNotNull(baseUrlError("http://example.org", cleartextPermitted = false)) + } + + @Test fun `release rejects cleartext to loopback too`() { + // Nothing in a release build can reach it, so accepting it would just + // save an address that fails every request. + assertNotNull(baseUrlError("http://localhost:8000", cleartextPermitted = false)) + } + + @Test fun `release accepts https`() { + assertNull(baseUrlError("https://example.org", cleartextPermitted = false)) + } + + @Test fun `bare host is fine - it normalises to https`() { + assertNull(baseUrlError("example.org", cleartextPermitted = false)) + } + + @Test fun `https with a path prefix is fine`() { + assertNull(baseUrlError("https://example.org/sheaf", cleartextPermitted = false)) + } + + @Test fun `debug allows cleartext to loopback hosts`() { + assertNull(baseUrlError("http://localhost:8000", cleartextPermitted = true)) + assertNull(baseUrlError("http://127.0.0.1:8000", cleartextPermitted = true)) + assertNull(baseUrlError("http://10.0.2.2:8000", cleartextPermitted = true)) + } + + @Test fun `debug still rejects cleartext to a non-loopback host`() { + // The debug network-security-config only whitelists the loopback set, + // so an http LAN address fails there as well. + assertNotNull(baseUrlError("http://192.168.1.10:8000", cleartextPermitted = true)) + } + + @Test fun `empty input is not an error - the caller substitutes a default`() { + assertNull(baseUrlError("", cleartextPermitted = false)) + assertNull(baseUrlError(" ", cleartextPermitted = false)) + } + + @Test fun `unparseable input is rejected`() { + assertNotNull(baseUrlError("https://", cleartextPermitted = false)) + } +}