Repository: pimutils/vdirsyncer

Issue:
Invalid ICS files (missing `END:VEVENT`s and `END:VCALENDAR`) are silently ignored
## Problem

When syncing from a remote ICS file via the http storage type, vdirsyncer does not report a parsing error when the ICS file is missing `END:VEVENT` and `END:VCALENDAR` lines. Instead, it proceeds as if the ICS file were valid but contained no events. Although the invalid ICS file clearly needs to be fixed by whoever is producing it, I think that vdirsyncer should also let users know when a file is invalid.

## Background

I recently ran into problems with vdirsyncer reporting that one of my calendars was empty (`error: Storage "calendar_remote" was completely emptied.`). This calendar is simply pulling from a published Outlook calendar (a `.ics` file) using the http storage type. I know that the calendar is not empty, and looking at it via Outlook's web UI comfirmed this.

After a bit of digging, I discovered that Outlook is publishing bad ICS files -- all events except the last are missing the `END:VEVENT`, and the file does not have an `END:VCALENDAR`. This issue has been reported to Microsoft and will hopefully be fixed soon: https://answers.microsoft.com/en-us/outlook_com/forum/all/ics-files-are-missing-end-nodes/fb2e1cc7-eed5-43e2-a62f-bebc653e119f.

I think there remains an issue in vdirsyncer, though -- it should report a parsing error when it encounters this invalid ICS file. Reporting these errors would make troubleshooting simpler when something goes wrong.

Further, if the local calendar has no events, vdirsyncer reports no error at all but fails to sync the events from the remote calendar. The test setup below reproduces this issue. Once again, vdirsyncer should be reporting a parsing error to the user, not reporting that the sync completed successfully.

## Minimal setup to reproduce

**vdirsyncer version**: 0.19.2

**Python version**: 3.11.6

**OS**: Arch Linux

**Config**:
```
[general]
status_path = "~/.config/vdirsyncer/status/"

[pair test_cal]
a = "test_cal_remote"
b = "test_cal_local"
collections = null

[storage test_cal_local]
type = "filesystem"
path = "~/scratch/test-cal/"
fileext = ".ics"

[storage test_cal_remote]
type = "http"
url = "http://localhost:8080/cal.ics"
```

**Steps to reproduce**:

1. Create the `~/scratch/test-cal` directory.
2. Copy the *`cal.ics` file contents* below into a file named `cal.ics`.
3. From within the directory containing `cal.ics`, run `python -m http.server 8080` to make the file available locally over HTTP.
4. Run `vdirsyncer discover test_cal`.
5. Run `vdirsyncer -vdebug sync test_cal`.

**Expected result**: A parsing error is reported.

**Actual result**: No error is reported, as seen in the `vdirsyncer sync` output below. However, no events are added to the `~/scratch/test-cal` directory either.

```
Syncing test_cal
debug: ====================
debug: GET http://localhost:8080/cal.ics
debug: {'User-Agent': 'vdirsyncer/0.19.2'}
debug: None
debug: Sending request...
debug: Removing latin1 fallback
debug: 200
debug: <CIMultiDictProxy('Server': 'SimpleHTTP/0.6 Python/3.11.6', 'Date': 'Wed, 13 Dec 2023 14:20:05 GMT', 'Content-Type': 'text/calendar', 'Content-Length': '1628', 'Last-Modified': 'Wed, 13 Dec 2023 13:59:08 GMT')>
debug: <StreamReader 1628 bytes eof>
```

