Responses inline. On 1/19/2022 11:22 AM, Vladimir Sitnikov wrote:
There's a case when users can override JDBCAppender, and override its flushBuffer method.
So removing the class would break "drop-in replacement" I would rather suggest doing the following: 1) Throw an exception from JDBCAppender#flushBuffer unless there's reload4.appender.jdbc.allow_insecure_sql_replace=true That would make the thing secure, and the people would have a migration plan
If JDBCAppender is removed thre would be a single exception thrown at config time. Throwing an exception within JDBCAppender#flushBuffer would cause many more exceptions to be thrown.
2) Having a secured JDBCApender makes sense a well, even if the users would need to adjust the configs. It is used a lot even in public GitHub repositories: https://github.com/search?l=Java+Properties&q=org.apache.log4j.jdbc.JDBCAppe... <https://github.com/search?l=Java+Properties&q=org.apache.log4j.jdbc.JDBCAppender&type=Code>
One of the ideas could be treating '%m' as ? (bind placeholder). That would automatically cover cases like https://github.com/Gitlishitong/bams/blob/806385d0f3d9dd1074f3436c2c70ab1e35... <https://github.com/Gitlishitong/bams/blob/806385d0f3d9dd1074f3436c2c70ab1e3555ff40/src/main/resources/log/database.properties#L15>
Then there is the case of MDC (%X), NCD (%x) and many other cases which we did not anticipate. I propose we solve the security issue first by removing JDBCAppender and circle back to creating a JDBCAppender based on PreparedStatement.
Vladimir
-- Ceki Gülcü Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch