Attach only valid HTTP headers in CORS filter

Filter receives `Access-Control-Request-Headers` request header (usually
send with OPTIONS HTTP request) and attaches all specified headers to
`Access-Control-Allow-Headers` response header. This commit makes filter
validate all provided headers and only attach valid ones.

community/server/src/main/java/org/neo4j/server/rest/web/CorsFilter.java

@@ -34,6 +34,8 @@
 import org.neo4j.logging.LogProvider;
 import org.neo4j.server.web.HttpMethod;
 
+import static org.neo4j.server.web.HttpHeaderUtils.isValidHttpHeaderName;
+
 /**
  * This filter adds the header "Access-Control-Allow-Origin : *" to all
  * responses that goes through it. This allows modern browsers to do cross-site
@@ -84,7 +86,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp
             while ( requestHeaderEnumeration.hasMoreElements() )
             {
                 String requestHeader = requestHeaderEnumeration.nextElement();
-                response.addHeader( ACCESS_CONTROL_ALLOW_HEADERS, requestHeader );
+                addAllowedHeaderIfValid( requestHeader, response );
             }
         }
 
@@ -99,14 +101,27 @@ public void destroy()
     private void addAllowedMethodIfValid( String methodName, HttpServletResponse response )
     {
         HttpMethod method = HttpMethod.valueOfOrNull( methodName );
-        if ( method == null )
+        if ( method != null )
+        {
+            response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, methodName );
+        }
+        else
         {
             log.warn( "Unknown HTTP method specified in " + ACCESS_CONTROL_REQUEST_METHOD + " '" + methodName + "'. " +
                       "It will be ignored and not attached to the " + ACCESS_CONTROL_ALLOW_METHODS + " response header" );
         }
+    }
+
+    private void addAllowedHeaderIfValid( String headerName, HttpServletResponse response )
+    {
+        if ( isValidHttpHeaderName( headerName ) )
+        {
+            response.addHeader( ACCESS_CONTROL_ALLOW_HEADERS, headerName );
+        }
         else
         {
-            response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, methodName );
+            log.warn( "Invalid HTTP header specified in " + ACCESS_CONTROL_REQUEST_HEADERS + " '" + headerName + "'. " +
+                      "It will be ignored and not attached to the " + ACCESS_CONTROL_ALLOW_HEADERS + " response header" );
         }
     }
 }

community/server/src/main/java/org/neo4j/server/web/HttpHeaderUtils.java

@@ -87,4 +87,33 @@ public static long getTransactionTimeout( HttpServletRequest request, Log errorL
         }
         return GraphDatabaseSettings.UNSPECIFIED_TIMEOUT;
     }
+
+    /**
+     * Validates given HTTP header name. Does not allow blank names and names with control characters, like '\n' (LF) and '\r' (CR).
+     * Can be used to detect and neutralize CRLF in HTTP headers.
+     *
+     * @param name the HTTP header name, like 'Accept' or 'Content-Type'.
+     * @return {@code true} when given name represents a valid HTTP header, {@code false} otherwise.
+     */
+    public static boolean isValidHttpHeaderName( String name )
+    {
+        if ( name == null || name.length() == 0 )
+        {
+            return false;
+        }
+        boolean isBlank = true;
+        for ( int i = 0; i < name.length(); i++ )
+        {
+            char c = name.charAt( i );
+            if ( Character.isISOControl( c ) )
+            {
+                return false;
+            }
+            if ( !Character.isWhitespace( c ) )
+            {
+                isBlank = false;
+            }
+        }
+        return !isBlank;
+    }
 }

community/server/src/test/java/org/neo4j/server/rest/web/CorsFilterTest.java

@@ -122,16 +122,22 @@ public void shouldAttachNoRequestHeadersToAccessControlAllowHeadersWhenHeaderIsN
     }
 
     @Test
