Mathieu Virbel
0a814b769d
- 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.
6.0 KiB
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-219server/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