-
Notifications
You must be signed in to change notification settings - Fork 39
Refactor to allow CSRF bypass, improve error logging for API authoring #176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Refactor to allow CSRF bypass, improve error logging for API authoring #176
Conversation
ab26cad to
3cba2c1
Compare
3cba2c1 to
c5443b7
Compare
|
@tonygermano, rewrote history here as well by topic. |
kpalang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why you've chosen to go with static configuration in the class instead of the previous instantiation approach?
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
c5443b7 to
8620766
Compare
tonygermano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few comments and questions. The headers should be addressed. The questions may just need to be answered with no changes if you don't think any are necessary.
I'm assuming that since there are not any methods yet which use the DontRequireRequestedWith annotation that it is not possible for someone to manually test whether it is working as intended or not?
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
server/test/com/mirth/connect/server/api/providers/RequestedWithFilterTest.java
Outdated
Show resolved
Hide resolved
server/test/com/mirth/connect/server/api/providers/RequestedWithFilterTest.java
Show resolved
Hide resolved
8620766 to
9b6b9f1
Compare
|
@tonygermano, as far as how to test the behavior dynamically:
|
dbdadd3 to
b5a5f4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the CSRF protection mechanism to support selective bypass via a @DontRequireRequestedWith annotation, and improves error logging for API provider registration issues. The changes migrate RequestedWithFilter from a servlet Filter to a JAX-RS ContainerRequestFilter to enable annotation-based access to resource metadata.
Key changes:
- Introduced
@DontRequireRequestedWithannotation for opt-in CSRF bypass on specific endpoints - Migrated
RequestedWithFilterfrom servlet Filter to JAX-RS ContainerRequestFilter with annotation checking - Enhanced error logging to include stack traces when API provider registration fails
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/com/mirth/connect/server/api/DontRequireRequestedWith.java | New annotation to mark methods/classes that don't require X-Requested-With header |
| server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java | Refactored from servlet Filter to JAX-RS ContainerRequestFilter with annotation-based bypass logic |
| server/test/com/mirth/connect/server/api/providers/RequestedWithFilterTest.java | Updated tests to match new ContainerRequestFilter implementation |
| server/src/com/mirth/connect/server/MirthWebServer.java | Updated filter configuration to use static configure() method and register filter class |
| server/src/com/mirth/connect/server/api/MirthServlet.java | Added null check with helpful error message for operation parameter |
| server/src/com/mirth/connect/client/core/Client.java | Enhanced error logging to include throwable for better debugging |
Comments suppressed due to low confidence (1)
server/test/com/mirth/connect/server/api/providers/RequestedWithFilterTest.java:80
- The test suite is missing coverage for the new annotation-based bypass feature. Consider adding tests that verify:
- A request to a method annotated with
@DontRequireRequestedWithsucceeds without the header - A request to a class annotated with
@DontRequireRequestedWithsucceeds without the header - The annotation is properly ignored when
server.api.require-requested-withis set to false
This is important functionality that should be tested to prevent regressions.
@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.configure(mirthProperties);
ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
try {
RequestedWithFilter filter = new RequestedWithFilter();
filter.filter(mockCtx);
ArgumentCaptor<Response> responseCaptor =
ArgumentCaptor.forClass(Response.class);
verify(mockCtx).abortWith(responseCaptor.capture());
Response response = responseCaptor.getValue();
assertEquals(400, response.getStatus());
assertEquals("All requests must have 'X-Requested-With' header", response.getEntity());
} catch (Exception e) {
e.printStackTrace();
}
}
@Test
//assert that HttpServletResponse.sendError() is NOT called when X-Requested-With is not required and not present
public void testDoFilterRequestedWithFalse() {
mirthProperties.setProperty("server.api.require-requested-with", "false");
RequestedWithFilter.configure(mirthProperties);
ContainerRequestContext mockCtx = Mockito.mock(ContainerRequestContext.class);
when(mockCtx.getHeaders()).thenReturn(new javax.ws.rs.core.MultivaluedHashMap<String, String>());
try {
RequestedWithFilter filter = new RequestedWithFilter();
filter.filter(mockCtx);
verify(mockCtx, never()).abortWith(ArgumentMatchers.any(javax.ws.rs.core.Response.class));
} catch (Exception e) {
e.printStackTrace();
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Outdated
Show resolved
Hide resolved
tonygermano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replying to the copilot comments. One is to be ignored, one is a nitpick, and one requires changes.
I think I'm good to approve after the required changes are made.
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Show resolved
Hide resolved
server/src/com/mirth/connect/server/api/providers/RequestedWithFilter.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
b5a5f4d to
61ee4d7
Compare
61ee4d7 to
90d853d
Compare
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
90d853d to
d387729
Compare
NicoPiel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Main concerns have already been addressed.
X-Requested-Withheader) on endpoints that opt-in with@DontRequireRequestedWithIn support for future merge of mgaffigan:feature/add-oidc-auth