-    public void shouldAttachRequestHeadersToAccessControlAllowHeaders() throws Exception
+    public void shouldAttachValidRequestHeadersToAccessControlAllowHeaders() throws Exception
     {
-        List<String> accessControlRequestHeaders = asList( "Accept", "Content-Length", "Content-Type" );
+        List<String> accessControlRequestHeaders = asList( "Accept", "X-Wrong\nHeader", "Content-Type", "Accept\r", "Illegal\r\nHeader", "", null, "   " );
         HttpServletRequest request = requestMock( emptyList(), accessControlRequestHeaders );
 
         filter.doFilter( request, response, chain );
 
         verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Accept" );
-        verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Content-Length" );
         verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Content-Type" );
+
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, null );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "" );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "   " );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "X-Wrong\nHeader" );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Accept\r" );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Illegal\r\nHeader" );
     }
 
     private static HttpServletRequest requestMock()

community/server/src/test/java/org/neo4j/server/web/HttpHeaderUtilsTest.java

@@ -19,6 +19,7 @@
  */
 package org.neo4j.server.web;
 
+import org.apache.http.HttpHeaders;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -30,7 +31,12 @@
 import org.neo4j.logging.Log;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.when;
+import static org.neo4j.server.web.HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER;
+import static org.neo4j.server.web.HttpHeaderUtils.getTransactionTimeout;
+import static org.neo4j.server.web.HttpHeaderUtils.isValidHttpHeaderName;
 
 public class HttpHeaderUtilsTest
 {
@@ -47,9 +53,9 @@ public void setUp()
     @Test
     public void retrieveCustomTransactionTimeout()
     {
-        when( request.getHeader( HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "100" );
+        when( request.getHeader( MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "100" );
         Log log = logProvider.getLog( HttpServletRequest.class );
-        long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log );
+        long transactionTimeout = getTransactionTimeout( request, log );
         assertEquals( "Transaction timeout should be retrieved.", 100, transactionTimeout );
         logProvider.assertNoLoggingOccurred();
     }
@@ -58,19 +64,43 @@ public void retrieveCustomTransactionTimeout()
     public void defaultValueWhenCustomTransactionTimeoutNotSpecified()
     {
         Log log = logProvider.getLog( HttpServletRequest.class );
-        long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log );
+        long transactionTimeout = getTransactionTimeout( request, log );
         assertEquals( "Transaction timeout not specified.", 0, transactionTimeout );
         logProvider.assertNoLoggingOccurred();
     }
 
     @Test
     public void defaultValueWhenCustomTransactionTimeoutNotANumber()
     {
-        when( request.getHeader( HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "aa" );
+        when( request.getHeader( MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "aa" );
         Log log = logProvider.getLog( HttpServletRequest.class );
-        long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log );
+        long transactionTimeout = getTransactionTimeout( request, log );
         assertEquals( "Transaction timeout not specified.", 0, transactionTimeout );
         logProvider.assertContainsMessageContaining("Fail to parse `max-execution-time` " +
                 "header with value: 'aa'. Should be a positive number.");
     }
+
+    @Test
+    public void shouldCheckHttpHeaders()
+    {
+        assertFalse( isValidHttpHeaderName( null ) );
+        assertFalse( isValidHttpHeaderName( "" ) );
+        assertFalse( isValidHttpHeaderName( " " ) );
+        assertFalse( isValidHttpHeaderName( "      " ) );
+        assertFalse( isValidHttpHeaderName( " \r " ) );
+        assertFalse( isValidHttpHeaderName( " \r\n\t " ) );
+
+        assertTrue( isValidHttpHeaderName( HttpHeaders.ACCEPT ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.ACCEPT_ENCODING ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.AGE ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.CONTENT_ENCODING ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.EXPIRES ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.IF_MATCH ) );
+        assertTrue( isValidHttpHeaderName( HttpHeaders.TRANSFER_ENCODING ) );
+        assertTrue( isValidHttpHeaderName( "Weird Header With Spaces" ) );
+
+        assertFalse( isValidHttpHeaderName( "My\nHeader" ) );
+        assertFalse( isValidHttpHeaderName( "Other\rStrange-Header" ) );
+        assertFalse( isValidHttpHeaderName( "Header-With-Tab\t" ) );
+    }
 }