[Bug] Regression in ATTACH parsing?

[kmurphy4]

Hi! I'm trying to upgrade from v6 to v7, and I noticed a change in behavior related to ATTACH parsing - I'm now seeing a vBinary component in a situation where I previously saw a vUri.

To reproduce

import icalendar

data = b"""
BEGIN:VCALENDAR
VERSION:2.0
BEGIN:VEVENT
DTSTAMP:19960704T120000Z
UID:uid1@example.com
ORGANIZER:mailto:jsmith@example.com
DTSTART:19960918T143000Z
DTEND:19960920T220000Z
STATUS:CONFIRMED
CATEGORIES:CONFERENCE
SUMMARY:Summary
DESCRIPTION:Description
ATTACH;VALUE=BINARY;ENCODING=BASE64;X-FILENAME=image.png:
 bm90IGEgcmVhbCBwbmcgYnV0IHNob3VsZG4ndCBtYXR0ZXIuLg==
END:VEVENT
END:VCALENDAR
"""

ical = icalendar.Calendar.from_ical(data)
print(ical)

attach = ical.subcomponents[0]["ATTACH"]
print(type(attach))

Output:

With icalendar==6.3.2 this produced

VCALENDAR({'VERSION': vText(b'2.0')}, VEVENT({'DTSTAMP': vDDDTypes(1996-07-04 12:00:00+00:00, Parameters({})), 'UID': vText(b'uid1@example.com'), 'ORGANIZER': vCalAddress('mailto:jsmith@example.com'), 'DTSTART': vDDDTypes(1996-09-18 14:30:00+00:00, Parameters({})), 'DTEND': vDDDTypes(1996-09-20 22:00:00+00:00, Parameters({})), 'STATUS': vText(b'CONFIRMED'), 'CATEGORIES': <icalendar.prop.vCategory object at 0x73ffd5abe4b0>, 'SUMMARY': vText(b'Summary'), 'DESCRIPTION': vText(b'Description'), 'ATTACH': 'bm90IGEgcmVhbCBwbmcgYnV0IHNob3VsZG4ndCBtYXR0ZXIuLg=='}))
<class 'icalendar.prop.vUri'>

but with icalendar==7.0.3 it produces

VCALENDAR({'VERSION': vText(b'2.0')}, VEVENT({'DTSTAMP': vDDDTypes(1996-07-04 12:00:00+00:00, Parameters({})), 'UID': vText(b'uid1@example.com'), 'ORGANIZER': vCalAddress('mailto:jsmith@example.com'), 'DTSTART': vDDDTypes(1996-09-18 14:30:00+00:00, Parameters({})), 'DTEND': vDDDTypes(1996-09-20 22:00:00+00:00, Parameters({})), 'STATUS': vText(b'CONFIRMED'), 'CATEGORIES': vCategory([vText(b'CONFERENCE')], params=Parameters({})), 'SUMMARY': vText(b'Summary'), 'DESCRIPTION': vText(b'Description'), 'ATTACH': vBinary(b'bm90IGEgcmVhbCBwbmcgYnV0IHNob3VsZG4ndCBtYXR0ZXIuLg==')}))
<class 'icalendar.prop.binary.vBinary'>

Environment

• Operating system: ubuntu 22.04
• Python version: 3.12.12
• icalendar version: 6.3.2 and 7.0.3

Additional context

• I tested it with the latest version. For example, I used the command pip3 install --upgrade https://github.com/collective/icalendar.git.
• I attached the .ics sample file example.ics.txt

[niccokunzmann]

Thanks for the clear report!

[kmurphy4]

Oh I forgot to mention - and you may have already figured this out - but I believe this issue was introduced in 2d4bd68.

[niccokunzmann]

Hi, that could be. Maybe we did prefer the default type over the VALUE parameter first. However, the VALUE states binary here. So, this should be ok. I recognise though that this is a very different way of handling it. So, we can solve this issue e.g. by adding compatibility code to vBinary that reduces the friction in dependent code. At least, we can document this in the upgrade guide.

What do you think about this? What do you think is the most useful solution for you?

[kmurphy4]

Hm, I'm still trying to figure out exactly how to proceed here - we can definitely switch our code to handling vBinary instead of vUri without much ado. (Yes, it would be nice to mention this in the upgrade guide.)

However, I'm noticing that there's another behavior difference between these two classes: vUri (v6.3.2) would call

icalendar/src/icalendar/prop.py

Lines 1601 to 1602 in 51f017e

	def __new__(cls, value, encoding=DEFAULT_ENCODING, params={}):
	value = to_unicode(value, encoding=encoding)

but vBinary (v7.0.3) calls

icalendar/src/icalendar/prop/binary.py

Line 21 in f513590

self.obj = to_unicode(obj)

with no encoding=. In our case, that means it tries and fails to use utf-8-sig decoding and then falls back to

icalendar/src/icalendar/parser_tools.py

Line 40 in 06768a6

return value.decode("utf-8-sig", "replace")

which silently corrupts/drops some of the data...

Should I make a new bug for this?

[kmurphy4]

Er, maybe the real difference is that vUri (v6.3.2) used raw, base64-encoded str as its value

icalendar/src/icalendar/prop.py

Lines 1611 to 1615 in 51f017e

	def from_ical(cls, ical):
	try:
	return cls(ical)
	except Exception:
	raise ValueError(f"Expected , got: {ical}")

the but now vBinary (v7.0.3) is adding an extra b64decode

icalendar/src/icalendar/prop/binary.py

Lines 33 to 37 in f513590

	def from_ical(ical: str | bytes) -> bytes:
	try:
	return base64.b64decode(ical)
	except ValueError as e:
	raise ValueError("Not valid base 64 encoding.") from e

so we're trying to "decode" twice (base64 => bytes then bytes => str). For binary data, though, it's not always going to be possible to do that second decode.

[kmurphy4]

Sorry for the noise - I think the double-decoding issue I'm running into is a dupe of #1289.

So I think my only remaining ask is this:

(Yes, it would be nice to mention this in the upgrade guide.)

😄

[niccokunzmann]

Thanks for the noise, it helps understand everything better. Yes, we have that bug #1289 open. If this is enough to solve your issue, we can just say that

• modify the upgrade guide to mention that vBinary is now used in places where we used other value types.
• mention how to properly use vBinary now
• mention this issue [Bug] Regression in ATTACH parsing? #1299
• include a code example of how to properly use vBinary

Would that work for you? Would you like to contribute? You are very welcome! You are probably more knowledgable on ATTACH than I am because you actually use vBinary, vUri and this attribute.

Also, if you want to leave a comment in #1289 about an example that needs to work in the proper way, i.e. the way that would be easy and right to use from your perspective, that would be good! I came to icalendar from calculating recurrences. I never use ATTACH, so your input on making the library most useful to everyone is most welcome.

What I tend to do is have a look which code I build around the properties and include edge cases and common use-cases into icalendar. ( That is the purpose of v7, make is easier to use correctly)
I.e. we can add Event.attachments as a property and make sure that the parameters can be accessed in a valuable way. An example of this way is Image

What are your thoughts on this?

See also:

• https://www.rfc-editor.org/rfc/rfc5545.html#section-3.8.1.1