mirror of
https://github.com/Monadical-SAS/reflector.git
synced 2025-12-21 20:59:05 +00:00
- Implement per-meeting locks using Redis to prevent concurrent processing - Add lock extension after slow API calls (Whereby) to handle long-running operations - Use redis-py's built-in lock.extend() with replace_ttl=True for simple TTL refresh - Track and log skipped meetings when locked by other workers - Document SSRF analysis showing it's low-risk due to async worker isolation This prevents multiple workers from processing the same meeting simultaneously, which could cause state corruption or duplicate deactivations.
175 lines
6.0 KiB
Markdown
175 lines
6.0 KiB
Markdown
# PR Feedback Work Items
|
|
|
|
## Security Issues
|
|
|
|
### 1. SSRF Vulnerability in ICS Fetch Service
|
|
**File:** `server/reflector/services/ics_sync.py:45-49`
|
|
**Issue:** The ICS fetch service doesn't validate or sanitize URLs before fetching them, which could lead to SSRF vulnerabilities.
|
|
**Severity:** ~~High~~ **Low** (Downgraded)
|
|
**Note:** After analysis, this is nearly unexploitable because:
|
|
- Runs async in Celery worker with 5+ minute delays
|
|
- No feedback to attacker (no errors, timing, or responses)
|
|
- Only processes valid ICS format
|
|
- Attacker can't verify if request succeeded or failed
|
|
**Decision:** Ignore for now - theoretical risk only
|
|
|
|
### 2. Race Condition in process_meetings
|
|
**File:** ~~`server/reflector/worker/ics_sync.py:149-219`~~ `server/reflector/worker/process.py:212-296`
|
|
**Issue:** Complex logic for handling meeting state transitions with potential race conditions when multiple workers process the same meeting simultaneously.
|
|
**Severity:** Medium
|
|
**Status:** ✅ FIXED - Implemented ExtendableLock with Redis
|
|
**Solution:** Added distributed locking using Redis with automatic lock extension for long-running processes:
|
|
- 60-second initial timeout with 30-second auto-extension
|
|
- Non-blocking acquisition to skip locked meetings
|
|
- Prevents duplicate processing across multiple workers
|
|
|
|
## Code Issues
|
|
|
|
### 3. Missing Import for meetings_controller
|
|
**File:** `server/reflector/views/rooms.py` (join meeting endpoint)
|
|
**Issue:** Missing import for `meetings_controller` will cause NameError.
|
|
**Severity:** High
|
|
|
|
### 4. Import Performance Issue
|
|
**File:** `server/reflector/views/rooms.py` (sync ICS endpoint)
|
|
**Issue:** `ics_sync_service` import is inside function body, imported on every request.
|
|
**Severity:** Low
|
|
|
|
### 5. Grace Period Logic Issue
|
|
**File:** `server/reflector/worker/ics_sync.py`
|
|
**Issue:** Grace period logic doesn't check if meeting is already inactive and doesn't verify if participants rejoined.
|
|
**Severity:** Medium
|
|
|
|
## Frontend Code Quality
|
|
|
|
### 6. Date Parameter Type Issue
|
|
**File:** `www/app/lib/timeUtils.ts:1`
|
|
**Issue:** Should just use "Date" and pass with `new Date(start_time)` - too much implicitness.
|
|
**Severity:** Low
|
|
|
|
### 7. Missing End Comment for Calendar Hooks
|
|
**File:** `www/app/lib/apiHooks.ts:749`
|
|
**Issue:** Need to signal end of "Calendar integration hooks" section.
|
|
**Severity:** Low
|
|
|
|
### 8. Non-null Assertion Issue
|
|
**File:** `www/app/lib/apiHooks.ts:650`
|
|
**Issue:** Using `roomName!` shadows null/undefined issues, better to fail explicitly.
|
|
**Severity:** Low
|
|
|
|
### 9. Missing Comment
|
|
**File:** `www/app/lib/apiHooks.ts:577`
|
|
**Issue:** Need comment like "Invalidate all meeting-related queries".
|
|
**Severity:** Low
|
|
|
|
### 10. Array Type Check
|
|
**File:** `www/app/lib/apiHooks.ts:582`
|
|
**Issue:** Should verify always returns array.
|
|
**Severity:** Low
|
|
|
|
### 11. Duplicate Condition Check
|
|
**File:** `www/app/lib/apiHooks.ts:584`
|
|
**Issue:** Should combine conditions: `&& (k.includes("/meetings/active") || k.includes("/meetings/upcoming"))`.
|
|
**Severity:** Low
|
|
|
|
### 12. Component Naming
|
|
**File:** `www/app/components/MinimalHeader.tsx:17`
|
|
**Issue:** Consider renaming to `MeetingMinimalHeader`.
|
|
**Severity:** Low
|
|
|
|
### 13. useEffect Meeting Creation Logic
|
|
**File:** `www/app/[roomName]/useRoomMeeting.tsx:46`
|
|
**Issue:** useEffect fires on any meetingId change, potentially creating duplicate meetings.
|
|
**Severity:** Medium
|
|
|
|
### 14. Obvious Comment
|
|
**File:** `www/app/[roomName]/page.tsx:374`
|
|
**Issue:** Comment seems obvious from code.
|
|
**Severity:** Low
|
|
|
|
### 15. URL Structure Concern
|
|
**File:** `www/app/[roomName]/page.tsx:302`
|
|
**Issue:** URL structure getting complex, needs refactoring.
|
|
**Severity:** Medium
|
|
|
|
### 16. Code Duplication (3rd time)
|
|
**File:** `www/app/[roomName]/[meetingId]/page.tsx:28`
|
|
**Issue:** Code repeated 3rd time, needs abstraction.
|
|
**Severity:** Medium
|
|
|
|
### 17. Copy-Paste Issue
|
|
**File:** `www/app/[roomName]/[meetingId]/page.tsx:43`
|
|
**Issue:** Code appears to be copy-pasted from `[roomName]/page.tsx`.
|
|
**Severity:** Medium
|
|
|
|
### 18. Unnecessary Code
|
|
**File:** `www/app/[roomName]/MeetingSelection.tsx:33`
|
|
**Issue:** Code not needed.
|
|
**Severity:** Low
|
|
|
|
### 19. Missing Punctuation
|
|
**File:** `www/app/[roomName]/MeetingSelection.tsx:57`
|
|
**Issue:** Missing period.
|
|
**Severity:** Low
|
|
|
|
### 20. Inconsistent Time Value
|
|
**File:** `www/app/[roomName]/MeetingSelection.tsx:73`
|
|
**Issue:** Comment says 1 or 5 minutes?
|
|
**Severity:** Low
|
|
|
|
### 21. Data Partitioning Logic
|
|
**File:** `www/app/[roomName]/MeetingSelection.tsx:76`
|
|
**Issue:** Extracting currentMeetings and upcomingMeetings may have intersection issues, needs partition function.
|
|
**Severity:** Medium
|
|
|
|
### 22. Missing Character
|
|
**File:** `www/app/[roomName]/MeetingSelection.tsx:104`
|
|
**Issue:** Missing "?".
|
|
**Severity:** Low
|
|
|
|
### 23. Code Duplication
|
|
**File:** `www/app/(app)/rooms/page.tsx:330`
|
|
**Issue:** Duplicates code after "detailedEditedRoom".
|
|
**Severity:** Medium
|
|
|
|
### 24. Emoji Constant
|
|
**File:** `www/app/(app)/rooms/page.tsx:835`
|
|
**Issue:** ✅ emoji should be a constant with string template.
|
|
**Severity:** Low
|
|
|
|
### 25. Missing Punctuation
|
|
**File:** `www/app/(app)/rooms/_components/ICSSettings.tsx:70`
|
|
**Issue:** Missing period.
|
|
**Severity:** Low
|
|
|
|
### 26. Type Safety Issue
|
|
**File:** `www/app/(app)/rooms/_components/RoomTable.tsx:87`
|
|
**Issue:** Use `meeting.calendar_metadata?.["title"]` instead of "as any".
|
|
**Severity:** Low
|
|
|
|
### 27. Duplicated Logic
|
|
**File:** `www/app/(app)/rooms/_components/RoomTable.tsx:114`
|
|
**Issue:** Minutes logic duplicated in another place.
|
|
**Severity:** Low
|
|
|
|
## Documentation
|
|
|
|
### 28. Unclear Purpose
|
|
**File:** `consent-handler.md`
|
|
**Issue:** What's this file for? Most likely NOT needed.
|
|
**Severity:** Low
|
|
|
|
## Error Handling
|
|
|
|
### 29. Silent Failures in pre_create_upcoming_meetings
|
|
**File:** `server/reflector/worker/ics_sync.py:103-198`
|
|
**Issue:** Function catches all exceptions at top level but doesn't properly handle individual meeting creation failures.
|
|
**Severity:** Medium
|
|
|
|
---
|
|
|
|
## Priority Levels:
|
|
- **High**: Items 1, 3
|
|
- **Medium**: Items 2, 5, 13, 15, 16, 17, 21, 23, 29
|
|
- **Low**: Items 4, 6, 7, 8, 9, 10, 11, 12, 14, 18, 19, 20, 22, 24, 25, 26, 27, 28
|