Feature/send sms based on schedule#857
Conversation
…le-review-fixes # Conflicts: # web/pages/settings/index.vue
…ixes feat: send schedules based on review feedback
PR Summary
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 145 |
| Duplication | 13 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Greptile SummaryThis PR adds a
Confidence Score: 3/5Not safe to merge as-is — the missing ownership check on schedule_id allows cross-user FK references that can be silently mutated by the schedule owner's delete action. One P1 finding (cross-user schedule attachment enabling cross-user data mutation via FK cascade) caps the score at 4; the presence of a security-relevant gap lowers it further to 3. api/pkg/validators/phone_handler_validator.go — needs an ownership check before accepting schedule_id on a phone update.
|
| Filename | Overview |
|---|---|
| api/pkg/validators/phone_handler_validator.go | Adds UUID-format validation for schedule_id but lacks ownership verification — a user can attach another user's schedule to their phone, causing cross-user data mutation via FK cascade. |
| api/pkg/entities/send_schedule.go | New entity and ResolveScheduledAt algorithm; logic is sound for typical schedules but an active schedule with empty windows silently falls back to "send immediately". |
| api/pkg/repositories/gorm_phone_notification_repository.go | Schedule parameter wired into rate-limited notification scheduling; the two-step resolve (schedule window + rate limit) logic looks correct for both the transactional and non-transactional paths. |
| api/pkg/validators/send_schedule_handler_validator.go | Validates windows per day, start/end ranges, and overlap; logic is correct, though empty-windows case is not blocked here. |
| api/pkg/services/phone_notification_service.go | Schedule is loaded with the caller's userID before notification scheduling, providing correct ownership isolation at runtime; silently falls back to immediate send if schedule is not found. |
| web/store/index.ts | Adds CRUD Vuex actions for send schedules; error handlers use unnecessary Promise.all wrapping, and getSendSchedules passes an ignored limit parameter to the backend. |
| api/pkg/di/container.go | Wires new repository, service, validator, handler, and listener into the DI container; AutoMigrate for MessageSendSchedule is correctly ordered before Phone. |
| api/pkg/handlers/send_schedule_handler.go | Standard CRUD handler with proper auth middleware, not-found handling, and validation flow; no issues found. |
| web/pages/settings/send-schedules/index.vue | New dedicated page for managing send schedules with full CRUD UI; no significant issues found. |
Sequence Diagram
sequenceDiagram
participant Client
participant SendScheduleHandler
participant PhoneHandler
participant PhoneNotificationService
participant SendScheduleRepository
participant PhoneNotificationRepository
Client->>SendScheduleHandler: POST /v1/send-schedules
SendScheduleHandler->>SendScheduleRepository: Store(schedule)
Client->>PhoneHandler: PUT /v1/phones {schedule_id}
PhoneHandler->>PhoneHandler: ValidateUUID(schedule_id) [no ownership check]
PhoneHandler->>PhoneNotificationService: Upsert phone with ScheduleID
Note over PhoneNotificationService,SendScheduleRepository: On message send
PhoneNotificationService->>SendScheduleRepository: Load(userID, phone.ScheduleID)
SendScheduleRepository-->>PhoneNotificationService: schedule (or ErrNotFound → nil)
PhoneNotificationService->>PhoneNotificationRepository: Schedule(messagesPerMinute, schedule, notification)
PhoneNotificationRepository->>PhoneNotificationRepository: resolveScheduledAt(schedule, now)
Note right of PhoneNotificationRepository: Finds next window start, applies rate limit, re-resolves
PhoneNotificationRepository-->>PhoneNotificationService: notification.ScheduledAt set
Reviews (1): Last reviewed commit: "Merge pull request #841 from giresse19/f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }) | ||
|
|
||
| result := v.ValidateStruct() | ||
| if request.ScheduleID != nil && strings.TrimSpace(*request.ScheduleID) != "" { | ||
| if uuidErrors := validator.ValidateUUID(strings.TrimSpace(*request.ScheduleID), "schedule_id"); len(uuidErrors) > 0 { | ||
| for key, values := range uuidErrors { | ||
| for _, value := range values { | ||
| result.Add(key, value) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validator confirms that schedule_id is a valid UUID, but never verifies that the schedule belongs to the authenticated user. A user can therefore store any schedule UUID (including one that belongs to a different user) on their phone.
At runtime the notification service does re-load the schedule using the caller's own userID (so it silently falls back to no schedule), but the DB still records the cross-user FK reference. Because Phone.Schedule carries constraint:OnDelete:SET NULL, whenever the real schedule owner deletes their schedule, GORM/the DB nulls out the schedule_id on the attacker's phone — meaning one user's action silently modifies another user's data.
An ownership check (e.g. service.Load(ctx, userID, scheduleID)) should be added to ValidateUpsert before the phone is persisted.
| func (schedule *MessageSendSchedule) ResolveScheduledAt(current time.Time) time.Time { | ||
| if schedule == nil || !schedule.IsActive || len(schedule.Windows) == 0 { | ||
| return current.UTC() |
There was a problem hiding this comment.
When IsActive is true but Windows is empty, ResolveScheduledAt returns current.UTC() (the early-exit path on line 32). This means messages are sent immediately instead of being held, which is the opposite of what a user who configured an active-but-window-less schedule might expect. Consider either disallowing an active schedule with zero windows (add a validator rule) or treating it the same as IsActive: false.
| resolve(response.data.data) | ||
| }) | ||
| .catch(async (error: AxiosError) => { | ||
| await Promise.all([ | ||
| context.dispatch('addNotification', { | ||
| message: | ||
| (error.response?.data as any)?.message ?? |
There was a problem hiding this comment.
Promise.all wrapping
All four new action error handlers wrap a single dispatch call in Promise.all([...]). Promise.all over a one-element array adds no value and makes the intent harder to read. A plain await context.dispatch(...) is clearer.
| axios | ||
| .get<ResponsesSendSchedulesResponse>(`/v1/send-schedules`, { | ||
| params: { | ||
| limit: 100, | ||
| }, |
There was a problem hiding this comment.
limit query param sent to unimplemented pagination
getSendSchedules sends { params: { limit: 100 } } but the backend GET /v1/send-schedules handler ignores all query params and returns every schedule for the user. The param is silently discarded, which may also mask the fact that if a user ever has more than 100 schedules the frontend won't show them all without backend pagination support.
2 participants
No description provided.