Merge pull request #475 from drifterza/fix/issue-474-api-key-bad-request

app/routes/auth/login/action.ts

@@ -1,4 +1,4 @@
-import { data, redirect } from "react-router";
+import { redirect } from "react-router";
 
 import { isDataWithApiError } from "~/server/headscale/api/error-client";
 import log from "~/utils/log";
@@ -15,7 +15,10 @@ export async function loginAction({ request, context }: Route.LoaderArgs) {
       "auth",
       "If this is unexpected, ensure your reverse proxy (if applicable) is configured correctly",
     );
-    throw data("Missing `api_key`", { status: 400 });
+    return {
+      success: false,
+      message: "Missing API key. Please enter your API key.",
+    };
   }
 
   if (apiKey.length === 0) {
@@ -24,13 +27,15 @@ export async function loginAction({ request, context }: Route.LoaderArgs) {
       "auth",
       "If this is unexpected, ensure your reverse proxy (if applicable) is configured correctly",
     );
-    throw data("Received an empty `api_key`", { status: 400 });
+    return {
+      success: false,
+      message: "API key cannot be empty. Please enter a valid API key.",
+    };
   }
 
   const api = context.hsApi.getRuntimeClient(apiKey);
   try {
     const apiKeys = await api.getApiKeys();
-    console.log(apiKeys);
 
     // We don't need to check for 0 API keys because this request cannot
     // be authenticated correctly without an API key
@@ -47,7 +52,10 @@ export async function loginAction({ request, context }: Route.LoaderArgs) {
 
     if (lookup.expiration === null || lookup.expiration === undefined) {
       log.error("auth", "Got an API key without an expiration");
-      throw data("API key is malformed", { status: 500 });
+      return {
+        success: false,
+        message: "API key is malformed (missing expiration). Please generate a new API key.",
+      };
     }
 
     const expiry = new Date(lookup.expiration);

tests/unit/auth/login-action.test.ts

@@ -0,0 +1,192 @@
+import { describe, expect, test, vi } from "vitest";
+
+// Helper to create a mock FormData with optional api_key
+function mockFormData(apiKey?: string): FormData {
+  const formData = new FormData();
+  if (apiKey !== undefined) {
+    formData.set("api_key", apiKey);
+  }
+  return formData;
+}
+
+// Helper to create mock request
+function mockRequest(formData: FormData): Request {
+  return {
+    formData: () => Promise.resolve(formData),
+  } as unknown as Request;
+}
+
+// Types for test clarity
+interface LoginResult {
+  success: boolean;
+  message: string;
+}
+
+interface MockApiKey {
+  prefix: string;
+  expiration: string | null;
+}
+
+// Mock the log module to avoid console spam during tests
+vi.mock("~/utils/log", () => ({
+  default: {
+    warn: vi.fn(),
+    error: vi.fn(),
+    debug: vi.fn(),
+  },
+}));
+
+describe("Login action validation", () => {
+  test("returns error when api_key field is missing", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const formData = mockFormData(); // no api_key
+    const request = mockRequest(formData);
+
+    const mockContext = {
+      hsApi: { getRuntimeClient: vi.fn() },
+      sessions: { createSession: vi.fn() },
+    };
+
+    const result = (await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any)) as LoginResult;
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain("Missing");
+  });
+
+  test("returns error when api_key is empty string", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const formData = mockFormData("");
+    const request = mockRequest(formData);
+
+    const mockContext = {
+      hsApi: { getRuntimeClient: vi.fn() },
+      sessions: { createSession: vi.fn() },
+    };
+
+    const result = (await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any)) as LoginResult;
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain("empty");
+  });
+
+  test("returns error when api key not found in database", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const formData = mockFormData("some-invalid-key-12345");
+    const request = mockRequest(formData);
+
+    const mockGetApiKeys = vi
+      .fn()
+      .mockResolvedValue([{ prefix: "other-prefix", expiration: "2030-01-01T00:00:00Z" }]);
+
+    const mockContext = {
+      hsApi: {
+        getRuntimeClient: () => ({ getApiKeys: mockGetApiKeys }),
+      },
+      sessions: { createSession: vi.fn() },
+    };
+
+    const result = (await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any)) as LoginResult;
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain("not found");
+  });
+
+  test("returns error when api key has expired", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const expiredKey = "expired-key-prefix.secret";
+    const formData = mockFormData(expiredKey);
+    const request = mockRequest(formData);
+
+    const mockGetApiKeys = vi
+      .fn()
+      .mockResolvedValue([{ prefix: "expired-key-prefix", expiration: "2020-01-01T00:00:00Z" }]);
+
+    const mockContext = {
+      hsApi: {
+        getRuntimeClient: () => ({ getApiKeys: mockGetApiKeys }),
+      },
+      sessions: { createSession: vi.fn() },
+    };
+
+    const result = (await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any)) as LoginResult;
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain("expired");
+  });
+
+  test("returns error when api key has no expiration field", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const keyWithoutExpiry = "malformed-key.secret";
+    const formData = mockFormData(keyWithoutExpiry);
+    const request = mockRequest(formData);
+
+    const mockGetApiKeys = vi
+      .fn()
+      .mockResolvedValue([{ prefix: "malformed-key", expiration: null } as MockApiKey]);
+
+    const mockContext = {
+      hsApi: {
+        getRuntimeClient: () => ({ getApiKeys: mockGetApiKeys }),
+      },
+      sessions: { createSession: vi.fn() },
+    };
+
+    const result = (await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any)) as LoginResult;
+
+    expect(result.success).toBe(false);
+    expect(result.message).toContain("malformed");
+  });
+
+  test("handles asterisks in api key prefix from headscale 0.28+", async () => {
+    const { loginAction } = await import("~/routes/auth/login/action");
+    const apiKey = "my-key-prefix.the-secret-part";
+    const formData = mockFormData(apiKey);
+    const request = mockRequest(formData);
+
+    const futureDate = new Date();
+    futureDate.setFullYear(futureDate.getFullYear() + 1);
+
+    const mockGetApiKeys = vi
+      .fn()
+      .mockResolvedValue([{ prefix: "my-***-prefix", expiration: futureDate.toISOString() }]);
+
+    const mockCreateSession = vi.fn().mockResolvedValue("session-cookie");
+
+    const mockContext = {
+      hsApi: {
+        getRuntimeClient: () => ({ getApiKeys: mockGetApiKeys }),
+      },
+      sessions: { createSession: mockCreateSession },
+    };
+
+    const result = await loginAction({
+      request,
+      context: mockContext,
+      params: {},
+    } as any);
+
+    // Should match despite asterisks in stored prefix
+    // Result will be a redirect (Response) on success, not our error object
+    expect(result).toBeDefined();
+  });
+});