Special cases when using `FilterKeywords()` with `EnableFormDataLogging()`

[liviriniu]

Found that if the keywordBlackList parameter contains string.Empty / "", ALL the form values are obfuscated. I understood that IndexOf() that works internally on the list, finds the empty string at the beginning of any non-null string. Is this to be expected? I think that the intention would be more obvious if one could be able to configure EnableFormDataLogging with something like .ObfuscateAllValues() while the empty string will be ignored or stripped from the keywordBlackList.

Also, if a null slips into keywordBlackList, it crashes with the same IndexOf() throwing ArgumentNullException. Null should be ignored/stripped from the list.

What are your thoughts on these?

[tsimbalar]

Hi @liviriniu ,

I would probably handle different cases in a different way.

if a null slips into keywordBlackList, it crashes with the same IndexOf() throwing ArgumentNullException. Null should be ignored/stripped from the list.

If a null slips into keywordBlackList, I think I would just through a ArgumentException in the call .FilterKeywords(...) , so that the caller detects it as early as possible and can correct it (this will then fix the issue down the line).

Found that if the keywordBlackList parameter contains string.Empty / "", ALL the form values are obfuscated

Passing a "" is indeed an edge-case , I would say the behavior does make sense and is aligned with the documentation of the method :

The Form Data values will be offuscated when the key contains one of the black-listed words.

and I would probably not change it now, so as not to break clients who might be relying on that behavior.

I think that the intention would be more obvious if one could be able to configure EnableFormDataLogging with something like .ObfuscateAllValues() while the empty string will be ignored or stripped from the keywordBlackList.

But as you propose, I do think it could make sense to have an option like ObfuscateAllValues() that will just obfuscate all values .... but I honestly have a hard-time figuring out a use case where this would make sense 🤔 (if you are enabling FormData, I'd suspect you are interested in seeing at least some of the values, right ? ).

Overall, I think we have to be a bit careful introducing possibly breaking changes in the library, as it is used in many places, and I probably don't want a change to accidentally cause sensitive information to appear in logs.

So I think I would :

• happily accept some extra protection / guards against nulls in the list of keywords
• not want to change the behavior when passing "" in the list of keywords so as to not introduce breaking change
• maybe not accept a new method ObfuscateAllValues() as I sort of have a hard time imagining a use case for this ... and also having a hard-time finding a name that would be consistent with the other methods there (.DisablePasswordFiltering(), .FilterKeywords(), FilterKeywords())

Does this make sense ?

thanks again for your contribution 👍

[liviriniu]

It does make a lot of sense, yes. I see the bigger picture now, thanks.

1) Can't see through/decide between:

• if I understood correctly: make .FilterKeywords(keywordBlackList) throw something like "the black list does not support null items" as to be clear why it breaks. The caller is helped to see the null early and avoid problems later in its own code. By this, logger keeps the current intolerant behavior to null although it could be made to tolerate it.
• the other way, silently strip all nulls and let the filtering always happen. Null doesn't make sense as a keyword, so why not ignore it. I reckon it's the caller's responsibility to take care of own code. But yeah, caller would not know about eventual problem later on because of null. Also, existing API consumers after a change like this could wake up with code treating nulls at the point of calling FilterKeywords that is redundant. I fail to see the importance of this. Maybe let it all be as it is now :))

2) I agree with not changing behavior of passing "", did not see the breaking change risk. Indeed it is.

3) ObfuscateAllValues() is not a good idea, I agree with your arguments on this and do not see benefit in complicating things by trying to achieve consistency with existing methods. If someone would ever want to, let's say, just enumerate the form field names for the sake of some special debug and at the same time keep log volume low, a different kind of FormData property would make sense - array of strings. Well, I just explored the idea, making this happen probably necessitates a different (derived from same base) config builder anyhow.

[tsimbalar]

To choose between :

• rejecting invalid inputs
• tolerate slighly incorrect inputs

is choosing between :

• Enforcing preconditions or
• "Be conservative in what you send, be liberal in what you accept" of the Robustness principle

Both could probably be fine, but in that specific case, if you remember from the code, there are already default keywords that are obfuscated (*password* and things like that if I recall properly). Not providing your own "keywords" means "use the default" .... but if you provide [null] ... is an array with null the same as not providing keywords ? one could decide both ways, but there is no really obvious interpretations ... so it might be better to just get rid of the ambiguity and not allow cases that could cause seemingly incorrect behavior in the first place.

So yes, I would probably add a Guard Clause, in the form of throwing a ArgumentException, indicating that the argument is not correct, the name of the argument and why it's not correct ("cannot contain null" or something of the sort, as you said).

In my opinion, ending up with an collection that contains nulls probably means something is wrong prior to there, so I'm fine rejecting, and possibly helping the caller figure out that they are indeed doing something wrong.

Does that sound sensible ? :)

[tsimbalar]

(also, if we go ahead and add this behavior, it would be nice to have a test to "make it official" that is is the desired behavior)

[liviriniu]

Learning something every day :)

better to just get rid of the ambiguity and not allow cases that could cause seemingly incorrect behavior in the first place

I understand now that it's better this way, I failed to see the "use default" aspect. I do have my past experiences with PHP ambiguity in using basic functions... so yeah. This reminds me of the weak vs strong typing discussion.

I'll see what I can do