[reload4j] the jdbc_cve branch

[Ceki Gülcü]

Hi Vladimir,

First of all, I would like to say that the work you have done yesterday
in a short lapse time, is quite impressive. Thank you for your hard
work. It is much appreciated.

You seem to favor leaving an insecure mode as an opt-in feature. Given
the enormity of the log4shell vulnerability, and given the purpose of
reload4j, an insecure mode might be quite hard to defend in the current
climate, even as an opt-in feature.

As for the project affected by this change, they can add JDBCAppender2.
It is not that hard.

I think with the secure mode, we are going above and beyond of what
might be expected from a small team such as ours. That is very much
thanks to you.

More inline.

On 1/20/2022 7:29 PM, Vladimir Sitnikov wrote:
>>I have created a jdbc_cve
>>branch in qos-ch/reload4j repo based on PR 26
> 
> Please check 26 once again. I made it to contain two commits:
> 1) revert "jdbcappender removal"
> 2) cve fix.

> That enables minimizing and isolating changes related to the CVE fix.

Unless I am mistaken, I have started from the latest PR 26.

> -----
> 
>>1) reformat using javaConventions.xml
> 
> While I have no problems with doing that,
> reformatting **between**  CVE-related fixes (mine and your activateOptions)
> makes review harder.

> For instance, as one of the changes you **killed** JDBCAppender#removes
> field
> which silently breaks backward compatibility. 

Not silently as we would have amply publicized this.


> If you absolutely want reformat, then I would suggest:
> * "revert removal jdbc"
> * "reformat"
> * "fix CVE"

That is exactly what I have done, in the order you describe.

>>2) JdbcPatterParser is created only once in JdbcAppender.activateOptions
> and then reused.
> 
> I did not know activateOptions existed.
> Picking options in activateOptions
> 
>>3) Modified JdbcPatternParserTest to make them a bit easier to follow,
>>at least for me
> 
> While I do not care how the three tests are written,
> I strongly believe ParserState is way harder for both **read** and
> **write** test.

> It overcomplicates things (e.g. equals/hashcode boilerplate which is one
> for place for error).
> 
> My test is 25 lines, your test is 100 lines, while effectively they have
> exactly the same test coverage.
> 
> As you introduced ParserState, the test creation became a manual process.
> You need to carefully create ParserState instance, add quotes, etc.

You must construct the expected result by hand. This implies that you
know what you are doing when you write the test. That is kind of the point.

> Then, the error message would look different from what the code looks like.
> That means a failing test would be harder to understand.

The error message shows the expected and actual strings as well.

> My approach was to compare strings, so I can easily add new tests by just
> adding input and leaving the expected value blank.
> Then I would just grab the "expected" output from the test failure and
> use it as the expected
> value in the test assertion.
> 
> Then, I see you've removed assert message. 
> That is really sad as it would make it complicated to analyze the failure.
> 
[cut]
> 
> Note how you don't know **WHY** something is expected, and you can't
> proofread the failure to tell if that is legit or not.
> The assertion message should be there to tell what was the input, so the
> one who runs into test failure can understand it.

Yes, the assertion message is missing. I agree it would be better to put
it back. Will do.

> Final 2c: in your branch, JdbcPatternParserTest mixes tabs and spaces,
> which is a frustrating thing for a **new** class.

I was not aware of that. It must be the mixed tab mode which is the
default. Would "only spaces" be better?

> Vladimir
> 

-- 
Ceki Gülcü

Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch