Skip to content

Commit 1b68b3d

Browse files
committed
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`
1 parent 1a7112a commit 1b68b3d

File tree

4 files changed

+47
-7
lines changed

4 files changed

+47
-7
lines changed

src/site/antora/modules/ROOT/pages/manual/api.adoc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ include::partial$manual/api-best-practice-exception-as-last-argument.adoc[]
7878
7979
include::partial$manual/api-best-practice-dont-use-string-concat.adoc[]
8080
81+
include::partial$manual/api-best-practice-dont-mix-concat-and-params.adoc[]
82+
8183
[#best-practice-supplier]
8284
=== Use ``Supplier``s to pass computationally expensive arguments
8385

src/site/antora/modules/ROOT/pages/manual/getting-started.adoc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ include::partial$manual/api-best-practice-exception-as-last-argument.adoc[]
121121
122122
include::partial$manual/api-best-practice-dont-use-string-concat.adoc[]
123123
124+
include::partial$manual/api-best-practice-dont-mix-concat-and-params.adoc[]
125+
124126
[#install-app]
125127
== How do I install Log4j Core to run my **application**?
126128
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
////
2+
Licensed to the Apache Software Foundation (ASF) under one or more
3+
contributor license agreements. See the NOTICE file distributed with
4+
this work for additional information regarding copyright ownership.
5+
The ASF licenses this file to You under the Apache License, Version 2.0
6+
(the "License"); you may not use this file except in compliance with
7+
the License. You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
////
17+
18+
If you are mixing `String` concatenation and parameterized logging, you are doing something very wrong and dangerous!
19+
20+
* [ ] The format string of a parameterized statement should be a compile time constant!
21+
An attacker could mangle your logs by inserting `{}` placeholders in the values!
22+
Try these examples with `userId="{}\nbadUser"` and `reason="root logged in successfully"`
23+
+
24+
[source,java]
25+
----
26+
/* BAD! */ LOGGER.info("User " + userId + " logged out: {}", reason);
27+
----
28+
29+
* [x] Use message parameters
30+
+
31+
[source,java]
32+
----
33+
/* GOOD */ LOGGER.info("User {} logged out: {}", userId, reason);
34+
----

src/site/antora/modules/ROOT/partials/manual/api-best-practice-dont-use-string-concat.adoc

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,8 @@
1515
limitations under the License.
1616
////
1717
18-
If you are using `String` concatenation while logging, you are doing something very wrong and dangerous!
19-
20-
* [ ] Don't use `String` concatenation to format arguments!
21-
This circumvents the handling of arguments by message type and layout.
22-
More importantly, **this approach is prone to attacks!**
23-
Imagine `userId` being provided by the user with the following content:
24-
`placeholders for non-existing args to trigger failure: {} {} \{dangerousLookup}`
18+
* [ ] Don't use `String` concatenation to format arguments:
19+
the log message will be formatted, even if the logger is not enabled, and you will suffer a performance penalty!
2520
+
2621
[source,java]
2722
----
@@ -34,3 +29,10 @@ Imagine `userId` being provided by the user with the following content:
3429
----
3530
/* GOOD */ LOGGER.info("failed for user ID `{}`", userId);
3631
----
32+
33+
* [x] Use message lambdas
34+
+
35+
[source,java]
36+
----
37+
/* GOOD */ LOGGER.info(() -> "failed for user ID: " + userId);
38+
----

0 commit comments

Comments
 (0)