Mark some properties as sensitive

[juchom]

Is your feature request related to a problem? Please describe.

Actually when we use FluentValidation and there are errors the value is stored in clear text.

If we log this errors we could have some sensitive data like passwords, credit card number, ...

For exemple here is a logged error in Seq :

{
  "AttemptedValue": "toto",
  "CustomState": null,
  "ErrorCode": "EqualValidator",
  "ErrorMessage": "La confirmation n''est pas identique au mot de passe.",
  "FormattedMessagePlaceholderValues": {
    "ComparisonProperty": "Password",
    "ComparisonValue": "titi",
    "PropertyName": "Confirm Password",
    "PropertyValue": "toto"
  },
  "PropertyName": "ConfirmPassword",
  "Severity": "Error"
}

Describe the solution you'd like

We can hide properties from logging in Serilog using Destructurama (https://github.com/destructurama/attributed)

public class CustomizedMaskedLogs
{
    /// <summary>
    /// 123456789 results in "***"
    /// </summary>
    [LogMasked]
    public string Password { get; set; }

    /// <summary>
    /// 123456789 results in "***"
    /// </summary>
    [LogMasked]
    public string ConfirmPassword { get; set; }
}

Then the log would look like this :

{
  "AttemptedValue": "***",
  "CustomState": null,
  "ErrorCode": "EqualValidator",
  "ErrorMessage": "La confirmation n''est pas identique au mot de passe.",
  "FormattedMessagePlaceholderValues": {
    "ComparisonProperty": "Password",
    "ComparisonValue": "***",
    "PropertyName": "Confirm Password",
    "PropertyValue": "***"
  },
  "PropertyName": "ConfirmPassword",
  "Severity": "Error"
}

Even if it would be very convenient for me, I'm not sure this would be the best way to mark the data as sensitive for FluentValidation.

Describe alternatives you've considered

One option could be to mark them as sensitive

RuleFor(m => m.Password).NotEmpty().Sensitive();

Additional Context

No response

[JeremySkinner]

Thanks for the suggestion. This isn't something I'm personally interested in implementing, but if you think this would be valuable to the community and would be willing to provide a PR then I'll happily review it. Please note the core FluentValidation library shouldn't take any additional dependenceis (eg on Destructurama), so an extension method approach may be better.

Edit: The other option would be to not put this into the core library, but instead add extension points which a separate library/package could hook into to implement this, then you could have the external library take a dependency on Destructurama. Let me know what you think

[juchom]

Edit: The other option would be to not put this into the core library, but instead add extension points which a separate library/package could hook into to implement this, then you could have the external library take a dependency on Destructurama. Let me know what you think

I started to look at the code and this is the best option to solve this. I have to dig this to make sure I understand the code.

Thanks

[JeremySkinner]

For the extension point I was thinking we could add a new hook to ValidatorOptions.Global, something like OnFailureCreated or something like that which you could set in your application startup:

ValidatorOptions.Global.OnFailureCreated = (ValidationFailure failure, IValidationContext context, object propertyValue, IValidationRule rule, IRuleComponent component) => {
  if (rule.Member != null) {
     bool hasMaskedAttribute = ... // do the necessary reflection to check for the attribute.
     if (hasMaskedAttribute) {
       failure.AttemptedValue = "***";
       failure.FormattedMessagePlaceholerValues["PropertyValue"] = "***";
       if (failure.FormattedMessagePlaceholderValues.ContainsKey("ComparisonValue")) {
          failure.FormattedMessagePlaceholderValues["ComparisonValue"] = "***";
       }
    }
  }
  return failure;
}

would that work for you?

[juchom]

This extension point seems good, in my case this would allow me to nicely plug a new method that supports Destructurama's attributes so I can leverage it everywhere in my application.

EDIT: Not sure if this would be a good idea, but this extension point could be behind an interface ? This could hold an instance with some private properties. (In my case I would probably add a dictionary of cached properties)

[JeremySkinner]

Not quite sure what you mean about an interface - the solution I had in mind was using a callback func as detailed above, which you can then swap out as necessary. This is similar to our other global extension points. But do feel free to propose an alternative design if you have something specific in mind.

[juchom]

I'm not very familiar with the codebase so I may be wrong, I was wondering if instead of asking a Func<,,> like this

public Func<Type, MemberInfo, LambdaExpression, string> PropertyNameResolver

The extension point would look like this

public IFailureCreator OnFailureCreated 

and IFailureCreator would look like

public interface IFailureCreator
{
   ValidationFailure CreateValidationFailure(IValidationContext context, object propertyValue, IValidationRule rule, IRuleComponent component);
}

Then for people who wants to override this property it would be easier to implement the interface and assign an instance of it to ValidatorOptions.Global.OnFailureCreated

EDIT: After writing this, I'm wondering if it's not overkill for this need

[JeremySkinner]

I don't think an interface adds any value here.

An interface with a single method has the same end result as a settable func - both provide a contract, the difference being an interface provides a contract for a class and a delegate provides a contract for a method; one is an object-oriented approach, the other is a more functional approach. The end result is the same - you're replacing the method implementation.

[juchom]

one is an object-oriented approach, the other is a more functional approach

I love this explanation, I did not see it like this. But this exactly it 👍

Thanks a lot !

[JeremySkinner]

@juchom I've added the code for this in #2120. Please can you confirm if that'll be suitable for your use case?

[juchom]

Hello @JeremySkinner;

Thanks for this quick PR. This works for me !

[JeremySkinner]

I've pushed out 11.6.0 with this change