From bb11907e8b04d519c4ea3e67d4c7ce5a6226914b Mon Sep 17 00:00:00 2001 From: Mohamed Bassem Date: Mon, 21 Jul 2025 00:33:33 +0000 Subject: fix: Remove bcrypt from the api key validation route --- apps/web/server/api/client.ts | 2 +- apps/web/server/auth.ts | 1 + packages/trpc/auth.ts | 55 ++-- packages/trpc/routers/apiKeys.test.ts | 515 ++++++++++++++++++++++++++++++++++ packages/trpc/routers/apiKeys.ts | 12 +- packages/trpc/routers/users.ts | 8 +- 6 files changed, 568 insertions(+), 25 deletions(-) create mode 100644 packages/trpc/routers/apiKeys.test.ts diff --git a/apps/web/server/api/client.ts b/apps/web/server/api/client.ts index b36459a2..69a8e10a 100644 --- a/apps/web/server/api/client.ts +++ b/apps/web/server/api/client.ts @@ -17,7 +17,7 @@ export async function createContextFromRequest(req: Request) { if (authorizationHeader && authorizationHeader.startsWith("Bearer ")) { const token = authorizationHeader.split(" ")[1]; try { - const user = await authenticateApiKey(token); + const user = await authenticateApiKey(token, db); return { user, db, diff --git a/apps/web/server/auth.ts b/apps/web/server/auth.ts index 3abc682f..1feeec28 100644 --- a/apps/web/server/auth.ts +++ b/apps/web/server/auth.ts @@ -108,6 +108,7 @@ const providers: Provider[] = [ return await validatePassword( credentials?.email, credentials?.password, + db, ); } catch { return null; diff --git a/packages/trpc/auth.ts b/packages/trpc/auth.ts index a01288d8..01966b9e 100644 --- a/packages/trpc/auth.ts +++ b/packages/trpc/auth.ts @@ -1,28 +1,33 @@ -import { randomBytes } from "crypto"; +import { createHash, randomBytes } from "crypto"; import * as bcrypt from "bcryptjs"; -import { db } from "@karakeep/db"; import { apiKeys } from "@karakeep/db/schema"; import serverConfig from "@karakeep/shared/config"; -// API Keys +import type { Context } from "./index"; const BCRYPT_SALT_ROUNDS = 10; -const API_KEY_PREFIX = "ak1"; +const API_KEY_PREFIX_V1 = "ak1"; +const API_KEY_PREFIX_V2 = "ak2"; export function generatePasswordSalt() { return randomBytes(32).toString("hex"); } -export async function generateApiKey(name: string, userId: string) { +export async function generateApiKey( + name: string, + userId: string, + database: Context["db"], +) { const id = randomBytes(10).toString("hex"); - const secret = randomBytes(10).toString("hex"); - const secretHash = await bcrypt.hash(secret, BCRYPT_SALT_ROUNDS); + const secret = randomBytes(16).toString("hex"); - const plain = `${API_KEY_PREFIX}_${id}_${secret}`; + const secretHash = createHash("sha256").update(secret).digest("base64"); + + const plain = `${API_KEY_PREFIX_V2}_${id}_${secret}`; const key = ( - await db + await database .insert(apiKeys) .values({ name: name, @@ -40,6 +45,7 @@ export async function generateApiKey(name: string, userId: string) { key: plain, }; } + function parseApiKey(plain: string) { const parts = plain.split("_"); if (parts.length != 3) { @@ -47,18 +53,19 @@ function parseApiKey(plain: string) { `Malformd API key. API keys should have 3 segments, found ${parts.length} instead.`, ); } - if (parts[0] !== API_KEY_PREFIX) { + if (parts[0] !== API_KEY_PREFIX_V1 && parts[0] !== API_KEY_PREFIX_V2) { throw new Error(`Malformd API key. Got unexpected key prefix.`); } return { + version: parts[0] == API_KEY_PREFIX_V1 ? (1 as const) : (2 as const), keyId: parts[1], keySecret: parts[2], }; } -export async function authenticateApiKey(key: string) { - const { keyId, keySecret } = parseApiKey(key); - const apiKey = await db.query.apiKeys.findFirst({ +export async function authenticateApiKey(key: string, database: Context["db"]) { + const { version, keyId, keySecret } = parseApiKey(key); + const apiKey = await database.query.apiKeys.findFirst({ where: (k, { eq }) => eq(k.keyId, keyId), with: { user: true, @@ -71,7 +78,19 @@ export async function authenticateApiKey(key: string) { const hash = apiKey.keyHash; - const validation = await bcrypt.compare(keySecret, hash); + let validation = false; + switch (version) { + case 1: + validation = await bcrypt.compare(keySecret, hash); + break; + case 2: + validation = + createHash("sha256").update(keySecret).digest("base64") == hash; + break; + default: + throw new Error("Invalid API Key"); + } + if (!validation) { throw new Error("Invalid API Key"); } @@ -83,11 +102,15 @@ export async function hashPassword(password: string, salt: string | null) { return await bcrypt.hash(password + (salt ?? ""), BCRYPT_SALT_ROUNDS); } -export async function validatePassword(email: string, password: string) { +export async function validatePassword( + email: string, + password: string, + database: Context["db"], +) { if (serverConfig.auth.disablePasswordAuth) { throw new Error("Password authentication is currently disabled"); } - const user = await db.query.users.findFirst({ + const user = await database.query.users.findFirst({ where: (u, { eq }) => eq(u.email, email), }); diff --git a/packages/trpc/routers/apiKeys.test.ts b/packages/trpc/routers/apiKeys.test.ts new file mode 100644 index 00000000..b3e57db3 --- /dev/null +++ b/packages/trpc/routers/apiKeys.test.ts @@ -0,0 +1,515 @@ +import { eq } from "drizzle-orm"; +import { beforeEach, describe, expect, test, vi } from "vitest"; + +import { apiKeys } from "@karakeep/db/schema"; + +import type { CustomTestContext } from "../testUtils"; +import { defaultBeforeEach, getApiCaller } from "../testUtils"; + +vi.mock("@karakeep/shared/config", async (original) => { + const mod = (await original()) as typeof import("@karakeep/shared/config"); + return { + ...mod, + default: { + ...mod.default, + auth: { + ...mod.default.auth, + disablePasswordAuth: false, + }, + }, + }; +}); + +beforeEach(defaultBeforeEach(false)); + +describe("API Keys Routes", () => { + describe("create", () => { + test("creates API key successfully", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const result = await api.create({ name: "Test Key" }); + + expect(result.name).toBe("Test Key"); + expect(result.id).toBeDefined(); + expect(result.key).toMatch(/^ak2_[a-f0-9]{20}_[a-f0-9]{32}$/); + expect(result.createdAt).toBeInstanceOf(Date); + }); + + test("requires authentication", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.create({ name: "Test Key" }), + ).rejects.toThrow(/UNAUTHORIZED/); + }); + }); + + describe("list", () => { + test("lists user's API keys", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + + await api.create({ name: "Key 1" }); + await api.create({ name: "Key 2" }); + + const result = await api.list(); + + expect(result.keys).toHaveLength(2); + expect(result.keys[0]).toMatchObject({ + id: expect.any(String), + name: expect.any(String), + createdAt: expect.any(Date), + keyId: expect.any(String), + }); + expect(result.keys[0]).not.toHaveProperty("key"); + }); + + test("returns empty list for new user", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const result = await api.list(); + + expect(result.keys).toHaveLength(0); + }); + + test("privacy isolation between users", async ({ + unauthedAPICaller, + db, + }) => { + const user1 = await unauthedAPICaller.users.create({ + name: "User 1", + email: "user1@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const user2 = await unauthedAPICaller.users.create({ + name: "User 2", + email: "user2@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api1 = getApiCaller(db, user1.id, user1.email).apiKeys; + const api2 = getApiCaller(db, user2.id, user2.email).apiKeys; + + await api1.create({ name: "User 1 Key" }); + await api2.create({ name: "User 2 Key" }); + + const result1 = await api1.list(); + const result2 = await api2.list(); + + expect(result1.keys).toHaveLength(1); + expect(result1.keys[0].name).toBe("User 1 Key"); + + expect(result2.keys).toHaveLength(1); + expect(result2.keys[0].name).toBe("User 2 Key"); + }); + + test("requires authentication", async ({ + unauthedAPICaller, + }) => { + await expect(() => unauthedAPICaller.apiKeys.list()).rejects.toThrow( + /UNAUTHORIZED/, + ); + }); + }); + + describe("revoke", () => { + test("revokes API key successfully", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + + const createdKey = await api.create({ name: "Test Key" }); + await api.revoke({ id: createdKey.id }); + + const remainingKeys = await db + .select() + .from(apiKeys) + .where(eq(apiKeys.id, createdKey.id)); + + expect(remainingKeys).toHaveLength(0); + }); + + test("cannot revoke another user's key", async ({ + unauthedAPICaller, + db, + }) => { + const user1 = await unauthedAPICaller.users.create({ + name: "User 1", + email: "user1@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const user2 = await unauthedAPICaller.users.create({ + name: "User 2", + email: "user2@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api1 = getApiCaller(db, user1.id, user1.email).apiKeys; + const api2 = getApiCaller(db, user2.id, user2.email).apiKeys; + + const user1Key = await api1.create({ name: "User 1 Key" }); + + await api2.revoke({ id: user1Key.id }); + + const remainingKeys = await db + .select() + .from(apiKeys) + .where(eq(apiKeys.id, user1Key.id)); + + expect(remainingKeys).toHaveLength(1); + }); + + test("silently handles non-existent key", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + + await expect( + api.revoke({ id: "non-existent-id" }), + ).resolves.toBeUndefined(); + }); + + test("requires authentication", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.revoke({ id: "some-id" }), + ).rejects.toThrow(/UNAUTHORIZED/); + }); + }); + + describe("validate", () => { + test("validates correct API key", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const createdKey = await api.create({ name: "Test Key" }); + + const result = await unauthedAPICaller.apiKeys.validate({ + apiKey: createdKey.key, + }); + + expect(result.success).toBe(true); + }); + + test("rejects invalid API key", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: "invalid-key", + }), + ).rejects.toThrow(); + }); + + test("rejects malformed API key", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: "ak2_invalid", + }), + ).rejects.toThrow(); + }); + + test("rejects non-existent key ID", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: "ak2_1234567890abcdef1234_1234567890abcdef1234", + }), + ).rejects.toThrow(); + }); + + test("rejects key with wrong secret", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const createdKey = await api.create({ name: "Test Key" }); + const keyParts = createdKey.key.split("_"); + const wrongKey = `${keyParts[0]}_${keyParts[1]}_wrongsecret123456`; + + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: wrongKey, + }), + ).rejects.toThrow(); + }); + + test("validates revoked key fails", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const createdKey = await api.create({ name: "Test Key" }); + await api.revoke({ id: createdKey.id }); + + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: createdKey.key, + }), + ).rejects.toThrow(); + }); + }); + + describe("exchange", () => { + test("exchanges credentials for API key", async ({ + db, + unauthedAPICaller, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "exchange@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const result = await unauthedAPICaller.apiKeys.exchange({ + keyName: "Extension Key", + email: "exchange@test.com", + password: "password123", + }); + + expect(result.name).toBe("Extension Key"); + expect(result.key).toMatch(/^ak2_[a-f0-9]{20}_[a-f0-9]{32}$/); + expect(result.createdAt).toBeInstanceOf(Date); + + const dbKeys = await db + .select() + .from(apiKeys) + .where(eq(apiKeys.userId, user.id)); + + expect(dbKeys).toHaveLength(1); + expect(dbKeys[0].name).toBe("Extension Key"); + }); + + test("rejects wrong password", async ({ + unauthedAPICaller, + }) => { + await unauthedAPICaller.users.create({ + name: "Test User", + email: "wrongpass@test.com", + password: "password123", + confirmPassword: "password123", + }); + + await expect(() => + unauthedAPICaller.apiKeys.exchange({ + keyName: "Extension Key", + email: "wrongpass@test.com", + password: "wrongpassword", + }), + ).rejects.toThrow(/UNAUTHORIZED/); + }); + + test("rejects non-existent user", async ({ + unauthedAPICaller, + }) => { + await expect(() => + unauthedAPICaller.apiKeys.exchange({ + keyName: "Extension Key", + email: "nonexistent@test.com", + password: "password123", + }), + ).rejects.toThrow(/UNAUTHORIZED/); + }); + }); + + describe("integration scenarios", () => { + test("full API key lifecycle", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "lifecycle@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + + const createdKey = await api.create({ name: "Lifecycle Test" }); + + const validationResult = await unauthedAPICaller.apiKeys.validate({ + apiKey: createdKey.key, + }); + expect(validationResult.success).toBe(true); + + const listResult = await api.list(); + expect(listResult.keys).toHaveLength(1); + expect(listResult.keys[0].name).toBe("Lifecycle Test"); + + await api.revoke({ id: createdKey.id }); + + await expect(() => + unauthedAPICaller.apiKeys.validate({ + apiKey: createdKey.key, + }), + ).rejects.toThrow(); + + const finalListResult = await api.list(); + expect(finalListResult.keys).toHaveLength(0); + }); + + test("multiple keys per user", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "multikey@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + + await api.create({ name: "Key 1" }); + const key2 = await api.create({ name: "Key 2" }); + await api.create({ name: "Key 3" }); + + const listResult = await api.list(); + expect(listResult.keys).toHaveLength(3); + + const keyNames = listResult.keys.map((k) => k.name).sort(); + expect(keyNames).toEqual(["Key 1", "Key 2", "Key 3"]); + + await api.revoke({ id: key2.id }); + + const updatedListResult = await api.list(); + expect(updatedListResult.keys).toHaveLength(2); + + const remainingNames = updatedListResult.keys.map((k) => k.name).sort(); + expect(remainingNames).toEqual(["Key 1", "Key 3"]); + }); + + test("exchange creates usable key", async ({ + unauthedAPICaller, + }) => { + await unauthedAPICaller.users.create({ + name: "Exchange User", + email: "exchangetest@test.com", + password: "password123", + confirmPassword: "password123", + }); + + const exchangedKey = await unauthedAPICaller.apiKeys.exchange({ + keyName: "Exchange Test Key", + email: "exchangetest@test.com", + password: "password123", + }); + + const validationResult = await unauthedAPICaller.apiKeys.validate({ + apiKey: exchangedKey.key, + }); + + expect(validationResult.success).toBe(true); + }); + }); + + describe("backward compatibility", () => { + test("validates version 1 keys continues to work", async ({ + unauthedAPICaller, + db, + }) => { + const user = await unauthedAPICaller.users.create({ + name: "Test User", + email: "test@test.com", + password: "password123", + confirmPassword: "password123", + }); + + // Manually generated v1 key and its corresponding hash + const key = "ak1_1316296acfe14b7961d5_aa88970c058be93e3c7a"; + await db + .insert(apiKeys) + .values({ + name: "Test Key", + userId: user.id, + keyId: "1316296acfe14b7961d5", + keyHash: + "$2a$10$pnJyG.0NPTHImX/nukeUteibD//ztBg4MTjWYRI9n3d54Z/TWvcNC", + }) + .returning(); + + const api = getApiCaller(db, user.id, user.email).apiKeys; + const result = await api.validate({ apiKey: key }); + + expect(result.success).toBe(true); + }); + }); +}); diff --git a/packages/trpc/routers/apiKeys.ts b/packages/trpc/routers/apiKeys.ts index a7a7ad09..b7d11d41 100644 --- a/packages/trpc/routers/apiKeys.ts +++ b/packages/trpc/routers/apiKeys.ts @@ -29,7 +29,7 @@ export const apiKeysAppRouter = router({ ) .output(zApiKeySchema) .mutation(async ({ input, ctx }) => { - return await generateApiKey(input.name, ctx.user.id); + return await generateApiKey(input.name, ctx.user.id, ctx.db); }), revoke: authedProcedure .input( @@ -85,7 +85,7 @@ export const apiKeysAppRouter = router({ }), ) .output(zApiKeySchema) - .mutation(async ({ input }) => { + .mutation(async ({ input, ctx }) => { let user; // Special handling as otherwise the extension would show "username or password is wrong" if (serverConfig.auth.disablePasswordAuth) { @@ -95,11 +95,11 @@ export const apiKeysAppRouter = router({ }); } try { - user = await validatePassword(input.email, input.password); + user = await validatePassword(input.email, input.password, ctx.db); } catch { throw new TRPCError({ code: "UNAUTHORIZED" }); } - return await generateApiKey(input.keyName, user.id); + return await generateApiKey(input.keyName, user.id, ctx.db); }), validate: publicProcedure .use( @@ -111,8 +111,8 @@ export const apiKeysAppRouter = router({ ) // 30 requests per minute .input(z.object({ apiKey: z.string() })) .output(z.object({ success: z.boolean() })) - .mutation(async ({ input }) => { - await authenticateApiKey(input.apiKey); // Throws if the key is invalid + .mutation(async ({ input, ctx }) => { + await authenticateApiKey(input.apiKey, ctx.db); // Throws if the key is invalid return { success: true, }; diff --git a/packages/trpc/routers/users.ts b/packages/trpc/routers/users.ts index 97784901..6aa12454 100644 --- a/packages/trpc/routers/users.ts +++ b/packages/trpc/routers/users.ts @@ -264,7 +264,11 @@ export const usersAppRouter = router({ invariant(ctx.user.email, "A user always has an email specified"); let user; try { - user = await validatePassword(ctx.user.email, input.currentPassword); + user = await validatePassword( + ctx.user.email, + input.currentPassword, + ctx.db, + ); } catch { throw new TRPCError({ code: "UNAUTHORIZED" }); } @@ -319,7 +323,7 @@ export const usersAppRouter = router({ } try { - await validatePassword(ctx.user.email, input.password); + await validatePassword(ctx.user.email, input.password, ctx.db); } catch { throw new TRPCError({ code: "UNAUTHORIZED", -- cgit v1.2.3-70-g09d2