fix: per-app notification routing (stop wallet/station cross-app leak)

[gomesalexandre]

what

Both Station notification bugs share ONE root cause: the APNs topic was hardcoded to the wallet bundle id (const appID = "com.vultisig.wallet") for every push. APNs only delivers a push when the topic matches the app that owns the device token, so an agent app (Station, money.terra.station) that shares a vault_id with the wallet had its pushes delivered to the wallet (topic match) and rejected for Station (topic mismatch). Result: the wallet received Station's notifications, and Station received nothing.

This makes notifications route per-app: a device is pushed on its own app's APNs topic, and a notification can target a single app so it doesn't fan out to other apps sharing the vault.

how

• Device gains app_id (defaults to com.vultisig.wallet). The APNs topic is now the device's own app_id instead of the hardcoded wallet id.
• /notify gains an optional app_id: when set, only that app's devices are notified (closes the wallet leak); when omitted, it resolves to the wallet so behaviour is unchanged.
• Per-app APNs certificates (app-certificates config / APP_CERTIFICATES_JSON env): APNs validates the provider cert against the topic, so each app needs its own cert. An app with no configured cert is skipped (never pushed with the wrong cert, which would 400 and could then delete a valid token).
• Reason-aware unregister: only delete a token on 410 Unregistered / 400 BadDeviceToken, never on a topic/cert mismatch.
• App-scoped tokenless unregister + app-aware dedup (normalized so omitted and explicit-wallet collapse to one bucket).
• WebSocket realtime path (wallet-only) is isolated on both ends: non-wallet-targeted notifications aren't published to it, and non-wallet devices can't subscribe.

no regression for the regular Vultisig wallet (the hard requirement)

• The wallet never sends app_id, so it always resolves to the wallet bucket on register, notify, and unregister - identical to today.
• whereAppID makes the wallet bucket match app_id = 'com.vultisig.wallet' OR '' OR NULL, so it does not depend on the AutoMigrate backfill running: a pre-migration row with no app_id is always still reachable (and unregisterable) as a wallet device.
• Device identity stays (vault_id, party_name, token) (APNs/FCM tokens are app-unique), so a wallet re-register idempotently updates its row (backfilling legacy app_id) rather than duplicating it.
• Untargeted notifications stay wallet-only (they were already, via the hardcoded topic) - no new wallet->other-app leak.
• Per-app certs are variadic/opt-in; with none configured the only credential is the wallet's.

rollout

