Parse invalid request targets in a poor man's fashion, so that obfusc…

…ation is applied on a best-effort basis
twz123 · Feb 22, 2016 · 757a56a · 757a56a
1 parent 0209416
commit 757a56a
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 22 deletions.
diff --git a/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java b/logbook-core/src/main/java/org/zalando/logbook/ObfuscatedHttpRequest.java
@@ -29,6 +29,7 @@
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.Optional;
 
 import static com.google.common.collect.Multimaps.transformEntries;
 
@@ -56,24 +57,50 @@ protected HttpRequest delegate() {
     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;
+        return parseUri(requestUri).map(this::obfuscateUri)
+                                   .orElseGet(() -> lenientObfuscateUri(requestUri));
+    }
+
+    private String obfuscateUri(final URI uri) {
+        final QueryParameters parameters = QueryParameters.parse(uri.getQuery());
+
+        if (parameters.isEmpty()) {
+            return uri.toASCIIString();
+        }
+
+        final String queryString = parameters.obfuscate(parameterObfuscator).toString();
+
+        return createUri(uri, queryString).toASCIIString();
+    }
+
+    private String lenientObfuscateUri(final String uri) {
+        final int startOfQuery = uri.indexOf('?');
+
+        if (startOfQuery < 0) {
+            return uri;
         }
 
-        final QueryParameters parameters = QueryParameters.parse(parsedUri.getQuery());
+        final int startOfFragment = uri.indexOf('#', startOfQuery);
+        final String query = startOfFragment < 0 ? uri.substring(startOfQuery + 1)
+                                                 : uri.substring(startOfQuery + 1, startOfFragment);
+
+        final QueryParameters parameters = QueryParameters.parse(query);
 
         if (parameters.isEmpty()) {
-            return requestUri;
+            return uri;
         }
 
         final String queryString = parameters.obfuscate(parameterObfuscator).toString();
+        return startOfFragment < 0 ? uri.substring(0, startOfQuery + 1) + queryString
+                                   : uri.substring(0, startOfQuery + 1) + queryString + uri.substring(startOfFragment);
+    }
 
-        return createUri(parsedUri, queryString).toASCIIString();
+    private static Optional<URI> parseUri(final String uri) {
+        try {
+            return Optional.of(new URI(uri));
+        } catch (final URISyntaxException invalid) {
+            return Optional.empty();
+        }
     }
 
     @VisibleForTesting
@@ -91,7 +118,7 @@ public Multimap<String, String> getHeaders() {
         return obfuscate(delegate().getHeaders(), headerObfuscator);
     }
 
-    private Multimap<String, String> obfuscate(final Multimap<String, String> values, final Obfuscator obfuscator) {
+    private static Multimap<String, String> obfuscate(final Multimap<String, String> values, final Obfuscator obfuscator) {
         return transformEntries(values, obfuscator::obfuscate);
     }
 

diff --git a/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java b/logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java
@@ -44,17 +44,35 @@ public final class ObfuscatedHttpRequestTest {
             (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());
+    public void shouldObfuscateInvalidUris() {
+        final String invalidPath = "/af.cgi?_browser_out=.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2Fetc%2Fpasswd&foo=bar#fragment";
+        final HttpRequest invalidRequest = buildInvalidRequest(invalidPath, Obfuscator.obfuscate("_browser_out"::equalsIgnoreCase, "unknown"));
+
+        assertThat(invalidRequest.getRequestUri(), is("/af.cgi?_browser_out=unknown&foo=bar#fragment"));
+    }
+
+    @Test
+    public void shouldNotFailOnInvalidUrisWithEmptyQueryString() {
+        final String invalidPath = "/unterminated_percent_%F?";
+        final HttpRequest invalidRequest = buildInvalidRequest(invalidPath, Obfuscator.none());
+
+        assertThat(invalidRequest.getRequestUri(), is("/unterminated_percent_%F?"));
+    }
+
+    @Test
+    public void shouldNotFailOnInvalidUrisWithQueryStringOnly() {
+        final String invalidPath = "/unterminated_percent_%F?q";
+        final HttpRequest invalidRequest = buildInvalidRequest(invalidPath, Obfuscator.none());
+
+        assertThat(invalidRequest.getRequestUri(), is("/unterminated_percent_%F?q"));
+    }
+
+    @Test
+    public void shouldNotFailOnInvalidUrisWithFragmentOnly() {
+        final String invalidPath = "/unterminated_percent_%F#";
+        final HttpRequest invalidRequest = buildInvalidRequest(invalidPath, Obfuscator.none());
 
-        assertThat(invalidRequest.getRequestUri(), is(invalidUri));
+        assertThat(invalidRequest.getRequestUri(), is("/unterminated_percent_%F#"));
     }
 
     @Test
@@ -97,4 +115,12 @@ public void shouldObfuscateBodyContent() throws IOException {
         assertThat(new String(unit.getBody(), unit.getCharset()), is("My secret is f4k3"));
     }
 
-}
+    private static ObfuscatedHttpRequest buildInvalidRequest(final String path, final Obfuscator parameterObfuscator) {
+        return new ObfuscatedHttpRequest(
+                MockHttpRequest.builder().requestUri(path).build(),
+                Obfuscator.none(),
+                parameterObfuscator,
+                BodyObfuscator.none());
+    }
+
+}