diff --git a/cli/tests/marmot/tests-extras.sh b/cli/tests/marmot/tests-extras.sh index fa602e656..dd4e03e3d 100644 --- a/cli/tests/marmot/tests-extras.sh +++ b/cli/tests/marmot/tests-extras.sh @@ -240,12 +240,24 @@ test_14_wn_removes_a() { # Amy should observe that she's no longer a member. `group show` returns # a not_member error as soon as the Remove commit is applied locally. + # + # The amy CLI contract puts error JSON on stderr (README: "stdout is + # JSON … stderr is for humans"), but stderr is interleaved with debug + # logs from `Log.d(…)` calls in quartz, so feeding the captured stream + # straight into `jq` fails to parse. Capture stderr alongside stdout + # via `2>&1` and grep for the `"error":"not_member"` literal — that + # signature is unambiguous (the debug log lines never produce JSON). + # Also tee the per-iteration capture into $LOG_FILE so post-mortem + # logs include each polling sync's `[cli] ingest …` trace, mirroring + # what amy_json normally writes when stderr isn't being captured. local deadline=$(( $(date +%s) + 120 )) removed=0 while [[ $(date +%s) -lt $deadline ]]; do local show rc - show=$(amy_json marmot group show "$a_gid" 2>&1) + show=$("$AMY_BIN" --data-dir "$A_DIR" --secret-backend plaintext \ + marmot group show "$a_gid" 2>&1) rc=$? - if [[ $rc -ne 0 ]] && printf '%s' "$show" | jq -e '.error == "not_member"' >/dev/null 2>&1; then + printf '%s\n' "$show" >>"$LOG_FILE" + if [[ $rc -ne 0 ]] && printf '%s' "$show" | grep -qE '"error":[[:space:]]*"not_member"'; then removed=1; break fi sleep 3 diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt index 1174e9910..56f522d6a 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroup.kt @@ -133,6 +133,18 @@ class MlsGroup private constructor( val leafIndex: Int get() = myLeafIndex val extensions: List get() = groupContext.extensions + /** + * True while our own leaf is still live in the tree. + * + * After `processCommit` applies a Remove proposal that targets us, the + * group is intentionally kept in the manager so callers can inspect the + * final tree state — but our leaf is null and `leafCount` may have + * shrunk past `myLeafIndex`. Either condition means we can't propose, + * commit, encrypt, or decrypt any further; surface that here so callers + * get a clean answer instead of poking at private tree internals. + */ + fun isLocalMember(): Boolean = myLeafIndex < tree.leafCount && tree.getLeaf(myLeafIndex) != null + // --- Marmot admin helpers (MIP-01 / MIP-03) --- /** Raw BasicCredential identity bytes of the member at the given leaf, or null. */ @@ -1272,6 +1284,26 @@ class MlsGroup private constructor( newLeavesInCommit.add(applyProposalAdd(add)) } + // If the proposals just removed *us*, there is no path-decrypt to do + // and no confirmation_tag to verify against our (now bogus) commit + // secret — we have no valid leaf and our copy of `commit_secret` + // would be all-zeros, so the keyschedule would diverge from every + // remaining member's. Without this short-circuit, the path-decrypt + // block calls `BinaryTree.directPath(myLeafIndex, tree.leafCount)` + // with an out-of-range index and OOMs the JVM (interop test 14). + // + // The proposals have already mutated the tree; preserve those + // mutations on return so the outer state machine knows we are out + // of the group. `pendingProposals` are cleared because they were + // tied to the previous epoch. + val removedSelf = + myLeafIndex >= tree.leafCount || tree.getLeaf(myLeafIndex) == null + if (removedSelf) { + pendingProposals.clear() + sentKeys.clear() + return + } + // Process UpdatePath var commitSecret = ByteArray(MlsCryptoProvider.HASH_OUTPUT_LENGTH) if (commit.updatePath != null) { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt index 500d43a3d..7c6d5cb09 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt @@ -158,9 +158,18 @@ class MlsGroupManager( fun activeGroupIds(): Set = groups.keys.toSet() /** - * Check if we are a member of the given group. + * Check if we are a (live) member of the given group. + * + * After processing a commit that removes us, the group entry stays in + * [groups] so callers can still inspect the post-Remove tree, but our + * own leaf has been set to null and the leaf count may have shrunk + * past `myLeafIndex`. Either condition means we cannot encrypt any + * more messages, propose anything, or decrypt future epochs — i.e. + * we are functionally not a member, and surface as such here so + * callers (`amy marmot group show`, the inbound processor) get a + * clean `not_member` answer instead of a misleading `true`. */ - fun isMember(nostrGroupId: HexKey): Boolean = groups.containsKey(nostrGroupId) + fun isMember(nostrGroupId: HexKey): Boolean = groups[nostrGroupId]?.isLocalMember() ?: false // --- Group Creation --- diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt index 2bfa316fd..a95ce22fe 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/tree/BinaryTree.kt @@ -88,34 +88,40 @@ object BinaryTree { /** * Parent of a node in a tree with [nodeCount] total nodes. * Uses the left-balanced tree parent formula per RFC 9420 Appendix C. + * + * The root and any index ≥ [nodeCount] have no defined parent — the + * left-balanced formula keeps walking up through virtual parents that + * never come back into range, integer-overflows after ~31 doublings, + * and either returns garbage or OOMs the call site that's storing the + * result. Both modes are signs of a tree-accounting bug upstream + * (e.g., calling directPath with a leafIndex past the post-Remove + * leafCount); fail loudly here so callers can't silently corrupt + * the rest of the commit-processing pipeline. */ fun parent( nodeIndex: Int, nodeCount: Int, ): Int { - // For left-balanced trees with non-power-of-2 leaves, - // the parent might be beyond nodeCount. In that case, - // the node's parent is the next valid ancestor. - val k = level(nodeIndex) - val b = (nodeIndex shr (k + 1)) and 1 - val p = - if (b == 0) { - nodeIndex + (1 shl k) - } else { - nodeIndex - (1 shl k) - } - - return if (p < nodeCount) { - p - } else { - // Parent is out of range. This node is at the rightmost - // edge of a non-full tree. Walk up through virtual parents - // until we find one within range. - parentInRange(p, nodeCount) + require(nodeCount > 0) { "nodeCount must be positive, got $nodeCount" } + require(nodeIndex in 0 until nodeCount) { + "nodeIndex $nodeIndex out of range [0, $nodeCount)" } + // The structural root of a left-balanced tree with `leafCount` + // leaves sits at (1 << ceil(log2(leafCount))) - 1, derivable from + // nodeCount via leafCount = (nodeCount + 1) / 2. + val leafCount = (nodeCount + 1) / 2 + require(nodeIndex != root(leafCount)) { + "nodeIndex $nodeIndex is the root of a $leafCount-leaf tree and has no parent" + } + return parentRec(nodeIndex, nodeCount) } - private fun parentInRange( + /** + * Inner walker that assumes [nodeIndex] has at least one valid parent + * within [nodeCount]. Public callers go through [parent] which performs + * the bounds check above. + */ + private fun parentRec( nodeIndex: Int, nodeCount: Int, ): Int { @@ -127,7 +133,7 @@ object BinaryTree { } else { nodeIndex - (1 shl k) } - return if (p < nodeCount) p else parentInRange(p, nodeCount) + return if (p < nodeCount) p else parentRec(p, nodeCount) } /** Root node index for a tree with [leafCount] leaves */ @@ -141,11 +147,20 @@ object BinaryTree { /** * Direct path from a leaf to the root (excluding the leaf itself). * These are the parent nodes along the path from leaf to root. + * + * [leafIndex] must be in `[0, leafCount)`. Calling with `leafIndex >= + * leafCount` is an upstream bug — most often a stale local index that + * was valid before the tree shrank under a Remove proposal — and used + * to OOM here because `parent()` would loop on the out-of-range start. */ fun directPath( leafIndex: Int, leafCount: Int, ): List { + require(leafCount > 0) { "leafCount must be positive, got $leafCount" } + require(leafIndex in 0 until leafCount) { + "leafIndex $leafIndex out of range [0, $leafCount)" + } val nodeIdx = leafToNode(leafIndex) val n = nodeCount(leafCount) val rootIdx = root(leafCount) @@ -162,11 +177,18 @@ object BinaryTree { /** * Copath of a leaf: the siblings of each node on the direct path. * The copath determines which nodes need to receive encrypted path secrets. + * + * Same range contract as [directPath]: `leafIndex` must be in + * `[0, leafCount)`. */ fun copath( leafIndex: Int, leafCount: Int, ): List { + require(leafCount > 0) { "leafCount must be positive, got $leafCount" } + require(leafIndex in 0 until leafCount) { + "leafIndex $leafIndex out of range [0, $leafCount)" + } val nodeIdx = leafToNode(leafIndex) val n = nodeCount(leafCount) val rootIdx = root(leafCount) diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt index f7b517312..95923a6c3 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/mls/BinaryTreeTest.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.marmot.mls import com.vitorpamplona.quartz.marmot.mls.tree.BinaryTree import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFails import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -160,4 +161,121 @@ class BinaryTreeTest { // Subtree of leaf 0 is just [0] assertEquals(listOf(0), BinaryTree.subtreeLeaves(0, 4)) } + + // ----- Non-power-of-2 tree shapes ---------------------------------------- + // + // RFC 9420 Appendix C only worked example is the 4-leaf tree, but every + // group with ≠ a power-of-2 members exercises the parentInRange branch in + // BinaryTree.parent. Those branches were entirely uncovered before, and a + // bug there (infinite recursion / infinite loop in directPath) is what + // OOM'd amy when wn removed her in marmot-interop test 14. + + @Test + fun testRoot_nonPowerOfTwo() { + // root = (1 << ceil(log2(n))) - 1 + assertEquals(3, BinaryTree.root(3)) // ceil(log2(3))=2, 2^2-1 = 3 + assertEquals(7, BinaryTree.root(5)) // ceil(log2(5))=3, 2^3-1 = 7 + assertEquals(7, BinaryTree.root(6)) + assertEquals(7, BinaryTree.root(7)) + assertEquals(15, BinaryTree.root(9)) + } + + @Test + fun testDirectPath3Leaves() { + // 3 leaves, n=5, root=3: + // 3 + // / \ + // 1 4 + // / \ \ + // 0 2 (leaf 2 sits at node 4; node 5 doesn't exist) + // Node 4 is a leaf node here because the tree is not full — its + // "parent slot" 5 would be ≥ nodeCount and is collapsed away by + // parentInRange. + assertEquals(listOf(1, 3), BinaryTree.directPath(0, 3)) + assertEquals(listOf(1, 3), BinaryTree.directPath(1, 3)) + assertEquals(listOf(3), BinaryTree.directPath(2, 3)) + } + + @Test + fun testDirectPath5Leaves() { + // 5 leaves, n=9, root=7: + // 7 + // / \ + // 3 8 ← leaf 4 collapsed up + // / \ + // 1 5 + // / \ / \ + // 0 2 4 6 + assertEquals(listOf(1, 3, 7), BinaryTree.directPath(0, 5)) + assertEquals(listOf(1, 3, 7), BinaryTree.directPath(1, 5)) + assertEquals(listOf(5, 3, 7), BinaryTree.directPath(2, 5)) + assertEquals(listOf(5, 3, 7), BinaryTree.directPath(3, 5)) + assertEquals(listOf(7), BinaryTree.directPath(4, 5)) + } + + @Test + fun testDirectPathTerminatesForAllLeafCountsUpTo32() { + // Property: every valid leaf in any tree size 1..32 has a directPath + // that ends at root(leafCount) and has length == log2-ish. We don't + // assert exact lengths — we just want to catch any future regression + // where some (leafCount, leafIndex) triggers the parent() infinite + // loop. Each call is wrapped in a generous timeout via the kotlin + // test runner (default test timeout is fine: a non-terminating call + // would OOM long before any test deadline). + for (leafCount in 1..32) { + val rootIdx = BinaryTree.root(leafCount) + for (leafIndex in 0 until leafCount) { + val dp = BinaryTree.directPath(leafIndex, leafCount) + if (leafCount == 1) { + assertTrue(dp.isEmpty(), "leafCount=1 has no path") + } else { + assertEquals(rootIdx, dp.last(), "directPath($leafIndex, $leafCount) should end at root") + // copath must align with the directPath one-for-one + assertEquals(dp.size, BinaryTree.copath(leafIndex, leafCount).size) + } + } + } + } + + // ----- Out-of-range inputs ----------------------------------------------- + // + // Marmot-interop test 14 fails with a Java OOM because amy calls + // `BinaryTree.directPath(myLeafIndex, tree.leafCount)` with `myLeafIndex + // == tree.leafCount` (her own leaf was just removed and the tree shrank + // past it). `parent()` then walks node indices ≥ nodeCount forever and + // the result list explodes. + // + // These tests pin down that boundary so the failure becomes a clean + // IllegalArgumentException instead of an OOM, AND so the MLS code path + // that hits it has a deterministic regression. + + @Test + fun testDirectPathRejectsOutOfRangeLeafIndex() { + // leafIndex == leafCount — the exact shape the post-Remove path hits. + assertFails { BinaryTree.directPath(2, 2) } + // leafIndex > leafCount. + assertFails { BinaryTree.directPath(5, 3) } + } + + @Test + fun testDirectPathRejectsNegativeLeafIndex() { + assertFails { BinaryTree.directPath(-1, 4) } + } + + @Test + fun testCopathRejectsOutOfRangeLeafIndex() { + assertFails { BinaryTree.copath(2, 2) } + assertFails { BinaryTree.copath(-1, 4) } + } + + @Test + fun testParentRejectsRootOrAbove() { + // The root has no parent. Asking anyway is the symptom of a tree + // accounting bug — surface it loudly instead of looping. + val n = BinaryTree.nodeCount(4) + assertFails { BinaryTree.parent(BinaryTree.root(4), n) } + // Index strictly above nodeCount also has no defined parent. + assertFails { BinaryTree.parent(n, n) } + assertFails { BinaryTree.parent(n + 5, n) } + } }