From c8ecd7830aa18e0d7dab54be86a4ed2f858ae72d Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 4 Sep 2026 01:53:39 -0400 Subject: [PATCH] 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 | 1 + .../apps/push/src/host-session-store.test.ts | 17 +++--- cloud/apps/push/src/host-session-store.ts | 26 ++++----- cloud/apps/push/src/index.ts | 2 +- cloud/apps/push/src/push-observability.ts | 2 + .../src/push-server-harness.test-fixture.ts | 3 +- .../apps/push/src/push-server-limits.test.ts | 57 ++++++++++++++++--- cloud/apps/push/src/push-server.ts | 30 +++++++++- .../scripts/push-gateway-workflow.test.mjs | 7 +++ cloud/docs/push-gateway.md | 9 +++ cloud/infra/terraform/push-gateway.tf | 11 ++++ .../push-contract/src/contract.test.ts | 5 +- .../packages/push-contract/src/push-limits.ts | 12 +++- .../push-contract/src/send-messages.test.ts | 22 +++++++ .../push-contract/src/send-messages.ts | 9 ++- 15 files changed, 175 insertions(+), 38 deletions(-) diff --git a/.github/workflows/cloud-push-deploy.yml b/.github/workflows/cloud-push-deploy.yml index b165aa4a38c..53af9f5d755 100644 --- a/.github/workflows/cloud-push-deploy.yml +++ b/.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 \ diff --git a/cloud/apps/push/src/host-session-store.test.ts b/cloud/apps/push/src/host-session-store.test.ts index a59f2e1d8c4..129dba2134c 100644 --- a/cloud/apps/push/src/host-session-store.test.ts +++ b/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 }) }) }) diff --git a/cloud/apps/push/src/host-session-store.ts b/cloud/apps/push/src/host-session-store.ts index 6442d4326c5..d400ce9b926 100644 --- a/cloud/apps/push/src/host-session-store.ts +++ b/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 { - const [result] = await this.database.query( - 'DELETE FROM push_sessions WHERE host_fingerprint = ?', - [hostFingerprint] - ) - return Number(result?.changes ?? 0) - } - async pruneExpired(): Promise { const [result] = await this.database.query('DELETE FROM push_sessions WHERE expires_at < ?', [ this.now() diff --git a/cloud/apps/push/src/index.ts b/cloud/apps/push/src/index.ts index baca364addd..04d259801fd 100644 --- a/cloud/apps/push/src/index.ts +++ b/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({ diff --git a/cloud/apps/push/src/push-observability.ts b/cloud/apps/push/src/push-observability.ts index bbfc18ac9a6..c5a8c6d233e 100644 --- a/cloud/apps/push/src/push-observability.ts +++ b/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', diff --git a/cloud/apps/push/src/push-server-harness.test-fixture.ts b/cloud/apps/push/src/push-server-harness.test-fixture.ts index cd7e1389d6a..60d8a6102ad 100644 --- a/cloud/apps/push/src/push-server-harness.test-fixture.ts +++ b/cloud/apps/push/src/push-server-harness.test-fixture.ts @@ -157,7 +157,8 @@ export async function createPushServerHarness() { }, close: async (): Promise => { server.coalescer.stop() - await database.close() + // A test may close the database itself to provoke a route failure. + await database.close().catch(() => undefined) } } } diff --git a/cloud/apps/push/src/push-server-limits.test.ts b/cloud/apps/push/src/push-server-limits.test.ts index fe02aa39c3c..9423e4022a7 100644 --- a/cloud/apps/push/src/push-server-limits.test.ts +++ b/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) diff --git a/cloud/apps/push/src/push-server.ts b/cloud/apps/push/src/push-server.ts index 29fe8065021..5bc58337476 100644 --- a/cloud/apps/push/src/push-server.ts +++ b/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( diff --git a/cloud/dev/scripts/push-gateway-workflow.test.mjs b/cloud/dev/scripts/push-gateway-workflow.test.mjs index d7aacedf3fe..947a9133554 100644 --- a/cloud/dev/scripts/push-gateway-workflow.test.mjs +++ b/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/) }) diff --git a/cloud/docs/push-gateway.md b/cloud/docs/push-gateway.md index 0d70d8fb757..0ca81bd370a 100644 --- a/cloud/docs/push-gateway.md +++ b/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 diff --git a/cloud/infra/terraform/push-gateway.tf b/cloud/infra/terraform/push-gateway.tf index 9fc3e4bc6fe..87d12ae2693 100644 --- a/cloud/infra/terraform/push-gateway.tf +++ b/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" { diff --git a/cloud/packages/push-contract/src/contract.test.ts b/cloud/packages/push-contract/src/contract.test.ts index e8a0cce4b85..e81ac2ad02f 100644 --- a/cloud/packages/push-contract/src/contract.test.ts +++ b/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') diff --git a/cloud/packages/push-contract/src/push-limits.ts b/cloud/packages/push-contract/src/push-limits.ts index 1d7cf5479e8..5d46b994d06 100644 --- a/cloud/packages/push-contract/src/push-limits.ts +++ b/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 = { diff --git a/cloud/packages/push-contract/src/send-messages.test.ts b/cloud/packages/push-contract/src/send-messages.test.ts index 968a40ef65b..8a261938c43 100644 --- a/cloud/packages/push-contract/src/send-messages.test.ts +++ b/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, diff --git a/cloud/packages/push-contract/src/send-messages.ts b/cloud/packages/push-contract/src/send-messages.ts index 0edb68de3d6..e0e08786423 100644 --- a/cloud/packages/push-contract/src/send-messages.ts +++ b/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,