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

Possible issue with handling http method OPTIONS [SPR-14410] #18981

Closed
spring-projects-issues opened this issue Jun 28, 2016 · 7 comments
Closed

Possible issue with handling http method OPTIONS [SPR-14410] #18981

spring-projects-issues opened this issue Jun 28, 2016 · 7 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jun 28, 2016

Ionut David opened SPR-14410 and commented

I currently upgraded spring framework to 4.3 from 4.2.5 and all of the integration tests that use OPTIONS method are failing.
For the test we use RestAssured and the scenario is like this:

given().
    	auth().basic(some_user_Name, some_password).
    	when().options("some_base_path").
    	then().statusCode(200).and().header(....)

I see that the way the http request method matching is changed and specially for method OPTIONS.

Code snippet from RequestMethodsRequestCondition:

public RequestMethodsRequestCondition getMatchingCondition(HttpServletRequest request) {
		if (CorsUtils.isPreFlightRequest(request)) {
			return matchPreFlight(request);
		}
		if (getMethods().isEmpty()) {
			if (RequestMethod.OPTIONS.name().equals(request.getMethod())) {
				return null; // No implicit match for OPTIONS (we handle it)
			}
			return this;
		}
		return matchRequestMethod(request.getMethod());
	}

but in RequestMappingInfo.getMatchingCondition sinse the getMatchingCondition for http methods return null my url won't be matched.

public RequestMappingInfo getMatchingCondition(HttpServletRequest request) {
RequestMethodsRequestCondition methods = this.methodsCondition.getMatchingCondition(request);
....
if (methods == null || params == null || headers == null || consumes == null || produces == null) {
			return null;
		}
...
}

Thanks in advance for the help.


Affects: 4.3 GA

Referenced from: commits 9c29ed7

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 29, 2016

Rossen Stoyanchev commented

We return null only if the controller method is not explicitly mapped to any HTTP method. Later on under RequestMapingInfoHandlerMapping#handleNoMatch there is automatic handling for HTTP OPTIONS. So this is all by design and it should just work.

Please provide more information before we can discuss a solution. What is the actual response? What does the controller method declaration look like? What was responsible for handling HTTP OPTIONS before, if anything at all (it's possible it was default HttpServlet handling listing all methods) ?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 30, 2016

Ionut David commented

We are using the Milton Webdav library to retrieve calendars and contacts and this library will first attempt to retrieve the information without authentication which of course will fail, because we have some authentication filters, and the second attempt uses authentication and if it's ok it will retrieve the info. We are also using spring-boot-1.3.3 and we implemented a custom error controller:

@RestController
public class FilterExceptionHandler implements ErrorController {
 @RequestMapping(value = "${error.path:/error}")
    public ResponseEntity<BaseResponse> handleException(HttpServletRequest request) {
    ....
   return ResponseEntity.status(errorCode).body(errorResponse);
    }
}

So when I run this scenario

given().
    	auth().basic(some_user_Name, some_password).
    	when().options("/myApp/caldav/principals/MockUser/calendars/address_book/").
    	then().statusCode(200).and().header(....)

The first attempt will be without authentication so it should be redirected to the error handler. When I debug the code I end up in RequestMappingInfo.getMatchingCondition but the /error url is not matched because of the OPTIONS http method, later in the debug the RequestMapingInfoHandlerMapping#handleNoMatch is invoked but from what I see in case of OPTIONS a new HandlerMethod is created instead of using my handler for url '/error'. See the code snipped bellow.

if (HttpMethod.OPTIONS.matches(request.getMethod())) {
				HttpOptionsHandler handler = new HttpOptionsHandler(allowedMethods);
				return new HandlerMethod(handler, HTTP_OPTIONS_HANDLE_METHOD);
			}

In the end it fails with a 500 error instead of invoking my custom error handler.

Also in the debug mod I could see in the mapping registry the correct handler for the /error url but it's not used.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 30, 2016

Rossen Stoyanchev commented

Okay thanks for the details. What you see in handleNoMatch is the automated handling of HTTP OPTIONS and if you look at the HTTP_OPTIONS_HANDLE_METHOD, that results in a 200 response lists allowed HTTP methods based on how the controller is mapped, which is what you want under "normal" circumstances.

In this case an exception has bubbled up to the Servlet container level and is mapped so an error dispatch is made and handled by the ErrorController. Clearly for an error dispatch, automated HTTP OPTIONS handling, and business as usual, doesn't makes sense and there won't be a 200 response. So I think we need to update HTTP OPTIONS handling in RequestMethodsRequestCondition for error dispatches. I'll have something to try later today.

1 similar comment
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 30, 2016

Rossen Stoyanchev commented

Okay thanks for the details. What you see in handleNoMatch is the automated handling of HTTP OPTIONS and if you look at the HTTP_OPTIONS_HANDLE_METHOD, that results in a 200 response lists allowed HTTP methods based on how the controller is mapped, which is what you want under "normal" circumstances.

In this case an exception has bubbled up to the Servlet container level and is mapped so an error dispatch is made and handled by the ErrorController. Clearly for an error dispatch, automated HTTP OPTIONS handling, and business as usual, doesn't makes sense and there won't be a 200 response. So I think we need to update HTTP OPTIONS handling in RequestMethodsRequestCondition for error dispatches. I'll have something to try later today.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 30, 2016

Rossen Stoyanchev commented

There is a fix now available. If you could please give it a try with 4.3.1.BUILD-SNAPSHOT.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 1, 2016

Ionut David commented

I added the 4.3.1.BUILD-SNAPSHOT dependency and the fix you provided worked, all the test are green.
Thanks for the help.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 1, 2016

Rossen Stoyanchev commented

Great, thanks so much for checking and reporting the issue.

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