ppkarwasz commented on PR #3855: URL: https://github.com/apache/logging-log4j2/pull/3855#issuecomment-3113194935
Hi @garydgregory, > TLDR: Let's bring this PR in and then... > > This PR is _small and focused_, which is a good thing. I am willing to volunteer to migrate the other manager code ((rolling) from IO to NIO _in a separate PR_ over the weekend probably, or at the very least initially from presenting `Path` instead/in addition to `String` in the API. I believe it's actually **too** narrowly scoped, to the point where it loses sight of the broader context. To be candid: would anyone upgrade Log4j just because the changelog says that `FileManager` gained a new `getPath()` method? Instead of merging this in isolation, I’d suggest a slightly broader approach. Tackling the rolling file appender is definitely hard (otherwise I wouldn’t have asked you in the first place!). I’ve actually got a local branch where I started migrating the [`o.a.l.l.core.appender.rolling.action.Action`](https://logging.apache.org/log4j/2.x/javadoc/log4j-core/org/apache/logging/log4j/core/appender/rolling/action/Action.html) to NIO but it still requires a lot of work. What would you say about: **Step 1: Fix `fileName` handling in the config layer** * Convert `fileName` from `String` to `Path` during configuration parsing (there’s already a `TypeConverters.PathConverter` that makes this easier). This would have immediate security and maintenance benefits. Every call to `new File(String)` or `Paths.get(String)` triggers a [`PATH_TRAVERSAL_IN`](https://find-sec-bugs.github.io/bugs.htm#PATH_TRAVERSAL_IN) warning in SpotBugs. By confining all `Path` creation to the appender builders, we make the codebase more robust and auditable from a security standpoint. * Expose the `Path`-based filename via the APIs of all relevant file appenders: `File`, `RandomFile`, and `MemoryMappedFile`. This change would provide real value worth highlighting in release notes. Would you be open to expand your PR to cover this step? **Step 2: Migrate rolling file appenders to NIO** * This is a larger task and can be handled in a follow-up PR or even after the release of Step 1. * I’d love to bundle that with an additional feature: protection against accidental file overwrites during rollover. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
