opencontainers / distribution-spec

OCI Distribution Specification
https://opencontainers.org
Apache License 2.0
792 stars 199 forks source link

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

Closed sudo-bmitch closed 8 months ago

sudo-bmitch commented 1 year ago

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 commented 1 year ago

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

sudo-bmitch commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 7 months ago

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

See:

  1. https://github.com/json-api/json-api/issues/1547
  2. https://github.com/expressjs/express/issues/3490
sudo-bmitch commented 7 months ago

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 commented 7 months ago

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

https://github.com/opencontainers/distribution-spec/blob/9317d9bca4f2f57355d875f562a2c9f688043961/conformance/03_discovery_test.go#L295

sudo-bmitch commented 7 months ago

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. 😄