**`cal.ics` file contents**:
```
BEGIN:VCALENDAR
METHOD:PUBLISH
PRODID:Microsoft Exchange Server 2010
VERSION:2.0
X-WR-CALNAME:Test Calendar
BEGIN:VTIMEZONE
TZID:Eastern Standard Time
BEGIN:STANDARD
DTSTART:16010101T020000
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=1SU;BYMONTH=11
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:16010101T020000
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
RRULE:FREQ=YEARLY;INTERVAL=1;BYDAY=2SU;BYMONTH=3
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DESCRIPTION:\n
UID:040000008200E00074C5B7101A82E008000000003C3BCE59CC2DDA01000000000000000
 0100000000AF0B22E27B17A4F9E0584AB47C308DB
SUMMARY:Test event
DTSTART;TZID=Eastern Standard Time:20231213T133000
DTEND;TZID=Eastern Standard Time:20231213T140000
CLASS:PUBLIC
PRIORITY:5
DTSTAMP:20231213T135907Z
TRANSP:OPAQUE
STATUS:CONFIRMED
SEQUENCE:0
LOCATION:
X-MICROSOFT-CDO-APPT-SEQUENCE:0
X-MICROSOFT-CDO-BUSYSTATUS:BUSY
X-MICROSOFT-CDO-INTENDEDSTATUS:BUSY
X-MICROSOFT-CDO-ALLDAYEVENT:FALSE
X-MICROSOFT-CDO-IMPORTANCE:1
X-MICROSOFT-CDO-INSTTYPE:0
BEGIN:VEVENT
DESCRIPTION:\n
UID:040000008200E00074C5B7101A82E008000000003592F85FCC2DDA01000000000000000
 01000000019D89DD98006DB47AC801AB519BA086B
SUMMARY:Test event 2
DTSTART;TZID=Eastern Standard Time:20231213T143000
DTEND;TZID=Eastern Standard Time:20231213T150000
CLASS:PUBLIC
PRIORITY:5
DTSTAMP:20231213T135907Z
TRANSP:OPAQUE
STATUS:CONFIRMED
SEQUENCE:0
LOCATION:
X-MICROSOFT-CDO-APPT-SEQUENCE:0
X-MICROSOFT-CDO-BUSYSTATUS:BUSY
X-MICROSOFT-CDO-INTENDEDSTATUS:BUSY
X-MICROSOFT-CDO-ALLDAYEVENT:FALSE
X-MICROSOFT-CDO-IMPORTANCE:1
X-MICROSOFT-CDO-INSTTYPE:0
END:VEVENT
```

## P.S.

vdirsyncer has been an awesome tool for me. Thank you to everyone who's helped make it what it is!

Discussion:

Patch:
diff --git a/AUTHORS.rst b/AUTHORS.rst
index a298bb8..140936c 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -9,6 +9,7 @@ In alphabetical order:
 - Corey Hinshaw
 - Kai Herlemann
 - Hugo Osvaldo Barrera
+- Jason Cox
 - Julian Mehne
 - Malte Kiefer
 - Marek Marczykowski-Górecki
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index da711e8..815f8e7 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -15,6 +15,7 @@ Version 0.19.3
 - Added a no_delete option to the storage configuration. :gh:`1090`
 - Fix crash when running ``vdirsyncer repair`` on a collection. :gh:`1019`
 - Add an option to request vCard v4.0.  :gh:`1066`
+- Require matching ``BEGIN`` and ``END`` lines in vobjects. :gh:`1103`
 
 Version 0.19.2
 ==============
diff --git a/vdirsyncer/vobject.py b/vdirsyncer/vobject.py
index d53c75b..aae9fb9 100644
--- a/vdirsyncer/vobject.py
+++ b/vdirsyncer/vobject.py
@@ -281,6 +281,12 @@ class _Component:
                     stack.append(cls(c_name, [], []))
                 elif line.startswith("END:"):
                     component = stack.pop()
+                    c_name = line[len("END:") :].strip().upper()
+                    if c_name != component.name:
+                        raise ValueError(
+                            f"Got END:{c_name}, expected END:{component.name}"
+                            + f" at line {_i + 1}"
+                        )
                     if stack:
                         stack[-1].subcomponents.append(component)
                     else:
@@ -291,6 +297,11 @@ class _Component:
         except IndexError:
             raise ValueError(f"Parsing error at line {_i + 1}")
 
+        if len(stack) > 0:
+            raise ValueError(
+                f"Missing END for component(s): {', '.join(c.name for c in stack)}"
+            )
+
         if multiple:
             return rv
         elif len(rv) != 1:


Test patch:
diff --git a/tests/unit/utils/test_vobject.py b/tests/unit/utils/test_vobject.py
index 3745411..150c92f 100644
--- a/tests/unit/utils/test_vobject.py
+++ b/tests/unit/utils/test_vobject.py
@@ -237,6 +237,31 @@ def test_broken_item():
     assert item.parsed is None
 
 
+def test_mismatched_end():
+    with pytest.raises(ValueError) as excinfo:
+        vobject._Component.parse(
+            [
+                "BEGIN:FOO",
+                "END:BAR",
+            ]
+        )
+
+    assert "Got END:BAR, expected END:FOO at line 2" in str(excinfo.value)
+
+
+def test_missing_end():
+    with pytest.raises(ValueError) as excinfo:
+        vobject._Component.parse(
+            [
+                "BEGIN:FOO",
+                "BEGIN:BAR",
+                "END:BAR",
+            ]
+        )
+
+    assert "Missing END for component(s): FOO" in str(excinfo.value)
+
+
 def test_multiple_items():
     with pytest.raises(ValueError) as excinfo:
         vobject._Component.parse(