fix: Remove bcrypt from the api key validation route

4 files changed, 566 insertions, 24 deletions

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<CustomTestContext>(defaultBeforeEach(false));
+
+describe("API Keys Routes", () => {
+  describe("create", () => {
+    test<CustomTestContext>("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<CustomTestContext>("requires authentication", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() =>
+        unauthedAPICaller.apiKeys.create({ name: "Test Key" }),
+      ).rejects.toThrow(/UNAUTHORIZED/);
+    });
+  });
+
+  describe("list", () => {
+    test<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("requires authentication", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() => unauthedAPICaller.apiKeys.list()).rejects.toThrow(
+        /UNAUTHORIZED/,
+      );
+    });
+  });
+
+  describe("revoke", () => {
+    test<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("requires authentication", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() =>
+        unauthedAPICaller.apiKeys.revoke({ id: "some-id" }),
+      ).rejects.toThrow(/UNAUTHORIZED/);
+    });
+  });
+
+  describe("validate", () => {
+    test<CustomTestContext>("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<CustomTestContext>("rejects invalid API key", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() =>
+        unauthedAPICaller.apiKeys.validate({
+          apiKey: "invalid-key",
+        }),
+      ).rejects.toThrow();
+    });
+
+    test<CustomTestContext>("rejects malformed API key", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() =>
+        unauthedAPICaller.apiKeys.validate({
+          apiKey: "ak2_invalid",
+        }),
+      ).rejects.toThrow();
+    });
+
+    test<CustomTestContext>("rejects non-existent key ID", async ({
+      unauthedAPICaller,
+    }) => {
+      await expect(() =>
+        unauthedAPICaller.apiKeys.validate({
+          apiKey: "ak2_1234567890abcdef1234_1234567890abcdef1234",
+        }),
+      ).rejects.toThrow();
+    });
+
+    test<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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<CustomTestContext>("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",