fix(marmot): run the interop harness, and fix what it found

The harness had never actually been run. It now builds MDK 0.9.20 — against the same OpenMLS fork rev our vector generator pins — boots a local relay, brings up both wnd daemons and amy, and executes all 17 scenarios. They all still fail, downstream of MDK not finding A's KeyPackage, but "it runs" is the difference between having an interop signal and not having one. Four environment blockers stood between preflight and a run: protoc is now a build prerequisite; MDK 0.9.x needs WN_ALLOW_LOOPBACK_RELAYS=1 before it will accept a ws:// loopback relay at all; it refuses to create its socket unless the parent directory is 0700; and `wn --json whoami` moved to {"ok":true,"result":{"accounts":[…]}}, which the harness's extractor probed right past. Two defects in our own code came out of it. `amy relay add` reported success from its DECISION to write rather than from the store's answer, so a rejected or no-op write printed `added: yes` and the caller only discovered otherwise much later. The more consequential one: we read our OWN relay lists back through the local-network filter. That filter is correct for someone else's list — it is attacker-supplied input, and it is also what exempts a relay from Tor — but applied to a list we published ourselves it made a deliberately configured local relay look like no configuration at all. The publisher then fell back to a default set, and the harness sent A's KeyPackage to five PUBLIC relays instead of its loopback, which is the exact opposite of what a "nothing leaves the machine" harness is for. `allRelays()` now exists for reading back our own lists; the KeyPackage publish goes only to the configured relay. Test 01 is still blocked on a narrower puzzle: the kind-10051 list persists under `relay key-package set` but not under `relay add`, while kind 10050 works through the identical code path. That is a storage/CLI thread, not a protocol one, and it needs its own pass. Separately, and not a bug on either side: MDK accepts ws:// only for a loopback host while quartz strips exactly those hosts from relay lists. No address satisfies both, so a loopback-relay harness cannot pass until one side moves — and changing a Tor-adjacent privacy guard is a maintainer call, not one to make in passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq
2026-10-06 03:38:23 +00:00 · 2026-09-08 22:01:42 +00:00
parent 88fddce324
commit 0f264dd767
9 changed files with 139 additions and 30 deletions
@@ -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<NormalizedRelayUrl> =
-        keyPackageRelaysOf(identity.pubKeyHex)?.relays()?.takeIf { it.isNotEmpty() }?.toSet()
+        keyPackageRelaysOf(identity.pubKeyHex)?.allRelays()?.takeIf { it.isNotEmpty() }?.toSet()
            ?: outboxRelays()

    /** Union of all three buckets. */
@@ -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 }))
                    }
            }

@@ -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
@@ -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=""
@@ -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" \
@@ -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
@@ -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<NormalizedRelayUrl> = 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<NormalizedRelayUrl> = 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<String>): NormalizedRelayUrl? {
+    fun parse(tag: Array<String>): NormalizedRelayUrl? = parseUnfiltered(tag)?.takeUnless { it.isLocalHost() }
+
+    fun parseUnfiltered(tag: Array<String>): 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)
@@ -42,6 +42,9 @@ class ChatMessageRelayListEvent(
 ) : BaseReplaceableEvent(id, pubKey, createdAt, KIND, tags, content, sig) {
    fun relays(): List<NormalizedRelayUrl> = tags.mapNotNull(RelayTag::parse)

+    /** Every relay in this list, local ones included. For reading back our OWN list. */
+    fun allRelays(): List<NormalizedRelayUrl> = tags.mapNotNull(RelayTag::parseUnfiltered)
+
    companion object {
        const val KIND = 10050

@@ -34,16 +34,34 @@ class RelayTag {

        fun notMatch(tag: Array<String>) = !(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<String>): 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<String>): 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)