Repository: zalando/logbook
Pull request #49 (2016-02-22 11:14:57)

Don't fail on invalid HTTP request targets
**Problem:**
When broken or malicious clients issue HTTP requests with invalid request targets (e.g. wrongly escaped query parameters) in the HTTP Request Line, logbook fails by throwing a `IllegalArgumentException`/`URISyntaxException`. This results in unlogged HTTP requests and most probably in `500 Internal Server Error` responses from HTTP backends.

 **Proposed fix:**
Don't use `java.net.URI` as return type of `BaseHttpRequest::getRequestUri`, but a plain `String`. Implementation was straight forward, with one exception: Query Parameter Obfuscation. If the request URI is malformed, query parameters cannot be extracted in the usual way in order to apply the `Obfuscator` to them. Currently, malformed URIs are then forwarded as-is. This may not be desirable. What do you think?


Patch:
diff --git a/logbook-core/src/main/java/org/zalando/logbook/BaseHttpRequest.java b/logbook-core/src/main/java/org/zalando/logbook/BaseHttpRequest.java
index b068b77b..d5483b2b 100644
--- a/logbook-core/src/main/java/org/zalando/logbook/BaseHttpRequest.java
+++ b/logbook-core/src/main/java/org/zalando/logbook/BaseHttpRequest.java
@@ -20,8 +20,6 @@ package org.zalando.logbook;
  * #L%
  */
 
