From 1f18e85863fd846b58e3f46d78fdee01e4eeab28 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Mon, 21 Sep 2026 17:15:54 -0400 Subject: [PATCH 1/2] fix: read the query of an opaque uri on JVM and Android `amethyst+walletconnect:dlnwc?value=...` came back as "Amethyst received a URI to open but that uri was invalid", while the `//` spelling of the very same link worked. isWalletConnectRoute() accepts three spellings; only two survived the parse behind it. A uri whose scheme is followed by anything but `/` is *opaque* to java.net.URI: it reports no query at all and folds `dlnwc?value=...` into one scheme-specific part. The linux actual of UriParser -- a hand-rolled parser -- looks for `?` wherever it sits, so the actuals disagreed and the JVM one was the one losing data. Nip47DeepLink.parseCallbackValue rides the same call, so NWC callbacks were exposed to it too. The query is now recovered from the scheme-specific part when the uri is opaque. The fragment needs no such rescue: java.net.URI does parse `#...` off an opaque uri, and a test pins that. UriParserTest states the contract for every actual. The existing UriToRouteTest only covered the bare `dlnwc?value=` form -- the spelling that already worked -- which is how this went unnoticed; it now covers all three, and fails on the one-colon form without this change. Co-Authored-By: Claude Opus 5 (1M context) --- .../amethyst/ui/UriToRouteTest.kt | 18 ++++ .../quartz/utils/UriParserTest.kt | 97 +++++++++++++++++++ .../quartz/utils/UriParser.jvmAndroid.kt | 22 ++++- 3 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/utils/UriParserTest.kt 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/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 -> From 71b1438c41834826f26b30769c885a162feb0c95 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Mon, 21 Sep 2026 17:16:07 -0400 Subject: [PATCH 2/2] fix: say why a bunker:// address cannot sign in, instead of "invalid key" Pasting a `bunker://` address into the login field reported "Invalid key: Could not sign in: Invalid hex bunker://...", which reads as "you mistyped it" for a string the user pasted correctly. It is not a key Amethyst failed to read, it is a sign-in method Android does not have. `AccountSettings.isWriteable()` is `privKey != null || externalSignerPackageName != null`: the only remote signing it can persist is an external signer app. The nip46* settings are Amethyst acting *as* a bunker for other apps -- the opposite direction -- and BunkerLoginUseCase is referenced only by desktopApp. So the login screen now says so. `nostrconnect://` is guarded the same way and for the same reason: it is an app asking to connect to your signer, and belongs on the NIP-46 signer screen. Both matched the wording the scanner's outcome sheet already uses. This does not add bunker sign-in to Android; it stops misreporting its absence. Whether to implement it is a separate decision. Verified on device: the add-account login screen shows the explanation. Co-Authored-By: Claude Opus 5 (1M context) --- .../screen/loggedOff/login/LoginViewModel.kt | 15 ++++ amethyst/src/main/res/values/strings.xml | 2 + .../login/LoginViewModelKeyKindTest.kt | 81 +++++++++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedOff/login/LoginViewModelKeyKindTest.kt 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/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()) + } +}