fix: validateOnReuse incorrectly throws

README.md

@@ -410,6 +410,8 @@ Used to customise the error response <code>statusCode</code>, the contained erro
 
 <p>By default if a <code>csrf-csrf</code> cookie already exists on an incoming request, <code>generateCsrfToken</code> will not overwrite it, it will return the existing token so long as the token is valid. If you wish to force a token generation, you can use the <code>overwrite</code> option of the third parameter:</p>
 
+<p>The <code>validateOnReuse</code> parameter is a bit misleading, and is also deprecated (will be removed with the next major release). A better name for it would be <code>throwOnReuseInvalid</code>.</p>
+
 ```ts
 generateCsrfToken(req, res, { overwrite: true }); // This will force a new token to be generated, and a new cookie to be set, even if one already exists
 ```

src/index.ts

@@ -71,15 +71,19 @@ export function doubleCsrf({
     const possibleSecrets = getPossibleSecrets(req);
     // If cookie is not present, this is a new user (no existing csrfToken)
     // If ovewrite is true, always generate a new token.
-    // If overwrite is false and validateOnReuse is true and there is an existing token, validate it first
-    // If overwrite is false and validateOnReuse is false, just return the cookie value
+    // If the existing token is valid, return it
+    // If the existing token is not valid and validateOnReuse is true, throw an error
+    // If the existing token is not valid and validateOnReuse is false, silently ignore, return new valid token
     if (cookieName in req.cookies && !overwrite) {
-      if (!validateOnReuse || (validateOnReuse && validateCsrfToken(req, possibleSecrets))) {
-        //  If validateOnReuse is false, or if the token is valid, reuse it
+      // If the current token is valid, reuse it
+      if (validateCsrfTokenCookie(req, possibleSecrets)) {
         return getCsrfTokenFromCookie(req);
       }
-      // This only happens if overwrite is false and validateOnReuse is true
-      throw invalidCsrfTokenError;
+
+      // If the current token is invalid and validateOnReuse is true, throw here
+      if (validateOnReuse) {
+        throw invalidCsrfTokenError;
+      }
     }
     // otherwise, generate a completely new token
     // the 'newest' or preferred secret is the first one in the array
@@ -112,7 +116,38 @@ export function doubleCsrf({
     return csrfToken;
   };
 
-  const getCsrfTokenFromCookie = (req: Request) => req.cookies[cookieName] as string;
+  const getCsrfTokenFromCookie = (req: Request) => req.cookies[cookieName] ?? "";
+
+  const validateHmac = ({
+    expectedHmac,
+    req,
+    randomValue,
+    possibleSecrets,
+  }: { expectedHmac: string; possibleSecrets: Array<string>; randomValue: string; req: Request }) => {
+    const message = constructMessage(req, randomValue);
+    for (const secret of possibleSecrets) {
+      const hmacForSecret = generateHmac(secret, message);
+      if (expectedHmac === hmacForSecret) return true;
+    }
+
+    return false;
+  };
+
+  const validateCsrfTokenCookie = (req: Request, possibleSecrets: Array<string>) => {
+    const csrfTokenFromCookie = getCsrfTokenFromCookie(req);
+    const [expectedHmac, randomValue] = csrfTokenFromCookie.split(csrfTokenDelimiter);
+
+    if (
+      typeof expectedHmac !== "string" ||
+      expectedHmac === "" ||
+      typeof randomValue !== "string" ||
+      randomValue === ""
+    ) {
+      return false;
+    }
+
+    return validateHmac({ expectedHmac, possibleSecrets, randomValue, req });
+  };
 
   // given an array of secrets, checks whether at least one of the secrets constructs a matching hmac
   const validateCsrfToken: CsrfTokenValidator = (req, possibleSecrets) => {
@@ -131,13 +166,7 @@ export function doubleCsrf({
     // The reason it's safe for us to only validate the hmac and random value from the cookie here
     // is because we've already checked above whether the token in the cookie and the token provided
     // by the request are the same.
-    const message = constructMessage(req, randomValue);
-    for (const secret of possibleSecrets) {
-      const hmacForSecret = generateHmac(secret, message);
-      if (receivedHmac === hmacForSecret) return true;
-    }
-
-    return false;
+    return validateHmac({ expectedHmac: receivedHmac, req, possibleSecrets, randomValue });
   };
 
   const validateRequest: CsrfRequestValidator = (req) => {

src/tests/testsuite.ts

@@ -10,7 +10,7 @@ import {
   getCookieValueFromResponse,
   switchSecret,
 } from "./utils/helpers.js";
-import { generateMocks, generateMocksWithToken, next } from "./utils/mock.js";
+import { generateMocks, generateMocksWithToken, next, setCsrfCookieOnRequest } from "./utils/mock.js";
 
 type CreateTestsuite = (
   name: string,
@@ -171,14 +171,29 @@ export const createTestSuite: CreateTestsuite = (name, doubleCsrfOptions) => {
         expect(generatedToken).not.toBe(csrfToken);
       });
 
-      it("should return existing CSRF token for a GET request that does not include the CSRF token", () => {
+      it("should return existing CSRF token for a GET request that does not send the CSRF token", () => {
         const { mockRequest, mockResponse, csrfToken } = generateMocksWithTokenInternal();
         mockRequest.method = "GET";
         mockRequest.headers[HEADER_KEY] = undefined;
         const reusedToken = generateCsrfToken(mockRequest, mockResponse);
 
         expect(reusedToken).toBe(csrfToken);
       });
+
+      it("should return a new and valid CSRF token for a GET request that does not send the CSRF token or have the CSRF cookie", () => {
+        const { mockRequest, mockResponse, csrfToken } = generateMocksWithTokenInternal();
+        mockRequest.method = "GET";
+        mockRequest.headers[HEADER_KEY] = undefined;
+        mockRequest.headers.cookie = `${cookieName}=`;
+        mockRequest.cookies[cookieName] = undefined;
+
+        const newToken = generateCsrfToken(mockRequest, mockResponse);
+        setCsrfCookieOnRequest(mockRequest, mockResponse, cookieName);
+        expect(newToken).not.toBe(csrfToken);
+        expect(validateRequest(mockRequest)).toBe(false);
+        mockRequest.headers[HEADER_KEY] = newToken;
+        expect(validateRequest(mockRequest)).toBe(true);
+      });
     });
 
     describe("validateRequest", () => {

src/tests/utils/mock.ts

@@ -78,6 +78,18 @@ export type GenerateMocksWithTokenOptions = {
   sessionIdentifier?: string;
 };
 
+export const setCsrfCookieOnRequest = (req: Request, response: Response, cookieName: string) => {
+  const { setCookie, cookieValue } = getCookieValueFromResponse(response);
+  req.headers.cookie = `${cookieName}=${cookieValue};`;
+
+  (req as any).cookies = undefined;
+  cookieParserMiddleware(req, response, next);
+  return {
+    setCookie,
+    cookieValue,
+  };
+};
+
 // Generate the request and response mocks.
 // Set them up as if they have been pre-processed in a valid state.
 export const generateMocksWithToken = ({
@@ -89,15 +101,12 @@ export const generateMocksWithToken = ({
   const { mockRequest, mockResponse, mockResponseHeaders } = generateMocks(sessionIdentifier);
 
   const csrfToken = generateCsrfToken(mockRequest, mockResponse);
-  const { setCookie, cookieValue } = getCookieValueFromResponse(mockResponse);
-  mockRequest.headers.cookie = `${cookieName}=${cookieValue};`;
+  const { setCookie, cookieValue } = setCsrfCookieOnRequest(mockRequest, mockResponse, cookieName);
   const decodedCookieValue = decodeURIComponent(cookieValue);
   // Have to delete the cookies object otherwise cookieParser will skip it's parsing.
   // After removing @types/cookie-parser and relying on cookie-parser provided types
   // the types prevent cookies from being undefined despite it being valid for cases
   // before the middleware runs.
-  (mockRequest as any).cookies = undefined;
-  cookieParserMiddleware(mockRequest, mockResponse, next);
   expect(getCookieFromRequest(cookieName, mockRequest)).toBe(decodedCookieValue);
 
   mockRequest.headers[HEADER_KEY] = csrfToken;