@RequestMapping without any attributes behaves as "default endpoint"

[anatoliy-balakirev]

Spring version: 5.1.5.RELEASE

In one of our micro services we've made a mistake and instead of writing something like this:

@RestController
@RequestMapping("/my/custom/controller")
public class MyController {

// GET and POST methods mapped to `/my/custom/controller` endpoint

Created this:

@RestController("/my/custom/controller")
public class MyController {

Everything worked fine, all our requests were handled but then at some point we realised that all requests, even for non-existing endpoints, are mapped to that controller. So this:

http://localhost:8080/whatever

Was getting into our controller by default:

2019-03-07 16:47:28.014 DEBUG 83179 --- [nio-8080-exec-3] o.s.web.servlet.DispatcherServlet        : GET "/whatever", parameters={}
2019-03-07 16:47:28.016 DEBUG 83179 --- [nio-8080-exec-3] s.w.s.m.m.a.RequestMappingHandlerMapping : Mapped to public void com.example.demo.MyController.doSomething()
Got to my custom controller 1
2019-03-07 16:47:28.017 DEBUG 83179 --- [nio-8080-exec-3] m.m.a.RequestResponseBodyMethodProcessor : Using 'application/json', given [*/*] and supported [application/json, application/*+json, application/json, application/*+json]
2019-03-07 16:47:28.017 DEBUG 83179 --- [nio-8080-exec-3] m.m.a.RequestResponseBodyMethodProcessor : Nothing to write: null body
2019-03-07 16:47:28.017 DEBUG 83179 --- [nio-8080-exec-3] o.s.web.servlet.DispatcherServlet        : Completed 200 OK

With that config in place, I would expect to get 404 for both: /whatever and /my/custom/controller requests, because in fact there is no mapping for /my/custom/controller in my app (there is a bean with that name instead).

Here is a whole controller's code, no other config is required to reproduce this, just standard spring boot app with this single controller:

package com.example.demo;

import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController("/my/custom/controller")
public class MyController {

    @GetMapping
    public void doSomething() {
        System.out.println("Got to my custom controller 1");
    }

    @PutMapping
    public void doSomethingElse() {
        System.out.println("Got to my custom controller 2");
    }

}

[sbrannen]

Please note that Spring MVC supports mapping from bean names (that begin with a forward slash /) to controllers.

See the javadoc for BeanNameUrlHandlerMapping for details.

Thus, in your use case, when "/my/custom/controller" is configured as the bean name for the controller (i.e., via the value attribute in @RestController), that path is used by BeanNameUrlHandlerMapping to register a handler mapping automatically.

So that explains why you do not get a 404 for requests to "/my/custom/controller".

Regarding the /whatever requests, is there a similarity between the real value for "whatever" and the actual bean name of the REST controller?

[anatoliy-balakirev]

Nope, there is no similarity. I'm literally sending that whatever in the request. In fact any request is getting mapped to that controller. Just tried this: http://localhost:8080/some_non_existing_endpoint And here are logs:

2019-03-07 17:19:09.198  INFO 83733 --- [nio-8080-exec-2] o.a.c.c.C.[Tomcat].[localhost].[/]       : Initializing Spring DispatcherServlet 'dispatcherServlet'
2019-03-07 17:19:09.198  INFO 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : Initializing Servlet 'dispatcherServlet'
2019-03-07 17:19:09.198 DEBUG 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : Detected StandardServletMultipartResolver
2019-03-07 17:19:09.211 DEBUG 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : enableLoggingRequestDetails='false': request parameters and headers will be masked to prevent unsafe logging of potentially sensitive data
2019-03-07 17:19:09.212  INFO 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : Completed initialization in 14 ms
2019-03-07 17:19:09.222 DEBUG 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : GET "/some_non_existing_endpoint", parameters={}
2019-03-07 17:19:09.230 DEBUG 83733 --- [nio-8080-exec-2] s.w.s.m.m.a.RequestMappingHandlerMapping : Mapped to public void com.example.demo.MyController.doSomething()
Got to my custom controller 1
2019-03-07 17:19:09.263 DEBUG 83733 --- [nio-8080-exec-2] m.m.a.RequestResponseBodyMethodProcessor : Using 'application/json', given [*/*] and supported [application/json, application/*+json, application/json, application/*+json]
2019-03-07 17:19:09.264 DEBUG 83733 --- [nio-8080-exec-2] m.m.a.RequestResponseBodyMethodProcessor : Nothing to write: null body
2019-03-07 17:19:09.264 DEBUG 83733 --- [nio-8080-exec-2] o.s.web.servlet.DispatcherServlet        : Completed 200 OK

More readable extraction from there:

GET "/some_non_existing_endpoint", parameters={}
Mapped to public void com.example.demo.MyController.doSomething()

This controller class is the only bean I have in my freshly created spring boot app.

[sbrannen]

@anatoliy-balakirev, thanks for the feedback.

This controller class is the only bean I have in my freshly created spring boot app.

Good to know.

Based on some quick experimentation, you should experience the same behavior if you simply annotate your controller class with @RestController (i.e., without specifying any bean name).

[anatoliy-balakirev]

Here is a sample project: https://github.com/anatoliy-balakirev/rest-controller-mapping

[anatoliy-balakirev]

Based on some quick experimentation, you should experience the same behavior if you simply annotate your controller class with @RestController (i.e., without specifying any bean name).

Yep, seems to be the case. So looks like it's kind of getting mapped to "*", which seems to be not so good default, as for me. Would be better to get it not mapped at all as in fact there is no mapping provided.

[sbrannen]

I agree that this behavior does not appear to be what the default should be in such cases.

I have been able to reproduce this in the following test class based purely on core Spring (using this Person class).

In summary, a GET request to /whatever effectively gets mapped onto the PersonController#getPerson() method.

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.MediaType;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.test.context.web.WebAppConfiguration;
import org.springframework.test.web.Person;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.context.WebApplicationContext;
import org.springframework.web.servlet.config.annotation.EnableWebMvc;

import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@RunWith(SpringRunner.class)
@WebAppConfiguration
public class MissingExplicitRequestMappingTests {

    @Autowired
    WebApplicationContext wac;

    MockMvc mockMvc;

    @Before
    public void setup() {
        this.mockMvc = MockMvcBuilders.webAppContextSetup(this.wac).build();
    }

    @Test
    public void person() throws Exception {
        this.mockMvc.perform(get("/whatever").accept(MediaType.APPLICATION_JSON))//
                .andDo(print())//
                .andExpect(status().isOk())//
                .andExpect(content().string("{\"name\":\"Joe\",\"someDouble\":0.0,\"someBoolean\":false}"));
    }

    @Configuration
    @EnableWebMvc
    static class WebConfig {

        @Bean
        PersonController personController() {
            return new PersonController();
        }

    }

    @RestController("/person")
    // @RequestMapping("/person")
    static class PersonController {

        @GetMapping
        public Person getPerson() {
            return new Person("Joe");
        }

    }

}

@rstoyanchev, I seem to recall that we once discussed this particular scenario. Can you perhaps provide some light on the subject before I dive any deeper into a search for a solution?

[anatoliy-balakirev]

To add more context to this and why it might be security issue in some cases: In our case all endpoints are secured, except for actuator ones. Actuator endpoints are started on the separate port (not 8080), which is not accessible from the outside of the cluster. However, when I've sent a request to http://localhost:8080/actuator/stats (which is enabled, but not secured; and it was not localhost in my case obviously) - it bypassed security because it's an actuator endpoint (despite the fact that request was sent not to actuator's port) and got into my default controller, which sounds quite dangerous if my endpoint would do something bad.

[sbrannen]

Tentatively slated for 5.1.6 for further investigation.

[bclozel]

I disagree with the analysis here.

Adding a @GetMapping or a @RequestMapping annotation on a Controller handler means that you're mapping this method as a handler and providing information about which requests should be mapped there. By default the @RequestMapping annotation is catching basically everything and it's up to you to refine what the method should handle (see javadoc).

If we were to constraint that, we'd break many valid use cases where people are mapping requests based on params or headers, and not providing any constraint about path.

If anything, I think this issue shows that it's easy to get confused and annotate @Controller or @RestController with something that we think is a path constraint. Maybe mapping by bean names is not used much theses days and this is creating confusion.

[anatoliy-balakirev]

@bclozel maybe I'm missing something, but can you please, point me to the place in the spec, which is saying that @RequestMapping without provided path is catching everything? Wasn't able to find anything like that there...

If backward compatibility in this particular case is really needed (even though I find this default behaviour really confusing) - what about at least issuing some warning at startup? So that when needed - people can explicitly define it as @RequestMapping("*"), otherwise they will go and fix it (if it's not a desired behaviour as was in our case). Just silently allowing this is a potential source of security issues as I described above.

[bclozel]

@anatoliy-balakirev the @RequestMapping javadoc is already explaining what you can add as attributes to refine the matching process. We could add there a sentence saying that "if you don't specify constraints, it will match" but I'm not sure that a negative phrasing will really help.

If this were about security, then we'd force developers to provide a path and we'd make that attribute mandatory. It's never been the case. Adding a WARN log wouldn't really solve the issue in my opinion.

In this case you ran into a problem and I totally get the frustration - the thing is when this happens the problem is quite obvious since this will match many unintended requests, like requests for static resources in a typical Spring Boot setup.

I think your suggestion to improve the documentation is the right one, can you suggest places (in the javadoc/reference doc) where you think this could be helpful?

[rstoyanchev]

This is somewhat related to #21585 in the sense that in both cases the class-level mapping is expressed in @RestController. However the proposal there is for a separate path attribute that probably still wouldn't have helped with the issue here.

The trouble is that from a Spring MVC perspective the controller appears to have an empty @GetMapping. As Brian said once the annotation is added it expresses the intent to expose an endpoint but the only mapping constraint is the HTTP method.

Perhaps what we could consider flagging such cases where there is nothing but an empty @RequestMapping. I can't imagine a valid scenario for that. If the intent is to really match every URL pattern it should be more explicit with "/**" or "**".

[sbrannen]

I can't imagine a valid scenario for that. If the intent is to really match every URL pattern it should be more explicit with "/**" or "**".

I fully agree with that.

That's actually what I meant when I said I don't think the reported behavior is what the default should be.

Perhaps what we could consider flagging such cases where there is nothing but an empty @RequestMapping.

What are you proposing with "flagging": logging or throwing an exception?

To me it really seems like a user configuration error if the user doesn't supply any pattern at all.

So I'm wondering if throwing an exception would be appropriate (perhaps with a SpringProperties flag to change the behavior between log and throw exception).

[bclozel]

If we consider that it's not a valid scenario, then I'd vote to throw an exception, just like we do already for duplicate mappings. But then I'd also vote to move that to 5.2.

[sbrannen]

But then I'd also vote to move that to 5.2.

True. If we change the behavior, I also agree that we should do this in 5.2.

[anatoliy-balakirev]

Sorry, just got back here.

@bclozel when I look at RequestMapping's path method:

    /**
     * The path mapping URIs (e.g. "/myPath.do").
     * Ant-style path patterns are also supported (e.g. "/myPath/*.do").
     * At the method level, relative paths (e.g. "edit.do") are supported
     * within the primary mapping expressed at the type level.
     * Path mapping URIs may contain placeholders (e.g. "/${connect}").
     * <p><b>Supported at the type level as well as at the method level!</b>
     * When used at the type level, all method-level mappings inherit
     * this primary mapping, narrowing it for a specific handler method.
     * @see org.springframework.web.bind.annotation.ValueConstants#DEFAULT_NONE
     * @since 4.2
     */
    @AliasFor("value")
    String[] path() default {};

It's not clear what default {} actually is. Adding some comment, saying that if it's not provided neither on class nor on method level - then it will be mapped to * would already help (for all relevant methods in that class). But as discussed above throwing an exception seems to be a better option.

Regarding this:

the thing is when this happens the problem is quite obvious since this will match many unintended requests, like requests for static resources in a typical Spring Boot setup.

For us problem was absolutely not obvious. In our micro service we have a couple of endpoints and only one had this issue (out of curiosity I tried to do the same for the second one and got startup failure). All endpoints, except for actuator ones were secured, so all requests without security token were returning unauthorised. However, requests to actuator endpoints (but using main app's port) were successfully forwarded to our default controller. We accidentally found it in our logs, when were testing something else. Then it took us quite some time to actually figure out why requests are mapped there (it's not so obvious when you read @RestController("/my/custom/controller") that it's actually supposed to be @RequestMapping("/my/custom/controller") if you don't write a lot of those every day). Having at least some warning in log would already help significantly.

P.S. But as discussed above - I would also prefer it to throw an exception in that case.

[rstoyanchev]

Yes by flagging I meant an IllegalStateException probably in RequestMappingHandlerMapping when a RequestMappingInfo is created. A target of 5.2 makes sense to me.

[sbrannen]

Sounds good. I'll slate this for 5.2 M1 for the time being.

[sbrannen]

in progress

[sbrannen]

This has been addressed for Spring MVC controllers in c0b52d09f57fcabd75e664b7994dea12ba28f408.

@rstoyanchev, do you foresee any need to do anything similar for WebFlux?

[rstoyanchev]

Absolutely.

[odrotbohm]

The change here causes Spring Data REST controllers to break. We have the additional complexity of a global, configurable base URI (e.g. /base). If that path is configured, we alter the registered request mappings to prepend that base path to all mappings detected (see Spring . Data REST's BasePathAwareHandlerMapping.customize(…).

We then have handler methods mapped to both @RequestMapping({ "/", "" }) to indicate that they should be selected for both requests to /base as well as /base/. The latter mapping is now rejected. If I remove that mapping, for requests to /base the mapped method is not selected anymore.

I can of course change our mapping customization to also register a mapping for "" if the only mapping found is /. However, I wonder if that then wouldn't be subject to rejection in case no base path is configured. I would essentially want to map both http://localhost:8080 and http://localhost:8080/. I haven't tried yet but unless something special is done to the "no path at all" case, this should be rejected then as well, right?

[sbrannen]

Update: we've decided to log WARNING messages instead of throwing an exception, and we'll only do that if there is not at least one non-empty path mapping.

[sbrannen]

The revised implementation has been pushed to master in commit 72027b1746c5dfd3f6e35b57e522b78290a4a4f0.

[rstoyanchev]

Sorry to re-open, partly by mistake (hitting the button) but also to discuss a little further.

@odrotbohm when is the prefix applied, i.e. what kind of hook is it using? Perhaps we could change where we check for this and still raise an exception? Or alternatively at runtime if we see we're dealing with an empty mapping we have an opportunity still to change how we treat it.

[rstoyanchev]

@odrotbohm, keep in mind also that as of 5.1 there is an option for inserting a common prefix to controller mappings, and if that option is used I would expect the change to work as it was implemented originally. If you're not using that feature perhaps it's something to try and switch to?

[sbrannen]

@odrotbohm, since we're planning on releasing 5.2 M1 on Tuesday, it would be great if you could provide feedback to Rossen's questions by midday tomorrow (Monday).

Thanks in advance!

[odrotbohm]

I‘m on PTO next week so I am not sure I’ll be able to investigate. As indicated in Slack Spring Data REST is back to work after you changes. Any chance we postpone any further discussion to after the release?

[sbrannen]

I‘m on PTO next week so I am not sure I’ll be able to investigate.

OK. Thanks for the heads up.

As indicated in Slack Spring Data REST is back to work after you changes.

That's because we switched to logging a warning instead of throwing an exception; however, we are considering changing back to throwing an exception. Hence Rossen's line of questions.

Any chance we postpone any further discussion to after the release?

The Spring Framework team will make that decision during tomorrow's team call.

[rstoyanchev]

After further review, currently a single (i.e. method-level) @RequestMapping() does not narrow the mapping and in effect matches to any path, which is the surprising behavior from this issue. If however there is also a @RequestMapping() at the type level (or some variation of it but with empty paths still), the resulting combined mapping has one empty pattern (i.e. "") which matches to empty paths only, e.g. "http://example.org". That's an intuitive default and the intended behavior. The fact that a method-level only mapping does not is an oversight and after discussion, the goal now is to ensure that empty path mapping always behaves as an empty path.

[rstoyanchev]

This is likely to break some existing tests that rely on the current behavior. Our own tests had several but it also helped to reveal bad tests in MockWebMvcConnectionTests which were passing only because the mapping was accepting any path. If there is production code that is broken as a result of this, it should result in 404 errors, and arguably it's a good idea to switch to an explicit "/**" if the intent was really to match any path.

[anatoliy-balakirev]

Hi. Sorry, I'm a bit lost here. So how is it solved eventually? By logging warning of throwing an exception?

[rstoyanchev]

Neither warning, nor exception. Rather the same as "" (empty path).