Adapt to trailing slashes no longer being matched by default

See gh-31563
This commit is contained in:
Andy Wilkinson 2022-07-07 14:14:05 +01:00
parent 97d96eebdf
commit e9136e023b
12 changed files with 79 additions and 18 deletions

View File

@ -217,7 +217,7 @@ public final class EndpointRequest {
streamPaths(this.excludes, pathMappedEndpoints).forEach(paths::remove); streamPaths(this.excludes, pathMappedEndpoints).forEach(paths::remove);
List<ServerWebExchangeMatcher> delegateMatchers = getDelegateMatchers(paths); List<ServerWebExchangeMatcher> delegateMatchers = getDelegateMatchers(paths);
if (this.includeLinks && StringUtils.hasText(pathMappedEndpoints.getBasePath())) { if (this.includeLinks && StringUtils.hasText(pathMappedEndpoints.getBasePath())) {
delegateMatchers.add(new PathPatternParserServerWebExchangeMatcher(pathMappedEndpoints.getBasePath())); delegateMatchers.add(new LinksServerWebExchangeMatcher());
} }
return new OrServerWebExchangeMatcher(delegateMatchers); return new OrServerWebExchangeMatcher(delegateMatchers);
} }
@ -275,7 +275,9 @@ public final class EndpointRequest {
private ServerWebExchangeMatcher createDelegate(WebEndpointProperties properties) { private ServerWebExchangeMatcher createDelegate(WebEndpointProperties properties) {
if (StringUtils.hasText(properties.getBasePath())) { if (StringUtils.hasText(properties.getBasePath())) {
return new PathPatternParserServerWebExchangeMatcher(properties.getBasePath()); return new OrServerWebExchangeMatcher(
new PathPatternParserServerWebExchangeMatcher(properties.getBasePath()),
new PathPatternParserServerWebExchangeMatcher(properties.getBasePath() + "/"));
} }
return EMPTY_MATCHER; return EMPTY_MATCHER;
} }

View File

@ -1,5 +1,5 @@
/* /*
* Copyright 2012-2019 the original author or authors. * Copyright 2012-2022 the original author or authors.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -88,24 +88,28 @@ class EndpointRequestTests {
void toEndpointClassShouldMatchEndpointPath() { void toEndpointClassShouldMatchEndpointPath() {
ServerWebExchangeMatcher matcher = EndpointRequest.to(FooEndpoint.class); ServerWebExchangeMatcher matcher = EndpointRequest.to(FooEndpoint.class);
assertMatcher(matcher).matches("/actuator/foo"); assertMatcher(matcher).matches("/actuator/foo");
assertMatcher(matcher).matches("/actuator/foo/");
} }
@Test @Test
void toEndpointClassShouldNotMatchOtherPath() { void toEndpointClassShouldNotMatchOtherPath() {
ServerWebExchangeMatcher matcher = EndpointRequest.to(FooEndpoint.class); ServerWebExchangeMatcher matcher = EndpointRequest.to(FooEndpoint.class);
assertMatcher(matcher).doesNotMatch("/actuator/bar"); assertMatcher(matcher).doesNotMatch("/actuator/bar");
assertMatcher(matcher).doesNotMatch("/actuator/bar/");
} }
@Test @Test
void toEndpointIdShouldMatchEndpointPath() { void toEndpointIdShouldMatchEndpointPath() {
ServerWebExchangeMatcher matcher = EndpointRequest.to("foo"); ServerWebExchangeMatcher matcher = EndpointRequest.to("foo");
assertMatcher(matcher).matches("/actuator/foo"); assertMatcher(matcher).matches("/actuator/foo");
assertMatcher(matcher).matches("/actuator/foo/");
} }
@Test @Test
void toEndpointIdShouldNotMatchOtherPath() { void toEndpointIdShouldNotMatchOtherPath() {
ServerWebExchangeMatcher matcher = EndpointRequest.to("foo"); ServerWebExchangeMatcher matcher = EndpointRequest.to("foo");
assertMatcher(matcher).doesNotMatch("/actuator/bar"); assertMatcher(matcher).doesNotMatch("/actuator/bar");
assertMatcher(matcher).doesNotMatch("/actuator/bar/");
} }
@Test @Test
@ -136,9 +140,13 @@ class EndpointRequestTests {
endpoints.add(mockEndpoint(EndpointId.of("baz"), "baz")); endpoints.add(mockEndpoint(EndpointId.of("baz"), "baz"));
PathMappedEndpoints pathMappedEndpoints = new PathMappedEndpoints("/actuator", () -> endpoints); PathMappedEndpoints pathMappedEndpoints = new PathMappedEndpoints("/actuator", () -> endpoints);
assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/foo"); assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/foo");
assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/foo/");
assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/baz"); assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/baz");
assertMatcher(matcher, pathMappedEndpoints).doesNotMatch("/actuator/baz/");
assertMatcher(matcher).matches("/actuator/bar"); assertMatcher(matcher).matches("/actuator/bar");
assertMatcher(matcher).matches("/actuator/bar/");
assertMatcher(matcher).matches("/actuator"); assertMatcher(matcher).matches("/actuator");
assertMatcher(matcher).matches("/actuator/");
} }
@Test @Test
@ -146,30 +154,40 @@ class EndpointRequestTests {
ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks() ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks()
.excluding(FooEndpoint.class); .excluding(FooEndpoint.class);
assertMatcher(matcher).doesNotMatch("/actuator/foo"); assertMatcher(matcher).doesNotMatch("/actuator/foo");
assertMatcher(matcher).doesNotMatch("/actuator/foo/");
assertMatcher(matcher).doesNotMatch("/actuator"); assertMatcher(matcher).doesNotMatch("/actuator");
assertMatcher(matcher).doesNotMatch("/actuator/");
} }
@Test @Test
void excludeByIdShouldNotMatchExcluded() { void excludeByIdShouldNotMatchExcluded() {
ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excluding("foo"); ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excluding("foo");
assertMatcher(matcher).doesNotMatch("/actuator/foo"); assertMatcher(matcher).doesNotMatch("/actuator/foo");
assertMatcher(matcher).doesNotMatch("/actuator/foo/");
assertMatcher(matcher).matches("/actuator/bar"); assertMatcher(matcher).matches("/actuator/bar");
assertMatcher(matcher).matches("/actuator/bar/");
assertMatcher(matcher).matches("/actuator"); assertMatcher(matcher).matches("/actuator");
assertMatcher(matcher).matches("/actuator/");
} }
@Test @Test
void excludeByIdShouldNotMatchLinksIfExcluded() { void excludeByIdShouldNotMatchLinksIfExcluded() {
ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks().excluding("foo"); ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks().excluding("foo");
assertMatcher(matcher).doesNotMatch("/actuator/foo"); assertMatcher(matcher).doesNotMatch("/actuator/foo");
assertMatcher(matcher).doesNotMatch("/actuator/foo/");
assertMatcher(matcher).doesNotMatch("/actuator"); assertMatcher(matcher).doesNotMatch("/actuator");
assertMatcher(matcher).doesNotMatch("/actuator/");
} }
@Test @Test
void excludeLinksShouldNotMatchBasePath() { void excludeLinksShouldNotMatchBasePath() {
ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks(); ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint().excludingLinks();
assertMatcher(matcher).doesNotMatch("/actuator"); assertMatcher(matcher).doesNotMatch("/actuator");
assertMatcher(matcher).doesNotMatch("/actuator/");
assertMatcher(matcher).matches("/actuator/foo"); assertMatcher(matcher).matches("/actuator/foo");
assertMatcher(matcher).matches("/actuator/foo/");
assertMatcher(matcher).matches("/actuator/bar"); assertMatcher(matcher).matches("/actuator/bar");
assertMatcher(matcher).matches("/actuator/bar/");
} }
@Test @Test
@ -178,14 +196,18 @@ class EndpointRequestTests {
RequestMatcherAssert assertMatcher = assertMatcher(matcher, ""); RequestMatcherAssert assertMatcher = assertMatcher(matcher, "");
assertMatcher.doesNotMatch("/"); assertMatcher.doesNotMatch("/");
assertMatcher.matches("/foo"); assertMatcher.matches("/foo");
assertMatcher.matches("/foo/");
assertMatcher.matches("/bar"); assertMatcher.matches("/bar");
assertMatcher.matches("/bar/");
} }
@Test @Test
void noEndpointPathsBeansShouldNeverMatch() { void noEndpointPathsBeansShouldNeverMatch() {
ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint(); ServerWebExchangeMatcher matcher = EndpointRequest.toAnyEndpoint();
assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/foo"); assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/foo");
assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/foo/");
assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/bar"); assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/bar");
assertMatcher(matcher, (PathMappedEndpoints) null).doesNotMatch("/actuator/bar/");
} }
private RequestMatcherAssert assertMatcher(ServerWebExchangeMatcher matcher) { private RequestMatcherAssert assertMatcher(ServerWebExchangeMatcher matcher) {

View File

@ -76,7 +76,7 @@ abstract class AbstractEndpointRequestIntegrationTests {
getContextRunner().run((context) -> { getContextRunner().run((context) -> {
WebTestClient webTestClient = getWebTestClient(context); WebTestClient webTestClient = getWebTestClient(context);
webTestClient.get().uri("/actuator").exchange().expectStatus().isOk(); webTestClient.get().uri("/actuator").exchange().expectStatus().isOk();
webTestClient.get().uri("/actuator/").exchange().expectStatus().isOk(); webTestClient.get().uri("/actuator/").exchange().expectStatus().isNotFound();
}); });
} }

View File

@ -1,5 +1,5 @@
/* /*
* Copyright 2012-2020 the original author or authors. * Copyright 2012-2022 the original author or authors.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -42,7 +42,7 @@ class MvcEndpointRequestIntegrationTests extends AbstractEndpointRequestIntegrat
void toLinksWhenServletPathSetShouldMatch() { void toLinksWhenServletPathSetShouldMatch() {
getContextRunner().withPropertyValues("spring.mvc.servlet.path=/admin").run((context) -> { getContextRunner().withPropertyValues("spring.mvc.servlet.path=/admin").run((context) -> {
WebTestClient webTestClient = getWebTestClient(context); WebTestClient webTestClient = getWebTestClient(context);
webTestClient.get().uri("/admin/actuator/").exchange().expectStatus().isOk(); webTestClient.get().uri("/admin/actuator/").exchange().expectStatus().isNotFound();
webTestClient.get().uri("/admin/actuator").exchange().expectStatus().isOk(); webTestClient.get().uri("/admin/actuator").exchange().expectStatus().isOk();
}); });
} }

View File

@ -180,8 +180,9 @@ public abstract class AbstractWebFluxEndpointHandlerMapping extends RequestMappi
private void registerLinksMapping() { private void registerLinksMapping() {
String path = this.endpointMapping.getPath(); String path = this.endpointMapping.getPath();
String linksPath = StringUtils.hasLength(path) ? path : "/";
String[] produces = StringUtils.toStringArray(this.endpointMediaTypes.getProduced()); String[] produces = StringUtils.toStringArray(this.endpointMediaTypes.getProduced());
RequestMappingInfo mapping = RequestMappingInfo.paths(path).methods(RequestMethod.GET).produces(produces) RequestMappingInfo mapping = RequestMappingInfo.paths(linksPath).methods(RequestMethod.GET).produces(produces)
.build(); .build();
LinksHandler linksHandler = getLinksHandler(); LinksHandler linksHandler = getLinksHandler();
registerMapping(mapping, linksHandler, registerMapping(mapping, linksHandler,

View File

@ -240,9 +240,11 @@ public abstract class AbstractWebMvcEndpointHandlerMapping extends RequestMappin
} }
private void registerLinksMapping() { private void registerLinksMapping() {
RequestMappingInfo mapping = RequestMappingInfo.paths(this.endpointMapping.createSubPath("")) String path = this.endpointMapping.getPath();
.methods(RequestMethod.GET).produces(this.endpointMediaTypes.getProduced().toArray(new String[0])) String linksPath = (StringUtils.hasLength(path)) ? this.endpointMapping.createSubPath("/") : "/";
.options(this.builderConfig).build(); RequestMappingInfo mapping = RequestMappingInfo.paths(linksPath).methods(RequestMethod.GET)
.produces(this.endpointMediaTypes.getProduced().toArray(new String[0])).options(this.builderConfig)
.build();
LinksHandler linksHandler = getLinksHandler(); LinksHandler linksHandler = getLinksHandler();
registerMapping(mapping, linksHandler, ReflectionUtils.findMethod(linksHandler.getClass(), "links", registerMapping(mapping, linksHandler, ReflectionUtils.findMethod(linksHandler.getClass(), "links",
HttpServletRequest.class, HttpServletResponse.class)); HttpServletRequest.class, HttpServletResponse.class));

View File

@ -123,9 +123,9 @@ public abstract class AbstractWebEndpointIntegrationTests<T extends Configurable
} }
@Test @Test
void operationWithTrailingSlashShouldMatch() { void operationWithTrailingSlashShouldNotMatch() {
load(TestEndpointConfiguration.class, (client) -> client.get().uri("/test/").exchange().expectStatus().isOk() load(TestEndpointConfiguration.class,
.expectBody().jsonPath("All").isEqualTo(true)); (client) -> client.get().uri("/test/").exchange().expectStatus().isNotFound());
} }
@Test @Test

View File

@ -30,6 +30,8 @@ import org.junit.jupiter.api.Test;
import org.springframework.boot.actuate.metrics.web.servlet.DefaultWebMvcTagsProvider; import org.springframework.boot.actuate.metrics.web.servlet.DefaultWebMvcTagsProvider;
import org.springframework.boot.actuate.metrics.web.servlet.WebMvcTagsContributor; import org.springframework.boot.actuate.metrics.web.servlet.WebMvcTagsContributor;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.web.servlet.HandlerMapping;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
@ -69,6 +71,22 @@ class DefaultWebMvcTagsProviderTests {
assertThat(tags).containsOnlyKeys("method", "uri", "alpha", "bravo", "charlie"); assertThat(tags).containsOnlyKeys("method", "uri", "alpha", "bravo", "charlie");
} }
@Test
void trailingSlashIsIncludedByDefault() {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/the/uri/");
request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "{one}/{two}/");
Map<String, Tag> tags = asMap(new DefaultWebMvcTagsProvider().getTags(request, null, null, null));
assertThat(tags.get("uri").getValue()).isEqualTo("{one}/{two}/");
}
@Test
void trailingSlashCanBeIgnored() {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/the/uri/");
request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "{one}/{two}/");
Map<String, Tag> tags = asMap(new DefaultWebMvcTagsProvider(true).getTags(request, null, null, null));
assertThat(tags.get("uri").getValue()).isEqualTo("{one}/{two}");
}
private Map<String, Tag> asMap(Iterable<Tag> tags) { private Map<String, Tag> asMap(Iterable<Tag> tags) {
return StreamSupport.stream(tags.spliterator(), false) return StreamSupport.stream(tags.spliterator(), false)
.collect(Collectors.toMap(Tag::getKey, Function.identity())); .collect(Collectors.toMap(Tag::getKey, Function.identity()));

View File

@ -77,7 +77,10 @@ import org.springframework.web.context.WebApplicationContext;
import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.ModelAndView;
import org.springframework.web.servlet.config.annotation.EnableWebMvc; import org.springframework.web.servlet.config.annotation.EnableWebMvc;
import org.springframework.web.servlet.config.annotation.PathMatchConfigurer;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
import org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitter; import org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitter;
import org.springframework.web.util.pattern.PathPatternParser;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatCode;
@ -312,6 +315,7 @@ class WebMvcMetricsFilterTests {
@Test @Test
void trailingSlashShouldNotRecordDuplicateMetrics() throws Exception { void trailingSlashShouldNotRecordDuplicateMetrics() throws Exception {
this.mvc.perform(get("/api/c1/simple/10")).andExpect(status().isOk()); this.mvc.perform(get("/api/c1/simple/10")).andExpect(status().isOk());
this.mvc.perform(get("/api/c1/simple/10/")).andExpect(status().isOk()); this.mvc.perform(get("/api/c1/simple/10/")).andExpect(status().isOk());
assertThat(this.registry.get("http.server.requests").tags("status", "200", "uri", "/api/c1/simple/{id}").timer() assertThat(this.registry.get("http.server.requests").tags("status", "200", "uri", "/api/c1/simple/{id}").timer()
@ -328,7 +332,7 @@ class WebMvcMetricsFilterTests {
@Configuration(proxyBeanMethods = false) @Configuration(proxyBeanMethods = false)
@EnableWebMvc @EnableWebMvc
@Import({ Controller1.class, Controller2.class }) @Import({ Controller1.class, Controller2.class })
static class MetricsFilterApp { static class MetricsFilterApp implements WebMvcConfigurer {
@Bean @Bean
Clock micrometerClock() { Clock micrometerClock() {
@ -393,6 +397,14 @@ class WebMvcMetricsFilterTests {
return new FaultyWebMvcTagsProvider(); return new FaultyWebMvcTagsProvider();
} }
@Override
@SuppressWarnings("deprecation")
public void configurePathMatch(PathMatchConfigurer configurer) {
PathPatternParser pathPatternParser = new PathPatternParser();
pathPatternParser.setMatchOptionalTrailingSeparator(true);
configurer.setPatternParser(pathPatternParser);
}
} }
@RestController @RestController

View File

@ -1,5 +1,5 @@
/* /*
* Copyright 2012-2021 the original author or authors. * Copyright 2012-2022 the original author or authors.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -118,6 +118,9 @@ abstract class AbstractSampleActuatorCustomSecurityTests {
ResponseEntity<Object> entity = adminRestTemplate().getForEntity(getManagementPath() + "/actuator/env", ResponseEntity<Object> entity = adminRestTemplate().getForEntity(getManagementPath() + "/actuator/env",
Object.class); Object.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
entity = adminRestTemplate().getForEntity(getManagementPath() + "/actuator/env/", Object.class);
// EndpointRequest matches the trailing slash but MVC doesn't
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
entity = adminRestTemplate().getForEntity( entity = adminRestTemplate().getForEntity(
getManagementPath() + "/actuator/env/management.endpoints.web.exposure.include", Object.class); getManagementPath() + "/actuator/env/management.endpoints.web.exposure.include", Object.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);

View File

@ -76,7 +76,7 @@ class SampleActuatorCustomSecurityApplicationTests extends AbstractSampleActuato
Object.class); Object.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
entity = beansRestTemplate().getForEntity(getManagementPath() + "/actuator/beans/", Object.class); entity = beansRestTemplate().getForEntity(getManagementPath() + "/actuator/beans/", Object.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.FORBIDDEN);
} }
} }

View File

@ -33,7 +33,8 @@ import static org.assertj.core.api.Assertions.assertThat;
* @author HaiTao Zhang * @author HaiTao Zhang
*/ */
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT,
properties = { "management.endpoints.web.base-path=/", "management.server.port=0" }) properties = { "management.endpoints.web.base-path=/", "management.server.port=0",
"logging.level.org.springframework.web=trace" })
class ManagementDifferentPortSampleActuatorApplicationTests { class ManagementDifferentPortSampleActuatorApplicationTests {
@LocalManagementPort @LocalManagementPort
@ -42,7 +43,7 @@ class ManagementDifferentPortSampleActuatorApplicationTests {
@Test @Test
void linksEndpointShouldBeAvailable() { void linksEndpointShouldBeAvailable() {
ResponseEntity<String> entity = new TestRestTemplate("user", "password") ResponseEntity<String> entity = new TestRestTemplate("user", "password")
.getForEntity("http://localhost:" + this.managementPort + "/", String.class); .getForEntity("http://localhost:" + this.managementPort, String.class);
assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
assertThat(entity.getBody()).contains("\"_links\""); assertThat(entity.getBody()).contains("\"_links\"");
} }