Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 15, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ...c/test/java/software/amazon/lambda/powertools/logging/internal/LambdaJsonEncoderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@
// Make sure file is cleaned up before running tests
try {
FileChannel.open(Paths.get("target/logfile.json"), StandardOpenOption.WRITE).truncate(0).close();
} catch (NoSuchFileException e) {
// file may not exist on the first launch
}

Check failure on line 87 in powertools-logging/powertools-logging-logback/src/test/java/software/amazon/lambda/powertools/logging/internal/LambdaJsonEncoderTest.java

View workflow job for this annotation

GitHub Actions / pmd_analyse

Avoid empty catch blocks 

Empty Catch Block finds instances where an exception is caught, but nothing is done.
In most circumstances, this swallows an exception which should either be acted on
or reported.

EmptyCatchBlock (Priority: 1, Ruleset: Error Prone)
https://docs.pmd-code.org/pmd-doc-7.13.0/pmd_rules_java_errorprone.html#emptycatchblock
}

@AfterEach
Expand Down Expand Up @@ -127,7 +127,8 @@
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\"");
Expand Down Expand Up @@ -158,7 +159,8 @@
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\"");
Expand Down Expand Up @@ -295,7 +297,8 @@
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
Expand Down
36 changes: 17 additions & 19 deletions ...ging/src/main/java/software/amazon/lambda/powertools/logging/internal/JsonSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public JsonSerializer(StringBuilder builder) {
}
this.builder = builder;
}

public void writeStartArray() {
builder.append('[');
}
Expand Down Expand Up @@ -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("\"");
}
}

Expand Down Expand Up @@ -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()) {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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()) {
Expand All @@ -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(',');
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -595,5 +595,3 @@ public void close() {
// nothing to do
}
}


Loading