Skip to content

Commit df239b6

Browse files
jzheauxjzheaux
committed
Improve RequestMatcher Validation
Closes gh-13551
1 parent a939f17 commit df239b6

File tree

3 files changed

+148
-17
lines changed

3 files changed

+148
-17
lines changed

‎config/src/main/java/org/springframework/security/config/annotation/web/AbstractRequestMatcherRegistry.java

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,11 @@
1919
import java.util.ArrayList;
2020
import java.util.Arrays;
2121
import java.util.List;
22+
import java.util.Map;
2223

2324
import javax.servlet.DispatcherType;
25+
import javax.servlet.ServletContext;
26+
import javax.servlet.ServletRegistration;
2427

2528
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
2629
import org.springframework.context.ApplicationContext;
@@ -36,6 +39,7 @@
3639
import org.springframework.security.web.util.matcher.RequestMatcher;
3740
import org.springframework.util.Assert;
3841
import org.springframework.util.ClassUtils;
42+
import org.springframework.web.context.WebApplicationContext;
3943
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;
4044

4145
/**
@@ -297,14 +301,47 @@ public C requestMatchers(RequestMatcher... requestMatchers) {
297301
* @since 5.8
298302
*/
299303
public C requestMatchers(HttpMethod method, String... patterns) {
300-
List<RequestMatcher> matchers = new ArrayList<>();
301-
if (mvcPresent) {
302-
matchers.addAll(createMvcMatchers(method, patterns));
304+
if (!mvcPresent) {
305+
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
306+
}
307+
if (!(this.context instanceof WebApplicationContext)) {
308+
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
309+
}
310+
WebApplicationContext context = (WebApplicationContext) this.context;
311+
ServletContext servletContext = context.getServletContext();
312+
if (servletContext == null) {
313+
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
314+
}
315+
Map<String, ? extends ServletRegistration> registrations = servletContext.getServletRegistrations();
316+
if (registrations == null) {
317+
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
318+
}
319+
if (!hasDispatcherServlet(registrations)) {
320+
return requestMatchers(RequestMatchers.antMatchersAsArray(method, patterns));
303321
}
304-
else {
305-
matchers.addAll(RequestMatchers.antMatchers(method, patterns));
322+
Assert.isTrue(registrations.size() == 1,
323+
"This method cannot decide whether these patterns are Spring MVC patterns or not. If this endpoint is a Spring MVC endpoint, please use requestMatchers(MvcRequestMatcher); otherwise, please use requestMatchers(AntPathRequestMatcher).");
324+
return requestMatchers(createMvcMatchers(method, patterns).toArray(new RequestMatcher[0]));
325+
}
326+
327+
private boolean hasDispatcherServlet(Map<String, ? extends ServletRegistration> registrations) {
328+
if (registrations == null) {
329+
return false;
330+
}
331+
Class<?> dispatcherServlet = ClassUtils.resolveClassName("org.springframework.web.servlet.DispatcherServlet",
332+
null);
333+
for (ServletRegistration registration : registrations.values()) {
334+
try {
335+
Class<?> clazz = Class.forName(registration.getClassName());
336+
if (dispatcherServlet.isAssignableFrom(clazz)) {
337+
return true;
338+
}
339+
}
340+
catch (ClassNotFoundException ex) {
341+
return false;
342+
}
306343
}
307-
return requestMatchers(matchers.toArray(new RequestMatcher[0]));
344+
return false;
308345
}
309346

310347
/**
@@ -380,12 +417,7 @@ private RequestMatchers() {
380417
* @return a {@link List} of {@link AntPathRequestMatcher} instances
381418
*/
382419
static List<RequestMatcher> antMatchers(HttpMethod httpMethod, String... antPatterns) {
383-
String method = (httpMethod != null) ? httpMethod.toString() : null;
384-
List<RequestMatcher> matchers = new ArrayList<>();
385-
for (String pattern : antPatterns) {
386-
matchers.add(new AntPathRequestMatcher(pattern, method));
387-
}
388-
return matchers;
420+
return Arrays.asList(antMatchersAsArray(httpMethod, antPatterns));
389421
}
390422

391423
/**
@@ -399,6 +431,15 @@ static List<RequestMatcher> antMatchers(String... antPatterns) {
399431
return antMatchers(null, antPatterns);
400432
}
401433

434+
static RequestMatcher[] antMatchersAsArray(HttpMethod httpMethod, String... antPatterns) {
435+
String method = (httpMethod != null) ? httpMethod.toString() : null;
436+
RequestMatcher[] matchers = new RequestMatcher[antPatterns.length];
437+
for (int index = 0; index < antPatterns.length; index++) {
438+
matchers[index] = new AntPathRequestMatcher(antPatterns[index], method);
439+
}
440+
return matchers;
441+
}
442+
402443
/**
403444
* Create a {@link List} of {@link RegexRequestMatcher} instances.
404445
* @param httpMethod the {@link HttpMethod} to use or {@code null} for any

‎config/src/test/java/org/springframework/security/config/annotation/web/AbstractRequestMatcherRegistryTests.java

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,16 @@
1818

1919
import java.lang.reflect.Field;
2020
import java.lang.reflect.Modifier;
21+
import java.util.LinkedHashMap;
2122
import java.util.List;
23+
import java.util.Map;
2224

2325
import javax.servlet.DispatcherType;
26+
import javax.servlet.Servlet;
27+
import javax.servlet.ServletContext;
28+
import javax.servlet.ServletRegistration;
2429

30+
import org.jetbrains.annotations.NotNull;
2531
import org.junit.jupiter.api.BeforeEach;
2632
import org.junit.jupiter.api.Test;
2733

@@ -34,6 +40,8 @@
3440
import org.springframework.security.web.util.matcher.DispatcherTypeRequestMatcher;
3541
import org.springframework.security.web.util.matcher.RegexRequestMatcher;
3642
import org.springframework.security.web.util.matcher.RequestMatcher;
43+
import org.springframework.web.context.WebApplicationContext;
44+
import org.springframework.web.servlet.DispatcherServlet;
3745

3846
import static org.assertj.core.api.Assertions.assertThat;
3947
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -56,12 +64,17 @@ public <O> O postProcess(O object) {
5664

5765
private TestRequestMatcherRegistry matcherRegistry;
5866

67+
private WebApplicationContext context;
68+
5969
@BeforeEach
6070
public void setUp() {
6171
this.matcherRegistry = new TestRequestMatcherRegistry();
62-
ApplicationContext context = mock(ApplicationContext.class);
63-
given(context.getBean(ObjectPostProcessor.class)).willReturn(NO_OP_OBJECT_POST_PROCESSOR);
64-
this.matcherRegistry.setApplicationContext(context);
72+
this.context = mock(WebApplicationContext.class);
73+
ServletContext servletContext = new MockServletContext();
74+
servletContext.addServlet("dispatcherServlet", DispatcherServlet.class);
75+
given(this.context.getBean(ObjectPostProcessor.class)).willReturn(NO_OP_OBJECT_POST_PROCESSOR);
76+
given(this.context.getServletContext()).willReturn(servletContext);
77+
this.matcherRegistry.setApplicationContext(this.context);
6578
}
6679

6780
@Test
@@ -184,6 +197,32 @@ public void requestMatchersWhenMvcPresentInClassPathAndMvcIntrospectorBeanNotAva
184197
"Please ensure Spring Security & Spring MVC are configured in a shared ApplicationContext");
185198
}
186199

200+
@Test
201+
public void requestMatchersWhenNoDispatcherServletThenAntPathRequestMatcherType() {
202+
MockServletContext servletContext = new MockServletContext();
203+
given(this.context.getServletContext()).willReturn(servletContext);
204+
List<RequestMatcher> requestMatchers = this.matcherRegistry.requestMatchers("/**");
205+
assertThat(requestMatchers).isNotEmpty();
206+
assertThat(requestMatchers).hasSize(1);
207+
assertThat(requestMatchers.get(0)).isExactlyInstanceOf(AntPathRequestMatcher.class);
208+
servletContext.addServlet("servletOne", Servlet.class);
209+
servletContext.addServlet("servletTwo", Servlet.class);
210+
requestMatchers = this.matcherRegistry.requestMatchers("/**");
211+
assertThat(requestMatchers).isNotEmpty();
212+
assertThat(requestMatchers).hasSize(1);
213+
assertThat(requestMatchers.get(0)).isExactlyInstanceOf(AntPathRequestMatcher.class);
214+
}
215+
216+
@Test
217+
public void requestMatchersWhenAmbiguousServletsThenException() {
218+
MockServletContext servletContext = new MockServletContext();
219+
given(this.context.getServletContext()).willReturn(servletContext);
220+
servletContext.addServlet("dispatcherServlet", DispatcherServlet.class);
221+
servletContext.addServlet("servletTwo", Servlet.class);
222+
assertThatExceptionOfType(IllegalArgumentException.class)
223+
.isThrownBy(() -> this.matcherRegistry.requestMatchers("/**"));
224+
}
225+
187226
private void mockMvcIntrospector(boolean isPresent) {
188227
ApplicationContext context = this.matcherRegistry.getApplicationContext();
189228
given(context.containsBean("mvcHandlerMappingIntrospector")).willReturn(isPresent);
@@ -217,4 +256,25 @@ protected List<RequestMatcher> chainRequestMatchers(List<RequestMatcher> request
217256

218257
}
219258

259+
private static class MockServletContext extends org.springframework.mock.web.MockServletContext {
260+
261+
private final Map<String, ServletRegistration> registrations = new LinkedHashMap<>();
262+
263+
@NotNull
264+
@Override
265+
public ServletRegistration.Dynamic addServlet(@NotNull String servletName, Class<? extends Servlet> clazz) {
266+
ServletRegistration.Dynamic dynamic = mock(ServletRegistration.Dynamic.class);
267+
given(dynamic.getClassName()).willReturn(clazz.getName());
268+
this.registrations.put(servletName, dynamic);
269+
return dynamic;
270+
}
271+
272+
@NotNull
273+
@Override
274+
public Map<String, ? extends ServletRegistration> getServletRegistrations() {
275+
return this.registrations;
276+
}
277+
278+
}
279+
220280
}

‎config/src/test/java/org/springframework/security/config/annotation/web/configurers/HttpSecuritySecurityMatchersTests.java

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,14 @@
1818

1919
import java.lang.reflect.Field;
2020
import java.lang.reflect.Modifier;
21+
import java.util.LinkedHashMap;
22+
import java.util.Map;
2123

24+
import javax.servlet.Servlet;
25+
import javax.servlet.ServletRegistration;
2226
import javax.servlet.http.HttpServletResponse;
2327

28+
import org.jetbrains.annotations.NotNull;
2429
import org.junit.jupiter.api.AfterEach;
2530
import org.junit.jupiter.api.BeforeEach;
2631
import org.junit.jupiter.api.Test;
@@ -34,7 +39,6 @@
3439
import org.springframework.mock.web.MockFilterChain;
3540
import org.springframework.mock.web.MockHttpServletRequest;
3641
import org.springframework.mock.web.MockHttpServletResponse;
37-
import org.springframework.mock.web.MockServletContext;
3842
import org.springframework.security.config.annotation.web.AbstractRequestMatcherRegistry;
3943
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
4044
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
@@ -48,12 +52,15 @@
4852
import org.springframework.web.bind.annotation.RequestMapping;
4953
import org.springframework.web.bind.annotation.RestController;
5054
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
55+
import org.springframework.web.servlet.DispatcherServlet;
5156
import org.springframework.web.servlet.config.annotation.EnableWebMvc;
5257
import org.springframework.web.servlet.config.annotation.PathMatchConfigurer;
5358
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
5459
import org.springframework.web.servlet.handler.HandlerMappingIntrospector;
5560

5661
import static org.assertj.core.api.Assertions.assertThat;
62+
import static org.mockito.BDDMockito.given;
63+
import static org.mockito.Mockito.mock;
5764
import static org.springframework.security.config.Customizer.withDefaults;
5865

5966
/**
@@ -233,7 +240,9 @@ public void securityMatchersWhenMultiMvcMatcherThenAllPathsAreDenied() throws Ex
233240
public void loadConfig(Class<?>... configs) {
234241
this.context = new AnnotationConfigWebApplicationContext();
235242
this.context.register(configs);
236-
this.context.setServletContext(new MockServletContext());
243+
MockServletContext servletContext = new MockServletContext();
244+
servletContext.addServlet("dispatcherServlet", DispatcherServlet.class);
245+
this.context.setServletContext(servletContext);
237246
this.context.refresh();
238247
this.context.getAutowireCapableBeanFactory().autowireBean(this);
239248
}
@@ -564,4 +573,25 @@ public void configurePathMatch(PathMatchConfigurer configurer) {
564573

565574
}
566575

576+
private static class MockServletContext extends org.springframework.mock.web.MockServletContext {
577+
578+
private final Map<String, ServletRegistration> registrations = new LinkedHashMap<>();
579+
580+
@NotNull
581+
@Override
582+
public ServletRegistration.Dynamic addServlet(@NotNull String servletName, Class<? extends Servlet> clazz) {
583+
ServletRegistration.Dynamic dynamic = mock(ServletRegistration.Dynamic.class);
584+
given(dynamic.getClassName()).willReturn(clazz.getName());
585+
this.registrations.put(servletName, dynamic);
586+
return dynamic;
587+
}
588+
589+
@NotNull
590+
@Override
591+
public Map<String, ? extends ServletRegistration> getServletRegistrations() {
592+
return this.registrations;
593+
}
594+
595+
}
596+
567597
}

0 commit comments

Comments
 (0)