LibWeb: Implement and use "isomorphic decoding"

[Gingeh]

To isomorphic decode a byte sequence input, return a string whose code point length is equal to input’s length and whose code points have the same values as the values of input’s bytes, in the same order.

This is essentially spec-speak for "Decode as ISO-8859-1 / Latin-1", but existing code interpreted it as "Transparently interpret as UTF-8" which is only correct for ASCII characters (< 0x80).

This change fixes a crash in http://wpt.live/mimesniff/mime-types/charset-parameter.window.html when parsing a non-ascii header value. The test now fully passes when combining these changes with #1879.

[Gingeh]

Marked as a draft while I figure out what's going on with https://wpt.live/mimesniff/mime-types/parsing.any.html

[Gingeh]

Testing for each change:

• Content-Type header parsing:
  • http://wpt.live/mimesniff/mime-types/charset-parameter.window.html
    • Used to crash on non ascii chars, now fully passes
  • https://wpt.live/mimesniff/mime-types/parsing.any.html
    • 258 new failures!
    • These tests both rely on and contradict the outdated File API spec (see w3c/FileAPI#43)
    • The regressed tests are all because we end up returning mojibake (e.g. ÿ becomes ÿ) from Blob.type, the spec (and chrome/firefox) says we should return an empty string for non-ascii mimetypes but the tests disagree and expect the original string to be returned (with the correct encoding).
    • I guess it's time to intentionally fail some wpt subtests.
• Refresh header usage:
  • Deno.serve((_req) => new Response("", {headers: {"Refresh": "1; https://example.org/ÿ/"}}));
  • Used to redirect to https://example.org/%EF%BF%BD/ now redirects to https://example.org/%C3%BF/ (same as accessing url directly)
• Base64 data: urls:
  • No real difference between previous and current behaviour because non-ascii chars are never valid in base64

[Gingeh]

Un-drafting this because I'm happy with my testing.
Our Blob.type implementation needs to be changed to return an empty string if the mimetype contains non-ascii characters, I'm not sure if that should be done now or in a follow-up pr.

[Gingeh]

Updates:

• New Infra::isomorphic_encode and Infra::isomorphic_decode helper functions to reduce code duplication (and because the Latin-1 encoder isn't actually isomorphic).
• Changed Header::from_string_pair to isomorphic encode its input strings because headers are expected to be ISO-8859-1 encoded.
  • This solves all test regressions in https://wpt.live/mimesniff/mime-types/parsing.any.html and passes an entirely new subtest.
• Renamed commit and PR to LibWeb: Implement and use "isomorphic decoding" because this is not using the Latin-1 en/decoder anymore.

[trflynn89]

I don't think this is the right thing to do - String is by definition UTF-8, but here we can be throwing any encoding into the string. The to_string_without_validation isn't a path to avoid that invariant, rather it's a performance optimization for when we already know the data going into String is UTF-8 (as validation isn't free).

We use ByteString to represent strings of arbitrary encodings.

[Gingeh]

Unless I'm misunderstanding what append_code_point does that's exactly what I'm doing. This function takes ISO-8859-1 encoded bytes and converts them to the corresponding UTF-8 encoding.

[Gingeh]

See also: Latin1Decoder::process and Decoder::to_utf8

[trflynn89]

Ah I see, I think I was misunderstanding what isomorphic decoding means. But yes, this looks right. Thanks :)

[F3n67u]

Changed Header::from_string_pair to isomorphic encode its input strings because headers are expected to be ISO-8859-1 encoded.

@Gingeh There are still several places creating Infrastructure::Header using aggregate initialization, which may cause Infrastructure::Header is not ISO-8859-1 encoded. Do we need to change them to using Infrastructure::Header::from_string_pair instead? Or do we need to forbidden aggregate initialization of Infrastructure::Header` to avoid this kind of bug?

Also, when using Infrastructure::Header's value or name, many of them still assume they are utf-8 encoded and didn't use "isomorphic decoding". I found a crash that was caused by this in #2814, but there are more places like that.

[Gingeh]

@F3n67u

There are still several places creating Infrastructure::Header using aggregate initialization, which may cause Infrastructure::Header is not ISO-8859-1 encoded. Do we need to change them to using Infrastructure::Header::from_string_pair instead? Or do we need to forbidden aggregate initialization of Infrastructure::Header` to avoid this kind of bug?

The safest option would be to disallow aggregate initialization and exclusively use Header::from_string_pair. There could be a slight optimization by skipping it when the input is either already encoded or statically known to be pure ASCII, but I'm not sure if that's worth the risk.

Maybe the aggregate initialization could be forbidden in favour of a Header::from_latin1_pair constructor which makes the expected encoding more explicit?

Also, when using Infrastructure::Header's value or name, many of them still assume they are utf-8 encoded and didn't use "isomorphic decoding". I found a crash that was caused by this in #2814, but there are more places like that.

I'm not surprised, a lot of code still assumes headers are ASCII. These cases will need to be found and fixed over time.

EDIT:
Another thing to be careful of is the fact that isomorphic encoding a value that has already been encoded will result in garbage data, I've just found several cases of this in Fetching.cpp

[F3n67u]

The safest option would be to disallow aggregate initialization and exclusively use Header::from_string_pair. There could be a slight optimization by skipping it when the input is either already encoded or statically known to be pure ASCII, but I'm not sure if that's worth the risk.
Maybe the aggregate initialization could be forbidden in favour of a Header::from_latin1_pair constructor which makes the expected encoding more explicit?

I agreed that we should disallow aggregate initialization to in favor of Header::from_string_pair and Header::from_latin1_pair!

EDIT:
Another thing to be careful of is the fact that isomorphic encoding a value that has already been encoded will result in garbage data, I've just found several cases of this in Fetching.cpp

Yes! It is causing https://wpt.live/fetch/api/basic/request-headers-nonascii.any.html failed. Header::from_latin1_pair would be handy when handling those cases.