mirror of
https://github.com/Monadical-SAS/reflector.git
synced 2025-12-21 04:39:06 +00:00
remove work.md
This commit is contained in:
Igor Loskutov
174
work.md
174
work.md
@@ -1,174 +0,0 @@
|
|||||||
# 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
|
|
||||||
Reference in New Issue
Block a user