diff --git a/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/login/LoginContent.kt b/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/login/LoginContent.kt index 9af37217b..699ed810f 100644 --- a/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/login/LoginContent.kt +++ b/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/login/LoginContent.kt @@ -61,7 +61,6 @@ import de.davis.keygo.feature.item.create.presentation.component.KeyGoItemForm import de.davis.keygo.feature.item.create.presentation.component.OverrideTotpDialog import de.davis.keygo.feature.item.create.presentation.component.SelectItemForTotpModificationDialog import de.davis.keygo.feature.item.create.presentation.component.TAG_DELIMITERS -import de.davis.keygo.feature.item.create.presentation.component.TotpParseErrorDialog import de.davis.keygo.feature.item.create.presentation.login.model.DialogState import de.davis.keygo.feature.item.create.presentation.login.model.LoginBaseState import de.davis.keygo.feature.item.create.presentation.login.model.LoginPasskeyInfo @@ -73,6 +72,7 @@ import de.davis.keygo.feature.item.create.presentation.model.SharedItemState import de.davis.keygo.feature.item.create.presentation.model.VaultsState import de.davis.keygo.feature.item.create.presentation.password.GeneratePasswordModalBottomSheet import de.davis.keygo.feature.totp.presentation.component.QRScanner +import de.davis.keygo.feature.totp.presentation.component.TotpParseErrorDialog import de.davis.keygo.core.item.R as CoreItemR import de.davis.keygo.feature.item.core.R as ItemCoreR diff --git a/feature/item/create/src/main/res/values/strings.xml b/feature/item/create/src/main/res/values/strings.xml index c8294dacb..819cf1e82 100644 --- a/feature/item/create/src/main/res/values/strings.xml +++ b/feature/item/create/src/main/res/values/strings.xml @@ -11,9 +11,6 @@ Override TOTP fields? The current item has the following TOTP fields that differ from the fields specified in the totp information. Select fields you want to override. - TOTP Parse Error - The TOTP code could not be parsed. - TOTP Secret Override @@ -29,7 +26,6 @@ One or more entries match the issuer or account name from the totp code. Would you like to update an existing entry or create a new one? Warning - OK Cancel Delete Passkey diff --git a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginContent.kt b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginContent.kt index 89ae7a34d..a13bc2ed6 100644 --- a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginContent.kt +++ b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginContent.kt @@ -92,6 +92,7 @@ import de.davis.keygo.feature.item.view.login.model.ViewLoginUiEvent import de.davis.keygo.feature.item.view.onHold import de.davis.keygo.feature.totp.domain.model.TotpValue import de.davis.keygo.feature.totp.presentation.component.QRScanner +import de.davis.keygo.feature.totp.presentation.component.TotpParseErrorDialog import de.davis.keygo.core.item.R as CoreItemR import de.davis.keygo.core.ui.R as CoreUiR import de.davis.keygo.feature.item.core.R as ItemCoreR @@ -460,6 +461,13 @@ fun ViewLoginContent(state: ViewLoginState, onEvent: (ViewLoginUiEvent) -> Unit) } } + if (state.totpParseError) { + TotpParseErrorDialog( + onDismiss = { onEvent(ViewLoginUiEvent.OnTotpParseErrorDismiss) }, + modifier = Modifier.fillMaxWidth(), + ) + } + if (state.scanning) { QRScanner( onClose = { onEvent(ViewLoginUiEvent.OnBackClick) }, diff --git a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginViewModel.kt b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginViewModel.kt index 06a557900..b080275af 100644 --- a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginViewModel.kt +++ b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/ViewLoginViewModel.kt @@ -16,6 +16,7 @@ import de.davis.keygo.core.util.domain.resolver.RegistrableDomainResolver import de.davis.keygo.core.util.domain.usecase.SortUseCase import de.davis.keygo.core.util.fold import de.davis.keygo.core.util.getOrNull +import de.davis.keygo.core.util.isSuccess import de.davis.keygo.core.util.onFailure import de.davis.keygo.core.util.onSuccess import de.davis.keygo.feature.item.core.domain.model.ItemUpsertError @@ -76,6 +77,7 @@ internal class ViewLoginViewModel( private val _modificationDialogState = MutableStateFlow(null) private val _scanning = MutableStateFlow(false) + private val _totpParseError = MutableStateFlow(false) private val _itemId = MutableStateFlow(null) @OptIn(ExperimentalCoroutinesApi::class) @@ -135,10 +137,12 @@ internal class ViewLoginViewModel( _stateWithoutModification, _modificationDialogState, _scanning, - ) { state, modificationDialog, scanning -> + _totpParseError, + ) { state, modificationDialog, scanning, totpParseError -> state.copy( modificationDialog = modificationDialog, scanning = scanning, + totpParseError = totpParseError, ) }.stateIn( scope = viewModelScope, @@ -230,10 +234,14 @@ internal class ViewLoginViewModel( is ViewLoginUiEvent.OnCodesScanned -> { _scanning.update { false } - val uriOrSecret = event.codes.firstNotNullOfOrNull { qrCode -> - // This just validates whether `qrCode` is a valid totp uri - we return qrCode - totpService.getInfoFromUriWithResult(qrCode).getOrNull()?.let { qrCode } - } ?: return + val uriOrSecret = event.codes.firstOrNull { + totpService.getInfoFromUriWithResult(it).isSuccess() + } + + if (uriOrSecret == null) { + _totpParseError.update { true } + return + } _itemId.value?.let { id -> viewModelScope.launch { @@ -247,6 +255,10 @@ internal class ViewLoginViewModel( } } + ViewLoginUiEvent.OnTotpParseErrorDismiss -> { + _totpParseError.update { false } + } + is ViewLoginUiEvent.OnSubmitModification -> { val dialog = _modificationDialogState.value ?: return val newText = fieldUpdate(event.input) diff --git a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginState.kt b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginState.kt index e768dc83e..a51e8a1f2 100644 --- a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginState.kt +++ b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginState.kt @@ -21,5 +21,6 @@ data class ViewLoginState( val note: String = "", val modificationDialog: ModificationDialog? = null, val scanning: Boolean = false, + val totpParseError: Boolean = false, val pinned: Boolean = false, ) diff --git a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginUiEvent.kt b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginUiEvent.kt index 5581b7735..c6b14a913 100644 --- a/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginUiEvent.kt +++ b/feature/item/view/src/main/kotlin/de/davis/keygo/feature/item/view/login/model/ViewLoginUiEvent.kt @@ -16,4 +16,5 @@ sealed interface ViewLoginUiEvent { data object OnScanCodeRequest : ViewLoginUiEvent data class OnCodesScanned(val codes: List) : ViewLoginUiEvent + data object OnTotpParseErrorDismiss : ViewLoginUiEvent } diff --git a/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/component/TotpParseErrorDialog.kt b/feature/totp/src/main/kotlin/de/davis/keygo/feature/totp/presentation/component/TotpParseErrorDialog.kt similarity index 88% rename from feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/component/TotpParseErrorDialog.kt rename to feature/totp/src/main/kotlin/de/davis/keygo/feature/totp/presentation/component/TotpParseErrorDialog.kt index a3de56a9e..5a6d7b104 100644 --- a/feature/item/create/src/main/kotlin/de/davis/keygo/feature/item/create/presentation/component/TotpParseErrorDialog.kt +++ b/feature/totp/src/main/kotlin/de/davis/keygo/feature/totp/presentation/component/TotpParseErrorDialog.kt @@ -1,4 +1,4 @@ -package de.davis.keygo.feature.item.create.presentation.component +package de.davis.keygo.feature.totp.presentation.component import androidx.compose.material3.AlertDialog import androidx.compose.material3.Text @@ -6,7 +6,7 @@ import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource -import de.davis.keygo.feature.item.create.R +import de.davis.keygo.feature.totp.R @Composable fun TotpParseErrorDialog(onDismiss: () -> Unit, modifier: Modifier = Modifier) { diff --git a/feature/totp/src/main/res/values/strings.xml b/feature/totp/src/main/res/values/strings.xml index b0a768f6a..d8e13a17d 100644 --- a/feature/totp/src/main/res/values/strings.xml +++ b/feature/totp/src/main/res/values/strings.xml @@ -9,4 +9,8 @@ Camera permission Camera permission was denied. To scan QR codes, please enable it in your app settings. Open Settings + OK + + TOTP Parse Error + The TOTP code could not be parsed. \ No newline at end of file diff --git a/rust/rust-code/lib/src/totp.rs b/rust/rust-code/lib/src/totp.rs index 1d63351bc..b63fb4ad6 100644 --- a/rust/rust-code/lib/src/totp.rs +++ b/rust/rust-code/lib/src/totp.rs @@ -1,8 +1,12 @@ +use std::ops::RangeInclusive; use std::time::SystemTimeError; use thiserror::Error; pub use totp_rs::Algorithm; use totp_rs::{Secret, SecretParseError, TOTP, TotpUrlError}; +/// Digit counts RFC 6238 permits. +const ALLOWED_DIGITS: RangeInclusive = 6..=8; + #[derive(Debug, Error)] pub enum TotpError { #[error("url error: {0}")] @@ -13,17 +17,54 @@ pub enum TotpError { Secret(SecretParseError), } -pub fn get_totp( +/// The checks `TOTP::new` runs, minus its 128 bit secret floor. The unchecked +/// constructors skip all of them, so every caller runs this instead. +fn validate(totp: &TOTP) -> Result<(), TotpError> { + if totp.secret.is_empty() { + return Err(TotpError::Url(TotpUrlError::SecretSize(0))); + } + + if !ALLOWED_DIGITS.contains(&totp.digits) { + return Err(TotpError::Url(TotpUrlError::DigitsNumber(totp.digits))); + } + + if let Some(issuer) = totp.issuer.as_deref().filter(|it| it.contains(':')) { + return Err(TotpError::Url(TotpUrlError::Issuer(issuer.to_string()))); + } + + if totp.account_name.contains(':') { + return Err(TotpError::Url(TotpUrlError::AccountName( + totp.account_name.clone(), + ))); + } + + Ok(()) +} + +fn build_totp( algorithm: Algorithm, digits: usize, step: u64, secret: String, -) -> Result { + issuer: Option, + account_name: String, +) -> Result { let secret = Secret::Encoded(secret) .to_bytes() .map_err(TotpError::Secret)?; - let totp = TOTP::new(algorithm, digits, 1, step, secret, None, "".to_string()) - .map_err(TotpError::Url)?; + let totp = TOTP::new_unchecked(algorithm, digits, 1, step, secret, issuer, account_name); + validate(&totp)?; + + Ok(totp) +} + +pub fn get_totp( + algorithm: Algorithm, + digits: usize, + step: u64, + secret: String, +) -> Result { + let totp = build_totp(algorithm, digits, step, secret, None, "".to_string())?; totp.generate_current().map_err(TotpError::Time) } @@ -37,10 +78,11 @@ pub struct TotpInfo { } pub fn get_totp_info_from_uri(uri: String) -> Result { - let totp = TOTP::from_url(uri).map_err(TotpError::Url)?; - let secret = Secret::Raw(totp.secret.clone()).to_encoded().to_string(); + let totp = TOTP::from_url_unchecked(uri).map_err(TotpError::Url)?; + validate(&totp)?; + Ok(TotpInfo { - secret, + secret: Secret::Raw(totp.secret.clone()).to_encoded().to_string(), issuer: totp.issuer.clone(), account_name: totp.account_name.clone(), algorithm: totp.algorithm, @@ -57,15 +99,82 @@ pub fn get_totp_url( issuer: Option, account_name: String, ) -> Result { - let secret = Secret::Encoded(secret) - .to_bytes() - .map_err(TotpError::Secret)?; - - let totp = TOTP::new(algorithm, digits, 1, step, secret, issuer, account_name) - .map_err(TotpError::Url)?; + let totp = build_totp(algorithm, digits, step, secret, issuer, account_name)?; Ok(totp.get_url()) } pub(crate) fn is_valid_totp_secret(s: &str) -> bool { base32::decode(base32::Alphabet::Rfc4648 { padding: false }, s).is_some() } + +#[cfg(test)] +mod tests { + use super::*; + + /// 80 bits, the kind of secret services like GitHub hand out. + const SHORT_SECRET: &str = "JBSWY3DPEHPK3PXP"; + /// 160 bits, the length RFC 4226 recommends. + const LONG_SECRET: &str = "JBSWY3DPEHPK3PXPJBSWY3DPEHPK3PXP"; + + fn uri(secret: &str) -> String { + format!("otpauth://totp/GitHub:alice?secret={secret}&issuer=GitHub") + } + + fn url_with_issuer(issuer: &str) -> Result { + get_totp_url( + Algorithm::SHA1, + 6, + 30, + SHORT_SECRET.to_string(), + Some(issuer.to_string()), + "alice".to_string(), + ) + } + + #[test] + fn generates_code_for_secret_below_128_bits() { + let code = get_totp(Algorithm::SHA1, 6, 30, SHORT_SECRET.to_string()).unwrap(); + assert_eq!(6, code.len()); + } + + #[test] + fn parses_uri_regardless_of_secret_length() { + let short = get_totp_info_from_uri(uri(SHORT_SECRET)).unwrap(); + let long = get_totp_info_from_uri(uri(LONG_SECRET)).unwrap(); + + assert_eq!(SHORT_SECRET, short.secret); + assert_eq!(LONG_SECRET, long.secret); + } + + #[test] + fn builds_url_for_secret_below_128_bits() { + let url = url_with_issuer("GitHub").unwrap(); + + assert!(url.contains(SHORT_SECRET), "unexpected url: {url}"); + } + + #[test] + fn rejects_empty_secret() { + assert!(get_totp(Algorithm::SHA1, 6, 30, "".to_string()).is_err()); + } + + #[test] + fn rejects_digits_outside_rfc_range() { + assert!(get_totp(Algorithm::SHA1, 9, 30, SHORT_SECRET.to_string()).is_err()); + } + + #[test] + fn rejects_colon_in_issuer() { + assert!(url_with_issuer("Git:Hub").is_err()); + } + + #[test] + fn rejects_uri_without_secret() { + assert!(get_totp_info_from_uri(uri("")).is_err()); + } + + #[test] + fn rejects_secret_that_is_not_base32() { + assert!(get_totp(Algorithm::SHA1, 6, 30, "not base32!".to_string()).is_err()); + } +}