-import java.net.URI;
-
 public interface BaseHttpRequest {
 
     String getRemote();
@@ -31,8 +29,10 @@ public interface BaseHttpRequest {
     /**
      * Request URI including query string.
      *
-     * @return the requested URI
+     * <p>Note that the URI may be invalid if the client issued an HTTP request using a malformed URL.</p>
+     *
+     * @return  the requested URI
      */
-    URI getRequestUri();
+    String getRequestUri();
 
 }
diff --git a/logbook-core/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java b/logbook-core/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
index d48f3866..51c65503 100644
--- a/logbook-core/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
+++ b/logbook-core/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
@@ -4,7 +4,7 @@ package org.zalando.logbook;
  * #%L
  * Logbook: Core
  * %%
- * Copyright (C) 2015 Zalando SE
+ * Copyright (C) 2015 - 2016 Zalando SE
  * %%
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,8 +20,6 @@ package org.zalando.logbook;
  * #L%
  */
 
-import java.net.URI;
-
 public abstract class ForwardingHttpRequest extends ForwardingHttpMessage implements HttpRequest {
 
     @Override
@@ -38,7 +36,7 @@ public abstract class ForwardingHttpRequest extends ForwardingHttpMessage implem
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         return delegate().getRequestUri();
     }
 
diff --git a/logbook-core/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java b/logbook-core/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
index d0e75e37..a1857cc8 100644
--- a/logbook-core/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
+++ b/logbook-core/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
@@ -23,7 +23,6 @@ package org.zalando.logbook;
 import com.google.common.collect.ForwardingObject;
 
 import java.io.IOException;
-import java.net.URI;
 
 public abstract class ForwardingRawHttpRequest extends ForwardingObject implements RawHttpRequest {
 
@@ -46,7 +45,7 @@ public abstract class ForwardingRawHttpRequest extends ForwardingObject implemen
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         return delegate().getRequestUri();
     }
 
diff --git a/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java b/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java
index 7effdd4b..090ab5c4 100644
--- a/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java
+++ b/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java
@@ -28,6 +28,7 @@ import com.google.gag.annotation.remark.OhNoYouDidnt;
 import javax.annotation.Nullable;
 import java.io.IOException;
 import java.net.URI;
+import java.net.URISyntaxException;
 
 import static com.google.common.collect.Multimaps.transformEntries;
 
@@ -52,9 +53,19 @@ final class ObfuscatedHttpRequest extends ForwardingHttpRequest {
     }
 
     @Override
-    public URI getRequestUri() {
-        final URI requestUri = super.getRequestUri();
-        final QueryParameters parameters = QueryParameters.parse(requestUri.getQuery());
+    public String getRequestUri() {
+        final String requestUri = super.getRequestUri();
+
+        final URI parsedUri;
+        try {
+            parsedUri = new URI(requestUri);
+        } catch (final URISyntaxException invalid) {
+            // It's an invalid URI, so the parameters
+            // cannot be extracted for obfuscation.
+            return requestUri;
+        }
+
+        final QueryParameters parameters = QueryParameters.parse(parsedUri.getQuery());
 
         if (parameters.isEmpty()) {
             return requestUri;
@@ -62,7 +73,7 @@ final class ObfuscatedHttpRequest extends ForwardingHttpRequest {
 
         final String queryString = parameters.obfuscate(parameterObfuscator).toString();
 
-        return createUri(requestUri, queryString);
+        return createUri(parsedUri, queryString).toASCIIString();
     }
 
     @VisibleForTesting
diff --git a/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/Request.java b/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/Request.java
index c687ccd2..412c95db 100644
--- a/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/Request.java
+++ b/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/Request.java
@@ -48,7 +48,7 @@ final class Request implements RawHttpRequest, org.zalando.logbook.HttpRequest {
 
     private final HttpRequest request;
     private final Localhost localhost;
-    
+
     private byte[] body;
 
     Request(final HttpRequest request, final Localhost localhost) {
@@ -71,14 +71,14 @@ final class Request implements RawHttpRequest, org.zalando.logbook.HttpRequest {
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         final HttpRequest original = request instanceof HttpRequestWrapper ?
                 HttpRequestWrapper.class.cast(request).getOriginal() :
                 request;
 
         return original instanceof HttpUriRequest ?
-                HttpUriRequest.class.cast(original).getURI():
-                URI.create(request.getRequestLine().getUri());
+                HttpUriRequest.class.cast(original).getURI().toASCIIString():
+                request.getRequestLine().getUri();
     }
 
     @Override
@@ -125,7 +125,7 @@ final class Request implements RawHttpRequest, org.zalando.logbook.HttpRequest {
         } else {
             this.body = new byte[0];
         }
-        
+
         return this;
     }
 
diff --git a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/TeeRequest.java b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/TeeRequest.java
index f475103d..4d45c29d 100644
--- a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/TeeRequest.java
+++ b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/TeeRequest.java
@@ -66,10 +66,10 @@ final class TeeRequest extends HttpServletRequestWrapper implements RawHttpReque
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         final String uri = getRequestURL().toString();
         @Nullable final String queryString = getQueryString();
-        return URI.create(queryString == null ? uri : uri + "?" + queryString);
+        return queryString == null ? uri : uri + "?" + queryString;
     }
 
     @Override
diff --git a/pom.xml b/pom.xml
index ad42615a..1a7ab739 100644
--- a/pom.xml
+++ b/pom.xml
@@ -225,7 +225,7 @@
             <dependency>
                 <groupId>com.github.stefanbirkner</groupId>
                 <artifactId>system-rules</artifactId>
-                <version>1.12.1</version>
+                <version>1.16.0</version>
                 <scope>test</scope>
                 <exclusions>
                     <exclusion>


Test patch:
diff --git a/logbook-core/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java b/logbook-core/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
index 881120a2..064020f2 100644
--- a/logbook-core/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
+++ b/logbook-core/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
@@ -27,7 +27,6 @@ import java.io.IOException;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.emptyString;
-import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 
@@ -39,17 +38,17 @@ public final class ForwardingHttpRequestTest {
             return MockHttpRequest.create();
         }
     };
-    
+
     @Test
     public void shouldDelegate() throws IOException {
         assertThat(unit.getRemote(), is("127.0.0.1"));
         assertThat(unit.getMethod(), is("GET"));
-        assertThat(unit.getRequestUri(), hasToString("http://localhost/"));
+        assertThat(unit.getRequestUri(), is("http://localhost/"));
         assertThat(unit.getHeaders().values(), is(empty()));
         assertThat(unit.getContentType(), is(""));
         assertThat(unit.getCharset(), is(UTF_8));
         assertThat(unit.getBody(), is("".getBytes(UTF_8)));
         assertThat(unit.getBodyAsString(), is(emptyString()));
     }
-    
+
 }
\ No newline at end of file
diff --git a/logbook-core/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java b/logbook-core/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
index f2aff2b9..6991a45a 100644
--- a/logbook-core/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
+++ b/logbook-core/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
@@ -27,7 +27,6 @@ import java.io.IOException;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.emptyString;
-import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 
@@ -46,7 +45,7 @@ public final class ForwardingRawHttpRequestTest {
     public void shouldDelegate() throws IOException {
         assertThat(unit.getRemote(), is("127.0.0.1"));
         assertThat(unit.getMethod(), is("GET"));
-        assertThat(unit.getRequestUri(), hasToString("http://localhost/"));
+        assertThat(unit.getRequestUri(), is("http://localhost/"));
     }
 
     @Test
@@ -55,7 +54,7 @@ public final class ForwardingRawHttpRequestTest {
 
         assertThat(request.getRemote(), is("127.0.0.1"));
         assertThat(request.getMethod(), is("GET"));
-        assertThat(request.getRequestUri(), hasToString("http://localhost/"));
+        assertThat(request.getRequestUri(), is("http://localhost/"));
         assertThat(request.getHeaders().values(), is(empty()));
         assertThat(request.getContentType(), is(""));
         assertThat(request.getCharset(), is(UTF_8));
diff --git a/logbook-core/src/test/java/org/zalando/logbook/MockHttpRequest.java b/logbook-core/src/test/java/org/zalando/logbook/MockHttpRequest.java
index 1516d74c..7156faee 100644
--- a/logbook-core/src/test/java/org/zalando/logbook/MockHttpRequest.java
+++ b/logbook-core/src/test/java/org/zalando/logbook/MockHttpRequest.java
@@ -27,7 +27,6 @@ import lombok.Builder;
 import lombok.Singular;
 
 import javax.annotation.Nullable;
-import java.net.URI;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
@@ -39,7 +38,7 @@ public final class MockHttpRequest implements HttpRequest {
 
     private final String remote;
     private final String method;
-    private final URI requestUri;
+    private final String requestUri;
     private final Map<String, String> headers;
     private final String contentType;
     private final Charset charset;
@@ -55,7 +54,7 @@ public final class MockHttpRequest implements HttpRequest {
             @Nullable final String body) {
         this.remote = firstNonNull(remote, "127.0.0.1");
         this.method = firstNonNull(method, "GET");
-        this.requestUri = URI.create(firstNonNull(requestUri, "http://localhost/"));
+        this.requestUri = firstNonNull(requestUri, "http://localhost/");
         this.headers = firstNonNullNorEmpty(headers, ImmutableMap.of());
         this.contentType = firstNonNull(contentType, "");
         this.charset = firstNonNull(charset, StandardCharsets.UTF_8);
@@ -77,7 +76,7 @@ public final class MockHttpRequest implements HttpRequest {
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         return requestUri;
     }
 
diff --git a/logbook-core/src/test/java/org/zalando/logbook/MockRawHttpRequest.java b/logbook-core/src/test/java/org/zalando/logbook/MockRawHttpRequest.java
index bb4f5d6b..55bed83b 100644
--- a/logbook-core/src/test/java/org/zalando/logbook/MockRawHttpRequest.java
+++ b/logbook-core/src/test/java/org/zalando/logbook/MockRawHttpRequest.java
@@ -26,7 +26,6 @@ import lombok.NoArgsConstructor;
 import lombok.Value;
 
 import java.io.IOException;
-import java.net.URI;
 
 @Value
 @Getter
@@ -36,14 +35,14 @@ public class MockRawHttpRequest implements RawHttpRequest {
 
     private String remote = "127.0.0.1";
     private String method = "GET";
-    private URI requestUri = URI.create("http://localhost/");
+    private String requestUri = "http://localhost/";
 
     @Override
     public HttpRequest withBody() throws IOException {
         return MockHttpRequest.builder()
                 .remote(remote)
                 .method(method)
-                .requestUri(requestUri.toASCIIString())
+                .requestUri(requestUri)
                 .build();
     }
 
diff --git a/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java b/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java
index ec36bd07..626714b6 100644
--- a/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java
+++ b/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java
@@ -28,7 +28,6 @@ import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasEntry;
-import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 
@@ -44,6 +43,20 @@ public final class ObfuscatedHttpRequestTest {
             Obfuscator.obfuscate("password"::equalsIgnoreCase, "unknown"),
             (contentType, body) -> body.replace("s3cr3t", "f4k3"));
 
+    @Test
+    public void shouldNotFailOnInvalidUri() {
+        final String invalidUri = "/af.cgi?_browser_out=.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2Fetc%2Fpasswd";
+        final ObfuscatedHttpRequest invalidRequest = new ObfuscatedHttpRequest(
+                MockHttpRequest.builder()
+                               .requestUri(invalidUri)
+                               .build(),
+                Obfuscator.none(),
+                Obfuscator.obfuscate("_browser_out"::equalsIgnoreCase, "unknown"),
+                BodyObfuscator.none());
+
+        assertThat(invalidRequest.getRequestUri(), is(invalidUri));
+    }
+
     @Test
     public void shouldObfuscateAuthorizationHeader() {
         assertThat(unit.getHeaders().asMap(), hasEntry(equalTo("Authorization"), contains("XXX")));
@@ -61,17 +74,17 @@ public final class ObfuscatedHttpRequestTest {
                 Obfuscator.obfuscate(x -> true, "*"),
                 BodyObfuscator.none());
 
-        assertThat(request.getRequestUri(), hasToString("http://localhost/"));
+        assertThat(request.getRequestUri(), is("http://localhost/"));
     }
 
     @Test
     public void shouldObfuscatePasswordButNotLimitParameter() {
-        assertThat(unit.getRequestUri(), hasToString(containsString("password=unknown")));
+        assertThat(unit.getRequestUri(), containsString("password=unknown"));
     }
 
     @Test
     public void shouldNotObfuscateLimitParameter() {
-        assertThat(unit.getRequestUri(), hasToString(containsString("limit=1")));
+        assertThat(unit.getRequestUri(), containsString("limit=1"));
     }
 
     @Test
diff --git a/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockHttpRequest.java b/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockHttpRequest.java
index b0e42025..76484b5a 100644
--- a/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockHttpRequest.java
+++ b/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockHttpRequest.java
@@ -40,7 +40,7 @@ public final class MockHttpRequest implements HttpRequest {
 
     private final String remote;
     private final String method;
-    private final URI requestUri;
+    private final String requestUri;
     private final Map<String, String> headers;
     private final String contentType;
     private final Charset charset;
@@ -56,7 +56,7 @@ public final class MockHttpRequest implements HttpRequest {
             @Nullable final String body) {
         this.remote = firstNonNull(remote, "127.0.0.1");
         this.method = firstNonNull(method, "GET");
-        this.requestUri = URI.create(firstNonNull(requestUri, "/"));
+        this.requestUri = firstNonNull(requestUri, "/");
         this.headers = firstNonNullNorEmpty(headers, ImmutableMap.of());
         this.contentType = firstNonNull(contentType, "");
         this.charset = firstNonNull(charset, StandardCharsets.UTF_8);
@@ -78,7 +78,7 @@ public final class MockHttpRequest implements HttpRequest {
     }
 
     @Override
-    public URI getRequestUri() {
+    public String getRequestUri() {
         return requestUri;
     }
 
diff --git a/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockRawHttpRequest.java b/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockRawHttpRequest.java
index 52129dc3..185cd69d 100644
--- a/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockRawHttpRequest.java
+++ b/logbook-spring-boot-starter/src/test/java/org/zalando/logbook/spring/MockRawHttpRequest.java
@@ -28,7 +28,6 @@ import org.zalando.logbook.HttpRequest;
 import org.zalando.logbook.RawHttpRequest;
 
 import java.io.IOException;
-import java.net.URI;
 
 @Value
 @Getter
@@ -38,14 +37,14 @@ public class MockRawHttpRequest implements RawHttpRequest {
 
     private String remote = "127.0.0.1";
     private String method = "GET";
-    private URI requestUri = URI.create("/");
+    private String requestUri = "/";
 
     @Override
     public HttpRequest withBody() throws IOException {
         return MockHttpRequest.builder()
                 .remote(remote)
                 .method(method)
-                .requestUri(requestUri.toASCIIString())
+                .requestUri(requestUri)
                 .build();
     }