Attach only valid HTTP methods in CORS filter

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

community/server/src/main/java/org/neo4j/server/modules/RESTApiModule.java

@@ -70,7 +70,7 @@ public void start()
         URI restApiUri = restApiUri( );
 
         webServer.addFilter( new CollectUserAgentFilter( clientNames() ), "/*" );
-        webServer.addFilter( new CorsFilter(), "/*" );
+        webServer.addFilter( new CorsFilter( logProvider ), "/*" );
         webServer.addJAXRSClasses( getClassNames(), restApiUri.toString(), null );
         loadPlugins();
     }

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

@@ -30,6 +30,10 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.neo4j.logging.Log;
+import org.neo4j.logging.LogProvider;
+import org.neo4j.server.web.HttpMethod;
+
 /**
  * This filter adds the header "Access-Control-Allow-Origin : *" to all
  * responses that goes through it. This allows modern browsers to do cross-site
@@ -43,6 +47,13 @@ public class CorsFilter implements Filter
     public static final String ACCESS_CONTROL_REQUEST_METHOD = "Access-Control-Request-Method";
     public static final String ACCESS_CONTROL_REQUEST_HEADERS = "Access-Control-Request-Headers";
 
+    private final Log log;
+
+    public CorsFilter( LogProvider logProvider )
+    {
+        this.log = logProvider.getLog( getClass() );
+    }
+
     @Override
     public void init( FilterConfig filterConfig ) throws ServletException
     {
@@ -63,7 +74,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp
             while ( requestMethodEnumeration.hasMoreElements() )
             {
                 String requestMethod = requestMethodEnumeration.nextElement();
-                response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, requestMethod );
+                addAllowedMethodIfValid( requestMethod, response );
             }
         }
 
@@ -84,4 +95,18 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp
     public void destroy()
     {
     }
+
+    private void addAllowedMethodIfValid( String methodName, HttpServletResponse response )
+    {
+        HttpMethod method = HttpMethod.valueOfOrNull( methodName );
+        if ( method == null )
+        {
+            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" );
+        }
+        else
+        {
+            response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, methodName );
+        }
+    }
 }

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

@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.server.web;
+
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+import static java.util.Collections.unmodifiableMap;
+
+public enum HttpMethod
+{
+    OPTIONS,
+    GET,
+    HEAD,
+    POST,
+    PUT,
+    PATCH,
+    DELETE,
+    TRACE,
+    CONNECT;
+
+    private static final Map<String,HttpMethod> methodsByName = indexMethodsByName();
+
+    @Nullable
+    public static HttpMethod valueOfOrNull( String name )
+    {
+        return methodsByName.get( name );
+    }
+
+    private static Map<String,HttpMethod> indexMethodsByName()
+    {
+        HttpMethod[] methods = HttpMethod.values();
+        Map<String,HttpMethod> result = new HashMap<>( methods.length * 2 );
+        for ( HttpMethod method : methods )
+        {
+            result.put( method.name(), method );
+        }
+        return unmodifiableMap( result );
+    }
+}

community/server/src/test/java/org/neo4j/server/rest/security/AuthorizationCorsIT.java

@@ -23,6 +23,7 @@
 
 import javax.ws.rs.core.HttpHeaders;
 
+import org.neo4j.server.web.HttpMethod;
 import org.neo4j.test.server.HTTP;
 
 import static com.sun.jersey.api.client.ClientResponse.Status.FORBIDDEN;
@@ -36,6 +37,10 @@
 import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_ALLOW_ORIGIN;
 import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_REQUEST_HEADERS;
 import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_REQUEST_METHOD;
+import static org.neo4j.server.web.HttpMethod.DELETE;
+import static org.neo4j.server.web.HttpMethod.GET;
+import static org.neo4j.server.web.HttpMethod.PATCH;
+import static org.neo4j.server.web.HttpMethod.POST;
 import static org.neo4j.test.server.HTTP.RawPayload.quotedJson;
 
 public class AuthorizationCorsIT extends CommunityServerTestBase
@@ -96,13 +101,10 @@ public void shouldAddCorsMethodsHeader() throws Exception
     {
         startServer( false );
 
-        HTTP.Builder requestBuilder = requestWithHeaders( "authDisabled", "authDisabled" )
-                .withHeaders( ACCESS_CONTROL_REQUEST_METHOD, "POST, GET, DELETE" );
-        HTTP.Response response = runQuery( requestBuilder );
-
-        assertEquals( OK.getStatusCode(), response.status() );
-        assertCorsHeaderPresent( response );
-        assertEquals( "POST, GET, DELETE", response.header( ACCESS_CONTROL_ALLOW_METHODS ) );
+        testCorsAllowMethods( POST );
+        testCorsAllowMethods( GET );
+        testCorsAllowMethods( PATCH );
+        testCorsAllowMethods( DELETE );
     }
 
     @Test
@@ -119,6 +121,17 @@ public void shouldAddCorsRequestHeaders() throws Exception
         assertEquals( "Accept, X-Not-Accept", response.header( ACCESS_CONTROL_ALLOW_HEADERS ) );
     }
 
+    private void testCorsAllowMethods( HttpMethod method ) throws Exception
+    {
+        HTTP.Builder requestBuilder = requestWithHeaders( "authDisabled", "authDisabled" )
+                .withHeaders( ACCESS_CONTROL_REQUEST_METHOD, method.toString() );
+        HTTP.Response response = runQuery( requestBuilder );
+
+        assertEquals( OK.getStatusCode(), response.status() );
+        assertCorsHeaderPresent( response );
+        assertEquals( method, HttpMethod.valueOf( response.header( ACCESS_CONTROL_ALLOW_METHODS ) ) );
+    }
+
     private HTTP.Response changePassword( String username, String oldPassword, String newPassword )
     {
         HTTP.RawPayload passwordChange = quotedJson( "{'password': '" + newPassword + "'}" );

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

@@ -0,0 +1,159 @@
+/*
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.server.rest.web;
+
+import org.junit.Test;
+
+import java.util.List;
+import javax.servlet.FilterChain;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.neo4j.logging.NullLogProvider;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.enumeration;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_ALLOW_HEADERS;
+import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_ALLOW_METHODS;
+import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_ALLOW_ORIGIN;
+import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_REQUEST_HEADERS;
+import static org.neo4j.server.rest.web.CorsFilter.ACCESS_CONTROL_REQUEST_METHOD;
+
+public class CorsFilterTest
+{
+    private final HttpServletRequest emptyRequest = requestMock( emptyList(), emptyList() );
+    private final HttpServletResponse response = responseMock();
+    private final FilterChain chain = filterChainMock();
+
+    private final CorsFilter filter = new CorsFilter( NullLogProvider.getInstance() );
+
+    @Test
+    public void shouldCallChainDoFilter() throws Exception
+    {
+        filter.doFilter( emptyRequest, response, chain );
+
+        verify( chain ).doFilter( emptyRequest, response );
+    }
+
+    @Test
+    public void shouldSetAccessControlAllowOrigin() throws Exception
+    {
+        filter.doFilter( emptyRequest, response, filterChainMock() );
+
+        verify( response ).setHeader( ACCESS_CONTROL_ALLOW_ORIGIN, "*" );
+    }
+
+    @Test
+    public void shouldAttachNoHttpMethodsToAccessControlAllowMethodsWhenHeaderIsEmpty() throws Exception
+    {
+        filter.doFilter( emptyRequest, response, chain );
+
+        verify( response, never() ).addHeader( eq( ACCESS_CONTROL_ALLOW_METHODS ), anyString() );
+    }
+
+    @Test
+    public void shouldAttachNoHttpMethodsToAccessControlAllowMethodsWhenHeaderIsNull() throws Exception
+    {
+        HttpServletRequest request = requestMock();
+        when( request.getHeaders( ACCESS_CONTROL_REQUEST_METHOD ) ).thenReturn( null );
+
+        filter.doFilter( request, response, chain );
+
+        verify( response, never() ).addHeader( eq( ACCESS_CONTROL_ALLOW_METHODS ), anyString() );
+    }
+
+    @Test
+    public void shouldAttachValidHttpMethodsToAccessControlAllowMethods() throws Exception
+    {
+        List<String> accessControlRequestMethods = asList( "GET", "WRONG", "POST", "TAKE", "CONNECT" );
+        HttpServletRequest request = requestMock( accessControlRequestMethods, emptyList() );
+
+        filter.doFilter( request, response, chain );
+
+        verify( response ).addHeader( ACCESS_CONTROL_ALLOW_METHODS, "GET" );
+        verify( response ).addHeader( ACCESS_CONTROL_ALLOW_METHODS, "POST" );
+        verify( response ).addHeader( ACCESS_CONTROL_ALLOW_METHODS, "CONNECT" );
+
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_METHODS, "TAKE" );
+        verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_METHODS, "WRONG" );
+    }
+
+    @Test
+    public void shouldAttachNoRequestHeadersToAccessControlAllowHeadersWhenHeaderIsEmpty() throws Exception
+    {
+        filter.doFilter( emptyRequest, response, chain );
+
+        verify( response, never() ).addHeader( eq( ACCESS_CONTROL_ALLOW_HEADERS ), anyString() );
+    }
+
+    @Test
+    public void shouldAttachNoRequestHeadersToAccessControlAllowHeadersWhenHeaderIsNull() throws Exception
+    {
+        HttpServletRequest request = requestMock();
+        when( request.getHeaders( ACCESS_CONTROL_REQUEST_HEADERS ) ).thenReturn( null );
+
+        filter.doFilter( request, response, chain );
+
+        verify( response, never() ).addHeader( eq( ACCESS_CONTROL_ALLOW_HEADERS ), anyString() );
+    }
+
+    @Test
+    public void shouldAttachRequestHeadersToAccessControlAllowHeaders() throws Exception
+    {
+        List<String> accessControlRequestHeaders = asList( "Accept", "Content-Length", "Content-Type" );
+        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" );
+    }
+
+    private static HttpServletRequest requestMock()
+    {
+        return requestMock( emptyList(), emptyList() );
+    }
+
+    private static HttpServletRequest requestMock( List<String> accessControlRequestMethods, List<String> accessControlRequestHeaders )
+    {
+        HttpServletRequest mock = mock( HttpServletRequest.class );
+        when( mock.getHeaders( ACCESS_CONTROL_REQUEST_METHOD ) ).thenReturn( enumeration( accessControlRequestMethods ) );
+        when( mock.getHeaders( ACCESS_CONTROL_REQUEST_HEADERS ) ).thenReturn( enumeration( accessControlRequestHeaders ) );
+        return mock;
+    }
+
+    private static HttpServletResponse responseMock()
+    {
+        return mock( HttpServletResponse.class );
+    }
+
+    private static FilterChain filterChainMock()
+    {
+        return mock( FilterChain.class );
+    }
+}

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

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.server.web;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class HttpMethodTest
+{
+    @Test
+    public void shouldLookupExistingMethodByName()
+    {
+        for ( HttpMethod method : HttpMethod.values() )
+        {
+            assertEquals( method, HttpMethod.valueOfOrNull( method.toString() ) );
+        }
+    }
+
+    @Test
+    public void shouldLookupNonExistingMethodByName()
+    {
+        assertNull( HttpMethod.valueOfOrNull( "get" ) );
+        assertNull( HttpMethod.valueOfOrNull( "post" ) );
+        assertNull( HttpMethod.valueOfOrNull( "PoSt" ) );
+        assertNull( HttpMethod.valueOfOrNull( "WRONG" ) );
+        assertNull( HttpMethod.valueOfOrNull( "" ) );
+    }
+
+    @Test
+    public void shouldLookupNothingByNull()
+    {
+        assertNull( HttpMethod.valueOfOrNull( null ) );
+    }
+}