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

Character encoding is reset when setting Content-Type #10349

Closed
hiji opened this issue Aug 21, 2023 · 3 comments · Fixed by #10358
Closed

Character encoding is reset when setting Content-Type #10349

hiji opened this issue Aug 21, 2023 · 3 comments · Fixed by #10358
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@hiji
Copy link

hiji commented Aug 21, 2023

Jetty version(s)

12.0.0

Jetty Environment

ee10

Java version/vendor (use: java -version)

openjdk version "17.0.8" 2023-07-18
OpenJDK Runtime Environment Temurin-17.0.8+7 (build 17.0.8+7)
OpenJDK 64-Bit Server VM Temurin-17.0.8+7 (build 17.0.8+7, mixed mode)

OS type/version

macOS 13.4.1

Description

If the character encoding of HTTP response (HttpServletResponse) is set by setCharacterEncoding method and Content-Type that does not include charset (e.g. text/html) is set, the character encoding set by setCharacterEncoding method will be used as the charset of Content-Type.

However, if you set same Content-Type (e.g. text/html) again, the character encoding set by setCharacterEncoding method will be reset and a different Content-Type will be set.

Checking the source code, onRemoveField is called when setting the set header again, but if the header is Content-Type, the character encoding is removed. (reference)

This is implemented in #9927 in 12.0.0.beta3, so it doesn't happen in 12.0.0.beta2 and earlier.

How to reproduce?

Perform the following processing in Servlet.

  1. set character encoding with response.setCharacterEncoding.
  2. set Content-Type that does not include charset with response.setContentType.
  3. perform same process again as step 2.

For example, the following code.

response.setCharacterEncoding(StandardCharsets.UTF_8.name());
response.setContentType("text/html"); // Content-Type after processing is "text/html;charset=utf-8"
response.setContentType("text/html"); // Content-Type after processing is "text/html;charset=iso-8859-1"
@hiji hiji added the Bug For general bugs on Jetty side label Aug 21, 2023
@gregw
Copy link
Contributor

gregw commented Aug 23, 2023

I've reproduced a problem, but after the second set I get "text/html;charset=null". Anyway, looking!

gregw added a commit that referenced this issue Aug 23, 2023
Fix #10349 which was caused by a put of ContentType being intercepted as a remove followed by an add.  The remove was incorrectly forgetting the charset without reference to the source of the charset.
@gregw gregw linked a pull request Aug 23, 2023 that will close this issue
gregw added a commit that referenced this issue Aug 23, 2023
Fix #10349 which was caused by a put of ContentType being intercepted as a remove followed by an add.  The remove was incorrectly forgetting the charset without reference to the source of the charset.
@hiji
Copy link
Author

hiji commented Aug 23, 2023

@gregw
Thank you for confirming.
After checking again, the charset after calling setContentType method was definitely null.
In environment I confirmed, Result was output using getWriter method after that, but getCharacterEncoding method was called in getWriter method, and default character encoding iso-8859-1 was set.
(that code is here)

So the correct state is as follows.

response.setCharacterEncoding(StandardCharsets.UTF_8.name());
response.setContentType("text/html"); // Content-Type after processing is "text/html;charset=utf-8"
response.setContentType("text/html"); // Content-Type after processing is "text/html;charset=null"

@gregw
Copy link
Contributor

gregw commented Aug 24, 2023

This has been fixed by #10358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants