Skip to content

Conversation

@mgaffigan
Copy link
Contributor

  • Refactor to allow CSRF bypass (optional X-Requested-With header) on endpoints that opt-in with @DontRequireRequestedWith
  • Improve error logging for API authoring on a few sharp edges

In support for future merge of mgaffigan:feature/add-oidc-auth

@mgaffigan
Copy link
Contributor Author

@tonygermano, rewrote history here as well by topic.

@kpalang kpalang requested review from a team, jonbartels, kayyagari, kpalang and tonygermano and removed request for a team September 8, 2025 15:32
Copy link
Contributor

@kpalang kpalang left a 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?

@mgaffigan mgaffigan force-pushed the maint/add-server-error branch from c5443b7 to 8620766 Compare October 11, 2025 13:26
@mgaffigan mgaffigan requested a review from kpalang October 11, 2025 14:32
@kpalang kpalang requested a review from a team October 11, 2025 15:27
kpalang
kpalang previously approved these changes Oct 11, 2025
jonbartels
jonbartels previously approved these changes Oct 14, 2025
Copy link
Member

@tonygermano tonygermano left a 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?

@mgaffigan
Copy link
Contributor Author

@tonygermano, as far as how to test the behavior dynamically:

  1. Test an existing API endpoint without X-Requested-With: foo, observe 400 response
  2. Test an existing API endpoint with X-Requested-With: foo, observe normal response
  3. Add annotation to an endpoint (example use), test without X-Requested-With, observe normal response

@mgaffigan mgaffigan force-pushed the maint/add-server-error branch 3 times, most recently from dbdadd3 to b5a5f4d Compare November 27, 2025 00:35
jonbartels
jonbartels previously approved these changes Dec 3, 2025
@pacmano1 pacmano1 requested review from Copilot and removed request for kpalang December 9, 2025 05:27
Copy link

Copilot AI left a 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 @DontRequireRequestedWith annotation for opt-in CSRF bypass on specific endpoints
  • Migrated RequestedWithFilter from 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:
  1. A request to a method annotated with @DontRequireRequestedWith succeeds without the header
  2. A request to a class annotated with @DontRequireRequestedWith succeeds without the header
  3. The annotation is properly ignored when server.api.require-requested-with is 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.

Copy link
Member

@tonygermano tonygermano left a 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.

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Copy link
Collaborator

@NicoPiel NicoPiel left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants