Improve String concatenation best practice
This change splits the "don't use String concatenation" best practice into two parts:
- A recommendation concerning performance: string concatenation is not efficient if the logger is off.
- A security recommendation: format string must be constants to prevent `{}` placeholder injection.
The `${dangerousLookup}` part of the example is removed, since lookups are not executed since version `2.15.0`
diff --git a/src/site/antora/modules/ROOT/pages/manual/api.adoc b/src/site/antora/modules/ROOT/pages/manual/api.adoc
index 525c5d5..732ea50 100644
--- a/src/site/antora/modules/ROOT/pages/manual/api.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/api.adoc
@@ -78,6 +78,8 @@
include::partial$manual/api-best-practice-dont-use-string-concat.adoc[]
+include::partial$manual/api-best-practice-dont-mix-concat-and-params.adoc[]
+
[#best-practice-supplier]
=== Use ``Supplier``s to pass computationally expensive arguments
diff --git a/src/site/antora/modules/ROOT/pages/manual/getting-started.adoc b/src/site/antora/modules/ROOT/pages/manual/getting-started.adoc
index 9b6da47..19c8a2d 100644
--- a/src/site/antora/modules/ROOT/pages/manual/getting-started.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/getting-started.adoc
@@ -121,6 +121,8 @@
include::partial$manual/api-best-practice-dont-use-string-concat.adoc[]
+include::partial$manual/api-best-practice-dont-mix-concat-and-params.adoc[]
+
[#install-app]
== How do I install Log4j Core to run my **application**?
diff --git a/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-mix-concat-and-params.adoc b/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-mix-concat-and-params.adoc
new file mode 100644
index 0000000..ca53a7e
--- /dev/null
+++ b/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-mix-concat-and-params.adoc
@@ -0,0 +1,34 @@
+////
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+////
+
+If you are mixing `String` concatenation and parameterized logging, you are doing something very wrong and dangerous!
+
+* [ ] The format string of a parameterized statement should be a compile time constant!
+An attacker could mangle your logs by inserting `{}` placeholders in the values!
+Try these examples with `userId="{}\nbadUser"` and `reason="root logged in successfully"`
++
+[source,java]
+----
+/* BAD! */ LOGGER.info("User " + userId + " logged out: {}", reason);
+----
+
+* [x] Use message parameters
++
+[source,java]
+----
+/* GOOD */ LOGGER.info("User {} logged out: {}", userId, reason);
+----
diff --git a/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc b/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc
index 5759d12..82ccf66 100644
--- a/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc
+++ b/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc
@@ -15,13 +15,8 @@
limitations under the License.
////
-If you are using `String` concatenation while logging, you are doing something very wrong and dangerous!
-
-* [ ] Don't use `String` concatenation to format arguments!
-This circumvents the handling of arguments by message type and layout.
-More importantly, **this approach is prone to attacks!**
-Imagine `userId` being provided by the user with the following content:
-`placeholders for non-existing args to trigger failure: {} {} \{dangerousLookup}`
+* [ ] Don't use `String` concatenation to format arguments:
+the log message will be formatted, even if the logger is not enabled, and you will suffer a performance penalty!
+
[source,java]
----
@@ -34,3 +29,10 @@
----
/* GOOD */ LOGGER.info("failed for user ID `{}`", userId);
----
+
+* [x] Use message lambdas
++
+[source,java]
+----
+/* GOOD */ LOGGER.info(() -> "failed for user ID: " + userId);
+----
