Hi Vladimir, I am still studying PR 26. In the mean time, I have created a jdbc_cve branch in qos-ch/reload4j repo based on PR 26. Here is the link: https://github.com/qos-ch/reload4j/tree/jdbc_cve Here are changes so far: 1) reformat using javaConventions.xml 2) JdbcPatterParser is created only once in JdbcAppender.activateOptions and then reused. 3) Modified JdbcPatternParserTest to make them a bit easier to follow, at least for me I think point 2) is quote useful. Anyway, more later. -- Ceki Gülcü Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch
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. -----
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. If you absolutely want reformat, then I would suggest: * "revert removal jdbc" * "reformat" * "fix CVE"
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. Then, the error message would look different from what the code looks like. That means a failing test would be harder to understand. 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. Here's an example: My approach: assertParameterizedSql( "JdbcPatternParser{sql=insert into t(message) values ('abc'),args=[]}", "insert into t(message) values ('%m''%m')" ); Failure: org.junit.ComparisonFailure: parser.setPattern(...).toString() for insert into t(message) values ('%m''%m') Expected :JdbcPatternParser{sql=insert into t(message) values ('abc'),args=[]} Actual :JdbcPatternParser{sql=insert into t(message) values (?),args=[%m'%m]} ---- Your approach: public void testSingleQuotesAndSpaces2() { ParserState expected = new ParserState("insert into t(message) values ('abc')"); otherAssert("insert into t(message) values ('%m''%m')", expected); Failure: Expected :ParserState [expected=insert into t(message) values ('abc'), args=[]] Actual :ParserState [expected=insert into t(message) values (?), args=[%m'%m]] 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. Final 2c: in your branch, JdbcPatternParserTest mixes tabs and spaces, which is a frustrating thing for a **new** class. Vladimir
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
Not silently as we would have amply publicized this.
I strongly suggest keeping the current APIs if possible (keep old fields, types, etc). It does not help much if we drop JDBCAppender#removes field, and the removal definitely breaks apps. So why remove it? I intentionally avoided making extra cleanup changes in PR26 so we can agree on the approach and merge it. I assumed the cleanup can go afterward if needed. Frankly speaking, I'm inclined that there might be a good case for several JDBC appenders. For instance: -jdbc-tableappender.jar The users would be able to configure table name and column names. The appender would write to the table and that would be it. -jdbc-freesql.jar The user configures SQL, and the appender uses it via prepared statement. This is less secure than -jdbc-tableappender, because an attacker would be able to execute an arbitrary SQL if they can edit the configuration file. The same thing might apply to other appenders (and logback? ;) ). If that is something appealing to you, then imagine we can move the current JDBCAppender to reload4j-jdbc-legacy.jar and we could let it die there. We don't need to refactor and modernize it then.
cure mode might be quite hard to defend in the current climate, even as an opt-in feature.
Frankly speaking, I assumed that there might be systems that use the current JDBCAppender in a secure manner (e.g. their format message does not include user-provided input), so I assumed it might be not that bad to just leave the fallback. On the other hand, if their format string is not supported by the new parser, adding quotes should be enough, so it might be just fine to drop insecure mode. There's a possibility that people used "table rotation" via insert into logs%d{yyyy}(...). This case is not supported by my fix, and I think it would be hard to support in a secure manner.
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
Frankly speaking, I think the added ParserState boilerplate (equals, hashcode, etc) negates all that. getCopyOfpatternStringRepresentationList looks crazy (naming is hard), so I would prefer simpler designs, especially for an internal (package-private) class like JdbcPatternParser. Then, JdbcPatternParser#toString() assertion ensured the object would look sane in the debugger. ---
I was not aware of that. It must be the mixed tab mode which is the default. Would "only spaces" be better?
That is why suggested .editorconfig and its indent_style = space. Reviewing changes is harder when the code layout is broken, and it is often broken when tabs and spaces are mixed in the same file. Vladimir
participants (2)
-
Ceki Gülcü -
Vladimir Sitnikov