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

Are additional parameters allowed on Content-Type headers? #408

Closed
sudo-bmitch opened this issue May 4, 2023 · 9 comments · Fixed by #469
Closed

Are additional parameters allowed on Content-Type headers? #408

sudo-bmitch opened this issue May 4, 2023 · 9 comments · Fixed by #469
Milestone

Comments

@sudo-bmitch
Copy link
Contributor

sudo-bmitch commented May 4, 2023

RFC1521 RFC2045 describes the parsing of the Content-Type field, including optional parameters after the base media type. E.g. following the RFC, a registry could output:

Content-Type: application/vnd.oci.image.index.v1+json; charset=utf-8

Should this be explicitly allowed in the distribution-spec? Currently we say:

If the manifest has a mediaType field, clients SHOULD reject unless the mediaType field's value matches the type specified by the Content-Type header.

@brackendawson
Copy link
Contributor

Does "the type specified by the Content-Type header" not already exclude parameters?

@sudo-bmitch
Copy link
Contributor Author

sudo-bmitch commented May 5, 2023

I could see a debate that it's valid since the base value of the media type does match. But given testing of various tools, I worry there's a lot of code out there that's looking for an exact match, lower case, and no parameters.

Because of that, my preference is to say tools SHOULD/MUST generate a Content-Type without parameters, and lower case. If we don't already, that would be good to check in the conformance tests. We can also note that tools consuming the field MAY normalize the media type according to the RFC.

@tianon
Copy link
Member

tianon commented May 5, 2023

This REQUIRED property contains the media type of the referenced content. Values MUST comply with RFC 6838, including the naming requirements in its section 4.2.

(proxying for @jonjohnsonjr - ie, we already have this exclusion)

@sudo-bmitch
Copy link
Contributor Author

This REQUIRED property contains the media type of the referenced content. Values MUST comply with RFC 6838, including the naming requirements in its section 4.2.

(proxying for @jonjohnsonjr - ie, we already have this exclusion)

I'm not sure I follow. That text covers the descriptor's mediaType value, where I'm looking at the Content-Type header.

@brackendawson
Copy link
Contributor

I worry there's a lot of code out there that's looking for an exact match, lower case, and no parameters.

I'm pretty sure distribution is one of these for Accept headers. Which is nice and inconsistent because its own errors have a Content-Type of application/json; charset=utf-8.

My preference would be for all tools to do content negotiation properly. You can for sure offer explicit Content-Type/Accept values in lower case without params if you want to be defensive, but I think we'd be better off if we all parse Content-Type/Accept values according to the existing specs.

@rbirkby
Copy link

rbirkby commented Dec 10, 2023

Frameworks such as ExpressJS actively prevent this from being implemented for security reasons.

See:

  1. Spec says no media type parameters, but this creates a potential vulnerable json-api/json-api#1547
  2. No way to disable charset in Content-Type header? expressjs/express#3490

@sudo-bmitch
Copy link
Contributor Author

The term we used was "SHOULD" so a tool that doesn't follow it isn't in violation of the spec (that would require a MUST), but may encounter compatibility issues with other tools (a significant number of tools do a direct string comparison without parsing the contents of the field). Changing the language of the spec would not change the existing tooling.

@rbirkby
Copy link

rbirkby commented Dec 11, 2023

As a SHOULD not MUST, why does the conformance suite validate that the content-type excludes parameters?

Expect(resp.Header().Get("Content-Type")).To(Equal("application/vnd.oci.image.index.v1+json"))

@sudo-bmitch
Copy link
Contributor Author

That's worth opening an issue / PR. The test was written before the "SHOULD" language was added. It would be good to use a warning for anything that violates a SHOULD.

We can list this as one of the examples of things that do a direct string comparison. 😄

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

Successfully merging a pull request may close this issue.

4 participants