Refine location checks for PathResource

[stewue]

Hi,

I have currently have a Spring Boot application using Spring Framework 6.1. I tried to upgrade to the newest Spring Boot and Framework version. During upgrading I realized that in my implemented WebMvcConfigurer now an IllegalArgumentException is thrown.

Here a minimal example on how you can reproduce it:

@Configuration
public class MyResourcesConfiguration implements WebMvcConfigurer {

    @Override
    public void addResourceHandlers(ResourceHandlerRegistry registry) {
            registry.addResourceHandler("/my/**").addResourceLocations(new PathResource("/demo-directory/"));
    }
}

In the example a Caused by: java.lang.IllegalArgumentException: Resource location does not end with slash: /demo-directory is thrown. The issue is that in 59ec871 an additional check was added in ResourceHandlerUtils. All constructors of PathResource call Path.normalize which removes the passed trailing slash.

I know in this minimal example just ResourceHandlerRegistration.addResourceLocations(String...) can be used. However in reality I'm dynamically adding different resources in a multi module project.

Is this an undesirable side effect of #33815 or how should ResourceHandlerRegistration.addResourceLocations(Resource...) be used with PathResource?

Thank you in advance!

[tompson]

This is also blocking us from upgrading to Spring Framework 6.2

Our config looks like

@Configuration
public class WebMvcConfigurationDev implements WebMvcConfigurer {

	@Override
	public void addResourceHandlers(ResourceHandlerRegistry registry) {
		registry
				.addResourceHandler("/messages/*")
				.addResourceLocations(new ClassPathResource("/"))
				.resourceChain(false)
				.addResolver(new MessagesResolver())
				.addTransformer(new MessagesTransformer());
	}
}

which we use to resolve message bundles from the classpath.

[bclozel]

Thanks for reaching out. I guess we can refine checks for specific resource implementations.

For your case @stewue, the Path API itself will strip the trailing separator so there is no way to enforce it at the resource level. For example, Path path = Paths.get("/static/"); will yield "/static". On the other hand, the createRelative behavior will always result in a nested path element.

PathResource pathResource = new PathResource("/static");
Resource other = pathResource.createRelative("other.txt");
// will yield "/static/other.txt"

So I think that we should generally leave PathResource alone and not attempt to check the resulting path for the location. This should fix your use case.

Now for @tompson 's case, the new ClassPathResource("/") will strip the leading separator as documented:
A leading slash will be removed, as the {@code ClassLoader} resource access methods will not accept it.. So we could ignore the path check if the path is completely empty. This one is a bit tougher to consider since exposing the entire classpath is an application vulnerability in the first place. I get that this should not be a problem in your case because of the custom MessagesResolver/MessagesTransformer, but we should consider this one carefully.

I'll discuss this with the web team and report back here.

[bclozel]

We have discussed this today, here are our conclusions.

We are repurposing this issue to refine the location checks to account for the PathResource behavior. As a result, applications won't get IllegalArgumentException thrown anymore with PathResource.

As for the new ClassPathResource("/") case raised by @tompson , we are aware that there is a specific behavior for root classpath locations (same goes for the JDK actually). But we don't think that we should relax the location check for this case because using a root classpath location is very dangerous in the first place. The application is in danger of exposing the entire classpath to the outside world by doing so. Here, we're advising to relocate the message properties files into a common /messages/ folder and change that to new ClassPathResource("/messages/") instead. We do understand that this is a breaking behavior change but we still think that the security aspect is worth that change.

[izeye]

The "status: backported" label doesn't seem to be valid as #34213 has been closed as invalid.

[tompson]

@bclozel ok, thanks for looking into this, we decided to move the files into a /messages subfolder which is better for security anyway