Refactor to allow CSRF bypass, improve error logging for API authoring

server/src/com/mirth/connect/client/core/Client.java

@@ -196,7 +196,7 @@ public Connector getConnector(javax.ws.rs.client.Client client, Configuration ru
                 try {
                     config.register(Class.forName(apiProviderClass));
                 } catch (Throwable t) {
-                    logger.error("Error registering API provider class: " + apiProviderClass);
+                    logger.error("Error registering API provider class: {}", apiProviderClass, t);
                 }
             }
         }
@@ -219,7 +219,7 @@ public void registerApiProviders(Set<String> packageNames, Set<String> classes)
                         client.register(clazz);
                     }
                 } catch (Throwable t) {
-                    logger.error("Error registering API provider package: " + packageName);
+                    logger.error("Error registering API provider package: {}", packageName, t);
                 }
             }
         }
@@ -229,7 +229,7 @@ public void registerApiProviders(Set<String> packageNames, Set<String> classes)
                 try {
                     client.register(Class.forName(clazz));
                 } catch (Throwable t) {
-                    logger.error("Error registering API provider class: " + clazz);
+                    logger.error("Error registering API provider class: {}", clazz, t);
                 }
             }
         }

server/src/com/mirth/connect/server/MirthWebServer.java

@@ -454,7 +454,7 @@ private ServletContextHandler createApiServletContextHandler(String contextPath,
         apiServletContextHandler.setContextPath(contextPath + baseAPI + apiPath);
         apiServletContextHandler.addFilter(new FilterHolder(new ApiOriginFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
         apiServletContextHandler.addFilter(new FilterHolder(new ClickjackingFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
-        apiServletContextHandler.addFilter(new FilterHolder(new RequestedWithFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
+        RequestedWithFilter.configure(mirthProperties);
         apiServletContextHandler.addFilter(new FilterHolder(new MethodFilter()), "/*", EnumSet.of(DispatcherType.REQUEST));
         apiServletContextHandler.addFilter(new FilterHolder(new StrictTransportSecurityFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
         setConnectorNames(apiServletContextHandler, apiAllowHTTP);

server/src/com/mirth/connect/server/api/DontRequireRequestedWith.java

@@ -0,0 +1,15 @@
+package com.mirth.connect.server.api;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * If this annotation is present on a method or class, the X-Requested-With header
+ * requirement will not be enforced for that resource.
+ */
+@Target({ElementType.METHOD, ElementType.TYPE})
+@Retention(RetentionPolicy.RUNTIME)
+public @interface DontRequireRequestedWith {
+}

server/src/com/mirth/connect/server/api/MirthServlet.java

@@ -210,6 +210,9 @@ private void setContext() {
     }
 
     public void setOperation(Operation operation) {
+        if (operation == null) {
+            throw new MirthApiException("Method operation cannot be null.");
+        }
         if (extensionName != null) {
             operation = new ExtensionOperation(extensionName, operation);
         }

server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java

@@ -1,64 +1,67 @@
-/*
- * Copyright (c) Mirth Corporation. All rights reserved.
- * 
- * http://www.mirthcorp.com
- * 
- * The software in this package is published under the terms of the MPL license a copy of which has
- * been included with this distribution in the LICENSE.txt file.
- */
-
 package com.mirth.connect.server.api.providers;
 
 import java.io.IOException;
+import java.lang.reflect.Method;
+import java.util.List;
 
-import javax.servlet.Filter;
-import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
-import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
+import javax.annotation.Priority;
+import javax.ws.rs.Priorities;
+import javax.ws.rs.container.ContainerRequestContext;
+import javax.ws.rs.container.ContainerRequestFilter;
+import javax.ws.rs.container.ResourceInfo;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.Provider;
 
 import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.apache.commons.lang3.StringUtils;
 
+import com.mirth.connect.server.api.DontRequireRequestedWith;
+
 @Provider
-public class RequestedWithFilter implements Filter {
+@Priority(Priorities.AUTHENTICATION + 100)
+public class RequestedWithFilter implements ContainerRequestFilter {
 
-    private boolean isRequestedWithHeaderRequired = true; 
+    @Context
+    private ResourceInfo resourceInfo;
 
+    private static boolean isRequestedWithHeaderRequired = true;
 
-    public RequestedWithFilter(PropertiesConfiguration mirthProperties) {
-
+    // Jax requires a no-arg constructor to instantiate providers via classpath scanning.
+    public RequestedWithFilter() {
+    }
+
+    public static void configure(PropertiesConfiguration mirthProperties) {
         isRequestedWithHeaderRequired = mirthProperties.getBoolean("server.api.require-requested-with", true);
     }
 
-    @Override
-    public void init(FilterConfig filterConfig) throws ServletException {}
+    public static boolean isRequestedWithHeaderRequired() {
+        return isRequestedWithHeaderRequired;
+    }
 
     @Override
-    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
-        HttpServletResponse res = (HttpServletResponse) response;
+    public void filter(ContainerRequestContext requestContext) throws IOException {
+        if (!isRequestedWithHeaderRequired) {
+            return;
+        }
+
+        // If the resource method or class is annotated with DontRequireRequestedWith, skip the check
+        if (resourceInfo != null) {
+            Method method = resourceInfo.getResourceMethod();
+            if (method != null && method.getAnnotation(DontRequireRequestedWith.class) != null) {
+                return;
+            }
+            Class<?> resourceClass = resourceInfo.getResourceClass();
+            if (resourceClass != null && resourceClass.getAnnotation(DontRequireRequestedWith.class) != null) {
+                return;
+            }
+        }
 
-        HttpServletRequest servletRequest = (HttpServletRequest)request;
-        String requestedWithHeader = (String) servletRequest.getHeader("X-Requested-With");
+        List<String> header = requestContext.getHeaders().get("X-Requested-With");
 
         //if header is required and not present, send an error
-        if(isRequestedWithHeaderRequired && StringUtils.isBlank(requestedWithHeader)) {
-            res.sendError(400, "All requests must have 'X-Requested-With' header");
+        if (header == null || header.isEmpty() || StringUtils.isBlank(header.get(0))) {
+            requestContext.abortWith(Response.status(400).entity("All requests must have 'X-Requested-With' header").build());
         }
-        else {
-            chain.doFilter(request, response);
-        }
-
     }
-
-    public boolean isRequestedWithHeaderRequired() {
-        return isRequestedWithHeaderRequired;
-    }
-
-    @Override
-    public void destroy() {}
-}
+}

server/test/com/mirth/connect/server/api/providers/RequestedWithFilterTest.java

@@ -2,10 +2,9 @@
 
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
-import javax.servlet.FilterChain;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
+import javax.ws.rs.container.ContainerRequestContext;
 
 import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.junit.Test;
@@ -24,24 +23,25 @@ public class RequestedWithFilterTest extends TestCase {
     public void testConstructor() {
 
         mirthProperties.setProperty("server.api.require-requested-with", "false");
-        RequestedWithFilter requestedWithFilter = new RequestedWithFilter(mirthProperties);
-        assertEquals(requestedWithFilter.isRequestedWithHeaderRequired(), false);
+        assertEquals(RequestedWithFilter.isRequestedWithHeaderRequired(), true);
+        RequestedWithFilter.configure(mirthProperties);
+        assertEquals(RequestedWithFilter.isRequestedWithHeaderRequired(), false);
     }
 
     @Test
     //assert that HttpServletResponse.sendError() is called when X-Requested-With is required but not present 
     public void testDoFilterRequestedWithTrue() {
 
         mirthProperties.setProperty("server.api.require-requested-with", "true");
-        RequestedWithFilter testFilter = new RequestedWithFilter(mirthProperties);
-
-        HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class);
-        HttpServletResponse mockResp = Mockito.mock(HttpServletResponse.class);
-        FilterChain mockFilterChain = Mockito.mock(FilterChain.class);
-
+        RequestedWithFilter.configure(mirthProperties);
+
+        ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
+        when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
+
         try {
-            testFilter.doFilter(mockReq, mockResp, mockFilterChain);
-            verify(mockResp).sendError(HttpServletResponse.SC_BAD_REQUEST, "All requests must have 'X-Requested-With' header");
+            RequestedWithFilter filter = new RequestedWithFilter();
+            filter.filter(mockCtx);
+            verify(mockCtx).abortWith(org.mockito.Matchers.any(javax.ws.rs.core.Response.class));
         } catch (Exception e) {
             e.printStackTrace();
         }
@@ -52,15 +52,15 @@ public void testDoFilterRequestedWithTrue() {
     public void testDoFilterRequestedWithFalse() {
 
         mirthProperties.setProperty("server.api.require-requested-with", "false");
-        RequestedWithFilter testFilter = new RequestedWithFilter(mirthProperties);
-
-        HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class);
-        HttpServletResponse mockResp = Mockito.mock(HttpServletResponse.class);
-        FilterChain mockFilterChain = Mockito.mock(FilterChain.class);
-
+        RequestedWithFilter.configure(mirthProperties);
+
+        ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
+        when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
+
         try {
-            testFilter.doFilter(mockReq, mockResp, mockFilterChain);
-            verify(mockResp, never()).sendError(HttpServletResponse.SC_BAD_REQUEST, "All requests must have 'X-Requested-With' header");
+            RequestedWithFilter filter = new RequestedWithFilter();
+            filter.filter(mockCtx);
+            verify(mockCtx, never()).abortWith(org.mockito.Matchers.any(javax.ws.rs.core.Response.class));
         } catch (Exception e) {
             e.printStackTrace();
         }