This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch doc/string-concatenation in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 1b68b3de81a65bb2d22488b1f184d4884db6667a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Wed Mar 19 22:25:38 2025 +0100 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` --- src/site/antora/modules/ROOT/pages/manual/api.adoc | 2 ++ .../modules/ROOT/pages/manual/getting-started.adoc | 2 ++ ...=> api-best-practice-dont-mix-concat-and-params.adoc} | 14 ++++++-------- .../manual/api-best-practice-dont-use-string-concat.adoc | 16 +++++++++------- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/site/antora/modules/ROOT/pages/manual/api.adoc b/src/site/antora/modules/ROOT/pages/manual/api.adoc index 525c5d54e0..732ea5096a 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-exception-as-last-argument.adoc[] 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 9b6da47430..19c8a2d720 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-exception-as-last-argument.adoc[] 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-use-string-concat.adoc b/src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-mix-concat-and-params.adoc similarity index 61% copy from src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc copy to src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-mix-concat-and-params.adoc index 5759d12dc9..ca53a7eba0 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-mix-concat-and-params.adoc @@ -15,22 +15,20 @@ limitations under the License. //// -If you are using `String` concatenation while logging, you are doing something very wrong and dangerous! +If you are mixing `String` concatenation and parameterized 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}` +* [ ] 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("failed for user ID: " + userId); +/* BAD! */ LOGGER.info("User " + userId + " logged out: {}", reason); ---- * [x] Use message parameters + [source,java] ---- -/* GOOD */ LOGGER.info("failed for user ID `{}`", userId); +/* 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 5759d12dc9..82ccf6657c 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 @@ Imagine `userId` being provided by the user with the following content: ---- /* GOOD */ LOGGER.info("failed for user ID `{}`", userId); ---- + +* [x] Use message lambdas ++ +[source,java] +---- +/* GOOD */ LOGGER.info(() -> "failed for user ID: " + userId); +----
