From 984c5c25207afb33d8172fe8fb43dabbf7afda99 Mon Sep 17 00:00:00 2001 From: "Jeremy D. Miller" Date: Thu, 30 Jul 2026 06:30:31 -0500 Subject: [PATCH] fix: never let one property getter lose a whole OptionsDescription (#590) An OptionsDescription is a diagnostic view built by calling arbitrary property getters on somebody else's configuration object, so any one of them can throw -- and losing the entire description over one bad property is a terrible trade. Reported from the field as JasperFx/wolverine#3740, where AzureServiceBusTransport.HostName threw a NullReferenceException for credential-based connections and a monitored service could consequently never build its ServiceCapabilities snapshot at all. - Skip set-only properties and indexers, neither of which can be read via PropertyInfo.GetValue(subject) (ArgumentException / TargetParameterCountException). Wolverine.Pulsar's PulsarTransport has a this[Uri] indexer, which was enough to make it undescribable. - Catch whatever a getter throws -- including from the [ChildDescription], [DescribeAsStringArray] and [DescribeAsConfigurationState] branches -- and record OptionsValue.Unreadable(...) in its place. - Report the exception TYPE only, never the message: descriptions get shipped to monitoring consoles and work hard to keep secrets out, and exception messages habitually quote the offending configuration value. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RainvDHdde5JUAiGEPEgWw --- .../Descriptors/OptionsDescriptionTests.cs | 81 ++++++++++++++ .../Descriptors/OptionsDescription.cs | 105 +++++++++++------- src/JasperFx/Descriptors/OptionsValue.cs | 48 +++++++- 3 files changed, 191 insertions(+), 43 deletions(-) diff --git a/src/CoreTests/Descriptors/OptionsDescriptionTests.cs b/src/CoreTests/Descriptors/OptionsDescriptionTests.cs index c7e0527e..5afd94ea 100644 --- a/src/CoreTests/Descriptors/OptionsDescriptionTests.cs +++ b/src/CoreTests/Descriptors/OptionsDescriptionTests.cs @@ -61,6 +61,87 @@ public void children_and_sets_round_trip_through_default_json_options() } } +public class describing_awkward_properties +{ + // Regression for the report in JasperFx/wolverine#3740: AzureServiceBusTransport.HostName threw a + // NullReferenceException for credential-based connections, and because an OptionsDescription reads every + // public property reflectively, that single getter took out the whole Wolverine ServiceCapabilities + // snapshot -- so the monitoring console got nothing at all for the service, permanently. + [Fact] + public void a_throwing_getter_does_not_lose_the_whole_description() + { + var description = new OptionsDescription(new AwkwardObject()); + + description.PropertyFor(nameof(AwkwardObject.Name))!.Value.ShouldBe("Rogue"); + description.PropertyFor(nameof(AwkwardObject.Tolerable))!.Value.ShouldBe("42"); + + var explosive = description.PropertyFor(nameof(AwkwardObject.Explosive)); + explosive.ShouldNotBeNull(); + explosive.Value.ShouldStartWith(OptionsValue.UnreadablePrefix); + explosive.Value.ShouldContain(nameof(NullReferenceException)); + explosive.RawValue.ShouldBeNull(); + } + + [Fact] + public void the_exception_message_is_never_included() + { + // Descriptions are shipped to monitoring tools and go to some lengths to keep secrets out; exception + // messages love to quote the offending configuration value + var description = new OptionsDescription(new AwkwardObject()); + description.PropertyFor(nameof(AwkwardObject.Explosive))!.Value + .ShouldNotContain(AwkwardObject.SecretishMessage); + } + + [Fact] + public void skips_indexers_and_set_only_properties() + { + var description = new OptionsDescription(new AwkwardObject()); + + // "Item" is what an indexer is called in reflection -- reading one with no arguments throws + description.Properties.ShouldNotContain(x => x.Name == "Item"); + description.Properties.ShouldNotContain(x => x.Name == nameof(AwkwardObject.WriteOnly)); + } + + [Fact] + public void a_throwing_child_description_does_not_lose_the_whole_description() + { + var description = new OptionsDescription(new AwkwardParent()); + + description.PropertyFor(nameof(AwkwardParent.Name))!.Value.ShouldBe("Storm"); + description.Children.ShouldNotContainKey(nameof(AwkwardParent.Child)); + description.PropertyFor(nameof(AwkwardParent.Child))!.Value + .ShouldStartWith(OptionsValue.UnreadablePrefix); + } + + [Fact] + public void still_serializable_with_unreadable_properties() + { + new OptionsDescription(new AwkwardObject()).ShouldBeSerializable(); + } +} + +public class AwkwardObject +{ + public const string SecretishMessage = "Endpoint=sb://myns.servicebus.windows.net;SharedAccessKey=shhh"; + + public string Name { get; set; } = "Rogue"; + public int Tolerable => 42; + + public string Explosive => throw new NullReferenceException(SecretishMessage); + + public string WriteOnly { set { } } + + public string this[int index] => throw new NotSupportedException(); +} + +public class AwkwardParent +{ + public string Name { get; set; } = "Storm"; + + [ChildDescription] + public AwkwardObject Child => throw new InvalidOperationException("nope"); +} + public class SomeObject : ITagged { public string[] Tags => ["blue", "green"]; diff --git a/src/JasperFx/Descriptors/OptionsDescription.cs b/src/JasperFx/Descriptors/OptionsDescription.cs index 54a3aa3f..9ef39c90 100644 --- a/src/JasperFx/Descriptors/OptionsDescription.cs +++ b/src/JasperFx/Descriptors/OptionsDescription.cs @@ -1,5 +1,6 @@ using System.Collections; using System.Diagnostics.CodeAnalysis; +using System.Reflection; using JasperFx.Core; using JasperFx.Core.Reflection; @@ -107,54 +108,78 @@ private void readProperties(object subject) foreach (var property in type.GetProperties().Where(x => !x.HasAttribute())) { - if (property.HasAttribute()) + // Neither of these can be read with PropertyInfo.GetValue(subject): a set-only property has no + // getter at all, and an indexer demands index arguments (TargetParameterCountException without + // them). Both are legal on a type that's being described, and neither is configuration data. + if (!property.CanRead || property.GetIndexParameters().Length > 0) continue; + + // An OptionsDescription is a *diagnostic* view of somebody else's configuration object, built by + // calling arbitrary property getters. Any one of those getters can throw -- lazily connecting to + // a broker, asserting on state that isn't initialized yet, dereferencing a null connection string + // -- and losing the entire description (and everything built on top of it, e.g. Wolverine's + // ServiceCapabilities snapshot) over one bad property is a terrible trade. Report what couldn't be + // read, in place, and carry on. + try { - var child = property.GetValue(subject); - if (child == null) continue; - - var childDescription = child is IDescribeMyself describes ? describes.ToDescription() : new OptionsDescription(child); - Children[property.Name] = childDescription; - - continue; + readProperty(subject, type, property); } - - if (property.HasAttribute()) + catch (Exception e) { - var value = property.GetValue(subject); - var items = value is IEnumerable enumerable - ? enumerable.Cast().Select(x => x?.ToString() ?? string.Empty).ToArray() - : Array.Empty(); - - Properties.Add(new OptionsValue - { - Subject = $"{type.FullNameInCode()}.{property.Name}", - Name = property.Name, - Type = PropertyType.StringArray, - RawValue = items, - Value = items.Join(", ") - }); - - continue; + Properties.Add(OptionsValue.Unreadable(property, subject, e)); } + } + } + + [RequiresUnreferencedCode("Reads subject.GetType().GetProperties() reflectively.")] + private void readProperty(object subject, Type type, PropertyInfo property) + { + if (property.HasAttribute()) + { + var child = property.GetValue(subject); + if (child == null) return; + + var childDescription = child is IDescribeMyself describes ? describes.ToDescription() : new OptionsDescription(child); + Children[property.Name] = childDescription; - if (property.HasAttribute()) + return; + } + + if (property.HasAttribute()) + { + var value = property.GetValue(subject); + var items = value is IEnumerable enumerable + ? enumerable.Cast().Select(x => x?.ToString() ?? string.Empty).ToArray() + : Array.Empty(); + + Properties.Add(new OptionsValue { - var state = property.GetValue(subject) != null ? "Configured" : "Default"; - Properties.Add(new OptionsValue - { - Subject = $"{type.FullNameInCode()}.{property.Name}", - Name = property.Name, - Type = PropertyType.Text, - RawValue = state, - Value = state - }); - - continue; - } + Subject = $"{type.FullNameInCode()}.{property.Name}", + Name = property.Name, + Type = PropertyType.StringArray, + RawValue = items, + Value = items.Join(", ") + }); + + return; + } - if (property.PropertyType != typeof(string) && property.PropertyType.IsEnumerable()) continue; - Properties.Add(OptionsValue.Read(property, subject)); + if (property.HasAttribute()) + { + var state = property.GetValue(subject) != null ? "Configured" : "Default"; + Properties.Add(new OptionsValue + { + Subject = $"{type.FullNameInCode()}.{property.Name}", + Name = property.Name, + Type = PropertyType.Text, + RawValue = state, + Value = state + }); + + return; } + + if (property.PropertyType != typeof(string) && property.PropertyType.IsEnumerable()) return; + Properties.Add(OptionsValue.Read(property, subject)); } diff --git a/src/JasperFx/Descriptors/OptionsValue.cs b/src/JasperFx/Descriptors/OptionsValue.cs index a414254b..68558356 100644 --- a/src/JasperFx/Descriptors/OptionsValue.cs +++ b/src/JasperFx/Descriptors/OptionsValue.cs @@ -50,8 +50,20 @@ public static OptionsValue Read(T subject, Expression> express public static OptionsValue Read(PropertyInfo property, object subject) { - var value = property.GetValue(subject); - + object? value; + try + { + value = property.GetValue(subject); + } + catch (Exception e) + { + // Reading a property on somebody else's configuration object can fail in all kinds of ways -- + // it might lazily open a connection, assert on uninitialized state, or dereference a null. This + // is a diagnostic view, so record *that* it couldn't be read and let the rest of the description + // through instead of throwing out of a getter walk. + return Unreadable(property, subject, e); + } + var description = new OptionsValue { Subject = $"{subject.GetType().FullNameInCode()}.{property.Name}", @@ -62,10 +74,40 @@ public static OptionsValue Read(PropertyInfo property, object subject) }; description.WriteValue(value); - + return description; } + /// + /// Prefix on marking a property whose getter threw when the description was built. + /// + public const string UnreadablePrefix = "Could not be read -- "; + + /// + /// Represents a property that could not be read at all -- its getter threw. Used in place of the real + /// value so that one misbehaving property can't cost you an entire diagnostic description. Deliberately + /// reports only the exception *type*: descriptions are shipped off to monitoring tools, and exception + /// messages have a habit of quoting the very configuration values (connection strings, keys) that + /// descriptions work hard to keep out. + /// + public static OptionsValue Unreadable(PropertyInfo property, object subject, Exception exception) + { + // Reflection wraps anything thrown by the getter itself; the inner exception is the interesting one + var actual = exception is TargetInvocationException { InnerException: not null } wrapped + ? wrapped.InnerException + : exception; + + return new OptionsValue + { + Subject = $"{subject.GetType().FullNameInCode()}.{property.Name}", + Name = property.Name, + Type = PropertyType.Text, + RawValue = null, + Value = UnreadablePrefix + actual.GetType().Name + }; + } + + public void WriteValue(object? value) { if (value == null)