fix(logging): Escape double-quotes when serializing strings into JSON. (#1845)

Author: phipag

powertools-logging/powertools-logging-logback/src/test/java/software/amazon/lambda/powertools/logging/internal/LambdaJsonEncoderTest.java

@@ -127,7 +127,8 @@ void shouldLogArgumentsAsJsonWhenUsingRawJson() {
         assertThat(contentOf(logFile))
                 .contains("\"input\":{\"awsRegion\":\"eu-west-1\",\"body\":\"plop\",\"eventSource\":\"eb\",\"messageAttributes\":{\"keyAttribute\":{\"stringListValues\":[\"val1\",\"val2\",\"val3\"]}},\"messageId\":\"1212abcd\"}")
                 .contains("\"message\":\"1212abcd\"")
-                .contains("\"message\":\"Message body = plop and id = \"1212abcd\"\"");
+                // Should auto-escape double quotes around id
+                .contains("\"message\":\"Message body = plop and id = \\\"1212abcd\\\"\"");
         // Reserved keys should be ignored
         PowertoolsLoggedFields.stringValues().stream().forEach(reservedKey -> {
             assertThat(contentOf(logFile)).doesNotContain("\"" + reservedKey + "\":\"shouldBeIgnored\"");
@@ -158,7 +159,8 @@ void shouldLogArgumentsAsJsonWhenUsingKeyValue() {
         assertThat(contentOf(logFile))
                 .contains("\"input\":{\"awsRegion\":\"eu-west-1\",\"body\":\"plop\",\"eventSource\":\"eb\",\"messageAttributes\":{\"keyAttribute\":{\"stringListValues\":[\"val1\",\"val2\",\"val3\"]}},\"messageId\":\"1212abcd\"}")
                 .contains("\"message\":\"1212abcd\"")
-                .contains("\"message\":\"Message body = plop and id = \"1212abcd\"\"");
+                // Should auto-escape double quotes around id
+                .contains("\"message\":\"Message body = plop and id = \\\"1212abcd\\\"\"");
         // Reserved keys should be ignored
         PowertoolsLoggedFields.stringValues().stream().forEach(reservedKey -> {
             assertThat(contentOf(logFile)).doesNotContain("\"" + reservedKey + "\":\"shouldBeIgnored\"");
@@ -295,7 +297,8 @@ void shouldLogEventAsStringForStreamHandler() throws IOException {
         File logFile = new File("target/logfile.json");
         assertThat(contentOf(logFile))
                 .contains("\"message\":\"Handler Event\"")
-                .contains("\"event\":\"{\"key\":\"value\"}\""); // logged as String for StreamHandler
+                // logged as String for StreamHandler (should auto-escape double-quotes to avoid breaking JSON format)
+                .contains("\"event\":\"{\\\"key\\\":\\\"value\\\"}\"");
     }
 
     @Test

powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/JsonSerializer.java

@@ -53,7 +53,7 @@ public JsonSerializer(StringBuilder builder) {
         }
         this.builder = builder;
     }
-    
+
     public void writeStartArray() {
         builder.append('[');
     }
@@ -84,7 +84,8 @@ public void writeString(String text) {
         if (text == null) {
             writeNull();
         } else {
-            builder.append("\"").append(text).append("\"");
+            // Escape double quotes to avoid breaking JSON format
+            builder.append("\"").append(text.replace("\"", "\\\"")).append("\"");
         }
     }
 
@@ -314,7 +315,7 @@ public void writeMap(final Map<?, ?> map) {
             writeNull();
         } else {
             writeStartObject();
-            for (Iterator<? extends Map.Entry<?, ?>> entries = map.entrySet().iterator(); entries.hasNext(); ) {
+            for (Iterator<? extends Map.Entry<?, ?>> entries = map.entrySet().iterator(); entries.hasNext();) {
                 Map.Entry<?, ?> entry = entries.next();
                 writeObjectField(String.valueOf(entry.getKey()), entry.getValue());
                 if (entries.hasNext()) {
@@ -326,16 +327,16 @@ public void writeMap(final Map<?, ?> map) {
     }
 
     public void writeObject(Object value) {
-        
+
         // null
         if (value == null) {
             writeNull();
-        } 
-        
+        }
+
         else if (value instanceof String) {
             writeString((String) value);
-        } 
-        
+        }
+
         // number & boolean
         else if (value instanceof Number) {
             Number n = (Number) value;
@@ -364,24 +365,23 @@ else if (value instanceof Number) {
             writeBoolean((Boolean) value);
         } else if (value instanceof AtomicBoolean) {
             writeBoolean(((AtomicBoolean) value).get());
-        } 
-        
+        }
+
         // list & collection
         else if (value instanceof List) {
             final List<?> list = (List<?>) value;
             writeArray(list);
         } else if (value instanceof Collection) {
             final Collection<?> collection = (Collection<?>) value;
             writeArray(collection);
-        } 
-        
+        }
+
         // map
         else if (value instanceof Map) {
             final Map<?, ?> map = (Map<?, ?>) value;
             writeMap(map);
         }
 
-
         // arrays
         else if (value instanceof char[]) {
             final char[] charValues = (char[]) value;
@@ -411,7 +411,7 @@ else if (value instanceof char[]) {
             final Object[] values = (Object[]) value;
             writeArray(values);
         }
-        
+
         else if (value instanceof JsonNode) {
             JsonNode node = (JsonNode) value;
 
@@ -462,7 +462,7 @@ else if (value instanceof JsonNode) {
                 case OBJECT:
                 case POJO:
                     writeStartObject();
-                    for (Iterator<Map.Entry<String, JsonNode>> entries = node.fields(); entries.hasNext(); ) {
+                    for (Iterator<Map.Entry<String, JsonNode>> entries = node.fields(); entries.hasNext();) {
                         Map.Entry<String, JsonNode> entry = entries.next();
                         writeObjectField(entry.getKey(), entry.getValue());
                         if (entries.hasNext()) {
@@ -475,7 +475,7 @@ else if (value instanceof JsonNode) {
                 case ARRAY:
                     ArrayNode arrayNode = (ArrayNode) node;
                     writeStartArray();
-                    for (Iterator<JsonNode> elements = arrayNode.elements(); elements.hasNext(); ) {
+                    for (Iterator<JsonNode> elements = arrayNode.elements(); elements.hasNext();) {
                         writeObject(elements.next());
                         if (elements.hasNext()) {
                             builder.append(',');
@@ -558,7 +558,7 @@ public void writeTree(TreeNode rootNode) {
             writeNull();
         } else if (rootNode instanceof TextNode) {
             writeString(((TextNode) rootNode).asText());
-        } else if  (rootNode instanceof BooleanNode) {
+        } else if (rootNode instanceof BooleanNode) {
             writeBoolean(((BooleanNode) rootNode).asBoolean());
         } else if (rootNode instanceof NumericNode) {
             NumericNode numericNode = (NumericNode) rootNode;
@@ -595,5 +595,3 @@ public void close() {
         // nothing to do
     }
 }
-
-