From a412247bf366164c5ddc333d1207319205c82375 Mon Sep 17 00:00:00 2001 From: null Date: Tue, 16 Jun 2026 22:42:53 -0500 Subject: [PATCH] =?UTF-8?q?security:=20kimi-k2.7=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20Ed25519=20crypto=20API,=20Firestore=20rules=20try/c?= =?UTF-8?q?atch=20removal,=20atomic=20idempotency,=20RevenueCat=208.20.0,?= =?UTF-8?q?=20rate=20limiter=20fix,=20remove=20plaintext=20fallback,=20tig?= =?UTF-8?q?hten=20push=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/build.gradle.kts | 4 +- .../SharedPreferencesLocalAnswerRepository.kt | 24 ++--- firestore.rules | 72 +++++++------ server/.env.example | 14 ++- server/src/middleware/rateLimiter.ts | 11 +- server/src/routes/webhooks.ts | 102 +++++++++++++----- server/src/services/entitlement.ts | 38 ++++--- server/src/services/fcm.ts | 8 +- settings.gradle.kts | 3 - 9 files changed, 172 insertions(+), 104 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 317d291f..71b5e928 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -97,8 +97,8 @@ dependencies { // Encrypted storage implementation("androidx.security:security-crypto:1.0.0") - // RevenueCat - implementation("com.revenuecat.purchases:purchases:10.10.0") + // RevenueCat native Android SDK (group: com.revenuecat.purchases, artifact: purchases) + implementation("com.revenuecat.purchases:purchases:8.20.0") // Coroutines implementation("org.jetbrains.kotlinx:kotlinx-coroutines-android:1.9.0") diff --git a/app/src/main/java/app/closer/data/repository/SharedPreferencesLocalAnswerRepository.kt b/app/src/main/java/app/closer/data/repository/SharedPreferencesLocalAnswerRepository.kt index 21466b1d..a6404424 100644 --- a/app/src/main/java/app/closer/data/repository/SharedPreferencesLocalAnswerRepository.kt +++ b/app/src/main/java/app/closer/data/repository/SharedPreferencesLocalAnswerRepository.kt @@ -23,20 +23,16 @@ class SharedPreferencesLocalAnswerRepository @Inject constructor( private val prefs: SharedPreferences = run { // Remove legacy plaintext file on first migration context.deleteSharedPreferences("local_answers") - try { - // In 1.0.0, EncryptedSharedPreferences.create takes (alias, name, context, ...) - val masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC) - EncryptedSharedPreferences.create( - masterKeyAlias, // alias (first param) - "local_answers_secure", // name (second param) - context, // context (third param) - EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, - EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM - ) - } catch (_: Exception) { - // Fallback to unencrypted SharedPreferences if encryption fails - context.getSharedPreferences("local_answers_secure", Context.MODE_PRIVATE) - } + + // In security-crypto 1.0.0, EncryptedSharedPreferences.create takes (alias, name, context, ...) + val masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC) + EncryptedSharedPreferences.create( + masterKeyAlias, // alias (first param) + "local_answers_secure", // name (second param) + context, // context (third param) + EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, + EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM + ) } private val answers = MutableStateFlow(readAnswers()) diff --git a/firestore.rules b/firestore.rules index 86c2fa4c..e1b2561e 100644 --- a/firestore.rules +++ b/firestore.rules @@ -24,21 +24,14 @@ service cloud.firestore { } function isNotAlreadyPaired() { - // Check that the requesting user does not already have a coupleId - // Handle case where user doc might not exist (use getIfExists to avoid throw) - try { - let userDoc = get(/databases/$(database)/documents/users/$(request.auth.uid)); - return !('coupleId' in userDoc.data) || userDoc.data.coupleId == null; - } catch (e) { - // User doc doesn't exist - treat as unpaired - return true; - } + // Check that the requesting user does not already have a coupleId. + // A missing user doc is treated as unpaired. + let userPath = /databases/$(database)/documents/users/$(request.auth.uid); + return !exists(userPath) || get(userPath).data.coupleId == null; } - function isServer() { - // Check if request comes from admin SDK (no request.auth) - return request.auth == null; - } + // Admin SDK / Cloud Functions bypass Firestore rules, so any operation that + // must only be performed server-side is denied for all direct client writes. function isImmutable(fields) { // Helper to check that certain fields haven't changed during an update @@ -83,16 +76,22 @@ service cloud.firestore { // Expired invites should not be readable by non-inviters && (request.auth.uid == resource.data.inviterUserId || request.time < resource.data.expiresAt); - // Create: ownership, code format, and required fields validation + // Create: ownership, code format, and required fields validation. + // hasOnly prevents injecting unrelated fields (e.g. coupleId) at creation. allow create: if isSignedIn() && request.resource.data.inviterUserId == request.auth.uid && isValidInviteCode(code) && isValidInviteCode(request.resource.data.code) && request.resource.data.code == code && request.resource.data.status == 'pending' - && request.resource.data.keys().hasAll(['inviterUserId', 'code', 'status', 'expiresAt']); + && request.resource.data.expiresAt is timestamp + && request.time < request.resource.data.expiresAt + && request.resource.data.keys().hasAll(['inviterUserId', 'code', 'status', 'expiresAt']) + && request.resource.data.keys().hasOnly(['inviterUserId', 'code', 'status', 'expiresAt']); - // Update (accept): proper validation for changing status to accepted + // Update (accept): proper validation for changing status to accepted. + // If coupleId is supplied, it must reference an existing couple where + // the acceptor is a member. (Server-side creation bypasses rules.) allow update: if isSignedIn() && resource.data.status == 'pending' // Cannot accept your own invite @@ -101,13 +100,23 @@ service cloud.firestore { && request.resource.data.acceptorUserId == request.auth.uid // Status must change to accepted && request.resource.data.status == 'accepted' - // Acceptance timestamp must be set + // Acceptance timestamp must be set and be a Firestore timestamp && request.resource.data.acceptedAt != null + && request.resource.data.acceptedAt is timestamp // No other fields should be modified in this update && request.resource.data.keys().hasOnly( ['status', 'acceptorUserId', 'acceptedAt', 'coupleId']) // Expired invites cannot be accepted - && request.time < resource.data.expiresAt; + && request.time < resource.data.expiresAt + // coupleId, if provided, must point to a real couple that includes the acceptor + && ( + !('coupleId' in request.resource.data) + || ( + request.resource.data.coupleId != null + && exists(/databases/$(database)/documents/couples/$(request.resource.data.coupleId)) + && request.auth.uid in get(/databases/$(database)/documents/couples/$(request.resource.data.coupleId)).data.userIds + ) + ); } // ── Couples ─────────────────────────────────────────────────────────────── @@ -118,8 +127,9 @@ service cloud.firestore { // Read: both members can read allow read: if isCouplesMember(coupleId); - // Create: only via invite flow (server-side or admin SDK) - allow create: if isServer(); + // Create: only via invite flow (server-side or admin SDK). + // Admin SDK bypasses rules; direct client writes are denied. + allow create: if false; // Update: field-level restrictions // - user IDs are immutable (cannot change who is in the couple) @@ -133,8 +143,8 @@ service cloud.firestore { // streakCount and lastStreakAt must not be modified by clients && !request.resource.data.diff(resource.data).affectedKeys().hasAny(['streakCount', 'lastStreakAt']); - // Delete: server-only (admin SDK only) - allow delete: if isServer(); + // Delete: server-only (admin SDK only). Admin SDK bypasses rules. + allow delete: if false; match /sessions/{sessionId} { // Read: both members can read @@ -144,18 +154,20 @@ service cloud.firestore { allow create: if isCouplesMember(coupleId) && request.resource.data.startedByUserId == request.auth.uid; - // Update: only the user who started the session can update it, OR valid status transitions + // Update: only the user who started the session can update it, OR valid status transitions. + // startedByUserId is immutable for direct client writes. allow update: if isCouplesMember(coupleId) // Either the original starter can update && (resource.data.startedByUserId == request.auth.uid // Or status transition is valid: active → completed || (resource.data.status == 'active' && request.resource.data.status == 'completed')) - // Check that other fields haven't been tampered with - && (request.resource.data.startedByUserId == resource.data.startedByUserId - || isServer()); + // startedByUserId cannot be changed by clients + && request.resource.data.startedByUserId == resource.data.startedByUserId + // Only a fixed set of fields may change + && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['status', 'completedAt']); - // Delete: server-only (admin SDK) - allow delete: if isServer(); + // Delete: server-only (admin SDK). Admin SDK bypasses rules. + allow delete: if false; } // Question threads live under the couple document. @@ -180,8 +192,8 @@ service cloud.firestore { // No other fields should change && request.resource.data.diff(resource.data).affectedKeys().hasOnly(['status', 'currentIndex']); - // Delete: server-only (admin SDK) - allow delete: if isServer(); + // Delete: server-only (admin SDK). Admin SDK bypasses rules. + allow delete: if false; // Answers: each user writes their own; both members can read all answers. match /answers/{userId} { diff --git a/server/.env.example b/server/.env.example index f5b0f383..5059cf76 100644 --- a/server/.env.example +++ b/server/.env.example @@ -6,9 +6,21 @@ FIREBASE_SERVICE_ACCOUNT_PATH=/run/secrets/firebase-service-account.json # Your Firebase project ID FIREBASE_PROJECT_ID=your-project-id -# RevenueCat webhook shared secret +# RevenueCat webhook shared secret (legacy; prefer signing key below) # Set this in RevenueCat Dashboard > Project > Integrations > Webhooks > Authorization header REVENUECAT_WEBHOOK_SECRET=your-revenuecat-webhook-secret +# RevenueCat webhook Ed25519 signing key (modern, preferred) +# Found in RevenueCat Dashboard > Project > Integrations > Webhooks > Webhook signing +REVENUECAT_SIGNING_KEY=base64-ed25519-public-key + +# Comma-separated product IDs that grant premium access +REVENUECAT_PREMIUM_PRODUCT_IDS=closer_premium_monthly,closer_premium_yearly + +# Rate limits (requests per minute) +RATE_LIMIT_WEBHOOK=10 +RATE_LIMIT_HEALTH=30 +RATE_LIMIT_DEFAULT=60 + # Server port (default 8080) PORT=8080 diff --git a/server/src/middleware/rateLimiter.ts b/server/src/middleware/rateLimiter.ts index b0ab75a8..74a7463c 100644 --- a/server/src/middleware/rateLimiter.ts +++ b/server/src/middleware/rateLimiter.ts @@ -18,42 +18,41 @@ const DEFAULT_LIMIT = getRateLimit('RATE_LIMIT_DEFAULT', 60) /** * Webhook limiter: 10 requests per minute per IP (configurable via RATE_LIMIT_WEBHOOK) - * RevenueCat retries are spaced minutes apart, so this is generous + * RevenueCat retries are spaced minutes apart, so this is generous. + * Successful webhooks (HTTP 200) DO count toward the limit — a processed + * webhook should not give an attacker unlimited free requests. */ export const webhookLimiter = rateLimit({ windowMs: 60 * 1000, // 1 minute max: WEBHOOK_LIMIT, standardHeaders: true, legacyHeaders: false, - skipSuccessfulRequests: true, keyGenerator: (req) => req.ip || req.socket?.remoteAddress || 'unknown', skip: (req) => skipLocalhost(req), }) /** * Health limiter: 30 requests per minute per IP (configurable via RATE_LIMIT_HEALTH) - * Allows frequent health checks without abuse + * Allows frequent health checks without abuse. */ export const healthLimiter = rateLimit({ windowMs: 60 * 1000, // 1 minute max: HEALTH_LIMIT, standardHeaders: true, legacyHeaders: false, - skipSuccessfulRequests: true, keyGenerator: (req) => req.ip || req.socket?.remoteAddress || 'unknown', skip: (req) => skipLocalhost(req), }) /** * Default/API limiter: 60 requests per minute per IP (configurable via RATE_LIMIT_DEFAULT) - * For future authenticated routes + * For future authenticated routes. */ export const defaultLimiter = rateLimit({ windowMs: 60 * 1000, // 1 minute max: DEFAULT_LIMIT, standardHeaders: true, legacyHeaders: false, - skipSuccessfulRequests: true, keyGenerator: (req) => req.ip || req.socket?.remoteAddress || 'unknown', skip: (req) => skipLocalhost(req), }) diff --git a/server/src/routes/webhooks.ts b/server/src/routes/webhooks.ts index 271dd0c2..747a3ff5 100644 --- a/server/src/routes/webhooks.ts +++ b/server/src/routes/webhooks.ts @@ -1,15 +1,44 @@ import { Router, Request, Response } from 'express' import { syncEntitlement } from '../services/entitlement' import { RevenueCatEvent } from '../types' -import { getEnv } from '../config/env' +import { getEnvValue } from '../config/env' import * as crypto from 'crypto' const router = Router() +/** + * Loads a RevenueCat Ed25519 public key from its base64 form. + * RevenueCat typically distributes the raw 32-byte public key (base64). + * We also accept DER SPKI form for forward compatibility. + */ +function loadRevenueCatPublicKey(base64Key: string): crypto.KeyObject { + const keyBytes = Buffer.from(base64Key, 'base64') + + if (keyBytes.length === 32) { + // Raw Ed25519 public key -> encode as JWK + return crypto.createPublicKey({ + key: { + kty: 'OKP', + crv: 'Ed25519', + x: keyBytes.toString('base64url'), + }, + format: 'jwk', + }) + } + + // Otherwise assume DER-encoded SubjectPublicKeyInfo + return crypto.createPublicKey({ + key: keyBytes, + format: 'der', + type: 'spki', + }) +} + /** * Verifies RevenueCat webhook signature using Ed25519. * RevenueCat signs the raw request body with their signing key. - * If REVENUECAT_SIGNING_KEY is not set, throws an error (fail-closed). + * Returns false for invalid signature/header; throws only for misconfiguration + * so the caller can return a 500 (fail-closed). */ function verifyRevenueCatSignature(req: Request): boolean { const signingKey = process.env.REVENUECAT_SIGNING_KEY @@ -34,14 +63,14 @@ function verifyRevenueCatSignature(req: Request): boolean { } try { - // RevenueCat sends base64-encoded Ed25519 signature - const signature = Buffer.from(signatureHeader, 'base64') - const publicKey = Buffer.from(signingKey, 'base64') + const signature = Buffer.from( + Array.isArray(signatureHeader) ? signatureHeader[0] : signatureHeader, + 'base64' + ) + const publicKey = loadRevenueCatPublicKey(signingKey) - // Verify using Ed25519 - const verify = crypto.createVerify('Ed25519') - verify.update(rawBody) - return verify.verify(publicKey, signature) + // Ed25519 uses no digest algorithm in Node.js + return crypto.verify(null, rawBody, publicKey, signature) } catch (err) { console.error('[webhook] Signature verification failed:', err) return false @@ -50,25 +79,30 @@ function verifyRevenueCatSignature(req: Request): boolean { /** * Verifies RevenueCat webhook secret using constant-time comparison. - * DEPRECATED: Now uses signature verification. Kept for backwards compatibility. + * Kept as a fallback when signature verification is not configured. + * A missing secret causes verification to fail (fail-closed). */ function verifyRevenueCatSecret(req: Request): boolean { try { - const secret = getEnv('REVENUECAT_WEBHOOK_SECRET') - const auth = req.headers['authorization'] ?? '' + const secret = getEnvValue('REVENUECAT_WEBHOOK_SECRET') + const authHeader = req.headers['authorization'] + const auth = (Array.isArray(authHeader) ? authHeader[0] : authHeader) ?? '' + + // Missing secret = fail closed + if (!secret) { + console.error('[webhook] REVENUECAT_WEBHOOK_SECRET not set — rejecting all requests (fail-closed)') + return false + } // Constant-time comparison to prevent timing attacks const authBuffer = Buffer.from(auth) const secretBuffer = Buffer.from(secret) - // Use timingSafeEqual with fixed buffer length to prevent length leakage - const maxLength = Math.max(authBuffer.length, secretBuffer.length) - const paddedAuth = Buffer.alloc(maxLength) - const paddedSecret = Buffer.alloc(maxLength) - authBuffer.copy(paddedAuth) - secretBuffer.copy(paddedSecret) + if (authBuffer.length !== secretBuffer.length) { + return false + } - return crypto.timingSafeEqual(paddedAuth, paddedSecret) + return crypto.timingSafeEqual(authBuffer, secretBuffer) } catch (err) { console.error('[webhook] Secret verification error:', err) return false @@ -76,16 +110,30 @@ function verifyRevenueCatSecret(req: Request): boolean { } router.post('/revenuecat', async (req: Request, res: Response) => { - // Try signature verification first (modern, prefered) + const signatureKeyConfigured = !!process.env.REVENUECAT_SIGNING_KEY?.trim() + const secretConfigured = !!process.env.REVENUECAT_WEBHOOK_SECRET?.trim() + + // Fail closed: at least one auth method must be configured + if (!signatureKeyConfigured && !secretConfigured) { + console.error('[webhook] No webhook auth configured — rejecting all requests (fail-closed)') + res.status(500).json({ error: 'webhook_auth_not_configured' }) + return + } + + let verified = false try { - if (!verifyRevenueCatSignature(req)) { - res.status(401).json({ error: 'unauthorized' }) - return - } + verified = signatureKeyConfigured + ? verifyRevenueCatSignature(req) + : verifyRevenueCatSecret(req) } catch (err: any) { - // If signature verification throws (e.g., missing key), reject with 500 - console.error('[webhook] Signature verification error:', err) - res.status(500).json({ error: 'signature_verification_failed', message: err.message }) + // Configuration/internal errors are surfaced as 500 (fail-closed) + console.error('[webhook] Auth verification error:', err) + res.status(500).json({ error: 'auth_verification_failed', message: err.message }) + return + } + + if (!verified) { + res.status(401).json({ error: 'unauthorized' }) return } diff --git a/server/src/services/entitlement.ts b/server/src/services/entitlement.ts index 0b5b134b..50ed4c0d 100644 --- a/server/src/services/entitlement.ts +++ b/server/src/services/entitlement.ts @@ -1,6 +1,7 @@ import { db } from '../config/firebase' import { RevenueCatEvent } from '../types' import { getEnvValue } from '../config/env' +import * as admin from 'firebase-admin' // Product IDs that grant premium access (comma-separated, configurable via env) const DEFAULT_PREMIUM_PRODUCT_IDS = 'closer_premium_monthly,closer_premium_yearly' @@ -43,28 +44,32 @@ export async function verifyPremiumActive(uid: string): Promise { // hasPremium must be true AND expiration must be in the future if (!data.hasPremium) return false - const expiresAt = data.premiumExpiresAt as FirebaseFirestore.Timestamp | undefined + const expiresAt = data.premiumExpiresAt as FirebaseFirestore.Timestamp | Date | undefined if (!expiresAt) return false const now = new Date() - const expirationDate = expiresAt.toDate() + const expirationDate = typeof (expiresAt as any).toDate === 'function' + ? (expiresAt as FirebaseFirestore.Timestamp).toDate() + : (expiresAt as Date) return expirationDate > now } export async function syncEntitlement(event: RevenueCatEvent): Promise { const { type, app_user_id: uid, id: eventId, product_id } = event.event - // Check idempotency - skip if we've already processed this event + // Idempotency: atomically create the event record. If it already exists, + // another concurrent request processed it and we abort cleanly. const eventRef = db().collection('entitlement_events').doc(eventId) - const eventDoc = await eventRef.get() - if (eventDoc.exists) { - console.log(`[entitlement] skipping duplicate event: ${eventId}`) - return + try { + await eventRef.create({ processedAt: new Date() }) + } catch (err: any) { + if (err?.code === 6 || err?.message?.includes('ALREADY_EXISTS')) { + console.log(`[entitlement] skipping duplicate event: ${eventId}`) + return + } + throw err } - // Store event for idempotency (with 7-day TTL via Firestore rule or scheduled cleanup) - await eventRef.set({ processedAt: new Date() }) - // Validate product_id against allowlist if (!PRODUCT_ID_ALLOWLIST.has(product_id)) { console.log(`[entitlement] ignored event for unknown product_id: ${product_id}`) @@ -78,27 +83,26 @@ export async function syncEntitlement(event: RevenueCatEvent): Promise { return } + const userRef = db().collection('users').doc(uid) + if (PREMIUM_ACTIVE_TYPES.has(type)) { const updates: { hasPremium: true; premiumExpiresAt?: FirebaseFirestore.Timestamp } = { hasPremium: true } // Store expiration timestamp if provided const expirationAtMs = event.event.expiration_at_ms if (expirationAtMs !== undefined) { - updates.premiumExpiresAt = new db().firestore.Timestamp.fromMillis(expirationAtMs) + updates.premiumExpiresAt = admin.firestore.Timestamp.fromMillis(expirationAtMs) } - await db().collection('users').doc(uid).update(updates) + await userRef.update(updates) console.log(`[entitlement] hasPremium=true for ${uid} (${type})`) return } if (PREMIUM_REVOKED_TYPES.has(type)) { - const updates: { hasPremium: false; premiumExpiresAt?: null } = { hasPremium: false } + const updates: { hasPremium: false; premiumExpiresAt: null } = { hasPremium: false, premiumExpiresAt: null } - // Clear expiration timestamp for revocation events - updates.premiumExpiresAt = null as any - - await db().collection('users').doc(uid).update(updates) + await userRef.update(updates) console.log(`[entitlement] hasPremium=false for ${uid} (${type})`) return } diff --git a/server/src/services/fcm.ts b/server/src/services/fcm.ts index 273865e5..108c9bb7 100644 --- a/server/src/services/fcm.ts +++ b/server/src/services/fcm.ts @@ -24,17 +24,17 @@ export async function sendPushToUser( /** * Notify a user that their partner has answered a question. - * Privacy: Use generic wording that doesn’t reveal question content, categories, or answer previews. + * Privacy: Use generic wording that doesn’t reveal question content, categories, + * answer previews, or even the partner's name on the lock screen. */ export async function sendPartnerAnsweredNotification( partnerFcmToken: string, - partnerName: string + _partnerName: string ): Promise { - // Partner name is used only to personalize the greeting; no specific context is included await sendPushToUser( partnerFcmToken, 'You have something new from your partner', - `${partnerName} answered a question. Tap to see what they said.`, + 'Tap to open Closer and see what they shared.', { type: 'partner_answered' } ) } diff --git a/settings.gradle.kts b/settings.gradle.kts index b1f71842..9cb2bb5b 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -10,9 +10,6 @@ dependencyResolutionManagement { repositories { google() mavenCentral() - maven(url = "https://central.sonatype.com/repository/maven-snapshots/") { - content { includeGroup("com.revenuecat.purchases") } - } } } rootProject.name = "Closer"