✨(push) add Push Notifications system for iOS, Android, Web#727
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesPush notifications are implemented end to end across backend registration and delivery, Web Push and native clients, configuration, notifications UI, service-worker handling, VAPID tooling, operational documentation, and comprehensive tests. Channel secret handling and template name resolution are also updated. Push Notifications
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant InboundPipeline
participant Celery
participant PushOrchestrator
participant DeviceSender
participant PushGateway
InboundPipeline->>Celery: enqueue after message commit
Celery->>PushOrchestrator: resolve recipients and devices
PushOrchestrator->>DeviceSender: enqueue per-device notification
DeviceSender->>PushGateway: deliver thin payload
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sylvinus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/tests/api/test_channel_scope_level.py (1)
1021-1029: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the one-time JWT credential on the personal-scope create path.
With
auth_method: "jwt"here, this test should also verify that/users/me/channels/returns the one-timesecret(and notapi_key). The mailbox-scoped coverage insrc/backend/core/tests/api/test_channels.pywill not catch a regression in the personal-scope endpoint. As per coding guidelines,Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/tests/api/test_channel_scope_level.py` around lines 1021 - 1029, The personal webhook create test in test_channel_scope_level should assert the JWT one-time credential returned by the /users/me/channels/ create path. Update the test around the api_client.post for the personal-webhook so it explicitly checks that the response includes secret and does not include api_key when auth_method is jwt, using the existing test case as the single focused coverage for this endpoint.Source: Coding guidelines
src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture path for webhook-authored assignment events.
The new branch in
buildAssignmentMessage()only runs whenauthorisnullandauthor_displayis non-null, but this helper forcesauthor_displaytonullwheneverauthorIdisnull. That leaves the new"Webhook: … assigned …"wording path unrepresentable in this test file.Example adjustment
const makeEvent = ( type: 'assign' | 'unassign', authorId: string | null, assignees: { id: string; name: string }[], + authorDisplay: string | null = authorId === null ? null : `User ${authorId}`, ): AssignmentEvent => ({ @@ - author_display: authorId === null ? null : `User ${authorId}`, + author_display: authorDisplay,const event = makeEvent('assign', null, [{ id: SELF_ID, name: 'You' }], 'Webhook: GitHub'); expect(buildAssignmentMessage(event, SELF_ID, fakeT)).toBe('Webhook: GitHub assigned you');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts` around lines 18 - 30, The makeEvent test helper in assignment-message.test.ts cannot represent webhook-authored assignment events because it always sets author_display to null when authorId is null. Update makeEvent so it can accept an explicit author_display override for the null-author case, then add a fixture using buildAssignmentMessage with author null and a non-null display label to cover the new webhook wording path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/push-notifications.md`:
- Around line 27-28: The push notifications docs incorrectly say push channels
are blocked from create via the generic endpoint, which conflicts with the
documented/tested flow. Update the bullet in the push notifications section to
reflect that push devices are created with POST on users/me/channels using
type="push", and only the generic PATCH/update path is blocked for push
metadata; keep the list/revoke wording tied to GET and DELETE.
In `@docs/webhooks.md`:
- Around line 91-95: The webhook docs use the wrong regenerate-secret path in
the root secret description; update the wording in the relevant webhook sections
to match the actual scope-specific routes used by the API. Refer to the webhook
secret handling text around the `auth_method` and `regenerate-secret`
descriptions, and replace the generic `/channels/{id}/regenerate-secret/` path
with the correct mailbox- and user-scoped endpoints so readers are directed to
real routes.
- Around line 451-480: The minimal receiver example in inbox_hook is currently
unauthenticated and can accept spoofed POSTs, so update the snippet to fail
closed unless request verification is performed. Add explicit JWT/API-key
validation before processing the body and only proceed when the signature/key is
valid, or otherwise show a clearly non-production example that rejects
unauthorized requests by default. Keep the guidance tied to inbox_hook,
request.headers, and request.get_json so readers understand auth must be wired
before using the sample.
In `@src/backend/core/admin.py`:
- Around line 612-619: The admin secret rotation flow in the webhook admin
should display the correct credential for api-key webhooks instead of the raw
secret returned by rotate_secret(). Update the logic around
channel.rotate_secret() in the admin rotation view to branch on
auth_method=api_key and use get_webhook_api_key() for the displayed secret,
while keeping the existing raw secret behavior for other webhook types. Make
sure the context’s "secret" value and the generated "New secret generated" page
both reflect the derived API key for the webhook model/instance being rotated.
In `@src/backend/core/api/serializers.py`:
- Around line 2814-2822: The Web Push key validation in the serializer currently
only checks truthiness, so non-string values like objects or lists can slip
through and get stored in encrypted_settings. Update the validation in this
branch of the serializer method handling attrs["platform"] ==
enums.PushPlatformChoices.WEB to explicitly require both keys["p256dh"] and
keys["auth"] to be non-empty strings before persisting them. Keep the existing
normalization to attrs["keys"], but reject any missing or non-string values with
serializers.ValidationError using the existing web push key error message.
- Around line 2245-2250: The webhook URL validation in the serializer should
return a 400 instead of bubbling a ValueError for malformed netlocs. Update the
URL handling in the serializer method that calls urlparse() and reads
parsed_url.hostname to catch ValueError around that parsing/hostname extraction,
and convert it into serializers.ValidationError with the existing settings error
shape so bad webhook URLs are rejected consistently.
In `@src/backend/core/enums.py`:
- Around line 222-224: The push-notification channel comment is pointing to the
wrong place for the token hash storage. Update the comment near the user-scoped
push target definition to say that the device push token lives in
encrypted_settings, the token hash is stored in Channel.lookup_hash, and
settings holds the platform/client metadata; use Channel and lookup_hash as the
reference points so future callers check the correct fields.
In `@src/backend/core/management/commands/derive_vapid_public_key.py`:
- Around line 30-33: The `derive_vapid_public_key()` command help text is
advertising a private-key format that the implementation does not support.
Update the `--private-key` option help in `derive_vapid_public_key` to describe
only PEM unless you also add base64url parsing before calling
`Vapid02.from_pem(...)`, so the documented format matches the actual accepted
input.
In `@src/backend/core/mda/dispatch_webhooks.py`:
- Around line 593-617: In the webhook result handling inside the dispatch flow,
avoid merging any side effects from blocking responses that do not continue,
because aborting decisions like retry can still propagate flags such as
mark_trashed and mark_archived into ctx. Update the logic around result.decision
so that the method returns immediately for any non-CONTINUE decision before
applying ctx updates, and only apply labels, pending assigns/events/drafts, and
boolean merges when the webhook actually continues. Use the existing decision
handling and the surrounding webhook result processing in dispatch_webhooks to
place the early return.
In `@src/backend/core/mda/inbound_pipeline.py`:
- Around line 497-525: The warnings in inbound_pipeline.py are logging sensitive
assignee email addresses directly, so update the assignee resolution flow to
avoid emitting PII. In the webhook assignee handling and the assignable-role
check, replace email-based log fields in logger.warning calls with a
non-reversible internal identifier or a position/count-based reference tied to
candidate_users/candidate_ids. Keep the existing control flow in the assignee
matching logic, but ensure the messages in the candidate_users loop and the
assignable_ids validation no longer include raw email values.
- Around line 346-347: The fallback rspamd auth path in inbound_pipeline should
handle transient rspamd failures the same way as the spam step instead of
silently ignoring them. In the branch that calls _call_rspamd in the inbound
auth check, capture and inspect the second return value (the exception/error
from _call_rspamd) and apply the same retry/quarantine behavior used by the
rspamd spam logic when the auth check cannot be completed. Keep the existing
ctx.rspamd_result assignment on success, but make sure the sender-auth banner is
not dropped when rspamd is temporarily unavailable.
- Around line 177-210: The header-matching logic in the inbound pipeline is
incorrectly lowercasing regex patterns, which can break `header_match_regex`
rules and may also raise on invalid patterns. Update the normalization in
`inbound_pipeline.py` so exact-match comparisons still use a lowered/trimmed
value, but regex matching in the header rule path keeps the original pattern
text before `re.fullmatch`; use the existing header rule flow around
`header_match`/`header_match_regex` and add `re.error` handling so a bad regex
rule is skipped instead of aborting processing.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 145-149: The lock in process_inbound_message is hardcoded to
expire after 300s, which can be shorter than the worst-case webhook pipeline
runtime and allow duplicate processing. Update the locking approach around
cache.add(lock_key, "locked", 300) to either refresh the lock while processing
or compute a TTL based on the webhook count and per-webhook timeout so the lock
stays valid until InboundMessage deletion and finalization are complete.
In `@src/backend/core/models.py`:
- Around line 535-541: The hash-input guidance in the model comment is
inconsistent with the push registration shape, since `lookup_hash` is currently
derived from the bare token and can collide under the global unique constraint.
Update the push registration path and any related lookup/tests to namespace the
hashed input with a type-specific prefix (for example in the push registration
flow using `sha256(...)`), and keep the `uniq_channel_lookup_hash`-related
behavior aligned with that scoped keying.
- Around line 681-692: get_webhook_api_key() may return None for misconfigured
channels, but the webhook dispatch path is still passing that value into
X-StMsg-Api-Key. Update the webhook-building logic in dispatch_webhooks.py to
fail closed before headers are created: either skip auth headers entirely or
raise a controlled error when get_webhook_api_key() returns None. Use
get_webhook_api_key() and the header construction code as the key locations to
patch.
In `@src/backend/core/services/push/common.py`:
- Around line 430-437: The stale-delete limiter in the delete-check logic is
using a fixed TTL bucket instead of a rolling window because cache.incr() does
not refresh expiry. Update the counter handling in the stale-delete path around
the cache_key logic so each delete hit renews the expiration (for example by
touching the key or resetting the TTL after incrementing), keeping the cap
aligned with the last minute rather than the first delete. Ensure the change is
applied in the code that uses cache.add(), cache.incr(), and
STALE_DELETE_WINDOW_LIMIT/STALE_DELETE_WINDOW_SECONDS.
In `@src/backend/core/services/push/tasks.py`:
- Around line 126-130: The fan-out loop in the push dispatch path aborts if a
single send_push_notification.delay call fails, so handle enqueue errors per
channel inside the loop in the dispatch logic. In the task method that iterates
over user_channels, catch exceptions around each delay call, log the failing
channel id with enough context, and continue to the next channel so one bad
device does not stop the rest from being dispatched.
In `@src/backend/core/services/push/webpush.py`:
- Around line 79-83: The warning in webpush.py logs the raw PUSH_VAPID_SUBJECT
value, which can expose sensitive PII in logs. Update the logger.warning call in
the Web Push settings validation path to report only that PUSH_VAPID_SUBJECT is
malformed and that Web Push is disabled, without interpolating
settings.PUSH_VAPID_SUBJECT or any other subject value. Keep the fix localized
to the VAPID subject validation logic so the behavior stays the same except for
redacting the logged content.
In `@src/frontend/public/sw.js`:
- Around line 89-103: The notification click handling in sw.js is reusing the
first same-origin window from clients.matchAll() without checking whether it is
the intended mailbox/thread tab. Update the loop in the notification navigation
path to only focus/navigate a matching client when it already represents the app
target, and otherwise fall back to clients.openWindow(url). Use the existing
clients.matchAll, client.focus, client.navigate, and clients.openWindow flow to
prefer a relevant tab rather than blindly redirecting any same-origin window.
- Around line 40-55: The enrichment fetch in buildNotification can hang
indefinitely and delay the fallback banner, so add a short timeout around the
MESSAGE_URL(payload.message_id) request in src/frontend/public/sw.js. Update the
fetch-to-enrich block to abort/stop waiting after the timeout and then continue
with the generic notification path, keeping the existing catch behavior for
offline/cross-origin/signed-out cases. Use the buildNotification and MESSAGE_URL
symbols to locate the logic.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/devices-view/user-devices-grid.tsx`:
- Around line 78-82: The browser enablement check in user-devices-grid should
also respect the backend push feature flag, not just isWebPushSupported() and
config.PUSH_VAPID_PUBLIC_KEY. Update the canEnableThisBrowser logic (and any
related button visibility/availability in this flow) to require
config.PUSH_ENABLED so the UI never offers push registration when the feature is
disabled and the registration endpoint would 404.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.ts`:
- Around line 107-124: The subscription reuse logic in `webPushSubscription` is
not validating that an existing `PushSubscription` was created with the current
`vapidPublicKey`, which can leave the browser stuck on an old key after
rotation. Update both `webPushSubscription` and `refreshWebPushSubscription` to
inspect the existing subscription’s app server key before reusing it, and if it
does not match the current `vapidPublicKey`, unsubscribe and create a fresh
subscription instead. Keep the existing `AbortError` handling in the
`registration.pushManager.subscribe` path intact.
In
`@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx`:
- Around line 111-128: When editing an existing webhook in
webhook-integration-form.tsx, don’t allow changing auth_method without also
exposing a usable credential. Update the edit flow around the newSettings
construction and the isEditing/updateMutation path so auth is either read-only,
or changing it triggers a secret rotation/regenerate-secret flow and surfaces
the returned secret/api_key to the user. Use the existing WebhookChannelSettings
shape and the mutateAsync/updateMutation logic to keep auth mode and credential
state consistent.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 258-259: The ThreadEvent author rendering is using
event.author_display for the avatar label, but avatarColor still hashes
event.author?.full_name || event.author?.email, which breaks webhook IM events
where author is null and all avatars look the same. Update the avatar color
source to use the same resolved author string as the label in this component, so
UserAvatar and the displayed text in the thread-event view stay consistent for
all event types.
---
Outside diff comments:
In `@src/backend/core/tests/api/test_channel_scope_level.py`:
- Around line 1021-1029: The personal webhook create test in
test_channel_scope_level should assert the JWT one-time credential returned by
the /users/me/channels/ create path. Update the test around the api_client.post
for the personal-webhook so it explicitly checks that the response includes
secret and does not include api_key when auth_method is jwt, using the existing
test case as the single focused coverage for this endpoint.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts`:
- Around line 18-30: The makeEvent test helper in assignment-message.test.ts
cannot represent webhook-authored assignment events because it always sets
author_display to null when authorId is null. Update makeEvent so it can accept
an explicit author_display override for the null-author case, then add a fixture
using buildAssignmentMessage with author null and a non-null display label to
cover the new webhook wording path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 371ac34b-0d5d-47b4-bbf4-4e04996f3ec0
⛔ Files ignored due to path filters (13)
src/backend/uv.lockis excluded by!**/*.locksrc/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/platform_enum.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/push_channel_create_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/push_channel_create_request_keys.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/push_channel_create_type_enum.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_api_key_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/regenerated_secret_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_channel_create_request_request.tsis excluded by!**/gen/**
📒 Files selected for processing (59)
docs/push-notifications.mddocs/webhooks.mdenv.d/development/backend.defaultssrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/management/commands/derive_vapid_public_key.pysrc/backend/core/mda/autoreply.pysrc/backend/core/mda/dispatch_webhooks.pysrc/backend/core/mda/inbound_create.pysrc/backend/core/mda/inbound_pipeline.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/migrations/0032_channel_lookup_hash.pysrc/backend/core/models.pysrc/backend/core/services/push/__init__.pysrc/backend/core/services/push/apns.pysrc/backend/core/services/push/common.pysrc/backend/core/services/push/fcm.pysrc/backend/core/services/push/tasks.pysrc/backend/core/services/push/webpush.pysrc/backend/core/services/ssrf.pysrc/backend/core/services/thread_events.pysrc/backend/core/templates/admin/core/channel/regenerated_api_key.htmlsrc/backend/core/tests/api/test_channel_scope_level.pysrc/backend/core/tests/api/test_channels.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/mda/test_inbound_auth.pysrc/backend/core/tests/mda/test_inbound_spoofed_sender.pysrc/backend/core/tests/mda/test_spam_processing.pysrc/backend/core/tests/test_push.pysrc/backend/messages/settings.pysrc/backend/pyproject.tomlsrc/frontend/public/locales/common/de-DE.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/public/sw.jssrc/frontend/src/features/controlled-modals/index.tssrc/frontend/src/features/controlled-modals/notifications.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/devices-view/user-devices-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.tssrc/frontend/src/features/layouts/components/mailbox-settings/integrations-view/integrations-data-grid.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/index.tsxsrc/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsxsrc/frontend/src/features/layouts/components/main/header/authenticated.tsxsrc/frontend/src/features/layouts/components/notifications-settings/modal-notifications.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-header.tsxsrc/frontend/src/features/providers/config.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (24)
docs/push-notifications.md (1)
27-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Don't say push channels are blocked from create here.
This contradicts the previous bullet and the tested API flow: push devices are created via
POST /api/v1.0/users/me/channels/withtype="push". Only the generic update path appears to be blocked for push metadata, not creation itself.Suggested doc fix
-- **List / revoke:** the normal `GET`/`DELETE /api/v1.0/users/me/channels/`. - Push channels are blocked from create/PATCH through the generic endpoint. +- **List / revoke:** the normal `GET`/`DELETE /api/v1.0/users/me/channels/`. + Push channels register through the collection `POST` above, but their + generic `PATCH` update path is blocked.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- **List / revoke:** the normal `GET`/`DELETE /api/v1.0/users/me/channels/`. Push channels register through the collection `POST` above, but their generic `PATCH` update path is blocked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/push-notifications.md` around lines 27 - 28, The push notifications docs incorrectly say push channels are blocked from create via the generic endpoint, which conflicts with the documented/tested flow. Update the bullet in the push notifications section to reflect that push devices are created with POST on users/me/channels using type="push", and only the generic PATCH/update path is blocked for push metadata; keep the list/revoke wording tied to GET and DELETE.docs/webhooks.md (2)
91-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the real regenerate-secret routes.
POST /channels/{id}/regenerate-secret/does not match the scope-specific endpoints exercised elsewhere in this PR (/mailboxes/{mailbox_id}/channels/{id}/regenerate-secret/and/users/me/channels/{id}/regenerate-secret/). As written, this will send readers to a non-existent path.Also applies to: 119-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/webhooks.md` around lines 91 - 95, The webhook docs use the wrong regenerate-secret path in the root secret description; update the wording in the relevant webhook sections to match the actual scope-specific routes used by the API. Refer to the webhook secret handling text around the `auth_method` and `regenerate-secret` descriptions, and replace the generic `/channels/{id}/regenerate-secret/` path with the correct mailbox- and user-scoped endpoints so readers are directed to real routes.
451-480: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't publish an unauthenticated receiver example.
This sample accepts any POST and ignores the documented JWT / API-key verification, so copying it verbatim produces a spoofable webhook endpoint. Please either verify the signature/key here or make the example fail closed until auth is wired.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/webhooks.md` around lines 451 - 480, The minimal receiver example in inbox_hook is currently unauthenticated and can accept spoofed POSTs, so update the snippet to fail closed unless request verification is performed. Add explicit JWT/API-key validation before processing the body and only proceed when the signature/key is valid, or otherwise show a clearly non-production example that rejects unauthorized requests by default. Keep the guidance tied to inbox_hook, request.headers, and request.get_json so readers understand auth must be wired before using the sample.src/backend/core/admin.py (1)
612-619: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Display the derived API key for
auth_method=api_keywebhooks.
rotate_secret()returns the rawwhsec_root for webhooks, but api-key webhooks authenticate withget_webhook_api_key()(whk_...). The admin rotation page will otherwise show operators a credential the receiver should not configure.Proposed fix
plaintext = channel.rotate_secret() + displayed_secret = plaintext + if ( + channel.type == ChannelTypes.WEBHOOK + and (channel.settings or {}).get("auth_method") == "api_key" + ): + displayed_secret = channel.get_webhook_api_key() context = { **self.admin_site.each_context(request), "opts": self.model._meta, # noqa: SLF001 "original": channel, "title": "New secret generated", - "secret": plaintext, + "secret": displayed_secret, }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.plaintext = channel.rotate_secret() displayed_secret = plaintext if ( channel.type == ChannelTypes.WEBHOOK and (channel.settings or {}).get("auth_method") == "api_key" ): displayed_secret = channel.get_webhook_api_key() context = { **self.admin_site.each_context(request), "opts": self.model._meta, # noqa: SLF001 "original": channel, "title": "New secret generated", "secret": displayed_secret,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/admin.py` around lines 612 - 619, The admin secret rotation flow in the webhook admin should display the correct credential for api-key webhooks instead of the raw secret returned by rotate_secret(). Update the logic around channel.rotate_secret() in the admin rotation view to branch on auth_method=api_key and use get_webhook_api_key() for the displayed secret, while keeping the existing raw secret behavior for other webhook types. Make sure the context’s "secret" value and the generated "New secret generated" page both reflect the derived API key for the webhook model/instance being rotated.src/backend/core/api/serializers.py (2)
2245-2250: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return a 400 for malformed webhook URLs.
urlparse()/parsed_url.hostnamecan raiseValueErrorfor malformed netlocs such as invalid bracketed IPv6. Catch it and raise a serializer validation error instead of letting a bad request become a 500.Proposed fix
- parsed_url = urlparse(url) + try: + parsed_url = urlparse(url) + host = (parsed_url.hostname or "").lower() + except ValueError as exc: + raise serializers.ValidationError( + {"settings": "webhook settings.url must be a valid URL."} + ) from exc if parsed_url.scheme not in ("http", "https"): raise serializers.ValidationError( {"settings": "webhook settings.url must be http:// or https://"} ) - host = (parsed_url.hostname or "").lower() if not host:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: parsed_url = urlparse(url) host = (parsed_url.hostname or "").lower() except ValueError as exc: raise serializers.ValidationError( {"settings": "webhook settings.url must be a valid URL."} ) from exc if parsed_url.scheme not in ("http", "https"): raise serializers.ValidationError( {"settings": "webhook settings.url must be http:// or https://"} )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/serializers.py` around lines 2245 - 2250, The webhook URL validation in the serializer should return a 400 instead of bubbling a ValueError for malformed netlocs. Update the URL handling in the serializer method that calls urlparse() and reads parsed_url.hostname to catch ValueError around that parsing/hostname extraction, and convert it into serializers.ValidationError with the existing settings error shape so bad webhook URLs are rejected consistently.
2814-2822: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate Web Push keys are non-empty strings.
The current truthiness check accepts objects/lists for
p256dhorauth, then persists them intoencrypted_settings; senders later expect subscription key strings.Proposed fix
if attrs.get("platform") == enums.PushPlatformChoices.WEB: keys = attrs.get("keys") or {} - if not keys.get("p256dh") or not keys.get("auth"): + p256dh = keys.get("p256dh") + auth = keys.get("auth") + if ( + not isinstance(p256dh, str) + or not p256dh.strip() + or not isinstance(auth, str) + or not auth.strip() + ): raise serializers.ValidationError( {"keys": "web push requires keys.p256dh and keys.auth."} ) # Persist only the two subscription keys we actually use, so a # client can't make us store arbitrary blobs in encrypted_settings. - attrs["keys"] = {"p256dh": keys["p256dh"], "auth": keys["auth"]} + attrs["keys"] = {"p256dh": p256dh, "auth": auth}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if attrs.get("platform") == enums.PushPlatformChoices.WEB: keys = attrs.get("keys") or {} p256dh = keys.get("p256dh") auth = keys.get("auth") if ( not isinstance(p256dh, str) or not p256dh.strip() or not isinstance(auth, str) or not auth.strip() ): raise serializers.ValidationError( {"keys": "web push requires keys.p256dh and keys.auth."} ) # Persist only the two subscription keys we actually use, so a # client can't make us store arbitrary blobs in encrypted_settings. attrs["keys"] = {"p256dh": p256dh, "auth": auth}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/serializers.py` around lines 2814 - 2822, The Web Push key validation in the serializer currently only checks truthiness, so non-string values like objects or lists can slip through and get stored in encrypted_settings. Update the validation in this branch of the serializer method handling attrs["platform"] == enums.PushPlatformChoices.WEB to explicitly require both keys["p256dh"] and keys["auth"] to be non-empty strings before persisting them. Keep the existing normalization to attrs["keys"], but reject any missing or non-string values with serializers.ValidationError using the existing web push key error message.src/backend/core/enums.py (1)
222-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the push channel storage comment.
The token hash is stored in
Channel.lookup_hash, whilesettingscarries platform/client metadata. The current comment points future callers at the wrong storage location.Proposed wording
- # device's push token lives in ``encrypted_settings``; ``settings`` carries - # the platform + a token hash for dedup/reclaim. See core.services.push. + # device's push token lives in ``encrypted_settings``; ``lookup_hash`` + # carries the token hash for dedup/reclaim; ``settings`` carries platform + # metadata. See core.services.push.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# A user-scoped push-notification target (one per mobile device). The # device's push token lives in ``encrypted_settings``; ``lookup_hash`` # carries the token hash for dedup/reclaim; ``settings`` carries platform # metadata. See core.services.push.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/enums.py` around lines 222 - 224, The push-notification channel comment is pointing to the wrong place for the token hash storage. Update the comment near the user-scoped push target definition to say that the device push token lives in encrypted_settings, the token hash is stored in Channel.lookup_hash, and settings holds the platform/client metadata; use Channel and lookup_hash as the reference points so future callers check the correct fields.src/backend/core/management/commands/derive_vapid_public_key.py (1)
30-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether py-vapid's from_pem accepts base64url private-key strings. python - <<'PY' import inspect from py_vapid import Vapid02 print(inspect.signature(Vapid02.from_pem)) print(Vapid02.from_pem.__doc__) PYRepository: suitenumerique/messages
Length of output: 282
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files first. git ls-files 'src/backend/core/management/commands/derive_vapid_public_key.py' \ 'src/backend/core/services/push/webpush.py' \ 'src/backend/core/services/push/*' \ | sed 's#^`#FILE` #' echo echo "=== outline: derive_vapid_public_key.py ===" ast-grep outline src/backend/core/management/commands/derive_vapid_public_key.py --view expanded || true echo echo "=== outline: webpush.py ===" ast-grep outline src/backend/core/services/push/webpush.py --view expanded || true echo echo "=== search for Vapid02.from_pem / base64url references ===" rg -n "from_pem|base64url|PUSH_VAPID_PRIVATE_KEY|derive_vapid_public_key" src/backend/core -SRepository: suitenumerique/messages
Length of output: 6188
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the relevant sections with line numbers. sed -n '1,220p' src/backend/core/management/commands/derive_vapid_public_key.py | cat -n echo sed -n '1,240p' src/backend/core/services/push/webpush.py | cat -nRepository: suitenumerique/messages
Length of output: 13575
Align the help text with the supported private-key format.
derive_vapid_public_key()passes the value straight toVapid02.from_pem(...), so--private-keyshould advertise PEM only unless base64url parsing is actually added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/management/commands/derive_vapid_public_key.py` around lines 30 - 33, The `derive_vapid_public_key()` command help text is advertising a private-key format that the implementation does not support. Update the `--private-key` option help in `derive_vapid_public_key` to describe only PEM unless you also add base64url parsing before calling `Vapid02.from_pem(...)`, so the documented format matches the actual accepted input.src/backend/core/mda/dispatch_webhooks.py (1)
593-617: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not merge side effects from aborting webhook responses.
A blocking webhook can return
{"action":"retry","mark_trashed":true}. The task will eventually quarantine-deliver that message, but these lines have already copiedmark_trashed/mark_archivedintoctx, so the “delivered to inbox so the warning is seen” path can still hide it. Return immediately on non-CONTINUEdecisions before applying side effects.Proposed fix
) + if result.decision != Decision.CONTINUE: + return result.decision + if result.is_spam_override is not None: ctx.is_spam = result.is_spam_override📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if result.decision != Decision.CONTINUE: return result.decision if result.is_spam_override is not None: ctx.is_spam = result.is_spam_override ctx.labels |= result.labels if result.assign_to: # Defer the actual assignment until after the thread # exists (post-message-creation). Each blocking webhook # that asked gets its own ThreadEvent ASSIGN, attributed # to this channel. ctx.pending_assigns.append((self.channel.id, result.assign_to)) # Bool flags OR-merge: any blocking webhook saying true sticks. ctx.mark_starred = ctx.mark_starred or result.mark_starred ctx.mark_read = ctx.mark_read or result.mark_read ctx.mark_trashed = ctx.mark_trashed or result.mark_trashed ctx.mark_archived = ctx.mark_archived or result.mark_archived ctx.skip_autoreply = ctx.skip_autoreply or result.skip_autoreply for event in result.events: # Per-event attribution like assigns — one ThreadEvent per # add_event entry, with channel set to the firing webhook. ctx.pending_events.append((self.channel.id, event)) if result.reply_draft_template_id: # Defer template lookup + draft creation until after the # message + thread land. Each blocking webhook that asked # produces its own draft, channel-attributed. ctx.pending_drafts.append((self.channel.id, result.reply_draft_template_id)) return result.decision🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/dispatch_webhooks.py` around lines 593 - 617, In the webhook result handling inside the dispatch flow, avoid merging any side effects from blocking responses that do not continue, because aborting decisions like retry can still propagate flags such as mark_trashed and mark_archived into ctx. Update the logic around result.decision so that the method returns immediately for any non-CONTINUE decision before applying ctx updates, and only apply labels, pending assigns/events/drafts, and boolean merges when the webhook actually continues. Use the existing decision handling and the surrounding webhook result processing in dispatch_webhooks to place the early return.src/backend/core/mda/inbound_pipeline.py (3)
177-210: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash python - <<'PY' import re assert r"\S+".lower() == r"\s+" assert re.fullmatch(r"\S+".lower(), "abc") is None PYRepository: suitenumerique/messages
Length of output: 161
Don't lowercase
header_match_regexvalues.value.lower()changes regex escapes like\S/\Dand can turn a valid pattern into the wrong match; keep exact-match values normalized separately and guardre.fullmatchwithre.errorso a bad rule doesn't abort the step.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 209-209: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.fullmatch(value, header_value)
Note: [CWE-1333] Inefficient Regular Expression Complexity.(redos-non-literal-regex-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/inbound_pipeline.py` around lines 177 - 210, The header-matching logic in the inbound pipeline is incorrectly lowercasing regex patterns, which can break `header_match_regex` rules and may also raise on invalid patterns. Update the normalization in `inbound_pipeline.py` so exact-match comparisons still use a lowered/trimmed value, but regex matching in the header rule path keeps the original pattern text before `re.fullmatch`; use the existing header rule flow around `header_match`/`header_match_regex` and add `re.error` handling so a bad regex rule is skipped instead of aborting processing.Source: Linters/SAST tools
346-347: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Retry when rspamd-backed inbound auth cannot be checked.
When
ctx.is_spamis already set, the rspamd spam step skips and this fallback is the only rspamd call for sender auth. Ignoring_call_rspamd(...)[1]means a transient rspamd outage silently removes the sender-auth banner instead of using the same retry/quarantine behavior.Proposed fix
def inbound_auth(ctx: InboundContext) -> Decision: if ctx.rspamd_result is None and get_inbound_auth_mode(spam_config) == "rspamd": - _, _, ctx.rspamd_result = _call_rspamd(ctx.raw_data, spam_config) + _, err, result = _call_rspamd(ctx.raw_data, spam_config) + if err: + logger.warning( + "rspamd error during inbound auth on message %s: %s", + ctx.inbound_message.id, + err, + ) + return Decision.RETRY + ctx.rspamd_result = result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if ctx.rspamd_result is None and get_inbound_auth_mode(spam_config) == "rspamd": _, err, result = _call_rspamd(ctx.raw_data, spam_config) if err: logger.warning( "rspamd error during inbound auth on message %s: %s", ctx.inbound_message.id, err, ) return Decision.RETRY ctx.rspamd_result = result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/inbound_pipeline.py` around lines 346 - 347, The fallback rspamd auth path in inbound_pipeline should handle transient rspamd failures the same way as the spam step instead of silently ignoring them. In the branch that calls _call_rspamd in the inbound auth check, capture and inspect the second return value (the exception/error from _call_rspamd) and apply the same retry/quarantine behavior used by the rspamd spam logic when the auth check cannot be completed. Keep the existing ctx.rspamd_result assignment on success, but make sure the sender-auth banner is not dropped when rspamd is temporarily unavailable.
497-525: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove assignee email addresses from logs.
These warnings log webhook-supplied or user email addresses directly. Log a position/count or non-reversible internal identifier instead. As per coding guidelines,
src/backend/**/*.py: “Do not log sensitive information (tokens, passwords, financial/health data, PII)”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/inbound_pipeline.py` around lines 497 - 525, The warnings in inbound_pipeline.py are logging sensitive assignee email addresses directly, so update the assignee resolution flow to avoid emitting PII. In the webhook assignee handling and the assignable-role check, replace email-based log fields in logger.warning calls with a non-reversible internal identifier or a position/count-based reference tied to candidate_users/candidate_ids. Keep the existing control flow in the assignee matching logic, but ensure the messages in the candidate_users loop and the assignable_ids validation no longer include raw email values.Source: Coding guidelines
src/backend/core/mda/inbound_tasks.py (1)
145-149: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the processing lock cover worst-case webhook runtime.
The lock expires after 300s, but each webhook can block for 30s and the pipeline may include many webhook steps. If the lock expires before the first worker deletes the
InboundMessage, the queue sweep can start a second run, duplicating webhook calls and non-idempotent finalize side effects such as IM events or reply drafts. Use a refreshed lock/heartbeat or size the TTL from the webhook count and timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/inbound_tasks.py` around lines 145 - 149, The lock in process_inbound_message is hardcoded to expire after 300s, which can be shorter than the worst-case webhook pipeline runtime and allow duplicate processing. Update the locking approach around cache.add(lock_key, "locked", 300) to either refresh the lock while processing or compute a TTL based on the webhook count and per-webhook timeout so the lock stays valid until InboundMessage deletion and finalization are complete.src/backend/core/models.py (2)
535-541: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Namespace
lookup_hashinputs before relying on global uniqueness.The comment says every type should namespace its hash input, but the documented push shape is
sha256(token)and the provided push registration context uses the bare token hash. With a global unique constraint, future external-identity channel types can collide on the same raw value. Use a type-scoped input such assha256(f"push:{token}")and update the registration lookup/tests together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/models.py` around lines 535 - 541, The hash-input guidance in the model comment is inconsistent with the push registration shape, since `lookup_hash` is currently derived from the bare token and can collide under the global unique constraint. Update the push registration path and any related lookup/tests to namespace the hashed input with a type-specific prefix (for example in the push registration flow using `sha256(...)`), and keep the `uniq_channel_lookup_hash`-related behavior aligned with that scoped keying.
681-692: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make missing webhook roots fail closed before building headers.
get_webhook_api_key()can returnNone, but the provideddispatch_webhooks.pycontext puts that value directly intoX-StMsg-Api-Key. Misconfigured rows should return no auth headers or raise a controlled error, not produce aNoneheader value at send time.Proposed downstream guard
- return {"X-StMsg-Api-Key": self.channel.get_webhook_api_key()} + api_key = self.channel.get_webhook_api_key() + if not api_key: + logger.warning( + "Webhook channel %s has no root secret — skipping", + self.channel.id, + ) + return None + return {"X-StMsg-Api-Key": api_key}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/models.py` around lines 681 - 692, get_webhook_api_key() may return None for misconfigured channels, but the webhook dispatch path is still passing that value into X-StMsg-Api-Key. Update the webhook-building logic in dispatch_webhooks.py to fail closed before headers are created: either skip auth headers entirely or raise a controlled error when get_webhook_api_key() returns None. Use get_webhook_api_key() and the header construction code as the key locations to patch.src/backend/core/services/push/common.py (1)
430-437: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -e echo "Configured cache backend(s):" rg -n -C3 'CACHES|RedisCache|PyMemcacheCache|Memcached|LocMemCache' src/backend/messages/settings.pyRepository: suitenumerique/messages
Length of output: 3203
🏁 Script executed:
#!/bin/bash set -e printf 'common.py around stale-delete helper:\n' sed -n '410,445p' src/backend/core/services/push/common.py printf '\nTest coverage around stale-delete behavior:\n' rg -n -C3 '_stale_delete_within_window|stale-delete|stale_delete' src/backend/core/tests/test_push.py printf '\nCache backend selection in settings:\n' rg -n -C2 'CACHES =|LocMemCache|django_redis.cache.RedisCache|REDIS_URL|CACHES_DEFAULT_TIMEOUT' src/backend/messages/settings.pyRepository: suitenumerique/messages
Length of output: 1907
🌐 Web query:
Django cache incr expiry semantics LocMemCache RedisCache django-redis does incr preserve timeout or ttl💡 Result:
In Django's cache framework, the
incr()anddecr()methods are intended to update a value without affecting the key's existing Time-To-Live (TTL) or expiration timeout [1][2]. However, the implementation of this behavior has historically varied across backends, often resulting in bugs where the timeout is inadvertently reset to the default value [3][4][2]. LocMemCache TheLocMemCachebackend explicitly preserves the existing TTL duringincr()anddecr()operations [1][5]. This was established to match the behavior of production-grade backends like Memcached, ensuring that cache updates do not prematurely expire or extend keys [1][2]. RedisCache (via django-redis)django-redisis designed to leverage native Redis atomicINCR/DECRcommands [6], which inherently do not modify a key's TTL [7]. However, becausedjango-redismust handle serialization and potential race conditions (e.g., when a key does not exist), historical implementations have occasionally suffered from bugs where the TTL was lost or reset if the operation failed or fell back to non-atomic paths [8][9]. Modern versions ofdjango-redisuse scripts or atomic logic intended to preserve the TTL, but developers should be aware that custom configurations or older versions may exhibit inconsistent behavior regarding expiration [8][6][9]. DatabaseCache TheDatabaseCachebackend has historically had a known issue whereincr()anddecr()reset the expiration date to the default timeout instead of preserving the original one [3][10][4]. Recent efforts in the Django project have been directed at fixing this, ensuring that theDatabaseCachebackend properly maintains the original expiry value during these operations [10][4]. Summary of Semantics When usingincr()in Django, the desired and generally supported behavior is that the TTL remains unchanged [1][2]. If you encounter a scenario where the TTL is reset, it is typically considered a bug within the specific cache backend implementation rather than intended framework semantics [3][4][1]. If you rely heavily on persistent TTLs for counters, it is recommended to verify the behavior with your specific Django and cache backend versions, as fixes for these semantics have been an ongoing maintenance area [10][4][1].Citations:
- 1: https://code.djangoproject.com/ticket/17287
- 2: https://code.djangoproject.com/ticket/16858
- 3: https://code.djangoproject.com/ticket/26619
- 4: https://code.djangoproject.com/ticket/36610
- 5: https://github.com/django/django/blob/main/django/core/cache/backends/locmem.py
- 6: https://github.com/jazzband/django-redis/
- 7: https://redis.io/docs/latest/commands/incr/
- 8: jazzband/django-redis#181
- 9: jazzband/django-redis#499
- 10: django/django#19875
Make the stale-delete cap actually rolling
cache.incr()preserves the key’s TTL here, so this is a fixed 60-second bucket from the first delete, not a rolling window. Refresh the expiry on each hit (ortouch()the key) if the intent is to cap deletes over the last minute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/common.py` around lines 430 - 437, The stale-delete limiter in the delete-check logic is using a fixed TTL bucket instead of a rolling window because cache.incr() does not refresh expiry. Update the counter handling in the stale-delete path around the cache_key logic so each delete hit renews the expiration (for example by touching the key or resetting the TTL after incrementing), keeping the cap aligned with the last minute rather than the first delete. Ensure the change is applied in the code that uses cache.add(), cache.incr(), and STALE_DELETE_WINDOW_LIMIT/STALE_DELETE_WINDOW_SECONDS.src/backend/core/services/push/tasks.py (1)
126-130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate per-device enqueue failures.
If one
send_push_notification.delay(...)raises, the fan-out loop aborts and all remaining devices for this message are skipped. Since push is best-effort, log the failed channel and continue dispatching the rest.Proposed fix
) for channel in user_channels: - send_push_notification.delay( # pylint: disable=no-member - str(channel.id), payload, collapse_key - ) - dispatched += 1 + try: + send_push_notification.delay( # pylint: disable=no-member + str(channel.id), payload, collapse_key + ) + except Exception as exc: + logger.warning( + "Failed to dispatch push task for channel %s: %s", + channel.id, + exc, + ) + continue + dispatched += 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for channel in user_channels: try: send_push_notification.delay( # pylint: disable=no-member str(channel.id), payload, collapse_key ) except Exception as exc: logger.warning( "Failed to dispatch push task for channel %s: %s", channel.id, exc, ) continue dispatched += 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/tasks.py` around lines 126 - 130, The fan-out loop in the push dispatch path aborts if a single send_push_notification.delay call fails, so handle enqueue errors per channel inside the loop in the dispatch logic. In the task method that iterates over user_channels, catch exceptions around each delay call, log the failing channel id with enough context, and continue to the next channel so one bad device does not stop the rest from being dispatched.src/backend/core/services/push/webpush.py (1)
79-83: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging the VAPID subject value.
PUSH_VAPID_SUBJECTis commonly amailto:URI, so this can put an email address in logs. Log only that the setting is malformed.Proposed fix
logger.warning( - "PUSH_VAPID_SUBJECT %r is not a mailto:/https: URI; " - "Web Push is disabled until it is fixed.", - settings.PUSH_VAPID_SUBJECT, + "PUSH_VAPID_SUBJECT is not a mailto:/https: URI; " + "Web Push is disabled until it is fixed." )As per coding guidelines,
src/backend/**/*.py: Do not log sensitive information, including PII.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.logger.warning( "PUSH_VAPID_SUBJECT is not a mailto:/https: URI; " "Web Push is disabled until it is fixed." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/webpush.py` around lines 79 - 83, The warning in webpush.py logs the raw PUSH_VAPID_SUBJECT value, which can expose sensitive PII in logs. Update the logger.warning call in the Web Push settings validation path to report only that PUSH_VAPID_SUBJECT is malformed and that Web Push is disabled, without interpolating settings.PUSH_VAPID_SUBJECT or any other subject value. Keep the fix localized to the VAPID subject validation logic so the behavior stays the same except for redacting the logged content.Source: Coding guidelines
src/frontend/public/sw.js (2)
40-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the enrichment fetch.
If
/api/v1.0/messages/${id}/stalls,event.waitUntil(buildNotification(payload))waits on that request and the generic fallback is delayed too, so a partial API outage can suppress the notification instead of degrading gracefully. This path needs a short timeout and then an immediate fallback to the content-free banner.Suggested fix
+const fetchWithTimeout = async (url, options, timeoutMs) => { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + return await fetch(url, { ...options, signal: controller.signal }); + } finally { + clearTimeout(timer); + } +}; + async function buildNotification(payload) { @@ if (payload.message_id) { try { - const resp = await fetch(MESSAGE_URL(payload.message_id), { + const resp = await fetchWithTimeout( + MESSAGE_URL(payload.message_id), + { credentials: "include", headers: { Accept: "application/json" }, - }); + }, + 2000, + ); if (resp.ok) { const message = await resp.json(); title = senderLabel(message);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const fetchWithTimeout = async (url, options, timeoutMs) => { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), timeoutMs); try { return await fetch(url, { ...options, signal: controller.signal }); } finally { clearTimeout(timer); } }; // Fetch-to-enrich: pull sender + subject over the authenticated session. if (payload.message_id) { try { const resp = await fetchWithTimeout( MESSAGE_URL(payload.message_id), { credentials: "include", headers: { Accept: "application/json" }, }, 2000, ); if (resp.ok) { const message = await resp.json(); title = senderLabel(message); options.body = message.subject || ""; } } catch { // Offline / cross-origin / signed out — keep the generic banner. } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/public/sw.js` around lines 40 - 55, The enrichment fetch in buildNotification can hang indefinitely and delay the fallback banner, so add a short timeout around the MESSAGE_URL(payload.message_id) request in src/frontend/public/sw.js. Update the fetch-to-enrich block to abort/stop waiting after the timeout and then continue with the generic notification path, keeping the existing catch behavior for offline/cross-origin/signed-out cases. Use the buildNotification and MESSAGE_URL symbols to locate the logic.
89-103: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't navigate the first same-origin tab blindly.
clients.matchAll()returns every controlled window for this origin. The current loop focuses and navigates the first one, so clicking a notification can yank an unrelated tab away from its current page instead of reusing a mailbox tab or opening a new one.Suggested fix
event.waitUntil( clients .matchAll({ type: "window", includeUncontrolled: true }) .then((windowClients) => { - // Reuse an existing tab where possible: focus it and navigate to the - // thread (refetches and clears the badge). - for (const client of windowClients) { - if ("focus" in client) { - if ("navigate" in client && url !== "/") { - return client.focus().then((c) => (c || client).navigate(url)); - } - return client.focus(); - } + const appClient = windowClients.find((client) => { + const pathname = new URL(client.url).pathname; + return pathname === "/" || pathname.startsWith("/mailbox/"); + }); + + if (appClient && "focus" in appClient) { + if ("navigate" in appClient && url !== "/") { + return appClient.focus().then((c) => (c || appClient).navigate(url)); + } + return appClient.focus(); } if (clients.openWindow) { return clients.openWindow(url); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.clients .matchAll({ type: "window", includeUncontrolled: true }) .then((windowClients) => { const appClient = windowClients.find((client) => { const pathname = new URL(client.url).pathname; return pathname === "/" || pathname.startsWith("/mailbox/"); }); if (appClient && "focus" in appClient) { if ("navigate" in appClient && url !== "/") { return appClient.focus().then((c) => (c || appClient).navigate(url)); } return appClient.focus(); } if (clients.openWindow) { return clients.openWindow(url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/public/sw.js` around lines 89 - 103, The notification click handling in sw.js is reusing the first same-origin window from clients.matchAll() without checking whether it is the intended mailbox/thread tab. Update the loop in the notification navigation path to only focus/navigate a matching client when it already represents the app target, and otherwise fall back to clients.openWindow(url). Use the existing clients.matchAll, client.focus, client.navigate, and clients.openWindow flow to prefer a relevant tab rather than blindly redirecting any same-origin window.src/frontend/src/features/layouts/components/mailbox-settings/devices-view/user-devices-grid.tsx (1)
78-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate this flow on
PUSH_ENABLEDtoo.This button only checks browser support plus
PUSH_VAPID_PUBLIC_KEY, but the backend now exposesPUSH_ENABLEDspecifically to hide push registration when the feature is off. In that state the registration endpoint returns 404, so this UI can advertise an action that is guaranteed to fail.Suggested fix
- const canEnableThisBrowser = - isWebPushSupported() && !!config.PUSH_VAPID_PUBLIC_KEY; + const canEnableThisBrowser = + config.PUSH_ENABLED && + isWebPushSupported() && + !!config.PUSH_VAPID_PUBLIC_KEY; @@ - if (!config.PUSH_VAPID_PUBLIC_KEY) { + if (!config.PUSH_ENABLED || !config.PUSH_VAPID_PUBLIC_KEY) { return; }Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/mailbox-settings/devices-view/user-devices-grid.tsx` around lines 78 - 82, The browser enablement check in user-devices-grid should also respect the backend push feature flag, not just isWebPushSupported() and config.PUSH_VAPID_PUBLIC_KEY. Update the canEnableThisBrowser logic (and any related button visibility/availability in this flow) to require config.PUSH_ENABLED so the UI never offers push registration when the feature is disabled and the registration endpoint would 404.src/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.ts (1)
107-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the Web Push Push API, is an existing PushSubscription still valid after the site's VAPID applicationServerKey changes, or should clients unsubscribe and resubscribe with the new key?💡 Result:
An existing PushSubscription is cryptographically bound to the VAPID applicationServerKey used at the time of subscription [1][2]. If you change your applicationServerKey, existing subscriptions will effectively become invalid because the push service will reject any push messages authenticated with the new private key, as they do not match the key associated with the original subscription [2][3]. To handle a VAPID key change, you must force clients to resubscribe [4][2]. The browser does not automatically update or replace the subscription when the key changes; attempting to subscribe with a new key while an old subscription exists may cause errors in some browsers [5]. The recommended approach to handle this transition is: 1. Detect that the applicationServerKey has changed (e.g., by comparing the current key used by your app with the key stored in the existing subscription's options) [4][6]. 2. If the keys do not match, call unsubscribe on the existing PushSubscription [4][5]. 3. Call subscribe again with the new applicationServerKey to create a fresh subscription [4][5]. 4. Send the new subscription details to your server to update your records [7]. Failure to perform this re-subscription flow will result in push delivery failures, typically manifesting as 401 Unauthorized or 403 Forbidden errors when your server attempts to send notifications [4][2][3].
Citations:
- 1: https://webpush-wg.github.io/webpush-vapid/draft-ietf-webpush-vapid.html
- 2: https://www.web-push-notifications.com/core-protocols-browser-implementation/vapid-key-generation-rotation/
- 3: https://datatracker.ietf.org/doc/rfc9749/
- 4: https://stackoverflow.com/questions/45994933/changing-application-server-key-in-push-manager-subscription
- 5: ryoppippi/web-push-neo@38087ce
- 6: https://developer.mozilla.org/en-US/docs/Web/API/PushSubscriptionOptions/applicationServerKey
- 7: https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe
🏁 Script executed:
#!/bin/bash set -euo pipefail file='src/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.ts' echo '== file outline ==' ast-grep outline "$file" --view expanded || true echo echo '== relevant lines ==' nl -ba "$file" | sed -n '1,260p'Repository: suitenumerique/messages
Length of output: 585
🏁 Script executed:
#!/bin/bash set -euo pipefail file='src/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.ts' echo '== relevant lines 80-240 ==' sed -n '80,240p' "$file" | cat -n echo echo '== search for subscription handling ==' rg -n "getSubscription|subscribe\\(|unsubscribe\\(|applicationServerKey|refreshWebPushSubscription|enableWebPush" "$file"Repository: suitenumerique/messages
Length of output: 5660
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '== VAPID / pushsubscription references ==' rg -n "vapid|pushsubscriptionchange|refreshWebPushSubscription|enableWebPush|applicationServerKey" src/frontend src -g '!**/node_modules/**'Repository: suitenumerique/messages
Length of output: 11641
🏁 Script executed:
#!/bin/bash set -euo pipefail file='src/frontend/public/sw.js' echo '== outline ==' ast-grep outline "$file" --view expanded || true echo echo '== relevant lines 120-170 ==' sed -n '120,170p' "$file" | cat -nRepository: suitenumerique/messages
Length of output: 2605
Resubscribe when the configured VAPID key changes. Both paths reuse an existing
PushSubscriptionwithout checking whether it was created with the currentvapidPublicKey, so a key rotation can leave the browser bound to the old key and keep re-posting a subscription that push delivery can no longer use. Also applies torefreshWebPushSubscription.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/mailbox-settings/devices-view/web-push.ts` around lines 107 - 124, The subscription reuse logic in `webPushSubscription` is not validating that an existing `PushSubscription` was created with the current `vapidPublicKey`, which can leave the browser stuck on an old key after rotation. Update both `webPushSubscription` and `refreshWebPushSubscription` to inspect the existing subscription’s app server key before reusing it, and if it does not match the current `vapidPublicKey`, unsubscribe and create a fresh subscription instead. Keep the existing `AbortError` handling in the `registration.pushManager.subscribe` path intact.src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx (1)
111-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid changing webhook auth mode without revealing a matching credential.
On edit, switching
auth_methodchanges what receivers must verify, but update responses never includesecret/api_key; those are only returned on create or rotation. This can leave an existing webhook configured to require a credential the user cannot see. Either make authentication read-only during edit, triggerregenerate-secretand show the returned credential, or require users to rotate explicitly after changing auth mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/mailbox-settings/modal-compose-integration/webhook-integration-form.tsx` around lines 111 - 128, When editing an existing webhook in webhook-integration-form.tsx, don’t allow changing auth_method without also exposing a usable credential. Update the edit flow around the newSettings construction and the isEditing/updateMutation path so auth is either read-only, or changing it triggers a secret rotation/regenerate-secret flow and surfaces the returned secret/api_key to the user. Use the existing WebhookChannelSettings shape and the mutateAsync/updateMutation logic to keep auth mode and credential state consistent.src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
258-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the same resolved author string for avatar color and label.
These lines render
author_display, butavatarColoris still derived fromevent.author?.full_name || event.author?.emailat Line 221. For webhook IM eventsauthorisnullwhileauthor_displayis populated, so every integration comment hashes from the empty string and ends up visually indistinguishable.Proposed fix
- const authorName = event.author?.full_name || event.author?.email || ""; + const authorName = event.author_display || event.author?.full_name || event.author?.email || t("Unknown"); const avatarColor = getAvatarColor(authorName); ... - <UserAvatar fullName={event.author_display || t("Unknown")} size="xsmall" /> - {event.author_display || t("Unknown")} + <UserAvatar fullName={authorName} size="xsmall" /> + {authorName}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const authorName = event.author_display || event.author?.full_name || event.author?.email || t("Unknown"); const avatarColor = getAvatarColor(authorName); ... <UserAvatar fullName={authorName} size="xsmall" /> {authorName}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx` around lines 258 - 259, The ThreadEvent author rendering is using event.author_display for the avatar label, but avatarColor still hashes event.author?.full_name || event.author?.email, which breaks webhook IM events where author is null and all avatars look the same. Update the avatar color source to use the same resolved author string as the label in this component, so UserAvatar and the displayed text in the thread-event view stay consistent for all event types.
9331775 to
27222a7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/push-notifications.md`:
- Line 76: Add a blank line after each flagged level-3 heading in the
documentation so markdownlint MD022 passes. Update every affected `###` section
header in `docs/push-notifications.md` by inserting a separator before the
following content, using the same pattern for all instances around the `push
notifications` section headings.
In `@src/backend/core/api/viewsets/config.py`:
- Around line 234-244: Make PUSH_ENABLED required in the config schema so it
matches the always-present runtime value. Update the ConfigEntry for
PUSH_ENABLED in the config viewset to use required=True, alongside the other
feature-flag entries, so the OpenAPI schema and generated TS client treat it as
non-optional.
In `@src/backend/core/services/push/fcm.py`:
- Around line 108-174: send_fcm currently lets batch-level failures escape,
unlike send_apns, so transient client setup or loop errors are treated as
sender_raised and won’t retry. Wrap the _fcm_client() initialization and the
entire per-channel loop in an outer try/except in send_fcm, mirroring the APNs
pattern, and return a transient PushResult(0, len(items)) (or equivalent) on
failure so send_push_notification can retry through PushTransientError.
In `@src/backend/core/tests/test_push.py`:
- Line 15: The pylint disable directive in the test module contains a duplicate
protected-access entry; clean up the comment by deduplicating the disable list
so each symbol appears only once. Update the top-of-file pylint pragma in
test_push.py and keep the existing disables intact, just remove the repeated
protected-access entry.
In `@src/backend/messages/settings.py`:
- Around line 238-279: Add boot-time partial-configuration validation for the
push settings in post_setup, mirroring the existing VAPID guard. The current
PUSH_APNS_* and PUSH_FCM_* groups can silently no-op when only some fields are
set, so update the settings validation logic to fail fast if APNs is partially
configured (PUSH_APNS_KEY, PUSH_APNS_KEY_ID, PUSH_APNS_TEAM_ID,
PUSH_APNS_BUNDLE_ID must all be present together) or if FCM is partially
configured (PUSH_FCM_CREDENTIALS and PUSH_FCM_PROJECT_ID must both be present).
Locate this in the post_setup validation path alongside the existing VAPID
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef58855e-378a-4bf2-830b-88b95ec6296c
⛔ Files ignored due to path filters (6)
src/backend/uv.lockis excluded by!**/*.locksrc/frontend/public/assets/icons/icon-mono-72.pngis excluded by!**/*.pngsrc/frontend/src/features/api/gen/channels/channels.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/channel_create_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**
📒 Files selected for processing (30)
docs/push-notifications.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/management/commands/derive_vapid_public_key.pysrc/backend/core/management/commands/generate_vapid_private_key.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/migrations/0033_channel_lookup_hash.pysrc/backend/core/models.pysrc/backend/core/services/push/__init__.pysrc/backend/core/services/push/apns.pysrc/backend/core/services/push/common.pysrc/backend/core/services/push/fcm.pysrc/backend/core/services/push/tasks.pysrc/backend/core/services/push/webpush.pysrc/backend/core/signals.pysrc/backend/core/tasks.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/test_push.pysrc/backend/core/tests/test_settings.pysrc/backend/messages/settings.pysrc/backend/pyproject.tomlsrc/frontend/public/locales/common/de-DE.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/sw.js
💤 Files with no reviewable changes (6)
- src/frontend/public/locales/common/de-DE.json
- src/backend/core/tests/mda/test_dispatch_webhooks.py
- src/frontend/public/locales/common/en-US.json
- src/frontend/public/sw.js
- src/frontend/public/locales/common/nl-NL.json
- src/frontend/public/locales/common/fr-FR.json
4a5161c to
240011e
Compare
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compose.yaml`:
- Around line 390-391: Update the frontend-mobile service’s volumes
configuration alongside its existing source mount to include the shared
frontend-node-modules volume, matching the dependency-volume usage of the other
frontend tooling services. Ensure the volume is mounted at the frontend
workspace’s node_modules location so mobile commands can resolve dependencies
without relying on host-installed modules.
In `@src/backend/core/api/serializers.py`:
- Around line 323-336: Update the serializer Meta definition to safely extend
the parent fields regardless of whether they are a tuple or list, and preserve
the parent read_only_fields instead of marking every field read-only so profile
updates continue to work. In get_csrf_token, handle an absent request in
serializer context without raising KeyError, returning the appropriate empty or
unavailable-token value while retaining normal request-based token generation.
In `@src/backend/core/management/commands/derive_vapid_public_key.py`:
- Around line 25-34: Remove the --private-key argument from add_arguments and
update the command to obtain the VAPID private key only from Django settings,
stdin, or a protected file. Eliminate any command logic that reads the
credential from argv while preserving the existing key parsing and derivation
behavior.
In `@src/backend/core/management/commands/generate_vapid_private_key.py`:
- Around line 25-30: Update the output in the management command’s stderr
success message to remove PUSH_VAPID_PRIVATE_KEY and its value entirely. Keep
only the public key and guidance needed to enable Web Push, ensuring the private
key is not emitted to stderr or logs.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 507-517: Wrap the enqueue_push_notifications call in the
created_now and non-spam branch with _safe_finalize, matching the existing
finalize side-effect pattern. Ensure push enqueue exceptions are handled by
_safe_finalize so the task still completes successfully after delivery.
In `@src/backend/core/services/push/apns.py`:
- Around line 125-131: Update the exception handling around _apns_auth_token and
the related auth-failure branch to catch jwt.exceptions.InvalidKeyError
separately, log it as a permanent signing-key failure, and return a non-retry
result for the batch. Preserve the existing PushResult(0, len(items)) fallback
for genuine transient client/cache failures, while changing unexpected exception
handling to logger.exception() so those failures retain their traceback.
In `@src/backend/core/services/push/tasks.py`:
- Around line 57-60: Contain broker publish failures in both push fan-out paths:
in src/backend/core/services/push/tasks.py lines 57-60, update the
transaction.on_commit callback around send_push_for_message.delay so post-commit
exceptions are caught; at lines 126-130, guard each send_push_notification.delay
call independently so a failure for one device does not stop subsequent devices.
In `@src/backend/core/services/push/webpush.py`:
- Around line 63-82: Update _valid_vapid_subject to require a non-empty contact
after the mailto: or https:// prefix, rejecting placeholder values such as bare
mailto: and https:// subjects while preserving acceptance of valid scheme-based
subjects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63b78074-6ae2-49d6-98a9-c2c8eb9c5d20
⛔ Files ignored due to path filters (4)
src/backend/uv.lockis excluded by!**/*.locksrc/frontend/android/app/src/main/res/drawable-xxhdpi/ic_stat_notification.pngis excluded by!**/*.pngsrc/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/public/assets/icons/icon-mono-72.pngis excluded by!**/*.png
📒 Files selected for processing (46)
compose.yamldeploy/env/backend.defaultsdeploy/env/frontend.defaultsdocs/env.mddocs/mobile.mddocs/push-notifications.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/management/commands/derive_vapid_public_key.pysrc/backend/core/management/commands/generate_vapid_private_key.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/migrations/0034_channel_lookup_hash.pysrc/backend/core/models.pysrc/backend/core/services/push/__init__.pysrc/backend/core/services/push/apns.pysrc/backend/core/services/push/common.pysrc/backend/core/services/push/fcm.pysrc/backend/core/services/push/tasks.pysrc/backend/core/services/push/webpush.pysrc/backend/core/signals.pysrc/backend/core/tasks.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/test_push.pysrc/backend/core/tests/test_settings.pysrc/backend/messages/settings.pysrc/backend/pyproject.tomlsrc/frontend/android/app/.gitignoresrc/frontend/android/app/capacitor.build.gradlesrc/frontend/android/app/src/main/AndroidManifest.xmlsrc/frontend/android/app/src/main/res/values-fr/strings.xmlsrc/frontend/android/app/src/main/res/values/strings.xmlsrc/frontend/android/capacitor.settings.gradlesrc/frontend/android/variables.gradlesrc/frontend/capacitor.config.tssrc/frontend/ios/App/App.xcodeproj/project.pbxprojsrc/frontend/ios/App/App/App.entitlementssrc/frontend/ios/App/App/AppDelegate.swiftsrc/frontend/ios/App/App/Info.plistsrc/frontend/ios/App/App/en.lproj/Localizable.stringssrc/frontend/ios/App/App/fr.lproj/Localizable.stringssrc/frontend/ios/App/CapApp-SPM/Package.swiftsrc/frontend/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compose.yaml`:
- Around line 390-391: Update the frontend-mobile service’s volumes
configuration alongside its existing source mount to include the shared
frontend-node-modules volume, matching the dependency-volume usage of the other
frontend tooling services. Ensure the volume is mounted at the frontend
workspace’s node_modules location so mobile commands can resolve dependencies
without relying on host-installed modules.
In `@src/backend/core/api/serializers.py`:
- Around line 323-336: Update the serializer Meta definition to safely extend
the parent fields regardless of whether they are a tuple or list, and preserve
the parent read_only_fields instead of marking every field read-only so profile
updates continue to work. In get_csrf_token, handle an absent request in
serializer context without raising KeyError, returning the appropriate empty or
unavailable-token value while retaining normal request-based token generation.
In `@src/backend/core/management/commands/derive_vapid_public_key.py`:
- Around line 25-34: Remove the --private-key argument from add_arguments and
update the command to obtain the VAPID private key only from Django settings,
stdin, or a protected file. Eliminate any command logic that reads the
credential from argv while preserving the existing key parsing and derivation
behavior.
In `@src/backend/core/management/commands/generate_vapid_private_key.py`:
- Around line 25-30: Update the output in the management command’s stderr
success message to remove PUSH_VAPID_PRIVATE_KEY and its value entirely. Keep
only the public key and guidance needed to enable Web Push, ensuring the private
key is not emitted to stderr or logs.
In `@src/backend/core/mda/inbound_tasks.py`:
- Around line 507-517: Wrap the enqueue_push_notifications call in the
created_now and non-spam branch with _safe_finalize, matching the existing
finalize side-effect pattern. Ensure push enqueue exceptions are handled by
_safe_finalize so the task still completes successfully after delivery.
In `@src/backend/core/services/push/apns.py`:
- Around line 125-131: Update the exception handling around _apns_auth_token and
the related auth-failure branch to catch jwt.exceptions.InvalidKeyError
separately, log it as a permanent signing-key failure, and return a non-retry
result for the batch. Preserve the existing PushResult(0, len(items)) fallback
for genuine transient client/cache failures, while changing unexpected exception
handling to logger.exception() so those failures retain their traceback.
In `@src/backend/core/services/push/tasks.py`:
- Around line 57-60: Contain broker publish failures in both push fan-out paths:
in src/backend/core/services/push/tasks.py lines 57-60, update the
transaction.on_commit callback around send_push_for_message.delay so post-commit
exceptions are caught; at lines 126-130, guard each send_push_notification.delay
call independently so a failure for one device does not stop subsequent devices.
In `@src/backend/core/services/push/webpush.py`:
- Around line 63-82: Update _valid_vapid_subject to require a non-empty contact
after the mailto: or https:// prefix, rejecting placeholder values such as bare
mailto: and https:// subjects while preserving acceptance of valid scheme-based
subjects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63b78074-6ae2-49d6-98a9-c2c8eb9c5d20
⛔ Files ignored due to path filters (4)
src/backend/uv.lockis excluded by!**/*.locksrc/frontend/android/app/src/main/res/drawable-xxhdpi/ic_stat_notification.pngis excluded by!**/*.pngsrc/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/public/assets/icons/icon-mono-72.pngis excluded by!**/*.png
📒 Files selected for processing (46)
compose.yamldeploy/env/backend.defaultsdeploy/env/frontend.defaultsdocs/env.mddocs/mobile.mddocs/push-notifications.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/channel.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/management/commands/derive_vapid_public_key.pysrc/backend/core/management/commands/generate_vapid_private_key.pysrc/backend/core/mda/inbound_tasks.pysrc/backend/core/migrations/0034_channel_lookup_hash.pysrc/backend/core/models.pysrc/backend/core/services/push/__init__.pysrc/backend/core/services/push/apns.pysrc/backend/core/services/push/common.pysrc/backend/core/services/push/fcm.pysrc/backend/core/services/push/tasks.pysrc/backend/core/services/push/webpush.pysrc/backend/core/signals.pysrc/backend/core/tasks.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/mda/test_dispatch_webhooks.pysrc/backend/core/tests/test_push.pysrc/backend/core/tests/test_settings.pysrc/backend/messages/settings.pysrc/backend/pyproject.tomlsrc/frontend/android/app/.gitignoresrc/frontend/android/app/capacitor.build.gradlesrc/frontend/android/app/src/main/AndroidManifest.xmlsrc/frontend/android/app/src/main/res/values-fr/strings.xmlsrc/frontend/android/app/src/main/res/values/strings.xmlsrc/frontend/android/capacitor.settings.gradlesrc/frontend/android/variables.gradlesrc/frontend/capacitor.config.tssrc/frontend/ios/App/App.xcodeproj/project.pbxprojsrc/frontend/ios/App/App/App.entitlementssrc/frontend/ios/App/App/AppDelegate.swiftsrc/frontend/ios/App/App/Info.plistsrc/frontend/ios/App/App/en.lproj/Localizable.stringssrc/frontend/ios/App/App/fr.lproj/Localizable.stringssrc/frontend/ios/App/CapApp-SPM/Package.swiftsrc/frontend/package.json
🛑 Comments failed to post (8)
compose.yaml (1)
390-391: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mount the shared
frontend-node-modulesvolume.
frontend-mobileinherits the lean image with no dependencies baked in, but unlike the other frontend tooling services it does not consume the volume populated bymake install-frozen-front. Mobile commands will fail when the host has nonode_modules.Proposed fix
volumes: - ./src/frontend/:/home/frontend/ + - frontend-node-modules:/home/frontend/node_modules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.volumes: - ./src/frontend/:/home/frontend/ - frontend-node-modules:/home/frontend/node_modules🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compose.yaml` around lines 390 - 391, Update the frontend-mobile service’s volumes configuration alongside its existing source mount to include the shared frontend-node-modules volume, matching the dependency-volume usage of the other frontend tooling services. Ensure the volume is mounted at the frontend workspace’s node_modules location so mobile commands can resolve dependencies without relying on host-installed modules.src/backend/core/api/serializers.py (1)
323-336: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix
Metainheritance bugs and handle missing request context.There are three issues in this implementation that will cause runtime crashes and silently break profile updates:
UserSerializer.Meta.fields + ["csrf_token"]will raise aTypeErroriffieldswas defined as a tuple in the parentUserSerializer.read_only_fields = fieldsmakes all fields read-only. If this serializer is ever used forPATCHorPUTendpoints, it will silently ignore all user profile updates.self.context["request"]raises aKeyErrorif the serializer is instantiated outside of a standard HTTP view context (e.g., in background tasks, tests, or WebSockets).🐛 Proposed fixes
class Meta(UserSerializer.Meta): - fields = UserSerializer.Meta.fields + ["csrf_token"] - read_only_fields = fields + fields = list(UserSerializer.Meta.fields) + ["csrf_token"] + read_only_fields = list(getattr(UserSerializer.Meta, "read_only_fields", [])) + ["csrf_token"] def get_csrf_token(self, _instance) -> str: """Return the session-bound CSRF token for the SPA to echo as X-CSRFToken. With ``CSRF_USE_SESSIONS`` the secret lives in the session and is no longer exposed as a cookie, so the token is delivered here over the authenticated ``/users/me/`` channel. """ - return get_token(self.context["request"]) + request = self.context.get("request") + if not request: + return "" + return get_token(request)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.csrf_token = serializers.SerializerMethodField(read_only=True) class Meta(UserSerializer.Meta): fields = list(UserSerializer.Meta.fields) + ["csrf_token"] read_only_fields = list(getattr(UserSerializer.Meta, "read_only_fields", [])) + [ "csrf_token" ] def get_csrf_token(self, _instance) -> str: """Return the session-bound CSRF token for the SPA to echo as X-CSRFToken. With ``CSRF_USE_SESSIONS`` the secret lives in the session and is no longer exposed as a cookie, so the token is delivered here over the authenticated ``/users/me/`` channel. """ request = self.context.get("request") if not request: return "" return get_token(request)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/api/serializers.py` around lines 323 - 336, Update the serializer Meta definition to safely extend the parent fields regardless of whether they are a tuple or list, and preserve the parent read_only_fields instead of marking every field read-only so profile updates continue to work. In get_csrf_token, handle an absent request in serializer context without raising KeyError, returning the appropriate empty or unavailable-token value while retaining normal request-based token generation.src/backend/core/management/commands/derive_vapid_public_key.py (1)
25-34: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not accept the VAPID private key through argv.
--private-keycan expose the credential through shell history and process listings. Read it from Django settings, stdin, or a protected file instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/management/commands/derive_vapid_public_key.py` around lines 25 - 34, Remove the --private-key argument from add_arguments and update the command to obtain the VAPID private key only from Django settings, stdin, or a protected file. Eliminate any command logic that reads the credential from argv while preserving the existing key parsing and derivation behavior.src/backend/core/management/commands/generate_vapid_private_key.py (1)
25-30: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not duplicate the VAPID private key on stderr.
This defeats the pipe-friendly separation: redirected stdout is safe, but stderr commonly reaches terminal or CI logs. Print only the public key and instructions there.
Proposed fix
self.style.SUCCESS( "Set the following env vars to enable Web Push:\n" - f" PUSH_VAPID_PRIVATE_KEY={private_b64}\n" + " PUSH_VAPID_PRIVATE_KEY=<value written to stdout>\n" f" PUSH_VAPID_PUBLIC_KEY={public_b64}\n" "Keep the private key secret." )As per coding guidelines, “Do not log sensitive information (tokens, passwords, financial/health data, PII).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.self.stderr.write( self.style.SUCCESS( "Set the following env vars to enable Web Push:\n" " PUSH_VAPID_PRIVATE_KEY=<value written to stdout>\n" f" PUSH_VAPID_PUBLIC_KEY={public_b64}\n" "Keep the private key secret."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/management/commands/generate_vapid_private_key.py` around lines 25 - 30, Update the output in the management command’s stderr success message to remove PUSH_VAPID_PRIVATE_KEY and its value entirely. Keep only the public key and guidance needed to enable Web Push, ensuring the private key is not emitted to stderr or logs.Source: Coding guidelines
src/backend/core/mda/inbound_tasks.py (1)
507-517: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap push enqueue in
_safe_finalizeto prevent task failures on side-effect errors.If
enqueue_push_notificationsraises an exception (e.g., a broker connection issue), the exception will bubble up and cause the task to return{"success": False}without logging the final success message. Since theInboundMessagequeue row has already been deleted, the task won't be retried anyway.To ensure consistency with the other finalize steps and prevent false-positive task failures for successfully delivered messages, wrap this call in
_safe_finalize.🛡️ Proposed fix
- # Truly last step: fire-and-forget push now that the message is - # fully delivered. Gated on `created_now` like every other side - # effect above: on a dedup hit (SMTP retry, greylisting) the push - # already fired for the original create and would otherwise re-alert - # the device — `enqueue_push_notifications` has no idempotency of - # its own. Spam is skipped: no point waking a device for it. - # `enqueue_push_notifications` already no-ops when push is - # disabled ("safe to call unconditionally"), so no extra gate here. - if created_now and not ctx.is_spam: - enqueue_push_notifications(inbound_msg) + if created_now: + # Truly last step: fire-and-forget push now that the message is + # fully delivered. Gated on `created_now` like every other side + # effect above: on a dedup hit (SMTP retry, greylisting) the push + # already fired for the original create and would otherwise re-alert + # the device — `enqueue_push_notifications` has no idempotency of + # its own. Spam is skipped: no point waking a device for it. + # `enqueue_push_notifications` already no-ops when push is + # disabled ("safe to call unconditionally"), so no extra gate here. + _safe_finalize( + "push", + inbound_message_id, + not ctx.is_spam, + lambda: enqueue_push_notifications(inbound_msg), + )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if created_now: # Truly last step: fire-and-forget push now that the message is # fully delivered. Gated on `created_now` like every other side # effect above: on a dedup hit (SMTP retry, greylisting) the push # already fired for the original create and would otherwise re-alert # the device — `enqueue_push_notifications` has no idempotency of # its own. Spam is skipped: no point waking a device for it. # `enqueue_push_notifications` already no-ops when push is # disabled ("safe to call unconditionally"), so no extra gate here. _safe_finalize( "push", inbound_message_id, not ctx.is_spam, lambda: enqueue_push_notifications(inbound_msg), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/mda/inbound_tasks.py` around lines 507 - 517, Wrap the enqueue_push_notifications call in the created_now and non-spam branch with _safe_finalize, matching the existing finalize side-effect pattern. Ensure push enqueue exceptions are handled by _safe_finalize so the task still completes successfully after delivery.src/backend/core/services/push/apns.py (1)
125-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C4 \ 'InvalidKeyError|provider-token mint failed|APNs batch send failed' \ src/backend/core/services/push/apns.py \ src/backend/core/tests/test_push.pyRepository: suitenumerique/messages
Length of output: 1617
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## apns.py (relevant ranges)\n' sed -n '1,260p' src/backend/core/services/push/apns.py | cat -n | sed -n '110,240p' printf '\n## tests around APNs push handling\n' sed -n '1,260p' src/backend/core/tests/test_push.py | cat -n | sed -n '1,260p' printf '\n## logger.exception usage in push code\n' rg -n 'logger\.exception\(' src/backend/core/services/push src/backend/core/testsRepository: suitenumerique/messages
Length of output: 18781
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## apns.py headers/imports and helpers\n' sed -n '1,120p' src/backend/core/services/push/apns.py | cat -n printf '\n## apns.py later batch-send section\n' sed -n '180,250p' src/backend/core/services/push/apns.py | cat -n printf '\n## push tests mentioning APNs, exceptions, or transient/retry\n' rg -n -C3 'APNs|apns|transient|retry|InvalidKeyError|exception\(' src/backend/core/tests/test_push.pyRepository: suitenumerique/messages
Length of output: 29930
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' src/backend/core/services/push/apns.py | cat -n | sed -n '1,260p'Repository: suitenumerique/messages
Length of output: 10565
Split permanent APNs auth failures from transient retries
src/backend/core/services/push/apns.py:125-131,219-222Catchjwt.exceptions.InvalidKeyErrorseparately so a malformed signing key doesn’t retry the whole batch. Keep the transient fallback for genuine client/cache failures, but switch unexpected branches tologger.exception()so they surface instead of being downgraded to warnings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/apns.py` around lines 125 - 131, Update the exception handling around _apns_auth_token and the related auth-failure branch to catch jwt.exceptions.InvalidKeyError separately, log it as a permanent signing-key failure, and return a non-retry result for the batch. Preserve the existing PushResult(0, len(items)) fallback for genuine transient client/cache failures, while changing unexpected exception handling to logger.exception() so those failures retain their traceback.Sources: Coding guidelines, Learnings
src/backend/core/services/push/tasks.py (1)
57-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/backend/core/services/push/tasks.pyRepository: suitenumerique/messages
Length of output: 8152
🏁 Script executed:
rg -n "enqueue_push_notifications|send_push_for_message|send_push_notification\.delay|on_commit\(" src/backend/core -SRepository: suitenumerique/messages
Length of output: 4440
🏁 Script executed:
sed -n '580,700p' src/backend/core/tests/test_push.pyRepository: suitenumerique/messages
Length of output: 5444
🏁 Script executed:
sed -n '1,160p' src/backend/core/services/push/__init__.pyRepository: suitenumerique/messages
Length of output: 4455
Contain publish failures in the push fan-out path.
src/backend/core/services/push/tasks.py#L57-L60:transaction.on_commit()only guards registration; ifsend_push_for_message.delay()fails after commit, the exception still escapes. Catch it inside the callback.src/backend/core/services/push/tasks.py#L126-L130: guard eachsend_push_notification.delay()call so one broker failure doesn’t abort the rest of the devices.📍 Affects 1 file
src/backend/core/services/push/tasks.py#L57-L60(this comment)src/backend/core/services/push/tasks.py#L126-L130🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/tasks.py` around lines 57 - 60, Contain broker publish failures in both push fan-out paths: in src/backend/core/services/push/tasks.py lines 57-60, update the transaction.on_commit callback around send_push_for_message.delay so post-commit exceptions are caught; at lines 126-130, guard each send_push_notification.delay call independently so a failure for one device does not stop subsequent devices.src/backend/core/services/push/webpush.py (1)
63-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the declared py-vapid version and locate subject-validation tests. fd -a 'pyproject.toml|uv.lock' -x rg -n -C2 'py-vapid|py_vapid' {} rg -n -C3 '_valid_vapid_subject|PUSH_VAPID_SUBJECT' src/backendRepository: suitenumerique/messages
Length of output: 9841
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant webpush implementation and settings validation. sed -n '1,280p' src/backend/core/services/push/webpush.py printf '\n===== settings =====\n' sed -n '1658,1688p' src/backend/messages/settings.py printf '\n===== tests =====\n' sed -n '1278,1305p' src/backend/core/tests/test_push.pyRepository: suitenumerique/messages
Length of output: 14511
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for any stronger VAPID subject validation or Safari-specific handling. rg -n -C3 "Safari|routable|localhost|\.local|VAPID subject|_valid_vapid_subject|PUSH_VAPID_SUBJECT" src/backendRepository: suitenumerique/messages
Length of output: 50380
🌐 Web query:
py-vapid VAPID subject validation mailto https subject scheme only signing 401💡 Result:
In the VAPID (Voluntary Application Server Identification) standard, the sub (subject) claim must be a URI that identifies the application server [1]. According to RFC 8292, this URI should be either a mailto: email address or an https: URL [1]. If you are encountering 401 Unauthorized errors when using py-vapid, ensure that your sub claim is correctly formatted as a full URI with the required scheme [2][3]. The library expects the string to be prefixed with the scheme [2][3]. Key Validation Details: 1. Scheme Requirement: The sub claim must explicitly include either mailto: or https: [1][3]. Using only an email address without the mailto: prefix will result in validation failure [3][4]. 2. Validation Logic: The py-vapid library includes regex-based validation for these fields [4]. Historically, this validation has been strict [5][4]. For example, the library previously enforced the presence of a dot in the email host part (e.g., rejecting user@localhost) [5], though this was addressed in later updates. 3. Common 401 Causes: - Missing Scheme: Providing just an email (e.g., admin@example.com) instead of the required URI (e.g., mailto:admin@example.com) [2][3]. - Double Prefixing: Some implementations might mistakenly add the mailto: prefix twice if your input already includes it [6]. Verify that your final JWT claims do not contain redundant prefixes like mailto:mailto: [6]. - Audience (aud) mismatch: Ensure your aud claim matches the scheme and host of the push service endpoint exactly, as this is also a frequent cause of 401 errors [2][3]. If you suspect your configuration is correct but validation still fails, check that you are not using an unsupported scheme or a malformed URI that fails the underlying regex or URL parser logic [5][4][7]. Reference your specific version of py-vapid, as validation strictness has evolved across releases [5][8][4].
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc8292
- 2: https://pypi.org/project/py-vapid/1.9.2/
- 3: https://github.com/web-push-libs/vapid/tree/main/python
- 4: web-push-libs/vapid@a6383f2
- 5: web-push-libs/vapid#90
- 6: SherClockHolmes/webpush-go#92
- 7: web-push-libs/web-push#789
- 8: https://github.com/web-push-libs/vapid/blob/main/python/py_vapid/main.py
🌐 Web query:
py-vapid subject validation mailto https URI subject empty path hostname💡 Result:
The py-vapid library includes validation logic for the 'sub' (Subject) claim in VAPID tokens to ensure it meets the requirements specified by RFC 8292 [1][2]. RFC 8292 states that the 'sub' claim should be a contact URI, specifically either a 'mailto:' email URI or an 'https:' URI [1][3]. In the py-vapid library, the 'sub' claim is validated using a regular expression that checks for these specific URI schemes [2]: 1. mailto: The library expects a 'mailto:' prefix followed by an email address [4][2]. The validation logic includes checks for the hostname part of the email address (e.g., ensuring it supports formats like 'admin@localhost' or standard domain structures) [5][6][2]. 2. https: The library also validates 'https:' URIs [2]. The regex used for validation requires an 'https://' prefix followed by a valid host [2]. It is designed to reject URIs that are malformed or missing the required scheme [2]. The library enforces these checks by default to ensure the 'sub' claim provides valid contact information, which is intended for use by operations teams if issues arise with a subscription [5][7][6]. If you need to bypass these strict validation checks, the library provides a 'no-strict' option that disables the 'sub' claim validation, although this is discouraged as it undermines the purpose of the 'sub' claim [5][2]. Top results: [5][1][2]
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc8292
- 2: https://github.com/web-push-libs/vapid/blob/main/python/py_vapid/__init__.py
- 3: https://www.rfc-editor.org/rfc/rfc8292.html
- 4: https://github.com/web-push-libs/vapid/blob/main/python/py_vapid/main.py
- 5: web-push-libs/vapid#92
- 6: web-push-libs/vapid#90
- 7: https://github.com/web-push-libs/vapid/tree/main/python
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pprint import pprint try: from py_vapid import Vapid02 print("py_vapid imported") print("Vapid02 attrs sample:", [a for a in dir(Vapid02) if not a.startswith("_")][:30]) # Try to create a usable instance. for factory_name in ("generate", "from_raw", "from_pem"): factory = getattr(Vapid02, factory_name, None) print(f"{factory_name}:", bool(factory)) # Try common construction paths and sign with malformed subjects. candidates = [] if hasattr(Vapid02, "generate"): try: candidates.append(("generate()", Vapid02.generate())) except Exception as e: print("generate() failed:", type(e).__name__, e) # If generate() is unavailable, try raw key creation from a zero scalar. if not candidates and hasattr(Vapid02, "from_raw"): try: candidates.append(("from_raw(zero32)", Vapid02.from_raw(b"\x01" * 32))) except Exception as e: print("from_raw() failed:", type(e).__name__, e) for label, vapid in candidates: print("instance:", label, type(vapid)) for sub in ["mailto:", "https://", "mailto:ops@example.com", "https://example.com"]: try: out = vapid.sign({"aud": "https://push.example", "sub": sub, "exp": 2000000000}) print("SIGN OK", sub, sorted(out)) except Exception as e: print("SIGN FAIL", sub, type(e).__name__, e) except Exception as e: print("IMPORT FAIL", type(e).__name__, e) PYRepository: suitenumerique/messages
Length of output: 220
Reject empty VAPID subjects, not just scheme prefixes.
mailto:/https://placeholders still pass this check, so a badPUSH_VAPID_SUBJECTcan reach Web Push and fail later instead of being caught at boot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/push/webpush.py` around lines 63 - 82, Update _valid_vapid_subject to require a non-empty contact after the mailto: or https:// prefix, rejecting placeholder values such as bare mailto: and https:// subjects while preserving acceptance of valid scheme-based subjects.
2 participants
Summary by CodeRabbit
New Features
Documentation