commons-lang icon indicating copy to clipboard operation
commons-lang copied to clipboard

Add methods allowing masking of Strings

Open greenman18523 opened this issue 6 years ago • 8 comments

maskStart and maskEnd allow masking the original str by replacing it's characters with the specified character-mask. Common usecase is to hide sensitive information from logs, by using it in toString of classes or in inputs to log calls.

greenman18523 avatar Jun 02 '18 20:06 greenman18523

Coverage Status

Coverage increased (+0.02%) to 95.254% when pulling 4c972e1d5d55f05a1786cce0cdacd2323399f482 on greenman18523:master into 6850d88880093733c0ba29c371f2c9823256d749 on apache:master.

coveralls avatar Jun 02 '18 20:06 coveralls

Hi @greenman18523 thank for your contribution - from my experience this is "must have" functionality for Commons Lang library because in almost all bit projects what I saw was their home grown masking function.

But what I would like to propose is to simplify the api and use one function mask() instead of two maskStart() and maskEnd(). This will simplify code of the functions especially both of them are quite similar. I created a pull request with my alternative implementation #335 Also I created a JIRA ticket https://issues.apache.org/jira/browse/LANG-1400

Please give your feedback, Thank you

stokito avatar Jun 23 '18 22:06 stokito

Hello @stokito, thanks for the input. The reasoning of creating two method was for a more intuitive API and to not add too many parameters in just one method. And I also didn't want to create a 3rd method that would just do the main logic without it being able to be exposed (i.e. private).

I like the idea of the mask being in the middle of the string, and I was thinking of incorporating it into the code, but was not sure how useful it would be for others.

An other goal of the way the methods are, is to clearly enforce a minimum amount of masking before being able to show any information. e.g. in the case of a credit card, even if the user has erroneously entered 8 out of the 16 characters, a call to StringUtils.maskEnd("35660020", '*', 12, 4) would not show any sensitive information, but a call to StringUtils.mask("35660020", 0, 4, '*') would expose.

I think that both implementations can be useful for a number of use-cases, some common to both, but some are covered by one of the two implementation.

Of course this can also be achieved by an extra parameter in your implementation. What do you think?

greenman18523 avatar Jun 24 '18 17:06 greenman18523

New method added, to cover usacases of https://issues.apache.org/jira/browse/LANG-1400

greenman18523 avatar Jul 06 '18 17:07 greenman18523

"must have" ... because in almost all bit projects what I saw was their home grown masking function.

Well, and I had to add another one to address NUTCH-2905 in apache/nutch#704. We had one additional use case: mask only a part of string matched by a regular expression.

sebastian-nagel avatar Nov 18 '21 15:11 sebastian-nagel

Masking might belong in Commons Text, not here in Commons Lang IMO.

garydgregory avatar Nov 18 '21 17:11 garydgregory

As a basic secure functionality it must be in core Java library or better even as a method of String. Security functions are "batteries" that must be included. Almost all projects that I saw had commons-lang as a dependency but the commons-text I can't even remember.

stokito avatar Nov 18 '21 19:11 stokito

As a basic secure functionality it must be in core Java library or better even as a method of String. Security functions are "batteries" that must be included. Almost all projects that I saw had commons-lang as a dependency but the commons-text I can't even remember.

There is no point arguing here what belongs in what JRE class ;-) We have been moving text based code to Commons Text for some time now while deprecating the original code in Commons Lang. Therefore, I don't see us adding more text based utilities to Lang. Masking is definitely a text concept IMO, as opposed to java.lang level processing.

garydgregory avatar Nov 18 '21 19:11 garydgregory