BeanUtils.setProperty may shouln't record trace log #276

F0otman · 2024-08-22T13:40:15Z

From BEANUTILS-568,
Sorry for lacked unit test, cause beanutils only use commons-logging, it cannot show the key point of log without settings or materialized log component.

The problem I want to warn is if I override bean method toString like
return "TestBean{" + "propertyPassword='" + "******" + "\" + '}'
When try to use BeanUtils.setProperty & BeanUtils.copyProperty & LocaleBeanUtilsBean.setProperty, it would record trace log the value of propertyPassword. So if someone tamper log level or set the wrong level, the shaded property may record in log.

The safety way is when toString method is overrided, do not show the value in log.

garydgregory · 2024-08-22T17:03:39Z

@F0otman

Would you like to use the new method from your code? If not, please make the method private.

F0otman · 2024-08-23T02:09:45Z

The method not only use for BeanUtilsBean.copyProperty and BeanUtilsBean.setProperty, but also for LocaleBeanUtilsBean.setProperty, they are same case and duplicate code. Protected id better

garydgregory

Hello @F0otman
Please see my comments.

garydgregory · 2024-08-24T22:21:22Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

-            }
-            sb.append(')');
-            LOG.trace(sb.toString());
+            LOG.trace(traceLogRecord(bean, name, value, sb).toString());;


Too many semicolons;;

garydgregory · 2024-08-24T22:21:42Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

-            }
-            sb.append(')');
-            LOG.trace(sb.toString());
+            LOG.trace(traceLogRecord(bean, name, value, sb).toString());;


Too many semicolons;;

garydgregory · 2024-08-24T22:22:15Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

+     * @param name Property name (can be nested/indexed/mapped/combo)
+     * @param value Value to be set
+     */
+    protected static StringBuilder traceLogRecord(Object bean, String name, Object value, StringBuilder sb) {


Missing @param tag.
Missing @return tag.

garydgregory · 2024-08-24T22:23:06Z

src/main/java/org/apache/commons/beanutils2/locale/LocaleBeanUtilsBean.java

-            }
-            sb.append(')');
-            LOG.trace(sb.toString());
+            LOG.trace(traceLogRecord(bean, name, value, sb).toString());;


Too many semicolons;;

F0otman · 2024-08-26T16:28:41Z

Thanks for the comments.
I heared beanutils 2 is coming, any feature I could supplement?

garydgregory · 2024-08-26T21:40:47Z

So if someone tamper log level or set the wrong level, the shaded property may record in log.

If some has this kind of access to your application, you're already in very deep trouble. Note that some folks recommend to never store credentials in Strings for this reason, and use char[] instead.

Let's see if anyone else has feedback.

garydgregory · 2024-08-26T21:43:59Z

Thanks for the comments. I heared beanutils 2 is coming, any feature I could supplement?

One remaining development task for 2.x is to pull up the "2" types
into their parent:

org.apache.commons.beanutils2.BeanUtilsBean2 extends BeanUtilsBean
org.apache.commons.beanutils2.ConvertUtilsBean2 extends ConvertUtilsBean

The last time I tried this, tests failed and I was semi-stuck as to
how to fix those.

See also:

Discussions and PRs on GitHub are welcome.

sigee · 2024-08-27T06:36:34Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

