refactor: use system secrets when possible (#248)

src/tests/secrets.test.ts

@@ -0,0 +1,191 @@
+// src/tests/keychain.test.ts
+// Tests for secrets utility functions
+
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import {
+  deleteApiKey,
+  deleteRefreshToken,
+  deleteSecureTokens,
+  getApiKey,
+  getRefreshToken,
+  getSecureTokens,
+  isKeychainAvailable,
+  keychainAvailablePrecompute,
+  type SecureTokens,
+  setApiKey,
+  setRefreshToken,
+  setSecureTokens,
+} from "../utils/secrets";
+
+describe("Secrets utilities", () => {
+  beforeEach(async () => {
+    if (keychainAvailablePrecompute) {
+      await deleteSecureTokens();
+    }
+  });
+
+  afterEach(async () => {
+    if (keychainAvailablePrecompute) {
+      await deleteSecureTokens();
+    }
+  });
+
+  test("isKeychainAvailable works", async () => {
+    const available = await isKeychainAvailable();
+    expect(typeof available).toBe("boolean");
+  });
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "can store and retrieve API key",
+    async () => {
+      const testApiKey = "sk-test-api-key-12345";
+
+      await setApiKey(testApiKey);
+      const retrievedApiKey = await getApiKey();
+
+      expect(retrievedApiKey).toBe(testApiKey);
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "can store and retrieve refresh token",
+    async () => {
+      const testRefreshToken = "rt-test-refresh-token-67890";
+
+      await setRefreshToken(testRefreshToken);
+      const retrievedRefreshToken = await getRefreshToken();
+
+      expect(retrievedRefreshToken).toBe(testRefreshToken);
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "can store and retrieve both tokens together",
+    async () => {
+      const tokens: SecureTokens = {
+        apiKey: "sk-test-api-key-combined",
+        refreshToken: "rt-test-refresh-token-combined",
+      };
+
+      await setSecureTokens(tokens);
+      const retrievedTokens = await getSecureTokens();
+
+      expect(retrievedTokens.apiKey).toBe(tokens.apiKey);
+      expect(retrievedTokens.refreshToken).toBe(tokens.refreshToken);
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)("can delete API key", async () => {
+    const testApiKey = "sk-test-api-key-delete";
+
+    await setApiKey(testApiKey);
+    let retrievedApiKey = await getApiKey();
+    expect(retrievedApiKey).toBe(testApiKey);
+
+    await deleteApiKey();
+    retrievedApiKey = await getApiKey();
+    expect(retrievedApiKey).toBe(null);
+  });
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "can delete refresh token",
+    async () => {
+      const testRefreshToken = "rt-test-refresh-token-delete";
+
+      await setRefreshToken(testRefreshToken);
+      let retrievedRefreshToken = await getRefreshToken();
+      expect(retrievedRefreshToken).toBe(testRefreshToken);
+
+      await deleteRefreshToken();
+      retrievedRefreshToken = await getRefreshToken();
+      expect(retrievedRefreshToken).toBe(null);
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "can delete all tokens",
+    async () => {
+      const tokens: SecureTokens = {
+        apiKey: "sk-test-api-key-delete-all",
+        refreshToken: "rt-test-refresh-token-delete-all",
+      };
+
+      await setSecureTokens(tokens);
+      let retrievedTokens = await getSecureTokens();
+      expect(retrievedTokens.apiKey).toBe(tokens.apiKey);
+      expect(retrievedTokens.refreshToken).toBe(tokens.refreshToken);
+
+      await deleteSecureTokens();
+      retrievedTokens = await getSecureTokens();
+      expect(retrievedTokens.apiKey).toBeUndefined();
+      expect(retrievedTokens.refreshToken).toBeUndefined();
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "returns null for non-existent tokens",
+    async () => {
+      // Ensure no tokens exist
+      await deleteSecureTokens();
+
+      const apiKey = await getApiKey();
+      const refreshToken = await getRefreshToken();
+      const tokens = await getSecureTokens();
+
+      expect(apiKey).toBe(null);
+      expect(refreshToken).toBe(null);
+      expect(tokens.apiKey).toBeUndefined();
+      expect(tokens.refreshToken).toBeUndefined();
+    },
+  );
+
+  test.skipIf(!keychainAvailablePrecompute)(
+    "handles partial token storage",
+    async () => {
+      // Store only API key
+      await setSecureTokens({ apiKey: "sk-only-api-key" });
+
+      let tokens = await getSecureTokens();
+      expect(tokens.apiKey).toBe("sk-only-api-key");
+      expect(tokens.refreshToken).toBeUndefined();
+
+      // Clean up and store only refresh token
+      await deleteSecureTokens();
+      await setSecureTokens({ refreshToken: "rt-only-refresh-token" });
+
+      tokens = await getSecureTokens();
+      expect(tokens.apiKey).toBeUndefined();
+      expect(tokens.refreshToken).toBe("rt-only-refresh-token");
+    },
+  );
+
+  test("gracefully handles secrets unavailability", async () => {
+    // This test should work even if secrets are not available
+    if (await isKeychainAvailable()) {
+      // If secrets are available, this is a basic functionality test
+      const tokens = await getSecureTokens();
+      expect(typeof tokens).toBe("object");
+    } else {
+      // If secrets are not available, functions should return null or throw appropriately
+      const tokens = await getSecureTokens();
+      expect(tokens.apiKey).toBeUndefined();
+      expect(tokens.refreshToken).toBeUndefined();
+
+      const apiKey = await getApiKey();
+      expect(apiKey).toBe(null);
+
+      const refreshToken = await getRefreshToken();
+      expect(refreshToken).toBe(null);
+
+      // Set operations should throw when secrets unavailable (handled by settings manager)
+      await expect(setSecureTokens({ apiKey: "test" })).rejects.toThrow();
+      await expect(setApiKey("test")).rejects.toThrow();
+      await expect(setRefreshToken("test")).rejects.toThrow();
+
+      // Delete operations should not throw (no-op when secrets unavailable)
+      await expect(deleteSecureTokens()).resolves.toBeUndefined();
+      await expect(deleteApiKey()).resolves.toBeUndefined();
+      await expect(deleteRefreshToken()).resolves.toBeUndefined();
+    }
+  });
+});

src/tests/settings-manager.test.ts

@@ -3,6 +3,11 @@ import { mkdtemp, rm } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { settingsManager } from "../settings-manager";
+import {
+  deleteSecureTokens,
+  isKeychainAvailable,
+  keychainAvailablePrecompute,
+} from "../utils/secrets.js";
 
 // Store original HOME to restore after tests
 const originalHome = process.env.HOME;
@@ -94,8 +99,24 @@ describe("Settings Manager - Initialization", () => {
 // ============================================================================
 
 describe("Settings Manager - Global Settings", () => {
+  let keychainSupported: boolean = false;
+
   beforeEach(async () => {
     await settingsManager.initialize();
+    // Check if secrets are available on this system
+    keychainSupported = await isKeychainAvailable();
+
+    if (keychainSupported) {
+      // Clean up any existing test tokens
+      await deleteSecureTokens();
+    }
+  });
+
+  afterEach(async () => {
+    if (keychainSupported) {
+      // Clean up after each test
+      await deleteSecureTokens();
+    }
   });
 
   test("Get settings returns a copy", () => {
@@ -162,12 +183,34 @@ describe("Settings Manager - Global Settings", () => {
     });
 
     const settings = settingsManager.getSettings();
+    // LETTA_API_KEY should not be in settings file (moved to keychain)
     expect(settings.env).toEqual({
-      LETTA_API_KEY: "sk-test-123",
       CUSTOM_VAR: "value",
     });
   });
 
+  test.skipIf(!keychainAvailablePrecompute)(
+    "Get settings with secure tokens (async method)",
+    async () => {
+      // This test verifies the async method that includes keychain tokens
+      settingsManager.updateSettings({
+        env: {
+          LETTA_API_KEY: "sk-test-async-123",
+          CUSTOM_VAR: "async-value",
+        },
+        refreshToken: "rt-test-refresh",
+        tokenExpiresAt: Date.now() + 3600000,
+      });
+
+      const settingsWithTokens =
+        await settingsManager.getSettingsWithSecureTokens();
+
+      // Should include the environment variables and other settings
+      expect(settingsWithTokens.env?.CUSTOM_VAR).toBe("async-value");
+      expect(typeof settingsWithTokens.tokenExpiresAt).toBe("number");
+    },
+  );
+
   test("LETTA_BASE_URL should not be cached in settings", () => {
     // This test verifies that LETTA_BASE_URL is NOT persisted to settings
     // It should only come from environment variables