Handling of form requests is now configurable

Fixes #94
Fixes #169

README.md

@@ -334,6 +334,20 @@ context.addFilter("LogbookFilter", new LogbookFilter(logbook))
     .addMappingForUrlPatterns(EnumSet.of(REQUEST, ASYNC, ERROR), true, "/*"); 
 ```
 
+The `LogbookFilter` will, by default, treat requests with a `application/x-www-form-urlencoded` body not different from
+any other request, i.e you will see the request body in the logs. The downside of this approach is that you won't be
+able to use any of the `HttpServletRequest.getParameter*(..)` methods. See issue [#94](../../issues/94) for some more
+details.
+
+As of Logbook 1.5.0, you can now specify one of three strategies that define how Logbook deals with this situation by
+using the `logbook.servlet.form-request` system property:
+
+| Value            | Pros                                                                              | Cons                                               |
+|------------------|-----------------------------------------------------------------------------------|----------------------------------------------------|
+| `body` (default) | Body is logged                                                                    | Downstream code can **not use `getParameter*()`**  |
+| `parameter`      | Body is logged (but it's reconstructed from parameters)                           | Downstream code can **not use `getInputStream()`** |
+| `off`            | Downstream code can decide whether to use `getInputStream()` or `getParameter*()` | Body is **not logged**                             |
+
 #### Security
 
 Secure applications usually need a slightly different setup. You should generally avoid logging unauthorized requests, especially the body, because it quickly allows attackers to flood your logfile — and, consequently, your precious disk space. Assuming that your application handles authorization inside another filter, you have two choices:
@@ -443,8 +457,6 @@ logbook:
 
 ## Known Issues
 
-The Logbook Servlet integration is **incompatible with incoming POST requests that use `application/x-www-form-urlencoded`** form parameters and use any of the `HttpServletRequest.getParameter*(..)` methods. See issue [#94](../../issues/94) for details.
-
 The Logbook HTTP Client integration is handling gzip-compressed response entities incorrectly if the interceptor runs before a decompressing interceptor. Since logging compressed contents is not really helpful it's advised to register the logbook interceptor as the last interceptor in the chain.
 
 ## Getting Help with Logbook

logbook-servlet/src/main/java/org/zalando/logbook/servlet/FormRequestMode.java

@@ -0,0 +1,21 @@
+package org.zalando.logbook.servlet;
+
+import javax.annotation.Nullable;
+
+import static java.util.Locale.ROOT;
+
+enum FormRequestMode {
+
+    BODY, PARAMETER, OFF;
+
+    static FormRequestMode fromProperties() {
+        @Nullable final String property = System.getProperty("logbook.servlet.form-request");
+
+        if (property == null) {
+            return BODY;
+        }
+
+        return FormRequestMode.valueOf(property.toUpperCase(ROOT));
+    }
+
+}

logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java

@@ -13,17 +13,23 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.nio.charset.Charset;
+import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Collections.list;
+import static java.util.stream.Collectors.joining;
 
 
 final class RemoteRequest extends HttpServletRequestWrapper implements RawHttpRequest, HttpRequest {
 
+    private static final byte[] EMPTY_BODY = new byte[0];
+
+    private final FormRequestMode formRequestMode = FormRequestMode.fromProperties();
+
     /**
      * Null until we successfully intercepted it.
      */
@@ -89,10 +95,33 @@ public Charset getCharset() {
 
     @Override
     public HttpRequest withBody() throws IOException {
+        if (isFormRequest()) {
+            switch (formRequestMode) {
+                case PARAMETER:
+                    this.body = reconstructBodyFromParameters();
+                    return this;
+                case OFF:
+                    this.body = EMPTY_BODY;
+                    return this;
+            }
+        }
+
         body = ByteStreams.toByteArray(super.getInputStream());
         return this;
     }
 
+    private boolean isFormRequest() {
+        return "application/x-www-form-urlencoded".equals(getContentType());
+    }
+
+    private byte[] reconstructBodyFromParameters() {
+        return getParameterMap().entrySet().stream()
+                .flatMap(entry -> Arrays.stream(entry.getValue())
+                        .map(value -> entry.getKey() + "=" + value))
+                .collect(joining("&"))
+                .getBytes(UTF_8);
+    }
+
     @Override
     public ServletInputStream getInputStream() throws IOException {
         return body == null ?

logbook-servlet/src/test/java/org/zalando/logbook/servlet/WritingTest.java

@@ -13,7 +13,9 @@
 import org.zalando.logbook.HttpLogWriter;
 import org.zalando.logbook.Logbook;
 import org.zalando.logbook.Precorrelation;
+import org.zalando.logbook.servlet.junit.RestoreSystemProperties;
 
+import javax.annotation.concurrent.NotThreadSafe;
 import java.io.IOException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -30,6 +32,8 @@
 /**
  * Verifies that {@link LogbookFilter} delegates to {@link HttpLogWriter} correctly.
  */
+@NotThreadSafe
+@RestoreSystemProperties
 public final class WritingTest {
 
     private final HttpLogFormatter formatter = spy(new ForwardingHttpLogFormatter(new DefaultHttpLogFormatter()));
@@ -74,6 +78,65 @@ void shouldLogRequest() throws Exception {
                         "Hello, world!"));
     }
 
+    @Test
+    void shouldNotLogFormRequest() throws Exception {
+        System.setProperty("logbook.servlet.form-request", "off");
+
+        mvc.perform(get("/api/sync")
+                .with(protocol("HTTP/1.1"))
+                .accept(MediaType.APPLICATION_JSON)
+                .header("Host", "localhost")
+                .contentType(MediaType.APPLICATION_FORM_URLENCODED)
+                .content("hello=world"));
+
+        @SuppressWarnings("unchecked") final ArgumentCaptor<Precorrelation<String>> captor = ArgumentCaptor.forClass(
+                Precorrelation.class);
+        verify(writer).writeRequest(captor.capture());
+        final Precorrelation<String> precorrelation = captor.getValue();
+
+        assertThat(precorrelation.getRequest(), startsWith("Incoming Request:"));
+        assertThat(precorrelation.getRequest(), endsWith(
+                "GET http://localhost/api/sync HTTP/1.1\n" +
+                        "Accept: application/json\n" +
+                        "Host: localhost\n" +
+                        "Content-Type: application/x-www-form-urlencoded"));
+    }
+
+    @Test
+    void shouldNotLogFormRequestBody() throws Exception {
+        System.setProperty("logbook.servlet.form-request", "body");
+        shouldLogFormRequest();
+    }
+
+    @Test
+    void shouldNotLogFormRequestParameter() throws Exception {
+        System.setProperty("logbook.servlet.form-request", "parameter");
+        shouldLogFormRequest();
+    }
+
+    private void shouldLogFormRequest() throws Exception {
+        mvc.perform(get("/api/sync")
+                .with(protocol("HTTP/1.1"))
+                .accept(MediaType.APPLICATION_JSON)
+                .header("Host", "localhost")
+                .contentType(MediaType.APPLICATION_FORM_URLENCODED)
+                .content("hello=world"));
+
+        @SuppressWarnings("unchecked") final ArgumentCaptor<Precorrelation<String>> captor = ArgumentCaptor.forClass(
+                Precorrelation.class);
+        verify(writer).writeRequest(captor.capture());
+        final Precorrelation<String> precorrelation = captor.getValue();
+
+        assertThat(precorrelation.getRequest(), startsWith("Incoming Request:"));
+        assertThat(precorrelation.getRequest(), endsWith(
+                "GET http://localhost/api/sync HTTP/1.1\n" +
+                        "Accept: application/json\n" +
+                        "Host: localhost\n" +
+                        "Content-Type: application/x-www-form-urlencoded\n" +
+                        "\n" +
+                        "hello=world"));
+    }
+
     @Test
     void shouldLogResponse() throws Exception {
         mvc.perform(get("/api/sync")

logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/RestoreSystemProperties.java

@@ -0,0 +1,16 @@
+package org.zalando.logbook.servlet.junit;
+
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.TYPE;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+@Target({ TYPE, METHOD })
+@Retention(RUNTIME)
+@ExtendWith(SystemPropertiesExtension.class)
+public @interface RestoreSystemProperties {
+}

logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/SystemPropertiesExtension.java

@@ -0,0 +1,27 @@
+package org.zalando.logbook.servlet.junit;
+
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.Extension;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+import java.util.Properties;
+
+public final class SystemPropertiesExtension
+        implements Extension, BeforeEachCallback, AfterEachCallback {
+
+    private final Properties original = new Properties();
+
+    @Override
+    public void beforeEach(final ExtensionContext context) throws Exception {
+        original.putAll(System.getProperties());
+    }
+
+    @Override
+    public void afterEach(final ExtensionContext context) throws Exception {
+        final Properties properties = System.getProperties();
+        properties.clear();
+        properties.putAll(original);
+    }
+
+}