Bug fix repository#142
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two related correctness/security areas in the calendar (.ics) handling and event-group tagging logic: preventing path traversal when resolving predefined ICS file paths (CWE-22) and fixing tag persistence to use the proper association table during event-group creation.
Changes:
- Hardened
locate_ics_by_path()to prevent escaping the predefined ICS directory and added HTTP 400 handling where it’s used in ICS endpoints. - Added Pydantic validation for
EventGroup.pathto reject absolute and parent-directory (..) paths. - Fixed event-group tag insertion to target the tag association table rather than an ORM relationship, and adjusted create() to return a fully-populated DB-backed view.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/parse/utils.py | Adds directory-escape protection when resolving predefined ICS paths. |
| src/modules/ics/utils.py | Converts locate_ics_by_path validation failures into HTTP 400s for personal ICS generation. |
| src/modules/ics/routes.py | Converts locate_ics_by_path validation failures into HTTP 400s for event-group ICS downloads. |
| src/modules/event_groups/schemas.py | Adds request-time validation to constrain path to stay within the predefined ICS directory. |
| src/modules/event_groups/routes.py | Converts locate_ics_by_path validation failures into HTTP 400s when uploading ICS files for a group. |
| src/modules/event_groups/repository.py | Fixes tag association inserts to use the association table/mapped class and returns the persisted entity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def locate_ics_by_path(path: str) -> Path: | ||
| return settings.predefined_dir / "ics" / path | ||
| base_dir = (settings.predefined_dir / "ics").resolve(strict=False) | ||
| ics_path = (base_dir / path).resolve(strict=False) | ||
|
|
||
| if not ics_path.is_relative_to(base_dir): | ||
| raise ValueError(f"Path escapes predefined ICS directory: {path!r}") | ||
|
|
||
| return ics_path |
| def _validate_event_group_path(path: str | None) -> str | None: | ||
| if path is None: | ||
| return None | ||
|
|
||
| normalized_path = PurePosixPath(path) | ||
| if normalized_path.is_absolute() or ".." in normalized_path.parts: | ||
| raise ValueError("Path must stay within predefined/ics") | ||
|
|
||
| return path |
There was a problem hiding this comment.
_validate_event_group_path() allows path == "" (it normalizes to PurePosixPath('.')), and also allows values like "." or paths ending in /, which represent directories rather than an ICS file. Since downstream code treats path as a file path, reject these directory-like values during validation (e.g., disallow empty/"." and require a non-empty final path component).
| @@ -94,7 +94,10 @@ async def get_personal_event_groups_ics(user: ViewUser) -> AsyncGenerator[bytes] | |||
| status_code=501, | |||
| detail="Can not create .ics file for event group on the fly (set static .ics file for the event group", | |||
There was a problem hiding this comment.
The HTTPException detail string here has an opening parenthesis without a closing one, which makes the error message confusing for API consumers. Please close the parenthesis (and consider rewording for clarity) so the returned detail is well-formed.
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) from e | ||
|
|
||
| if user_agent == "Google-Calendar-Importer": # patch reccurences for Google Calendar |
There was a problem hiding this comment.
Spelling in the inline comment: "reccurences" should be "recurrences".
Description of changes
Bug fix of CWE-22 in locate_ics_by_path function at utils.py
Fix of bug when creating an event group with tags, where the insertion went into an ORM-relationship instead of an association-table.