From 4125f74f5f431a43b6d755f5667b7f75070cd9ef Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 7 Jul 2026 08:42:20 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20CodeRabbit=20round=202=20=E2=80=94=20sco?= =?UTF-8?q?pe=20the=20insecure-HTTP=20probe=20flag=20+=20accessible=20togg?= =?UTF-8?q?le=20row?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AuthRepository.discover(): pendingAllowInsecureHttp was set but never reset. Since the NetworkModule OkHttp client is a shared singleton, a toggle-on discovery that failed or was abandoned left cleartext allowed for EVERY later request for the rest of the process (and concurrent discover() calls could race on the shared flag). Wrap the probe in try/finally and always reset the flag; commitInstance() still persists the real per-instance flag on a successful connect. - OnboardingScreen: make the whole "Allow insecure HTTP" row toggleable (Modifier.toggleable with Role.Switch, Switch onCheckedChange = null) so a screen reader announces the label with the state and the touch target is the full row. Verified: compileDebugKotlin + testDebugUnitTest pass. --- .../app/data/repository/AuthRepository.kt | 46 +++++++++++-------- .../ui/screens/onboarding/OnboardingScreen.kt | 14 +++++- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt b/app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt index c2f0aaa..eb0cbae 100644 --- a/app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt +++ b/app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt @@ -30,26 +30,34 @@ class AuthRepository( * to continue based on [HealthPayload.appAccessEnabled] and the transport warning. */ suspend fun discover(rawInstanceUrl: String, allowInsecure: Boolean = false): ApiResult { - // Carry the onboarding toggle into the cleartext gate: the instance isn't committed - // yet, so the persisted flag is still false and would block a plain-HTTP probe. + // Carry the onboarding toggle into the cleartext gate ONLY for the duration of this + // probe: the instance isn't committed yet, so the persisted flag is still false and + // would block a plain-HTTP health check. The NetworkModule OkHttp client is a shared + // singleton, so the flag is reset in finally() — otherwise a toggle-on discovery that + // fails or is abandoned would leave cleartext allowed for every later request. On a + // successful connect, commitInstance() persists the real per-instance flag. session.pendingAllowInsecureHttp = allowInsecure - val apiBaseUrl = NetworkModule.deriveApiBaseUrl(rawInstanceUrl, allowInsecure) - val origin = NetworkModule.deriveOrigin(rawInstanceUrl, allowInsecure) - val api = network.api(apiBaseUrl) - return when (val res = apiCall { api.health() }) { - is ApiResult.Success -> ApiResult.Success( - HealthDiscovery( - health = res.data, - origin = origin, - apiBaseUrl = apiBaseUrl, - insecureTransport = res.meta?.warning == "insecure_transport" || - res.meta?.https == false, - transportAllowed = NetworkModule.isTransportAllowed(apiBaseUrl, allowInsecure), - allowInsecure = allowInsecure, - ), - res.meta, - ) - is ApiResult.Failure -> res + try { + val apiBaseUrl = NetworkModule.deriveApiBaseUrl(rawInstanceUrl, allowInsecure) + val origin = NetworkModule.deriveOrigin(rawInstanceUrl, allowInsecure) + val api = network.api(apiBaseUrl) + return when (val res = apiCall { api.health() }) { + is ApiResult.Success -> ApiResult.Success( + HealthDiscovery( + health = res.data, + origin = origin, + apiBaseUrl = apiBaseUrl, + insecureTransport = res.meta?.warning == "insecure_transport" || + res.meta?.https == false, + transportAllowed = NetworkModule.isTransportAllowed(apiBaseUrl, allowInsecure), + allowInsecure = allowInsecure, + ), + res.meta, + ) + is ApiResult.Failure -> res + } + } finally { + session.pendingAllowInsecureHttp = false } } diff --git a/app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt b/app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt index 0318107..5b9e3b0 100644 --- a/app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt +++ b/app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt @@ -2,6 +2,7 @@ package com.pinakes.app.ui.screens.onboarding import androidx.compose.foundation.Image import androidx.compose.foundation.isSystemInDarkTheme +import androidx.compose.foundation.selection.toggleable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row @@ -32,6 +33,7 @@ import androidx.compose.ui.draw.clip import androidx.compose.ui.layout.ContentScale import androidx.compose.ui.res.painterResource import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.Role import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp import com.pinakes.app.R @@ -107,7 +109,15 @@ fun OnboardingScreen(onContinue: () -> Unit) { if (state.discovery == null) { Row( - modifier = form, + // Toggle from the whole row (not just the switch): a screen reader + // announces the label together with the on/off state, and the larger + // touch target is easier to hit. onCheckedChange = null makes the Switch + // a passive indicator of the row's toggle state. + modifier = form.toggleable( + value = state.allowInsecureHttp, + role = Role.Switch, + onValueChange = vm::onAllowInsecureChange, + ), verticalAlignment = Alignment.CenterVertically, ) { Column(Modifier.weight(1f)) { @@ -125,7 +135,7 @@ fun OnboardingScreen(onContinue: () -> Unit) { Spacer(Modifier.width(Spacing.md)) Switch( checked = state.allowInsecureHttp, - onCheckedChange = vm::onAllowInsecureChange, + onCheckedChange = null, ) }