Skip to content
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

DispatcherServletDelegatingRequestMatcher causes errors when there is more than one ServletContext #14418

Closed
LewisMcReu opened this issue Jan 8, 2024 · 15 comments · Fixed by #15195
Assignees
Labels
in: test An issue in spring-security-test type: bug A general bug
Milestone

Comments

@LewisMcReu
Copy link

LewisMcReu commented Jan 8, 2024

Describe the bug
In a Spring Boot application with multiple servlets registered to the context (DispatcherServlet and at least one other), an IllegalArgumentException with message Failed to find servlet [] in the servlet context is thrown when running tests with MockMvc and @SpringBootTest(webEnvironment = RANDOM_PORT).

The problem started occurring after an upgrade from Spring Boot 3.0.2 to Spring Boot 3.2.1 (Spring Security 6.2.1).

The exception is thrown in the DispatcherServletDelegatingRequestMatcher due to an incompatibility with the standard MockHttpServletRequest instances created with the MockMvcRequestBuilders class. These have a generic HttpRequestMapping with an empty String as the servletName, thus causing the Matcher to not find a ServletRegistration and throw the exception.

Gist with full stacktrace: Gist

To Reproduce
Run the tests in the provided sample to reproduce.

In the sample is a simple Spring Boot app with an extra servlet registered to the context and a basic security setup.
The included tests demonstrate 2 working cases and one failing case.
The test TestWithRandomPortEnvironment#expected_failure demonstrates the exception.
TestWithRandomPortEnvironment#expected_success demonstrates the fix I implemented to make this work with a RequestPostProcessor, but this solution would have to be applied everywhere I'm using MockMvc.

Expected behavior
The DispatcherServletDelegatingRequestMatcher should be compatible with the standard MockHttpServletRequest instances created with the MockMvcHttpRequestBuilders and not throw an exception.

Sample
Sample repository

@LewisMcReu LewisMcReu added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jan 8, 2024
@LewisMcReu LewisMcReu changed the title DispatcherServletDelegatingRequestMatcher causes errors when running in tests with ServletContext containing multiple servlets. DispatcherServletDelegatingRequestMatcher causes errors when running tests with MockMvc Jan 8, 2024
@atrepczik
Copy link

I have a very similar problem that is not just limited to running tests with MockMvc. It can also happen in a Spring Boot application at runtime, if all of the following conditions are given:

  1. management.server.port=8081 or anything that is not the same as server.port
  2. Use securityMatcher in Spring Security DSL
  3. Register a custom servlet
  4. Start the application and try to access a non-existing endpoint on the management port, e.g. curl -i http://localhost:8081/foo

This will get you an internal server error with HTTP status code 500. With Spring Boot 3.1.0, you would see a 404 as expected for a non-existing endpoint.

The stacktrace is similar to the one from the initial post, but the message is slightly different. In my case, the servlet it fails to find does have a non-empty name: Failed to find servlet [dispatcherServletRegistration] in the servlet context

The sample repository from the initial post seems unavailable. I have created another sample repository with a sample application to reproduce the issue I described: https://github.com/atrepczik/spring-security-request-matching-issue

@Sax388
Copy link

Sax388 commented Feb 1, 2024

@LewisMcReu Could you perhaps add the fix you had in mind? I didn't manage to (yet) and the repository you mentioned is empty right now.

Nevermind,... I just figured it out I suppose:

  static class FixMissingServletPathProcessor implements RequestPostProcessor {

    @Override
    public MockHttpServletRequest postProcessRequest(MockHttpServletRequest request) {

      request.setHttpServletMapping(new HttpServletMapping() {
        @Override
        public String getMatchValue() {
          return "";
        }

        @Override
        public String getPattern() {
          return "";
        }

        @Override
        public String getServletName() {
          return "dispatcherServlet";
        }

        @Override
        public MappingMatch getMappingMatch() {
          return MappingMatch.PATH;
        }
      });

      request.setContextPath("/");

      return request;
    }
  }

@sabinisshrestha
Copy link

Did anyone solve the issue. I am looking to solve this for a while. How do I get past this issue?

@seschi98
Copy link

I have a similar issue and spent the last 3 days trying to reproduce it and find the exact version of spring-boot / spring-security where it occured for the first time.

Scenario:

  • Spring Boot 3.1.x (just for testing, problem also occurs in 3.2.x)
  • Management server on different port than main server (in my case :8081)
  • Configured SecurityFilterChain
  • Additional servlet configured (in my "real" project this is done by a third-party dependency, in the demo I did it manually)

Link to my example repo: https://github.com/seschi98/demo-spring-dispatcher-servlet-error

With Spring Boot 3.1.1 (Security 6.1.1) everything works as expected. No errors on startup, localhost:8081/actuator/health displays {"status": "UP"}. Nice!

