fix(cbinsights): reject malformed ID lists and bound the token cache

- Reject an organization ID list containing an invalid entry instead of
  dropping it. Silently filtering meant a typo ran the request against a
  narrower set — spending credits on the wrong organizations, or quietly
  widening a filtered search — and still reported success.
- Apply the same rule to the optional firmographics ID filters, where a
  dropped filter broadens the search rather than narrowing it.
- Bound the process-wide token cache so a long-lived worker serving many
  CB Insights accounts does not grow with the cumulative number of accounts
  seen. Expired entries are swept on write, then the oldest evicted.

Author: waleedlatif1

apps/sim/tools/cbinsights/cbinsights.test.ts

@@ -10,7 +10,7 @@ import { cbinsightsListFundingsTool } from '@/tools/cbinsights/list_fundings'
 import { cbinsightsLookupOrganizationsTool } from '@/tools/cbinsights/lookup_organizations'
 import { cbinsightsRagTool } from '@/tools/cbinsights/rag'
 import { cbinsightsSearchFirmographicsTool } from '@/tools/cbinsights/search_firmographics'
-import { resetCbInsightsTokenCache } from '@/tools/cbinsights/utils'
+import { cbInsightsTokenCacheSize, resetCbInsightsTokenCache } from '@/tools/cbinsights/utils'
 
 const CREDS = { clientId: 'id', clientSecret: 'secret' }
 
@@ -133,6 +133,30 @@ describe('cbinsights authorization', () => {
   })
 })
 
