From 37cc1503a199e48f87dff1d93c7dd16447ad440c Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Tue, 14 Jul 2026 23:37:01 -0400 Subject: [PATCH] fix(security): stop credentials leaking via redirects, foreign 401s, and the server-switch race Three residual credential-exposure paths that the AuthInterceptor origin-scoping didn't cover, because they happen below the application interceptor layer or in a different component. 1. TokenAuthenticator was origin-blind. The shared authenticated client is used to fetch avatars (WidgetAvatars, via WidgetEntryPoint.okHttpClient), and those URLs can point at an external host. AuthInterceptor won't proactively attach a bearer there, but a 401 from that host made the authenticator refresh the token and retry the request to the external host with the freshly minted bearer. It now returns null for any 401 whose request isn't the API origin: no refresh, no retry, no leak. 2. Cross-origin redirects kept the CF-Access headers. AuthInterceptor is an application interceptor, so it only sees the original request; on an API -> external redirect, OkHttp strips Authorization but not custom headers, so CF-Access-* followed to the external host. Added CredentialGuardInterceptor as a NETWORK interceptor, which runs on every hop (redirect follow-ups and authenticator retries included) and strips Authorization, CF-Access-*, and Cookie from any request not going to the API origin. This is also a belt for (1). 3. Changing the server saved the new base URL before clearing the old session, leaving a window where the new origin was configured but the old token was still present, so a concurrent request would be treated as trusted and carry the old bearer to the new host. Both saveBaseUrl paths (AuthViewModel, Tests: the guard strips creds on foreign/CDN hops and preserves them on the API origin; the authenticator ignores foreign-host 401s without touching the refresh token. --- .../data/api/CredentialGuardInterceptor.kt | 47 +++++++++++ .../sheaf/data/api/TokenAuthenticator.kt | 7 ++ .../systems/lupine/sheaf/di/NetworkModule.kt | 7 ++ .../lupine/sheaf/ui/auth/AuthViewModel.kt | 10 +-- .../sheaf/ui/settings/SettingsViewModel.kt | 7 +- .../api/CredentialGuardInterceptorTest.kt | 83 +++++++++++++++++++ .../sheaf/data/api/TokenAuthenticatorTest.kt | 55 ++++++++++++ 7 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 sheaf/app/src/main/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptor.kt create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/api/CredentialGuardInterceptorTest.kt create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt 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 } + } +}