feat(onboarding): opt-in "Allow insecure HTTP" for HTTP-only self-hosted instances (#16)#20
Conversation
…ted instances (#16) Self-hosters who run Pinakes over plain HTTP (e.g. a QNAP/Docker instance with no TLS) couldn't connect: the app requires HTTPS for any non-loopback host, so a bare host is upgraded to https:// (which the server doesn't answer) and an explicit http:// URL is blocked by the OS cleartext policy — surfacing as the generic "Couldn't reach that address" while the same instance opens fine in a browser. Adds an explicit, off-by-default opt-in so those users can connect at their own risk without weakening the default for everyone: - network_security_config: a static config can't whitelist a runtime-entered host, so cleartext is permitted at the OS level and the app becomes the sole gatekeeper. - NetworkModule.deriveApiBaseUrl()/isTransportAllowed() take an allowInsecure flag: when set, a bare host is prefixed http:// and any http:// host is accepted; otherwise the existing "HTTPS required except loopback" rule is unchanged. - SessionStore persists the per-instance allow_insecure_http flag; AuthRepository threads it through discover()/commitInstance(). - Onboarding shows an "Allow insecure HTTP" switch (off by default) with a clear "connection won't be encrypted — use at your own risk" hint, in all 4 locales. Without the toggle the app still refuses plain-HTTP remote instances exactly as before. Unit tests cover both the opt-in path and the default-rejects-remote-http regression. Verified: compileDebugKotlin + NetworkUrlTest pass.
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughIntrodotto un flag opt-in ChangesOpt-in HTTP non sicuro
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OnboardingScreen
participant OnboardingViewModel
participant AuthRepository
participant NetworkModule
participant SessionStore
OnboardingScreen->>OnboardingViewModel: onAllowInsecureChange(true)
OnboardingViewModel->>OnboardingViewModel: aggiorna allowInsecureHttp, resetta errori/discovery
OnboardingScreen->>OnboardingViewModel: avvia discovery
OnboardingViewModel->>AuthRepository: discover(url, allowInsecureHttp)
AuthRepository->>NetworkModule: deriveApiBaseUrl(url, allowInsecure)
AuthRepository->>NetworkModule: isTransportAllowed(apiBaseUrl, allowInsecure)
NetworkModule-->>AuthRepository: apiBaseUrl, transportAllowed
AuthRepository-->>OnboardingViewModel: HealthDiscovery(allowInsecure, transportAllowed)
OnboardingViewModel->>AuthRepository: commitInstance(discovery)
AuthRepository->>SessionStore: saveInstance(origin, apiBaseUrl, libraryName, allowInsecure)
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt (1)
109-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRendere l'intera riga interattiva per l'accessibilità.
I due
Text(label e hint) non sono associati semanticamente alloSwitch, e l'area di tocco è limitata al solo switch. Spostando l'azione suModifier.toggleabledellaRowconrole = Role.Switche impostandoonCheckedChange = nullsulloSwitch, l'etichetta viene annunciata insieme allo stato dal lettore schermo e l'intera riga diventa toccabile.♻️ Refactor proposto per l'accessibilità
Row( - modifier = form, + modifier = form.toggleable( + value = state.allowInsecureHttp, + role = Role.Switch, + onValueChange = vm::onAllowInsecureChange, + ), verticalAlignment = Alignment.CenterVertically, ) { Column(Modifier.weight(1f)) { Text( stringResource(R.string.onboarding_allow_http_label), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurface, ) Text( stringResource(R.string.onboarding_allow_http_hint), style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, ) } Spacer(Modifier.width(Spacing.md)) Switch( checked = state.allowInsecureHttp, - onCheckedChange = vm::onAllowInsecureChange, + onCheckedChange = null, ) }Richiede gli import
androidx.compose.foundation.selection.toggleableeandroidx.compose.ui.semantics.Role.🤖 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/ui/screens/onboarding/OnboardingScreen.kt` around lines 109 - 130, Make the entire onboarding HTTP setting row accessible by moving the toggle interaction from the Switch to the parent Row in OnboardingScreen. Apply Modifier.toggleable with role = Role.Switch to the Row that wraps the label/hint and switch, so the full row is clickable and the text is announced with the state. Keep the existing state and handler wiring from state.allowInsecureHttp and vm::onAllowInsecureChange, and set onCheckedChange to null on the Switch so it only reflects the Row’s toggle behavior.
🤖 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.
Inline comments:
In `@app/src/main/res/xml/network_security_config.xml`:
- Around line 3-21: The permissive cleartext setting in network_security_config
is too broad because traffic can still bypass NetworkModule.isTransportAllowed()
via other loaders like Coil coverUrl and PdfReaderDialog downloads. Tighten the
network-security-config so cleartext is only allowed for loopback/dev hosts, or
route those non-NetworkModule requests through the same transport check used by
NetworkModule and OnboardingViewModel/SessionStore.allowInsecureHttp. Keep the
fix focused on the base-config and the related request paths that currently
bypass the app-level gate.
---
Nitpick comments:
In `@app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.kt`:
- Around line 109-130: Make the entire onboarding HTTP setting row accessible by
moving the toggle interaction from the Switch to the parent Row in
OnboardingScreen. Apply Modifier.toggleable with role = Role.Switch to the Row
that wraps the label/hint and switch, so the full row is clickable and the text
is announced with the state. Keep the existing state and handler wiring from
state.allowInsecureHttp and vm::onAllowInsecureChange, and set onCheckedChange
to null on the Switch so it only reflects the Row’s toggle behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0673014d-4107-4a56-a8ac-f61279a0d7cf
📒 Files selected for processing (11)
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.ktapp/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.ktapp/src/main/res/xml/network_security_config.xmlapp/src/test/java/com/pinakes/app/NetworkUrlTest.kti18n/de.jsoni18n/en.jsoni18n/fr.jsoni18n/it.json
…o (CodeRabbit) The opt-in flipped the network-security-config base to cleartextTrafficPermitted=true, which alone would let ANY loader do plain HTTP — not just the API client that goes through NetworkModule.isTransportAllowed(). Coil (book covers) and the ebook PDF download each build their own OkHttpClient and bypassed that gate, so on an HTTPS instance an http:// cover/PDF URL could have downgraded silently. Add a shared CleartextGuardInterceptor (blocks http:// to a non-loopback host unless SessionStore.allowInsecureHttp) and wire it into all three OkHttp clients: NetworkModule, the Coil ImageLoader (PinakesApplication, via a Hilt EntryPoint) and PdfReaderDialog's downloader. The app is now the real universal gate; cleartext is only ever issued for loopback/dev hosts or an instance the user knowingly accepted as insecure. Verified: compileDebugKotlin passes.
1 participant
Fixes #16.
Problem
A self-hoster on plain HTTP (Uwe's QNAP/Docker instance, no TLS) can't connect: the app requires HTTPS for any non-loopback host, so a bare host is upgraded to
https://(which the server doesn't answer) and an explicithttp://URL is blocked by Android's cleartext policy — both surface as the generic "Couldn't reach that address", even though the same instance opens fine in a browser on the same phone. So it looks like a networking/Docker problem when it's really the HTTPS-only enforcement.Change — explicit, off-by-default opt-in
Lets those users connect at their own risk without weakening the default for everyone:
deriveApiBaseUrl()/isTransportAllowed()take anallowInsecureflag — when set, a bare host is prefixedhttp://and anyhttp://host is accepted; otherwise the existing "HTTPS required except loopback" rule is untouched.allow_insecure_httpflag; AuthRepository threads it throughdiscover()/commitInstance().Without the toggle, the app refuses plain-HTTP remote instances exactly as before.
Tests
NetworkUrlTestgains coverage for the opt-in path (bare host →http://, explicithttps://still wins, remotehttp://accepted only with the flag) plus a regression guard that the default still rejects remote HTTP. Verified:compileDebugKotlin+testDebugUnitTest(NetworkUrlTest) pass.Note on the security trade-off
Cleartext is now permitted at the OS layer, so the app — not the manifest — enforces transport policy. This is the standard pattern for apps that let users add arbitrary self-hosted instances (Nextcloud/Jellyfin do the same). A cleartext request is only ever issued for loopback/dev hosts or an instance the user knowingly accepted as insecure.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation