Describe the bug
There are a number of instances where it is possible to the tracker to log data passed to it. This is typically seen as bad practice (even when the logs are "innocently" debug logs). Additional light has been shed on this practice with the recent Log4j 2 CVE-2021-44228.
Whilst the Java Tracker doesn't use log4j (at least directly, it uses the sfl4j facade), this CVE has highlighted that logging keys/values is generally something we shouldn't do. debug logging is fine, as that shouldn't be enabled in production and makes debugging far easier when keys/values are listed in the logs. I've listed some debug logging just so another pair of eyes can verify leaving them in seems correct.
To Reproduce
Steps to reproduce the behavior or code snippets that produce the issue.
Expected behavior
No properties passed into the Java Trackers functinos should be logged at info or higher.
Below are the logs I found. I'm not sure they all need fixing as some are debug level but listing here for completeness.
|
LOGGER.error("Could not process Map {} into JSON String: {}", map, e.getMessage()); |
|
LOGGER.error("Object {} could not be encoded: {}", o, e.getMessage()); |
Wrapped with null checks so probably fine, but perhaps printing the value seems pointless.
|
LOGGER.error("Invalid key detected: {}", key); |
Wrapped with null checks so probably fine, but perhaps printing the value seems pointless.
|
LOGGER.info("null or empty value detected: {}", value); |
|
LOGGER.debug("Adding new kv pair: {}->{}", key, value); |
|
LOGGER.debug("Adding new map: {}", map); |
|
LOGGER.debug("Adding new map: {}", map); |
Describe the bug
There are a number of instances where it is possible to the tracker to log data passed to it. This is typically seen as bad practice (even when the logs are "innocently" debug logs). Additional light has been shed on this practice with the recent Log4j 2 CVE-2021-44228.
Whilst the Java Tracker doesn't use log4j (at least directly, it uses the sfl4j facade), this CVE has highlighted that logging keys/values is generally something we shouldn't do.
debuglogging is fine, as that shouldn't be enabled in production and makes debugging far easier when keys/values are listed in the logs. I've listed some debug logging just so another pair of eyes can verify leaving them in seems correct.To Reproduce
Steps to reproduce the behavior or code snippets that produce the issue.
Expected behavior
No properties passed into the Java Trackers functinos should be logged at info or higher.
Below are the logs I found. I'm not sure they all need fixing as some are debug level but listing here for completeness.
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/Utils.java
Line 123 in 73cd5a4
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/Utils.java
Line 166 in 73cd5a4
Wrapped with null checks so probably fine, but perhaps printing the value seems pointless.
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/payload/TrackerPayload.java
Line 43 in 73cd5a4
Wrapped with null checks so probably fine, but perhaps printing the value seems pointless.
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/payload/TrackerPayload.java
Line 47 in 73cd5a4
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/payload/TrackerPayload.java
Line 50 in 73cd5a4
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/payload/TrackerPayload.java
Line 66 in 73cd5a4
snowplow-java-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/payload/TrackerPayload.java
Line 89 in 73cd5a4