feat(notifications): OS push delivery via FCM — device tokens + broadcaster sink#1442
feat(notifications): OS push delivery via FCM — device tokens + broadcaster sink#1442semen-flamingo wants to merge 16 commits into
Conversation
…aster sink) Extends NotificationBroadcaster, until now socket-only, with a push sink, and adds the device-token store push depends on. - PushDevice (collection push_devices — NOT devices, that is RMM machines) + repository: register upserts by token and RE-ASSOCIATES it to the caller, so a phone handed to another user after logout stops receiving the previous user's pushes. - PushSender port in openframe-data-nats, deliberately mirroring publishToUser(...), so the broadcaster stays dumb and no provider SDK leaks into a module half the platform depends on. A second provider later is a new implementation, not a broadcaster change. - New opt-in module openframe-notification-push (mirrors openframe-notification-mail) holding firebase-admin and FcmPushSender: multicast to all of a user's devices, and prune tokens only on permanent errors (UNREGISTERED / INVALID_ARGUMENT / SENDER_ID_MISMATCH) — a transient UNAVAILABLE must not cost a user their registration. - Auth is Application Default Credentials, not a service-account key: the GCP org enforces iam.disableServiceAccountKeyCreation, so no key can even be issued. The same code resolves Workload Identity in GKE and gcloud ADC locally; nothing to store, rotate or leak. - The data payload carries the whole serialized context (id/type/category/severity too), so the client can change its deep-link routing without a backend release. Oversized contexts are dropped rather than losing the push to FCM's ~4KB limit. - Push is a sink of its own, not a NATS fallback: it fires even with NATS disabled, because sockets reach a foreground client and push reaches a backgrounded one. Humans only — machines are agents, not phones. Best-effort throughout: a dead provider never fails the notification or the caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
📝 WalkthroughWalkthroughAdds tenant-scoped push-device persistence, a notification channel abstraction, optional broadcaster delivery, and a Firebase Cloud Messaging module with configuration, multicast sending, payload limits, dead-token cleanup, and tests. ChangesPush notification delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NotificationBroadcaster
participant FcmPushSender
participant PushDeviceRepository
participant FirebaseMessaging
NotificationBroadcaster->>FcmPushSender: deliver user notification
FcmPushSender->>PushDeviceRepository: findByUserId
PushDeviceRepository-->>FcmPushSender: device tokens
FcmPushSender->>FirebaseMessaging: sendEachForMulticast
FirebaseMessaging-->>FcmPushSender: token responses
FcmPushSender->>PushDeviceRepository: removeTokens for dead tokens
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.java (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using Spring's standard validation for configuration properties.
Currently, the
projectIdis checked programmatically inFcmConfig. Using@Validatedand@NotBlankis the idiomatic Spring Boot approach to fail fast on invalid properties during binding.♻️ Proposed refactor
import lombok.Data; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; +import jakarta.validation.constraints.NotBlank; `@Data` +@Validated `@ConfigurationProperties`(prefix = "openframe.push.fcm") public class FcmProperties { /** * Firebase project the pushes are sent from — differs per environment (dev/stage/prod each have * their own Firebase project, because a token issued for one project is meaningless to another). * Required: user-scoped Application Default Credentials carry no project, so it cannot be inferred. */ + `@NotBlank`(message = "openframe.push.fcm.project-id must be set when push is enabled") private String projectId;🤖 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 `@openframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.java` around lines 6 - 15, Update FcmProperties to enable Spring configuration-property validation with `@Validated` and mark projectId as `@NotBlank`, including the required validation imports. Remove the redundant programmatic projectId check from FcmConfig while preserving the existing valid configuration behavior.
🤖 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
`@openframe-data-nats/src/main/java/com/openframe/data/nats/service/NotificationBroadcaster.java`:
- Around line 88-96: Update NotificationBroadcaster’s push delivery loop to
offload per-user sendToUser calls from the broadcast thread using the class’s
existing or a dedicated background ExecutorService. Preserve best-effort
behavior and pushSafely error handling, while ensuring broadcast returns without
waiting for O(N) synchronous network calls.
In
`@openframe-notification-push/src/main/java/com/openframe/notification/push/FcmPushSender.java`:
- Around line 72-80: The FcmPushSender message-building flow must not pass more
than 500 tokens to MulticastMessage.builder().addAllTokens. Split the devices or
token list into chunks of at most 500 and build/send a multicast message for
each chunk, ensuring all devices are notified and the existing notification/data
fields are preserved for every batch.
---
Nitpick comments:
In
`@openframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.java`:
- Around line 6-15: Update FcmProperties to enable Spring configuration-property
validation with `@Validated` and mark projectId as `@NotBlank`, including the
required validation imports. Remove the redundant programmatic projectId check
from FcmConfig while preserving the existing valid configuration behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9df775da-33e3-460b-8261-ed2b6aa924fe
📒 Files selected for processing (14)
openframe-data-mongo-common/src/main/java/com/openframe/data/document/push/PushDevice.javaopenframe-data-mongo-common/src/main/java/com/openframe/data/document/push/PushPlatform.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/CustomPushDeviceRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/PushDeviceRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/impl/CustomPushDeviceRepositoryImpl.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/push/PushSender.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/service/NotificationBroadcaster.javaopenframe-data-nats/src/test/java/com/openframe/data/nats/service/NotificationBroadcasterTest.javaopenframe-notification-push/pom.xmlopenframe-notification-push/src/main/java/com/openframe/notification/push/FcmConfig.javaopenframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.javaopenframe-notification-push/src/main/java/com/openframe/notification/push/FcmPushSender.javaopenframe-notification-push/src/test/java/com/openframe/notification/push/FcmPushSenderTest.javapom.xml
| // Push is a sink of its own, not a fallback: it fires whether or not NATS is wired, because the | ||
| // two cover different states — sockets reach a foreground client, push reaches a backgrounded or | ||
| // killed one. Only human recipients get it; machines are agents, not phones. | ||
| pushSender.ifPresent(sender -> { | ||
| for (String userId : admins) { | ||
| pushSafely(() -> sender.sendToUser(userId, saved, category), saved.getId(), userId); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔴 Critical | 🏗️ Heavy lift
Prevent O(N) blocking network calls on the broadcast thread.
Iterating over admins and synchronously calling pushSender.sendToUser for each user will block the current thread for O(N) network round-trips to the push provider (e.g., Firebase). If broadcast is executed on an HTTP request thread or an event loop, this will severely degrade throughput, cause high latency, and potentially lead to upstream timeouts when the audience is large.
Consider offloading this loop to a background ExecutorService (or firing an internal async event) so that best-effort push delivery does not block the primary broadcast flow. Alternatively, the PushSender interface and its implementations could be refactored to use asynchronous provider APIs (e.g., sendEachForMulticastAsync).
🤖 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
`@openframe-data-nats/src/main/java/com/openframe/data/nats/service/NotificationBroadcaster.java`
around lines 88 - 96, Update NotificationBroadcaster’s push delivery loop to
offload per-user sendToUser calls from the broadcast thread using the class’s
existing or a dedicated background ExecutorService. Preserve best-effort
behavior and pushSafely error handling, while ensuring broadcast returns without
waiting for O(N) synchronous network calls.
…race
An upsert is not atomic against a unique index: two concurrent registrations of
the same token (a client retry, a double tap) can both miss the filter and both
attempt an insert, and exactly one wins the {tenantId, token} index — the loser
surfaced as a 500 on registration. Retry once; the row now exists, so the retry
takes the update branch instead of racing again.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
…pdate; trim javadoc Retrying the upsert on a duplicate key just races again. The catch block already knows the row exists — the winner of the insert race put it there — so a plain update re-associates it and cannot race. Two calls, no retry loop. Also cuts the javadoc back to the constraints the code cannot show. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
MulticastMessage.build() throws above 500 tokens, before anything is sent. Dead tokens accumulate when a client never unregisters, and pruning only runs AFTER a send — so a user who crossed the cap would stop receiving pushes forever, with the very tokens causing it never cleaned up, and the broadcaster swallowing the exception silently. Chunk instead; truncating would drop devices just as quietly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
… devices, generalise the sink to channels The feature was dead on arrival, and a code review caught it: FcmPushSender was a @component in com.openframe.notification.push, but the only service that broadcasts scans com.openframe.{ai.agent,data,core,api,...}. The bean was never created, so Optional<PushSender> was always empty and every push silently no-opped. Register the module through META-INF/spring/...AutoConfiguration.imports, the convention every other lib module here already follows, so consuming services need no change. Also caught, and worse in production: - INVALID_ARGUMENT was treated as a dead token, but FCM maps EVERY HTTP 400 to it — including an oversized payload, which fails for every token in the batch. One fat chat message would therefore have deleted every device a user owns. Only UNREGISTERED and SENDER_ID_MISMATCH can mean a dead token; payload size is bounded instead (title/body are now truncated, and the caps sum under FCM's 4KB limit). - The FCM call runs inline on the caller's request thread and had no timeouts, so a hung provider would hang the endpoint that triggered the notification — pushSafely isolates exceptions, not latency. Connect/read timeouts are now set. - The new module was missing from the CI paths-filter, so module-only changes would have skipped both the test job and the release build. - The duplicate-key fallback ignored matchedCount: if the row that won the insert race was deleted in between, the registration was silently dropped. It now inserts. And the sink is now a channel, per Dmytro: NotificationBroadcaster takes List<NotificationChannel> rather than a single Optional<PushSender>, so Slack (and the email service that already exists) is a new bean in its own module with no change here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
…startup, drop unscoped repository methods Two things your review of FcmProperties shook out. The budgets were in characters, which is the wrong unit: FCM's limit is a byte limit, and a CJK or emoji title costs up to four bytes per character — so the caps either wasted the payload or blew past it depending on the language. They are bytes now, truncation cuts on a code-point boundary, and their sum is validated at startup: raise one on its own and the context fails with a message naming the budgets, instead of FCM rejecting every push with INVALID_ARGUMENT for every device at once and nothing in our logs pointing at the config. Timeouts are Durations rather than raw millis. PushDeviceRepository no longer extends MongoRepository. Per the multi-tenancy guide, SimpleMongoRepository's count()/findAll()/findById()/existsById()/deleteById() pass a collection name rather than an entity class, so TenantAwareMongoTemplate cannot scope them — and the Layer-2 base class that is supposed to fix that does not exist in the code yet. Every operation this repository needs is in the custom fragment and goes through the scoped template, so the unscoped inherited methods are simply not exposed: nobody can call findAll() and get every tenant's devices. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
The check was an explicit call inside the @bean method: it did run at startup, but nothing in FcmProperties said so — the method read as dead code — and under global lazy-initialization it would have moved from deploy time to the first push under load. It is now afterPropertiesSet(), so the container guarantees it. The new test boots a real context with an over-budget config and asserts the context FAILS, which also covers the auto-configuration itself: that it loads, that the feature flag gates it, and that the properties bind. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
…ngoRepository I claimed the Layer-2 base class did not exist. It does — TenantScopedRepositoryImpl, in openframe-saas-lib — and it correctly re-implements count/findAll/findById/existsById/ deleteById to pass the entity class so TenantAwareMongoTemplate can scope them. The conclusion stands for a different reason: nothing in production wires it. repositoryBaseClass is set in exactly one place, TenantIsolationIntegrationTest, on a locally built factory. TenantAwareSyncConfig — the config every tenant service actually uses — declares @EnableMongoRepositories(basePackages = "com.openframe.data.repository") with no repositoryBaseClass, so tenant repositories run on the stock SimpleMongoRepository and those five methods are unscoped today. Not depending on that ever being wired: the methods stay unexposed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
…e repository rationale I overstated the tenant-isolation gap. Reading SimpleMongoRepository's bytecode (spring-data-mongodb 4.3.2): findAll(), findById(), existsById() and deleteById() all pass the entity class, so TenantAwareMongoTemplate scopes them — production is fine, and userRepository.findAll() has always been correct. The single method it cannot scope is count(), which calls count(Query, String) without an entity class — exactly and only what the guide says. Nothing in production calls count() on a tenant repository, so the gap is latent, not live. The repository still exposes no stock CRUD, but for the honest reason: it needs none of it, and not inheriting is simpler than reasoning about which method is safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
…ticate at all openframe-data-mongo-sync pulls google-cloud-storage:2.36.1, which drags google-auth-library-credentials:1.23.0 at a shallower depth than firebase-admin's 1.43.0. Maven picked 1.23.0, splitting google-auth in half: google-auth-library-oauth2-http:1.43.0 then died with NoClassDefFoundError: com/google/auth/CredentialTypeForMetrics on the first token refresh — i.e. every push failed before it left the JVM. Every test mocks FirebaseMessaging, so nothing caught it. Adds two tests that do: GoogleAuthClasspathTest (CI-safe, fails without the pin) and FcmLiveSmokeTest (env-gated, drives real ADC -> FCM and asserts the token is rejected, proving the credentials were accepted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKdcCJmnHipDNZe1qYqp9H
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openframe-notification-push/src/test/java/com/openframe/notification/push/FcmLiveSmokeTest.java (1)
28-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deleting the
FirebaseAppinstance after the test completes.
FirebaseApp.initializeAppspawns daemon threads and maintains SDK state. To prevent resource leaks or potential conflicts if this test is ever run alongside other live integrations in the same JVM, it's a good hygiene practice to clean up the application instance once the test finishes.♻️ Proposed refactor
void adc_authenticates_against_real_fcm() throws Exception { FirebaseApp app = FirebaseApp.initializeApp(FirebaseOptions.builder() .setCredentials(GoogleCredentials.getApplicationDefault()) .setProjectId(System.getenv().getOrDefault("FCM_PROJECT_ID", "flamingo-271f8")) .build(), "live-smoke"); - BatchResponse response = FirebaseMessaging.getInstance(app).sendEachForMulticast( - MulticastMessage.builder() - .addAllTokens(List.of("definitely-not-a-real-fcm-token")) - .putData("notificationId", "smoke") - .build()); - - SendResponse only = response.getResponses().get(0); - assertThat(only.isSuccessful()).isFalse(); - assertThat(only.getException().getMessagingErrorCode()).isEqualTo(MessagingErrorCode.INVALID_ARGUMENT); - assertThat(only.getException()).hasMessageContaining("registration token"); + try { + BatchResponse response = FirebaseMessaging.getInstance(app).sendEachForMulticast( + MulticastMessage.builder() + .addAllTokens(List.of("definitely-not-a-real-fcm-token")) + .putData("notificationId", "smoke") + .build()); + + SendResponse only = response.getResponses().get(0); + assertThat(only.isSuccessful()).isFalse(); + assertThat(only.getException().getMessagingErrorCode()).isEqualTo(MessagingErrorCode.INVALID_ARGUMENT); + assertThat(only.getException()).hasMessageContaining("registration token"); + } finally { + app.delete(); + } }🤖 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 `@openframe-notification-push/src/test/java/com/openframe/notification/push/FcmLiveSmokeTest.java` around lines 28 - 44, Update adc_authenticates_against_real_fcm to delete the FirebaseApp instance after the messaging assertions complete, ensuring cleanup runs even when the test fails by using the test’s existing exception-safe cleanup pattern or a finally block.openframe-notification-push/src/test/java/com/openframe/notification/push/GoogleAuthClasspathTest.java (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake version extraction resilient to flattened JAR distributions.
The current regex extracts the version from the directory path. If the test runs in an environment where dependencies are flattened into a single directory (e.g., inside an executable Spring Boot JAR or a container's
lib/directory), the regex will fail to match the directory structure, returning the full path and falsely failing the assertion.Consider extracting the version directly from the JAR filename to make the test more robust against different runtime environments.
♻️ Proposed refactor
private static String jarOf(Class<?> type) { URL location = type.getProtectionDomain().getCodeSource().getLocation(); - return location.getPath().replaceAll(".*/google-auth-library-[a-z2-]+/([^/]+)/.*", "$1"); + return location.getPath().replaceAll(".*/google-auth-library-[a-z2-]+-(.*?)\\.jar.*", "$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 `@openframe-notification-push/src/test/java/com/openframe/notification/push/GoogleAuthClasspathTest.java` around lines 24 - 27, Update GoogleAuthClasspathTest.jarOf to derive the google-auth-library version from the resolved JAR filename rather than assuming a versioned directory path. Preserve the existing version return behavior while supporting flattened dependency layouts and avoiding a full-path fallback when the directory structure does not match.
🤖 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 `@openframe-notification-push/pom.xml`:
- Around line 38-42: Review the transitive dependencies resolved by the
firebase-admin.version property, rather than treating the google-auth.version
pin as sufficient. Update the Firebase Admin dependency or its dependency
management so the resolved google-auth-library-credentials and related
transitive artifacts are vulnerability-free, while preserving version
compatibility with firebase-admin.
In
`@openframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.java`:
- Around line 41-49: Update the budget validation in FcmProperties so
maxTitleBytes, maxBodyBytes, and maxContextBytes are rejected when negative,
then calculate worstCase using long arithmetic before comparing it with
FCM_PAYLOAD_LIMIT_BYTES. Preserve the existing exception behavior and diagnostic
values while ensuring oversized sums cannot overflow and bypass the limit.
---
Nitpick comments:
In
`@openframe-notification-push/src/test/java/com/openframe/notification/push/FcmLiveSmokeTest.java`:
- Around line 28-44: Update adc_authenticates_against_real_fcm to delete the
FirebaseApp instance after the messaging assertions complete, ensuring cleanup
runs even when the test fails by using the test’s existing exception-safe
cleanup pattern or a finally block.
In
`@openframe-notification-push/src/test/java/com/openframe/notification/push/GoogleAuthClasspathTest.java`:
- Around line 24-27: Update GoogleAuthClasspathTest.jarOf to derive the
google-auth-library version from the resolved JAR filename rather than assuming
a versioned directory path. Preserve the existing version return behavior while
supporting flattened dependency layouts and avoiding a full-path fallback when
the directory structure does not match.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 660c220c-839c-4684-a4d0-f6ba17329fa3
📒 Files selected for processing (14)
openframe-data-mongo-common/src/main/java/com/openframe/data/document/push/PushDevice.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/CustomPushDeviceRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/PushDeviceRepository.javaopenframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/impl/CustomPushDeviceRepositoryImpl.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/channel/NotificationChannel.javaopenframe-data-nats/src/main/java/com/openframe/data/nats/service/NotificationBroadcaster.javaopenframe-notification-push/pom.xmlopenframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.javaopenframe-notification-push/src/main/java/com/openframe/notification/push/FcmPushSender.javaopenframe-notification-push/src/main/java/com/openframe/notification/push/PushAutoConfiguration.javaopenframe-notification-push/src/test/java/com/openframe/notification/push/FcmLiveSmokeTest.javaopenframe-notification-push/src/test/java/com/openframe/notification/push/FcmPropertiesTest.javaopenframe-notification-push/src/test/java/com/openframe/notification/push/FcmPushSenderTest.javaopenframe-notification-push/src/test/java/com/openframe/notification/push/GoogleAuthClasspathTest.java
💤 Files with no reviewable changes (2)
- openframe-data-mongo-common/src/main/java/com/openframe/data/document/push/PushDevice.java
- openframe-notification-push/src/test/java/com/openframe/notification/push/FcmPushSenderTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- openframe-data-nats/src/main/java/com/openframe/data/nats/channel/NotificationChannel.java
- openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/CustomPushDeviceRepository.java
- openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/push/impl/CustomPushDeviceRepositoryImpl.java
- openframe-data-nats/src/main/java/com/openframe/data/nats/service/NotificationBroadcaster.java
- openframe-notification-push/src/main/java/com/openframe/notification/push/FcmPushSender.java
| <properties> | ||
| <firebase-admin.version>9.9.0</firebase-admin.version> | ||
| <!-- Must match what firebase-admin's google-auth-library-oauth2-http resolves to. --> | ||
| <google-auth.version>1.43.0</google-auth.version> | ||
| </properties> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Are there any known security vulnerabilities or CVEs for com.google.firebase:firebase-admin version 9.9.0 or com.google.auth:google-auth-library-credentials version 1.43.0?
💡 Result:
As of July 14, 2026, the status of security vulnerabilities for the specified versions is as follows: For com.google.firebase:firebase-admin version 9.9.0: Authoritative repository data indicates that there are known vulnerabilities associated with this version [1]. These vulnerabilities typically stem from the package's transitive dependencies [1]. Users are advised to check the specific CVE identifiers listed on the Maven Repository artifact page for version 9.9.0 to understand the impact and remediation steps [1]. For com.google.auth:google-auth-library-credentials version 1.43.0: There are no known vulnerabilities reported directly for this specific version [2]. This version included security-focused updates, specifically bug fixes related to deserialization checks for valid class types in HttpTransportFactory to improve security [3][4]. It is recommended that you regularly audit your project dependencies using tools like OWASP Dependency-Check, Snyk, or GitHub Dependabot, as vulnerability databases are frequently updated [5]. Additionally, check the official GitHub repositories for both projects for any recent security disclosures [6][7][3].
Citations:
- 1: https://mvnrepository.com/artifact/com.google.firebase/firebase-admin/9.9.0
- 2: https://guide.sonatype.com/component/maven/com.google.auth%3Agoogle-auth-library-credentials/1.43.0
- 3: https://github.com/googleapis/google-auth-library-java/releases
- 4: https://docs.cloud.google.com/java/docs/reference/google-auth-library/latest/history
- 5: https://security.snyk.io/package/npm/firebase-admin
- 6: https://github.com/firebase/firebase-admin-java/releases/tag/v9.9.0
- 7: https://github.com/firebase/firebase-admin-java/releases
🌐 Web query:
com.google.firebase firebase-admin 9.9.0 GitHub Advisory CVE vulnerability
💡 Result:
As of July 14, 2026, there is no specific CVE associated directly with com.google.firebase:firebase-admin version 9.9.0 [1][2]. However, the library is frequently subject to security reports concerning its transitive dependencies [3][4][5][6][7]. Issues reported by the community regarding firebase-admin (Java) often involve transitive vulnerabilities in common dependencies such as jackson-core, netty, or guava [3][5][6][7]. For instance, users have reported that version 9.9.0 includes transitive dependencies that may contain unresolved security vulnerabilities, necessitating manual version overrides in build configuration files (e.g., Gradle or Maven) to remediate [3]. To determine if your application is affected by a specific vulnerability, you should: 1. Use dependency scanning tools (like dependencyCheck, Snyk, or GitHub Dependabot) to generate a dependency tree for your project [4][8]. 2. Identify if any identified CVEs originate from direct or transitive dependencies of firebase-admin [4][7]. 3. If a vulnerable transitive dependency is found, check the official Firebase Admin Java SDK GitHub repository [3] or release notes [9] to see if it is a known issue, and apply a version override for the specific sub-dependency in your build file if an official update is not yet available [3][7]. Always refer to the official Firebase Admin Java SDK GitHub repository for the most current security advisories and release information [1].
Citations:
- 1: https://github.com/firebase/firebase-admin-java/releases/tag/v9.9.0
- 2: https://central.sonatype.com/artifact/com.google.firebase/firebase-admin
- 3: High Severity CVE in transitive dependency
jackson-corefirebase/firebase-admin-java#1198 - 4: https://blog.oscarsixsecurityllc.com/blog/protobufjs-rce-firebase-admin-transitive-cve
- 5: [FR] Vulnerabilities in netty CVE-2025-58056, CVE-2025-58057 firebase/firebase-admin-java#1143
- 6: v9.2.0 Provides transitive vulnerable dependency maven:com.google.guava:guava:31.1-jre firebase/firebase-admin-java#899
- 7: Netty dependency version is vulnerable firebase/firebase-admin-java#423
- 8: Vulnerabilities introduced by package node-forge and @firebase/util firebase/firebase-admin-node#1392
- 9: https://firebase.google.com/support/release-notes/admin/java
🌐 Web query:
com.google.auth google-auth-library-credentials 1.43.0 CVE advisory vulnerability
💡 Result:
As of July 14, 2026, there are no known CVEs (Common Vulnerabilities and Exposures) specifically associated with version 1.43.0 of the google-auth-library-credentials library [1]. In version 1.43.0, released on February 10, 2026, the maintainers implemented a security fix related to deserialization, specifically to prevent arbitrary class loading [2][3]. This fix addresses a potential vulnerability where an attacker could exploit the HttpTransportFactory deserialization process to load gadget classes and execute arbitrary commands [3]. By implementing stricter validation on class types during deserialization, this update mitigates that risk [3]. It is common to confuse the google-auth-library-java project with other Google authentication libraries, such as the google-oauth-java-client, which has been subject to previous CVEs like CVE-2021-22573 [4][5]. Always ensure you are reviewing advisories specifically for the artifact and version you are using [6][7]. You can monitor the official GitHub repository for security advisories [6][7].
Citations:
- 1: https://guide.sonatype.com/component/maven/com.google.auth%3Agoogle-auth-library-credentials/1.43.0
- 2: https://github.com/googleapis/google-auth-library-java/releases
- 3: fix: Deserialization checks valid class types for HttpTransportFactory googleapis/google-auth-library-java#1882
- 4: https://www.sentinelone.com/vulnerability-database/cve-2021-22573/
- 5: GHSA-hw42-3568-wj87
- 6: https://github.com/googleapis/google-auth-library-java/security
- 7: https://github.com/googleapis/google-auth-library-java/blob/main/SECURITY.md
🌐 Web query:
firebase-admin-java 9.9.0 security advisory transitive dependency vulnerabilities
💡 Result:
There is no formal security advisory published by the Firebase team for version 9.9.0 of the firebase-admin-java SDK [1]. However, community reports indicate that version 9.9.0 contains transitive dependencies with known vulnerabilities [2]. Specifically, as of March 2026, users reported that the SDK includes transitive dependencies on certain libraries, such as jackson-core and io.opentelemetry:opentelemetry-api, which were flagged with security vulnerabilities [2]. It is common for such issues to arise in transitive dependencies—libraries that your dependencies rely on—rather than in the Firebase Admin SDK code itself [2][3]. If you are using firebase-admin-java 9.9.0 and need to address these warnings, you can typically use dependency management features in your build tool (such as Gradle or Maven) to override the vulnerable transitive versions with secure, patched versions [2][4]. Example Gradle override: dependencies { implementation("com.google.firebase:firebase-admin:9.9.0") constraints { implementation("com.fasterxml.jackson.core:jackson-core:2.18.6") { because("Fixes GHSA-72hv-8253-57qq") } } } You should monitor the official Firebase Admin Java GitHub repository for any future releases or official security advisories [1]. You can also review the repository's issue tracker to see if specific transitive dependency updates are being addressed in upcoming versions [2][3].
Citations:
- 1: https://github.com/firebase/firebase-admin-java/security
- 2: High Severity CVE in transitive dependency
jackson-corefirebase/firebase-admin-java#1198 - 3: netty 4.2.7.Final is flagged by dependabot firebase/firebase-admin-java#1158
- 4: v9.2.0 Provides transitive vulnerable dependency maven:com.google.guava:guava:31.1-jre firebase/firebase-admin-java#899
🌐 Web query:
google-auth-library-credentials 1.43.0 security advisory CVE
💡 Result:
There is no public security advisory or CVE (Common Vulnerabilities and Exposures) associated specifically with version 1.43.0 of the google-auth-library-credentials or related components in the google-auth-library family [1][2][3]. The version 1.43.0 for the Java-based google-auth-library (released on February 10, 2026) included a bug fix related to deserialization checks for HttpTransportFactory [2][3][4]. Similarly, the Python equivalent google-auth 2.43.0 (released on November 5, 2025) contained feature enhancements and general code improvements [5][6]. Security assessments for these packages have not identified known vulnerabilities at these versions [1]. If you are concerned about security vulnerabilities in your dependencies, it is recommended to monitor the official repository release notes and security advisories on GitHub [5][3] or use security scanning tools to check for vulnerabilities in your specific project's dependency tree.
Citations:
- 1: https://secure.software/pypi/packages/google-auth/vulnerabilities/2.43.0
- 2: googleapis/google-auth-library-java@v1.42.1...v1.43.0
- 3: https://github.com/googleapis/google-auth-library-java/releases
- 4: https://github.com/googleapis/google-auth-library-java/blob/main/CHANGELOG.md
- 5: https://github.com/googleapis/google-auth-library-python/releases/tag/v2.43.0
- 6: googleapis/google-auth-library-python@v2.41.1...v2.43.0
Check firebase-admin’s transitive dependencies. google-auth-library-credentials 1.43.0 has no known CVE, but firebase-admin 9.9.0 still pulls in transitive vulnerability reports, so this pin does not guarantee a clean security scan.
🤖 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 `@openframe-notification-push/pom.xml` around lines 38 - 42, Review the
transitive dependencies resolved by the firebase-admin.version property, rather
than treating the google-auth.version pin as sufficient. Update the Firebase
Admin dependency or its dependency management so the resolved
google-auth-library-credentials and related transitive artifacts are
vulnerability-free, while preserving version compatibility with firebase-admin.
| int worstCase = maxTitleBytes + maxBodyBytes + maxContextBytes + ENVELOPE_HEADROOM_BYTES; | ||
| if (worstCase > FCM_PAYLOAD_LIMIT_BYTES) { | ||
| throw new IllegalStateException(String.format( | ||
| "openframe.push.fcm budgets do not fit FCM's %d-byte limit: title(%d) + body(%d) " | ||
| + "+ context(%d) + envelope(%d) = %d. Lower one — otherwise every push is " | ||
| + "rejected with INVALID_ARGUMENT.", | ||
| FCM_PAYLOAD_LIMIT_BYTES, maxTitleBytes, maxBodyBytes, maxContextBytes, | ||
| ENVELOPE_HEADROOM_BYTES, worstCase)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent integer overflow and reject negative budgets.
If an administrator configures extremely large budgets (e.g., maxBodyBytes = 2147483647), the worstCase sum will overflow into a negative integer, bypassing the > FCM_PAYLOAD_LIMIT_BYTES check entirely. Furthermore, negative budget values could cause unexpected failures downstream during payload truncation.
Validate that all budgets are non-negative, and calculate the worst-case sum using a long to ensure the payload limit is strictly enforced against misconfiguration.
🛡️ Proposed fix
void validate() {
if (projectId == null || projectId.isBlank()) {
throw new IllegalStateException(
"openframe.push.fcm.project-id must be set when push is enabled — "
+ "Application Default Credentials carry no project and FCM cannot infer one");
}
+ if (maxTitleBytes < 0 || maxBodyBytes < 0 || maxContextBytes < 0) {
+ throw new IllegalStateException("openframe.push.fcm budgets must be non-negative.");
+ }
+
- int worstCase = maxTitleBytes + maxBodyBytes + maxContextBytes + ENVELOPE_HEADROOM_BYTES;
+ long worstCase = (long) maxTitleBytes + maxBodyBytes + maxContextBytes + ENVELOPE_HEADROOM_BYTES;
if (worstCase > FCM_PAYLOAD_LIMIT_BYTES) {
throw new IllegalStateException(String.format(📝 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.
| int worstCase = maxTitleBytes + maxBodyBytes + maxContextBytes + ENVELOPE_HEADROOM_BYTES; | |
| if (worstCase > FCM_PAYLOAD_LIMIT_BYTES) { | |
| throw new IllegalStateException(String.format( | |
| "openframe.push.fcm budgets do not fit FCM's %d-byte limit: title(%d) + body(%d) " | |
| + "+ context(%d) + envelope(%d) = %d. Lower one — otherwise every push is " | |
| + "rejected with INVALID_ARGUMENT.", | |
| FCM_PAYLOAD_LIMIT_BYTES, maxTitleBytes, maxBodyBytes, maxContextBytes, | |
| ENVELOPE_HEADROOM_BYTES, worstCase)); | |
| } | |
| if (maxTitleBytes < 0 || maxBodyBytes < 0 || maxContextBytes < 0) { | |
| throw new IllegalStateException("openframe.push.fcm budgets must be non-negative."); | |
| } | |
| long worstCase = (long) maxTitleBytes + maxBodyBytes + maxContextBytes + ENVELOPE_HEADROOM_BYTES; | |
| if (worstCase > FCM_PAYLOAD_LIMIT_BYTES) { | |
| throw new IllegalStateException(String.format( | |
| "openframe.push.fcm budgets do not fit FCM's %d-byte limit: title(%d) + body(%d) " | |
| "+ context(%d) + envelope(%d) = %d. Lower one — otherwise every push is " | |
| "rejected with INVALID_ARGUMENT.", | |
| FCM_PAYLOAD_LIMIT_BYTES, maxTitleBytes, maxBodyBytes, maxContextBytes, | |
| ENVELOPE_HEADROOM_BYTES, worstCase)); | |
| } |
🤖 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
`@openframe-notification-push/src/main/java/com/openframe/notification/push/FcmProperties.java`
around lines 41 - 49, Update the budget validation in FcmProperties so
maxTitleBytes, maxBodyBytes, and maxContextBytes are rejected when negative,
then calculate worstCase using long arithmetic before comparing it with
FCM_PAYLOAD_LIMIT_BYTES. Preserve the existing exception behavior and diagnostic
values while ensuring oversized sums cannot overflow and bypass the limit.
2 participants
Backend for Native OS Notifications — Push Infra (ClickUp
86ajebb5c). ExtendsNotificationBroadcaster, until now socket-only, with a push sink, plus the device-token store push depends on.Design
One provider, not two. iOS hands us an FCM token too (the client uses the Firebase messaging plugin) and Firebase forwards to APNs itself — so there is no per-platform routing and no APNs
environmentto track.PushDevice.environmentis kept nullable only so a direct-APNs provider stays possible without a migration.PushSenderport lives inopenframe-data-nats, the implementation does not. The port deliberately mirrorspublishToUser(userId, notification, category), so the broadcaster treats push as just another per-recipient sink and stays dumb.firebase-admin(heavy: google-cloud + gRPC) is confined to the new opt-inopenframe-notification-pushmodule — it must never leak intoopenframe-data-nats, which half the platform depends on. Adding a second provider later is a new implementation, not a broadcaster change.Push is a sink of its own, not a NATS fallback. It fires even when NATS is disabled, because the two cover different states: sockets reach a foreground client, push reaches a backgrounded or killed one. Humans only — machines are agents, not phones.
Auth is Application Default Credentials, not a service-account key. The GCP org enforces
iam.disableServiceAccountKeyCreation, so a key cannot even be issued. The samegetApplicationDefault()call resolves Workload Identity in GKE andgcloudADC locally — nothing to store, rotate, or leak, and no k8s secret. (The pod SA already had Workload Identity; it has been grantedroles/firebasecloudmessaging.adminon the Firebase project.)The data payload carries the whole serialized context, not a curated set of routing fields — the client owns deep-linking and can change it without a backend release. Oversized contexts are dropped rather than losing the push to FCM's ~4KB limit.
Correctness details worth a look
UNREGISTERED/INVALID_ARGUMENT/SENDER_ID_MISMATCHdrop a row. A transientUNAVAILABLEorQUOTA_EXCEEDEDmust not cost a user their device registration.tenantIdon insert, so the repository never inserts an entity directly — it upserts throughTenantAwareMongoTemplate, letting MongoDB build the row from the query's equality conditions. Document and read-filter therefore cannot drift apart. Uniqueness is{tenantId, token}(the conventionAuthUserandMongoRegisteredClientalready use), which today degenerates to uniqueness on the token and stays correct if tenant-isolation ever shares collections.Tests
openframe-data-nats53 green (incl. 4 new: push fires per admin never per machine; fires without NATS; absent bean is a no-op; a throwing sender is swallowed and the other recipients still get pushed).openframe-notification-push7 green (no devices = no-op; multicast; UNREGISTERED pruned; UNAVAILABLE not pruned; whole-batch failure swallowed; payload contract; oversized context dropped). ArchUnit repository rules pass.Wiring (registration endpoint + config) lands in openframe-saas-tenant on top of this.
🤖 Generated with Claude Code
Summary by CodeRabbit