Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LOG4J2-3211 - Remove Messge Lookups #623

Merged
merged 4 commits into from Dec 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -20,6 +20,7 @@
import java.util.List;
import java.util.Locale;

import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.plugins.Plugin;
Expand All @@ -38,25 +39,14 @@
@ConverterKeys({ "m", "msg", "message" })
@PerformanceSensitive("allocation")
public class MessagePatternConverter extends LogEventPatternConverter {

private static final String LOOKUPS = "lookups";
private static final String NOLOOKUPS = "nolookups";

private MessagePatternConverter() {
super("Message", "message");
}

private static boolean loadLookups(final String[] options) {
if (options != null) {
for (final String option : options) {
if (LOOKUPS.equalsIgnoreCase(option)) {
return true;
}
}
}
return false;
}

private static TextRenderer loadMessageRenderer(final String[] options) {
if (options != null) {
for (final String option : options) {
Expand Down Expand Up @@ -86,15 +76,11 @@ private static TextRenderer loadMessageRenderer(final String[] options) {
* @return instance of pattern converter.
*/
public static MessagePatternConverter newInstance(final Configuration config, final String[] options) {
boolean lookups = loadLookups(options);
String[] formats = withoutLookupOptions(options);
TextRenderer textRenderer = loadMessageRenderer(formats);
MessagePatternConverter result = formats == null || formats.length == 0
? SimpleMessagePatternConverter.INSTANCE
: new FormattedMessagePatternConverter(formats);
if (lookups && config != null) {
result = new LookupMessagePatternConverter(result, config);
}
if (textRenderer != null) {
result = new RenderingPatternConverter(result, textRenderer);
}
Expand All @@ -107,7 +93,9 @@ private static String[] withoutLookupOptions(final String[] options) {
}
List<String> results = new ArrayList<>(options.length);
for (String option : options) {
if (!LOOKUPS.equalsIgnoreCase(option) && !NOLOOKUPS.equalsIgnoreCase(option)) {
if (LOOKUPS.equalsIgnoreCase(option) || NOLOOKUPS.equalsIgnoreCase(option)) {
LOGGER.info("The {} option will be ignored. Message Lookups are no longer supported.", option);
} else {
results.add(option);
}
}
Expand Down Expand Up @@ -164,30 +152,6 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) {
}
}

private static final class LookupMessagePatternConverter extends MessagePatternConverter {
private final MessagePatternConverter delegate;
private final Configuration config;

LookupMessagePatternConverter(final MessagePatternConverter delegate, final Configuration config) {
this.delegate = delegate;
this.config = config;
}

/**
* {@inheritDoc}
*/
@Override
public void format(final LogEvent event, final StringBuilder toAppendTo) {
int start = toAppendTo.length();
delegate.format(event, toAppendTo);
int indexOfSubstitution = toAppendTo.indexOf("${", start);
if (indexOfSubstitution >= 0) {
config.getStrSubstitutor()
.replaceIn(event, toAppendTo, indexOfSubstitution, toAppendTo.length() - indexOfSubstitution);
}
}
}

private static final class RenderingPatternConverter extends MessagePatternConverter {

private final MessagePatternConverter delegate;
Expand Down
Expand Up @@ -22,7 +22,7 @@
import org.apache.logging.log4j.test.appender.ListAppender;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* See (LOG4J2-905) Ability to disable (date) lookup completely, compatibility issues with other libraries like camel.
Expand All @@ -38,7 +38,7 @@ public void testDateLookupInMessage(final LoggerContext context, @Named("List")
final String template = "${date:YYYY-MM-dd}";
context.getLogger(PatternLayoutLookupDateTest.class.getName()).info(template);
final String string = listAppender.getMessages().get(0);
assertFalse(string.contains(template), string);
assertTrue(string.contains(template), string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class and log4j-list-lookups.xml could be deleted.

}

}
Expand Up @@ -121,7 +121,7 @@ public void testLookup() {
.setMessage(msg).build();
final StringBuilder sb = new StringBuilder();
converter.format(event, sb);
assertEquals("bar", sb.toString(), "Unexpected result");
assertEquals("${foo}", sb.toString(), "Unexpected result");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the feature is gone, we can probably remove this test as it’s covered elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to remove any test for this release. Just change them to prove the change was made.

}

@Test
Expand Down
Expand Up @@ -67,7 +67,7 @@ public void testMessageReplacement() {
List<String> msgs = app.getMessages();
assertNotNull(msgs);
assertEquals(1, msgs.size(), "Incorrect number of messages. Should be 1 is " + msgs.size());
assertEquals("LoggerTest This is a test for Apache" + Strings.LINE_SEPARATOR, msgs.get(0));
assertEquals("LoggerTest This is a test for ${ctx:MyKey}" + Strings.LINE_SEPARATOR, msgs.get(0));
}

@Test
Expand Down
5 changes: 4 additions & 1 deletion src/changes/changes.xml
Expand Up @@ -29,10 +29,13 @@
- "update" - Change
- "remove" - Removed
-->
<release version="2.15.1" date="2021-12-11" description="GA Release 2.15.1">
<release version="2.16.0" date="2021-12-13" description="GA Release 2.16.0">
<action issue="LOG4J2-3208" dev="rgoers" type="fix">
Disable JNDI by default. Require log4j2.enableJndi to be set to true to allow JNDI.
</action>
<action issue="LOG4J2-3211" dev="rgoers" type="fix">
Completely remove support for Message Lookups.
</action>
</release>
<release version="2.15.0" date="2021-12-06" description="GA Release 2.15.0">
<!-- ADDS -->
Expand Down
23 changes: 8 additions & 15 deletions src/site/xdoc/manual/layouts.xml.vm
Expand Up @@ -1460,14 +1460,19 @@ WARN [main]: Message 2</pre>
<tr>
<td align="center">
<a name="PatternMessage"/>
<b>m</b>{lookups}{ansi}<br />
<b>msg</b>{lookups}{ansi}<br />
<b>message</b>{lookups}{ansi}
<b>m</b>{ansi}<br />
<b>msg</b>{ansi}<br />
<b>message</b>{ansi}
</td>
<td>
<p>
Outputs the application supplied message associated with the logging event.
</p>
<p>
From Log4j 2.16.0, support for lookups in log messages has been removed for security reasons.
Both the<code>{lookups}</code> and the <code>{nolookups}</code> options on the %m, %msg and %message
pattern are now ignored. If either is specified a message will be logged.
</p>
<!-- Copied and tweaked from Javadoc for org.apache.logging.log4j.core.pattern.JAnsiMessageRenderer -->
<p>
Add <code>{ansi}</code> to render messages with ANSI escape codes (requires JAnsi,
Expand Down Expand Up @@ -1497,18 +1502,6 @@ WARN [main]: Message 2</pre>
The call site can look like this:
</p>
<pre class="prettyprint linenums">logger.info("@|KeyStyle {}|@ = @|ValueStyle {}|@", entry.getKey(), entry.getValue());</pre>
<p>
Use <code>{lookups}</code> to log messages like <code>logger.info("Try ${esc.d}{date:YYYY-MM-dd}")</code>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to put a note here, something like this:

From Log4j 2.16.0, support for lookups in log messages has been removed for security reasons. Both the {lookups} and the {nolookups} options on the %m, %msg and %message pattern are now silently ignored.

using lookups, this will replace the date template <code>${esc.d}{date:YYYY-MM-dd}</code>
with an actual date. This can be confusing in many cases, and it's often both easier and
more obvious to handle the lookup in code.
This feature is disabled by default and the message string is logged untouched.
</p>
<p>
<b>Note: </b>Users are <b>STRONGLY</b> discouraged from using the lookups option. Doing so may allow uncontrolled user input
containing lookups to take unintended actions. In almost all cases the software developer can accomplish the same tasks
lookups perform directly in the application code.
</p>
</td>
</tr>
<tr>
Expand Down