iCalendar: the interview UID is written unescaped and an unknown outcome raises, and an imported archive sets both #450

New issue

Closed

opened 2026-10-01 10:46:47 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:47 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the iCalendar writer and the archive importer and rendering the feeds in a test database.

Checked again on 2026-10-01 against main at 0a5cad587: still present.

Since #218 every typed value on an iCalendar line goes through escape() or without_controls(), except the interview's UID, which is written as stored. calendar_status() looks the outcome up in a four-entry table with no fallback. Neither value can be set through a form or the API, but the archive importer restores both from the file without checking them: uid whenever it does not collide, and outcome and kind through **interview_entry, where companies, CVs and listing events have their kinds checked.

Where:
- applications/ical.py:172, event_lines: f"UID:{interview.uid}", with no without_controls.
- applications/ical.py:106-113, calendar_status: return STATUS_OF[interview.outcome].
- core/importer.py:706-717: uid = interview_entry.pop("uid", None) and Interview.objects.create(..., **({"uid": uid} if uid else {}), **interview_entry). Kinds are checked only at :564 (CompanyKind), :806 (CVKind) and :965 (ListingEventKind).
- Both functions are on the plugin surface (plugins/api.py:186-191) and used by postulo-dav (src/postulo_dav/sync.py:179, :189), which also builds the event's address from the uid (:576).
What happens (checked on the snapshot):
1. An interview holding an outcome this version does not know, as an archive from a later Postulo or a hand-edited one can carry, makes /applications/interviews/<pk>/calendar.ics answer 500 with KeyError. The diary feed is not affected, because it carries only interviews still scheduled; the CalDAV sync, which walks every recent interview whatever its outcome, raises on it.
2. An interview whose uid holds a line break (possible through the importer, which does not check it) puts an extra property of its own in /applications/interviews/calendar.ics and /api/v1/interviews/calendar.ics while the interview is upcoming: the class of injection #218 closed for every other field.
Who can trigger it: whoever supplies the archive that an operator restores with manage.py import_data into somebody's account; the effect stays within that account's own feeds and calendar.
Why it matters: the writer's safety depends on every future path that sets these fields validating them, and the importer already does not. The rest of the code expects archives from later versions (applications/endings.py:71-79, _label), and this one raises instead.
Fix: in ical.py, write UID:{without_controls(interview.uid)} and use STATUS_OF.get(interview.outcome, "CONFIRMED"). In the importer, replace a uid that does not match a safe pattern (for example ^[A-Za-z0-9@._-]{1,64}$) with a fresh one, and map an unknown outcome or kind to SCHEDULED and OTHER, as listing events already are.

Related to #218 (closed), which escaped every other value on these lines.

Done when

tests/security/test_feeds_and_pictures.py has a test with an interview whose uid contains \r\n, expecting no extra line in the feed, and one with an unknown outcome, expecting the single-interview file to answer 200.
A test in tests/test_backup.py that imports an archive with an unsafe uid and an unknown outcome and expects a fresh uid and a known outcome.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the iCalendar writer and the archive importer and rendering the feeds in a test database. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. Since #218 every typed value on an iCalendar line goes through `escape()` or `without_controls()`, except the interview's `UID`, which is written as stored. `calendar_status()` looks the outcome up in a four-entry table with no fallback. Neither value can be set through a form or the API, but the archive importer restores both from the file without checking them: `uid` whenever it does not collide, and `outcome` and `kind` through `**interview_entry`, where companies, CVs and listing events have their kinds checked. - **Where:** - `applications/ical.py:172`, `event_lines`: `f"UID:{interview.uid}"`, with no `without_controls`. - `applications/ical.py:106-113`, `calendar_status`: `return STATUS_OF[interview.outcome]`. - `core/importer.py:706-717`: `uid = interview_entry.pop("uid", None)` and `Interview.objects.create(..., **({"uid": uid} if uid else {}), **interview_entry)`. Kinds are checked only at `:564` (`CompanyKind`), `:806` (`CVKind`) and `:965` (`ListingEventKind`). - Both functions are on the plugin surface (`plugins/api.py:186-191`) and used by `postulo-dav` (`src/postulo_dav/sync.py:179`, `:189`), which also builds the event's address from the uid (`:576`). - **What happens** (checked on the snapshot): 1. An interview holding an outcome this version does not know, as an archive from a later Postulo or a hand-edited one can carry, makes `/applications/interviews/<pk>/calendar.ics` answer 500 with `KeyError`. The diary feed is not affected, because it carries only interviews still scheduled; the CalDAV sync, which walks every recent interview whatever its outcome, raises on it. 2. An interview whose uid holds a line break (possible through the importer, which does not check it) puts an extra property of its own in `/applications/interviews/calendar.ics` and `/api/v1/interviews/calendar.ics` while the interview is upcoming: the class of injection #218 closed for every other field. - **Who can trigger it:** whoever supplies the archive that an operator restores with `manage.py import_data` into somebody's account; the effect stays within that account's own feeds and calendar. - **Why it matters:** the writer's safety depends on every future path that sets these fields validating them, and the importer already does not. The rest of the code expects archives from later versions (`applications/endings.py:71-79`, `_label`), and this one raises instead. - **Fix:** in `ical.py`, write `UID:{without_controls(interview.uid)}` and use `STATUS_OF.get(interview.outcome, "CONFIRMED")`. In the importer, replace a uid that does not match a safe pattern (for example `^[A-Za-z0-9@._-]{1,64}$`) with a fresh one, and map an unknown `outcome` or `kind` to `SCHEDULED` and `OTHER`, as listing events already are. Related to #218 (closed), which escaped every other value on these lines. ## Done when - `tests/security/test_feeds_and_pictures.py` has a test with an interview whose uid contains `\r\n`, expecting no extra line in the feed, and one with an unknown outcome, expecting the single-interview file to answer 200. - A test in `tests/test_backup.py` that imports an archive with an unsafe uid and an unknown outcome and expects a fresh uid and a known outcome.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:46:47 +00:00

tiagoagueda added the

security

enhancement

tier

labels

2026-10-01 10:46:47 +00:00

tiagoagueda referenced this issue

2026-10-03 01:58:35 +00:00

Write down the contribution rules: look for a library before writing one, and the formats data leaves in #653

tiagoagueda referenced this issue