fix: validateOnReuse incorrectly throws

Logic for validateOnReuse parameter is still incorrect.
If validateOnReuse is false, overwrite is true, but the token
from the cookie is invalid, this should silently fail and return
a new valid token instead.

Author: psibean

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;