diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptor.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptor.kt new file mode 100644 index 0000000..bb46c70 --- /dev/null +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptor.kt @@ -0,0 +1,47 @@ +package systems.lupine.sheaf.data.api + +import systems.lupine.sheaf.data.repository.PreferencesRepository +import kotlinx.coroutines.flow.firstOrNull +import kotlinx.coroutines.runBlocking +import okhttp3.Interceptor +import okhttp3.Response +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Network-layer backstop that strips credentials from any hop not going to the + * configured API origin. + * + * [AuthInterceptor] is an application interceptor, so it only sees the original + * request and adds credentials once, scoped to the API origin. But two things + * happen below it, per network hop, that it can't police: + * + * - A cross-origin redirect. OkHttp drops `Authorization` when the host + * changes, but not custom headers, so `CF-Access-*` would follow an + * API -> external redirect to the external host. + * - A [TokenAuthenticator] retry. On a 401 from a foreign host (e.g. a widget + * fetching an avatar via the shared client) it would re-attach a freshly + * minted bearer directly on the retried request. + * + * As a NETWORK interceptor this runs on every hop, including redirect follow-ups + * and authenticator retries, so it is the last line that guarantees the session + * never leaves for a host that isn't ours. + */ +@Singleton +class CredentialGuardInterceptor @Inject constructor( + private val prefs: PreferencesRepository, +) : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val request = chain.request() + val isApiOrigin = runBlocking { originMatches(request.url, prefs.baseUrl.firstOrNull()) } + if (isApiOrigin) return chain.proceed(request) + + val stripped = request.newBuilder() + .removeHeader("Authorization") + .removeHeader("CF-Access-Client-Id") + .removeHeader("CF-Access-Client-Secret") + .removeHeader("Cookie") + .build() + return chain.proceed(stripped) + } +} diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt index 0a0b2a5..a64daf3 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt @@ -38,6 +38,13 @@ class TokenAuthenticator @Inject constructor( // when the server can't bind the request to a session via the auth // path it expects (separate from our Bearer flow) — refreshing won't // help and was previously spinning to MAX_FOLLOW_UPS. + // Never react to a 401 from a host that isn't our API. The shared client + // is used to fetch avatars (e.g. from widgets), which can point at an + // external host; refreshing and retrying there would both waste a token + // rotation and hand the freshly minted bearer to that host. + val baseForGuard = runBlocking { prefs.baseUrl.firstOrNull() } + if (!originMatches(response.request.url, baseForGuard)) return null + val path = response.request.url.encodedPath if (path.endsWith("/auth/refresh") || path.endsWith("/auth/delete-account") || diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt index 4fa8361..97fafc3 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/di/NetworkModule.kt @@ -4,6 +4,7 @@ import android.content.Context import systems.lupine.sheaf.BuildConfig import systems.lupine.sheaf.data.api.AuthInterceptor import systems.lupine.sheaf.data.api.BaseUrlInterceptor +import systems.lupine.sheaf.data.api.CredentialGuardInterceptor import systems.lupine.sheaf.data.api.FrontUpdateJsonAdapter import systems.lupine.sheaf.data.model.FrontUpdate import systems.lupine.sheaf.data.api.SheafApiService @@ -53,11 +54,17 @@ object NetworkModule { tokenAuthenticator: TokenAuthenticator, cookieJar: TrustedDeviceCookieJar, userAgentInterceptor: UserAgentInterceptor, + credentialGuard: CredentialGuardInterceptor, ): OkHttpClient { val builder = OkHttpClient.Builder() .cookieJar(cookieJar) .addInterceptor(baseUrlInterceptor) .addInterceptor(authInterceptor) + // Network (not application) interceptor: runs on every hop, so it + // strips credentials from redirect follow-ups and authenticator + // retries that leave the API origin, which the application-level + // AuthInterceptor above can't see. + .addNetworkInterceptor(credentialGuard) .authenticator(tokenAuthenticator) // Send "Sheaf Android/" on every API call instead of // OkHttp's default "okhttp/". The server records diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/AuthViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/AuthViewModel.kt index ac0cae7..4ca6f29 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/AuthViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/AuthViewModel.kt @@ -133,16 +133,16 @@ class AuthViewModel @Inject constructor( fun saveBaseUrl(url: String) { viewModelScope.launch { val previous = prefs.baseUrl.firstOrNull() - prefs.saveBaseUrl(url) - // Switching to a different instance must drop any session bound to - // the old one: otherwise the previous instance's tokens, cache and - // offline queue survive against the new host (and its bearer would - // ride to the new server). + // Clear the old session BEFORE storing the new origin. Saving first + // opens a window where the base URL is the new host but the old token + // is still present, so a concurrent request would be treated as + // trusted and carry the old bearer to the new host. if (!sameConfiguredOrigin(previous, url)) { authInterceptor.pendingToken = null prefs.clearTokens() accountDataWiper.wipe() } + prefs.saveBaseUrl(url) } } diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt index fe5207f..89bad08 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt @@ -168,15 +168,16 @@ class SettingsViewModel @Inject constructor( fun saveBaseUrl(url: String) { viewModelScope.launch { val previous = prefs.baseUrl.firstOrNull() - prefs.saveBaseUrl(url) // Changing the server here means switching instances while signed in. - // Drop the old session so its tokens, cache and offline queue can't - // carry over to (or leak toward) the new host. + // Drop the old session BEFORE storing the new origin: saving first + // leaves a window where the new host is configured but the old token + // is still present, letting a concurrent request carry it across. if (!systems.lupine.sheaf.data.api.sameConfiguredOrigin(previous, url)) { authInterceptor.pendingToken = null prefs.clearTokens() accountDataWiper.wipe() } + prefs.saveBaseUrl(url) } } diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptorTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptorTest.kt new file mode 100644 index 0000000..14965ae --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptorTest.kt @@ -0,0 +1,83 @@ +package systems.lupine.sheaf.data.api + +import io.mockk.every +import io.mockk.mockk +import io.mockk.slot +import kotlinx.coroutines.flow.flowOf +import okhttp3.Interceptor +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.Response +import systems.lupine.sheaf.data.repository.PreferencesRepository +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * The per-hop backstop. Whatever a redirect follow-up or an authenticator retry + * carried onto a request, credentials must not leave for a host that isn't the + * configured API origin. + */ +class CredentialGuardInterceptorTest { + + private val prefs = mockk().apply { + every { baseUrl } returns flowOf("https://app.sheaf.sh") + } + private val guard = CredentialGuardInterceptor(prefs) + + private fun proceedWith(request: Request): Request { + val forwarded = slot() + val chain = mockk() + every { chain.request() } returns request + every { chain.proceed(capture(forwarded)) } answers { + Response.Builder() + .request(forwarded.captured) + .protocol(Protocol.HTTP_1_1) + .code(200).message("OK") + .build() + } + guard.intercept(chain) + return forwarded.captured + } + + private fun credentialed(url: String) = Request.Builder() + .url(url) + .header("Authorization", "Bearer tok") + .header("CF-Access-Client-Id", "cf-id") + .header("CF-Access-Client-Secret", "cf-secret") + .header("Cookie", "sheaf_trusted_device=tdc") + .build() + + @Test fun `an API-origin hop keeps its credentials`() { + val sent = proceedWith(credentialed("https://app.sheaf.sh/v1/members")) + assertEquals("Bearer tok", sent.header("Authorization")) + assertEquals("cf-id", sent.header("CF-Access-Client-Id")) + assertEquals("sheaf_trusted_device=tdc", sent.header("Cookie")) + } + + @Test fun `a foreign-host hop is stripped of every credential`() { + val sent = proceedWith(credentialed("https://images.example.com/a.png")) + assertNull(sent.header("Authorization")) + assertNull(sent.header("CF-Access-Client-Id")) + assertNull(sent.header("CF-Access-Client-Secret")) + assertNull(sent.header("Cookie")) + } + + @Test fun `the CDN host is foreign to the API origin and is stripped`() { + // A redirect API -> CDN, or a widget avatar fetch, must not carry creds. + val sent = proceedWith(credentialed("https://cdn.sheaf.sh/avatars/x.png")) + assertNull(sent.header("Authorization")) + assertNull(sent.header("CF-Access-Client-Id")) + } + + @Test fun `non-credential headers are left alone on a foreign host`() { + val request = Request.Builder() + .url("https://images.example.com/a.png") + .header("Accept", "image/*") + .header("Authorization", "Bearer tok") + .build() + val sent = proceedWith(request) + assertEquals("image/*", sent.header("Accept")) + assertNull(sent.header("Authorization")) + } +} diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt new file mode 100644 index 0000000..d023cc2 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt @@ -0,0 +1,55 @@ +package systems.lupine.sheaf.data.api + +import com.squareup.moshi.Moshi +import dagger.Lazy +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.flow.flowOf +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.Response +import systems.lupine.sheaf.data.repository.AccountDataWiper +import systems.lupine.sheaf.data.repository.PreferencesRepository +import kotlin.test.Test +import kotlin.test.assertNull + +/** + * The authenticator must not react to a 401 from a foreign host. The shared + * client fetches avatars (e.g. for widgets) from hosts that aren't ours, and a + * refresh-and-retry there would both burn a refresh-token rotation and hand the + * new bearer to that host. + */ +class TokenAuthenticatorTest { + + private val prefs = mockk(relaxed = true).apply { + every { baseUrl } returns flowOf("https://app.sheaf.sh") + } + private val authenticator = TokenAuthenticator( + prefs = prefs, + moshi = Moshi.Builder().build(), + lazyClient = Lazy { mockk() }, + accountDataWiper = Lazy { mockk() }, + ) + + private fun response401(url: String) = Response.Builder() + .request(Request.Builder().url(url).header("Authorization", "Bearer old").build()) + .protocol(Protocol.HTTP_1_1) + .code(401).message("Unauthorized") + .build() + + @Test fun `a 401 from a foreign host does not trigger a refresh`() { + val result = authenticator.authenticate(null, response401("https://images.example.com/a.png")) + + assertNull(result) + // It short-circuits before ever touching the refresh token. + verify(exactly = 0) { prefs.refreshToken } + } + + @Test fun `a 401 from the image CDN is also ignored`() { + val result = authenticator.authenticate(null, response401("https://cdn.sheaf.sh/avatars/x.png")) + assertNull(result) + verify(exactly = 0) { prefs.refreshToken } + } +}