MockMvc needs to accept prepared URI with encoded URI path variables [SPR-11441]

[spring-projects-issues]

daniel carter opened SPR-11441 and commented

Good Afternoon,

I have a controller with the following mapping

@RequestMapping(value = "/circuit/{id}/view", method = RequestMethod.GET)
public String getCircuit(@PathVariable String id,
            @ModelAttribute("search") CircuitSearchController.SearchParams search, Model model, Principal principal) {...}

The id value can contain reserved characters so I URL encode when rendering the view. For an id of "ja-ran-17 gigabitethernet 10/0.5790740:579-747" the browser request thus looks like

http://localhost/myapp/circuit/ja-ran-17%20gigabitethernet%2010%2F0.5790740:579-747/view

This is working ok.

When i try to write a test for it however, the URL is getting double encoded

        ResultActions result = mockMvc.perform(get("/circuit/{id}/view", "ja-ran-17%20gigabitethernet%2010%2F0.5790740:579-747").principal(p));

        // THEN - circuit should be in the model
                    
result.andExpect(status().isOk()).andExpect(model().attribute("circuit", hasProperty("circuitId", is(id))));

log from real request in tomcat

DEBUG o.s.web.servlet.DispatcherServlet - DispatcherServlet with name 'dispatcherServlet' processing GET request for [/circuit-id-manager/circuit/ja-ran-17 gigabitethernet 10/0.5790740:579-747/view] 
DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/ja-ran-17%20gigabitethernet%2010%2F0.5790740:579-747/view 
DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Returning handler method [public java.lang.String [...]

log from mockmvc request

DEBUG o.s.t.w.s.TestDispatcherServlet - DispatcherServlet with name '' processing GET request for [/circuit/ja-ran-47%20gigabitethernet%2010%2F0.5790740:579-747/view] 
DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/ja-ran-47%2520gigabitethernet%252010%252F0.5790740:579-747/view 

With a real request my controller gets passed id="ja-ran-17 gigabitethernet 10/0.5790740:579-747"
With a mock request my controller is passed id="ja-ran-47%20gigabitethernet%2010%2F0.5790740:579-747"

Perhaps it is a feature of MockMvc that it automatically does URI encoding of path variables? If so it is inconsistent with other parts of the Web-Mvc framework see #16028

So i remove the encoding of my path variable and let MockMvc encode it. Now it doesn't find the handler

DEBUG o.s.t.w.s.TestDispatcherServlet - DispatcherServlet with name '' processing GET request for [/circuit/ja-ran-47 gigabitethernet 10/0.5790740:579-747/view] 
DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/ja-ran-47%20gigabitethernet%2010/0.5790740:579-747/view 
DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Did not find handler method for [/circuit/ja-ran-47%20gigabitethernet%2010/0.5790740:579-747/view] 

I had a similar problem where the framework was treating %2F as / in contravention of RFC3986[1]. That is solved by a BeanPostProcessor calling

            // URL decode after request mapping, not before.
            requestMappingHandlerMapping.setUrlDecode(false);

            // Workaround to make the previous fix work. See https://jira.springsource.org/browse/SPR-11101.
            requestMappingHandlerMapping.setAlwaysUseFullPath(true);

My guess is that the MockMvc Framework is not using the normal request mapping handler mapping, and so calling setUrlDecode on the normal one has no effect?

[1]"When a URI is dereferenced, the components and subcomponents
significant to the scheme-specific dereferencing process (if any)
must be parsed and separated before the percent-encoded octets within
those components can be safely decoded, as otherwise the data may be
mistaken for component delimiters."

and again in 7.3
"Percent-encoded octets must be decoded at some point during the
dereference process. Applications must split the URI into its
components and subcomponents prior to decoding the octets, as
otherwise the decoded octets might be mistaken for delimiters."

That is clearly what is happening here, %2F is being mistaken for a path delimiter. Path delimitation must take place before decoding.

---

Affects: 3.2.5

Attachments:

• uri-encode-test.tar.bz2 (4.96 kB)

Referenced from: commits 0b69a0b

0 votes, 5 watchers

[spring-projects-issues]

daniel carter commented

"My guess is that the MockMvc Framework is not using the normal request mapping handler mapping, and so calling setUrlDecode on the normal one has no effect?"

My guess is wrong. Having spent a couple hours debugging it. UrlPathHelper.getRequestUri(...)

if (uri == null) {
			uri = request.getRequestURI();
		}

Under Catalina request.getRequestURI returns
/circuit-id-manager/circuit/ja-ran-17%20gigabitethernet%2010%2F0.5790740:579-747/view
which is what i would expect.

While MockHttpServletRequest returns
/circuit/ja-ran-47%20gigabitethernet%2010/0.5790740:579-747/view
having somewhat randomly decided to decode %2F, but not %20.

There seems to be a consistent issue in spring-mvc whereby it mistakenly treating %2F as /
RFC3986 :
"If data for a URI component would conflict with a reserved
character's purpose as a delimiter, then the conflicting data must be
percent-encoded before the URI is formed.
The purpose of reserved characters is to provide a set of delimiting
characters that are distinguishable from other data within a URI.
URIs that differ in the replacement of a reserved character with its
corresponding percent-encoded octet are not equivalent. Percent-
encoding a reserved character, or decoding a percent-encoded octet
that corresponds to a reserved character, will *change how the URI is
interpreted* by most applications.

So by my reading, a "/" should be interpreted as a delimiter. "%2F" should be interpreted differently, that is, as data encoded into the path, and not as a path delimiter. The whole purpose of encoding something as %2F is to avoid it being treated as a path delimiter. Yet spring is doing that. Nowhere in RFC3986 does it distinguish %2F from the other reserved characters, and say it should be decoded. Why does spring treat it differently to %20 ?

[spring-projects-issues]

daniel carter commented

The problem seems to be in all the UriComponents stuff, first you do the expansions, and then you encode the whole path. For path variables, this is fundamentally flawed. Once you've done the substitutions, you don't know which / are path delimiter and which are part of path variables. The decision to have special URL Encoding mechanism where you encode all the reserved characters except for / is a symptom of this flaw. URL encoding must encode all reserved characters.

given the UrlTemplate ```
/circuit/{id}/view

you expand, giving /circuit/my/id/view
then when encoding how can you know that circuit/my is two path segments delimited with / while my/id is one path segment requiring encoding?


The solution is simply to encode as you substitute, not after. 


should be ```
this.uriComponents = UriComponentsBuilder.fromUriString(urlTemplate).buildAndExpandAndEncode(urlVariables);

instead of ```
this.uriComponents = UriComponentsBuilder.fromUriString(urlTemplate).buildAndExpand(urlVariables).encode();

For instance in HierarchicalUriComponents

public PathComponent expand(UriTemplateVariables uriVariables) {
     String expandedPath = expandUriComponent(getPath(), uriVariables);
     return new FullPathComponent(expandedPath);
}

You could do the encoding here.

There seems to be some support for saying that the uriVariables are already encoded?  Could exposing that through the MockMvcRequest, and doing the encoding in the test case, work-around this issue in the short term?  i.e. in MockHttpServletRequestBuilder constuctor
this

UriComponentsBuilder.fromUriString(urlTemplate).build(encoded).expand(urlVariables).encode();

intead of this

this.uriComponents = UriComponentsBuilder.fromUriString(urlTemplate).buildAndExpand(urlVariables).encode()

[spring-projects-issues]

daniel carter commented

From the RFC
"Applications must split the URI into its
components and subcomponents prior to decoding the octets, as
otherwise the decoded octets might be mistaken for delimiters"

The opposite of this of course is application must encode the components and subcomponents prior to composing the full URI, as otherwise the unencoded octets might be mistaken for delimiters. Which is what's happening here.

[spring-projects-issues]

Rossen Stoyanchev commented

UriComponentsBuilder is smart about expanding. If you look inside you'll see UriComponents is composed of fields, not a String, so we can encode each URI component accordingly down to the path segment and query variable. Whatever the problem is, this is not the cause of it.

Can you provide information on how you build your MockMvc? In other words do you load your spring configuration or do you use the StandaloneMockMvcBuilder? My guess is it's the latter in which case the urlDecode and alwaysUseFullPath properties of the RequestMappingHandlerMapping are not getting set. We can add support for those properties on StandaloneMockMvcBuilder but you can also try building MockMvc from actual Spring configuration.

[spring-projects-issues]

daniel carter commented

Hi,

Finally got the release out the door so can return to this issue.

I'm using MockMvc to load an actual spring configuration

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration({ "classpath:/spring/wcim-service-context.xml",
        "file:src/main/webapp/WEB-INF/wcim-mvc-context.xml",
        "/spring/load-properties-context.xml" })
@WebAppConfiguration
@Transactional
public class ControllerIT {

 @Inject
    private WebApplicationContext webAppCtx;

private MockMvc mockMvc;

@Before
    public void setUp() {

mockMvc = MockMvcBuilders.webAppContextSetup(webAppCtx).build();

I've tested in the debugger that urlDecode and alwaysUseFullPath properties are being set.

The problem is the MockHttpServletRequest.getRequestURI() returns a URL with %2F decoded to /
But a real webcontainer getRequestURI() returns the actual URL with the %2F retained.

In a real container, the HandlerMapping works
/circuit/my%2Fid/view matches /circuit/{id}/view

In MockMvc the mapping doesn't work
/circuit/my/id/view does not match /circuit/{id}/view

UriComponents is composed of fields, but that's not the issue. The issue is with variable substitution. You substitute variables into each field, then you encode the each field. This is incorrect.

You need to encode each variable, then substitute it into each field.

They way it works at the moment, with substitution then encoding, when you encounter a reserved character, how can you know if it was a literal character taking it's reserved meaning (and thus doesn't need encoding) or part of a variable (and thus does need encoding).

An example:
Given the request mapping = /circuit/{id}/view
where id = a'E# /&"%
id should be encoded = a'E%23%20%2F&%22%25
resulting in URL /circuit/a'E%23%20%2F&%22%25/view

but you do the substitution first
/circuit/a'E# /&"%/view
Now, when you encode, how do you know which reserved characters are literals that should take their special meaning (i.e. that / is a path delimiter) and which are application data values that need encoding?

You assume that variable might have a # or a space, or a " etc, and encode those, but for some reason you assume a / will never be in a variable.
resulting in URL /circuit/a'E%23%20/&%22%25/view

variables need to be encoded to escape reserved characters. In a path,

[spring-projects-issues]

daniel carter commented

A full working test case is worth a 1000 of my rambling words.

Extract the attached maven project and build.

Neither of the two tests pass
MockMvc, raw id

13:10:23.231 [main] DEBUG o.s.t.w.s.TestDispatcherServlet - DispatcherServlet with name '' processing GET request for [/circuit/a/b]
13:10:23.231 [main] DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/a/b
13:13:13.791 [main] DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Did not find handler method for [/circuit/a/b]

MockMvc, encoded id

13:13:38.916 [main] DEBUG o.s.t.w.s.TestDispatcherServlet - DispatcherServlet with name '' processing GET request for [/circuit/a%2Fb]
13:13:38.916 [main] DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/a%252Fb
13:13:38.932 [main] INFO  uriencodetest.TestController - ID Received: a%2Fb

Deploy the war to tomcat started with

-Dorg.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true

Hit the URL /uri-encode-test/test.jsp and it works fine.

Tomcat
13:09:44.435 [http-8080-2] DEBUG o.s.web.servlet.DispatcherServlet - DispatcherServlet with name 'dispatcherServlet' processing GET request for [/uri-encode-test/circuit/a/b]
13:09:44.450 [http-8080-2] DEBUG o.s.w.s.m.m.a.RequestMappingHandlerMapping - Looking up handler method for path /circuit/a%2Fb
13:09:44.482 [http-8080-2] INFO  uriencodetest.TestController - ID Received: a/b

[spring-projects-issues]

daniel carter commented

For another perspective, consider if this was implemented with old-style query parameters, and the id value contained a reserved delimiter

/circuits?id={id}

where id="abc&def" the following URL is generated

/circuits?id=abc%26def

Nobody says, oh & is a valid character in a query string, maybe they are expecting the value of one parameter to expand out into a whole query string, we better leave it alone and generate
/circuits?id=abc&def

[spring-projects-issues]

daniel carter commented

#16028 spring:url tag has the same issue

[spring-projects-issues]

Rossen Stoyanchev commented

A full working test case is worth a 1000 of my rambling words.

I'm sorry but this ticket has too much already. Please submit a repro project and I'll be happy to take a look and respond.

[spring-projects-issues]

daniel carter commented

Ok i have copied the demonstration project from this JIRA into a git repro project. It's my first time using git, do you see the pull request?

spring-attic/spring-framework-issues#69

[spring-projects-issues]

daniel carter commented

I've also verified that Jetty displays the expected behaviour

2014-03-14 13:38:55,857 DEBUG [org.springframework.web.servlet.DispatcherServlet] - <DispatcherServlet with name 'appServlet' processing GET request for [/SPR-11441-1.0-SNAPSHOT/circuit/a/b]>
2014-03-14 13:38:55,857 DEBUG [org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping] - <Looking up handler method for path /circuit/a%2Fb>
2014-03-14 13:38:55,857 DEBUG [org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping] - <Returning handler method [public java.lang.String org.springframework.issues.TestController.getCircuit(java.lang.String,org.springframework.ui.Model)]>
2014-03-14 13:38:55,857 DEBUG [org.springframework.web.servlet.DispatcherServlet] - <Last-Modified value for [/SPR-11441-1.0-SNAPSHOT/circuit/a/b] is: -1>
2014-03-14 13:38:55,872 INFO [org.springframework.issues.TestController] - <ID Received: a/b>

[spring-projects-issues]

Rossen Stoyanchev commented

daniel carter, thanks for the sample project. It shows exactly the problem.

MockMvcRequestBuilders process the provided URI template and also encode it. This is documented in the Javadoc and it works exactly like the RestTemplate methods.

However unlike the RestTemplate, MockMvcRequestBuilders does not provide alternative methods that accept a URI, which is already encoded (or not) and is used as is. So we will add additional methods to MockMvcRequestBuilders (one for each HTTP method) that accepts a prepared URI. That will match to the way the RestTemplate works.