[GEOS-7016] Enabling request body logging ruins binary data POST/PUT …

…requests

src/main/src/main/java/org/geoserver/filters/BufferedRequestStream.java

@@ -18,8 +18,8 @@
 public class BufferedRequestStream extends ServletInputStream{
     InputStream myInputStream;
 
-    public BufferedRequestStream(String buff) throws IOException {
-        myInputStream = new ByteArrayInputStream(buff.getBytes());
+    public BufferedRequestStream(byte[] buff) throws IOException {
+        myInputStream = new ByteArrayInputStream(buff);
         myInputStream.mark(16);
         myInputStream.read();
         myInputStream.reset();

src/main/src/main/java/org/geoserver/filters/BufferedRequestWrapper.java

@@ -27,24 +27,27 @@
 
 import org.geotools.util.Converters;
 
-public class BufferedRequestWrapper extends HttpServletRequestWrapper{
+public class BufferedRequestWrapper extends HttpServletRequestWrapper {
     protected HttpServletRequest myWrappedRequest;
-    protected String myBuffer;
+
+    protected byte[] myBuffer;
+
+    protected String charset;
     protected ServletInputStream myStream = null;
     protected BufferedReader myReader = null;
 	protected Map myParameterMap;
     protected Logger logger = 
         org.geotools.util.logging.Logging.getLogger("org.geoserver.filters");
 
-    public BufferedRequestWrapper(HttpServletRequest req, String buff){
+    public BufferedRequestWrapper(HttpServletRequest req, String charset, byte[] buff) {
         super(req);
-        myWrappedRequest = req;
-        myBuffer = buff;
-		logger.fine("Created BufferedRequestWrapper with String: \"" + buff + "\" as buffer");
+        this.myWrappedRequest = req;
+        this.myBuffer = buff;
+        this.charset = charset;
     }
 
     public ServletInputStream getInputStream() throws IOException{
-        if (myStream == null){
+        if (myStream == null) {
             if (myReader == null){
                 myStream = new BufferedRequestStream(myBuffer);
             } else {
@@ -58,12 +61,8 @@ public ServletInputStream getInputStream() throws IOException{
     public BufferedReader getReader() throws IOException{
         if (myReader == null){
             if (myStream == null){
-
-                myReader = new BufferedReader(
-                        new InputStreamReader(
-                            new BufferedRequestStream(myBuffer) 
-                            )
-                        );
+                myReader = new BufferedReader(new InputStreamReader(new BufferedRequestStream(
+                        myBuffer), charset));
             } else {
                 throw new IOException("Requesting a reader after a stream is already in use!!");
             }
@@ -88,7 +87,7 @@ public Map getParameterMap(){
 		while (it.hasNext()){
 			Map.Entry entry = (Map.Entry)it.next();
 			toArrays.put(entry.getKey(), 
-			  (String[])((List)entry.getValue()).toArray(new String[0]));
+			  ((List)entry.getValue()).toArray(new String[0]));
 		}
 
 		return Collections.unmodifiableMap(toArrays);
@@ -132,7 +131,13 @@ protected void parseFormBody(){
 		myParameterMap = new TreeMap();
 
 		// parse the body
-		String[] pairs = myBuffer.split("\\&");
+        String[] pairs;
+        try {
+            pairs = new String(myBuffer, charset).split("\\&");
+        } catch (UnsupportedEncodingException e) {
+            // should not happen
+            throw new RuntimeException(e);
+        }
 
 		for (int i = 0; i < pairs.length; i++){
 			parsePair(pairs[i]);

src/main/src/main/java/org/geoserver/filters/LoggingFilter.java

@@ -5,8 +5,9 @@
  */
 package org.geoserver.filters;
 
-import java.io.BufferedReader;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.util.logging.Logger;
 
 import javax.servlet.Filter;
@@ -17,6 +18,8 @@
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.io.IOUtils;
+
 /**
  * Filter to log requests for debugging or statistics-gathering purposes.
  *
@@ -53,15 +56,22 @@ public void doFilter(ServletRequest req, ServletResponse res,
                 if (logBodies && (hreq.getMethod().equals("PUT") || hreq.getMethod().equals("POST"))){
                     message += " request-size: " + hreq.getContentLength();
                     message += " body: ";
-                    StringBuffer buff = new StringBuffer();
-                    BufferedReader reader = hreq.getReader();
-                    char[] readIn = new char[256];
-                    int amountRead = 0;
-                    while ((amountRead = reader.read(readIn, 0 , 256)) != -1){
-                        buff.append(readIn, 0, amountRead);
+
+                    String encoding = hreq.getCharacterEncoding();
+                    if (encoding == null) {
+                        // the default encoding for HTTP 1.1
+                        encoding = "ISO-8859-1";
+                    }
+                    ByteArrayOutputStream bos = new ByteArrayOutputStream();
+                    byte[] bytes;
+                    try (InputStream is = hreq.getInputStream()) {
+                        IOUtils.copy(is, bos);
+                        bytes = bos.toByteArray();
+
+                        body = new String(bytes, encoding);
                     }
-                    body = buff.toString();
-                    req = new BufferedRequestWrapper(hreq, buff.toString());
+
+                    req = new BufferedRequestWrapper(hreq, encoding, bytes);
                 }
             } else {
                 message = "" + req.getRemoteHost() + " made a non-HTTP request";
@@ -99,4 +109,32 @@ protected String noNull(String s){
 
     public void destroy() {
     }
+
+    /**
+     * @return the enabled
+     */
+    public boolean isEnabled() {
+        return enabled;
+    }
+
+    /**
+     * @param enabled the enabled to set
+     */
+    public void setEnabled(boolean enabled) {
+        this.enabled = enabled;
+    }
+
+    /**
+     * @return the logBodies
+     */
+    public boolean isLogBodies() {
+        return logBodies;
+    }
+
+    /**
+     * @param logBodies the logBodies to set
+     */
+    public void setLogBodies(boolean logBodies) {
+        this.logBodies = logBodies;
+    }
 }

src/web/app/src/test/java/org/geoserver/filters/BufferedRequestStreamTest.java → src/main/src/test/java/org/geoserver/filters/BufferedRequestStreamTest.java

@@ -5,7 +5,7 @@
  */
 package org.geoserver.filters;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -21,7 +21,7 @@ public class BufferedRequestStreamTest {
 	@Before
     public void setUpInternal() throws Exception{
 		myTestString = "Hello, this is a test";
-		myBRS = new BufferedRequestStream(myTestString);
+        myBRS = new BufferedRequestStream(myTestString.getBytes());
 	}
 
 	@Test

src/main/src/test/java/org/geoserver/filters/BufferedRequestWrapperTest.java

@@ -0,0 +1,95 @@
+/* (c) 2014 Open Source Geospatial Foundation - all rights reserved
+ * (c) 2001 - 2013 OpenPlans
+ * This code is licensed under the GPL 2.0 license, available at the root
+ * application directory.
+ */
+package org.geoserver.filters;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.BufferedReader;
+import java.util.Map;
+
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServletRequest;
+
+import org.junit.Test;
+import org.springframework.web.util.WebUtils;
+
+public class BufferedRequestWrapperTest extends RequestWrapperTestSupport {
+
+    @Test
+    public void testGetInputStream() throws Exception {
+        for (int i = 0; i < testStrings.length; i++) {
+            doInputStreamTest(testStrings[i]);
+        }
+    }
+
+    @Test
+    public void testGetReader() throws Exception {
+        for (int i = 0; i < testStrings.length; i++) {
+            doGetReaderTest(testStrings[i]);
+        }
+    }
+
+    public void doInputStreamTest(String testString) throws Exception {
+        HttpServletRequest req = makeRequest(testString, null);
+
+        BufferedRequestWrapper wrapper = new BufferedRequestWrapper(req,
+                WebUtils.DEFAULT_CHARACTER_ENCODING, testString.getBytes());
+        ServletInputStream sis = req.getInputStream();
+        byte b[] = new byte[32];
+        int amountRead;
+
+        while ((sis.readLine(b, 0, 32)) > 0) { /* clear out the request body */
+        }
+
+        sis = wrapper.getInputStream();
+        StringBuffer buff = new StringBuffer();
+
+        while ((amountRead = sis.readLine(b, 0, 32)) != 0) {
+            buff.append(new String(b, 0, amountRead));
+        }
+
+        assertEquals(buff.toString(), testString);
+    }
+
+    public void doGetReaderTest(String testString) throws Exception {
+        HttpServletRequest req = makeRequest(testString, null);
+
+        BufferedReader br = req.getReader();
+        while ((br.readLine()) != null) { /* clear out the body */
+        }
+
+        BufferedRequestWrapper wrapper = new BufferedRequestWrapper(req,
+                WebUtils.DEFAULT_CHARACTER_ENCODING, testString.getBytes());
+        StringBuffer buff = new StringBuffer();
+        int c;
+        br = wrapper.getReader();
+
+        while ((c = br.read()) != -1) {
+            buff.append((char) c);
+        }
+
+        assertEquals(buff.toString(), testString);
+    }
+
+    @Test
+    public void testMixedRequest() throws Exception {
+        String body = "a=1&b=2";
+        String queryString = "c=3&d=4";
+        HttpServletRequest req = makeRequest(body, queryString);
+
+        BufferedReader br = req.getReader();
+        while ((br.readLine()) != null) { /* clear out the body */
+        }
+
+        BufferedRequestWrapper wrapper = new BufferedRequestWrapper(req, "UTF-8", body.getBytes());
+        Map params = wrapper.getParameterMap();
+        assertEquals(4, params.size());
+        assertEquals("1", ((String[]) params.get("a"))[0]);
+        assertEquals("2", ((String[]) params.get("b"))[0]);
+        assertEquals("3", ((String[]) params.get("c"))[0]);
+        assertEquals("4", ((String[]) params.get("d"))[0]);
+    }
+}

src/restconfig/src/test/java/org/geoserver/catalog/rest/DataStoreFileUploadTest.java

@@ -16,16 +16,21 @@
 import java.io.InputStream;
 import java.io.OutputStreamWriter;
 import java.net.URL;
+import java.util.Collections;
+import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 import java.util.zip.ZipInputStream;
 import java.util.zip.ZipOutputStream;
 
+import javax.servlet.Filter;
+
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
 import org.geoserver.catalog.Catalog;
 import org.geoserver.catalog.DataStoreInfo;
 import org.geoserver.data.test.MockData;
+import org.geoserver.filters.LoggingFilter;
 import org.geotools.data.DataUtilities;
 import org.h2.tools.DeleteDbFiles;
 import org.junit.After;
@@ -38,6 +43,14 @@
 
 public class DataStoreFileUploadTest extends CatalogRESTTestSupport {
 
+    @Override
+    protected List<Filter> getFilters() {
+        LoggingFilter filter = new LoggingFilter();
+        filter.setEnabled(true);
+        filter.setLogBodies(true);
+        return Collections.singletonList((Filter) filter);
+    }
+
     @Before
     public void removePdsDataStore() {
         removeStore("gs", "pds");

src/web/app/src/main/webapp/WEB-INF/web.xml

@@ -114,7 +114,7 @@
               parameter defaults to false.
               -->
          <param-name>enabled</param-name>
-         <param-value>false</param-value>
+         <param-value>true</param-value>
      </init-param>
      <init-param>
      <!-- The 'log-request-bodies' parameter is a boolean value, "true" (case-insensitive) for
@@ -125,7 +125,7 @@
           server.
           -->
      <param-name>log-request-bodies</param-name>
-     <param-value>false</param-value>
+     <param-value>true</param-value>
      </init-param>
    </filter>