With Spring Boot 3.1.2 (Security 6.1.2) the server won't start anymore. I could backtrack this problem to cf2c8da

With Spring Boot 3.1.3 (Security 6.1.3) its the same issue, just with a better error message. See 0df1884

With Spring Boot 3.1.4 (Security 6.1.4), Spring Boot 3.1.5 (Security 6.1.5) and Spring Boot 3.1.6 (Security 6.1.5) nothing changed.

With Spring Boot 3.1.7 (Security 6.1.6) the server starts again (yayyy), probably thanks to 624dcaf. Unfortunately when you call localhost:8081/actuator/health now, it throws the HTTP 500 error that was mentioned before:

java.lang.IllegalArgumentException: Failed to find servlet [dispatcherServletRegistration] in the servlet context
	at org.springframework.util.Assert.notNull(Assert.java:204) ~[spring-core-6.0.15.jar:6.0.15]
	at org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry$DispatcherServletDelegatingRequestMatcher.matcher(AbstractRequestMatcherRegistry.java:544) ~[spring-security-config-6.1.6.jar:6.1.6]

If I remove the line

management.server.port: 8081

from my application.yaml, the health endpoint works normally (of course now with port :8080).

I hope this information helps understanding the issue and someone with better knowledge of the spring-security internals will be able to figure out a solution for this.

@piotrszczepankowski
Copy link

