Hi Ralph,
On Thu, 18 Jul 2024 at 01:34, Ralph Goers <ralph.go...@dslextreme.com> wrote:
> > Although `$${date:...}` and `%d{...}` are not equivalent for
> > `filePattern`, I don't see a reason for users to use the former.
> > A file pattern like `$${date:yyyy-MM}/%d{yyyy-MM-dd}.log` that I often
> > see in user questions, simply doesn't do what the user wants. The
> > `2024-06-30.log` file ends up in the `2024-07` folder!
> > The user should use `%d{yyyy-MM}/%d{yyyy-MM-dd}.log`, which works
> > correctly since 2012 according to Git.
>
> Really? It only has a 50% chance of being correct as only one of those can be
> used to determine the rollover interval. As I said, this should be changed so
> that the rollover interval is in a separate attribute.
Not sure if it was intentional or not, but the frequency is always
determined using the last date pattern. Since that pattern is usually
the most specific, this works for some 99% of users.
I agree with the change, I'll make a PR after 2.24.0 is out.
> > If we exclude RoutingAppender, most lookups are used at configuration
> > time. Less than 10 attributes accept lookups at runtime. I have
> > documented them all recently:
> >
> > https://logging.staged.apache.org/log4j/2.x/manual/configuration.html#lazy-property-substitution
>
> Any sentence that starts with “If we exclude…” means you want to break those
> things being excluded.
Yes, it is a breaking change that we could consider for Log4j 4 (not 3).
I find the syntax of `RoutingAppender` quite counterintuitive: you put
something like `$${ctx:myKey}` in the pattern and then you need to use
`${ctx:myKey}` in the lazily evaluated children.
Maybe we could use `%X{myKey}` in the pattern and then something like
`${routing.key}` in the children. Just an idea.
> >> I am not a fan of making either of these pluggable. Making StrSubstitutor
> >> pluggable is a security risk. PropertiesUtil is pluggable by way of custom
> >> PropertySources.
> >
> > It is a security risk, but it is not **our** security risk.
> > If we make `StrSubstitutor` pluggable and Spring decides to use its
> > SpEL processor to evaluate configuration attributes, it is up to them
> > to make sure it is secure.
>
> Uhh, no. It will be OUR security risk for opening this up to allow people to
> do stupid stuff (just like we did).
Can you explain why is it our security risk?
IMHO if we use `ServiceLoader` to retrieve an alternative
`StrSubstitutor` and that substitutor has a vulnerability (e.g.
recursive evaluation), it is up to the alternative implementation to
issue a CVE and fix it.
If the implementation is based on something well-established (Spring
property replacement, SpEL, Jakarta EL), the problem will come out in
other libraries, before someone realizes, they can use it in Log4j
Core.
Piotr
