Add expiry and replay-protection tests for auth challenge flow (#132) (#147)

Fix atomic challenge consumption to close a replay race: verify was
reading the challenge, then doing an async signature check, and only
deleting it afterwards. Two concurrent verify calls with the same
signature could both read the still-valid nonce before either cleared
it, producing two sessions from one challenge. consumeChallenge now
deletes at read time, before the async verify call.

Closes #132

Author: karanjadavi

src/auth/session.test.ts

@@ -99,6 +99,7 @@ import {
   createChallenge,
   getChallenge,
   clearChallenge,
+  consumeChallenge,
   createSession,
   requireSession,
   revokeSession,
@@ -134,6 +135,31 @@ describe("challenges", () => {
     clearChallenge("0xfeed");
     expect(getChallenge("0xfeed")).toBeNull();
   });
+
+  it("consumeChallenge returns the record exactly once, then null on reuse", () => {
+    const { nonce } = createChallenge("0xC0FFEE");
+    const first = consumeChallenge("0xc0ffee");
+    expect(first?.nonce).toBe(nonce);
+    const second = consumeChallenge("0xc0ffee");
+    expect(second).toBeNull();
+  });
+
+  it("consumeChallenge rejects an expired challenge instead of returning it", () => {
+    createChallenge("0xdeadbeef");
+    vi.advanceTimersByTime(CHALLENGE_TTL_MS + 1);
+    expect(consumeChallenge("0xdeadbeef")).toBeNull();
+  });
+
+  it("consumeChallenge deletes before any caller can read it again (closes the replay race)", () => {
+    createChallenge("0xrace");
+    // Simulates two concurrent /auth/verify requests reading the same nonce:
+    // only the first should ever see a non-null record.
+    const attempt1 = consumeChallenge("0xrace");
+    const attempt2 = consumeChallenge("0xrace");
+    expect(attempt1).not.toBeNull();
+    expect(attempt2).toBeNull();
+    expect(getChallenge("0xrace")).toBeNull();
+  });
 });
 
 describe("sessions", () => {
@@ -234,4 +260,4 @@ describe("sessions", () => {
     const ok = await requireSession("0xabc", token);
     expect(ok).toBe(false);
   });
-});
+});

src/auth/session.ts

@@ -63,6 +63,26 @@ export function clearChallenge(address: string) {
   challenges.delete(address.toLowerCase());
 }
 
+/**
+ * Atomically reads and deletes the challenge for an address in a single step.
+ *
+ * This must be used (instead of getChallenge + a later clearChallenge) anywhere a
+ * challenge is about to be verified. getChallenge is read-only, so if it's read at
+ * the start of an async verification and only cleared afterwards, two concurrent
+ * requests can both read the same still-valid nonce before either one clears it —
+ * letting the same challenge be consumed twice (a replay bypass). Deleting it at
+ * read time closes that gap: the second concurrent caller sees it already gone.
+ *
+ * @param address - The user's Starknet wallet address
+ * @returns The challenge record if it existed and was still valid, otherwise null
+ */
+export function consumeChallenge(address: string) {
+  const rec = getChallenge(address);
+  if (!rec) return null;
+  challenges.delete(address.toLowerCase());
+  return rec;
+}
+
 /**
  * Creates a new session in PostgreSQL for the given wallet address.
  * Generates a random 24-byte hex token, hashes it with SHA-256 for database storage,
@@ -184,4 +204,4 @@ if (env.NODE_ENV !== "test") {
     });
   }, SESSION_SWEEP_INTERVAL_MS).unref();
 }
-/* v8 ignore stop */
+/* v8 ignore stop */

src/routes/auth.test.ts

@@ -215,6 +215,81 @@ describe("Auth Routes Integration", () => {
     expect(logoutPostLogoutRes.status).toBe(401);
   });
 
