fix(security): plug credential leaks via redirects, foreign 401s, and the server-switch race

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)
+    }
+}

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") ||

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/<version>" on every API call instead of
             // OkHttp's default "okhttp/<lib version>". The server records

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)
         }
     }
 

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)
         }
     }
 

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<PreferencesRepository>().apply {
+        every { baseUrl } returns flowOf("https://app.sheaf.sh")
+    }
+    private val guard = CredentialGuardInterceptor(prefs)
+
+    private fun proceedWith(request: Request): Request {
+        val forwarded = slot<Request>()
+        val chain = mockk<Interceptor.Chain>()
+        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"))
+    }
+}

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<PreferencesRepository>(relaxed = true).apply {
+        every { baseUrl } returns flowOf("https://app.sheaf.sh")
+    }
+    private val authenticator = TokenAuthenticator(
+        prefs = prefs,
+        moshi = Moshi.Builder().build(),
+        lazyClient = Lazy { mockk<OkHttpClient>() },
+        accountDataWiper = Lazy { mockk<AccountDataWiper>() },
+    )
+
+    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 }
+    }
+}