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

Unable to turn-off Url Decoding in 'UrlPathHelper.setUrlDecode' [SPR-15652] #20211

Closed
spring-issuemaster opened this issue Jun 11, 2017 · 10 comments
Closed

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jun 11, 2017

Izek Greenfield opened SPR-15652 and commented

Using spring-boot with Async MVC (defferdResult):

I have request mapping:

"/foo/{id}/add/{value}"

The URI I send is:

PUT /foo/vv%2F1234/add/2 (encoded URI of 'foo/vv/1234/add/2' )

I try to set 'UrlPathHelper.setUrlDecode' to false:

@SpringBootApplication
public class Startup extends WebMvcConfigurerAdapter  {
   @Override 
   public void configurePathMatch(PathMatchConfigurer configurer) {
    UrlPathHelper urlPathHelper = new UrlPathHelper();
    urlPathHelper.setUrlDecode(false);
    configurer.setUrlPathHelper(urlPathHelper);
  }
}

But this only partially succeedes:

The controller gets the right values for the PathVariables (i.e. the '{id}' get the value: 'vv/1234'), And all the logic is ok.
BUT the response I finally get is "405 Request Method 'PUT' not supported"

I found that the resource that is being used in the response path is not encoded and I also found that in many places in the code the Object 'UrlPathHelper' is newly created and doesn't use an injected reference to the object create and configure as per above.
so, the new objects are not configured to turn off decoding. I also noticed this behaviour (creating new instaces internall by spring) happens in many places (which seems to break one of springs most basic features - IOC).

for example:
org.springframework.web.context.request.async.WebAsyncManager
org.springframework.boot.actuate.autoconfigure.MetricsFilter


Affects: 4.2.8

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 12, 2017

Rossen Stoyanchev commented

controller gets the right values for the PathVariables

You had me up to this point and then you lost me. There is no indication what your controller looks like or how to reproduce the 405 outcome. Please provide more details.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 13, 2017

Izek Greenfield commented

Controller:

@RestController
public class SampleController {
	@RequestMapping(value = "/foo/{id}/add/{value}", method = RequestMethod.PUT)
	public DeferredResult<String> put(@PathVariable String id, @PathVariable String value) {
		final DeferredResult<String> deferredResult = new DeferredResult<String>();

		final String res = id + " " + value;
		Thread t = new Thread(() -> deferredResult.setResult(res));
		t.start();

		return deferredResult;
	}
}
@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 13, 2017

Rossen Stoyanchev commented

Thanks for that (note I corrected a couple of typos in the code snippet).

When the DeferredResult is set, we perform a dispatch() through the AsyncContext. Note however that we're not specifying the path. It should be the same as the requestUri of the HttpServletRequest, so as far as I can tell this a Tomcat bug.

I've asked on the mailing list to confirm.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 14, 2017

Rossen Stoyanchev commented

This is confirmed as a Tomcat issue. I've opened https://bz.apache.org/bugzilla/show_bug.cgi?id=61185.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 14, 2017

Izek Greenfield commented

@rstoyanchev Thank Rossen, I forget to mention i am using Jetty not tomcat.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 15, 2017

Yair Ogen commented

@Rossen Stoyanchev Aren't you missing the point where it was already identified that the root cause is internally created UrlPathHelper instances that don't enable shutting off the decoding from the outside?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 15, 2017

Rossen Stoyanchev commented

I debugged the issue with an actual sample (modifying the one provided above to correct typos). I analyzed the root case and logged a ticket against Tomcat. The Tomcat team looked at the issue and immediately acknowledged providing source code links that identify when the bug was introduced. So no I am not missing the point when it comes to this issue.

igreenfi I was hoping you might get around to creating it but here it is a ticket in Jetty to track and comment on:
eclipse/jetty.project#1618

As for creating instances of UrlPathHelper, that in itself is not an issue. There are places where it is completely legitimate so I would rather discuss in the context of a specific issue. Speaking more broadly for WebFlux we're making some very significant changes to the way the request path is treated, see #20207 and #20199. We're also then going to take advantage of some of those changes on the Spring MVC side as well (if you follow the sub-tasks on the referenced issues).

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 15, 2017

Rossen Stoyanchev commented

The Jetty issue was accepted as well it seems so we should see fixes soon.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 22, 2017

Rossen Stoyanchev commented

igreenfi, I have confirmed with my own little sample (based on your code snippet above) that the fix in Jetty 9.4.7 snapshot works. It would be more useful if you could confirm the fix in your application. It should work but you may have some extra processing once the String result is produced.

The Jetty snapshots repository is https://oss.sonatype.org/content/repositories/jetty-snapshots/ and the version you want to test with "9.4.7-SNAPSHOT". Feel free to comment directly on the Jetty ticket I created.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 5, 2017

Izek Greenfield commented

@rstoyanchev That snapshot solve the problem thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.