diff --git a/apps/api/src/hooks/is-bot.hook.test.ts b/apps/api/src/hooks/is-bot.hook.test.ts index 4f1bba10f..850b50eaa 100644 --- a/apps/api/src/hooks/is-bot.hook.test.ts +++ b/apps/api/src/hooks/is-bot.hook.test.ts @@ -73,6 +73,18 @@ describe('isBotHook', () => { expect(status).toHaveBeenCalledWith(202); }); + it('still handles bots when a client secret was sent but did not verify', async () => { + isBot.mockResolvedValue({ name: 'Googlebot', type: 'Search bot' }); + const req = makeReq({ clientSecretAuth: false }); + const { reply, status } = makeReply(); + + await isBotHook(req as never, reply); + + expect(isBot).toHaveBeenCalledWith('Googlebot/2.1'); + expect(createBotEvent).toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(202); + }); + it('passes legitimate public traffic through untouched', async () => { isBot.mockResolvedValue(null); const req = makeReq({ headers: { 'user-agent': 'node' } as never }); diff --git a/apps/api/src/utils/auth.test.ts b/apps/api/src/utils/auth.test.ts new file mode 100644 index 000000000..cf52a4e3c --- /dev/null +++ b/apps/api/src/utils/auth.test.ts @@ -0,0 +1,203 @@ +/** + * Tests for validateSdkRequest — the ingestion auth check behind POST /track + * and the deprecated POST /event. + * + * The behaviour guarded here: `req.clientSecretAuth` and revenue ingestion + * follow whether the supplied secret verified against the stored hash, not + * whether a secret string was present on the request. + */ + +import type { FastifyRequest } from 'fastify'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const verifyPassword = vi.fn(); +const getClientByIdCached = vi.fn(); +const redisGet = vi.fn(); +const redisSetex = vi.fn(); + +vi.mock('@openpanel/common/server', () => ({ + verifyPassword: (...args: unknown[]) => verifyPassword(...args), +})); +vi.mock('@openpanel/db', () => ({ + ClientType: { read: 'read', write: 'write', root: 'root' }, + getClientByIdCached: (...args: unknown[]) => getClientByIdCached(...args), +})); +vi.mock('@openpanel/redis', () => ({ + getRedisCache: () => ({ get: redisGet, setex: redisSetex }), +})); + +const { validateSdkRequest } = await import('./auth'); + +const CLIENT_ID = '11111111-1111-4111-8111-111111111111'; +const ORIGIN = 'https://app.example.com'; + +function makeClient(overrides: Record = {}) { + return { + id: CLIENT_ID, + projectId: 'proj-1', + secret: 'stored-hash', + ignoreCorsAndSecret: false, + ...overrides, + project: { + cors: [ORIGIN], + allowUnsafeRevenueTracking: false, + filters: [], + ...((overrides.project as Record) ?? {}), + }, + }; +} + +function makeReq({ + headers = {}, + revenue = false, +}: { + headers?: Record; + revenue?: boolean; +} = {}) { + return { + headers: { 'openpanel-client-id': CLIENT_ID, ...headers }, + clientIp: '1.2.3.4', + body: { + type: 'track', + payload: { + name: 'purchase', + properties: revenue ? { __revenue: 42 } : {}, + }, + }, + } as unknown as FastifyRequest & { clientSecretAuth?: boolean }; +} + +beforeEach(() => { + verifyPassword.mockReset(); + getClientByIdCached.mockReset(); + redisGet.mockReset(); + redisSetex.mockReset(); + redisGet.mockResolvedValue(null); + redisSetex.mockResolvedValue('OK'); + getClientByIdCached.mockResolvedValue(makeClient()); +}); + +describe('validateSdkRequest', () => { + it('does not mark a request authenticated when the secret does not match', async () => { + verifyPassword.mockResolvedValue(false); + const req = makeReq({ + headers: { origin: ORIGIN, 'openpanel-client-secret': 'guessed' }, + }); + + await expect(validateSdkRequest(req as never)).resolves.toMatchObject({ + id: CLIENT_ID, + }); + expect(req.clientSecretAuth).toBe(false); + expect(redisSetex).not.toHaveBeenCalled(); + }); + + it('rejects revenue from an origin-authorized request with a bad secret', async () => { + verifyPassword.mockResolvedValue(false); + const req = makeReq({ + headers: { origin: ORIGIN, 'openpanel-client-secret': 'guessed' }, + revenue: true, + }); + + await expect(validateSdkRequest(req as never)).rejects.toThrow( + 'Revenue tracking is not allowed without a client secret' + ); + expect(req.clientSecretAuth).toBe(false); + }); + + it('rejects a bad secret outright when no origin is allowed', async () => { + verifyPassword.mockResolvedValue(false); + const req = makeReq({ + headers: { 'openpanel-client-secret': 'guessed' }, + }); + + await expect(validateSdkRequest(req as never)).rejects.toThrow( + 'Invalid cors or secret' + ); + expect(req.clientSecretAuth).toBe(false); + }); + + it('lets ordinary browser traffic through on the origin alone', async () => { + const req = makeReq({ headers: { origin: ORIGIN } }); + + await expect(validateSdkRequest(req as never)).resolves.toMatchObject({ + id: CLIENT_ID, + }); + expect(req.clientSecretAuth).toBe(false); + expect(verifyPassword).not.toHaveBeenCalled(); + }); + + it('authorizes a correct secret without an origin and accepts revenue', async () => { + verifyPassword.mockResolvedValue(true); + const req = makeReq({ + headers: { 'openpanel-client-secret': 'correct' }, + revenue: true, + }); + + await expect(validateSdkRequest(req as never)).resolves.toMatchObject({ + id: CLIENT_ID, + }); + expect(req.clientSecretAuth).toBe(true); + expect(redisSetex).toHaveBeenCalledWith( + expect.stringContaining(`client:auth:${CLIENT_ID}:`), + 300, + 'true' + ); + }); + + it('trusts a cached successful verification without re-hashing', async () => { + redisGet.mockResolvedValue('true'); + const req = makeReq({ headers: { 'openpanel-client-secret': 'correct' } }); + + await validateSdkRequest(req as never); + + expect(req.clientSecretAuth).toBe(true); + expect(verifyPassword).not.toHaveBeenCalled(); + }); + + it('does not trust a cached "false" left over from earlier releases', async () => { + redisGet.mockResolvedValue('false'); + verifyPassword.mockResolvedValue(false); + const req = makeReq({ + headers: { origin: ORIGIN, 'openpanel-client-secret': 'guessed' }, + }); + + await validateSdkRequest(req as never); + + expect(req.clientSecretAuth).toBe(false); + expect(verifyPassword).toHaveBeenCalled(); + }); + + it('skips the cache entirely when the client has no stored secret', async () => { + getClientByIdCached.mockResolvedValue(makeClient({ secret: null })); + const req = makeReq({ + headers: { origin: ORIGIN, 'openpanel-client-secret': 'anything' }, + }); + + await validateSdkRequest(req as never); + + expect(req.clientSecretAuth).toBe(false); + expect(redisGet).not.toHaveBeenCalled(); + expect(redisSetex).not.toHaveBeenCalled(); + expect(verifyPassword).not.toHaveBeenCalled(); + }); + + it('accepts revenue with no secret when allowUnsafeRevenueTracking is on', async () => { + getClientByIdCached.mockResolvedValue( + makeClient({ project: { allowUnsafeRevenueTracking: true } }) + ); + const req = makeReq({ headers: { origin: ORIGIN }, revenue: true }); + + await expect(validateSdkRequest(req as never)).resolves.toMatchObject({ + id: CLIENT_ID, + }); + expect(req.clientSecretAuth).toBe(false); + }); + + it('rejects revenue with no secret when allowUnsafeRevenueTracking is off', async () => { + const req = makeReq({ headers: { origin: ORIGIN }, revenue: true }); + + await expect(validateSdkRequest(req as never)).rejects.toThrow( + 'Revenue tracking is not allowed without a client secret' + ); + }); +}); diff --git a/apps/api/src/utils/auth.ts b/apps/api/src/utils/auth.ts index 70cb59862..7415fc9ca 100644 --- a/apps/api/src/utils/auth.ts +++ b/apps/api/src/utils/auth.ts @@ -1,7 +1,7 @@ import { verifyPassword } from '@openpanel/common/server'; import type { IServiceClientWithProject } from '@openpanel/db'; import { ClientType, getClientByIdCached } from '@openpanel/db'; -import { getCache } from '@openpanel/redis'; +import { getRedisCache } from '@openpanel/redis'; import type { DeprecatedPostEventPayload, IProjectFilterIp, @@ -39,6 +39,46 @@ export class SdkAuthError extends Error { } } +const CLIENT_SECRET_CACHE_SEC = 60 * 5; + +/** + * Checks a supplied client secret against the stored hash. + * + * Only successful verifications are cached. The cache key contains the + * caller-supplied secret, so caching a negative result would let anyone create + * entries with keys of their choosing. A client with no stored secret skips the + * cache entirely. + */ +async function verifyClientSecret( + clientId: string, + clientSecret: string | undefined, + storedSecret: string | null | undefined +): Promise { + if (!(storedSecret && clientSecret)) { + return false; + } + + const cacheKey = `client:auth:${clientId}:${Buffer.from(clientSecret).toString('base64')}`; + + // Strict compare: entries written before only positives were cached may still + // hold "false". + if ((await getRedisCache().get(cacheKey)) === 'true') { + return true; + } + + const isVerified = await verifyPassword(clientSecret, storedSecret); + + if (isVerified) { + getRedisCache() + .setex(cacheKey, CLIENT_SECRET_CACHE_SEC, 'true') + .catch(() => { + // ignore error + }); + } + + return isVerified; +} + export async function validateSdkRequest( req: FastifyRequest<{ Body: ITrackHandlerPayload | DeprecatedPostEventPayload; @@ -59,10 +99,6 @@ export async function validateSdkRequest( clientSecretNew || clientSecretOld || clientSecretFromBody; const origin = headers.origin; - if (clientSecret) { - req.clientSecretAuth = true; - } - const createError = (message: string) => new SdkAuthError(message, { clientId, @@ -95,6 +131,16 @@ export async function validateSdkRequest( throw createError('Ingestion: Client has no project'); } + // Whether the supplied secret actually matches the stored hash. Everything + // downstream keys off this, not off the mere presence of a secret. + const secretVerified = await verifyClientSecret( + clientId, + clientSecret, + client.secret + ); + + req.clientSecretAuth = secretVerified; + // Filter out blocked IPs const ipFilter = client.project.filters.filter( (filter): filter is IProjectFilterIp => filter.type === 'ip' @@ -119,10 +165,10 @@ export async function validateSdkRequest( path(['payload', 'properties', '__revenue'], req.body) ?? path(['properties', '__revenue'], req.body); - // Only allow revenue tracking if it was sent with a client secret + // Only allow revenue tracking if it was sent with a verified client secret // or if the project has allowUnsafeRevenueTracking enabled if ( - !(client.project.allowUnsafeRevenueTracking || clientSecret) && + !(client.project.allowUnsafeRevenueTracking || secretVerified) && typeof revenue !== 'undefined' ) { throw createError( @@ -160,16 +206,8 @@ export async function validateSdkRequest( } } - if (client.secret && clientSecret) { - const isVerified = await getCache( - `client:auth:${clientId}:${Buffer.from(clientSecret).toString('base64')}`, - 60 * 5, - async () => await verifyPassword(clientSecret, client.secret!), - true - ); - if (isVerified) { - return client; - } + if (secretVerified) { + return client; } throw createError('Ingestion: Invalid cors or secret');