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

Header content-type not enforcing charset #26610

Closed
ghost opened this issue Feb 25, 2021 · 6 comments
Closed

Header content-type not enforcing charset #26610

ghost opened this issue Feb 25, 2021 · 6 comments
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket)

Comments

@ghost
Copy link

ghost commented Feb 25, 2021

Hi,

after upgrading from Spring boot 2.3.1.RELEASE to 2.4.2 it seems the value for the content-type header isn't fully validated.

  • openjdk version "11.0.9.1" 2020-11-04

  • Kotlin version 1.4.30

Example:

@RestController
@RequestMapping(
    produces = ["application/vnd.api+json;charset=utf-8"],
    consumes = ["application/vnd.api+json;charset=utf-8"]
)
class Controller(...) {
@ResponseStatus(HttpStatus.CREATED)
    @PostMapping("/some_path")
    fun doSomething(
        @Valid @RequestBody request: JsonApiRequest<Clazz1>
    ): Clazz2 {
}

Behavior on Spring 2.3.1-RELEASE:
Request with header Content-Type : application/vnd.api+json;charset=utf-8 is allowed ✅
Request with header Content-Type : application/vnd.api+json;charset=utf-16 rejected with 415 status ✅
Request with header Content-Type : application/vnd.api+json;charset=testrejected with 415 status ✅

Behavior on Spring 2.4.2:
Request with header Content-Type : application/vnd.api+json;charset=utf-8 is allowed ✅
Request with header Content-Type : application/vnd.api+json;charset=utf-16 isn't rejected with 415 status and results in an expcetion being thrown ❌

org.springframework.http.converter.HttpMessageNotReadableException: JSON parse error: Unrecognized token '笊': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false'); nested exception is com.fasterxml.jackson.core.JsonParseException: Unrecognized token '笊': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: (InputStreamReader); line: 1, column: 2]

Request with header Content-Type : application/vnd.api+json;charset=testrejected with 415 status ✅

Additionall info:
Request with header Content-Type : application/vnd.api+json (without specifying the charset) get's allowed (I'm assuming it's defaulting to utf-8) with both versions. I would expect the request to be rejected in this case.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 25, 2021
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 1, 2021

The produces condition does match any explicitly listed media parameters and values. However the consumes condition however doesn't. The reason UTF-16 is rejected in Boot 5.3.1 is because in Spring Framework 5.2.7, due to #25076, we briefly enforced checks in MappingJackson2HttpMessageConverter to see if Jackson supports the specified charset. That change caused issues for existing applications when reading non-Unicode content, see #25247, and in 5.2.8 the converter was changed again to no longer check the charset for reading but instead switch to using InputStreamReader with the specified charset.

First question is whether we still need some sort of improvement to the logic introduced in 5.2.7 and 5.2.8. Maybe the charset should be checked after all and for example UTF-16 filtered out in canRead. What do you think @poutsma?

Second, that is still not the same as the having the consumes condition enforce a match of explicitly listed media type parameters. In 5.2.7, UTF-16 happened to be filtered out as not supported. However if you pass UTF-16BE which is supported that passes and works fine even in 5.2.7. Sp @sasa-fajkovic do you really mean to only allow reading UTF-8 and why not allow other formats as input as long as they are supported and do work?

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Mar 1, 2021
@poutsma
Copy link
Contributor

poutsma commented Mar 2, 2021

In addition to what @rstoyanchev wrote, I am finding it difficult to reproduce the HttpMessageNotReadableException. When I post UTF-16 JSON data, it works as expected.

The only way I can reproduce the issue is by sending plain ASCII/UTF-8 data, and pretending it's UTF-16 in the Content-Type. For instance by using a curl command like:

curl -X POST http://localhost:8080/some_path -v -H "Content-Type: application/vnd.api+json;charset=utf-16" --data '{"foo":"bar"}'

However, I would say that this is expected behavior.

If you'd like us to spend some more time investigating, please take the time to provide a complete minimal sample (something that we can unzip or git clone, build, and deploy) that reproduces the problem.

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@poutsma @rstoyanchev I'll try to find some free time in the next day or two to create a demo project reproducing this behavior.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 2, 2021
@rstoyanchev
Copy link
Contributor

@sada-sigsci the quick summary is that what you saw in 5.2.7 was temporary behavior. The current (and correct) behavior as of 5.2.8 is that there shouldn't be a 415. Reading should just work as long as the charset is valid and supported. If you get a parse error, most likely it means the content submitted by the client wasn't encoded according to the charset specified in the media type. Related to that there is now an improvement with #26627 to rely on Jackson to auto-detect the charset even if the one on the media type doesn't match.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Mar 2, 2021
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Mar 9, 2021
@spring-projects-issues
Copy link
Collaborator

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 16, 2021
This issue is being transferred. Timeline may not be complete until it finishes.
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)
Projects
None yet
Development

No branches or pull requests

3 participants