THREESCALE-11063: Fix parsing response if Content-Type contains charset

[mayorova]

Fixing the issue: https://issues.redhat.com/browse/THREESCALE-11063

It was failing previously with:

AbstractAdapter::OIDC::AuthenticationError: Unknown Content-Type

[akostadinov]

I think that if we support charset=UTF-8, we also need to #force_encoding.

[mayorova]

I think that if we support charset=UTF-8, we also need to #force_encoding.

I opened https://issues.redhat.com/browse/THREESCALE-11073 for dealing with it, as I think we need to think a bit more about the best way to approach it.

[akostadinov]

I assume you checked utf-8 is how rails parses the string by default :)

[mayorova]

I think json does that indeed, while I'm not 100% sure that's the path: https://github.com/flori/json/blob/v2.6.1/lib/json/pure/parser.rb#L185-L187

[akostadinov]

I think that strings are converted to UTF8 from whatever they originally were, they are not read as UTF-8 unconditionally. Seems like in the part you link to, they have generated an ASCII-8BIT string that they want now to treat as UTF8 (because it was generated to be valid UTF-8). See

https://github.com/flori/json/blob/v2.6.1/lib/json/pure/parser.rb#L80

So the question is whether body is loaded with the supplied charset= property or is always treated as ASCII 8bit or is always treated as UTF-8.

My understanding is that if we see charset in the content type to be something, we need to treat that string as the specified charset. It would be nice if rack did that automatically but does it do it?

[akostadinov]

Some more information about the charset.

https://medium.com/@lefloh/when-to-use-a-charset-attribute-for-a-content-type-header-ffeb5838d3c3

json content type is supposed to default to utf-8. In this case I expect things to work with UTF-8. Interesting to know whether Rails automatically uses that information. I'll try to test that.

[akostadinov]

I see that this has nothing to do with Rails. We are using httpclient to make requests to the IDP. And it does take into account the charset parameter. And interprets body according to it.

So excuse me for the trouble 🐛

I think we can just go ahead with your original commit, no need to process the charset by ourselves.

$ cat /tmp/index.http 
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-16
Content-Length: 7
Server: netcat!

asdfgh

$ nc -l 8000 < /tmp/index.http # in a separate terminal

$ bundle exec httpclient
irb(HTTPClient):023:0> resp = get 'http://localhost:8000'
=> 
#<HTTP::Message:0x00007f296c299388
...
irb(HTTPClient):024:0> resp.content_type
=> "text/html; charset=UTF-16"
irb(HTTPClient):025:0> resp.body
=> "\x61\x73\x64\x66\x67\x68\n"
irb(HTTPClient):026:0> resp.body.encoding
=> #<Encoding:UTF-16 (dummy)>

If charset is UTF-8 then httpclient sets UTF-8 for the body. So I believe we are fine.

And while on it - yes, the body encoding for application/json is UTF-8 if no charset is specified.

[mayorova]

That's all great info @akostadinov Thank you for digging into it and providing the verification steps 👏

[mayorova]

Just a note: httpclient implements this feature here nahi/httpclient@e5efea5

ans fixes this issue nahi/httpclient#26

[mayorova]

Doing JSON.parse(body.force_encoding('utf-8')) here makes test complicated, because it throws "can't modify frozen string".

[akostadinov]

I think though, we may want to use start_with? instead of match?

[mayorova]

@akostadinov I reverted the 2nd commit, does it look good now? :)

[akostadinov]

If this code suggestion works, it would be much preferable. Otherwise we should be good with match?. Thank you!

[akostadinov]

Suggested change

	if Mime::Type.lookup(content_type).match? JSON_TYPE
	if Mime::Type.lookup(content_type).start_with? JSON_TYPE

[akostadinov]

If this code suggestion works, it would be much preferable. Otherwise we should be good with match?. Thank you!

[mayorova]

If this code suggestion works, it would be much preferable. Otherwise we should be good with match?. Thank you!

Well, they are not strings, they are instances of Mime::Type, and this match? is "special": https://github.com/rails/rails/blob/v7.0.8.1/actionpack/lib/action_dispatch/http/mime_type.rb#L287-L291
It takes into account some "synonyms", which I don't know what it is, and we probably don't need it. Potentially, we could either 1. apply to_s.downcase to both and match with start_with? as you suggest, or 2. drop he whole Mime::Type.lookup thing and work with plain strings.

[akostadinov]

Then keep it as is. Fine with me, thank you!