From 143e4f7db25f3da43e1f5bc6a5f318a1f36a1582 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Thu, 15 Oct 2026 01:22:47 +0200 Subject: [PATCH] Element ids: a non-public [Key] member on the way to an object list is refused at registration MessagePack writes a non-public [Key] member only under AllowPrivate, and the element-id walk reads public members, so the ids such a list holds could never be checked. Graph entries carry Hidden; a hidden list or a hidden edge whose child reaches adds one problem and nothing to the id path. Co-Authored-By: Claude Sonnet 5.5 --- src/GameCult.Caching/CultElementIds.cs | 63 ++++++++++++++----- .../ElementIdEngineTests.cs | 27 ++++++++ 2 files changed, 74 insertions(+), 16 deletions(-) diff --git a/src/GameCult.Caching/CultElementIds.cs b/src/GameCult.Caching/CultElementIds.cs index adf8db01..fcbf8079 100644 --- a/src/GameCult.Caching/CultElementIds.cs +++ b/src/GameCult.Caching/CultElementIds.cs @@ -109,8 +109,10 @@ namespace GameCult.Caching // can sit in a cycle, so the path is a fixed point over these edges and not a stack. private sealed class Graph { - public List<(Type Owner, MemberInfo? Member, Type Child, Type[] Through)> Edges { get; } = new(); - public List<(Type Owner, MemberInfo Member, Type Element, List Concrete, Type[] Through)> Lists { get; } = new(); + // Hidden is true when the member that starts the entry is non-public: MessagePack writes it only under AllowPrivate, + // and the walk reads public members only. + public List<(Type Owner, MemberInfo? Member, Type Child, Type[] Through, bool Hidden)> Edges { get; } = new(); + public List<(Type Owner, MemberInfo Member, Type Element, List Concrete, Type[] Through, bool Hidden)> Lists { get; } = new(); } private static readonly ConcurrentDictionary IdCapable = new(); @@ -133,15 +135,17 @@ namespace GameCult.Caching if (!seen.Add(type)) return; foreach (var (member, memberType) in KeyedMembers(type)) - VisitMemberType(documentType, type, member, memberType, Array.Empty(), seen, survey, graph); + VisitMemberType(documentType, type, member, memberType, Array.Empty(), false, seen, survey, graph); + foreach (var (member, memberType) in NonPublicKeyedMembers(type)) + VisitMemberType(documentType, type, member, memberType, Array.Empty(), true, seen, survey, graph); foreach (var union in type.GetCustomAttributes(true)) { - graph.Edges.Add((type, null, union.SubType, Array.Empty())); + graph.Edges.Add((type, null, union.SubType, Array.Empty(), false)); VisitObject(documentType, union.SubType, seen, survey, graph); } } - private static void VisitMemberType(Type documentType, Type owner, MemberInfo member, Type memberType, Type[] through, HashSet seen, Survey survey, Graph graph) + private static void VisitMemberType(Type documentType, Type owner, MemberInfo member, Type memberType, Type[] through, bool hidden, HashSet seen, Survey survey, Graph graph) { memberType = Nullable.GetUnderlyingType(memberType) ?? memberType; var (kind, inner) = Classify(memberType); @@ -150,14 +154,15 @@ namespace GameCult.Caching case Kind.Leaf: return; case Kind.Map: - VisitMemberType(documentType, owner, member, inner, through.Append(memberType).ToArray(), seen, survey, graph); + VisitMemberType(documentType, owner, member, inner, through.Append(memberType).ToArray(), hidden, seen, survey, graph); return; case Kind.Sequence when IsElement(inner): var concretes = ConcreteTypes(inner).ToList(); - graph.Lists.Add((owner, member, inner, concretes, through.Append(memberType).ToArray())); + graph.Lists.Add((owner, member, inner, concretes, through.Append(memberType).ToArray(), hidden)); foreach (var concrete in concretes) { - if (!survey.Judged.Add(concrete)) + // A hidden list is refused as a member (Settle), not once per element type. + if (hidden || !survey.Judged.Add(concrete)) continue; var problem = IsElement(concrete) ? IdProblem(concrete) : UnkeyedElementProblem; if (problem != null) @@ -167,10 +172,10 @@ namespace GameCult.Caching VisitObject(documentType, inner, seen, survey, graph); return; case Kind.Sequence: - VisitMemberType(documentType, owner, member, inner, through.Append(memberType).ToArray(), seen, survey, graph); + VisitMemberType(documentType, owner, member, inner, through.Append(memberType).ToArray(), hidden, seen, survey, graph); return; default: - graph.Edges.Add((owner, member, memberType, through)); + graph.Edges.Add((owner, member, memberType, through, hidden)); VisitObject(documentType, memberType, seen, survey, graph); return; } @@ -184,15 +189,16 @@ namespace GameCult.Caching // that does not write the [Key] layout the id lives in. private static void Settle(Type documentType, Survey survey, Graph graph) { - var reaching = new HashSet(graph.Lists.Select(list => list.Owner)); - var members = new List(graph.Lists.Select(list => list.Member)); - var through = new List(graph.Lists.SelectMany(list => list.Through)); + var lists = graph.Lists.Where(list => !list.Hidden).ToArray(); + var reaching = new HashSet(lists.Select(list => list.Owner)); + var members = new List(lists.Select(list => list.Member)); + var through = new List(lists.SelectMany(list => list.Through)); for (var grew = true; grew;) { grew = false; - foreach (var (owner, member, child, via) in graph.Edges) + foreach (var (owner, member, child, via, hidden) in graph.Edges) { - if (!reaching.Contains(child)) + if (hidden || !reaching.Contains(child)) continue; if (member != null && !members.Contains(member)) members.Add(member); @@ -212,7 +218,7 @@ namespace GameCult.Caching Add(type); foreach (var type in through) Add(type); - foreach (var list in graph.Lists) + foreach (var list in lists) { Add(list.Element); foreach (var concrete in list.Concrete) @@ -231,8 +237,18 @@ namespace GameCult.Caching if (Declares(member, typeof(MessagePackFormatterAttribute))) survey.Problems.Add($"Cult document {documentType.Name}: {member.DeclaringType!.Name}.{member.Name} leads to a list whose elements carry ids and declares [MessagePackFormatter], so its element ids would never reach disk; remove the attribute."); } + + // The fixed point has settled which types reach a list through public members. A non-public [Key] member that starts + // a list, or leads to a type that reaches one, is refused: nothing reads or checks the ids it holds. + foreach (var list in graph.Lists.Where(list => list.Hidden)) + survey.Problems.Add(HiddenProblem(documentType, list.Owner, list.Member)); + foreach (var edge in graph.Edges.Where(edge => edge.Hidden && reaching.Contains(edge.Child))) + survey.Problems.Add(HiddenProblem(documentType, edge.Owner, edge.Member!)); } + private static string HiddenProblem(Type documentType, Type owner, MemberInfo member) => + $"Cult document {documentType.Name}: {owner.Name}.{member.Name} is a non-public [Key] member on the way to a list whose elements carry ids; MessagePack writes it only under AllowPrivate and the element-id walk reads public members, so its ids could never be checked. Make it public, or keep lists out of it."; + // The types an element can actually be: the element type itself when it can be instantiated, and every union subtype // under it. Shapes are built from these, so registration judges exactly what minting will meet. private static IEnumerable ConcreteTypes(Type element) @@ -524,6 +540,21 @@ namespace GameCult.Caching // override of a base virtual member would not see its base's attribute. private static bool Declares(MemberInfo member, Type attribute) => Attribute.IsDefined(member, attribute, inherit: true); + // [Key] members MessagePack writes only under AllowPrivate: non-public fields and properties, up the base chain. + private static IEnumerable<(MemberInfo Member, Type Type)> NonPublicKeyedMembers(Type type) + { + for (var level = type; level != null && level != typeof(object); level = level.BaseType) + { + const BindingFlags flags = BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.DeclaredOnly; + foreach (var member in level.GetFields(flags).Cast() + .Concat(level.GetProperties(flags).Where(property => property.GetIndexParameters().Length == 0))) + { + if (KeyOf(member) != null) + yield return (member, TypeOf(member)); + } + } + } + private static int? KeyOf(MemberInfo member) => member.GetCustomAttribute(true)?.IntKey; private static Type TypeOf(MemberInfo member) => member is FieldInfo field ? field.FieldType : ((PropertyInfo)member).PropertyType; diff --git a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs index 608ef47d..4e9bb460 100644 --- a/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs +++ b/tests/GameCult.Caching.Tests/ElementIdEngineTests.cs @@ -452,6 +452,24 @@ namespace GameCult.Caching.Tests Assert.That(Assign(arm, "r", deterministic: false), Is.EqualTo(1)); } + // MessagePack writes a non-public [Key] member only under AllowPrivate, and the element-id walk reads public members, so + // an object list a non-public member leads to could never be checked. It is refused; a non-public member that reaches no + // object list is nobody's business. + [Test] + public void ANonPublicKeyedMemberOnTheWayToAnObjectListIsRefused() + { + foreach (var (type, member) in new[] + { + (typeof(HHiddenList), "Stash"), (typeof(HHiddenOwner), "stash"), (typeof(HOptHidden), "stash"), (typeof(HHiddenFromBase), "stash"), + }) + Assert.That(Problems(type).Single(), Does.Contain(member).And.Contain("non-public"), type.Name); + + Assert.That(Problems(typeof(HHiddenLeaf)), Is.Empty); + Assert.That(CultElementIds.Inspect(typeof(HHiddenLeaf)).IdPath, Is.EquivalentTo(new[] { typeof(HHiddenLeaf), typeof(List), typeof(EIded) }), + "a hidden member adds nothing to the id path"); + Assert.That(CultElementIds.Inspect(typeof(HHiddenList)).IdPath, Is.Empty); + } + // ---- shapes ---- [MessagePackObject] public class EBare { [Key(0)] public string X { get; set; } = ""; } [MessagePackObject] public class EIded { [Key(0)] public string X { get; set; } = ""; [Key(1)] [CultElementId] public string Id { get; set; } = ""; } @@ -625,6 +643,15 @@ namespace GameCult.Caching.Tests [MessagePackObject] public class HBase { [Key(0)] public BaseC B { get; set; } = new(); } [MessagePackObject] public class HObject { [Key(0)] public object? O { get; set; } } +#pragma warning disable CS0169, CS0414, CS0649 + [MessagePackObject] public class HHiddenList { [Key(0)] private List Stash { get; set; } = new(); } + [MessagePackObject] public class HHiddenOwner { [Key(0)] private HHasIds stash = new(); } + [MessagePackObject(AllowPrivate = true)] public class HOptHidden { [Key(0)] private List stash = new(); } + [MessagePackObject] public class HHiddenBase { [Key(0)] private List stash = new(); } + [MessagePackObject] public class HHiddenFromBase : HHiddenBase { [Key(1)] public int N { get; set; } } + [MessagePackObject] public class HHiddenLeaf { [Key(0)] public List Items { get; set; } = new(); [Key(1)] private int secret = 7; } +#pragma warning restore CS0169, CS0414, CS0649 + [MessagePackObject] public class EDerivedText { [Key(0)] public string? Name { get; set; } [Key(1)] [CultElementId(nameof(Name))] public string Id { get; set; } = ""; } [MessagePackObject] public class HDerivedText { [Key(0)] public List Items { get; set; } = new(); }