Add compacting body filters #357

nsmolenskii · 2018-10-07T08:39:33Z

Description

Add body filters to compact most common content types

Motivation and Context

Custom HttpLogForrmatters (different from JsonHttpLogFormatter) have also need to minify message body. To reduce code duplication from formatter to formatter, it could be extracted as delegating body filters.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.

jhorstmann · 2018-10-07T10:09:19Z

logbook-core/src/main/java/org/zalando/logbook/XmlCompactingBodyFilter.java

+        }
+    }
+
+    private Document documentWithoutTextNodes(final String body) {


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.

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 .

I had applied recommended changes, according to XXE

whiskeysierra · 2018-10-08T07:13:27Z

To reduce code duplication from formatter to formatter, it could be extracted as delegating body filters.

But from what I see there is nothing shared, is there?

nsmolenskii · 2018-10-08T08:09:24Z

Well, it shared not as part of every single formatter, but as part of infrastructural body filters.
E.g.:

you have json http log formatter, but some of your endpoints are xml based, you will receive minified xml, as soon as xml compacting body filter will be configured.
you introduces another http log formatter (like add key value http logger #358) and you don't have to repeat logic to minify json or xml, or your custom format. It's only about json/xml compacting body filter have to be configured.

whiskeysierra · 2018-10-08T08:23:07Z

Well, it shared not as part of every single formatter, but as part of infrastructural body filters.

I failed to realize that you moved the compacting from the formatter to the filter.

We could think about removing it from the JSONHttpLogFormatter in favor of registering those two filters by default, but that would be a breaking change. Maybe something for 2.x

whiskeysierra · 2018-10-08T08:24:50Z

logbook-core/src/main/java/org/zalando/logbook/JsonCompactingBodyFilter.java

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


&& !jsonCompactor.isCompacted(body)?

whiskeysierra · 2018-10-08T08:25:54Z

logbook-core/src/main/java/org/zalando/logbook/XmlCompactingBodyFilter.java

+    }
+
+    /**
+     * @return configured {@link DocumentBuilderFactory} against XML External Entity (XXE).


Can you add the links provided by Jörn here as @see tags?

whiskeysierra · 2018-10-08T08:26:34Z

logbook-core/src/main/java/org/zalando/logbook/XmlCompactingBodyFilter.java

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


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

nsmolenskii · 2018-10-08T08:39:14Z

I failed to realize that you moved the compacting from the formatter to the filter.

We could think about removing it from the JSONHttpLogFormatter in favor of registering those two filters by default, but that would be a breaking change. Maybe something for 2.x

Due to backward compatibility (JsonHttpLogFormatter marked as STABLE), I was not going into that direction myself. But I also find it would be a proper change.

whiskeysierra · 2018-10-08T09:40:02Z

logbook-core/src/main/java/org/zalando/logbook/XmlCompactingBodyFilter.java

+    }
+
+    private boolean shouldCompact(final String body) {
+        return Stream.of("<?xml", "\n", "  ").anyMatch(body::contains);


The xml declaration is redundant and should be covered by the content type check, shouldn't it? Spaces would be generally ok, since the whole log message would still fit into a single line.

I'd just change this in the same way as the json compactor:

return body.indexOf('\n') != -1;

It's quite possible to have xml prolog inside of the message, but do not have \n inside.
So, to not check <?xml potentially means to keep it in compacted version (due to compact skip)

But if there are no newlines, what exactly would you expect to happen when compacting? Removing other whitespaces?

Well I got your point. We have different points of view to minimisation. I consider extra whitespaces and xml prolog as too verbose in logs. But I could leave with, cause single line is much more important

nsmolenskii · 2018-10-09T18:33:42Z

So do we have some open tasks, preventing that PR ?

whiskeysierra · 2018-10-11T14:36:11Z

I released your changes as 1.10.0. 🎉

add compacting body filters

b962a1b

nsmolenskii requested review from AlexanderYastrebov, lukasniemeier-zalando and whiskeysierra as code owners October 7, 2018 08:39

jhorstmann reviewed Oct 7, 2018

View reviewed changes

configure DocumentBuilderFactory to prevent xxe vulnerability

9b10a7d

nsmolenskii force-pushed the add-compacting-body-filters branch from 541b54b to 9b10a7d Compare October 7, 2018 16:33

nsmolenskii mentioned this pull request Oct 8, 2018

add key value http logger #358

Merged

6 tasks

whiskeysierra reviewed Oct 8, 2018

View reviewed changes

fix code review issues

2beb3ed

nsmolenskii force-pushed the add-compacting-body-filters branch from 04c271b to 2beb3ed Compare October 8, 2018 09:36

whiskeysierra reviewed Oct 8, 2018

View reviewed changes

whiskeysierra mentioned this pull request Oct 8, 2018

Remove compacting functionality from json formatter #361

Closed

simplify xml shouldCompact logic

f56f608

whiskeysierra approved these changes Oct 10, 2018

View reviewed changes

whiskeysierra merged commit 987e390 into zalando:master Oct 10, 2018

nsmolenskii deleted the add-compacting-body-filters branch October 11, 2018 06:03

whiskeysierra mentioned this pull request Nov 10, 2018

Pretty-print json body #372

Closed

whiskeysierra mentioned this pull request Nov 18, 2018

Logbook: JSON module #373

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compacting body filters #357

Add compacting body filters #357

nsmolenskii commented Oct 7, 2018 •

edited

Loading

jhorstmann Oct 7, 2018

nsmolenskii Oct 7, 2018

nsmolenskii Oct 7, 2018

whiskeysierra commented Oct 8, 2018

nsmolenskii commented Oct 8, 2018 •

edited

Loading

whiskeysierra commented Oct 8, 2018

whiskeysierra Oct 8, 2018

whiskeysierra Oct 8, 2018

whiskeysierra Oct 8, 2018

nsmolenskii commented Oct 8, 2018

whiskeysierra Oct 8, 2018

nsmolenskii Oct 8, 2018

whiskeysierra Oct 10, 2018

nsmolenskii Oct 10, 2018

nsmolenskii commented Oct 9, 2018

whiskeysierra commented Oct 11, 2018

Add compacting body filters #357

Add compacting body filters #357

Conversation

nsmolenskii commented Oct 7, 2018 • edited Loading

Description

Motivation and Context

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whiskeysierra commented Oct 8, 2018

nsmolenskii commented Oct 8, 2018 • edited Loading

whiskeysierra commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsmolenskii commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsmolenskii commented Oct 9, 2018

whiskeysierra commented Oct 11, 2018

nsmolenskii commented Oct 7, 2018 •

edited

Loading

nsmolenskii commented Oct 8, 2018 •

edited

Loading