fix(mobile): close the final security review findings in push receive (#8129)

- a push with no epoch can no longer claim a seq-derived dedup key, in the
  foreground or from the tray; a forged seq:N could otherwise swallow the
  real bell at that seq
- a provider-delivered push with no host catalog, or no fingerprint at all,
  stays unrouted instead of falling back to the hostId its raw data carries
This commit is contained in:
Jinwoo-H
2026-09-06 15:19:22 -04:00
parent 7d11e89c2f
commit df8a70bd63
5 changed files with 123 additions and 20 deletions
+11 -3
View File
@@ -11,6 +11,7 @@ import { RpcClientProvider } from '../src/transport/client-context'
import { getNotificationNavigationTarget } from '../src/notifications/notification-routing'
import { useOpenNotificationRoute } from '../src/notifications/use-open-notification-route'
import {
isRemotePushTrigger,
pushNotificationRouteData,
shouldSuppressForegroundPush
} from '../src/notifications/push-receive'
@@ -116,10 +117,17 @@ export default function RootLayout() {
}
}
async function getNavigationTarget(data: unknown) {
async function getNavigationTarget(notification: Notifications.Notification) {
const hosts = await loadHostCatalog().catch(() => null)
const data: unknown = notification.request.content.data
// A gateway push names its host by key fingerprint, not by this device's hostId.
const routeData = hosts ? pushNotificationRouteData(data, hosts) : data
// With no catalog to resolve against, such a push stays unrouted instead of
// falling back to whatever hostId its raw data carries.
const routeData = pushNotificationRouteData(
data,
hosts ?? [],
isRemotePushTrigger(notification.request.trigger)
)
return getNotificationNavigationTarget(routeData, {
knownHostIds: hosts ? new Set(hosts.map((host) => host.id)) : undefined,
credentialStatusByHostId: hosts
@@ -148,7 +156,7 @@ export default function RootLayout() {
}
}
const target = await getNavigationTarget(response.notification.request.content.data)
const target = await getNavigationTarget(response.notification)
clearLastNotificationResponse()
if (disposed) {
return
+76 -8
View File
@@ -8,7 +8,11 @@ import {
getHostNotificationSession,
resetHostNotificationSessionsForTests
} from './notification-reconnect-catchup'
import { pushNotificationRouteData, shouldSuppressForegroundPush } from './push-receive'
import {
isRemotePushTrigger,
pushNotificationRouteData,
shouldSuppressForegroundPush
} from './push-receive'
vi.mock('../transport/host-store', () => ({ loadHostCatalog: vi.fn() }))
@@ -48,17 +52,29 @@ beforeEach(() => {
describe('shouldSuppressForegroundPush', () => {
it('suppresses a push whose id and seq the socket already delivered', async () => {
getHostNotificationSession('host-1').seen.add('id:agent:one#7')
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-1'
session.seen.add('id:agent:one#7')
await expect(
shouldSuppressForegroundPush(
apnsData({ hostFingerprint, notificationId: 'agent:one', notificationSeq: 7 })
apnsData({
hostFingerprint,
notificationId: 'agent:one',
notificationSeq: 7,
notificationEpoch: 'epoch-1'
})
)
).resolves.toBe(true)
})
it('shows an unseen push and marks it so the socket replay is dropped', async () => {
const data = apnsData({ hostFingerprint, notificationId: 'agent:one', notificationSeq: 7 })
const data = apnsData({
hostFingerprint,
notificationId: 'agent:one',
notificationSeq: 7,
notificationEpoch: 'epoch-1'
})
await expect(shouldSuppressForegroundPush(data)).resolves.toBe(false)
@@ -67,25 +83,55 @@ describe('shouldSuppressForegroundPush', () => {
})
it('reads the flat stringified fields an FCM data message carries', async () => {
getHostNotificationSession('host-1').seen.add('id:agent:one#7')
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-1'
session.seen.add('id:agent:one#7')
await expect(
shouldSuppressForegroundPush(
fcmData({ hostFingerprint, notificationId: 'agent:one', notificationSeq: 7 })
fcmData({
hostFingerprint,
notificationId: 'agent:one',
notificationSeq: 7,
notificationEpoch: 'epoch-1'
})
)
).resolves.toBe(true)
})
it('keys a terminal bell on its seq alone, since it carries no notification id', async () => {
getHostNotificationSession('host-1').seen.add('seq:4')
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-1'
session.seen.add('seq:4')
await expect(
shouldSuppressForegroundPush(
apnsData({ hostFingerprint, source: 'terminal-bell', notificationSeq: 4 })
apnsData({
hostFingerprint,
source: 'terminal-bell',
notificationSeq: 4,
notificationEpoch: 'epoch-1'
})
)
).resolves.toBe(true)
})
it('shows a push that names no counter lifetime without letting it claim a key', async () => {
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-1'
session.seen.add('seq:4')
// Without an epoch the seq cannot be tied to this counter, so a forged seq:4
// must neither be swallowed against it nor stop the real bell at seq 4.
await expect(
shouldSuppressForegroundPush(apnsData({ hostFingerprint, notificationSeq: 4 }))
).resolves.toBe(false)
await expect(
shouldSuppressForegroundPush(apnsData({ hostFingerprint, notificationSeq: 5 }))
).resolves.toBe(false)
expect(session.seen.has('seq:5')).toBe(false)
})
it('voids seen keys from a previous desktop lifetime before testing its own', async () => {
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-old'
@@ -192,6 +238,28 @@ describe('pushNotificationRouteData', () => {
expect(getNotificationNavigationTarget(data)).toBeNull()
})
it('leaves a remote push unrouted when no host catalog could be read', () => {
const data = { hostId: 'host-1', orca: { hostFingerprint, notificationId: 'agent:one' } }
expect(pushNotificationRouteData(data, [], true)).toBeNull()
})
it('leaves a remote push with no fingerprint unrouted instead of treating it as local', () => {
const data = { hostId: 'host-1', worktreeId: 'wt-1', source: 'agent-task-complete' }
expect(pushNotificationRouteData(data, hosts, true)).toBeNull()
// The same shape from this app's own scheduler still routes.
expect(pushNotificationRouteData(data, hosts, false)).toBe(data)
})
it('recognises only a provider-delivered trigger as remote', () => {
expect(isRemotePushTrigger({ type: 'push' })).toBe(true)
expect(isRemotePushTrigger({ type: 'timeInterval', seconds: 1 })).toBe(false)
expect(isRemotePushTrigger({ channelId: 'orca-desktop' })).toBe(false)
expect(isRemotePushTrigger(null)).toBe(false)
expect(isRemotePushTrigger(undefined)).toBe(false)
})
it('drops a gateway payload that pairs an unresolvable fingerprint with a stray hostId', () => {
const data = {
hostId: 'host-1',
+20 -3
View File
@@ -41,6 +41,11 @@ export async function shouldSuppressForegroundPush(data: unknown): Promise<boole
// reconnect replays the desktop's whole retained buffer.
seedWatermarkFromStorage(session, hostId)
await session.watermarkSeeded
// A push that names no counter lifetime cannot claim a seq-derived key: the
// desktop always sends the epoch, so this is shown as-is and never marked.
if (payload.notificationEpoch == null) {
return false
}
// The seen keys are seq-derived, so a push from a new desktop lifetime must void
// them before its own key is tested against a counter that no longer exists.
adoptNotificationEpoch(session, hostId, payload.notificationEpoch)
@@ -61,6 +66,15 @@ export async function shouldSuppressForegroundPush(data: unknown): Promise<boole
return false
}
/** Whether the OS says a notification came from a provider rather than this app. */
export function isRemotePushTrigger(trigger: unknown): boolean {
return (
typeof trigger === 'object' &&
trigger !== null &&
(trigger as { readonly type?: unknown }).type === 'push'
)
}
/**
* Notification data a tap can route with: the gateway names the host by fingerprint,
* so it is mapped back to this device's hostId. Locally scheduled data passes
@@ -68,15 +82,18 @@ export async function shouldSuppressForegroundPush(data: unknown): Promise<boole
*
* Why null and not the raw data when the fingerprint does not resolve: a gateway
* payload is attacker-adjacent input, and passing it on would let a stray `hostId`
* beside the `orca` block route a tap at a host the push never named.
* beside the `orca` block route a tap at a host the push never named. A remote
* push with no fingerprint at all is the same input minus the block, so it is
* unrouted too rather than handed to the local path as if this app scheduled it.
*/
export function pushNotificationRouteData(
data: unknown,
hosts: readonly { readonly id: string; readonly publicKeyB64: string }[]
hosts: readonly { readonly id: string; readonly publicKeyB64: string }[],
remote = false
): unknown {
const payload = readOrcaPushPayload(data)
if (!payload) {
return data
return remote ? null : data
}
const hostId = resolveHostIdForFingerprint(payload.hostFingerprint, hosts)
if (!hostId) {
@@ -101,6 +101,17 @@ describe('markPresentedPushesSeen', () => {
expect(session.lastDeliveredSeq).toBe(0)
})
it('drops a key that names no counter lifetime at all', () => {
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-1'
markPresentedPushesSeen(session, [{ key: 'seq:4', epoch: undefined }])
// The desktop always sends an epoch; a key without one cannot be shown to belong
// to this counter, and claiming it would drop the real bell at seq 4.
expect(session.seen.has('seq:4')).toBe(false)
})
it('drops a key from a desktop lifetime that has already been retired', () => {
const session = getHostNotificationSession('host-1')
session.lastDeliveredEpoch = 'epoch-2'
@@ -49,7 +49,10 @@ export async function readPresentedPushSeenKeys(
}
/**
* Claim the tray's keys on the session, skipping any from a dead counter lifetime.
* Claim the tray's keys on the session, skipping any that do not name the live
* counter lifetime. A push without an epoch cannot be tied to this counter, and
* the desktop always sends one, so it is left unclaimed rather than allowed to
* swallow a real event at the same seq.
*
* The watermark is deliberately untouched: a push seq proves one event was shown,
* not that everything below it was, and advancing past a gap would make the desktop
@@ -60,11 +63,7 @@ export function markPresentedPushesSeen(
keys: readonly PresentedPushSeenKey[]
): void {
for (const { key, epoch } of keys) {
if (
epoch != null &&
session.lastDeliveredEpoch != null &&
epoch !== session.lastDeliveredEpoch
) {
if (epoch == null || epoch !== session.lastDeliveredEpoch) {
continue
}
session.seen.add(key)