fix(push): close the final security review findings in the gateway and infra (#8129)

- app.onError logs only the error name and answers a bare 500; hono's default
  handler printed the whole error, and a pg error carries the row in detail
- a second per-IP bucket (240/min) runs ahead of the bearer lookup on every
  authenticated route, so forged bearers cannot spend the two-connection pool
- one live session per host: minting deletes the host's earlier row
- device-less hosts are pruned after 1 h, not 30 d; any keypair mints one free
- notificationId is printable ASCII, since it becomes the APNs collapse header
- the impersonated FCM probe token is masked in the workflow log
- prevent_destroy on the Apple secrets and the orca_push database

.github/workflows/cloud-push-deploy.yml

@@ -198,6 +198,7 @@ jobs:
           token="$(gcloud auth print-access-token \
             --impersonate-service-account "${PUSH_RUNTIME_SERVICE_ACCOUNT}")"
           test -n "${token}"
+          echo "::add-mask::${token}"
           body='{"validate_only":true,"message":{"token":"orca-push-deploy-probe-invalid-token","notification":{"title":"Orca","body":"deploy probe"}}}'
           for attempt in $(seq 1 5); do
             code="$(curl -sS -o "${RUNNER_TEMP}/push-fcm.json" -w '%{http_code}' --max-time 20 \

cloud/apps/push/src/host-session-store.test.ts

@@ -51,17 +51,20 @@ describe('push host session store', () => {
     await expect(sessions.resolve(session.sessionToken)).resolves.toMatchObject({ ok: true })
   })
 
-  it('prunes expired sessions and revokes a host on demand', async () => {
+  it('keeps one live session per host and prunes it once expired', async () => {
     const first = await sessions.create(HOST)
-    clock += PUSH_LIMITS.sessionTtlMs + 1
     const second = await sessions.create(HOST)
-    expect(await sessions.pruneExpired()).toBe(1)
-    await expect(sessions.resolve(first.sessionToken)).resolves.toMatchObject({ ok: false })
-    await expect(sessions.resolve(second.sessionToken)).resolves.toMatchObject({ ok: true })
-    expect(await sessions.revokeForHost(HOST)).toBe(1)
-    await expect(sessions.resolve(second.sessionToken)).resolves.toEqual({
+    // The earlier session is gone the moment its host proves again, so a flood
+    // of proofs leaves one row per host rather than one per proof.
+    await expect(sessions.resolve(first.sessionToken)).resolves.toEqual({
       ok: false,
       reason: 'unknown_session'
     })
+    await expect(sessions.resolve(second.sessionToken)).resolves.toMatchObject({ ok: true })
+    const other = await sessions.create('ponmlkjihgfedcba')
+    await expect(sessions.resolve(second.sessionToken)).resolves.toMatchObject({ ok: true })
+    clock += PUSH_LIMITS.sessionTtlMs + 1
+    expect(await sessions.pruneExpired()).toBe(2)
+    await expect(sessions.resolve(other.sessionToken)).resolves.toMatchObject({ ok: false })
   })
 })

cloud/apps/push/src/host-session-store.ts

@@ -26,11 +26,19 @@ export class PushHostSessionStore {
     const sessionToken = randomBytes(32).toString('base64url')
     const createdAt = this.now()
     const expiresAt = createdAt + PUSH_LIMITS.sessionTtlMs
-    await this.database.query(
-      `INSERT INTO push_sessions (token_hash, host_fingerprint, expires_at, created_at)
-       VALUES (?, ?, ?, ?)`,
-      [hashSessionToken(sessionToken), hostFingerprint, expiresAt, createdAt]
-    )
+    await this.database.transaction(async (transaction) => {
+      // Why: a desktop holds one session at a time and only re-proves once it is
+      // gone, so an earlier row is dead weight. It also bounds the table to one
+      // row per host however many proofs a self-minted identity answers.
+      await transaction.query('DELETE FROM push_sessions WHERE host_fingerprint = ?', [
+        hostFingerprint
+      ])
+      await transaction.query(
+        `INSERT INTO push_sessions (token_hash, host_fingerprint, expires_at, created_at)
+         VALUES (?, ?, ?, ?)`,
+        [hashSessionToken(sessionToken), hostFingerprint, expiresAt, createdAt]
+      )
+    })
     return { sessionToken, expiresAt, hostFingerprint }
   }
 
@@ -47,14 +55,6 @@ export class PushHostSessionStore {
     return { ok: true, hostFingerprint: String(row.host_fingerprint), expiresAt }
   }
 
-  async revokeForHost(hostFingerprint: string): Promise<number> {
-    const [result] = await this.database.query(
-      'DELETE FROM push_sessions WHERE host_fingerprint = ?',
-      [hostFingerprint]
-    )
-    return Number(result?.changes ?? 0)
-  }
-
   async pruneExpired(): Promise<number> {
     const [result] = await this.database.query('DELETE FROM push_sessions WHERE expires_at < ?', [
       this.now()

cloud/apps/push/src/index.ts

@@ -5,7 +5,7 @@ import { createPushServer } from './push-server.js'
 const CHALLENGE_PRUNE_INTERVAL_MS = 60_000
 const SESSION_PRUNE_INTERVAL_MS = 10 * 60_000
 const SEND_LOG_PRUNE_INTERVAL_MS = 30 * 60_000
-const STALE_HOST_PRUNE_INTERVAL_MS = 6 * 60 * 60_000
+const STALE_HOST_PRUNE_INTERVAL_MS = 30 * 60_000
 
 const config = loadPushConfig()
 const database = await openPushDatabase({

cloud/apps/push/src/push-observability.ts

@@ -1,5 +1,6 @@
 type PushCounterName =
   | 'ip_rate_limited'
+  | 'request_error'
   | 'challenge_issued'
   | 'challenge_rejected'
   | 'session_issued'
@@ -17,6 +18,7 @@ type PushCounterName =
 
 const COUNTER_NAMES: PushCounterName[] = [
   'ip_rate_limited',
+  'request_error',
   'challenge_issued',
   'challenge_rejected',
   'session_issued',

cloud/apps/push/src/push-server-harness.test-fixture.ts

@@ -157,7 +157,8 @@ export async function createPushServerHarness() {
     },
     close: async (): Promise<void> => {
       server.coalescer.stop()
-      await database.close()
+      // A test may close the database itself to provoke a route failure.
+      await database.close().catch(() => undefined)
     }
   }
 }

cloud/apps/push/src/push-server-limits.test.ts

@@ -1,5 +1,5 @@
 import { PUSH_LIMITS } from '@orca-cloud/push-contract'
-import { afterEach, beforeEach, describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import {
   createPushHostKeypair,
   hostPublicKeyB64
@@ -171,16 +171,37 @@ describe('push gateway request limits', () => {
     ).toBe(200)
   })
 
-  it('never throttles an authenticated device or send route', async () => {
+  it('gives the authenticated routes their own, wider bucket per client ip', async () => {
     const sessionToken = await harness.signIn(createPushHostKeypair(64))
-    for (let index = 0; index < PUSH_LIMITS.unauthenticatedRequestsPerMinutePerIp + 5; index++) {
-      const listed = await harness.authorized(
-        '/v1/devices',
-        { headers: { 'x-forwarded-for': CLIENT_IP } },
-        sessionToken
-      )
+    const headers = { 'x-forwarded-for': CLIENT_IP }
+    for (let index = 0; index < PUSH_LIMITS.authenticatedRequestsPerMinutePerIp; index++) {
+      const listed = await harness.authorized('/v1/devices', { headers }, sessionToken)
       expect(listed.status).toBe(200)
     }
+    const limited = await harness.authorized('/v1/devices', { headers }, sessionToken)
+    expect(limited.status).toBe(429)
+    // The handshake bucket is untouched by any of that.
+    const challenge = await harness.server.app.request('/v1/host/challenge', {
+      method: 'POST',
+      headers: { ...headers, 'content-type': 'application/json' },
+      body: JSON.stringify({ v: 1, hostPublicKeyB64: hostPublicKeyB64(createPushHostKeypair(67)) })
+    })
+    expect(challenge.status).toBe(200)
+  })
+
+  it('caps a flood of forged bearers before any of them reaches the session lookup', async () => {
+    const headers = { 'x-forwarded-for': CLIENT_IP }
+    const [before] = await harness.database.query('SELECT COUNT(*) AS sessions FROM push_sessions')
+    for (let index = 0; index < PUSH_LIMITS.authenticatedRequestsPerMinutePerIp; index++) {
+      const refused = await harness.authorized('/v1/send', { method: 'POST', headers }, 'forged')
+      expect(refused.status).toBe(401)
+    }
+    const limited = await harness.authorized('/v1/send', { method: 'POST', headers }, 'forged')
+    expect(limited.status).toBe(429)
+    expect(await limited.json()).toEqual({ error: 'rate_limited' })
+    expect(harness.server.unauthenticatedIps.trackedIpCount()).toBe(0)
+    const [after] = await harness.database.query('SELECT COUNT(*) AS sessions FROM push_sessions')
+    expect(Number(after?.sessions)).toBe(Number(before?.sessions))
   })
 
   it('answers 409 once a host has registered its device allowance', async () => {
@@ -208,6 +229,26 @@ describe('push gateway request limits', () => {
     )
   })
 
+  // Why: a database error carries the failing row in its message. The response
+  // and the log must both stop at the error's name.
+  it('answers an unexpected route failure with a bare 500 and logs only the name', async () => {
+    const sessionToken = await harness.signIn(createPushHostKeypair(66))
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined)
+    try {
+      await harness.database.close()
+      const response = await harness.authorized('/v1/devices', {}, sessionToken)
+      expect(response.status).toBe(500)
+      expect(await response.json()).toEqual({ error: 'internal' })
+      const logged = warn.mock.calls.map((call) => String(call[0])).join('\n')
+      expect(logged).toContain('"event":"orca_push_request_failed"')
+      expect(logged).not.toContain('SELECT')
+      expect(logged).not.toContain('push_devices')
+      expect(harness.server.observability.consume().request_error).toBe(1)
+    } finally {
+      warn.mockRestore()
+    }
+  })
+
   it('charges a repeated registration id once and returns one result', async () => {
     const sessionToken = await harness.signIn(createPushHostKeypair(65))
     const registrationId = await harness.registerAndroid(sessionToken)

cloud/apps/push/src/push-server.ts

@@ -102,7 +102,30 @@ export function createPushServer(
     trustedProxyHops: config.trustedProxyHops,
     onLimited: () => observability.record('ip_rate_limited')
   })
+  // Why a second bucket: a bearer has to be looked up before it can be refused,
+  // and that lookup takes one of very few pool connections. Capping the caller
+  // first keeps a flood of forged bearers from starving real hosts of the pool.
+  const authenticatedIps = new ClientIpRateLimiter({
+    now,
+    capacity: PUSH_LIMITS.authenticatedRequestsPerMinutePerIp
+  })
+  const limitAuthenticatedIp = clientIpRateLimit(authenticatedIps, {
+    trustedProxyHops: config.trustedProxyHops,
+    onLimited: () => observability.record('ip_rate_limited')
+  })
   const app = new Hono<{ Variables: PushVariables }>()
+  // Hono's default handler prints the whole error, and a pg error carries the
+  // offending row in `detail`. Only the error's name may reach the logs.
+  app.onError((error, context) => {
+    observability.record('request_error')
+    console.warn(
+      JSON.stringify({
+        event: 'orca_push_request_failed',
+        error: error instanceof Error ? error.name : 'unknown'
+      })
+    )
+    return context.json({ error: 'internal' }, 500)
+  })
 
   app.get('/health', (context) => context.json({ ok: true, pushProtocol: 1 }))
   app.get('/ready', async (context) =>
@@ -123,9 +146,10 @@ export function createPushServer(
     await next()
     return
   }
-  app.use('/v1/devices', bearerSession)
-  app.use('/v1/devices/*', bearerSession)
-  app.use('/v1/send', bearerSession)
+  // `/v1/devices/*` matches `/v1/devices` itself; a second registration for the
+  // bare path would run both middlewares twice on it.
+  app.use('/v1/devices/*', limitAuthenticatedIp, bearerSession)
+  app.use('/v1/send', limitAuthenticatedIp, bearerSession)
 
   app.post('/v1/host/challenge', limitUnauthenticatedIp, limitBody, async (context) => {
     const body = PushHostChallengeRequestSchema.safeParse(

cloud/dev/scripts/push-gateway-workflow.test.mjs

@@ -183,6 +183,13 @@ test('the FCM probe is validate-only and separates a bad token from a bad creden
     /--impersonate-service-account "\$\{PUSH_RUNTIME_SERVICE_ACCOUNT\}"/,
     'the probe must exercise the runtime credential, not the deploy identity'
   )
+  // Why: that token reads the Apple signing key. Masking it means a later `set -x` or a
+  // debug re-run cannot print it into a public log.
+  assert.match(
+    probe,
+    /test -n "\$\{token\}"\n {10}echo "::add-mask::\$\{token\}"/,
+    'the impersonated token must be masked before anything else runs'
+  )
   assert.match(workflow, /PUSH_RUNTIME_SERVICE_ACCOUNT: orca-cloud-push@onorca-cloud\.iam\.gserviceaccount\.com/)
 })
 

cloud/docs/push-gateway.md

@@ -77,6 +77,10 @@ Terraform owns the three Apple secret **names, labels, and replication, and neve
 The `.p8` is issued by the Apple developer portal, so a Terraform-managed version would put the
 private key in state and would fight the rotation below. The database URL secret is different:
 Terraform generates that password, so it owns that version, exactly as `relay-database.tf` does.
+That puts the generated password and the full database URL in the state bucket, which the shared
+deploy identity can read; the Apple key never appears there. The three Apple secrets and the
+`orca_push` database carry `prevent_destroy`, so disabling the gateway fails the plan instead
+of deleting the only copy of the signing key or every live device token.
 
 ## Importing what already exists
 
@@ -280,6 +284,11 @@ Two independent limits, both enforced in the gateway and both returning HTTP 200
 | 200 sends per rolling day | per `registrationId` |
 | 20 `registrationIds` | per request, hard cap, HTTP 400 over it |
 
+Ahead of all three sit two per-client-IP token buckets that answer HTTP 429: 30 requests per
+minute on the two unauthenticated handshake routes, and 240 per minute on every other `/v1`
+route, applied before the bearer is looked up so that a flood of forged bearers cannot spend
+the two-connection pool on session lookups. Both are per instance and in memory.
+
 `push_send_log` backs the two rolling counts and is pruned after 25 hours. Upstream of all
 three, FCM V1 bills project quota against `ORCA_PUSH_FCM_PROJECT_ID`, which is why the runtime
 account holds `roles/serviceusage.serviceUsageConsumer`; a project-level FCM quota exhaustion

cloud/infra/terraform/push-gateway.tf

@@ -97,6 +97,11 @@ resource "google_sql_database" "push" {
   project  = var.project_id
   name     = "orca_push"
   instance = local.relay_database_instance_name
+
+  # Why: this database holds every live device token. Disabling the gateway must not drop it.
+  lifecycle {
+    prevent_destroy = true
+  }
 }
 
 resource "random_password" "push_database" {
@@ -161,6 +166,12 @@ resource "google_secret_manager_secret" "push_provider" {
   replication {
     auto {}
   }
+
+  # Why: Apple issues a `.p8` once and Secret Manager has no undelete. Turning the gateway off
+  # must fail the plan rather than destroy the only copy of the signing key.
+  lifecycle {
+    prevent_destroy = true
+  }
 }
 
 resource "google_secret_manager_secret_iam_member" "push_provider_runtime_accessor" {

cloud/packages/push-contract/src/contract.test.ts

@@ -51,7 +51,10 @@ describe('push contract limits', () => {
       sessionTtlMs: 86_400_000,
       sendLogRetentionMs: 90_000_000,
       notificationTtlSeconds: 14_400,
-      apnsCollapseIdMaxBytes: 64
+      apnsCollapseIdMaxBytes: 64,
+      hostRetentionMs: 3_600_000,
+      unauthenticatedRequestsPerMinutePerIp: 30,
+      authenticatedRequestsPerMinutePerIp: 240
     })
     expect(PUSH_DEFAULTS.apnsTopic).toBe('com.stably.orca.mobile')
     expect(PUSH_DEFAULTS.fcmProjectId).toBe('onorca-cloud')

cloud/packages/push-contract/src/push-limits.ts

@@ -20,11 +20,17 @@ export const PUSH_LIMITS = {
   sendLogRetentionMs: 25 * 60 * 60 * 1000,
   notificationTtlSeconds: 4 * 60 * 60,
   apnsCollapseIdMaxBytes: 64,
-  // A host row outlives its devices only long enough to survive a phone swap.
-  hostRetentionMs: 30 * 24 * 60 * 60 * 1000,
+  // Nothing reads a host row, and any keypair mints one for free, so a host
+  // with no registration left is kept only long enough to survive a phone swap.
+  hostRetentionMs: 60 * 60 * 1000,
   // The challenge and session routes are the only unauthenticated writes, so
   // they are capped per client IP before any key material is generated.
-  unauthenticatedRequestsPerMinutePerIp: 30
+  unauthenticatedRequestsPerMinutePerIp: 30,
+  // Every other route looks its bearer up in the database before it can refuse
+  // it, so a flood of forged bearers is capped per client IP ahead of that.
+  // Wide enough for an office NAT full of hosts, each of which sends at most
+  // its hourly quota plus a registration per connect.
+  authenticatedRequestsPerMinutePerIp: 240
 } as const
 
 export const PUSH_DEFAULTS = {

cloud/packages/push-contract/src/send-messages.test.ts

@@ -70,6 +70,28 @@ describe('send schemas', () => {
       .toBe(false)
   })
 
+  it('rejects a notification id that could not be sent as a collapse header', () => {
+    for (const notificationId of ['line\nbreak', 'nul\0byte', 'émoji', '\t']) {
+      expect(
+        PushSendRequestSchema.safeParse({
+          v: 1,
+          registrationIds: ['reg-1'],
+          notification: { ...notification(), notificationId }
+        }).success
+      ).toBe(false)
+    }
+    expect(
+      PushSendRequestSchema.safeParse({
+        v: 1,
+        registrationIds: ['reg-1'],
+        notification: {
+          ...notification(),
+          notificationId: 'agent:repo%3A%3A%2FUsers%2Fme:pane-1:1700000000000'
+        }
+      }).success
+    ).toBe(true)
+  })
+
   it('dedupes repeated registration ids and keeps the first-seen order', () => {
     const parsed = PushSendRequestSchema.safeParse({
       v: 1,

cloud/packages/push-contract/src/send-messages.ts

@@ -9,7 +9,14 @@ import { OpaqueIdSchema, SequenceSchema } from './wire-scalars.js'
 export const PushNotificationSchema = z
   .object({
     // Absent for terminal-bell, which the desktop raises without a notification record.
-    notificationId: z.string().min(1).max(256).optional(),
+    // Printable ASCII only: the id becomes the APNs collapse header, and the
+    // desktop builds it from URL-encoded parts, so anything else is not Orca's.
+    notificationId: z
+      .string()
+      .min(1)
+      .max(256)
+      .regex(/^[\x20-\x7e]+$/)
+      .optional(),
     notificationSeq: SequenceSchema,
     notificationEpoch: OpaqueIdSchema,
     source: PushNotificationSourceSchema,