+  it("rejects verify once the challenge TTL has elapsed", async () => {
+    const address = "0xExpiredChallenge";
+    const appInstance = makeApp();
+
+    const challengeRes = await request(appInstance)
+      .post("/api/v1/auth/challenge")
+      .send({ address });
+    expect(challengeRes.status).toBe(200);
+
+    vi.advanceTimersByTime(challengeRes.body.expires_in_ms + 1);
+
+    mockProvider.verifyMessageInStarknet.mockResolvedValue(true);
+    const verifyRes = await request(appInstance)
+      .post("/api/v1/auth/verify")
+      .send({ address, signature: ["0xsig1", "0xsig2"] });
+
+    expect(verifyRes.status).toBe(400);
+    expect(verifyRes.body.error).toMatch(/No active challenge/);
+  });
+
+  it("rejects a replayed verify call reusing an already-consumed challenge", async () => {
+    const address = "0xReplayAttempt";
+    const appInstance = makeApp();
+
+    const challengeRes = await request(appInstance)
+      .post("/api/v1/auth/challenge")
+      .send({ address });
+    expect(challengeRes.status).toBe(200);
+
+    mockProvider.verifyMessageInStarknet.mockResolvedValue(true);
+
+    const firstVerify = await request(appInstance)
+      .post("/api/v1/auth/verify")
+      .send({ address, signature: ["0xsig1", "0xsig2"] });
+    expect(firstVerify.status).toBe(200);
+    expect(firstVerify.body.ok).toBe(true);
+
+    // Replay: same address/signature submitted again after the challenge was consumed.
+    const secondVerify = await request(appInstance)
+      .post("/api/v1/auth/verify")
+      .send({ address, signature: ["0xsig1", "0xsig2"] });
+
+    expect(secondVerify.status).toBe(400);
+    expect(secondVerify.body.error).toMatch(/No active challenge/);
+    // Only one session should have ever been created from the one valid challenge.
+    expect(mockState.sessions).toHaveLength(1);
+  });
+
+  it("accepts a valid challenge exactly once, even when verify is attempted concurrently", async () => {
+    const address = "0xConcurrentVerify";
+    const appInstance = makeApp();
+
+    const challengeRes = await request(appInstance)
+      .post("/api/v1/auth/challenge")
+      .send({ address });
+    expect(challengeRes.status).toBe(200);
+
+    mockProvider.verifyMessageInStarknet.mockResolvedValue(true);
+
+    // Fire two verify requests concurrently off the same still-valid challenge.
+    const [res1, res2] = await Promise.all([
+      request(appInstance)
+        .post("/api/v1/auth/verify")
+        .send({ address, signature: ["0xsig1", "0xsig2"] }),
+      request(appInstance)
+        .post("/api/v1/auth/verify")
+        .send({ address, signature: ["0xsig1", "0xsig2"] }),
+    ]);
+
+    const statuses = [res1.status, res2.status].sort();
+    // Exactly one succeeds; the other finds the challenge already consumed.
+    expect(statuses).toEqual([200, 400]);
+    expect(mockState.sessions).toHaveLength(1);
+  });
+
   it("returns 401 for unauthorized endpoints with generic message", async () => {
     const appInstance = makeApp();
 
@@ -226,4 +301,4 @@ describe("Auth Routes Integration", () => {
     expect(logoutRes.status).toBe(401);
     expect(logoutRes.body.error).toBe("Unauthorized");
   });
-});
+});

src/routes/auth.ts

@@ -3,10 +3,9 @@ import { z } from "zod";
 import { provider, getCachedNetworkInfo } from "../starknet/client.js";
 import { buildTypedChallenge } from "../auth/challenge.js";
 import {
-  clearChallenge,
+  consumeChallenge,
   createChallenge,
   createSession,
-  getChallenge,
   requireSession,
   revokeSession,
 } from "../auth/session.js";
@@ -60,7 +59,10 @@ authRouter.post("/auth/challenge", async (req, res, next) => {
 authRouter.post("/auth/verify", async (req, res, next) => {
   try {
     const { address, signature } = VerifyBody.parse(req.body);
-    const ch = getChallenge(address);
+    // Consume (read + delete) the challenge atomically, before the async verify call,
+    // so two concurrent requests can't both read it while it's still valid and both
+    // pass verification off the same nonce.
+    const ch = consumeChallenge(address);
     if (!ch) {
       res
         .status(400)
@@ -76,7 +78,6 @@ authRouter.post("/auth/verify", async (req, res, next) => {
       res.status(401).json({ error: "Invalid signature" });
       return;
     }
-    clearChallenge(address);
     const session = await createSession(address);
     res.json({
       ok: true,
@@ -117,4 +118,4 @@ authRouter.post("/auth/logout", requireAuth, async (req, res, next) => {
   } catch (e) {
     next(e);
   }
-});
+});