+describe('cbinsights token cache', () => {
+  /*
+   * The cache is process-wide. Without a bound, a long-lived worker serving many
+   * CB Insights accounts would grow with the cumulative number of accounts seen.
+   */
+  it('stays bounded as distinct credential pairs accumulate', async () => {
+    const responses = []
+    for (let index = 0; index < 200; index++) {
+      responses.push({ body: { token: `jwt-${index}` } }, { body: { orgs: [] } })
+    }
+    mockFetch(responses)
+
+    for (let index = 0; index < 200; index++) {
+      await cbinsightsLookupOrganizationsTool.directExecution!({
+        clientId: `id-${index}`,
+        clientSecret: 'secret',
+        names: 'a',
+      } as never)
+    }
+
+    expect(cbInsightsTokenCacheSize()).toBeLessThanOrEqual(128)
+  })
+})
+
 describe('cbinsights request building', () => {
   it('rejects a lookup with no search parameter', async () => {
     mockFetch([AUTH_OK])
@@ -186,6 +210,42 @@ describe('cbinsights request building', () => {
     ).rejects.toThrow(/at most 100 organization IDs/)
   })
 
+  /*
+   * Dropping the bad entries instead would run against a silently narrower set:
+   * a typo would spend credits on the wrong organizations and still report
+   * success. Both reviewers flagged this independently.
+   */
+  it('rejects a required ID list containing an invalid entry rather than dropping it', async () => {
+    mockFetch([AUTH_OK])
+    await expect(
+      cbinsightsListFundingsTool.directExecution!({
+        ...CREDS,
+        orgIds: '129410, notanid, 1034157',
+      } as never)
+    ).rejects.toThrow(/must contain only positive integers \(invalid: notanid\)/)
+  })
+
+  it('rejects a mistyped optional filter rather than silently widening the search', async () => {
+    mockFetch([AUTH_OK])
+    await expect(
+      cbinsightsSearchFirmographicsTool.directExecution!({
+        ...CREDS,
+        keyword: 'fintech',
+        sectorIds: 'four',
+      } as never)
+    ).rejects.toThrow(/"sectorIds" must contain only positive integers/)
+  })
+
+  it('still treats an unset optional filter as absent', async () => {
+    mockFetch([AUTH_OK, { body: { orgs: [] } }])
+    await cbinsightsSearchFirmographicsTool.directExecution!({
+      ...CREDS,
+      keyword: 'fintech',
+      sectorIds: '',
+    } as never)
+    expect(JSON.parse(String(calls[1].init.body))).toEqual({ keyword: 'fintech' })
+  })
+
   it('rejects a non-integer organization ID rather than interpolating it into the path', async () => {
     mockFetch([AUTH_OK])
     await expect(

apps/sim/tools/cbinsights/utils.ts

@@ -42,6 +42,31 @@ export const LIMIT_MAX = 100
 const tokenCache = new Map<string, { token: string; expiresAt: number }>()
 const TOKEN_CACHE_TTL_MS = 5 * 60 * 1000
 
+/**
+ * Ceiling on distinct credential pairs held at once.
+ *
+ * The cache is process-wide, so on a long-lived worker serving many CB Insights
+ * accounts it would otherwise grow with the cumulative number of accounts seen.
+ */
+const TOKEN_CACHE_MAX_ENTRIES = 128
+
+/**
+ * Drops expired entries, then the oldest surviving ones if still over the cap.
+ *
+ * Map iteration is insertion-ordered, so the first surviving key is the least
+ * recently authorized — evicting it costs at most one extra token exchange.
+ */
+function pruneTokenCache(): void {
+  const now = Date.now()
+  for (const [key, entry] of tokenCache) {
+    if (entry.expiresAt <= now) tokenCache.delete(key)
+  }
+  for (const key of tokenCache.keys()) {
+    if (tokenCache.size <= TOKEN_CACHE_MAX_ENTRIES) break
+    tokenCache.delete(key)
+  }
+}
+
 async function credentialDigest(clientId: string, clientSecret: string): Promise<string> {
   const bytes = new TextEncoder().encode(`${clientId}:${clientSecret}`)
   const digest = await crypto.subtle.digest('SHA-256', bytes)
@@ -140,6 +165,8 @@ async function getToken(
 
   const token = await authorize(clientId, clientSecret, signal)
   tokenCache.set(key, { token, expiresAt: Date.now() + TOKEN_CACHE_TTL_MS })
+  /* Pruned after the insert so the cap bounds the cache including this entry. */
+  pruneTokenCache()
   return token
 }
 
@@ -148,6 +175,11 @@ export function resetCbInsightsTokenCache(): void {
   tokenCache.clear()
 }
 
+/** Current number of cached tokens. Exported for tests. */
+export function cbInsightsTokenCacheSize(): number {
+  return tokenCache.size
+}
+
 interface CbInsightsRequestSpec {
   /** Path below the API origin, e.g. `/v2/firmographics`. */
   path: string
@@ -253,9 +285,7 @@ export function requireOrgIds(value: unknown): number[] {
     raw = [value]
   }
 
-  const orgIds = raw
-    .map((entry) => (typeof entry === 'number' ? entry : Number(String(entry).trim())))
-    .filter((entry) => Number.isInteger(entry) && entry > 0)
+  const orgIds = toPositiveIntegers(raw, 'orgIds')
 
   if (orgIds.length === 0) {
     throw new Error('CB Insights "orgIds" must contain at least one positive integer')
@@ -266,6 +296,36 @@ export function requireOrgIds(value: unknown): number[] {
   return orgIds
 }
 
+/**
+ * Converts every entry to a positive integer, rejecting the whole list if any
+ * entry is not one.
+ *
+ * Dropping the bad entries instead would run the request against a silently
+ * narrower set — a typo in an ID list would spend credits on the wrong
+ * organizations, or quietly widen a filtered search, and still report success.
+ */
+function toPositiveIntegers(entries: readonly unknown[], paramName: string): number[] {
+  const invalid: string[] = []
+  const ids: number[] = []
+
+  for (const entry of entries) {
+    const label = typeof entry === 'number' ? String(entry) : String(entry).trim()
+    const parsed = typeof entry === 'number' ? entry : Number(label)
+    if (!Number.isInteger(parsed) || parsed <= 0) {
+      invalid.push(label)
+      continue
+    }
+    ids.push(parsed)
+  }
+
+  if (invalid.length > 0) {
+    throw new Error(
+      `CB Insights "${paramName}" must contain only positive integers (invalid: ${invalid.join(', ')})`
+    )
+  }
+  return ids
+}
+
 /** Coerces a numeric param and clamps it into the documented [1, 100] range. */
 export function clampLimit(value: unknown): number | undefined {
   if (value === undefined || value === null || value === '') return undefined
@@ -307,14 +367,18 @@ export function parseListParam(value: unknown, paramName: string): unknown[] | u
   return [value]
 }
 
-/** Parses a list of IDs, keeping only positive integers. */
+/**
+ * Parses an optional list of IDs, rejecting any entry that is not a positive
+ * integer.
+ *
+ * An unset filter is fine and returns undefined; a filter the caller *did* set
+ * but mistyped is not, because dropping it would silently widen the search
+ * rather than narrow it — and the wider search still spends credits.
+ */
 export function parseIdListParam(value: unknown, paramName: string): number[] | undefined {
   const entries = parseListParam(value, paramName)
   if (!entries) return undefined
-  const ids = entries
-    .map((entry) => (typeof entry === 'number' ? entry : Number(String(entry).trim())))
-    .filter((entry) => Number.isInteger(entry) && entry > 0)
-  return ids.length > 0 ? ids : undefined
+  return toPositiveIntegers(entries, paramName)
 }
 
 /** Parses a list of free-text values. */