fix(onboarding): honor the insecure-HTTP opt-in during discovery (regression from #20)#21
Conversation
… just after commit Emulator smoke test caught a real regression from the cleartext-gate hardening: the CleartextGuardInterceptor reads SessionStore.allowInsecureHttp, which is only PERSISTED when an instance is committed. During onboarding discovery the instance isn't committed yet, so the flag was still false and the guard blocked the /health probe to a plain-HTTP host even with the "Allow insecure HTTP" switch ON — the toggle affected the derived URL and isTransportAllowed() but not the actual request gate, so discovery could never succeed. Add a transient, in-memory SessionStore.pendingAllowInsecureHttp set by AuthRepository.discover() from the onboarding toggle; the NetworkModule guard now allows cleartext when EITHER the persisted flag or the pending flag is set. Reset on clearAll() (forget instance). Verified on the emulator: with the toggle OFF a plain-HTTP host to a non-loopback address issues no request (guard blocks → "Couldn't reach"); with it ON the GET is actually attempted. Loopback HTTP (10.0.2.2) discovery returns 200 as before; HTTPS is unaffected.
WalkthroughIntrodotto un flag transitorio in memoria ChangesFlag pendingAllowInsecureHttp per onboarding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt (1)
32-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
pendingAllowInsecureHttpnon viene mai resettato dopo la richiesta di discovery.
session.pendingAllowInsecureHttpviene impostato a riga 35 ma non c'è nessunresetafalsedopo la chiamataapi.health()(né in caso di successo né di fallimento/eccezione). PoichéokHttpClientinNetworkModuleè un singleton condiviso da tutte le richieste dell'app (login, catalog, bookclub, ecc.), non solo dalla discovery, un flag rimasto bloccato atrue(es. utente attiva il toggle, la discovery fallisce o viene abbandonata senza un successivodiscover()conallowInsecure=false) indebolisce il cleartext-guard per qualsiasi host HTTP per l'intera durata del processo, ben oltre l'ambito della singola richiesta di onboarding per cui è stato pensato.In aggiunta, essendo uno stato mutabile condiviso a livello di sessione (non scoped per singola richiesta), chiamate concorrenti a
discover()possono generare una race: un'invocazione può sovrascrivere il flag prima che la richiesta HTTP dell'altra invocazione venga effettivamente eseguita.Consiglio di limitare l'ambito del flag alla durata della singola chiamata con un
try/finally.🔒 Fix proposto: limitare l'ambito del flag transitorio alla singola richiesta
suspend fun discover(rawInstanceUrl: String, allowInsecure: Boolean = false): ApiResult<HealthDiscovery> { // 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. 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 + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt` around lines 32 - 54, The temporary `session.pendingAllowInsecureHttp` flag in `AuthRepository.discover` is left set after the discovery request, so it should be scoped to only that call. Wrap the `api.health()` invocation in `discover()` with a `try/finally` and always reset `pendingAllowInsecureHttp` back to false in the `finally` block so it is cleared on both success and failure. Keep the change localized to `discover` and use the existing `session.pendingAllowInsecureHttp` field and `NetworkModule`/`apiCall` flow as the reference points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/java/com/pinakes/app/data/repository/AuthRepository.kt`:
- Around line 32-54: The temporary `session.pendingAllowInsecureHttp` flag in
`AuthRepository.discover` is left set after the discovery request, so it should
be scoped to only that call. Wrap the `api.health()` invocation in `discover()`
with a `try/finally` and always reset `pendingAllowInsecureHttp` back to false
in the `finally` block so it is cleared on both success and failure. Keep the
change localized to `discover` and use the existing
`session.pendingAllowInsecureHttp` field and `NetworkModule`/`apiCall` flow as
the reference points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47620bd2-d185-4a7b-86db-92d26ab9ef2f
📒 Files selected for processing (3)
app/src/main/java/com/pinakes/app/data/network/NetworkModule.ktapp/src/main/java/com/pinakes/app/data/repository/AuthRepository.ktapp/src/main/java/com/pinakes/app/data/store/SessionStore.kt
1 participant
Found by an emulator smoke test of #20, before cutting a release.
Regression: the CleartextGuardInterceptor (added in #20 to close a CodeRabbit gap) reads
SessionStore.allowInsecureHttp, which is only persisted when an instance is committed. During onboarding discovery the instance isn't committed yet, so the flag was stillfalseand the guard blocked the/healthprobe to a plain-HTTP host even with the "Allow insecure HTTP" switch ON. The toggle affected the derived URL andisTransportAllowed(), but not the actual request gate — so a self-hoster (issue #16) could enable the toggle and still never get past onboarding.Fix: a transient in-memory
SessionStore.pendingAllowInsecureHttp, set byAuthRepository.discover()from the onboarding toggle. The NetworkModule guard now allows cleartext when EITHER the persisted flag OR the pending flag is set. Reset onclearAll()(forget instance). Coil/PDF keep using the persisted flag only (they run post-commit).Verified on the emulator (android-35):
http://<non-loopback>→ no request issued (guard blocks) → "Couldn't reach that address".GET …/healthis actually attempted (guard passes).http://10.0.2.2:8081) discovery returns200 OKand the discovery card renders, as before.assembleDebugpasses; existingNetworkUrlTestunaffected.Summary by CodeRabbit