Base clientSecretAuth on the verification result

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 });

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<string, unknown> = {}) {
+  return {
+    id: CLIENT_ID,
+    projectId: 'proj-1',
+    secret: 'stored-hash',
+    ignoreCorsAndSecret: false,
+    ...overrides,
+    project: {
+      cors: [ORIGIN],
+      allowUnsafeRevenueTracking: false,
+      filters: [],
+      ...((overrides.project as Record<string, unknown>) ?? {}),
+    },
+  };
+}
+
+function makeReq({
+  headers = {},
+  revenue = false,
+}: {
+  headers?: Record<string, string>;
+  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<never> & { 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'
+    );
+  });
+});

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<boolean> {
+  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');