diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModel.kt index ef7e79f0bf..720f4ae845 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModel.kt @@ -128,6 +128,21 @@ class LoginViewModel : ViewModel() { return false } + // Caught here rather than left to the key parser. Neither of these is a key, so both used + // to fall through to the hex branch and come back as "Invalid key: ... Invalid hex + // bunker://...", which reads as "you mistyped it" for a string the user pasted correctly. + // Amethyst for Android is the bunker, never the bunker's client: the only remote signing it + // can persist is an external signer app (see AccountSettings.isWriteable). + val trimmedKey = key.text.trim() + if (trimmedKey.startsWith("bunker:", ignoreCase = true)) { + errorManager.error(R.string.login_bunker_not_supported) + return false + } + if (trimmedKey.startsWith("nostrconnect:", ignoreCase = true)) { + errorManager.error(R.string.login_nostrconnect_not_supported) + return false + } + if (needsPassword && password.text.isBlank()) { errorManager.error(R.string.password_is_required) return false diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index db5d7c8a76..22f30b780d 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -123,6 +123,8 @@ Follow back Hide Password Invalid key + Amethyst for Android can\'t sign in with a bunker:// address. It hands these out so other apps can ask it to sign, but it cannot sign in with one. Use an external signer app, or Amethyst Desktop. + That\'s an app asking to connect to your signer, not a way to sign in. Sign in first, then open it from the NIP-46 signer screen. Invalid key: %1$s Password is required Key is required diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/UriToRouteTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/UriToRouteTest.kt index 67425d5405..95b20c73c2 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/UriToRouteTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/UriToRouteTest.kt @@ -93,6 +93,24 @@ class UriToRouteTest { ) } + @Test + fun walletConnectDeepLinksRouteInEverySpellingIsWalletConnectRouteAccepts() { + // isWalletConnectRoute() accepts three spellings, so all three have to survive the parse + // behind it. They did not: `java.net.URI` calls a scheme followed by anything but `/` an + // *opaque* uri and reports it as having no query at all, so the one-colon form lost its + // `value=` and came back to the user as "that uri was invalid" -- while the `//` spelling + // of the very same link worked. Only the bare form was covered here before. + val encoded = URLEncoder.encode(NWC_URI, Charsets.UTF_8.name()) + + listOf( + "dlnwc?value=$encoded", + "amethyst+walletconnect:dlnwc?value=$encoded", + "amethyst+walletconnect://dlnwc?value=$encoded", + ).forEach { deepLink -> + assertEquals(deepLink, Route.WalletAddNwc(NWC_URI), uriToRoute(deepLink, account)) + } + } + @Test fun theLauncherShortcutOpensTheScannerDirectly() { // res/xml/shortcuts.xml fires `amethyst:scanqr`. If this stops resolving, long-pressing the diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModelKeyKindTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModelKeyKindTest.kt new file mode 100644 index 0000000000..839d80dc38 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModelKeyKindTest.kt @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.screen.loggedOff.login + +import androidx.compose.ui.text.input.TextFieldValue +import com.vitorpamplona.amethyst.R +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Test + +/** + * Amethyst for Android is the bunker, never the bunker's client. + * + * `AccountSettings.isWriteable()` is `privKey != null || externalSignerPackageName != null`: the + * only remote signing it can persist is an external signer app. A `bunker://` address pasted into + * the key field is therefore not a key it failed to read, it is a sign-in method it does not have + * — but it used to fall through to the hex branch and come back as + * "Invalid key: Could not sign in: Invalid hex bunker://…", which reads as "you mistyped it" for a + * string the user pasted correctly. + */ +class LoginViewModelKeyKindTest { + private fun viewModelWithKey(text: String) = + LoginViewModel().apply { + acceptedTerms = true + key = TextFieldValue(text) + } + + private fun errorResIdOf(model: LoginViewModel) = (model.errorManager.error as? LoginErrorManager.SingleErrorMsg)?.errorResId + + @Test + fun aBunkerAddressIsRefusedAsAnUnsupportedMethodNotAsABadKey() { + val model = viewModelWithKey("bunker://${"a".repeat(64)}?relay=wss%3A%2F%2Fr.example&secret=s1") + + assertFalse(model.checkCanLogin()) + assertEquals(R.string.login_bunker_not_supported, errorResIdOf(model)) + } + + /** Same defect, same field: a connect offer is not a sign-in method either. */ + @Test + fun aNostrConnectOfferIsRefusedWithItsOwnExplanation() { + val model = viewModelWithKey("nostrconnect://${"b".repeat(64)}?relay=wss%3A%2F%2Fr.example&secret=s1") + + assertFalse(model.checkCanLogin()) + assertEquals(R.string.login_nostrconnect_not_supported, errorResIdOf(model)) + } + + /** Pasted from a chat app, which loves to add a trailing space. */ + @Test + fun surroundingWhitespaceDoesNotHideTheExplanation() { + val model = viewModelWithKey(" bunker://${"a".repeat(64)}?secret=s1 ") + + assertFalse(model.checkCanLogin()) + assertEquals(R.string.login_bunker_not_supported, errorResIdOf(model)) + } + + /** An npub is still a key, so the guard must not swallow the ordinary path. */ + @Test + fun anOrdinaryKeyStillPassesTheCheck() { + val model = viewModelWithKey("npub1${"q".repeat(58)}") + + assertEquals(true, model.checkCanLogin()) + } +} diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UriParserTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UriParserTest.kt new file mode 100644 index 0000000000..a76776ad7f --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UriParserTest.kt @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * The contract every actual of [UriParser] has to keep. + * + * The interesting case is the *opaque* URI -- a scheme followed by anything but `/`. The JVM and + * Android actual delegates to `java.net.URI`, which declares such a URI to have no query at all + * and folds `dlnwc?value=...` into one scheme-specific part, while the hand-rolled actuals look + * for `?` wherever it sits. That disagreement is what made a wallet-connect deep link report + * "that uri was invalid" on Android while the `//` spelling of the same link worked. + */ +class UriParserTest { + private val nwc = "nostr+walletconnect://abc?relay=wss://r.example&secret=s1" + + @Test + fun readsTheQueryOfAnOpaqueUri() { + val parser = UriParser("amethyst+walletconnect:dlnwc?value=${encode(nwc)}") + + assertEquals(nwc, parser.getQueryParameter("value")?.firstOrNull()) + } + + /** The spelling that already worked, so the rescue above cannot have broken it. */ + @Test + fun readsTheQueryOfAHierarchicalUri() { + val parser = UriParser("amethyst+walletconnect://dlnwc?value=${encode(nwc)}") + + assertEquals(nwc, parser.getQueryParameter("value")?.firstOrNull()) + } + + /** No scheme at all, which is how the callback arrives from some wallets. */ + @Test + fun readsTheQueryOfASchemelessUri() { + val parser = UriParser("dlnwc?value=${encode(nwc)}") + + assertEquals(nwc, parser.getQueryParameter("value")?.firstOrNull()) + } + + @Test + fun opaqueUriWithoutAQueryHasNoParameters() { + val parser = UriParser("cashu:sometoken") + + assertNull(parser.getQueryParameter("value")) + assertEquals(emptySet(), parser.queryParameterNames()) + } + + @Test + fun namesEveryParameterOfAnOpaqueUri() { + val parser = UriParser("bunker:pubkeyhex?relay=wss%3A%2F%2Fr.example&secret=s1") + + assertEquals(setOf("relay", "secret"), parser.queryParameterNames()) + assertEquals("wss://r.example", parser.getQueryParameter("relay")?.firstOrNull()) + assertEquals("s1", parser.getQueryParameter("secret")?.firstOrNull()) + } + + /** A fragment is parsed off an opaque URI by every actual already; keep it that way. */ + @Test + fun keepsTheFragmentOfAnOpaqueUri() { + val parser = UriParser("scheme:thing?a=1#b=2") + + assertEquals("1", parser.getQueryParameter("a")?.firstOrNull()) + assertEquals(mapOf("b" to "2"), parser.fragments()) + } + + private fun encode(value: String) = + value + .replace("%", "%25") + .replace("+", "%2B") + .replace(":", "%3A") + .replace("/", "%2F") + .replace("?", "%3F") + .replace("&", "%26") + .replace("=", "%3D") +} diff --git a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/utils/UriParser.jvmAndroid.kt b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/utils/UriParser.jvmAndroid.kt index 72057834ef..a961dbb53f 100644 --- a/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/utils/UriParser.jvmAndroid.kt +++ b/quartz/src/jvmAndroid/kotlin/com/vitorpamplona/quartz/utils/UriParser.jvmAndroid.kt @@ -28,8 +28,28 @@ actual class UriParser actual constructor( ) { private val myUri = URI.create(uri) + /** + * The query string, including the one [java.net.URI] refuses to find. + * + * A URI whose scheme is followed by anything other than `/` is *opaque* to `java.net.URI`: + * `amethyst+walletconnect:dlnwc?value=...` has no query at all as far as it is concerned, and + * the whole `dlnwc?value=...` is one scheme-specific part. Every other actual of this class + * looks for `?` wherever it sits, so on JVM and Android alone the parameters of an opaque URI + * silently vanished -- which is how a perfectly valid wallet-connect deep link came back as + * "that uri was invalid", while the `//` spelling of the very same link worked. + * + * The fragment needs no such rescue: `java.net.URI` does parse `#...` off an opaque URI. + */ + private val rawQuery: String? = + myUri.rawQuery + ?: myUri + .takeIf { it.isOpaque } + ?.rawSchemeSpecificPart + ?.substringAfter('?', "") + ?.ifBlank { null } + private val queryParameters: Map> by lazy { - myUri.rawQuery?.ifBlank { null }?.let { query -> + rawQuery?.ifBlank { null }?.let { query -> val params = mutableMapOf>() query.split('&').forEach { paramValue ->