Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable by default suffix pattern matching in Spring MVC #11105

Closed
philwebb opened this issue Nov 22, 2017 · 16 comments

Comments

Projects
None yet
9 participants
@philwebb
Copy link
Member

commented Nov 22, 2017

See this twitter exchange. We should confirm the existing behavior and consider it again for 2.0.

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

/cc @rstoyanchev @rwinch for opinions on this. Is changing from the MVC default a good idea?

@bclozel

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

TL;DR: this issue is about flipping the default for favorPathExtension to false in Spring Boot MVC auto-configuration.
We could at least add a configuration properties so that developers can change that easily.

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

We don't like the default in Spring MVC to begin with. This is the current advice we give but we can't change it without causing at least as much pain.

Note that there are two aspects to this issue. One is suffix pattern matching (i.e. "/foo" also implies "/foo.*") and that can be turned off via PathMatchConfigurer. The other is content type determination and that can be either turned off completely via ContentNegotiationConfigurer or specific file extensions may be registered. Registering specific file extensions has the effect of restricting both suffix pattern matching and content type determination to the given file extensions.

Turning off suffix pattern matching and content type resolution by file extension in favor of using a query parameter (if needed) is the ideal place to be. The explicit registration of known extensions may be a good middle ground that significantly limits the feature while also minimizing the impact to existing applications.

One idea for a list of extensions is to see what was done as part of a broader fix for RFD attacks in AbstractMessageConverterMethodProcessor, look for safeExtensions. From the experience with the RFD fix, you can also see the counter-point to the tweet, i.e. why URL-based content type negotiation is still desirable sometimes, but again a query parameter should better serve such requirements.

For what it's worth on the WebFlux side we do not support mapping by file extension. Neither do we support content type negotiation by file extension. By default only HeaderContentTypeResolver is registered and you can add a ParameterContentTypeResolver (i.e. query parameter) if you want URL-based alternative but there is no built-in resolver for file name extensions.

@rwinch

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

To add to what Rossen stated. Allowing for suffix pattern matching and matrix variables has been a large source of security bypass vulnerabilities. I would love to see path matching be less complicated to reduce the possibility of vulnerabilities in applications.

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2017

It's very tempting to change our default in 2.0 and offer a quick property to flip things back. I know it could cause upgrade pain, but it also:

  • Aligns WebMvc and WebFlux
  • Reduces the chances of a security vulnerability
  • Follows our own documentation advice
@bclozel

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

I'd like to flip that default as well.

We should do that as soon as possible, because this might impact a lot of applications/clients relying on that behavior. By our own standards, changing that after the milestone phase is the very last opportunity to do that.

@bclozel bclozel self-assigned this Jan 10, 2018

@bclozel bclozel changed the title Check content negotiation with URL extensions Disable by default suffix pattern matching in Spring MVC Jan 12, 2018

@bclozel bclozel closed this in 67e5897 Jan 15, 2018

bclozel added a commit that referenced this issue Jan 16, 2018

Move spring.mvc.media-types to content-negotiation
This commit moves "spring.mvc.media-types" to the
"spring.mvc.content-negotiation.*" namespaces introduced in gh-11105.

Closes gh-11636
@dpash

This comment has been minimized.

Copy link

commented Mar 7, 2018

I believe this change has made WebMvcConfigurer.configureContentNegotiation(ContentNegotiationConfigurer configurer) ineffective, regardless of the @order value given, as WebMvcAutoConfiguration.configureContentNegotiation(ContentNegotiationConfigurer configurer) always seems to happen after the user configurer.

This means the only way to configure the content negotiation is through the property files.

In 1.5, I was calling configurer.favorParameter(true), but this gets overwritten by the default property, false.

@bclozel

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Hi @dpash, I've reproduced this and it seems to be a much broader issue about the ordering of Spring Boot's WebMvcConfigurer. Could you create a separate issue?

@dg-grow-virgiliu-ratoi

This comment has been minimized.

Copy link

commented Apr 6, 2018

The behaviour before Spring Boot 2.0 has some weird behaviour in particular scenarios.

I have a file like this {context-root}/.0.44a38907.chunk.js, please note that the file name starts with a dot . and when a request is made to download the file, Spring MVC via RequestMappingInfoHandlerMapping and AbstractHandlerMethodMapping matches this path to /.* and returns the view associated to the root / which happens to be an HTML file in my case.

Btw, it seems that whenever a request is made, the registered paths are iterated to find the matching one. Shouldn't they be cached somehow, in case a path has been matched for a request the next time a similar request is made the method mapping should not be recalculated?

@sportmachine

This comment has been minimized.

Copy link

commented Jun 10, 2018

no clear documents, changing fast. The spring security and suffix things waste a lot of people time, totally stupid, nutty!

@philwebb

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

For those looking for documentation, the migration guide should help.

@getjack

This comment has been minimized.

Copy link

commented Jul 5, 2018

This fix really killed my application and i had no idea where to look for the fix because there was no appropiate error warning.

Please consider my story:
I have a @RestController which accepts an email address as @PathVariable . You all know the RFC which allows the . char to appear in an URI so there is no percent-encoding for this. If you'd now call the endpoint e.g. GET /person/email@provider.com the spring application would always try to serve an unknown filetype because of the .com "file ending". Even when i enforced a media type via the produces parameter it would not work.

Even though i think it is usefull to have an auto-configuration like this, it breaks existing applications with no reason and feels like an anti-pattern. I'd suggest a bug here that the @RequestMapping annotation should be always considered first to override the behavior of suffix path matching. Feel free to argue but this really killed my code. I just noticed it by pure luck while lurking through the migration guide.

@bclozel

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

@getjack Sorry that you have to deal with this situation. I've got a few questions for you, so we can hopefully resolve that situation - could you answer those?

  1. How were you matching that @PathVariable previously? Could you share the controller method signature?

  2. Suffix pattern matching is more or less equivalent to suffix all your patterns with ".*". If you had a "/email/{email}" pattern on a controller, this means a "/email/{email}.*" pattern is considered. Did your current application work with emails like "user@domain.co.uk"?

  3. What happens if you change your route pattern with something like @GetMapping("/email/{email:.+}")?

@getjack

This comment has been minimized.

Copy link

commented Jul 6, 2018

Hi @bclozel ,
sorry i didn't thought through my comment. Could've given you the information from the get go.
This may answer all your questions at once.
@GetMapping(path = "/person/{email:.+}", produces = MediaType.APPLICATION_JSON_UTF8_VALUE) ResponseEntity<Person> getPerson(@Nonnull @PathVariable("email") final String email)
So no huge suprise here.

Regarding 2): Not sure if there was ever a check for domains like ".co.uk" . Just inherited the project from a co-worker.

@bclozel

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

@getjack there must be something else at play here, because this mapping still properly works in a Spring Boot 2.0 application. Do you have a sample application I can take a look at?

@getjack

This comment has been minimized.

Copy link

commented Jul 6, 2018

I know what went wrong here. There was an empty annotated class using @EnableWebMvc.
Sorry for bloating that closed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.