Deprecate trailing slash match and change default value from true to false
vpavic opened this issue · 42 comments
Whether to match to URLs irrespective of the presence of a trailing slash. If enabled a method mapped to
"/users"also matches to"/users/".
The default value istrue.
Even though this behavior has been long present in Spring, it introduces ambiguity that can (combined with some other choices) easily have consequences in shape of security vulnerabilities. Consider this example:
@SpringBootApplication
@RestController
public class SampleApplication {
public static void main(String[] args) {
SpringApplication.run(SampleApplication.class, args);
}
@GetMapping("/resources")
String resources() {
return "Hello from /resources";
}
@GetMapping("/resources/{id}")
String resourceById(@PathVariable Long id) {
return "Hello from /resources/" + id;
}
@Bean
SecurityFilterChain filterChain(HttpSecurity httpSecurity) throws Exception {
return httpSecurity
.authorizeHttpRequests(requests -> {
requests.antMatchers("/resources").hasRole("admin");
requests.antMatchers("/resources/**").hasRole("user");
requests.anyRequest().denyAll();
})
.httpBasic(Customizer.withDefaults())
.build();
}
}spring.security.user.password=password
spring.security.user.roles=userDefault user (with role user) will get 403 attempting to GET /resources but can avoid protection by issuing GET /resources/, which wouldn't be possible with trailing slash matching disabled.
Let me note that I'm aware that using mvcMatchers instead of antMatchers would have prevented this issue but that doesn't change the fact that there are many configurations out there relying on antMatchers and that sometimes antMatchers are simply more suitable for other reasons.
Also, I personally see little real benefit of having trailing slash matching enabled because:
- if application renders server-side generated views navigation is either way established using hyperlinks
- if application exposes APIs then even more so it is expected that requests are aligned with the API docs
For places where it's really needed for application to respond to both requests, I'd argue that it's either way better solution to configure redirects instead of application wide ambiguous request mappings.
Please consider making this change for 6.0.
I think most users would would expect that a trailing slash doesn't make a difference. From that perspective, the default is not unreasonable, and more than just HATEOAS style navigation or API guidance, even just manually typing a URL somewhere.
If we change the default, many would look to change it back, so overall it's hard to avoid the need to align Spring Security with with Spring MVC through mvcMatchers. One could argue that it's a safer default but this applies only when used with Spring Security. It's not unsafe by itself.
For configuring redirects, do you mean externally, like in a proxy? That would solve the problem at a different level, before Spring Security and Spring MVC, so I would be more interested in that direction but we can only make it a recommendation, and we can still make such a recommendation independent of this.
From that perspective, the default is not unreasonable, and more than just HATEOAS style navigation or API guidance, even just manually typing a URL somewhere.
I can see manually typing a URL as a real argument only in case of web apps (not APIs) and even there it's only potentially useful for a few select URLs that are considered to be entry points into the application and are likely to be directly entered by the users.
One could argue that it's a safer default but this applies only when used with Spring Security. It's not unsafe by itself.
This is the part I strongly disagree with - what Spring Security does is nothing special nor unique. You can end up with the same kind of risks with other Java filter based security frameworks (for example, Apache Shiro) or by applying security (authorization) in an external component that sits in front of your Spring application. After all, on a high level, conceptually these all take the same approach.
For configuring redirects, do you mean externally, like in a proxy?
Either externally in a proxy or using something like Tuckey UrlRewriteFilter or even simply using ViewControllerRegistry::addRedirectViewController to add redirects where needed.
What I would like to see in Spring are the defaults that are not ambiguous and are therefore less prone to abuse. When I see a handler annotated with @GetMapping("/api/resources") that it really maps to only what I see, unless I opt into any additional (ambiguous) behavior explicitly. This change together with #23915 would achieve that.
I'm not necessarily disagreeing. I'm merely making the point that by itself, trailing slash is not unsafe. It's only when combined with a proxy or filter that also interprets URLs, where it becomes a problem, and the problem more generally is that there may be differences, which could go both ways.
That said, this is also the reason, I've come to see over time that URL paths should be left alone as is as much as possible, avoiding normalization as much as possible. It's the only way for frameworks to minimize differences and align naturally. This is also why PathPatternParser was designed to not decode the full path in order to match to mappings, unlike AntPathMatcher, doesn't even support suffix patterns, etc.
In any case, the only way to really make a significant difference here I think is to deprecate the trailingSlash option entirely and encourage an alternative solution, like a proxy or filter to redirect. That enforces being more precise about where you want to do that exactly, and it can be done ahead of security decisions. Otherwise the possibility for a mismatch between web and security frameworks remains. It's just too easy to flip the default back and not think about it again.
This is something that we'll discuss as a possibility for 6.0.
In any case, the only way to really make a significant difference here I think is to deprecate the trailingSlash option entirely and encourage an alternative solution, like a proxy or filter to redirect.
I like that even better. Thanks for the feedback.
Team decision: we'll go ahead with this. It aligns with various other path matching changes we've made in 5.2 and 5.3 such suffix pattern matching, path segment trimming, and path decoding, to make path matching more transparent and explicit.
Will the default be changed to false as part of this deprecation?
Yes, I'm thinking that we might as well and that we'll have to, since otherwise you'd need to set it in order to stop using it. We had the same issue with suffix pattern matching.
The trailing slash option is now deprecated and set to false in all applicable places. The change applies mainly to annotated controllers, since SimpleUrlHandlerMapping, it turns out, was already set to false by default. Nevertheless, it's now deprecated throughout and to be removed eventually.
Looks like it, yes.
This little stunt just cost us over 10 man-hours of pure frustration staring at 404 issues. We debugged through the entire security filter chain, the servlet, everything (great fun!) - until we arrived at the path matchers where it became apparent that the difference between the pattern (and its path placeholders) and the provided path was only the trailing slash. This may be apparent in small applications, but we have over 300 endpoint mappings. If the little phrase 404 would have appeared somewhere in the lengthy migration notes, we would have found it.
@MartinHaeusler I'm sorry you went through this. Maybe we can improve the documentation to avoid the same issue for other developers. Can you share at which documentation you were looking at while working on this?
@bclozel we were looking at this:
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Migration-Guide
But given how big Spring Boot is (with all its modules) it's hard to tell which parts of this document are even relevant for any given application. We spent some time on google looking for "Spring Boot 3 404", but the search results produced nothing specific for the latest version. Up to this point, we were not even aware that some clients were sending trailing slashes to our endpoints, it just "happened to work". That's why the section on trailing slashes didn't really catch any attention. Making the migration guide easier to search through (i.e. "if you do not update X, you will receive error Y") would have been great. If there was any mention of "if your application relied on this behavior, you will receive HTTP 404 errors" in the migration guide, we would have found it instantly... No hard feelings. We are now aware of it and can move on.
@MartinHaeusler I've added a quick mention of "HTTP 404 errors" in the dedicated section of the Spring Boot migration guide, hopefully this will help others. Thanks for bringing this up.
Are there any considerations for trimming trailing slashes by default?
@bclozel Migration guide should also provide a workaround for SpingFlux, not only SpringMVC.
Something like:
@Configuration
class WebConfiguration implements WebFluxConfigurer {
@Override
public void configurePathMatching(PathMatchConfigurer configurer) {
configurer.setUseTrailingSlashMatch(true);
}
}@kdebski85 Thanks for the suggestions, I've updated the wiki.
This seems pretty problematic to me on a grander scale of things.
Consider that many Spring Boot applications are in fact APIs served to many different customers over which the API developer has very little, if any, control over.
Since this worked before Spring Boot 3 if any of those customers used legacy behavior suddenly application developer is in a impossible situation where he's essentially forced to support this legacy behavior indefinitely. I'm saying indefinitely because I find it highly unlikely anyone will be able to convince all of their customers to fix it in their code for what seems to be for superficial or puristic reasons.
Now I'm not necessarily against this change, I just expect Spring Framework teams will get a lot of flak for this because of the situation explained above. It puts developers into an impossible situation which can be hard to detect at first and cause a lot of frustration.
At Infobip I know where to fix this for all our public API's without duplicating this change all over our codebase, but I'm not sure everyone will be that lucky.
This brilliant move just cost our teams hundreds of man-hours. So thanks for that.
@dkoding sorry to hear that. Was it narrowing the cause that took most of this time, or implementing a solution, and is there anything more we could do besides this in the migration guide?
@lpandzic thanks for sharing this, I appreciate the perspective. I will disagree on this being superficial or purist reasons. Security issues are very real, not to mention much more damaging and disruptive.
@lpandzic thanks for sharing this, I appreciate the perspective. I will disagree on this being superficial or purist reasons. Security issues are very real, not to mention much more damaging and disruptive.
I said:
for what seems to be for superficial or puristic reasons.
You've missed the what seems part? I intentionally later on also put
Now I'm not necessarily against this change
and I'll try to elaborate a bit more on that part.
I understand the security perspective, but I also understand @dkoding 's perspective. This is a very disruptive change for reasons I explained above. It will cost a lot of companies a lot of money - customer dissatisfaction and engineering hours spent to fix this.
My understanding is that original issue can only happen in certain circumstances. Couldn't there be an implementation that detects such possible circumstances (cross checking registered controller paths against security configuration) and fail startup fast then and force developer to pick either to ban those paths with slashes or figure out some other way how to secure such paths? Such solution would be a lot less disruptive and costly.
And one more thing:
Doesn't the workaround suggested here defeat the security fix? If many APIs implement this workaround because they have customers depending on that slash are we actually fixing the security issue or just delegating it to developers that are using Spring?
For me this is not a proper solution for existing projects out there but more a solution for new projects which is quite problematic.
Couldn't there be an implementation that detects such possible circumstances (cross checking registered controller paths against security configuration) and fail startup fast then and force developer to pick either to ban those paths with slashes or figure out some other way how to secure such paths?
Not really. Trailing slash support is not a security issue, and the decision to turn it off is not a security fix. There is no concrete security issue to protect against on startup.
It is one of a number of areas of flexibility in Spring MVC request mappings that over a long period of time have been proven, through actual vulnerability reports, to lead to security issues when combined with additional layers including proxies, security filters, forwarded headers, determination of lookup paths, and so on.
You are right that you can turn it right back on, but that's only to allow time to evolve within a deprecation period. In order to evolve, you do not have to change clients outside your control. We expect use of proxy rules or a redirect filter such as Tuckey to intercept requests with trailing slashes earlier in the processing chain, and deal with them accordingly, i.e. responding with a permanent redirect if not expected.
This little stunt just cost us over 10 man-hours of pure frustration staring at 404 issues. We debugged through the entire security filter chain, the servlet, everything (great fun!)... @MartinHaeusler
AMEN! Exactly, we spend 3 people almost the whole day with it and we were quite unhappy :(
We have multiple clients, for which it is difficult to upgrade their configuration, and now we have to force them to change :(
This is awful. Upgrading to this Version is literally going to cost hundreds of hours in bigger environments with dozens of existing clients. Plus legacy clients are awful to handle.
Please offer some way of finding out which clients / URLs are even affected by this change. Crosschecking hundreds of URLs in dozens of clients is just not a feasible approach. Maybe you offer some kind of component you can register that gets called whenever an URL was not matched because of this (or just matched because the old behavior is turned back on). That way you could save affected clients / URLs / Controller to a DB / File etc. and don't have to check manually. Plus you can migrate clients gradually and see the progress.
Obviously that only catches those URLs that are being called but letting this run for a few months should help to cover 99% of the cases
After I tried the setting described in the official 3.0 migration guide, it appeared that they didn't work at all.
@Configuration
public class WebConfiguration implements WebMvcConfigurer {
@Override
public void configurePathMatch(PathMatchConfigurer configurer) {
configurer.setUseTrailingSlashMatch(true);
}
}I wrote the codes above along with the setting in applicaiton.yml
server:
forward-headers-strategy: native
tomcat.redirect-context-root: falseAs a result, when the URL was "http://my.domain/my-context" without a trailing slash, it showed a 401 error. Even after I added "/my-context" to the ignore list in Spring Security, it still seemed to always check the path and resulted in an "Access Denied" message in the debug console.
frustrated...
Does anyone work when using configurer.setUseTrailingSlashMatch(true);?
As an API developer, this is absolutely nuts. One of our primary missions to our customers is to provide a consistent API that does not break. The upgrade to Spring Boot 3 effectively breaks our API unless we use one of three provided "solutions":
- Add useTrailingSlash configuration, which is deprecated, so this is a non-solution in some random future Spring Boot version and we're in the same place as now.
- Changes to proxy, as our customers setup their own proxies, this is a non-solution. It may also create undesired side effects.
- Duplicate all mappings to use both paths. The only viable solution but also a huge maintenance burden. As we manage hundreds of endpoints, we have to update those mappings in those 100's of places. Our automated testing doesn't cover all trailing slash vs non-trailing slash combinations, so we can never be certain we got it perfectly done, unless we duplicate all testing, potentially hundreds of man-hours spent. There's also future maintenance burden for new endpoints as new developers may forget to duplicate the paths. Some may include trailing slashes while others do not, introducing inconsistentency. Path duplication also creates Swagger duplication, which clutters the interface with needless information.
You've placed a huge burden on all developers using your product with no reasonable solution. Is it possible to undeprecate the useTrailingSlash parameter?
@edebussc for the sake of completeness, option #2 does not need to be an external proxy. A filter, whether custom or existing like UrlRewriteFilter can also send redirects for URLs with a trailing slash.
For any further changes, we would more likely consider some such solution that handles trailing slashes early and in a centralized way vs deep within the framework as useTrailingSlash does and creates the possibility for mismatches with pattern matching elsewhere for authorization purposes.
@rstoyanchev
edebussc summed up the need to not break existing APIs well. Our company has customers using our API, some w/ SaaS, some with installs they have created in their own DCs. Breaking API changes will be a support nightmare.
Using a third-party solution like UrlRewriteFilter as a workaround is not great--I've been using OSS too long to have high confidence. Rolling our own filter is of course possible, but is error-prone. This was spring functionality; there should be a spring solution.
If you are considering "some such solution that handles trailing slashes early and in a centralized way", can you undeprecate useTrailingSlash until such time that another spring-based solution is available?
I had yesterday the issue with a request url having the "/" at the end, it was not working anymore after migrating to SB 3.x. With the deprecated
@Configuration
public class WebConfiguration implements WebMvcConfigurer {
@Override
public void configurePathMatch(PathMatchConfigurer configurer) {
configurer.setUseTrailingSlashMatch(true);
}
}
it works well. It would be nice to undeprecate it or at least provide an alternative solution.
My team just encountered this and we also have customers using our APIs which are not in our control. This whole situation with the suggested "solutions" is super frustrating, and honestly feels like a bad joke.
I agree, our favorite solution would also be to undeprecate setUseTrailingSlashMatch
I've created #31366 to explore built-in support for handling trailing slashes in a safer way than today. However, undeprecating trailing slash matching at this point, only to deprecate that later again, will only add to the confusion.
I was migrating a Spring Boot 2.7x project to Spring Boot 3.0.7. It cost me around 4 hours to figure out why random @PostMapping methods were failing. At long last I tried removing the backslash and suddenly this worked.
Had I been reading https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Migration-Guide#spring-mvc-and-webflux-url-matching-changes more carefully, I would've found the solution earlier.
It still bothers me, that I have to set configurer.setUseTrailingSlashMatch(true); even though the method is deprecated and the alternative is not even obvious. I wish I hadn't tried slipping the upgrade "under the hood". Good job -_-.
Prior to this change, a correctly authenticated and authorised request to a non-existent path returned a 404.
Following this change, such a request returns a 403. That means API client integrations to a valid path but including a trailing slash now returns a 403 instead of a 404. This is unhelpful and intuitive, and compounds the confusion and impact this PR is having.
I'd like to restore the behaviour of this resulting in a 404. Enabling configurer.setUseTrailingSlashMatch(true); does restore this behaviour, however I do not wish to actually support the superfluous trailing slashes - just return a 404 when an authenticated request is made to a valid endpoint with a trailing slash. How is this possible?
what is the deprecation period for PathMatchConfigurer#setUseTrailingSlashMatch ?
when setUseTrailingSlashMatch(true) it would work also for actuator api (i.e. actuator/env vs actuator/env/)?
Just to add to my initial response: in our project we have probably around 200 endpoints and 3 frontends, which access these endpoints in various manners (we hadn't used unified Feign Clients back when we started out).
This change means, that we have to adjust our endpoints in so they explicitly define the endpoint with a trailing slash and one without. We have to adjust all integration tests for all endpoints and we even should adjust all end-to-end tests in multiple platforms. This is easily 10+ business days of work for just this useless change. I'd resort to using setUseTrailingSlashMatch(true), but for some reason this method is marked as deprecated.
Isn't it possible to allow people, who did not pay 100% attention to the trailing slashes, opt-out of this "more security" aspect? We have implemente a few other means to strengthen our API security, so opting out of this thing would be okay.
Thanks Spring team! I spent an entire day on figuring out why the API stopped working.
You guys do great work with the framework; but this boggles my mind, how can such a decision be made?
We have literally hundreds of clients across dozens of environments, this will stall the adoption of SB3 for months and months and will undoubtedly cause issues even with all the due diligence in the world. Absolutely crazy change.
I whole heartedly support #31366 and especially the logging of such cases
We have literally hundreds of clients across dozens of environments, this will stall the adoption of SB3 for months and months and will undoubtedly cause issues even with all the due diligence in the world. Absolutely crazy change. I whole heartedly support #31366 and especially the logging of such cases
Spring upgrades have always been a pain, even between minor versions (remember "allow-bean-definition-overriding" ?).
You can use the deprecated option, it works (until it won't).
This was a horrible update. Should have been an optional property in application.yml with default false