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

Ability to differentiate different causes for WebInputException #28142

Closed
ascopes opened this issue Mar 7, 2022 · 12 comments
Closed

Ability to differentiate different causes for WebInputException #28142

ascopes opened this issue Mar 7, 2022 · 12 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@ascopes
Copy link
Contributor

ascopes commented Mar 7, 2022

This is a request for the ability to generate a message stating that a WebFlux controller request body (or other request component) is missing, without exposing potentially sensitive implementation details in the error message. By doing this, it would remove the need for implementing a custom handler for validation where Spring provides messages that may fail organisational governance.

If we write a controller such as the following:

@RestController
@Validated
public class TestController {
  @PostMapping("/foo/{bar}")
  @ResponseStatus(ACCEPTED)
  public Mono<Void> fooBar(@PathVariable String bar, @RequestBody Body body) {
    ...
  }
}

...the implementation for a missing request body appears to be currently defined in AbstractMessageReaderArgumentResolver:

	private ServerWebInputException handleMissingBody(MethodParameter parameter) {
		String paramInfo = parameter.getExecutable().toGenericString();
		return new ServerWebInputException("Request body is missing: " + paramInfo, parameter);
	}

The error message here cannot be presented to the consumer of the API on systems where governance prevents the exposure of the underlying implementation technology (this would fail penetration testing, for example). The reason for this is that the error message with the concatenated parameter info will render to something like:

Request body is missing: public reactor.core.publisher.Mono<java.lang.Void> com.mycompany.TestController.fooBar(java.lang.String, com.mycompany.Body)

This can potentially expose information allowing the inference of the underlying JDK or Spring Boot version by observing the names of the parameters and method in the underlying error, which may enable malicious actors to "comb" an API and determine if certain exploits may exist.

The issue arises that while we can override this exception with a custom exception handler, this then involves writing a significant amount of boilerplate to cater for parsing every potential binding annotation, and then extracting information such as the name of the parameter (in the case of headers), or the request body. The current API does not provide a simple way of determining that the request body is the part that is missing in this case.

If we instead use Spring Validation, and use @RequestBody(required = false) @Valid @NotNull, the issue will instead arise that a ConstraintViolationException is raised instead of a WebExchangeBindingException, which then requires two almost identical sets of exception handlers to achieve the same consistent validation error handling. This appears to be due to validate() not being called on the body in AbstractMessageReaderArgumentResolver.java when the body is determined to be missing.

If we omit the message altogether, it leaves a response to the consumer that is not helpful for diagnosing the issue with the request. For example:

{
  "status": 400,
  "message": "null"
}

Is there a workaround for this, or is this something that could be potentially tweaked? Providing consistent validation using the Spring Validation API with WebFlux is somewhat painful at the moment because of this, and it leads to code that can be somewhat difficult to maintain when it caters for multiple edge cases.