I have the same problem. On our side it started to appear after adding random port to spring boot test annotation :(

Not working: @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
Working: @SpringBootTest

Spring 3.2.2:
Exception:
java.lang.IllegalArgumentException: Failed to find servlet [] in the servlet context

@nstuart-idexx
Copy link

nstuart-idexx commented Mar 5, 2024

Having the same problem, but only with some of our applications. Digging into the difference, the problem is with the
DispatcherServletDelegatingRequestMatcher and only comes into play if there are more than one Servlet registrations, for us at least. I have not tied it to the Random port, as our app does in fact have two servlet registrations so I debugged just that configuration.

The key decision point is in AbstractRequestMatcherRegistry, here.

It does NOT use the MVC matcher, but picks the Delegating matcher instead. The delegating matcher does not seem to work with the MockMvc request as the servlet name is in fact null when it gets to here in DispatcherServletDelegatingRequestMatcher.

AbstractRequestMatcherRegistry does say it's new since 3.2, so seems like a regression when using MockMvc in tests.

@amb-sebastian-podgorski-pt
Copy link

amb-sebastian-podgorski-pt commented Mar 27, 2024

I faced same issue in my service, the problem is when you are using:

config.requestMatchers("/hello").permitAll();

then it delegates creating AntPathRequestMatcher to AbstractRequestMatcherRegistry here.

After that the resolve method is called (this one) which checks if there is only one dispatcherServlet in context configured (here). And when you declare additional dispatcherServlet then ant and mvc mapper is wrapped within DispatcherServletDelegatingRequestMatcher which is looking for bean with the name: dispatcherServletRegistration which is initialized in LAZY MODE.

In other words it is initialized after first call for different port e.g. localhost:8081/actuator/health so It can not be find in the context which has been configured during app initialization and as a result we are facing this issue. The simples fix is to just declare your own AntPathRequestMatcher and this will skip the steps I described before e.g.
config.requestMatchers(AntPathRequestMatcher.antMatcher("/hello")).permitAll();

But still IMHO it's bug within DispatcherServletDelegatingRequestMatcher which has to be addressed and fixed

@freddiN
Copy link

freddiN commented Apr 16, 2024

This is not only happening in tests:
https://stackoverflow.com/questions/77710370/actuator-endpoints-returning-500-error-after-upgrade-to-spring-boot-3
https://stackoverflow.com/questions/77785192/spring-boot-3-missing-dispatcherservletregistration-bean-in-actuator

I sprinkled some "AntPathRequestMatcher" calls around in my code, but imho this needs more attention as its a regression (that strangely enough not more people run into)

@laguiar
Copy link

laguiar commented Apr 16, 2024

So, no official response from spring-security team yet about this bug 😞
Got all the same issues described above after 3.2.x migration.

@DeRickulous
Copy link

DeRickulous commented May 30, 2024

As freddiN said above, this is not exclusive to tests. I used the following code as a hack for an affected application:

[code removed]

I can confirm that the code fix linked by @jzheaux below works like a charm.

@jzheaux jzheaux added in: test An issue in spring-security-test and removed status: waiting-for-triage An issue we've not yet triaged labels May 31, 2024
@jzheaux jzheaux self-assigned this May 31, 2024
@jzheaux jzheaux modified the milestone: 6.2.x May 31, 2024
jzheaux added a commit to jzheaux/spring-security that referenced this issue May 31, 2024
Because test do not always attach a fully-formulated ServletContext
to the mock request, it's beneficial to consult the ServletContext
presented during application startup.

In case the request does have needed material, though, it maybe
be helpful for Spring Security to first consult the request.

Issue spring-projectsgh-14418
@jzheaux
Copy link
Contributor

jzheaux commented Jun 1, 2024

Thanks for the reports and for your patience as this gets addressed.

A workaround for this failure is to instead use requestMatchers(requestMatcher) like so:

@Bean 
MvcRequestMatcher.Builder mvc(HandlerMappingIntrospector introspector) {
    return new MvcRequestMatcher.Builder(introspector);
}

@Bean
@Order(1)
public SecurityFilterChain dummyFilterChain(HttpSecurity http, MvcRequestMatcher.Builder mvc) throws Exception {

        // ...

        http.authorizeHttpRequests(config -> {
            config.requestMatchers(mvc.pattern("/hello")).permitAll();
            config.requestMatchers(mvc.pattern("/actuator"), mvc.pattern("/actuator/**"), mvc.pattern("/readyz"), mvc.pattern("/livez")).permitAll();
            config.anyRequest().fullyAuthenticated();
        });

        // ...

        return http.build();
    }

That said, I am investigating a fix to learn about its implications. I'm targeting the next milestone release to have a fix out.

jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
Spring Security cannot use the ServletContext attached
to the ApplicationContext since there may be child
ApplicationContext's with their own ServletContext.

Because of that, it is necessary to always use the
ServletContext attached to the request.

Closes spring-projectsgh-14418
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
Spring Security cannot use the ServletContext attached
to the ApplicationContext since there may be child
ApplicationContext's with their own ServletContext.

Because of that, it is necessary to always use the
ServletContext attached to the request.

Closes spring-projectsgh-14418
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
jzheaux added a commit to jzheaux/spring-security that referenced this issue Jun 3, 2024
Spring Security cannot use the ServletContext attached
to the ApplicationContext since there may be child
ApplicationContext's with their own ServletContext.

Because of that, it is necessary to always use the
ServletContext attached to the request.

Closes spring-projectsgh-14418
jzheaux added a commit that referenced this issue Jun 3, 2024
@jzheaux jzheaux closed this as completed in cdd6266 Jun 3, 2024
@jzheaux jzheaux modified the milestones: 6.2.x, 5.8.13 Jun 3, 2024
@jzheaux jzheaux changed the title DispatcherServletDelegatingRequestMatcher causes errors when running tests with MockMvc DispatcherServletDelegatingRequestMatcher causes errors when there is more than one ServletContext Jun 3, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Jun 3, 2024

Reopening to take a look the OP's use case, which is separate from some of the non-test-related reports here.

@jzheaux jzheaux reopened this Jun 3, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Jun 4, 2024

@atrepczik, regarding the bug you are seeing with MockMvc, the above commit will not resolve things completely.

As a bit of background, when using requestMatchers(String), Spring Security doesn't know whether it is an MVC endpoint or not. So, in applications that have both MVC and non-MVC endpoints (the custom servlet in your case), Spring Security needs more information.

Spring Security could possibly use the fact that it's running inside a MockMvc test, though I'm not clear of a generic way to do that. I'll reach out to the Spring Web team to see what they think.

In the meantime, you can use requestMatcher(RequestMatcher) instead. Or, if you update to the latest SNAPSHOT, you instead only need to update your tests like so:

this.mvc.perform(get("/").with((request) -> {
	request.setHttpServletMapping(new MockHttpServletMapping("/", "/", "dispatcherServlet", MappingMatch.PATH));
	return request;
})).andExpect(status().isOk());

Or, if you want it to be a little bit more generic, you can do:

private static RequestPostProcessor mvcMapping() {
	return (request) -> {
		String matchValue = request.getRequestURI();
		String servlet = "dispatcherServlet";
		String pattern = request.getServletContext().getServletRegistration(servlet).getMappings().iterator().next();
		HttpServletMapping mapping = new MockHttpServletMapping(matchValue, pattern, servlet, MappingMatch.PATH);
		request.setHttpServletMapping(mapping);
		return request;
	};
}

// ...

this.mvc.perform(get("/").with(mvcMapping()).andExpect(status().isOk());

@jzheaux
Copy link
Contributor

jzheaux commented Jun 6, 2024

With #13849 closed, the MockMvc tests in @atrepczik's sample all pass now. Please confirm by using the latest 5.8, 6.2, 6.3, or 6.4 snapshot.

@jzheaux jzheaux closed this as completed Jun 6, 2024
@atrepczik
Copy link

Can confirm there's no longer an error with spring-security 6.3.1-SNAPSHOT. Thanks a lot for fixing!

@jzheaux jzheaux moved this to Done in Spring Security Team Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test An issue in spring-security-test type: bug A general bug
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.