feat(api): session-scoped mobile endpoints (notifications inbox + calendar)#3529
Merged
Conversation
…endar)
Mobile's inbox and calendar tabs called RPC methods that take an arbitrary
$userId and are deliberately NOT @api-exposed (IDOR guards), so they got
-32601. Add session-scoped siblings that resolve the user from the token,
mirroring the existing getUnreadCount / getMyExternalCalendars pattern:
- Notifications.getInbox() — @api inbox list; getAllNotifications($userId)
stays internal/non-@api. (Mark-all-read is already covered by markRead('all').)
- Calendar.getMyCalendar() — @api, calendar.view-gated, pins to the session
user; getCalendar($userId) stays internal-only.
- Tag the two already-session-scoped calendar reads that only lacked @api:
getICalUrl() and getExternalCalendarEvents() (+ calendar.view gate). The VIEW
gate resolves capability-only when there is no session project (the mobile API
case), so it does not re-introduce -32001.
security(notifications): close a markNotificationUnread IDOR — it was @api,
ignored its $userId param, and the repo did an unscoped where('id') update, so
any user could flip any notification unread by id. Now session-scoped (matches
on (id, session user)); the misleading $userId param is dropped.
fix(i18n): the "My To-Dos" widget growls asked Language::__() for 11
notifications.* keys that only exist under short_notifications.* (or not at
all), so the raw key leaked (e.g. "notifications.date_updated"). Repoint to
short_notifications.* (matching the JS twins + the success path already on that
prefix) and add the 9 genuinely-missing short_notifications.* keys.
getICalUrl now throws MissingParameterException (clean RPC -32602) instead of a
plain \Exception (generic -32000) when no iCal feed is configured.
Tests: BearerApiCest covers getInbox/getMyCalendar/getExternalCalendarEvents/
getICalUrl exposure over Bearer for both the owner and a non-manager editor (the
masked per-project path), plus a cross-user markNotificationUnread IDOR
regression.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
marcelfolaron
requested review from
broskees and
Copilot
and removed request for
a team
June 14, 2026 15:29
Contributor
There was a problem hiding this comment.
Pull request overview
Adds session-scoped JSON-RPC endpoints needed by mobile (inbox + calendar) while keeping the existing userId-taking methods internal, and hardens notification mutations against IDOR. Also fixes “My To-Dos” widget i18n key usage and extends Bearer acceptance coverage for the new endpoints/regression.
Changes:
- Add
@apisession-scoped endpoints:Notifications.getInbox()andCalendar.getMyCalendar(from?, until?). - Expose already-session-scoped calendar reads (
getICalUrl,getExternalCalendarEvents) and improve error semantics for missing iCal configuration. - Fix notification IDOR in
markNotificationUnreadby scoping updates to(id, session user)and add acceptance regression coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Acceptance/API/BearerApiCest.php | Adds Bearer acceptance coverage for the new/exposed mobile RPC endpoints and the IDOR regression. |
| app/Language/en-US.ini | Adds missing short_notifications.* keys needed by widget growls. |
| app/Domain/Widgets/Hxcontrollers/MyToDos.php | Switches widget notifications from notifications.* to short_notifications.* keys. |
| app/Domain/Notifications/Services/Notifications.php | Makes markNotificationUnread session-scoped; adds getInbox() API wrapper. |
| app/Domain/Notifications/Repositories/Notifications.php | Scopes markNotificationUnread update by userId to prevent cross-user updates. |
| app/Domain/Calendar/Services/Calendar.php | Adds getMyCalendar() API wrapper; exposes getICalUrl()/getExternalCalendarEvents() with permission gate; uses MissingParameterException for missing iCal secret. |
Comments suppressed due to low confidence (1)
app/Domain/Calendar/Services/Calendar.php:740
getExternalCalendarEvents()usessession('userdata.id')directly in the cache key and repository call without guarding for a missing/empty session user. If this method is ever invoked without an established session user, it will cache undercalendar.external.and query calendars for an empty user id. Add an early guard and use a normalized$userIdfor the cache key and repo access.
public function getExternalCalendarEvents(null|string|CarbonImmutable $from = null, null|string|CarbonImmutable $until = null): array
{
$cacheKey = 'calendar.external.'.session('userdata.id');
$cached = Cache::get($cacheKey);
if (is_array($cached) && ! empty($cached)) {
return $cached;
}
// Get all external calendars for the user
$externalCalendars = $this->calendarRepo->getMyExternalCalendars(session('userdata.id'));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+187
to
+195
| public function getInbox(int $showNewOnly = 0, int $limitStart = 0, int $limitEnd = 100, array $filterOptions = []): array | ||
| { | ||
| $userId = (int) session('userdata.id'); | ||
| if ($userId === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| return $this->notificationsRepo->getAllNotifications($userId, (bool) $showNewOnly, $limitStart, $limitEnd, $filterOptions) ?: []; | ||
| } |
Comment on lines
+645
to
+648
| public function getMyCalendar(null|string|CarbonImmutable $from = null, null|string|CarbonImmutable $until = null): array | ||
| { | ||
| return $this->getCalendar($this->currentUserId() ?? 0, $from, $until); | ||
| } |
Comment on lines
+105
to
+108
| // getICalUrl is now exposed too. It legitimately errors when the user has no iCal secret | ||
| // configured yet, so assert only that it is FOUND (not -32601 method-not-found). | ||
| $icalUrl = $this->rpc($I, 'leantime.rpc.Calendar.Calendar.getICalUrl', new \stdClass); | ||
| Assert::assertNotSame(-32601, $icalUrl['error']['code'] ?? null, 'getICalUrl must be exposed via @api: '.json_encode($icalUrl)); |
…ead IDOR test
grabFromDatabase('zp_notifications','read',...) emits unquoted SELECT read FROM
(read is a MySQL reserved word) → PDO syntax error. The assertFalse on the RPC
result already proves the IDOR is closed (editor's session-scoped update matched
0 rows); the DB read-back was redundant.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- getInbox: drop the repo's arbitrary column=>value filter passthrough (mobile doesn't need it; it allowed unknown WHERE columns / SQL errors) and clamp pagination (limitStart>=0, limitEnd 1..100) so an @api caller can't request an unbounded page. - getMyCalendar: guard currentUserId() === 0 and return [] when unauthenticated, matching getInbox (instead of querying as userId 0). - BearerApiCest: strengthen the getICalUrl assertion — also fail on -32001 (permission regression), accept either a URL result or -32602. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Thanks @copilot — all three addressed in
|
Comment on lines
+644
to
+653
| #[RequiresPermission(CalendarPermissions::VIEW)] | ||
| public function getMyCalendar(null|string|CarbonImmutable $from = null, null|string|CarbonImmutable $until = null): array | ||
| { | ||
| $userId = $this->currentUserId() ?? 0; | ||
| if ($userId === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| return $this->getCalendar($userId, $from, $until); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A post-3.9.4 mobile audit found the inbox and calendar tabs were calling RPC methods that take an arbitrary
$userIdand are deliberately not@api-exposed (IDOR guards), so they returned-32601 Method not found. The fix is the same pattern already used bygetUnreadCount/getMyExternalCalendars: add session-scoped siblings that resolve the user from the token, leaving the userId-trusting originals internal.What
New
@apiendpoints (mobile drops theuserIdparam, calls with{}):Notifications.getInbox()— inbox list for the session user.getAllNotifications($userId)stays internal/non-@api. (Mark-all-read is already available viamarkRead('all').)Calendar.getMyCalendar(from?, until?)— session-scoped calendar feed; pins tocurrentUserId().getCalendar($userId)stays internal-only (the webShowMyCalendarcontroller keeps using it).Tag two reads that were already session-scoped and only lacked
@api:Calendar.getICalUrl()andCalendar.getExternalCalendarEvents()— both key offsession('userdata.id'), no spoofable param. Added@api+#[RequiresPermission(calendar.view)].Security —
security(notifications):markNotificationUnreadwas@api, ignored its$userIdparam, and the repo did an unscopedwhere('id')update → any authenticated user could flip any notification unread by guessing its id (IDOR; ids are a global sequence). Now session-scoped (matches(id, session user)); the misleading$userIdparam is dropped (RPC param-matching ignores the extra arg mobile still sends, so it's backward-compatible). A cross-user regression test is included.i18n —
fix(i18n): the "My To-Dos" widget growls askedLanguage::__()for 11notifications.*keys that only exist undershort_notifications.*(or not at all), so the raw key leaked (e.g.notifications.date_updatedshown literally). Repointed toshort_notifications.*— matching the JS twins (ticketsController.js/dashboardController.js) and the success path (short_notifications.status_updated) already on that prefix — and added the 9 genuinely-missing keys toen-US.ini. (The header inbox dropdown was a red herring — it renders the stored, pre-translated message.)Error semantics:
getICalUrlnow throwsMissingParameterException(clean RPC-32602) instead of a plain\Exception(generic-32000) when no iCal feed is configured. Still an\Exception, so the webExportcontroller'scatchis unaffected.Tests
BearerApiCestnow assertsgetInbox/getMyCalendar/getExternalCalendarEvents/getICalUrlexposure over Bearer for both the owner and a non-manager editor (the per-project authz path that owner-only testing masks), plus a cross-usermarkNotificationUnreadIDOR regression.Local:
php -l, Pint, and PHPStan (level 1) all green on the changed files. Acceptance runs in CI (the Docker container mounts the main repo, not a worktree).Verification by an independent adversarial review
#[RequiresPermission]is enforced only at dispatch entry points (Frontcontroller / CheckPermissions / Jsonrpc) — inert on internal service-to-service calls, so tagging the existinggetICalUrl/getExternalCalendarEventsdoes not affect their web callers.calendar.viewpath resolves capability-only (no fail-closed).🤖 Generated with Claude Code