From 3b2ad3a5284be87c490b863a3631522941e4d6bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesv=C3=A4rd?= Date: Fri, 4 Sep 2026 08:25:29 +0000 Subject: [PATCH] fix(api): base clientSecretAuth on the verification result `req.clientSecretAuth` was set from the presence of the client-secret header or body field, before anything compared it against the stored hash, so it reported "authenticated" for any string. The revenue guard read the same raw value. Both should follow whether the secret actually matched. Verification now runs once, right after the client loads, and its result feeds the flag, the revenue guard, and the final authorization check. The origin paths no longer return before that computation, so a browser request that happens to carry a non-matching secret reports clientSecretAuth false while still being authorized by its origin. Correct server-side SDK traffic and normal origin-authorized browser traffic behave exactly as before. Verification now also runs on inputs that previously skipped it, so only successful verifications are cached: the cache key embeds the supplied secret, and caching negatives would let a caller create entries with keys of their choosing. Clients with no stored secret skip the cache entirely. The read compares against "true" so entries written by earlier releases, which could hold "false", are not read as a pass. Co-Authored-By: Claude Opus 5 --- apps/api/src/hooks/is-bot.hook.test.ts | 12 ++ apps/api/src/utils/auth.test.ts | 203 +++++++++++++++++++++++++ apps/api/src/utils/auth.ts | 72 ++++++--- 3 files changed, 270 insertions(+), 17 deletions(-) create mode 100644 apps/api/src/utils/auth.test.ts 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');