An ideal scenario would be where we could obtain a string "type" of the parameter in question that failed validation (e.g. "header", "body", "pathVariable", etc, and then a simplified message such as "Request body is required" that can be presented back to the consumer of the API.

Any help or suggestions would be greatly appreciated!

Thanks

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 7, 2022
@ascopes
Copy link
Contributor Author

ascopes commented Mar 22, 2022

Can confirm that this also occurs in UnsupportedMediaTypeStatusException as well, where the implementation package name is leaked in the error reason.

415 UNSUPPORTED_MEDIA_TYPE "Content type 'application/x-yaml' not supported for bodyType=org.example.MyModelName

@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Apr 25, 2022
@rstoyanchev rstoyanchev added this to the Triage Queue milestone Apr 25, 2022
@rstoyanchev rstoyanchev self-assigned this Apr 25, 2022
@askar882
Copy link

I was also trying to find a way to tell the client that the request body missing without exposing much information. And I was able to make this work with a custom exception handler as follows.

@RestControllerAdvice
public class ExceptionHandler {
    @ResponseStatus(HttpStatus.BAD_REQUEST)
    @ExceptionHandler(HttpMessageNotReadableException.class)
    public Map<String, Object> requestBodyMissing(HttpServletRequest request) {
        HandlerMethod method = (HandlerMethod) request.getAttribute("org.springframework.web.servlet.HandlerMapping.bestMatchingHandler");
        String requestBody = Arrays.stream(method.getMethodParameters())
                .map(m -> m.getParameterType().getSimpleName() + " " + m.getParameterName())
                .collect(Collectors.joining(","));
        return Arrays.stream(new Object[][] {
                {"status", 400},
                {"message", "Required request body is missing: " + requestBody}
        }).collect(Collectors.toMap(o -> (String) o[0], o-> o[1]));
    }
}

When a client sends a request without a required body, it will receive something like:

{
    "status": 400,
    "message": "Required request body is missing: String name"
}

@rstoyanchev
Copy link
Contributor

@ascopes thanks for creating this issue.

ServerWebInputException is indeed raised for different kinds of missing input, which is reflected in the message, but no easy way for an exception handler to tell what's missing and customize the message. I will experiment with adding a sub-class for MissingRequestValueException that exposes the name and type of the missing value where the type is just a string, e.g. "request header", "cookie value", etc. Also add a sub-class for type mismatch issues (conversion) and request body decoding issues. This should cover most cases.

For validation, I see your point about ConstraintViolation with a class-level @Validated which is handled with an AOP interceptor (independent of Spring WebFlux) vs putting the same on @RequestBody which is handled in WebFlux and the ConstraintViolation is translated to Errors in SpringValidatorAdapter. We might be able to make that more easily accessible so the same adaptation can be performed from an exception handler.

Note also that for 6.0 we are making wider changes with #27052 to support problem details. This will provide more support for applications to set the response body directly from an @ExceptionHandler through the ProblemDetail type.

@rstoyanchev
Copy link
Contributor

@askar882 your snippet is for Spring MVC but this issue is for WebFlux. In Spring MVC there is a wider hierarchy of exceptions that make it possible to customize on a case by case basis.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 4, 2022
@rstoyanchev rstoyanchev modified the milestones: Triage Queue, 6.0.0-M4 May 4, 2022
@ascopes
Copy link
Contributor Author

ascopes commented May 4, 2022

@rstoyanchev that sounds great! Look forward to seeing what comes of this, thanks for taking a look.

@rstoyanchev rstoyanchev changed the title Ability to get WebFlux missing request body without internal detail Ability to differentiate different causes for WebInputException May 9, 2022
@rstoyanchev
Copy link
Contributor

I've added MissingRequestValueException which covers cases of missing "named" values (headers, cookies, request params, etc) and UnsatisfiedRequestParameterException as subclasses of WebInputException that also expose properties with details about the exception. For request body, I've adjusted it to raise WebInputException with a nested DecoderException which should help to signal a problem with the request body.

@dpratsun
Copy link

@rstoyanchev could you please tell at which version of Spring Boot MissingRequestValueException can be used?

@wimdeblauwe
Copy link

What is the difference between org.springframework.web.server.MissingRequestValueException which was added to Spring 6 and org.springframework.web.bind.MissingRequestValueException which is already in Spring 5?

@rstoyanchev
Copy link
Contributor

@wimdeblauwe, one depends on the Servlet API and is used in Spring MVC. The other is part of the ResponseStatusException hierarchy that's used in WebFlux.

@dpratsun, it's available as of 6.0.0-M4.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 5, 2022

@ascopes, please take a look at #28814, which may further help with concerns here, especially as it relates to "governance prevents the exposure of the underlying implementation technology " and the need to customize Spring MVC / WebFlux error messages.

@ascopes
Copy link
Contributor Author

ascopes commented Oct 5, 2022

Looks good, thanks for taking this into consideration!

@skaba
Copy link

skaba commented Oct 24, 2022

Will this be backported to Spring 5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants