Skip to content

Commit 3cba2c1

Browse files
mgaffiganmgaffigan
committed
Refactor to allow CSRF bypass, improve error logging for API authoring
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
1 parent 7ecf54b commit 3cba2c1

File tree

6 files changed

+87
-66
lines changed

6 files changed

+87
-66
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ public Connector getConnector(javax.ws.rs.client.Client client, Configuration ru
196196
try {
197197
config.register(Class.forName(apiProviderClass));
198198
} catch (Throwable t) {
199-
logger.error("Error registering API provider class: " + apiProviderClass);
199+
logger.error("Error registering API provider class: " + apiProviderClass, t);
200200
}
201201
}
202202
}
@@ -219,7 +219,7 @@ public void registerApiProviders(Set<String> packageNames, Set<String> classes)
219219
client.register(clazz);
220220
}
221221
} catch (Throwable t) {
222-
logger.error("Error registering API provider package: " + packageName);
222+
logger.error("Error registering API provider package: " + packageName, t);
223223
}
224224
}
225225
}
@@ -229,7 +229,7 @@ public void registerApiProviders(Set<String> packageNames, Set<String> classes)
229229
try {
230230
client.register(Class.forName(clazz));
231231
} catch (Throwable t) {
232-
logger.error("Error registering API provider class: " + clazz);
232+
logger.error("Error registering API provider class: " + clazz, t);
233233
}
234234
}
235235
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ private ServletContextHandler createApiServletContextHandler(String contextPath,
454454
apiServletContextHandler.setContextPath(contextPath + baseAPI + apiPath);
455455
apiServletContextHandler.addFilter(new FilterHolder(new ApiOriginFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
456456
apiServletContextHandler.addFilter(new FilterHolder(new ClickjackingFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
457-
apiServletContextHandler.addFilter(new FilterHolder(new RequestedWithFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
457+
RequestedWithFilter.configure(mirthProperties);
458458
apiServletContextHandler.addFilter(new FilterHolder(new MethodFilter()), "/*", EnumSet.of(DispatcherType.REQUEST));
459459
apiServletContextHandler.addFilter(new FilterHolder(new StrictTransportSecurityFilter(mirthProperties)), "/*", EnumSet.of(DispatcherType.REQUEST));
460460
setConnectorNames(apiServletContextHandler, apiAllowHTTP);

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package com.mirth.connect.server.api;
2+
3+
import java.lang.annotation.ElementType;
4+
import java.lang.annotation.Retention;
5+
import java.lang.annotation.RetentionPolicy;
6+
import java.lang.annotation.Target;
7+
8+
/**
9+
* If this annotation is present on a method or class, the X-Requested-With header
10+
* requirement will not be enforced for that resource.
11+
*/
12+
@Target({ElementType.METHOD, ElementType.TYPE})
13+
@Retention(RetentionPolicy.RUNTIME)
14+
public @interface DontRequireRequestedWith {
15+
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,9 @@ private void setContext() {
210210
}
211211

212212
public void setOperation(Operation operation) {
213+
if (operation == null) {
214+
throw new MirthApiException("Method operation cannot be null.");
215+
}
213216
if (extensionName != null) {
214217
operation = new ExtensionOperation(extensionName, operation);
215218
}

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

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,64 +1,67 @@
1-
/*
2-
* Copyright (c) Mirth Corporation. All rights reserved.
3-
*
4-
* http://www.mirthcorp.com
5-
*
6-
* The software in this package is published under the terms of the MPL license a copy of which has
7-
* been included with this distribution in the LICENSE.txt file.
8-
*/
9-
101
package com.mirth.connect.server.api.providers;
112

123
import java.io.IOException;
4+
import java.lang.reflect.Method;
5+
import java.util.List;
136

14-
import javax.servlet.Filter;
15-
import javax.servlet.FilterChain;
16-
import javax.servlet.FilterConfig;
17-
import javax.servlet.ServletException;
18-
import javax.servlet.ServletRequest;
19-
import javax.servlet.ServletResponse;
20-
import javax.servlet.http.HttpServletRequest;
21-
import javax.servlet.http.HttpServletResponse;
7+
import javax.annotation.Priority;
8+
import javax.ws.rs.Priorities;
9+
import javax.ws.rs.container.ContainerRequestContext;
10+
import javax.ws.rs.container.ContainerRequestFilter;
11+
import javax.ws.rs.container.ResourceInfo;
12+
import javax.ws.rs.core.Context;
13+
import javax.ws.rs.core.Response;
2214
import javax.ws.rs.ext.Provider;
2315

2416
import org.apache.commons.configuration2.PropertiesConfiguration;
2517
import org.apache.commons.lang3.StringUtils;
2618

19+
import com.mirth.connect.server.api.DontRequireRequestedWith;
20+
2721
@Provider
28-
public class RequestedWithFilter implements Filter {
22+
@Priority(Priorities.AUTHENTICATION + 100)
23+
public class RequestedWithFilter implements ContainerRequestFilter {
2924

30-
private boolean isRequestedWithHeaderRequired = true;
25+
@Context
26+
private ResourceInfo resourceInfo;
3127

28+
private static boolean isRequestedWithHeaderRequired = true;
3229

33-
public RequestedWithFilter(PropertiesConfiguration mirthProperties) {
34-
30+
// Jax requires a no-arg constructor to instantiate providers via classpath scanning.
31+
public RequestedWithFilter() {
32+
}
33+
34+
public static void configure(PropertiesConfiguration mirthProperties) {
3535
isRequestedWithHeaderRequired = mirthProperties.getBoolean("server.api.require-requested-with", true);
3636
}
3737

38-
@Override
39-
public void init(FilterConfig filterConfig) throws ServletException {}
38+
public static boolean isRequestedWithHeaderRequired() {
39+
return isRequestedWithHeaderRequired;
40+
}
4041

4142
@Override
42-
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
43-
HttpServletResponse res = (HttpServletResponse) response;
43+
public void filter(ContainerRequestContext requestContext) throws IOException {
44+
if (!isRequestedWithHeaderRequired) {
45+
return;
46+
}
47+
48+
// If the resource method or class is annotated with DontRequireRequestedWith, skip the check
49+
if (resourceInfo != null) {
50+
Method method = resourceInfo.getResourceMethod();
51+
if (method != null && method.getAnnotation(DontRequireRequestedWith.class) != null) {
52+
return;
53+
}
54+
Class<?> resourceClass = resourceInfo.getResourceClass();
55+
if (resourceClass != null && resourceClass.getAnnotation(DontRequireRequestedWith.class) != null) {
56+
return;
57+
}
58+
}
4459

45-
HttpServletRequest servletRequest = (HttpServletRequest)request;
46-
String requestedWithHeader = (String) servletRequest.getHeader("X-Requested-With");
60+
List<String> header = requestContext.getHeaders().get("X-Requested-With");
4761

4862
//if header is required and not present, send an error
49-
if(isRequestedWithHeaderRequired && StringUtils.isBlank(requestedWithHeader)) {
50-
res.sendError(400, "All requests must have 'X-Requested-With' header");
63+
if (header == null || header.isEmpty() || StringUtils.isBlank(header.get(0))) {
64+
requestContext.abortWith(Response.status(400).entity("All requests must have 'X-Requested-With' header").build());
5165
}
52-
else {
53-
chain.doFilter(request, response);
54-
}
55-
5666
}
57-
58-
public boolean isRequestedWithHeaderRequired() {
59-
return isRequestedWithHeaderRequired;
60-
}
61-
62-
@Override
63-
public void destroy() {}
64-
}
67+
}

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

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,9 @@
22

33
import static org.mockito.Mockito.never;
44
import static org.mockito.Mockito.verify;
5+
import static org.mockito.Mockito.when;
56

6-
import javax.servlet.FilterChain;
7-
import javax.servlet.http.HttpServletRequest;
8-
import javax.servlet.http.HttpServletResponse;
7+
import javax.ws.rs.container.ContainerRequestContext;
98

109
import org.apache.commons.configuration2.PropertiesConfiguration;
1110
import org.junit.Test;
@@ -24,24 +23,25 @@ public class RequestedWithFilterTest extends TestCase {
2423
public void testConstructor() {
2524

2625
mirthProperties.setProperty("server.api.require-requested-with", "false");
27-
RequestedWithFilter requestedWithFilter = new RequestedWithFilter(mirthProperties);
28-
assertEquals(requestedWithFilter.isRequestedWithHeaderRequired(), false);
26+
assertEquals(RequestedWithFilter.isRequestedWithHeaderRequired(), true);
27+
RequestedWithFilter.configure(mirthProperties);
28+
assertEquals(RequestedWithFilter.isRequestedWithHeaderRequired(), false);
2929
}
3030

3131
@Test
3232
//assert that HttpServletResponse.sendError() is called when X-Requested-With is required but not present
3333
public void testDoFilterRequestedWithTrue() {
3434

3535
mirthProperties.setProperty("server.api.require-requested-with", "true");
36-
RequestedWithFilter testFilter = new RequestedWithFilter(mirthProperties);
37-
38-
HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class);
39-
HttpServletResponse mockResp = Mockito.mock(HttpServletResponse.class);
40-
FilterChain mockFilterChain = Mockito.mock(FilterChain.class);
41-
36+
RequestedWithFilter.configure(mirthProperties);
37+
38+
ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
39+
when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
40+
4241
try {
43-
testFilter.doFilter(mockReq, mockResp, mockFilterChain);
44-
verify(mockResp).sendError(HttpServletResponse.SC_BAD_REQUEST, "All requests must have 'X-Requested-With' header");
42+
RequestedWithFilter filter = new RequestedWithFilter();
43+
filter.filter(mockCtx);
44+
verify(mockCtx).abortWith(org.mockito.Matchers.any(javax.ws.rs.core.Response.class));
4545
} catch (Exception e) {
4646
e.printStackTrace();
4747
}
@@ -52,15 +52,15 @@ public void testDoFilterRequestedWithTrue() {
5252
public void testDoFilterRequestedWithFalse() {
5353

5454
mirthProperties.setProperty("server.api.require-requested-with", "false");
55-
RequestedWithFilter testFilter = new RequestedWithFilter(mirthProperties);
56-
57-
HttpServletRequest mockReq = Mockito.mock(HttpServletRequest.class);
58-
HttpServletResponse mockResp = Mockito.mock(HttpServletResponse.class);
59-
FilterChain mockFilterChain = Mockito.mock(FilterChain.class);
60-
55+
RequestedWithFilter.configure(mirthProperties);
56+
57+
ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
58+
when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
59+
6160
try {
62-
testFilter.doFilter(mockReq, mockResp, mockFilterChain);
63-
verify(mockResp, never()).sendError(HttpServletResponse.SC_BAD_REQUEST, "All requests must have 'X-Requested-With' header");
61+
RequestedWithFilter filter = new RequestedWithFilter();
62+
filter.filter(mockCtx);
63+
verify(mockCtx, never()).abortWith(org.mockito.Matchers.any(javax.ws.rs.core.Response.class));
6464
} catch (Exception e) {
6565
e.printStackTrace();
6666
}

0 commit comments

Comments
 (0)