fix: return 401 for expired or invalid tokens in authMiddleware

.github/workflows/ci.yml

@@ -63,6 +63,10 @@ jobs:
       - name: Type-check
         run: pnpm run check-types
 
+      # The auth middleware tests sign real tokens and need no services.
+      - name: Test http-server
+        run: pnpm --filter http-server run test
+
       # The ws-server authorization tests (who may start, advance or end a
       # quiz) mock the database and Redis, so no services are needed.
       - name: Test ws-server

apps/http-server/package.json

@@ -5,6 +5,7 @@
   "main": "index.js",
   "scripts": {
     "build": "tsc -b",
+    "test": "vitest run",
     "dev": "tsc -b && node dist/index.js",
     "start": "node dist/index.js"
   },
@@ -38,6 +39,7 @@
     "ts-node-dev": "^2.0.0",
     "tsup": "^8.5.1",
     "typescript": "^6.0.2",
+    "vitest": "^5.0.2",
     "wait-on": "^9.0.4"
   }
 }

apps/http-server/src/middlewares/authmiddleware.test.ts

@@ -0,0 +1,81 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import jwt from "jsonwebtoken";
+import type { NextFunction, Request, Response } from "express";
+import { authMiddleware } from "./authmiddleware";
+
+const SECRET = "test-secret";
+
+// Just enough of a request and response for the middleware to run on.
+const run = async (authorization?: string) => {
+    const req = { headers: authorization ? { authorization } : {} } as unknown as Request;
+    const res = { status: vi.fn().mockReturnThis(), json: vi.fn() } as unknown as Response;
+    const next = vi.fn() as unknown as NextFunction;
+
+    await authMiddleware(req, res, next);
+
+    return { req, res, next };
+};
+
+describe("authMiddleware", () => {
+    beforeEach(() => {
+        vi.stubEnv("JWT_SECRET", SECRET);
+        vi.spyOn(console, "log").mockImplementation(() => {});
+    });
+
+    afterEach(() => {
+        vi.unstubAllEnvs();
+        vi.restoreAllMocks();
+    });
+
+    it("lets a valid token through and sets req.userId", async () => {
+        const token = jwt.sign({ userId: "user-1" }, SECRET);
+        const { req, res, next } = await run(`Bearer ${token}`);
+
+        expect(next).toHaveBeenCalledOnce();
+        expect(req.userId).toBe("user-1");
+        expect(res.status).not.toHaveBeenCalled();
+    });
+
+    it("answers 401 for an expired token", async () => {
+        const token = jwt.sign({ userId: "user-1" }, SECRET, { expiresIn: -10 });
+        const { res, next } = await run(`Bearer ${token}`);
+
+        expect(res.status).toHaveBeenCalledWith(401);
+        expect(next).not.toHaveBeenCalled();
+        expect(console.log).not.toHaveBeenCalled();
+    });
+
+    it("answers 401 for a garbage token", async () => {
+        const { res, next } = await run("Bearer not-a-real-token");
+
+        expect(res.status).toHaveBeenCalledWith(401);
+        expect(next).not.toHaveBeenCalled();
+        expect(console.log).not.toHaveBeenCalled();
+    });
+
+    it("answers 401 for a token signed with another secret", async () => {
+        const token = jwt.sign({ userId: "user-1" }, "some-other-secret");
+        const { res, next } = await run(`Bearer ${token}`);
+
+        expect(res.status).toHaveBeenCalledWith(401);
+        expect(next).not.toHaveBeenCalled();
+    });
+
+    it("answers 401 when the payload has no userId", async () => {
+        const token = jwt.sign({ foo: "bar" }, SECRET);
+        const { res, next } = await run(`Bearer ${token}`);
+
+        expect(res.status).toHaveBeenCalledWith(401);
+        expect(next).not.toHaveBeenCalled();
+    });
+
+    it("keeps answering 500 for errors that are not token problems", async () => {
+        vi.spyOn(jwt, "verify").mockImplementation(() => {
+            throw new Error("something broke");
+        });
+        const { res, next } = await run("Bearer anything");
+
+        expect(res.status).toHaveBeenCalledWith(500);
+        expect(next).not.toHaveBeenCalled();
+    });
+});

apps/http-server/src/middlewares/authmiddleware.ts

@@ -19,10 +19,16 @@ export const authMiddleware = async (req: Request, res: Response, next: NextFunc
         if (!token) return errorResponse(res, 401, "You're not authorized...")
 
         const decoded = jwt.verify(token, process.env.JWT_SECRET || "secret") as { userId: string }
+
+        if (!decoded.userId) return errorResponse(res, 401, "You're not authorized...")
+
         req.userId = decoded.userId;
         next();
 
     } catch (error) {
+        // Expired, malformed or badly signed token: the client needs a 401 to log the user out.
+        if (error instanceof jwt.JsonWebTokenError) return errorResponse(res, 401, "You're not authorized...")
+
         console.log("Error in authentication", error);
         return errorResponse(res, 500, "Internal Server Error...")
     }

apps/http-server/tsconfig.json

@@ -12,5 +12,6 @@
     "typeRoots": ["./node_modules/@types", "./src/types"]
   },
   "include": ["src"],
-  "exclude": ["node_modules", "dist"]
+  // Tests run through vitest and are not part of the shipped build.
+  "exclude": ["node_modules", "dist", "src/**/*.test.ts"]
 }