[GitHub] [logging-log4j2] remkop commented on pull request #630: Log4j2 is still vulnerable and underspecified. This updates documenta…

[GitBox <gi...@apache.org>]

remkop commented on pull request #630:
URL: https://github.com/apache/logging-log4j2/pull/630#issuecomment-997062659


   Thank you for your analysis and writeup.
   
   I can understand how there can be potential confusion between a call to `logger.debug(String)`, `logger.debug(Object)` and `logger.debug(Message)`. 
   
   It is key to understand that internally, Log4j processes `Message` objects. 
   
   - So, if an application passes in a String, Log4j will turn this into a Message, which becomes one of the properties of the LogEvent which is then further processed.
   - If an application passes in a Message object, great, there is no need to turn it into a Message, Log4j can just put it in the LogEvent and process that.
   - If an application passes in an Object, Log4j check if it is already a Message, or whether it needs to be wrapped in a Message, before putting it in the LogEvent and process that.
   
   Your comment mentions the following:
   
   > Currently, function documentation in Logger.java refers to the Message object: "the message string to be logged". This is a dangerous way to refer to the Message object.
   
   I am not convinced about the danger, but I agree that this javadoc is factually incorrect. However, your PR does not address this. In the PR, the Javadoc is now `@param msg the Message string to be logged`, so the word "Message" is capitalized, but it still says "string" where it should say object.
   
   Also, the proposed Javadoc in the PR now looks like this:
   `@param message the message CharSequence to log. THIS MAY OR MAY NOT BE INTERPRETED AS A MESSAGE OBJECT`
   If you want your PR to be merged, please improve this. This is not helpful text for the readers of the documentation.
   I would expect anyone contributing a PR to read the code, figure out whether it is interpreted as a `Message` object or not, and under which circumstances, and then document that. That would be a helpful improvement of the documentation for users of the library. As it stands, this cannot be merged.
   
   > Rename the "Message" class to "InterpretedMessage"
   
   Regardless of whether there is actually a problem here, this proposed solution is a non-starter, because it would break backwards compatibility. `Message` is an interface with many different implementations. It cannot be renamed. 
   Furthermore, from a design point of view, it is not the responsibility of the `Message` interface to determine whether implementations should interpolate embedded lookups or not.
   
   > Specify which inputs are interpreted or not
   
   You buried the lede a bit, but I think this is your key point.
   (That said, I am still not 100% sure what you mean by "interpreted". I will take "interpreted" to mean that "lookup strings embedded in the message string are interpolated". Please correct me if I misunderstood.)
   
   The short answer is, "it depends". 
   
   Whether lookup strings embedded in the message string are interpreted or not depends on the configuration, and therefore cannot be specified in the API documentation. 
   
   Some layouts do not interpret lookups. PatternLayout does. Also, this behaviour can be controlled with system properties and environment variables. There may be some flaws in the implementation of this control mechanism, but the principle remains that this behaviour is configurable and cannot be specified in the API documentation.
   
   Again, thank you for your analysis and writeup.
   The design of Log4j is that much of the behaviour is controlled via configuration and can therefore not be specified in the API documentation.


-- 
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-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org