diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt index d330568c1e..15b1205945 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/Context.kt @@ -480,12 +480,19 @@ class Context( ?: DefaultDMRelayList.toSet() /** - * KeyPackage relays (MIP-00 kind:10051) for this account. Falls - * back to [outboxRelays] when no kind:10051 has been seen — same - * fallback the Android app uses for KeyPackage discovery. + * Our own KeyPackage relay list (MIP-00 kind:10051). Falls back to + * [outboxRelays] when no kind:10051 has been seen — the same fallback the + * Android app uses for KeyPackage discovery. + * + * `allRelays()`, not `relays()`: the filtered accessor drops local-network + * entries because someone else's list is attacker-supplied input, but this + * is a list we published ourselves. Reading it filtered made a deliberately + * configured local relay look like no configuration at all, and the + * publisher then fell back to a default relay set the operator never chose + * — sending a KeyPackage somewhere they did not pick. */ suspend fun keyPackageRelays(): Set = - keyPackageRelaysOf(identity.pubKeyHex)?.relays()?.takeIf { it.isNotEmpty() }?.toSet() + keyPackageRelaysOf(identity.pubKeyHex)?.allRelays()?.takeIf { it.isNotEmpty() }?.toSet() ?: outboxRelays() /** Union of all three buckets. */ diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt index 292a19b6b0..cb2fabe839 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/RelayCommands.kt @@ -146,7 +146,11 @@ object RelayCommands { "dm", ChatMessageRelayListEvent.KIND, setOf("chat", "inbox-dm"), - read = { c, pk -> c.dmInboxOf(pk)?.relays().orEmpty() }, + // allRelays(), not relays(): every Flat here is read for SELF, + // and the filtered accessor drops local entries meant for + // attacker-supplied lists — making a deliberately configured + // local relay report as no configuration at all. + read = { c, pk -> c.dmInboxOf(pk)?.allRelays().orEmpty() }, build = { c, r -> ChatMessageRelayListEvent.create(r, c.signer) }, ), Flat( @@ -154,7 +158,7 @@ object RelayCommands { "key_package", KeyPackageRelayListEvent.KIND, setOf("keypackage", "key_package"), - read = { c, pk -> c.keyPackageRelaysOf(pk)?.relays().orEmpty() }, + read = { c, pk -> c.keyPackageRelaysOf(pk)?.allRelays().orEmpty() }, build = { c, r -> KeyPackageRelayListEvent.create(r, c.signer) }, ), Flat( @@ -452,15 +456,21 @@ object RelayCommands { "add" -> { val url = urlArg(args) ?: return Output.invalidRelayUrl(args.positional(0, "url")) val existing = flat.read(ctx, self) - val added = existing.none { it.url == url.url } - if (added) ctx.verifyAndStore(flat.build(ctx, existing + url)) + // Report what the STORE did, not what we decided to try. + // These used to report the decision, so a rejected write + // printed `added: yes` and the caller only found out much + // later, when a publish silently fell back to defaults. + val added = + existing.none { it.url == url.url } && + ctx.verifyAndStore(flat.build(ctx, existing + url)) Output.emit(mapOf("noun" to flat.noun, "kind" to flat.kind, "url" to url.url, "added" to added)) } "remove", "rm" -> { val url = urlArg(args) ?: return Output.invalidRelayUrl(args.positional(0, "url")) val existing = flat.read(ctx, self) - val removed = existing.any { it.url == url.url } - if (removed) ctx.verifyAndStore(flat.build(ctx, existing.filterNot { it.url == url.url })) + val removed = + existing.any { it.url == url.url } && + ctx.verifyAndStore(flat.build(ctx, existing.filterNot { it.url == url.url })) Output.emit(mapOf("noun" to flat.noun, "kind" to flat.kind, "url" to url.url, "removed" to removed)) } "set" -> { @@ -601,13 +611,11 @@ object RelayCommands { val existing = flat.read(ctx, self) changed[flat.jsonKey] = if (add) { - val doAdd = existing.none { it.url == url.url } - if (doAdd) ctx.verifyAndStore(flat.build(ctx, existing + url)) - doAdd + existing.none { it.url == url.url } && + ctx.verifyAndStore(flat.build(ctx, existing + url)) } else { - val doRemove = existing.any { it.url == url.url } - if (doRemove) ctx.verifyAndStore(flat.build(ctx, existing.filterNot { it.url == url.url })) - doRemove + existing.any { it.url == url.url } && + ctx.verifyAndStore(flat.build(ctx, existing.filterNot { it.url == url.url })) } } diff --git a/cli/tests/lib.sh b/cli/tests/lib.sh index 913c2d8f9f..8109f1076b 100644 --- a/cli/tests/lib.sh +++ b/cli/tests/lib.sh @@ -320,6 +320,11 @@ extract_pubkey() { # JSON: {"result": [ {"pubkey": …}, … ]} — post-v0.2 `wn --json whoami` shape v=$(printf '%s' "$raw" | jq -r '.result[0].pubkey // .result[0].npub // .result[0].public_key // empty' 2>/dev/null || true) if [[ -n "$v" && "$v" != "null" ]]; then printf '%s' "$v"; return; fi + # JSON: {"ok":true,"result":{"accounts":[{"npub": ...}, ...]}} — MDK 0.9.x. + # `result` became an object with a named list, so the array-indexed probes + # above miss it entirely and the caller sees an empty npub. + v=$(printf '%s' "$raw" | jq -r '.result.accounts[0].npub // .result.accounts[0].pubkey // empty' 2>/dev/null || true) + if [[ -n "$v" && "$v" != "null" ]]; then printf '%s' "$v"; return; fi # JSON: array of accounts (whoami may return a list) v=$(printf '%s' "$raw" | jq -r '.[0].pubkey // .[0].npub // .[0].public_key // empty' 2>/dev/null || true) if [[ -n "$v" && "$v" != "null" ]]; then printf '%s' "$v"; return; fi diff --git a/cli/tests/marmot/marmot-interop-headless.sh b/cli/tests/marmot/marmot-interop-headless.sh index 59bd501fa3..3d18d10f91 100755 --- a/cli/tests/marmot/marmot-interop-headless.sh +++ b/cli/tests/marmot/marmot-interop-headless.sh @@ -54,6 +54,15 @@ RELAY_PORT="${RELAY_PORT:-8080}" RELAY_URL="ws://$RELAY_HOST:$RELAY_PORT" NO_BUILD=0 +# Required as of MDK 0.9.x. `validate_relay_url` accepts `wss://` +# unconditionally but `ws://` only for a loopback host AND only behind this +# explicit opt-in; without it wnd refuses the harness relay with "invalid relay +# URL" and exits before creating its socket. 127.0.0.2 is inside 127.0.0.0/8 and +# already passes MDK's own loopback test, so the env var is the gate, not the +# address. Exported once here so `wn` and `wnd` both inherit it — `wn` runs the +# same validation on any relay argument. +export WN_ALLOW_LOOPBACK_RELAYS=1 + A_NPUB="" A_HEX="" B_NPUB="" diff --git a/cli/tests/marmot/setup.sh b/cli/tests/marmot/setup.sh index 0c06556b9c..5d64c2d654 100644 --- a/cli/tests/marmot/setup.sh +++ b/cli/tests/marmot/setup.sh @@ -210,6 +210,11 @@ start_daemon() { -exec rm -rf {} + 2>/dev/null || true fi mkdir -p "$data_dir/logs" + # MDK refuses to create its socket if the socket's parent directory is + # group-writable or world-accessible ("unsafe on-disk permissions"). A default + # umask gives 0755, so tighten it explicitly rather than depending on whatever + # umask the caller's shell happens to have. + chmod 700 "$data_dir" # --discovery-relays / --default-account-relays are native wnd flags that # force both the discovery plane and freshly-created accounts' NIP-65 / inbox # / key-package lists onto our loopback relay (kills the "can't reach nos.lol" @@ -222,6 +227,7 @@ start_daemon() { # --secret-store file replaces the old mock-keyring source patch: account # secrets live in files under the data dir, so the daemon comes up in # containers and CI where the kernel keyring is unavailable. + # nohup "$WND_BIN" --data-dir "$data_dir" --logs-dir "$data_dir/logs" \ --socket "$socket" --secret-store file \ --discovery-relays "$RELAY_URL" --default-account-relays "$RELAY_URL" \ diff --git a/quartz/plans/2026-09-08-marmot-spec-resync.md b/quartz/plans/2026-09-08-marmot-spec-resync.md index f29c2b4f17..183c22ae0e 100644 --- a/quartz/plans/2026-09-08-marmot-spec-resync.md +++ b/quartz/plans/2026-09-08-marmot-spec-resync.md @@ -586,9 +586,38 @@ Writing the producer side immediately found two bugs the reader-side tests could ## What is NOT done -- **The interop harness has never been run.** Everything above is verified against the spec's - own published fixtures and against our own MLS stack talking to itself. Neither proves we - interoperate with a live MDK build; that needs MDK's pinned toolchain and a local relay. +- **The interop harness now RUNS but does not pass.** It used to die in preflight; it now + builds MDK 0.9.20 (against the same OpenMLS fork rev `mdk-vector-gen` pins), boots + nostr-rs-relay, brings up both `wnd` daemons and `amy`, and executes all 17 scenarios. Every + one fails, all downstream of Test 01 (MDK cannot find A's KeyPackage). Four environment + blockers were fixed to get that far, all recorded in the harness: + - `protoc` is a build prerequisite MDK now needs. + - MDK 0.9.x requires `WN_ALLOW_LOOPBACK_RELAYS=1` before it will accept a `ws://` loopback + relay at all; without it `wnd` exits before creating its socket. + - MDK refuses to create its socket unless the socket's parent directory is `0700`. + - `wn --json whoami` moved to `{"ok":true,"result":{"accounts":[…]}}`; the harness's + `extract_pubkey` probed only the older array shapes and silently returned nothing. + + Two real defects in our own code came out of the run, both fixed: + - `amy relay add` reported success from the DECISION to write, not the store's answer, so a + rejected or no-op write printed `added: yes`. + - Our own KeyPackage/DM relay lists were read back through the local-network filter. That + filter is right for someone else's list — it is attacker-supplied input, and it is also what + exempts a relay from Tor — but applying it to a list we published ourselves made a + deliberately configured local relay look like no configuration at all. The publisher then + fell back to a default set, and A's KeyPackage went to five PUBLIC relays instead of the + harness's loopback. `allRelays()` now exists for reading back our own lists, and the + KeyPackage publish goes only to the configured relay. + + **Still blocking Test 01:** the kind-10051 list persists under `relay key-package set` but not + under `relay add`/`relay key-package add`, so MDK finds no relay list to fetch A's KeyPackage + from. `verifyAndStore` returns true and kind 10050 works through the identical code path, so + this is a storage/CLI issue rather than a protocol one, and it needs its own focused pass. + + **Also unresolved, and it is a design conflict rather than a bug:** MDK accepts `ws://` ONLY + for a loopback host, while quartz strips exactly those hosts out of relay lists. No address + satisfies both, so a loopback-relay harness cannot work until one side moves. Changing a + Tor-adjacent privacy guard is a maintainer decision, not one to make in passing. - **`MarmotManager.createGroup` (the MIP-era path) is still the one the UI calls.** `createCurrentProfileGroup` exists, is wired, and is tested, but the Android and desktop "new group" flows still call the legacy one. KeyPackage publishing HAS switched: it now diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageRelayListEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageRelayListEvent.kt index 137afb2c55..613cba6650 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageRelayListEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mip00KeyPackages/KeyPackageRelayListEvent.kt @@ -25,6 +25,7 @@ import com.vitorpamplona.quartz.nip01Core.core.Address import com.vitorpamplona.quartz.nip01Core.core.BaseReplaceableEvent import com.vitorpamplona.quartz.nip01Core.core.HexKey import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl +import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer import com.vitorpamplona.quartz.nip01Core.relay.normalizer.isLocalHost import com.vitorpamplona.quartz.nip01Core.signers.NostrSigner import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerSync @@ -48,8 +49,32 @@ class KeyPackageRelayListEvent( content: String, sig: HexKey, ) : BaseReplaceableEvent(id, pubKey, createdAt, KIND, tags, content, sig) { + /** + * Relays from this list, with local-network entries dropped. + * + * The drop is for lists that came from SOMEONE ELSE. A relay list is + * attacker-supplied input, and an entry pointing at `127.0.0.1` or a + * RFC 1918 address would aim our connection at our own machine or LAN. It + * is also what exempts a relay from Tor, so an unfiltered entry could + * quietly strip a relay's own onion routing. + * + * Use [allRelays] to read back a list this account published itself; our + * own configuration is not attacker-supplied, and silently dropping it + * makes a deliberately configured local relay look unconfigured. + */ fun relays(): List = tags.mapNotNull(RelayTag::parse) + /** + * Every relay in this list, including local ones. + * + * For reading back OUR OWN published list. A user who configured a local + * relay meant it, and [relays] would report their list as empty — which a + * publisher then treats as "unconfigured" and answers with a default set + * the user never chose. Sending a KeyPackage somewhere the user did not + * pick is a worse outcome than the one the filter guards against. + */ + fun allRelays(): List = tags.mapNotNull(RelayTag::parseUnfiltered) + companion object { const val KIND = 10051 @@ -98,12 +123,11 @@ class KeyPackageRelayListEvent( private object RelayTag { const val TAG_NAME = "relay" - fun parse(tag: Array): NormalizedRelayUrl? { + fun parse(tag: Array): NormalizedRelayUrl? = parseUnfiltered(tag)?.takeUnless { it.isLocalHost() } + + fun parseUnfiltered(tag: Array): NormalizedRelayUrl? { if (tag.size < 2 || tag[0] != TAG_NAME || tag[1].isEmpty()) return null - val relay = - com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer - .normalizeOrNull(tag[1]) - return relay?.takeUnless { it.isLocalHost() } + return RelayUrlNormalizer.normalizeOrNull(tag[1]) } fun assemble(relay: NormalizedRelayUrl) = arrayOf(TAG_NAME, relay.url) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/ChatMessageRelayListEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/ChatMessageRelayListEvent.kt index 80ed9a356f..10847c6d38 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/ChatMessageRelayListEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/ChatMessageRelayListEvent.kt @@ -42,6 +42,9 @@ class ChatMessageRelayListEvent( ) : BaseReplaceableEvent(id, pubKey, createdAt, KIND, tags, content, sig) { fun relays(): List = tags.mapNotNull(RelayTag::parse) + /** Every relay in this list, local ones included. For reading back our OWN list. */ + fun allRelays(): List = tags.mapNotNull(RelayTag::parseUnfiltered) + companion object { const val KIND = 10050 diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/tags/RelayTag.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/tags/RelayTag.kt index 1268939741..ffc32f424f 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/tags/RelayTag.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip17Dm/settings/tags/RelayTag.kt @@ -34,16 +34,34 @@ class RelayTag { fun notMatch(tag: Array) = !(tag.has(0) && tag[0] == TAG_NAME) + /** + * Parse, dropping local-network entries. + * + * The drop is for lists that came from SOMEONE ELSE: a relay list is + * attacker-supplied input, and an entry naming `127.0.0.1` or an + * RFC 1918 address would aim our connection at our own machine or LAN. + * Use [parseUnfiltered] to read back a list this account published + * itself. + */ fun parse(tag: Array): NormalizedRelayUrl? { + val relay = parseUnfiltered(tag) + ensure(relay != null && !relay.isLocalHost()) { return null } + return relay + } + + /** + * Parse without the local-network drop, for reading back OUR OWN list. + * + * A user who configured a local relay meant it, and [parse] would + * report their list as empty — which a publisher then treats as + * "unconfigured" and answers with a default relay set the user never + * chose. + */ + fun parseUnfiltered(tag: Array): NormalizedRelayUrl? { ensure(tag.has(1)) { return null } ensure(tag[0] == TAG_NAME) { return null } ensure(tag[1].isNotEmpty()) { return null } - - val relay = RelayUrlNormalizer.normalizeOrNull(tag[1]) - - ensure(relay != null && !relay.isLocalHost()) { return null } - - return relay + return RelayUrlNormalizer.normalizeOrNull(tag[1]) } fun assemble(relay: NormalizedRelayUrl) = arrayOf(TAG_NAME, relay.url)