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

Risk of having duplicated content-type in response headers #5949

Open
Lincong opened this issue Oct 18, 2024 · 3 comments
Open

Risk of having duplicated content-type in response headers #5949

Lincong opened this issue Oct 18, 2024 · 3 comments
Labels
Milestone

Comments

@Lincong
Copy link

Lincong commented Oct 18, 2024

We are on Armeria 1.16.3 and we noticed that the below code pattern can yield a HTTP response with 2 entries keyed by content-type:

import com.linecorp.armeria.common.HttpResponse

val responseMsg = "Hi"
val responseBuilder = HttpResponse.builder()
responseBuilder.status(200)
responseBuilder.content(responseMsg) // This adds a response header `content-type: text/plain; charset=utf-8`
responseBuilder.header("content-type", "application/json")
responseBuilder.build()

One the client side, the received HTTP response headers contain:

... 
content-type: text/plain; charset=utf-8
content-type: application/json

It may confuse the client and lead to failures. I wonder if it is worth changing the semantics of HttpResponseBuilder API to reduce the risk of having duplicated content-type entries. For example, we can make the last specified content-type win. I hope to learn what you think, thanks :)

@jrhee17
Copy link
Contributor

jrhee17 commented Oct 18, 2024

Hi, I think I agree with you

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

ref: https://www.rfc-editor.org/rfc/rfc7230#section-3.2.2

And it seems like content-type is not a comma-separated list

Content-Type = media-type
media-type = type "/" subtype *( OWS ";" OWS parameter )
type = token
subtype = token

ref: https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5

I wasn't able to find any practical use-cases not defined by the rfc from a simple google search either.
Having said this, let me wait ask the other maintainers as well since I may be missing something.

@jrhee17 jrhee17 added this to the 1.31.0 milestone Oct 18, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Oct 18, 2024

responseBuilder.content(responseMsg) // This adds a response header content-type: text/plain; charset=utf-8

#5020
I think other maintainers have the same opinion that we need to fix not to set the default content-type or lazily set on build() method if missing.

@ikhoon ikhoon added the defect label Oct 18, 2024
@jrhee17
Copy link
Contributor

jrhee17 commented Oct 21, 2024

Talked with the others, instead of enforcing the validation of a single content-type in ResponseHeaders, it's probably enough to not set a content-type when calling content.

How we can do this without introducing breaking changes may need further discussion though.

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

No branches or pull requests

3 participants