From b9be0e3e0f26a9fade23552bf58b0f019941daaf Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Mon, 10 Apr 2017 11:47:15 +0100 Subject: [PATCH] Skip actuator path extension content negotiation Allow `PathExtensionContentNegotiationStrategy` to be bypassed by actuator endpoints. Prior to this commit calling `/loggers/com.aaa.cab` would return a HTTP 406 response due to `.cab` being a known extension. Fixes gh-8765 --- .../mvc/AbstractEndpointHandlerMapping.java | 29 ++++++++- .../EndpointWebMvcAutoConfigurationTests.java | 8 +-- .../AbstractEndpointHandlerMappingTests.java | 7 +- .../endpoint/mvc/LoggersMvcEndpointTests.java | 10 +++ .../endpoint/mvc/MetricsMvcEndpointTests.java | 11 +++- .../web/WebMvcAutoConfiguration.java | 64 +++++++++++++++++-- 6 files changed, 116 insertions(+), 13 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMapping.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMapping.java index f00922087c..c0a218b57e 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMapping.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2016 the original author or authors. + * Copyright 2012-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,17 +26,21 @@ import java.util.List; import java.util.Set; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.springframework.boot.actuate.endpoint.Endpoint; +import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration; import org.springframework.context.ApplicationContext; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; +import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.cors.CorsUtils; import org.springframework.web.servlet.HandlerExecutionChain; import org.springframework.web.servlet.HandlerInterceptor; import org.springframework.web.servlet.HandlerMapping; +import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; import org.springframework.web.servlet.mvc.condition.PatternsRequestCondition; import org.springframework.web.servlet.mvc.method.RequestMappingInfo; import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; @@ -204,6 +208,11 @@ public abstract class AbstractEndpointHandlerMapping return addSecurityInterceptor(chain); } + @Override + protected void extendInterceptors(List interceptors) { + interceptors.add(new SkipPathExtensionContentNegotiation()); + } + private HandlerExecutionChain addSecurityInterceptor(HandlerExecutionChain chain) { List interceptors = new ArrayList(); if (chain.getInterceptors() != null) { @@ -279,4 +288,22 @@ public abstract class AbstractEndpointHandlerMapping return this.corsConfiguration; } + /** + * {@link HandlerInterceptorAdapter} to ensure that + * {@link PathExtensionContentNegotiationStrategy} is skipped for actuator endpoints. + */ + private static final class SkipPathExtensionContentNegotiation + extends HandlerInterceptorAdapter { + + @Override + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, + Object handler) throws Exception { + request.setAttribute( + WebMvcAutoConfiguration.SKIP_PATH_EXTENSION_CONTENT_NEGOTIATION_ATTRIBUTE, + Boolean.TRUE); + return true; + } + + } + } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfigurationTests.java index 44dac20d9f..39e09ca7b1 100755 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfigurationTests.java @@ -193,7 +193,7 @@ public class EndpointWebMvcAutoConfigurationTests { .getBean(ManagementContextResolver.class).getApplicationContext(); List interceptors = (List) ReflectionTestUtils.getField( managementContext.getBean(EndpointHandlerMapping.class), "interceptors"); - assertThat(interceptors).hasSize(1); + assertThat(interceptors).hasSize(2); } @Test @@ -211,7 +211,7 @@ public class EndpointWebMvcAutoConfigurationTests { .getBean(ManagementContextResolver.class).getApplicationContext(); List interceptors = (List) ReflectionTestUtils.getField( managementContext.getBean(EndpointHandlerMapping.class), "interceptors"); - assertThat(interceptors).hasSize(1); + assertThat(interceptors).hasSize(2); EmbeddedServletContainerFactory parentContainerFactory = this.applicationContext .getBean(EmbeddedServletContainerFactory.class); EmbeddedServletContainerFactory managementContainerFactory = managementContext @@ -536,7 +536,7 @@ public class EndpointWebMvcAutoConfigurationTests { .getBean(ManagementContextResolver.class).getApplicationContext(); List interceptors = (List) ReflectionTestUtils.getField( managementContext.getBean(EndpointHandlerMapping.class), "interceptors"); - assertThat(interceptors).hasSize(1); + assertThat(interceptors).hasSize(2); ManagementServerProperties managementServerProperties = this.applicationContext .getBean(ManagementServerProperties.class); assertThat(managementServerProperties.getSsl()).isNotNull(); @@ -577,7 +577,7 @@ public class EndpointWebMvcAutoConfigurationTests { .getBean(ManagementContextResolver.class).getApplicationContext(); List interceptors = (List) ReflectionTestUtils.getField( managementContext.getBean(EndpointHandlerMapping.class), "interceptors"); - assertThat(interceptors).hasSize(1); + assertThat(interceptors).hasSize(2); ManagementServerProperties managementServerProperties = this.applicationContext .getBean(ManagementServerProperties.class); assertThat(managementServerProperties.getSsl()).isNotNull(); diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMappingTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMappingTests.java index 768ad901ad..fb3e7e81ad 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMappingTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/AbstractEndpointHandlerMappingTests.java @@ -60,7 +60,8 @@ public abstract class AbstractEndpointHandlerMappingTests { Collections.singletonList(endpoint)); mapping.setApplicationContext(this.context); mapping.afterPropertiesSet(); - assertThat(mapping.getHandler(request("POST", "/a")).getInterceptors()).isNull(); + assertThat(mapping.getHandler(request("POST", "/a")).getInterceptors()) + .hasSize(1); } @Test @@ -75,8 +76,8 @@ public abstract class AbstractEndpointHandlerMappingTests { mapping.afterPropertiesSet(); MockHttpServletRequest request = request("POST", "/a"); request.addHeader("Origin", "http://example.com"); - assertThat(mapping.getHandler(request).getInterceptors().length).isEqualTo(2); - assertThat(mapping.getHandler(request).getInterceptors()[1]) + assertThat(mapping.getHandler(request).getInterceptors().length).isEqualTo(3); + assertThat(mapping.getHandler(request).getInterceptors()[2]) .isEqualTo(securityInterceptor); } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/LoggersMvcEndpointTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/LoggersMvcEndpointTests.java index ac04663ec6..499062e206 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/LoggersMvcEndpointTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/LoggersMvcEndpointTests.java @@ -178,6 +178,16 @@ public class LoggersMvcEndpointTests { verifyZeroInteractions(this.loggingSystem); } + @Test + public void logLevelForLoggerWithNameThatCouldBeMistakenForAPathExtension() + throws Exception { + given(this.loggingSystem.getLoggerConfiguration("com.png")) + .willReturn(new LoggerConfiguration("com.png", null, LogLevel.DEBUG)); + this.mvc.perform(get("/loggers/com.png")).andExpect(status().isOk()) + .andExpect(content().string(equalTo( + "{\"configuredLevel\":null," + "\"effectiveLevel\":\"DEBUG\"}"))); + } + @Configuration @Import({ JacksonAutoConfiguration.class, HttpMessageConvertersAutoConfiguration.class, diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpointTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpointTests.java index fdc394ec4e..e6a22de193 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpointTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpointTests.java @@ -125,6 +125,13 @@ public class MetricsMvcEndpointTests { .andExpect(content().string(equalTo("{\"foo\":1}"))); } + @Test + public void specificMetricWithNameThatCouldBeMistakenForAPathExtension() + throws Exception { + this.mvc.perform(get("/metrics/bar.png")).andExpect(status().isOk()) + .andExpect(content().string(equalTo("{\"bar.png\":1}"))); + } + @Test public void specificMetricWhenDisabled() throws Exception { this.context.getBean(MetricsEndpoint.class).setEnabled(false); @@ -138,7 +145,8 @@ public class MetricsMvcEndpointTests { @Test public void regexAll() throws Exception { - String expected = "\"foo\":1,\"group1.a\":1,\"group1.b\":1,\"group2.a\":1,\"group2_a\":1"; + String expected = "\"foo\":1,\"bar.png\":1,\"group1.a\":1,\"group1.b\":1" + + ",\"group2.a\":1,\"group2_a\":1"; this.mvc.perform(get("/metrics/.*")).andExpect(status().isOk()) .andExpect(content().string(containsString(expected))); } @@ -178,6 +186,7 @@ public class MetricsMvcEndpointTests { public Collection> metrics() { ArrayList> metrics = new ArrayList>(); metrics.add(new Metric("foo", 1)); + metrics.add(new Metric("bar.png", 1)); metrics.add(new Metric("group1.a", 1)); metrics.add(new Metric("group1.b", 1)); metrics.add(new Metric("group2.a", 1)); diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/WebMvcAutoConfiguration.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/WebMvcAutoConfiguration.java index 5213e24628..b96fc8b152 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/WebMvcAutoConfiguration.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/WebMvcAutoConfiguration.java @@ -20,6 +20,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.List; +import java.util.ListIterator; import java.util.Map; import java.util.Map.Entry; @@ -67,8 +68,13 @@ import org.springframework.util.StringUtils; import org.springframework.validation.DefaultMessageCodesResolver; import org.springframework.validation.MessageCodesResolver; import org.springframework.validation.Validator; +import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.accept.ContentNegotiationManager; +import org.springframework.web.accept.ContentNegotiationStrategy; +import org.springframework.web.accept.PathExtensionContentNegotiationStrategy; import org.springframework.web.bind.support.ConfigurableWebBindingInitializer; +import org.springframework.web.context.request.NativeWebRequest; +import org.springframework.web.context.request.RequestAttributes; import org.springframework.web.context.request.RequestContextListener; import org.springframework.web.filter.HiddenHttpMethodFilter; import org.springframework.web.filter.HttpPutFormContentFilter; @@ -125,9 +131,16 @@ import org.springframework.web.servlet.view.InternalResourceViewResolver; ValidationAutoConfiguration.class }) public class WebMvcAutoConfiguration { - public static String DEFAULT_PREFIX = ""; + public static final String DEFAULT_PREFIX = ""; - public static String DEFAULT_SUFFIX = ""; + public static final String DEFAULT_SUFFIX = ""; + + /** + * Attribute that can be added to the web request when the + * {@link PathExtensionContentNegotiationStrategy} should be be skipped. + */ + public static final String SKIP_PATH_EXTENSION_CONTENT_NEGOTIATION_ATTRIBUTE = PathExtensionContentNegotiationStrategy.class + .getName() + ".SKIP"; @Bean @ConditionalOnMissingBean(HiddenHttpMethodFilter.class) @@ -404,8 +417,7 @@ public class WebMvcAutoConfiguration { getClass().getClassLoader())) { return super.mvcValidator(); } - return WebMvcValidator.get(getApplicationContext(), - getValidator()); + return WebMvcValidator.get(getApplicationContext(), getValidator()); } @Override @@ -453,6 +465,22 @@ public class WebMvcAutoConfiguration { } } + @Bean + @Override + public ContentNegotiationManager mvcContentNegotiationManager() { + ContentNegotiationManager manager = super.mvcContentNegotiationManager(); + List strategies = manager.getStrategies(); + ListIterator iterator = strategies.listIterator(); + while (iterator.hasNext()) { + ContentNegotiationStrategy strategy = iterator.next(); + if (strategy instanceof PathExtensionContentNegotiationStrategy) { + iterator.set(new OptionalPathExtensionContentNegotiationStrategy( + strategy)); + } + } + return manager; + } + } @Configuration @@ -550,4 +578,32 @@ public class WebMvcAutoConfiguration { } + /** + * Decorator to make {@link PathExtensionContentNegotiationStrategy} optional + * depending on a request attribute. + */ + static class OptionalPathExtensionContentNegotiationStrategy + implements ContentNegotiationStrategy { + + private final ContentNegotiationStrategy delegate; + + OptionalPathExtensionContentNegotiationStrategy( + ContentNegotiationStrategy delegate) { + this.delegate = delegate; + } + + @Override + public List resolveMediaTypes(NativeWebRequest webRequest) + throws HttpMediaTypeNotAcceptableException { + Object skip = webRequest.getAttribute( + SKIP_PATH_EXTENSION_CONTENT_NEGOTIATION_ATTRIBUTE, + RequestAttributes.SCOPE_REQUEST); + if (skip != null && Boolean.parseBoolean(skip.toString())) { + return Collections.emptyList(); + } + return this.delegate.resolveMediaTypes(webRequest); + } + + } + }