Skip to content

Disable by default suffix pattern matching in Spring MVC #11105

Closed
@philwebb

Description

@philwebb
Member

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

Activity

added this to the 2.0.0.RC1 milestone on Nov 22, 2017
philwebb

philwebb commented on Nov 29, 2017

@philwebb
MemberAuthor

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

bclozel

bclozel commented on Nov 29, 2017

@bclozel
Member

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

rstoyanchev commented on Nov 29, 2017

@rstoyanchev
Contributor

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

rwinch commented on Nov 29, 2017

@rwinch
Member

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

philwebb commented on Nov 30, 2017

@philwebb
MemberAuthor

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

bclozel commented on Jan 5, 2018

@bclozel
Member

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.

7 remaining items

dpash

dpash commented on Mar 7, 2018

@dpash

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

bclozel commented on Mar 7, 2018

@bclozel
Member

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?

virgiliu-ratoi-ec-ext

virgiliu-ratoi-ec-ext commented on Apr 6, 2018

@virgiliu-ratoi-ec-ext

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

sportmachine commented on Jun 10, 2018

@sportmachine

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

philwebb

philwebb commented on Jun 11, 2018

@philwebb
MemberAuthor

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

maunzCache

maunzCache commented on Jul 5, 2018

@maunzCache

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

bclozel commented on Jul 5, 2018

@bclozel
Member

@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:.+}")?

maunzCache

maunzCache commented on Jul 6, 2018

@maunzCache

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

bclozel commented on Jul 6, 2018

@bclozel
Member

@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?

maunzCache

maunzCache commented on Jul 6, 2018

@maunzCache

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

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @bclozel@dpash@maunzCache@rwinch@rstoyanchev

      Issue actions

        Disable by default suffix pattern matching in Spring MVC · Issue #11105 · spring-projects/spring-boot