Skip to content
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.

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

Add compacting body filters #357

Merged
merged 4 commits into from
Oct 10, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 11 additions & 0 deletions logbook-core/src/main/java/org/zalando/logbook/BodyFilters.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.zalando.logbook;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.apiguardian.api.API;

import java.util.HashSet;
Expand Down Expand Up @@ -67,4 +68,14 @@ public static BodyFilter truncate(final int maxSize) {
return (contentType, body) -> body.length() <= maxSize ? body : body.substring(0, maxSize) + "...";
}

@API(status = EXPERIMENTAL)
public static BodyFilter compactJson(final ObjectMapper objectMapper) {
return new JsonCompactingBodyFilter(objectMapper);
}

@API(status = EXPERIMENTAL)
public static BodyFilter compactXml() {
return new XmlCompactingBodyFilter();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package org.zalando.logbook;

import com.fasterxml.jackson.databind.ObjectMapper;
import lombok.extern.slf4j.Slf4j;

import javax.annotation.Nullable;
import java.io.IOException;
import java.util.function.Predicate;


@Slf4j
class JsonCompactingBodyFilter implements BodyFilter {

private final JsonCompactor jsonCompactor;
private final JsonHeuristic heuristic = new JsonHeuristic();
private final Predicate<String> contentTypes = MediaTypeQuery.compile("application/json", "application/*+json");

JsonCompactingBodyFilter(final ObjectMapper objectMapper) {
jsonCompactor = new JsonCompactor(objectMapper);
}

@Override
public String filter(@Nullable final String contentType, final String body) {
return shouldCompact(contentType, body) ? compact(body) : body;
}

private boolean shouldCompact(@Nullable final String contentType, final String body) {
return contentTypes.test(contentType) && heuristic.isProbablyJson(body);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& !jsonCompactor.isCompacted(body)?

}

private String compact(final String body) {
try {
return jsonCompactor.compact(body);
} catch (final IOException e) {
log.trace("Unable to compact body, is it a JSON?. Keep it as-is: `{}`", e.getMessage());
return body;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package org.zalando.logbook;

import lombok.extern.slf4j.Slf4j;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

import javax.annotation.Nullable;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;
import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathFactory;
import java.io.ByteArrayInputStream;
import java.io.StringWriter;
import java.util.function.Predicate;

import static javax.xml.transform.OutputKeys.INDENT;
import static javax.xml.transform.OutputKeys.OMIT_XML_DECLARATION;
import static javax.xml.xpath.XPathConstants.NODESET;
import static org.zalando.fauxpas.FauxPas.throwingSupplier;

@Slf4j
class XmlCompactingBodyFilter implements BodyFilter {

private final Predicate<String> contentTypes = MediaTypeQuery.compile("*/xml", "*/*+xml");
private final Transformer transformer = transformerFactory();

@Override
public String filter(@Nullable final String contentType, final String body) {
return contentTypes.test(contentType) ? compact(body) : body;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we cheaply detect whether the body may not even need compacting, e.g. when it doesn't contain a new line?

}

private String compact(final String body) {
if (body.trim().isEmpty()) return body;
try {
final StringWriter output = new StringWriter();
final Document document = documentWithoutTextNodes(body);
transformer.transform(new DOMSource(document), new StreamResult(output));
return output.toString();
} catch (Exception e) {
log.trace("Unable to compact body, is it a XML?. Keep it as-is: `{}`", e.getMessage());
return body;
}
}

private Document documentWithoutTextNodes(final String body) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems rather inefficient, a custom sax or stax parser should have much less overhead.

There are also some security implication when parsing the request payload, see https://en.wikipedia.org/wiki/XML_external_entity_attack and https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#JAXP_DocumentBuilderFactory.2C_SAXParserFactory_and_DOM4J for how to prevent this in java.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with a fact, that sax/stax parsing will be more effective than dom in general.
Unfortunately, potential benefit from introducing custom parser with supporting rules will be miserable, due to fact of parsing already allocated in memory string .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had applied recommended changes, according to XXE

try {
final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
final Document document = factory.newDocumentBuilder().parse(new ByteArrayInputStream(body.getBytes()));

final XPathFactory xPathFactory = XPathFactory.newInstance();
final XPath xpath = xPathFactory.newXPath();
final NodeList empty = (NodeList) xpath.evaluate("//text()[normalize-space(.) = '']", document, NODESET);
for (int i = 0; i < empty.getLength(); i++) {
final Node node = empty.item(i);
node.getParentNode().removeChild(node);
}
return document;
} catch (Exception e) {
throw new IllegalArgumentException("Can not parse document", e);
}
}

private Transformer transformerFactory() {
final TransformerFactory factory = TransformerFactory.newInstance();
final Transformer transformer = throwingSupplier(factory::newTransformer).get();
transformer.setOutputProperty(INDENT, "no");
transformer.setOutputProperty(OMIT_XML_DECLARATION, "yes");
return transformer;
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package org.zalando.logbook;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Test;

import java.util.Collections;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;

public final class BodyFiltersTest {
Expand Down Expand Up @@ -72,4 +74,17 @@ void shouldNotTruncateBodyIfTooShort() {
assertThat(actual, is("{\"foo\":\"secret\"}"));
}

@Test
void shouldReturnJsonCompactingBodyFilter() {
final BodyFilter bodyFilter = BodyFilters.compactJson(new ObjectMapper());

assertThat(bodyFilter, instanceOf(JsonCompactingBodyFilter.class));
}

@Test
void shouldReturnXmlCompactingBodyFilter() {
final BodyFilter bodyFilter = BodyFilters.compactXml();

assertThat(bodyFilter, instanceOf(XmlCompactingBodyFilter.class));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package org.zalando.logbook;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.UUID;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

class JsonCompactingBodyFilterTest {

private JsonCompactingBodyFilter bodyFilter;

/*language=JSON*/
private final String prettifiedJson = "{\n" +
" \"root\": {\n" +
" \"child\": \"text\"\n" +
" }\n" +
"}";

/*language=JSON*/
private final String minimisedJson = "{\"root\":{\"child\":\"text\"}}";

@BeforeEach
void setUp() {
bodyFilter = new JsonCompactingBodyFilter(new ObjectMapper());
}

@Test
void shouldIgnoreEmptyBody() {
final String filtered = bodyFilter.filter("application/json", "");
assertThat(filtered, is(""));
}

@Test
void shouldIgnoreInvalidContent() {
final String invalidBody = UUID.randomUUID().toString();
final String filtered = bodyFilter.filter("application/json", invalidBody);
assertThat(filtered, is(invalidBody));
}

@Test
void shouldIgnoreInvalidContentType() {
final String filtered = bodyFilter.filter("text/plain", prettifiedJson);
assertThat(filtered, is(prettifiedJson));
}

@Test
void shouldTransformValidJsonRequestWithSimpleContentType() {
final String filtered = bodyFilter.filter("application/json", prettifiedJson);
assertThat(filtered, is(minimisedJson));
}

@Test
void shouldTransformValidJsonRequestWithCompatibleContentType() {
final String filtered = bodyFilter.filter("application/custom+json", prettifiedJson);
assertThat(filtered, is(minimisedJson));
}

@Test
void shouldSkipInvalidJsonLookingLikeAValidOne() {
final String invalidJson = "{invalid}";
final String filtered = bodyFilter.filter("application/custom+json", invalidJson);
assertThat(filtered, is(invalidJson));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ void shouldMatchAllMatch() {
deny("text/plain", "application/json");
allow("*/*", "text/plain");
allow("text/*", "text/plain");
allow("*/plain", "text/plain");
allow("text/plain", "text/plain");
allow("text/plain", "text/plain;charset=UTF-8");
allow("text/plain;charset=UTF-8", "text/plain"); // TODO should deny
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package org.zalando.logbook;


import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.UUID;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

class XmlCompactingBodyFilterTest {

private XmlCompactingBodyFilter bodyFilter;

/*language=XML*/
private final String prettifiedXml = "" +
"<?xml version=\"1.0\"?>" +
"<root>\n\n" +
" <child>text</child>\n" +
"</root>";

/*language=XML*/
private final String minimisedXml = "<root><child>text</child></root>";

@BeforeEach
void setUp() {
bodyFilter = new XmlCompactingBodyFilter();
}

@Test
void shouldIgnoreEmptyBody() {
final String filtered = bodyFilter.filter("application/xml", "");
assertThat(filtered, is(""));
}

@Test
void shouldIgnoreInvalidContent() {
final String invalidBody = UUID.randomUUID().toString();
final String filtered = bodyFilter.filter("application/xml", invalidBody);
assertThat(filtered, is(invalidBody));
}

@Test
void shouldIgnoreInvalidContentType() {
final String filtered = bodyFilter.filter("text/plain", prettifiedXml);
assertThat(filtered, is(prettifiedXml));
}

@Test
void shouldTransformValidXmlRequestWithSimpleContentType() {
final String filtered = bodyFilter.filter("application/xml", prettifiedXml);
assertThat(filtered, is(minimisedXml));
}

@Test
void shouldTransformValidXmlRequestWithCompatibleContentType() {
final String filtered = bodyFilter.filter("application/custom+xml", prettifiedXml);
assertThat(filtered, is(minimisedXml));
}

}