Microsoft.Extensions.Configuration.ConfigurationBinder.BindProperty(PropertyInfo,object, IConfiguration, BinderOptions) – harmful call to PropertyInfo.GetValue(object)

[deep-outcome]

Description

The ConfigurationBinder.BindProperty seems to be

private static void BindProperty(PropertyInfo property, object instance, IConfiguration config, BinderOptions options)
{
    // We don't support set only, non public, or indexer properties
    if (property.GetMethod == null ||
        (!options.BindNonPublicProperties && !property.GetMethod.IsPublic) ||
        property.GetMethod.GetParameters().Length > 0)
    {
        return;
    }

    object propertyValue = property.GetValue(instance);
    bool hasSetter = property.SetMethod != null && (property.SetMethod.IsPublic || options.BindNonPublicProperties);

    if (propertyValue == null && !hasSetter)
    {
        // Property doesn't have a value and we cannot set it so there is no
        // point in going further down the graph
        return;
    }

    propertyValue = GetPropertyValue(property, instance, config, options);

    if (propertyValue != null && hasSetter)
    {
        property.SetValue(instance, propertyValue);
    }
}

Lines object propertyValue = property.GetValue(instance); ,

if (propertyValue == null && !hasSetter)
{
  // Property doesn't have a value and we cannot set it so there is no
  // point in going further down the graph
  return;
}

are obviously some debris somebody forgotten to remove. The line object propertyValue = property.GetValue(instance); can call to properties with not setter – like static properties – forcing them to execute.

Moreover, why to obtain any property value before asking if it can be passed to existing setter. 👎🏻

Configuration

Microsoft.Extensions.Configuration.Binder 5.0.0 on .NET 5.0

Area

area-Extensions-Configuration

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The ConfigurationBinder.BindProperty seems to be

private static void BindProperty(PropertyInfo property, object instance, IConfiguration config, BinderOptions options)
{
    // We don't support set only, non public, or indexer properties
    if (property.GetMethod == null ||
        (!options.BindNonPublicProperties && !property.GetMethod.IsPublic) ||
        property.GetMethod.GetParameters().Length > 0)
    {
        return;
    }

    object propertyValue = property.GetValue(instance);
    bool hasSetter = property.SetMethod != null && (property.SetMethod.IsPublic || options.BindNonPublicProperties);

    if (propertyValue == null && !hasSetter)
    {
        // Property doesn't have a value and we cannot set it so there is no
        // point in going further down the graph
        return;
    }

    propertyValue = GetPropertyValue(property, instance, config, options);

    if (propertyValue != null && hasSetter)
    {
        property.SetValue(instance, propertyValue);
    }
}

Lines object propertyValue = property.GetValue(instance); ,

if (propertyValue == null && !hasSetter)
{
  // Property doesn't have a value and we cannot set it so there is no
  // point in going further down the graph
  return;
}

are obviously some debris somebody forgotten to remove. The line object propertyValue = property.GetValue(instance); can call to properties with not setter – like static properties – forcing them to execute.

Moreover, why to obtain any property value before asking if it can be passed to existing setter. 👎🏻

Configuration

Microsoft.Extensions.Configuration.Binder 5.0.0 on .NET 5.0

Area

area-Extensions-Configuration

Author:	Uzivatel919
Assignees:	-
Labels:	area-Extensions-Configuration, untriaged
Milestone:	-

[david-jarman]

This is currently biting me, where I have a read-only property on one of my objects that can potentially throw an exception if called before some extra setup is executed. This forces me to modify the behavior of the property so it doesn't throw while the binding process is happening.

example:

public class BindingObject
{
    public string BindThisString { get; set; }

    public OtherObject ThisIsNotSetDuringBinding { get; set; }

    // This will throw an exception during binding, because the get is called for no good reason
    public string DerivedPropertyString => $"DerivedProperty={ThisIsNotSetDuringBinding .Id}"
}

[ericstj]

Reopening since the fix introduced a regression and will be reverted.

[ericstj]

Make sure that any fix for this in the future accounts for the scenario here: #63479