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

mediatypes: detect utf-8 encoded JSON manifest #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

steveej
Copy link
Contributor

@steveej steveej commented Sep 5, 2019

This is returned by the Satellite registry.

@steveej steveej requested a review from lucab September 5, 2019 10:25
@lucab
Copy link
Member

lucab commented Sep 5, 2019

This is a bug in Satellite, as application/json is a binary format and thus doesn't allow a textual charset.
This was already discussed, covered and fixed in opencontainers/distribution-spec#41.

While I don't have problems landing this PR as a temporary fix, I would really like to first have a ticket to track on Satellite side to fix it (and some timebox to remove this hack).

@steveej
Copy link
Contributor Author

steveej commented Sep 5, 2019

I filed a BZ ticket for this: https://bugzilla.redhat.com/show_bug.cgi?id=1749317

This is returned by the Satellite registry.
@steveej steveej force-pushed the pr/mediatype-detect-utf8-applicationjson branch from fbe80da to 9d33035 Compare September 5, 2019 13:13
@steveej
Copy link
Contributor Author

steveej commented Sep 5, 2019

I think I misunderstood your comment. Do I understand correctly now that the content-type is actually contradicting in itself?

@steveej steveej changed the title mediatyeps: detect utf-8 encoded JSON manifest mediatypes: detect utf-8 encoded JSON manifest Sep 5, 2019
@lucab
Copy link
Member

lucab commented Sep 6, 2019

Do I understand correctly now that the content-type is actually contradicting in itself?

Yes. application/json takes no additional parameters, as per RFC.

That said, I'm fine with either landing this band-aid or do #83 and relax the parser, as long Satellite gets fixed at some point.

edwardgeorge added a commit to edwardgeorge/dkregistry-rs that referenced this pull request Sep 9, 2020
If getting a 404 response when calling 'has_manifest' then if the content-type
returned for the json error body is 'application/json; charset=utf-8'
then the call to 'evaluate_media_type' will fail because it does not expect
charset=utf-8 to be valid here. see discussion in:
camallo#113

This change follows the approach taken in other methods above which is to
match on 'status' before continuing.
edwardgeorge added a commit to edwardgeorge/dkregistry-rs that referenced this pull request Sep 11, 2020
If getting a 404 response when calling 'has_manifest' then if the content-type
returned for the json error body is 'application/json; charset=utf-8'
then the call to 'evaluate_media_type' will fail because it does not expect
charset=utf-8 to be valid here. see discussion in:
camallo#113

This change follows the approach taken in other methods above which is to
match on 'status' before continuing.
@lucab lucab removed their request for review February 21, 2024 11:15
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 this pull request may close these issues.

2 participants