From 5d69f8d6bc1769d880cc95b46c1b8cf050ed1215 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Willi=20Sch=C3=B6nborn?= Date: Fri, 10 Nov 2017 14:37:05 +0100 Subject: [PATCH] Handling of form requests is now configurable Fixes #94 Fixes #169 --- README.md | 16 ++++- .../logbook/servlet/FormRequestMode.java | 21 +++++++ .../logbook/servlet/RemoteRequest.java | 29 +++++++++ .../zalando/logbook/servlet/WritingTest.java | 63 +++++++++++++++++++ .../junit/RestoreSystemProperties.java | 16 +++++ .../junit/SystemPropertiesExtension.java | 27 ++++++++ 6 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 logbook-servlet/src/main/java/org/zalando/logbook/servlet/FormRequestMode.java create mode 100644 logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/RestoreSystemProperties.java create mode 100644 logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/SystemPropertiesExtension.java diff --git a/README.md b/README.md index 92cfe2ae7..29de5027b 100644 --- a/README.md +++ b/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 diff --git a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/FormRequestMode.java b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/FormRequestMode.java new file mode 100644 index 000000000..bde257de3 --- /dev/null +++ b/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)); + } + +} diff --git a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java index c07ea9680..327c2a204 100644 --- a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java +++ b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java @@ -13,6 +13,7 @@ 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; @@ -20,10 +21,15 @@ 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 ? diff --git a/logbook-servlet/src/test/java/org/zalando/logbook/servlet/WritingTest.java b/logbook-servlet/src/test/java/org/zalando/logbook/servlet/WritingTest.java index 877cf670c..5d60a3137 100644 --- a/logbook-servlet/src/test/java/org/zalando/logbook/servlet/WritingTest.java +++ b/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> captor = ArgumentCaptor.forClass( + Precorrelation.class); + verify(writer).writeRequest(captor.capture()); + final Precorrelation 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> captor = ArgumentCaptor.forClass( + Precorrelation.class); + verify(writer).writeRequest(captor.capture()); + final Precorrelation 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") diff --git a/logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/RestoreSystemProperties.java b/logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/RestoreSystemProperties.java new file mode 100644 index 000000000..2c082d2f1 --- /dev/null +++ b/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 { +} diff --git a/logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/SystemPropertiesExtension.java b/logbook-servlet/src/test/java/org/zalando/logbook/servlet/junit/SystemPropertiesExtension.java new file mode 100644 index 000000000..35b2c4164 --- /dev/null +++ b/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); + } + +}