From e4ed38a117022dbd97df6c7cf0745e3caf4c0a17 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Wed, 3 Jun 2026 18:37:41 -0700 Subject: [PATCH] chore(api-key): remove legacy scan+decrypt auth fallback --- apps/sim/lib/api-key/auth.test.ts | 167 +---------------------- apps/sim/lib/api-key/auth.ts | 56 -------- apps/sim/lib/api-key/service.test.ts | 74 ++--------- apps/sim/lib/api-key/service.ts | 191 +++++---------------------- 4 files changed, 44 insertions(+), 444 deletions(-) diff --git a/apps/sim/lib/api-key/auth.test.ts b/apps/sim/lib/api-key/auth.test.ts index ac4bb4089f0..6a67b144f37 100644 --- a/apps/sim/lib/api-key/auth.test.ts +++ b/apps/sim/lib/api-key/auth.test.ts @@ -10,12 +10,7 @@ */ import { randomBytes } from 'crypto' -import { - createEncryptedApiKey, - createLegacyApiKey, - expectApiKeyInvalid, - expectApiKeyValid, -} from '@sim/testing' +import { createEncryptedApiKey, createLegacyApiKey } from '@sim/testing' import { describe, expect, it, vi } from 'vitest' const cryptoMock = vi.hoisted(() => ({ @@ -40,7 +35,6 @@ const cryptoMock = vi.hoisted(() => ({ vi.mock('@/lib/api-key/crypto', () => cryptoMock) import { - authenticateApiKey, formatApiKeyForDisplay, getApiKeyLast4, isEncryptedKey, @@ -113,110 +107,6 @@ describe('isLegacyApiKeyFormat', () => { }) }) -describe('authenticateApiKey', () => { - describe('encrypted format key (sk-sim-) against encrypted storage', () => { - it('should authenticate matching encrypted key', async () => { - const plainKey = 'sk-sim-test-key-123' - const encryptedStorage = `mock-iv:${Buffer.from(plainKey).toString('hex')}:mock-tag` - - const result = await authenticateApiKey(plainKey, encryptedStorage) - expectApiKeyValid(result) - }) - - it('should reject non-matching encrypted key', async () => { - const inputKey = 'sk-sim-test-key-123' - const differentKey = 'sk-sim-different-key' - const encryptedStorage = `mock-iv:${Buffer.from(differentKey).toString('hex')}:mock-tag` - - const result = await authenticateApiKey(inputKey, encryptedStorage) - expectApiKeyInvalid(result) - }) - - it('should reject encrypted format key against plain text storage', async () => { - const inputKey = 'sk-sim-test-key-123' - const plainStorage = inputKey // Same key but stored as plain text - - const result = await authenticateApiKey(inputKey, plainStorage) - expectApiKeyInvalid(result) - }) - }) - - describe('legacy format key (sim_) against storage', () => { - it('should authenticate legacy key against encrypted storage', async () => { - const plainKey = 'sim_legacy-test-key' - const encryptedStorage = `mock-iv:${Buffer.from(plainKey).toString('hex')}:mock-tag` - - const result = await authenticateApiKey(plainKey, encryptedStorage) - expectApiKeyValid(result) - }) - - it('should authenticate legacy key against plain text storage', async () => { - const plainKey = 'sim_legacy-test-key' - const plainStorage = plainKey - - const result = await authenticateApiKey(plainKey, plainStorage) - expectApiKeyValid(result) - }) - - it('should reject non-matching legacy key', async () => { - const inputKey = 'sim_test-key' - const storedKey = 'sim_different-key' - - const result = await authenticateApiKey(inputKey, storedKey) - expectApiKeyInvalid(result) - }) - }) - - describe('unrecognized format keys', () => { - it('should authenticate unrecognized key against plain text match', async () => { - const plainKey = 'custom-api-key-format' - const plainStorage = plainKey - - const result = await authenticateApiKey(plainKey, plainStorage) - expectApiKeyValid(result) - }) - - it('should authenticate unrecognized key against encrypted storage', async () => { - const plainKey = 'custom-api-key-format' - const encryptedStorage = `mock-iv:${Buffer.from(plainKey).toString('hex')}:mock-tag` - - const result = await authenticateApiKey(plainKey, encryptedStorage) - expectApiKeyValid(result) - }) - - it('should reject non-matching unrecognized key', async () => { - const inputKey = 'custom-key-1' - const storedKey = 'custom-key-2' - - const result = await authenticateApiKey(inputKey, storedKey) - expectApiKeyInvalid(result) - }) - }) - - describe('edge cases', () => { - it('should reject empty input key', async () => { - const result = await authenticateApiKey('', 'sim_stored-key') - expectApiKeyInvalid(result) - }) - - it('should reject empty stored key', async () => { - const result = await authenticateApiKey('sim_input-key', '') - expectApiKeyInvalid(result) - }) - - it('should handle keys with special characters', async () => { - const specialKey = 'sim_key-with-special+chars/and=more' - const result = await authenticateApiKey(specialKey, specialKey) - expectApiKeyValid(result) - }) - - it('should be case-sensitive', async () => { - const result = await authenticateApiKey('sim_TestKey', 'sim_testkey') - expectApiKeyInvalid(result) - }) - }) -}) - describe('isValidApiKeyFormat', () => { it('should accept valid length keys', () => { expect(isValidApiKeyFormat(`sim_${'a'.repeat(20)}`)).toBe(true) @@ -330,58 +220,3 @@ describe('generateEncryptedApiKey', () => { expect(key.length).toBeLessThan(100) }) }) - -describe('API key lifecycle', () => { - it('should authenticate newly generated legacy key against itself (plain storage)', async () => { - const key = generateApiKey() - const result = await authenticateApiKey(key, key) - expectApiKeyValid(result) - }) - - it('should authenticate newly generated encrypted key against encrypted storage', async () => { - const key = generateEncryptedApiKey() - const encryptedStorage = `mock-iv:${Buffer.from(key).toString('hex')}:mock-tag` - const result = await authenticateApiKey(key, encryptedStorage) - expectApiKeyValid(result) - }) - - it('should reject key if storage is tampered', async () => { - const key = generateApiKey() - const lastChar = key.slice(-1) - // Ensure tampered character is different from original (handles edge case where key ends in 'X') - const tamperedChar = lastChar === 'X' ? 'Y' : 'X' - const tamperedStorage = `${key.slice(0, -1)}${tamperedChar}` - const result = await authenticateApiKey(key, tamperedStorage) - expectApiKeyInvalid(result) - }) -}) - -describe('security considerations', () => { - it('should not accept partial key matches', async () => { - const fullKey = 'sim_abcdefghijklmnop' - const partialKey = 'sim_abcdefgh' - const result = await authenticateApiKey(partialKey, fullKey) - expectApiKeyInvalid(result) - }) - - it('should not accept keys with extra characters', async () => { - const storedKey = 'sim_abcdefgh' - const extendedKey = 'sim_abcdefghXXX' - const result = await authenticateApiKey(extendedKey, storedKey) - expectApiKeyInvalid(result) - }) - - it('should not accept key with whitespace variations', async () => { - const key = 'sim_testkey' - const keyWithSpace = ' sim_testkey' - const result = await authenticateApiKey(keyWithSpace, key) - expectApiKeyInvalid(result) - }) - - it('should not accept key with trailing whitespace', async () => { - const key = 'sim_testkey' - const keyWithTrailing = 'sim_testkey ' - const result = await authenticateApiKey(keyWithTrailing, key) - expectApiKeyInvalid(result) - }) -}) diff --git a/apps/sim/lib/api-key/auth.ts b/apps/sim/lib/api-key/auth.ts index 76c2cad229e..5a8e44ddcea 100644 --- a/apps/sim/lib/api-key/auth.ts +++ b/apps/sim/lib/api-key/auth.ts @@ -1,7 +1,6 @@ import { db } from '@sim/db' import { apiKey } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { safeCompare } from '@sim/security/compare' import { generateShortId } from '@sim/utils/id' import { and, eq } from 'drizzle-orm' import { @@ -32,61 +31,6 @@ export function isEncryptedKey(storedKey: string): boolean { return storedKey.includes(':') && storedKey.split(':').length === 3 } -/** - * Authenticates an API key against a stored key, supporting both legacy and new encrypted formats - * @param inputKey - The API key provided by the client - * @param storedKey - The key stored in the database (may be plain text or encrypted) - * @returns Promise - true if the key is valid - */ -export async function authenticateApiKey(inputKey: string, storedKey: string): Promise { - try { - // If input key has new encrypted prefix (sk-sim-), only check against encrypted storage - if (isEncryptedApiKeyFormat(inputKey)) { - if (isEncryptedKey(storedKey)) { - try { - const { decrypted } = await decryptApiKey(storedKey) - return safeCompare(inputKey, decrypted) - } catch (decryptError) { - logger.error('Failed to decrypt stored API key:', { error: decryptError }) - return false - } - } - // New format keys should never match against plain text storage - return false - } - - // If input key has legacy prefix (sim_), check both encrypted and plain text - if (isLegacyApiKeyFormat(inputKey)) { - if (isEncryptedKey(storedKey)) { - try { - const { decrypted } = await decryptApiKey(storedKey) - return safeCompare(inputKey, decrypted) - } catch (decryptError) { - logger.error('Failed to decrypt stored API key:', { error: decryptError }) - // Fall through to plain text comparison if decryption fails - } - } - // Legacy format can match against plain text storage - return safeCompare(inputKey, storedKey) - } - - // If no recognized prefix, fall back to original behavior - if (isEncryptedKey(storedKey)) { - try { - const { decrypted } = await decryptApiKey(storedKey) - return safeCompare(inputKey, decrypted) - } catch (decryptError) { - logger.error('Failed to decrypt stored API key:', { error: decryptError }) - } - } - - return safeCompare(inputKey, storedKey) - } catch (error) { - logger.error('API key authentication error:', { error }) - return false - } -} - /** * Encrypts an API key for secure storage * @param apiKey - The plain text API key to encrypt diff --git a/apps/sim/lib/api-key/service.test.ts b/apps/sim/lib/api-key/service.test.ts index c38b4a7d550..0fab5400d1e 100644 --- a/apps/sim/lib/api-key/service.test.ts +++ b/apps/sim/lib/api-key/service.test.ts @@ -1,10 +1,9 @@ /** * Tests for authenticateApiKeyFromHeader. * - * The path was rewritten to look up rows by the SHA-256 hash of the incoming - * API key. A fallback loop — full scan + decrypt — is preserved while the - * `key_hash` backfill runs, and emits a warn log whenever it actually matches - * a row so we can tell when it's safe to delete. + * Authentication looks up a single row by the SHA-256 hash of the incoming + * API key and applies the scope / expiry / permission gates. Any miss — no + * matching hash or a failed gate — returns an invalid result. * * @vitest-environment node */ @@ -36,14 +35,6 @@ vi.mock('@sim/logger', () => ({ getRequestContext: vi.fn(() => undefined), })) -const { mockAuthenticateApiKey } = vi.hoisted(() => ({ - mockAuthenticateApiKey: vi.fn(), -})) - -vi.mock('@/lib/api-key/auth', () => ({ - authenticateApiKey: mockAuthenticateApiKey, -})) - const { mockGetWorkspaceBillingSettings } = vi.hoisted(() => ({ mockGetWorkspaceBillingSettings: vi.fn(), })) @@ -63,15 +54,12 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({ import { hashApiKey } from '@/lib/api-key/crypto' import { authenticateApiKeyFromHeader } from '@/lib/api-key/service' -const warnSpy = serviceLogger.warn - function personalKeyRecord(overrides: Partial> = {}) { return { id: 'key-1', userId: 'user-1', workspaceId: null as string | null, type: 'personal', - key: 'encrypted:stored:value', expiresAt: null as Date | null, ...overrides, } @@ -80,7 +68,6 @@ function personalKeyRecord(overrides: Partial> = {}) { describe('authenticateApiKeyFromHeader', () => { beforeEach(() => { vi.clearAllMocks() - mockAuthenticateApiKey.mockReset() mockGetWorkspaceBillingSettings.mockReset() mockGetUserEntityPermissions.mockReset() }) @@ -91,7 +78,7 @@ describe('authenticateApiKeyFromHeader', () => { expect(dbChainMockFns.where).not.toHaveBeenCalled() }) - it('resolves on the fast path when the hash lookup finds a row', async () => { + it('resolves when the hash lookup finds a row', async () => { const record = personalKeyRecord() dbChainMockFns.where.mockResolvedValueOnce([record]) @@ -107,8 +94,6 @@ describe('authenticateApiKeyFromHeader', () => { workspaceId: undefined, }) expect(dbChainMockFns.where).toHaveBeenCalledTimes(1) - expect(mockAuthenticateApiKey).not.toHaveBeenCalled() - expect(warnSpy).not.toHaveBeenCalled() }) it('returns invalid when the hash lookup finds a row that fails scope checks', async () => { @@ -121,63 +106,20 @@ describe('authenticateApiKeyFromHeader', () => { expect(result).toEqual({ success: false, error: 'Invalid API key' }) expect(dbChainMockFns.where).toHaveBeenCalledTimes(1) - expect(mockAuthenticateApiKey).not.toHaveBeenCalled() - }) - - it('falls back to the decrypt loop when no row matches the hash, and warns on success', async () => { - const record = personalKeyRecord() - dbChainMockFns.where.mockResolvedValueOnce([]).mockResolvedValueOnce([record]) - mockAuthenticateApiKey.mockResolvedValueOnce(true) - - const result = await authenticateApiKeyFromHeader('sk-sim-plain-key', { - userId: 'user-1', - }) - - expect(result).toEqual({ - success: true, - userId: 'user-1', - keyId: 'key-1', - keyType: 'personal', - workspaceId: undefined, - }) - expect(dbChainMockFns.where).toHaveBeenCalledTimes(2) - expect(mockAuthenticateApiKey).toHaveBeenCalledWith( - 'sk-sim-plain-key', - 'encrypted:stored:value' - ) - expect(warnSpy).toHaveBeenCalledWith('API key matched via fallback decrypt loop', { - keyId: 'key-1', - }) - }) - - it('returns invalid when the hash lookup misses and the fallback scan also misses', async () => { - dbChainMockFns.where.mockResolvedValueOnce([]).mockResolvedValueOnce([]) - - const result = await authenticateApiKeyFromHeader('sk-sim-plain-key', { - userId: 'user-1', - }) - - expect(result).toEqual({ success: false, error: 'Invalid API key' }) - expect(dbChainMockFns.where).toHaveBeenCalledTimes(2) - expect(mockAuthenticateApiKey).not.toHaveBeenCalled() - expect(warnSpy).not.toHaveBeenCalled() }) - it('returns invalid when the hash lookup misses and every fallback candidate fails decrypt comparison', async () => { - const record = personalKeyRecord() - dbChainMockFns.where.mockResolvedValueOnce([]).mockResolvedValueOnce([record]) - mockAuthenticateApiKey.mockResolvedValueOnce(false) + it('returns invalid when the hash lookup finds no row', async () => { + dbChainMockFns.where.mockResolvedValueOnce([]) const result = await authenticateApiKeyFromHeader('sk-sim-plain-key', { userId: 'user-1', }) expect(result).toEqual({ success: false, error: 'Invalid API key' }) - expect(mockAuthenticateApiKey).toHaveBeenCalledTimes(1) - expect(warnSpy).not.toHaveBeenCalled() + expect(dbChainMockFns.where).toHaveBeenCalledTimes(1) }) - it('queries by the sha256 hash of the incoming header on the fast path', async () => { + it('queries by the sha256 hash of the incoming header', async () => { dbChainMockFns.where.mockResolvedValueOnce([personalKeyRecord()]) await authenticateApiKeyFromHeader('sk-sim-plain-key', { userId: 'user-1' }) diff --git a/apps/sim/lib/api-key/service.ts b/apps/sim/lib/api-key/service.ts index 4c0709b9df6..b00166805d1 100644 --- a/apps/sim/lib/api-key/service.ts +++ b/apps/sim/lib/api-key/service.ts @@ -2,7 +2,6 @@ import { db } from '@sim/db' import { apiKey as apiKeyTable } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { and, eq } from 'drizzle-orm' -import { authenticateApiKey } from '@/lib/api-key/auth' import { hashApiKey } from '@/lib/api-key/crypto' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { getWorkspaceBillingSettings, type WorkspaceBillingSettings } from '@/lib/workspaces/utils' @@ -53,11 +52,9 @@ interface HashCandidate { /** * Authenticate an API key from header with flexible filtering options. * - * Tries the hash lookup first. If that misses (legacy row not yet backfilled, - * or writer missed the hash column), falls back to the original scan+decrypt - * loop. The fallback emits a warn log whenever it actually matches a row so - * we can confirm the fast path is covering 100% of traffic before deleting - * the fallback block below in a follow-up PR. + * Looks up a single row by `sha256(apiKeyHeader)` and applies the scope / + * expiry / permission gates. Any miss — no matching hash or a failed gate — + * returns `INVALID`. */ export async function authenticateApiKeyFromHeader( apiKeyHeader: string, @@ -77,180 +74,62 @@ export async function authenticateApiKeyFromHeader( } } - const hashResult = await authenticateApiKeyByHash(apiKeyHeader, options, workspaceSettings) - if (hashResult !== null) return hashResult - - // LEGACY FALLBACK — delete once `logger.warn('API key matched via fallback - // decrypt loop', ...)` count stays at zero in prod. The block below is the - // pre-hash-lookup implementation, preserved verbatim as a safety net while - // the `key_hash` backfill rolls out. - let query = db + const keyHash = hashApiKey(apiKeyHeader) + const rows: HashCandidate[] = await db .select({ id: apiKeyTable.id, userId: apiKeyTable.userId, workspaceId: apiKeyTable.workspaceId, type: apiKeyTable.type, - key: apiKeyTable.key, expiresAt: apiKeyTable.expiresAt, }) .from(apiKeyTable) + .where(eq(apiKeyTable.keyHash, keyHash)) - const conditions = [] + if (rows.length === 0) return INVALID - if (options.userId) { - conditions.push(eq(apiKeyTable.userId, options.userId)) - } + const record = rows[0] + const keyType = record.type as 'personal' | 'workspace' - if (options.keyTypes?.length) { - if (options.keyTypes.length === 1) { - conditions.push(eq(apiKeyTable.type, options.keyTypes[0])) - } - } + if (options.userId && record.userId !== options.userId) return INVALID + if (options.keyTypes?.length && !options.keyTypes.includes(keyType)) return INVALID + if (record.expiresAt && record.expiresAt < new Date()) return INVALID - if (conditions.length > 0) { - query = query.where(and(...conditions)) as any + if ( + options.workspaceId && + keyType === 'workspace' && + record.workspaceId !== options.workspaceId + ) { + return INVALID } - const keyRecords = await query - - const filteredRecords = keyRecords.filter((record) => { - const keyType = record.type as 'personal' | 'workspace' - - if (options.keyTypes?.length && !options.keyTypes.includes(keyType)) { - return false - } - - if (options.workspaceId) { - if (keyType === 'workspace') { - return record.workspaceId === options.workspaceId - } - - if (keyType === 'personal') { - return workspaceSettings?.allowPersonalApiKeys ?? false - } - } - - return true - }) - - const permissionCache = new Map() - - for (const storedKey of filteredRecords) { - if (storedKey.expiresAt && storedKey.expiresAt < new Date()) { - continue - } - - if (options.workspaceId && (storedKey.type as 'personal' | 'workspace') === 'personal') { - if (!workspaceSettings?.allowPersonalApiKeys) { - continue - } - - if (!storedKey.userId) { - continue - } + if (options.workspaceId && keyType === 'personal') { + if (!workspaceSettings?.allowPersonalApiKeys) return INVALID + if (!record.userId) return INVALID - if (!permissionCache.has(storedKey.userId)) { - const permission = await getUserEntityPermissions( - storedKey.userId, - 'workspace', - options.workspaceId - ) - permissionCache.set(storedKey.userId, permission !== null) - } + const permission = await getUserEntityPermissions( + record.userId, + 'workspace', + options.workspaceId + ) + if (permission === null) return INVALID + } - if (!permissionCache.get(storedKey.userId)) { - continue - } - } + logger.debug('API key matched via hash lookup', { keyId: record.id, keyType }) - try { - const isValid = await authenticateApiKey(apiKeyHeader, storedKey.key) - if (isValid) { - logger.warn('API key matched via fallback decrypt loop', { keyId: storedKey.id }) - return { - success: true, - userId: storedKey.userId, - keyId: storedKey.id, - keyType: storedKey.type as 'personal' | 'workspace', - workspaceId: storedKey.workspaceId || options.workspaceId || undefined, - } - } - } catch (error) { - logger.error('Error authenticating API key:', error) - } + return { + success: true, + userId: record.userId, + keyId: record.id, + keyType, + workspaceId: record.workspaceId || options.workspaceId || undefined, } - - return INVALID } catch (error) { logger.error('API key authentication error:', error) return { success: false, error: 'Authentication failed' } } } -/** - * Fast path: look up a single row by `sha256(apiKeyHeader)` and apply the - * scope / expiry / permission gates. Returns `null` when no row matched the - * hash (caller should fall through to the legacy scan+decrypt loop). A hash - * hit that fails a gate returns a concrete `INVALID` — the key definitely - * belongs to that row, it's just not authorized in this scope. - */ -async function authenticateApiKeyByHash( - apiKeyHeader: string, - options: ApiKeyAuthOptions, - workspaceSettings: WorkspaceBillingSettings | null -): Promise { - const keyHash = hashApiKey(apiKeyHeader) - const rows: HashCandidate[] = await db - .select({ - id: apiKeyTable.id, - userId: apiKeyTable.userId, - workspaceId: apiKeyTable.workspaceId, - type: apiKeyTable.type, - expiresAt: apiKeyTable.expiresAt, - }) - .from(apiKeyTable) - .where(eq(apiKeyTable.keyHash, keyHash)) - - if (rows.length === 0) return null - - const record = rows[0] - const keyType = record.type as 'personal' | 'workspace' - - if (options.userId && record.userId !== options.userId) return INVALID - if (options.keyTypes?.length && !options.keyTypes.includes(keyType)) return INVALID - if (record.expiresAt && record.expiresAt < new Date()) return INVALID - - if ( - options.workspaceId && - keyType === 'workspace' && - record.workspaceId !== options.workspaceId - ) { - return INVALID - } - - if (options.workspaceId && keyType === 'personal') { - if (!workspaceSettings?.allowPersonalApiKeys) return INVALID - if (!record.userId) return INVALID - - const permission = await getUserEntityPermissions( - record.userId, - 'workspace', - options.workspaceId - ) - if (permission === null) return INVALID - } - - logger.debug('API key matched via hash lookup', { keyId: record.id, keyType }) - - return { - success: true, - userId: record.userId, - keyId: record.id, - keyType, - workspaceId: record.workspaceId || options.workspaceId || undefined, - } -} - /** * Update the last used timestamp for an API key */