diff --git a/src/GameCult.Networking/CultNetSelectionEvaluator.cs b/src/GameCult.Networking/CultNetSelectionEvaluator.cs index b48fb38a..298eaae2 100644 --- a/src/GameCult.Networking/CultNetSelectionEvaluator.cs +++ b/src/GameCult.Networking/CultNetSelectionEvaluator.cs @@ -174,9 +174,19 @@ namespace GameCult.Networking // to gate it - a public entry point that answers an unvalidated selection does not exist. selection.Validate(registry.AllDescriptors.ToArray()); - var byKey = new Dictionary(StringComparer.Ordinal); + // R-V: a row's identity is (schemaId, recordKey), never the record key alone. A bare-key + // dictionary here silently let one schema's row displace another's at the same key (last + // writer wins), so a hop resolving "the row at this key" picked whichever schema happened + // to insert last instead of the schema the reference/citation actually declares. Every key + // that appears on more than one schema's row is kept, not collapsed, so hop resolution can + // still tell them apart. + var byKey = new Dictionary>(StringComparer.Ordinal); foreach (var row in allRows) - byKey[row.Key.Value] = row; + { + if (!byKey.TryGetValue(row.Key.Value, out var rowsAtKey)) + byKey[row.Key.Value] = rowsAtKey = new List(); + rowsAtKey.Add(row); + } var candidates = allRows.Where(row => MatchesSchemaKeysFields(row.Descriptor, row.Key, row.Document, selection)); @@ -187,9 +197,9 @@ namespace GameCult.Networking var citedEdges = new List(); if (selection.Cited != null) { - var incoming = BuildIncomingIndex(registry, byKey, selection.Cited.Role, citedEdges); + var incoming = BuildIncomingIndex(registry, allRows, byKey, selection.Cited.Role, citedEdges); var exists = selection.Cited.Exists; - candidates = candidates.Where(row => incoming.Contains(row.Key.Value) == exists); + candidates = candidates.Where(row => incoming.Contains((row.Descriptor.SchemaId, row.Key.Value)) == exists); } var citesEdges = new List(); @@ -283,13 +293,17 @@ namespace GameCult.Networking // answering every page) reports edges against its whole matched set, not one 200-row page. public static EdgeMatch[] EdgesFor(IReadOnlyList rows, FullEvaluation full) { - var pageRowIndex = new Dictionary(StringComparer.Ordinal); + // R-V: the anchor lookup is keyed by (schemaId, recordKey), not the bare key - two schemas' + // rows sharing one record key can both legitimately be on the same page now that the hop + // resolves rows by identity, and a bare-key index would let one silently steal the other's + // edges. + var pageRowIndex = new Dictionary<(string SchemaId, string RecordKey), int>(); for (var i = 0; i < rows.Count; i++) - pageRowIndex[rows[i].Key.Value] = i; + pageRowIndex[(rows[i].Descriptor.SchemaId, rows[i].Key.Value)] = i; return full.Edges - .Where(pair => pageRowIndex.ContainsKey(pair.Anchor.Key.Value)) - .OrderBy(pair => pageRowIndex[pair.Anchor.Key.Value]) + .Where(pair => pageRowIndex.ContainsKey((pair.Anchor.Descriptor.SchemaId, pair.Anchor.Key.Value))) + .OrderBy(pair => pageRowIndex[(pair.Anchor.Descriptor.SchemaId, pair.Anchor.Key.Value)]) .ThenBy(pair => pair.Edge.From.Descriptor.SchemaId, CultNetCodePointComparer.Instance) .ThenBy(pair => pair.Edge.From.Key.Value, CultNetCodePointComparer.Instance) .ThenBy(pair => pair.Edge.Role, CultNetCodePointComparer.Instance) @@ -324,39 +338,74 @@ namespace GameCult.Networking // D9: a reference's target set is every registered leaf assignable to its declared target type. // A stored edge naming a row whose schema is outside that set refuses the selection (S18). - // R-T(b): this runs for every edge, including one whose member carries no declared target type - // (CultDocumentRegistry.PersistedMember.TargetType is null only for a shape the cache could not - // infer a target from - it no longer special-cases that away here without checking anything; - // typeof(object) is every registered leaf, which is the correct target set for a genuinely - // unconstrained reference and keeps this the one path, not a skip plus a duplicate path. - private static void EnsureWithinDeclaredTarget(CultDocumentRegistry registry, Type? targetType, Row from, string role, Row to) + // S-8: targetType is never null here - D11 (CultCache's DiscoverMembers) refuses every reference + // shape whose target the cache could not infer (a bare ICultRecordRef, scalar or many) at + // registration, before a document carrying one can ever reach the evaluator. The old + // `targetType ?? typeof(object)` fallback had no fixture that could reach it and is deleted + // rather than kept as an unreachable branch. + // + // R-V: a reference's stored target is a bare record key - the wire carries no schema for it - + // so resolving "the row this edge names" means searching every row sharing that key (byKey) for + // the one whose schema falls inside the reference's declared leaf set, not grabbing whichever + // row a bare-key dictionary happened to keep. Two schemas legitimately sharing one record key + // resolve to two different rows depending on which reference declared which target (Soul's P2). + // + // R-W: this is called for every stored edge a reference member carries, unconditionally, before + // any citation- or role-specific filtering decides whether that edge is the one the caller + // asked about - a many-reference edge into a schema outside its declared target refuses the + // selection even when the caller's own key/role filter would otherwise have skipped past it + // without ever looking at it (Soul's P3). + private static bool TryResolveReferenceTarget( + CultDocumentRegistry registry, + IReadOnlyDictionary> byKey, + Type targetType, + Row from, + string role, + string recordKey, + out Row resolved) { - var leaves = registry.ResolveTargetLeaves(targetType ?? typeof(object)); - if (leaves.Any(leaf => leaf.SchemaId == to.Descriptor.SchemaId)) return; - throw new CultNetSelectionReferenceOutsideTargetException(from.Descriptor.SchemaId, from.Key.Value, role, to.Descriptor.SchemaId, to.Key.Value); + resolved = default; + if (!byKey.TryGetValue(recordKey, out var candidates) || candidates.Count == 0) + return false; // a dangling reference - no row at all exists at this key (R-T(b) style: not an error). + + var leaves = registry.ResolveTargetLeaves(targetType); + foreach (var candidate in candidates) + { + if (leaves.Any(leaf => leaf.SchemaId == candidate.Descriptor.SchemaId)) + { + resolved = candidate; + return true; + } + } + + // Every row sharing this record key falls outside the reference's declared target: refuse + // deterministically (by schema id, code-point order) rather than let insertion order decide + // which of them the refusal names (R-V: "both row orders give the same answer"). + var outside = candidates.OrderBy(c => c.Descriptor.SchemaId, CultNetCodePointComparer.Instance).First(); + throw new CultNetSelectionReferenceOutsideTargetException(from.Descriptor.SchemaId, from.Key.Value, role, outside.Descriptor.SchemaId, outside.Key.Value); } private static bool MatchesCitation( CultDocumentRegistry registry, - IReadOnlyDictionary byKey, + IReadOnlyDictionary> byKey, Row citer, CultNetCitation citation, List edgeSink) { - if (!byKey.TryGetValue(citation.Target.RecordKey, out var target)) - return false; - var found = false; foreach (var member in ReferenceMembers(citer.Descriptor, citation.Role)) { var role = member.IndexAlias ?? member.MemberName; foreach (var (targetKey, payload) in citer.Descriptor.ReferencesOf(citer.Document, role)) { + // R-W: resolve and check the edge's own declared target first, for every edge this + // reference carries - before the citation's key/schema decides whether this is the + // edge the caller asked about. + if (!TryResolveReferenceTarget(registry, byKey, member.TargetType!, citer, role, targetKey.Value, out var resolved)) + continue; + if (!string.Equals(targetKey.Value, citation.Target.RecordKey, StringComparison.Ordinal)) continue; - if (!byKey.TryGetValue(targetKey.Value, out var resolved)) - continue; - EnsureWithinDeclaredTarget(registry, member.TargetType, citer, role, resolved); // R-E: one schema-identity rule everywhere - a cites target goes through the alias // matcher, the same as `schemas`, instead of an exact CultDocumentDescriptor.SchemaId // compare that a schema alias or version string could never satisfy. @@ -370,24 +419,27 @@ namespace GameCult.Networking return found; } - private static HashSet BuildIncomingIndex( + private static HashSet<(string SchemaId, string RecordKey)> BuildIncomingIndex( CultDocumentRegistry registry, - IReadOnlyDictionary byKey, + IReadOnlyList allRows, + IReadOnlyDictionary> byKey, string role, List edgeSink) { - var incoming = new HashSet(StringComparer.Ordinal); - foreach (var citer in byKey.Values) + // R-V: iterate every row directly, not byKey.Values - a bare-key index collapses two + // schemas' rows sharing a key into one slot, which used to silently drop one of them from + // consideration as a citer entirely. + var incoming = new HashSet<(string, string)>(); + foreach (var citer in allRows) { foreach (var member in ReferenceMembers(citer.Descriptor, role)) { var memberRole = member.IndexAlias ?? member.MemberName; foreach (var (targetKey, payload) in citer.Descriptor.ReferencesOf(citer.Document, memberRole)) { - if (!byKey.TryGetValue(targetKey.Value, out var resolved)) + if (!TryResolveReferenceTarget(registry, byKey, member.TargetType!, citer, memberRole, targetKey.Value, out var resolved)) continue; - EnsureWithinDeclaredTarget(registry, member.TargetType, citer, memberRole, resolved); - incoming.Add(targetKey.Value); + incoming.Add((resolved.Descriptor.SchemaId, resolved.Key.Value)); edgeSink.Add(new EdgeMatch(citer, memberRole, resolved, payload)); } } @@ -488,9 +540,10 @@ namespace GameCult.Networking } /// - /// The opaque cursor: asOf, the last (ordinal, schemaId, recordKey), and an HMAC digest of the - /// selection with cursor and limit cleared, keyed under the answering process's - /// (R-O). Minted only by the answering server. + /// The opaque cursor: asOf, the last (ordinal, schemaId, recordKey), and an HMAC digest that covers + /// that same body as well as the selection (with cursor and limit cleared), keyed under the + /// answering process's (R-O/R-Y). Minted only by the + /// answering server. /// public readonly struct CultNetSelectionCursor { @@ -511,7 +564,7 @@ namespace GameCult.Networking public static string Mint(ulong asOf, CultNetSelectionEvaluator.Row lastRow, CultNetSelection selection, CultNetSelectionCursorKey key) { - var digest = ComputeDigest(selection, key); + var digest = ComputeDigest(asOf, lastRow.Ordinal, lastRow.Descriptor.SchemaId, lastRow.Key.Value, selection, key); // R-O: the cursor body is length-prefixed fields, not a delimiter-joined string - a record // key carrying any character at all, including whatever delimiter an earlier scheme would // have chosen (Soul found a key containing U+241F broke a delimiter split), round-trips. @@ -550,27 +603,46 @@ namespace GameCult.Networking /// /// Verifies this cursor's digest against under /// in fixed time (R-O): a cursor forged without the key, or minted under a different process's - /// key (a restart), does not verify. + /// key (a restart), does not verify. R-Y: the digest is recomputed over this cursor's own body + /// (, , , ) + /// as well as the selection, so a cursor whose body was rewritten after minting - the ordinal or + /// record key edited underneath a digest that was still legitimately minted for this selection - + /// no longer verifies either (Soul's P4: "forged cursor position, reused digest"). /// public bool VerifyDigest(CultNetSelection selection, CultNetSelectionCursorKey key) { - var expected = ComputeDigest(selection, key); + var expected = ComputeDigest(AsOf, Ordinal, SchemaId, RecordKey, selection, key); return CryptographicOperations.FixedTimeEquals( Encoding.UTF8.GetBytes(Digest), Encoding.UTF8.GetBytes(expected)); } /// - /// An HMAC-SHA256 digest, keyed under , of the selection with cursor and - /// limit cleared, so a cursor is bound both to the selection that minted it and to the process - /// that minted it (R-O). R-H: every string and list is length-prefixed, so no delimiter choice + /// An HMAC-SHA256 digest, keyed under , of the cursor's own body + /// (, , , + /// ) plus the selection with cursor and limit cleared, so a cursor is + /// bound to the exact position it was minted at as well as to the selection and the process that + /// minted it (R-O/R-Y). R-H: every string and list is length-prefixed, so no delimiter choice /// can make two different selections collide - a delimiter-joined form used to digest /// values ["a|b"] the same as ["a","b"], and keys ["a,b"] the same as ["a","b"]. /// - public static string ComputeDigest(CultNetSelection selection, CultNetSelectionCursorKey? key = null) + public static string ComputeDigest( + ulong asOf, + long ordinal, + string schemaId, + string recordKey, + CultNetSelection selection, + CultNetSelectionCursorKey? key = null) { key ??= CultNetSelectionCursorKey.ProcessDefault; var sb = new StringBuilder(); + // R-Y: the body is part of what the HMAC signs - without this, a forged cursor could keep a + // legitimately minted digest and rewrite the ordinal or record key underneath it, since + // nothing about the body itself was ever authenticated. + AppendString(sb, asOf.ToString(CultureInfo.InvariantCulture)); + AppendString(sb, ordinal.ToString(CultureInfo.InvariantCulture)); + AppendString(sb, schemaId); + AppendString(sb, recordKey); AppendList(sb, selection.Schemas); AppendList(sb, selection.Keys); diff --git a/tests/GameCult.Networking.Tests/CultNetSelectionFixBatchTests.cs b/tests/GameCult.Networking.Tests/CultNetSelectionFixBatchTests.cs index 04401aba..b45c072d 100644 --- a/tests/GameCult.Networking.Tests/CultNetSelectionFixBatchTests.cs +++ b/tests/GameCult.Networking.Tests/CultNetSelectionFixBatchTests.cs @@ -183,11 +183,15 @@ namespace GameCult.Networking.Tests { var keysOne = new CultNetSelection { Keys = new[] { "a,b" } }; var keysTwo = new CultNetSelection { Keys = new[] { "a", "b" } }; - Assert.That(CultNetSelectionCursor.ComputeDigest(keysOne), Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(keysTwo))); + Assert.That( + CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", keysOne), + Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", keysTwo))); var valuesOne = new CultNetSelection { Fields = new[] { new CultNetFieldPredicate { Index = "kind", Op = "any_of", Values = new[] { "a|b" } } } }; var valuesTwo = new CultNetSelection { Fields = new[] { new CultNetFieldPredicate { Index = "kind", Op = "any_of", Values = new[] { "a", "b" } } } }; - Assert.That(CultNetSelectionCursor.ComputeDigest(valuesOne), Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(valuesTwo))); + Assert.That( + CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", valuesOne), + Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", valuesTwo))); } // S5-DigestNoFields (fix batch 3 "Surviving mutants"): the digest reads selection.Fields - a @@ -201,8 +205,12 @@ namespace GameCult.Networking.Tests var withFieldB = new CultNetSelection { Fields = new[] { new CultNetFieldPredicate { Index = "kind", Op = "any_of", Values = new[] { "b" } } } }; var noFields = new CultNetSelection(); - Assert.That(CultNetSelectionCursor.ComputeDigest(withFieldA), Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(withFieldB))); - Assert.That(CultNetSelectionCursor.ComputeDigest(withFieldA), Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(noFields))); + Assert.That( + CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", withFieldA), + Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", withFieldB))); + Assert.That( + CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", withFieldA), + Is.Not.EqualTo(CultNetSelectionCursor.ComputeDigest(0, 0, "s", "k", noFields))); } // R-O (fix batch 3, F6 "cursors are keyed"): a cursor's digest is an HMAC under the answering @@ -254,6 +262,52 @@ namespace GameCult.Networking.Tests Assert.That(ex!.Code, Is.EqualTo("cursor_invalid")); } + // R-Y (fix batch 4, Soul's P4 "forged cursor position, reused digest"): before this, the HMAC + // covered only the selection, not the cursor's own body - so a forged cursor could keep a + // legitimately minted digest and rewrite the ordinal (or schemaId/recordKey) underneath it, and + // the server would page from wherever the forged position named instead of refusing it. + [Test] + public void Page_RefusesAForgedCursorThatReusesALegitimateDigestUnderARewrittenOrdinal() + { + var registry = Registry(); + var rows = Enumerable.Range(0, 3) + .Select(i => Row(registry, new CultNetSelectionEvaluatorTests.SelLeafA { Name = $"n{i}", Kind = "k", Mass = i }, $"k{i}", i)) + .ToArray(); + var selection = new CultNetSelection { Limit = 1 }; + var key = CultNetSelectionCursorKey.Random(); + + var first = CultNetSelectionEvaluator.Select(registry, rows, selection, asOf: 1, cursorKey: key); + Assert.That(first.NextCursor, Is.Not.Null); + var legitimate = CultNetSelectionCursor.Parse(first.NextCursor!); + + var forged = ForgeCursorBody(legitimate.AsOf, ordinal: 999, legitimate.SchemaId, legitimate.RecordKey, legitimate.Digest); + Assert.That(forged, Is.Not.EqualTo(first.NextCursor), "the forged cursor must actually differ from the legitimate one"); + + selection.Cursor = forged; + var ex = Assert.Throws( + () => CultNetSelectionEvaluator.Select(registry, rows, selection, asOf: 1, cursorKey: key)); + Assert.That(ex!.Code, Is.EqualTo("cursor_invalid")); + } + + /// + /// Builds a cursor string carrying an arbitrary body under a caller-supplied digest, the same + /// wire shape produces - used to simulate a forged + /// cursor that reuses a legitimately minted digest under a rewritten body (R-Y). + /// + private static string ForgeCursorBody(ulong asOf, long ordinal, string schemaId, string recordKey, string digest) + { + var body = new System.Text.StringBuilder(); + void AppendString(string value) => + body.Append(System.Text.Encoding.UTF8.GetByteCount(value)).Append(':').Append(value); + AppendString(asOf.ToString(System.Globalization.CultureInfo.InvariantCulture)); + AppendString(ordinal.ToString(System.Globalization.CultureInfo.InvariantCulture)); + AppendString(schemaId); + AppendString(recordKey); + AppendString(digest); + var bytes = System.Text.Encoding.UTF8.GetBytes(body.ToString()); + return Convert.ToBase64String(bytes).TrimEnd('=').Replace('+', '-').Replace('/', '_'); + } + // R-O: the cursor body is length-prefixed, not delimiter-joined, so a record key carrying any // character at all - including U+241F, which broke the pre-fix delimiter-split parse - round-trips. [Test] @@ -304,5 +358,84 @@ namespace GameCult.Networking.Tests Assert.That(evaluation.Rows.Select(r => r.Key.Value), Is.EqualTo(new[] { "big" })); } + + private static CultDocumentRegistry NarrowAndManyRegistry() => CultDocumentRegistry.ForTypes(new[] + { + typeof(CultNetSelectionEvaluatorTests.SelLeafA), + typeof(CultNetSelectionEvaluatorTests.SelLeafB), + typeof(CultNetSelectionEvaluatorTests.SelCiterNarrow), + typeof(CultNetSelectionEvaluatorTests.SelCiterManyUntyped) + }); + + // R-V (S-1, Soul's P2 "two schemas at one key"): a row's identity is (schemaId, recordKey), not + // the record key alone. SelLeafA and SelLeafB rows both live at key "k"; SelCiterNarrow's + // NarrowRef is declared to SelLeafA only, so it must always resolve to the SelLeafA row at "k", + // never the SelLeafB one - regardless of which of the two same-keyed rows a bare-key dictionary + // would have kept. Before this, byKey was a bare-key dictionary that kept only one of them (last + // writer wins), so which row NarrowRef resolved to - and therefore whether the selection matched + // or refused reference_outside_target - depended on row insertion order. + [TestCase(false)] + [TestCase(true)] + public void Evaluator_CitedResolvesARowSharingItsKeyWithAnotherSchema_ByDeclaredTargetNotInsertionOrder(bool reverseRowOrder) + { + var registry = NarrowAndManyRegistry(); + var leafA = Row(registry, new CultNetSelectionEvaluatorTests.SelLeafA { Name = "a", Kind = "k", Mass = 1 }, "k", 0); + var leafB = Row(registry, new CultNetSelectionEvaluatorTests.SelLeafB { Name = "b", Kind = "k", Mass = 1 }, "k", 1); + var citer = Row(registry, new CultNetSelectionEvaluatorTests.SelCiterNarrow + { + Name = "citer", + NarrowRef = new CultRecordRef(new CultRecordKey("k")) + }, "citer", 2); + + var rows = reverseRowOrder + ? new[] { citer, leafB, leafA } + : new[] { citer, leafA, leafB }; + + var selection = new CultNetSelection { Cited = new CultNetIncoming { Role = "NarrowRef", Exists = true } }; + var evaluation = CultNetSelectionEvaluator.Select(registry, rows, selection, asOf: 1); + + // Only the SelLeafA row at "k" is cited - SelLeafB's row at the same key is a different + // identity and must not match, in either row order. + Assert.That(evaluation.Rows.Select(r => (r.Descriptor.SchemaId, r.Key.Value)), + Is.EqualTo(new[] { (registry.GetRequired().SchemaId, "k") })); + } + + // R-W (S-2, Soul's P3): EnsureWithinDeclaredTarget's equivalent check must run for every edge a + // many-reference carries, before the citation's own key filters that edge out - not only for the + // edge the citation happens to be asking about. SelCiterManyUntyped.ManyRefs is (by inference, + // R-T(b)) declared to SelLeafA alone; one element points at a SelLeafA row (in-target) and a + // second at a SelLeafB row (out-of-target). Querying `cites` for a record key neither element + // carries must still refuse: the out-of-target edge is wrong regardless of what the caller asked. + [Test] + public void Evaluator_CitesRefusesAnOutOfTargetEdgeEvenWhenTheQueriedKeyMatchesNeitherEdge() + { + var registry = NarrowAndManyRegistry(); + var leafA = Row(registry, new CultNetSelectionEvaluatorTests.SelLeafA { Name = "a", Kind = "k", Mass = 1 }, "a", 0); + var leafB = Row(registry, new CultNetSelectionEvaluatorTests.SelLeafB { Name = "b", Kind = "k", Mass = 1 }, "b", 1); + var citer = Row(registry, new CultNetSelectionEvaluatorTests.SelCiterManyUntyped + { + Name = "citer", + ManyRefs = new[] + { + new CultRecordRef(new CultRecordKey("a")), + new CultRecordRef(new CultRecordKey("b")) + } + }, "citer", 2); + var rows = new[] { leafA, leafB, citer }; + + var selection = new CultNetSelection + { + Cites = new CultNetCitation + { + Target = new CultNetRecordRef { SchemaId = registry.GetRequired().SchemaId, RecordKey = "does-not-exist" }, + Role = "ManyRefs" + } + }; + + var ex = Assert.Throws( + () => CultNetSelectionEvaluator.Select(registry, rows, selection, asOf: 1)); + Assert.That(ex!.ToSchemaId, Is.EqualTo(registry.GetRequired().SchemaId)); + Assert.That(ex.ToKey, Is.EqualTo("b")); + } } }