+     * @param sb the start of stringBuilder
+     * @return the value should log as trace
+     */
+    protected static StringBuilder traceLogRecord(Object bean, String name, Object value, StringBuilder sb) {


For me passing a StringBuilder seems a bit odd, just because there is an initial value in it. I would rather pass an Immutable String as initial value.

Parameters could be final.

Your suggestion may right, the futher code I wrote tried to keep as same as original possibly.
Your's are more sence

sigee · 2024-08-27T06:45:06Z

I think a couple of test cases would be nice, at least for the new logic.

F0otman · 2024-08-28T16:21:32Z

Test cases must by fine, but the changed code is use for logging. Beanutils only use commons-logging as logging adapter system, I cannot directly log without import any other implementations logging software (At least I have no idea).

Anyone have idea?

sigee · 2024-08-28T19:47:18Z

@F0otman
Typically, logging is not a feature you have to test, but with this change, you put a little "business logic" into it.

About the test writing. You are right, it is a bit complicated because the code may not be in a testable format.
"Never hide a TUF within a TUC."
TUF - Test Unfriendly Feature

a piece of functionality embodied in code, which makes unit testing difficult
database access
filesystem access
network access
static variable usage

TUC - Test Unfriendly Construct

final methods
final classes
static methods
private methods
static initialization expressions

I'll think about it and will be back tomorrow.
Maybe a small refactor (create a constructor where you can inject a LOG) and you can assert the LOG.trace() was called with the expected parameter.

sigee · 2024-08-29T08:25:42Z

I created a PoC commit here.
Not my best work, but I thought about something like this.

I removed the final modifyer from LOG and created a constructor to inject my spy object.
In the test class I created 3 test cases
- Basic case
- Null case
- toString() is overriden
I also created the LogSpy to check the parameters trace was called with.
I also created a child test bean where I override toString()

For the spy Mockito would be great, because this hand-made spy feels wierd. But Mockito is not in the project and I'm not sure why.

Anyway. What do you guys think? Is is a way to go or I just overengineering it?

Added some cases and comments

F0otman · 2024-08-30T18:34:07Z

It's a good idea to implement as a simplest SimpleLog

But it add a new public constructor for BeanUtilsBean, is it worth for test?

melloware · 2024-08-31T12:42:51Z

+1 on this change in general though as we have been burned by stuff like this in the past someone logging a password field in a toString.

garydgregory

Hello @F0otman

Please see my comment.

garydgregory · 2024-09-03T13:44:52Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

@@ -1067,4 +1039,60 @@ public void setProperty(final Object bean, String name, final Object value)
                (e, "Cannot set " + propName);
        }
    }
+
+    /**
+     * <p>Build the stringBuilder by using set/copy bean property for log, only


This comment is not quite right IMO: It mentions logging but the method does not log or check log levels. Since it is a protected method, it can be called from a subclass so you have no idea how it will be used. All of this to say, I think the Javadoc should just say what it does.

garydgregory · 2024-09-03T13:46:49Z

src/main/java/org/apache/commons/beanutils2/BeanUtilsBean.java

+    public BeanUtilsBean(final ConvertUtilsBean convertUtilsBean, final PropertyUtilsBean propertyUtilsBean, final Log log) {
+        this.convertUtilsBean = convertUtilsBean;
+        this.propertyUtilsBean = propertyUtilsBean;
+        LOG = log;


This looks very bad IMO: Every time an instance is created, a global variable is edited, which sounds to me like a recipe for bugs and libraries blaming each other.

Not every time. The original constructors have not changed, they do not edit the global LOG. But you have to have some edit option if you want to inject dependency for mocking. It could be a constructor or a setter. For my PoC this was easyer, but as I said, not my best work. It could be much better if someone have time to really think about the solution. Unfortunately I can't work on it now, because I only have my phone for 2 weeks as technology.

F0otman · 2024-09-14T09:09:30Z

Sorry for wrong git operate as closed, I synced fork master from here. I would pull a new pr these days :-(

garydgregory requested changes Aug 24, 2024

View reviewed changes

sigee reviewed Aug 27, 2024

View reviewed changes

sigee added a commit to sigee/commons-beanutils that referenced this pull request Aug 29, 2024

Add test cases for apache#276

fb1f217

F0otman pushed a commit to F0otman/commons-beanutils that referenced this pull request Aug 30, 2024

Add test cases for apache#276

41b6902

F0otman pushed a commit to F0otman/commons-beanutils that referenced this pull request Aug 30, 2024

Add test cases for apache#276

d58bd14

Added some cases and comments

garydgregory requested changes Sep 3, 2024

View reviewed changes

F0otman closed this Sep 14, 2024

F0otman force-pushed the master branch from d58bd14 to a9d9af0 Compare September 14, 2024 06:53

F0otman mentioned this pull request Oct 15, 2024

BeanUtils.setProperty may shouln't record trace log #304

Open

BeanUtils.setProperty may shouln't record trace log #276

BeanUtils.setProperty may shouln't record trace log #276

Uh oh!

Conversation

F0otman commented Aug 22, 2024

Uh oh!

garydgregory commented Aug 22, 2024

Uh oh!

F0otman commented Aug 23, 2024

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

F0otman commented Aug 26, 2024

Uh oh!

garydgregory commented Aug 26, 2024

Uh oh!

garydgregory commented Aug 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigee commented Aug 27, 2024

Uh oh!

F0otman commented Aug 28, 2024

Uh oh!

sigee commented Aug 28, 2024

Uh oh!

sigee commented Aug 29, 2024

Uh oh!

F0otman commented Aug 30, 2024

Uh oh!

melloware commented Aug 31, 2024

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

F0otman commented Sep 14, 2024

Uh oh!

Uh oh!