Existing Station installs are currently stored in the wallet bucket (they never sent app_id) and were not correctly receiving Station notifications (the bug). They migrate by re-registering when the updated Station app launches (sending app_id = money.terra.station); until then they stay in the wallet bucket exactly as today. The wallet is unaffected throughout. Companion changes (Station/SDK sending app_id, agent-backend /notify sending app_id, and provisioning Station's APNs cert) ship alongside.

review

7 rounds of codex exec review, each with real iterations (per-app certs + safe-skip, reason-aware unregister, WS isolation both ends, env-loadable certs, app-scoped queries with a backfill-independent wallet bucket, dedup normalization, and reverting an over-aggressive unique-index change that regressed token identity). Converged to a rollout note.

receipts

Background service / no HTTP-contract change to curl; model-level backward-compat invariants are unit-tested.

$ go test ./... && go vet ./...
ok  github.com/vultisig/notification/models   (ResolvedAppID fallback + GetDeviceDBModel default)
ok  github.com/vultisig/notification/service
ok  github.com/vultisig/notification/stream
ok  github.com/vultisig/notification/ws

risk

Medium (shared notification service), mitigated by the no-regression guarantees above + 7 review rounds. Wallet delivery path is behaviourally unchanged; new behaviour is strictly opt-in via app_id + provisioned per-app certs.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Multi-app support: notifications and device registrations are app-ID scoped
  • Per-app APNs certificate routing for iOS delivery
  • App-aware notification deduplication (uses resolved app ID)

• Bug Fixes

  • Device unregistration now scoped to app and tightened for specific APNs error cases
  • WebSocket access restricted to wallet app devices to prevent cross-app leaks

• Configuration

  • Support for APP_CERTIFICATES_JSON to configure per-app APNs certificates

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 95f7d750-6e3e-4eaa-84da-a3ee0a504723

📥 Commits

Reviewing files that changed from the base of the PR and between 4bd5853 and 1e336e5.

📒 Files selected for processing (3)

• models/notification.go
• service/notification.go
• ws/handler_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• models/notification.go
• service/notification.go

---

Walkthrough

This PR introduces multi-application support with app-scoped APNs certificate routing and device isolation. Devices now carry an optional app ID that defaults to the wallet bundle, notifications are deduplicated and routed per app, and WebSocket subscriptions are restricted to wallet-app clients only to prevent cross-app message leakage.

Changes

Multi-app notification support with app-scoped APNs certificates

Layer / File(s)	Summary
Device model and request schema models/device.go, models/device_test.go, models/notification.go	Device defines DefaultAppID constant ("com.vultisig.wallet"), adds AppID field and ResolvedAppID() method that defaults to wallet when empty; GetDeviceDBModel() persists the resolved app ID to the database. NotificationRequest gains an optional AppID field for app-targeted notifications. Unit tests validate app ID resolution and DB persistence.
APNs certificate configuration config/config.go, cmd/worker/main.go	New AppCertificate struct holds per-app certificate metadata. Config.AppCertificates slice is populated from APP_CERTIFICATES_JSON environment variable when the config file does not provide one. main builds and passes app certificates as variadic arguments to NewNotificationService.
Storage layer app-scoped queries storage/database.go	RegisterDevice upsert now updates app_id on conflict. UnregisterDeviceByParty signature accepts appID parameter and defaults to wallet app; tokenless device deletion is scoped to that app via new whereAppID helper. GetRegisteredDevices signature accepts appID and applies app-scoped filtering so wallet notifications do not fan out across other apps.
Notification service app-aware routing service/notification.go	New APNsAppCertificate type; NotificationService stores credentials per app in apnsCredentials map. NewNotificationService accepts variadic app certificate inputs and seeds the default wallet certificate. During notification delivery, device lookup passes request.AppID for scoping; Apple push selects certificate and topic from the device's resolved app ID, skipping devices without configured credentials. APNs error handling is tightened to unregister tokens only on HTTP 410 or HTTP 400 with BadDeviceToken.
API and WebSocket app isolation api/server.go, ws/handler.go, ws/handler_test.go	Unregister endpoint now accepts app_id in the request body and scopes tokenless unregistration by app. Notification dedup key changes from vault-only to vault_id:app_id (resolved to wallet default when empty), and WebSocket/Redis-stream publishing is gated to wallet-app notifications only. WebSocket handler enforces wallet-only access by checking device's resolved app ID and returning HTTP 403 for non-wallet apps; tests added to verify rejection for non-wallet devices.

Possibly related PRs

• vultisig/notification#25: Modifies device unregistration behavior in storage/database.go to make it idempotent; this PR refactors unregistration to be app-scoped and could benefit from or conflict with that idempotence change.

Suggested reviewers

• johnnyluo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing per-app notification routing to prevent cross-app leakage between wallet and station.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix_per_app_notification_routing

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@models/notification.go`:
- Around line 10-15: The comment for the AppID field in models/notification.go
is inaccurate: empty app_id no longer means "notify all vault devices" but is
resolved to the wallet-default scope; update the doc comment above the AppID
field (the AppID field/comment block) to state that when AppID is empty routing
resolves to the wallet-default app scope and only devices subscribed to that
default scope are notified, so integrators do not expect fan-out to every
app/topic for the vault.

🪄 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: CHILL

Plan: Pro

Run ID: fea7409b-c44f-4171-aa74-4bccb6deb690

📥 Commits

Reviewing files that changed from the base of the PR and between d00489b and 4bd5853.

📒 Files selected for processing (9)

• api/server.go
• cmd/worker/main.go
• config/config.go
• models/device.go
• models/device_test.go
• models/notification.go
• service/notification.go
• storage/database.go
• ws/handler.go

[gomesalexandre]

done @coderabbitai - 127c5e2 corrects the empty-app_id contract on NotificationRequest.AppID. confirmed the actual behavior end-to-end:

• service/notification.go:153 passes request.AppID straight into GetRegisteredDevices(ctx, vaultId, partyName, appID)
• storage/database.go:184 resolves empty to DefaultAppID (com.vultisig.wallet)
• whereAppID() at storage/database.go:206-211 for wallet bucket matches app_id = DefaultAppID OR '' OR NULL so pre-migration rows still resolve

so empty app_id only fans out to wallet-bundle devices + pre-migration empty/NULL rows, never to Station etc. updated the doc comment to say exactly that + that non-wallet apps must target their bundle id explicitly. gucci 🚀

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@gomesalexandre Thanks for the detailed trace — the chain is clear and the backward-compat story (NULL/empty → wallet bucket via whereAppID) is solid. Marking this as addressed.

(¯·.¸¸.·´¯·.¸¸.·´¯) 🐇 (¯·.¸¸.·´¯·.¸¸.·´¯)

---

✏️ Learnings added

Learnt from: gomesalexandre
URL: https://github.com/vultisig/notification/pull/27

Timestamp: 2026-05-30T13:27:40.196Z
Learning: In `vultisig/notification`, `NotificationRequest.AppID` (in `models/notification.go`) being empty means "wallet only" — not "all apps". The full chain: `service/notification.go` passes `request.AppID` straight into `GetRegisteredDevices`; `storage/database.go` resolves empty to `DefaultAppID` (`com.vultisig.wallet`); `whereAppID()` (storage/database.go) matches wallet bucket as `app_id = DefaultAppID OR '' OR NULL`, so pre-migration rows with no app_id still resolve to wallet. Non-wallet apps (e.g., Station) must explicitly pass their bundle ID as `app_id` in the notify request.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[NeOMakinG]

testing approach

Triple-lane review (3 adversarial passes) — Claude deep trace + GLM third-eye (Codex auth stale, skipped per [[feedback_codex_auth_brittleness]]). Two in-band fixes applied and pushed as 1e336e5.

PASSES: 3

Pass 1 — full code trace of every routing path.
Pass 2 — adversarial re-read: empty/null app_id injection, legacy-row migration safety, race conditions, allowlist gaps, WS subscription timing.
Pass 3 — blast radius: all four cross-app leak scenarios closed; GORM migration safety confirmed by reading migrator source.

---

blocking

None.

preferably-blocking

None.

should-fix

s1 — use apns2.ReasonBadDeviceToken constant (fixed in-band, 1e336e5)

service/notification.go:239 used the raw string literal "BadDeviceToken" rather than the exported apns2.ReasonBadDeviceToken constant from the sideshow/apns2 library. The library already exports all reason strings as constants (response.go:20). A future audit that changes the constant value would break the comparison silently; using the exported constant makes drift a compile-time catch. Fixed in 1e336e5.

s2 — missing test for WS non-wallet device rejection (fixed in-band, 1e336e5)

ws/handler_test.go's mockDeviceFinder always returns &models.DeviceDBModel{} (AppID = empty → ResolvedAppID = DefaultAppID = wallet). Every WS test therefore simulated a wallet device. The 403-rejection path at handler.go:67–70 — the guard that prevents a Station device from subscribing to the wallet's realtime stream — had zero test coverage. A regression that removed that guard would pass all pre-existing tests. Added TestNonWalletDeviceRejected (+ fixedDeviceFinder helper) which creates a device with AppID="money.terra.station" and asserts HTTP 403. Test passes and is non-vacuous: removing the guard causes it to fail with 101 Switching Protocols. Fixed in 1e336e5.

suggestion

Validation of app_id on /register and /notify. Currently any arbitrary string is accepted as app_id. An allowlist derived from configured AppCertificates + DefaultAppID would close the theoretical injection path (a client registering with app_id="com.vultisig.wallet" and a Station token). The practical risk is low — the attacker already needs a valid (vault_id, party_name, APNs-token) triple — but the defence is cheap: reject unknown app_id values with 400 at registration time. Not blocking; the DB filter and the cert-skip in processAppleNotification already limit the blast radius of a wrong app_id.

q

Android/FCM per-app routing: FCM tokens are already app-unique (a Station token can only be delivered by Firebase to the Station app), and GetRegisteredDevices already filters by app_id before FCM dispatch. So the routing is correct without per-FCM-project certs. Worth documenting explicitly so the next reviewer doesn't raise the same question.

---

pre-existing bugs i bumped into

None.

risk

Low post-fix. The original cross-app leak (Station→Wallet on both APNs and WebSocket) is fully closed. Wallet regression is prevented at three layers: (1) whereAppID covers empty/NULL/explicit-wallet rows, (2) GORM AutoMigrate backfills existing rows with DEFAULT 'com.vultisig.wallet' (confirmed by reading migrator.go:97–103 + schema/field.go:211), and (3) app_id is excluded from the unique index so wallet re-registrations idempotently UPDATE rather than duplicate.

verdict

APPROVE. Three adversarial passes, zero exploitable cross-app leak paths found. Two in-band fixes pushed: s1 (apns2 constant) and s2 (WS rejection test). CI: CodeRabbit ✅, build ✅. All 4 test packages green on HEAD 1e336e5.

---

🤖 vultisig-ops reviewer

[NeOMakinG]

QA Evidence — notification#27 r1

Test suite (HEAD 1e336e5)

$ go test ./... -v
ok  github.com/vultisig/notification/models   0.390s
    --- PASS: TestResolvedAppID_FallsBackToWalletBundle/empty_falls_back_to_wallet
    --- PASS: TestResolvedAppID_FallsBackToWalletBundle/explicit_wallet_preserved
    --- PASS: TestResolvedAppID_FallsBackToWalletBundle/station_preserved
    --- PASS: TestGetDeviceDBModel_PersistsResolvedAppID
ok  github.com/vultisig/notification/service  0.486s
    --- PASS: TestAPNPayload
    --- PASS: TestWebPushPayload
    --- PASS: TestWebPushSubscriptionUnmarshal
ok  github.com/vultisig/notification/stream   7.955s
    --- PASS: TestPublishSubscribe
    --- PASS: TestAck
    --- PASS: TestContextCancel
    --- PASS: TestStalePendingSkipped
    --- PASS: TestPublishMultipleVaults
ok  github.com/vultisig/notification/ws       0.858s
    --- PASS: TestMissingParams
    --- PASS: TestNonWalletDeviceRejected   (NEW — added by reviewer in 1e336e5)
    --- PASS: TestUnauthorized
    --- PASS: TestConnectionLimit
    --- PASS: TestNotificationDelivery
    --- PASS: TestAckFlow

GORM migration safety (verified from source)

gorm.io/gorm v1.31.0 migrator/migrator.go:97–103 + schema/field.go:211:

• AppID string gorm:"default:com.vultisig.wallet"`` sets field.HasDefaultValue=true, `field.DefaultValue="com.vultisig.wallet"`.
• FullDataTypeOf returns SQL including DEFAULT 'com.vultisig.wallet' (string type → DefaultValueInterface is set, bound as a parameterized value).
• MySQL ALTER TABLE devices ADD COLUMN app_id VARCHAR(128) NOT NULL DEFAULT 'com.vultisig.wallet' backfills all existing rows with com.vultisig.wallet. No separate migration script needed.
• whereAppID also covers OR app_id = '' OR app_id IS NULL as belt-and-suspenders for any edge case where the backfill did not occur (e.g., a row inserted between migration start and commit), ensuring wallet queries never silently drop pre-migration devices.

Cross-app leak verification (3 adversarial passes)

Scenario	Result
Wallet push (no app_id) → Station device	NOT matched: whereAppID('com.vultisig.wallet') filters (app_id='com.vultisig.wallet' OR app_id='' OR IS NULL); Station row with app_id='money.terra.station' excluded
Station push (app_id='money.terra.station') → Wallet device	NOT matched: whereAppID('money.terra.station') uses WHERE app_id='money.terra.station'; wallet rows excluded
Station push → WS stream (realtime path)	NOT published: server.go:249 gates Publish on req.AppID == "" OR req.AppID == DefaultAppID; Station push has non-wallet AppID
Station device → WS subscribe	403 Forbidden: handler.go:67–70 rejects device.ResolvedAppID() != DefaultAppID; covered by new TestNonWalletDeviceRejected
Legacy row (empty/NULL app_id) → Wallet push	MATCHED via OR app_id='' OR IS NULL; no regression

In-band fixes (1e336e5)

1. s1 service/notification.go:239: replaced raw string "BadDeviceToken" with apns2.ReasonBadDeviceToken exported constant.
2. s2 ws/handler_test.go: added TestNonWalletDeviceRejected + fixedDeviceFinder helper to cover the 403-rejection path for non-wallet devices. Non-vacuity: removing the handler.go:67–70 guard causes this test to fail with 101 (WS upgrade succeeds instead of 403).

CI

CodeRabbit ✅ | build (18s) ✅ — both passing on original HEAD 127c5e2; fixes in 1e336e5 are logic-only (no new dependencies, no API changes).

APNs runtime delivery

Not exercised against live APNs — this is a BE-only service with no iOS sim attachment needed. Correct topic routing (device.ResolvedAppID() → notification.Topic) is verified by code trace + test coverage. End-to-end delivery (Station app receives push, wallet does not) requires the companion changes in Station SDK + agent-backend noted in the PR body.

🤖 vultisig-ops reviewer