Unnecessary substring in UTF-7 BOM list

[PalmtopTiger]

Hi. While analyzing the code, I discovered an unnecessary line in UTF-7 BOM substring list: b"\x2b\x2f\x76\x38\x2d"

ENCODING_MARKS: dict[str, bytes | list[bytes]] = {
	...
    "utf_7": [
        b"\x2b\x2f\x76\x38",
        b"\x2b\x2f\x76\x39",
        b"\x2b\x2f\x76\x2b",
        b"\x2b\x2f\x76\x2f",
        b"\x2b\x2f\x76\x38\x2d", # < this line
    ],
    ...
}

If you look at the code here

for mark in marks:
    if sequence.startswith(mark):
        return iana_encoding, mark

You can see that this list is processed in a loop, with the function exiting on the first startswith match. This means that if the first element of the list, b"\x2b\x2f\x76\x38", matches, the loop doesn't reach the last one, b"\x2b\x2f\x76\x38\x2d". If the first doesn't match, the last one doesn't match either, since it starts with the same four bytes. Therefore, the fifth substring does nothing but add an additional delay during execution.

Also, I don't understand where the fifth substring of five bytes came from. Wikipedia only mentions four-byte substrings:

UTF-7: 2B 2F 76
...
Followed by 38, 39, 2B, or 2F (ASCII 8, 9, + or /), depending on what the next character is.

[jayaddison]

...
> b"\x2b\x2f\x76\x38\x2d", # < this line
...

According to a 2012-11-25 contribution to Wikipedia's Byte Order Mark article, a contribution that remained in place until 2020-09-24, and so I think should be reliable, there is a special-case for an empty-string, in which the 38 hex suffix char is followed by a 2D hex char. This applies in the case of an empty string -- and that line represents it.

However: yes, given that the startswith conditional seems to be the only usage of this bytemark within the list, and that it'll never match given the self-prefix that precedes it, I think it should be fine to remove. Ref:

charset_normalizer/src/charset_normalizer/utils.py

Lines 291 to 299 in 5478b84

	for iana_encoding in ENCODING_MARKS:
	marks: bytes | list[bytes] = ENCODING_MARKS[iana_encoding]
	if isinstance(marks, bytes):
	marks = [marks]
	for mark in marks:
	if sequence.startswith(mark):
	return iana_encoding, mark

[jayaddison]

I think there's more to this than it may first appear.

In particular, UTF7 encoding of content that contains a standalone Byte-Order-Mark character is not yet handled correctly by charset_normalizer, as far as I can tell.

>>> from charset_normalizer.api import from_bytes
>>> import unicodedata
>>> char = unicodedata.lookup('BOM')
>>> det = from_bytes(char.encode('utf-7'))
>>> str(det.best())
'-'

(we should expect the result of that script to be an empty string, I think)

[PalmtopTiger]

I still don't understand the logic of checking for a 5-byte string if it starts with the same 4-byte string. It just adds an extra check, nothing more.

>>> from charset_normalizer import detect, constant

>>> [b.hex() for b in constant.ENCODING_MARKS['utf_7']]
['2b2f7638', '2b2f7639', '2b2f762b', '2b2f762f', '2b2f76382d']

>>> detect("\uFEFF".encode("utf-7"))
{'encoding': 'utf_7', 'language': '', 'confidence': 1.0}

>>> detect("\uFEFF\U0001F415".encode("utf-7"))
{'encoding': 'utf_7', 'language': '', 'confidence': 1.0}


>>> constant.ENCODING_MARKS['utf_7'].pop()
b'+/v8-'

>>> [b.hex() for b in constant.ENCODING_MARKS['utf_7']]
['2b2f7638', '2b2f7639', '2b2f762b', '2b2f762f']

>>> detect("\uFEFF".encode("utf-7"))
{'encoding': 'utf_7', 'language': '', 'confidence': 1.0}

>>> detect("\uFEFF\U0001F415".encode("utf-7"))
{'encoding': 'utf_7', 'language': '', 'confidence': 1.0}


>>> constant.ENCODING_MARKS['utf_7'].pop(0)
b'+/v8'

>>> [b.hex() for b in constant.ENCODING_MARKS['utf_7']]
['2b2f7639', '2b2f762b', '2b2f762f']

>>> detect("\uFEFF".encode("utf-7"))
{'encoding': 'utf-8', 'language': '', 'confidence': 0.8}

>>> detect("\uFEFF\U0001F415".encode("utf-7"))
{'encoding': 'utf_7', 'language': '', 'confidence': 1.0}

"\U0001F415" - the dog emoji.

[jayaddison]

The additional pattern can't make a difference to the encoding detection functionality -- but it could affect another feature: charset_normalizer can subtract the matched prefix-with-BOM bytepattern from the stream, allowing the data consumer to retrieve the content.

...except that the pattern we're discussing is at the end of the utf_7 list -- so it has never been subtracted from content streams, so far (and indeed has been entirely unused, as you mentioned to begin with). But it does have a possible use.

[jayaddison]

I'm now doubting whether an UTF7 stream that begins with the 2b 2f 76 38 2d pattern guarantees that the content is empty -- I think it's more likely that an empty content is one of many possible valid content-streams with that prefix. And if so, subtracting it from the start of the stream is likely incorrect, because we'll have trimmed a leading byte (2d) from the bytestream.

[PalmtopTiger]

I think the BOM should be trimmed after decoding, not before, because in Python it always turns into FEFF, whatever it is in the byte string.

[jayaddison]

I think the BOM should be trimmed after decoding, not before, because in Python it always turns into FEFF, whatever it is in the byte string.

I'd agree with that; I explored a few simple examples, and I think that the existing trimming logic in combination with the current BOM patterns can cause subsequent content to be decoded incorrectly -- I've filed #718 to describe that (apologies if it seems to duplicate/distract from this issue -- I think it is a slightly separate problem, though).

[jayaddison]

@PalmtopTiger this should be fixed now; the redundant substring has been removed by PR #730