From a98a0cc395691b25fe0b1175b415cecd70fa4f63 Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Tue, 14 Jul 2026 10:26:12 -0400 Subject: [PATCH] fix: honour a base URL path prefix, reject unusable server URLs, test wear in CI BaseUrlInterceptor copied scheme/host/port from the configured base URL and dropped everything else. Endpoint paths are absolute ("/v1/members"), so an instance hosted at https://example.org/sheaf/ had every request sent to https://example.org/v1/... instead: whatever happens to live at that domain's root. Only origin-root deployments ever worked. The rewrite now carries the base URL's path prefix through, extracted as a pure applyBaseUrl() with tests (query params were already fine). Cleartext: the app told users to "type the http:// prefix explicitly" for a plaintext server, normalizeBaseUrl accepted it, and then targetSdk 35's default (cleartextTrafficPermitted=false) blocked every request and surfaced it as a generic network failure. The affordance we advertised only ever worked in debug, against loopback, which is the only thing the debug network-security-config whitelists. Validate the address when it is entered instead: non-loopback http is refused with an explanation, debug builds still allow the loopback set, and the misleading copy in both the login flow and Settings > Server is gone. Also rejects a bare "https://", which normalizeBaseUrl otherwise mangled into something that parses. CI: the release workflows ran :app:test only, so every wear release (which they build and ship) went out without its tests ever running. Both now run :app:test :wear:test and collect the wear reports. --- .github/workflows/dev-release.yml | 7 +- .github/workflows/tagged-release.yml | 9 ++- CHANGELOG.md | 9 +++ .../sheaf/data/api/BaseUrlInterceptor.kt | 30 ++++++-- .../data/repository/PreferencesRepository.kt | 35 +++++++++ .../lupine/sheaf/ui/auth/LoginScreen.kt | 21 ++++-- .../ui/settings/SettingsCategoryScreens.kt | 23 ++++-- .../lupine/sheaf/data/api/ApplyBaseUrlTest.kt | 71 +++++++++++++++++++ .../sheaf/data/repository/BaseUrlErrorTest.kt | 53 ++++++++++++++ 9 files changed, 237 insertions(+), 21 deletions(-) create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/api/ApplyBaseUrlTest.kt create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/repository/BaseUrlErrorTest.kt 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 9963db6..e5f88e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,15 @@ 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. + ## [1.2.2] - 2026-07-11 ### Added 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 245c7d3..42e5184 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( @@ -253,6 +261,7 @@ private fun CfAccessDialog( private fun ServerUrlStep( urlDraft: String, onUrlChange: (String) -> Unit, + error: String?, onContinue: () -> Unit, ) { Column(verticalArrangement = Arrangement.spacedBy(16.dp)) { @@ -279,9 +288,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)) + } +}