Merge pull request #264 from NYPL/main

csrf bugfix

Author: charmingduchess

.eslintrc

@@ -36,7 +36,8 @@
     "react/display-name": 0,
     "@typescript-eslint/no-use-before-define": 0,
     "@typescript-eslint/camelcase": "off",
-    "react/react-in-jsx-scope": 0
+    "react/react-in-jsx-scope": 0,
+    "@typescript-eslint/explicit-module-boundary-types": 0
   },
   "settings": {
     "react": {

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## CHANGE LOG
 
+### v1.1.1 Fix CSRF regression
+- Ensure that CSRF headers are not overwritten by nyplUserHasRegistered headers
+- Refactor CSRF utils
+
 ### v1.1.0 Duplicate patron bug fixes
 - Add cookie-based redirect back to congrats page from any page after success
 - Remove hasUsernameBeenValidated flag and hidden input field

pages/new/index.tsx

@@ -8,7 +8,12 @@ import { useEffect } from "react";
 import { Heading } from "@nypl/design-system-react-components";
 import RoutingLinks from "../../src/components/RoutingLinks.tsx";
 import useFormDataContext from "../../src/context/FormDataContext";
-import { getCsrfToken } from "../../src/utils/utils";
+import {
+  generateNewCookieTokenAndHash,
+  generateNewToken,
+  parseTokenFromPostRequestCookies,
+} from "../../src/utils/csrfUtils";
+import * as cookie from "../../src/utils/CookieUtils";
 
 import { GetServerSideProps } from "next";
 import { useTranslation } from "next-i18next";
@@ -61,12 +66,24 @@ function HomePage({ policyType, csrfToken, lang }: HomePageProps) {
 }
 
 export const getServerSideProps: GetServerSideProps = async (context) => {
-  const { csrfToken } = getCsrfToken(context.req, context.res);
+  const tokenFromRequestCookie = parseTokenFromPostRequestCookies(context.req);
   const { query } = context;
-  context.res.setHeader(
-    "Set-Cookie",
-    `nyplUserRegistered=false; Max-Age=-1; path=/; domain=${cookieDomain};`
-  );
+  let newTokenCookie;
+  let csrfToken = tokenFromRequestCookie.value;
+
+  if (!csrfToken) {
+    csrfToken = generateNewToken();
+    const newTokenCookieString = generateNewCookieTokenAndHash(csrfToken);
+    newTokenCookie = cookie.buildCookieHeader(newTokenCookieString);
+  }
+
+  const headers = [
+    // reset cookie that would otherwise bump users out of the flow
+    // to succcess page
+    `nyplUserRegistered=false; Max-Age=-1; path=/; domain=${cookieDomain};`,
+    newTokenCookie,
+  ];
+  context.res.setHeader("set-cookie", headers);
 
   return {
     props: {

src/utils/CookieUtils.ts

@@ -5,14 +5,44 @@ import { serialize } from "cookie";
  * Function to set cookies on the server side based on with minor updates:
  * https://github.com/vercel/next.js/blob/master/examples/api-routes-middleware/utils/cookies.js
  */
-const set = (res, name, value, options) => {
+const set = (res, value) => {
+  const cookie = buildCookieHeader(value);
+  return res.setHeader("Set-Cookie", cookie);
+};
+
+const buildCookieHeader = (value) => {
+  const { name, options } = metadata().csrfToken;
   const stringValue =
     typeof value === "object" ? "j:" + JSON.stringify(value) : String(value);
+  return serialize(name, String(stringValue), options);
+};
+
+// Use secure cookies if the site uses HTTPS
+// This being conditional allows cookies to work non-HTTPS development URLs
+// Honour secure cookie option, which sets 'secure' and also adds '__Secure-'
+// prefix, but enable them by default if the site URL is HTTPS; but not for
+// non-HTTPS URLs like http://localhost which are used in development).
+// For more on prefixes see:
+// https://googlechrome.github.io/samples/cookie-prefixes/
 
-  return res.setHeader(
-    "Set-Cookie",
-    serialize(name, String(stringValue), options)
-  );
+const metadata = () => {
+  const useSecureCookies = process.env.NODE_ENV === "production";
+  const name = `${useSecureCookies ? "__Host-" : ""}nypl.csrf-token`;
+  return {
+    // default cookie options
+    csrfToken: {
+      // Default to __Host- for CSRF token for additional protection if using
+      // useSecureCookies.
+      // NB: The `__Host-` prefix is stricter than the `__Secure-` prefix.
+      name,
+      options: {
+        httpOnly: true,
+        sameSite: "lax",
+        path: "/",
+        secure: useSecureCookies,
+      },
+    },
+  };
 };
 
-export default { set };
+export { set, metadata, buildCookieHeader };

src/utils/__tests__/api.test.ts

@@ -8,12 +8,20 @@ import {
   validateUsername,
   callPatronAPI,
 } from "../api";
-import utils from "../utils";
 const axios = require("axios");
 import moment from "moment";
 
 jest.mock("axios");
-jest.mock("../utils");
+jest.mock("../csrfUtils", () => {
+  return {
+    ...jest.requireActual("../csrfUtils"),
+    parseCsrfToken: jest.fn(() => "12345"),
+    validateCsrfToken: jest.fn(() => ({
+      csrfToken: "csrfToken",
+      csrfTokenValid: true,
+    })),
+  };
+});
 let mockedReturnedJson = {};
 class MockRes {
   status() {
@@ -25,10 +33,6 @@ class MockRes {
   }
 }
 const mockRes = new MockRes();
-utils.getCsrfToken = jest.fn(() => ({
-  csrfToken: "csrfToken",
-  csrfTokenValid: true,
-}));
 
 describe("constructApiHeaders", () => {
   test("it returns authorization headers object", () => {
@@ -266,7 +270,7 @@ describe("validateAddress", () => {
         isWorkAddress: false,
       },
       cookies: {
-        "next-auth.csrf-token": "csrfToken",
+        "nypl.csrf-token": "csrfToken",
       },
     };
     axios.post.mockResolvedValueOnce({
@@ -356,7 +360,7 @@ describe("validateUsername", () => {
         username: "tomnook42",
       },
       cookies: {
-        "next-auth.csrf-token": "csrfToken",
+        "nypl.csrf-token": "csrfToken",
       },
     };
     axios.post.mockResolvedValue({
@@ -400,7 +404,7 @@ describe("validateUsername", () => {
         username: "tomnook42",
       },
       cookies: {
-        "next-auth.csrf-token": "csrfToken",
+        "nypl.csrf-token": "csrfToken",
       },
     };
     axios.post.mockRejectedValue({

src/utils/__tests__/csrfUtils.test.ts

@@ -0,0 +1,51 @@
+import * as csrfUtils from "../csrfUtils";
+
+jest.mock("../CookieUtils", () => {
+  return {
+    ...jest.requireActual("../CookieUtils.ts"),
+    set: jest.fn(),
+  };
+});
+jest.mock("crypto", () => {
+  return {
+    createHash: jest.fn().mockReturnValue({
+      update: () => ({
+        digest: () => "666",
+      }),
+    }),
+  };
+});
+
+describe("validateCsrfToken", () => {
+  test("it returns invalid when no token is set", () => {
+    const csrfTokenValid = csrfUtils.validateCsrfToken({
+      cookies: {},
+    });
+    // We don't really care what it is, just that it's there.
+    expect(csrfTokenValid).toEqual(false);
+  });
+
+  test("it returns token valid when request token matches cookie token", () => {
+    const isValid = csrfUtils.validateCsrfToken({
+      method: "POST",
+      body: { csrfToken: "12345" },
+      cookies: {
+        "nypl.csrf-token": "12345|666",
+      },
+    });
+
+    expect(isValid).toEqual(true);
+  });
+
+  test("it returns token invalid when request token does not match cookie token", () => {
+    const isValid = csrfUtils.validateCsrfToken({
+      method: "POST",
+      body: { csrfToken: "12345" },
+      cookies: {
+        "nypl.csrf-token": "12345|789",
+      },
+    });
+
+    expect(isValid).toEqual(false);
+  });
+});

src/utils/__tests__/utils.test.ts

@@ -1,16 +1,8 @@
-import {
-  getPageTitles,
-  createQueryParams,
-  createNestedQueryParams,
-  getCsrfToken,
-} from "../utils";
-import cookieUtils from "../CookieUtils";
-
-jest.mock("../CookieUtils");
+import * as utils from "../utils";
 
 describe("getPageTiles", () => {
   test("it returns text saying there are 5 steps", () => {
-    expect(getPageTitles()).toEqual({
+    expect(utils.getPageTitles()).toEqual({
       personal: "Step 1 of 5: Personal Information",
       address: "Step 2 of 5: Address",
       workAddress: "Alternate Address",
@@ -23,7 +15,7 @@ describe("getPageTiles", () => {
 
 describe("createQueryParams", () => {
   test("it should return an empty string with an empty object", () => {
-    expect(createQueryParams({})).toEqual("");
+    expect(utils.createQueryParams({})).toEqual("");
   });
 
   test("it should return a url query string", () => {
@@ -32,16 +24,16 @@ describe("createQueryParams", () => {
       key2: "value2",
       key3: "value3",
     };
-    expect(createQueryParams(data)).toEqual(
+    expect(utils.createQueryParams(data)).toEqual(
       "&key1=value1&key2=value2&key3=value3"
     );
   });
 });
 
 describe("createNestedQueryParams", () => {
   test("it should return an empty string with an empty string or type argument", () => {
-    expect(createNestedQueryParams({}, "key")).toEqual("");
-    expect(createNestedQueryParams({ key: "somevalue" }, "")).toEqual("");
+    expect(utils.createNestedQueryParams({}, "key")).toEqual("");
+    expect(utils.createNestedQueryParams({ key: "somevalue" }, "")).toEqual("");
   });
 
   test("it should return a nested url query string", () => {
@@ -50,46 +42,12 @@ describe("createNestedQueryParams", () => {
       key2: "value2",
       key3: "value3",
     };
-    expect(createNestedQueryParams(data, "results")).toEqual(
+    expect(utils.createNestedQueryParams(data, "results")).toEqual(
       `&results=${JSON.stringify(data)}`
     );
 
-    expect(createNestedQueryParams(data, "errors")).toEqual(
+    expect(utils.createNestedQueryParams(data, "errors")).toEqual(
       `&errors=${JSON.stringify(data)}`
     );
   });
 });
-
-describe("getCsrfToken", () => {
-  beforeAll(() => {
-    // We don't actually want to set any cookies so mock this.
-    cookieUtils.set = jest.fn(() => "ok");
-  });
-
-  test("it returns a new token which is not valid since it's new and not compared to an existing token", () => {
-    const { csrfToken, csrfTokenValid } = getCsrfToken({ cookies: {} }, {});
-    // We don't really care what it is, just that it's there.
-    expect(csrfToken).toBeDefined();
-    expect(csrfTokenValid).toEqual(false);
-  });
-
-  // TODO: it's hard to test when the true case happens because the secret is
-  // private in the function, by design and security.
-  test("it returns a false token validation", () => {
-    const firstCall = getCsrfToken({ cookies: {} }, {});
-
-    expect(firstCall.csrfToken).toBeDefined();
-    expect(firstCall.csrfTokenValid).toEqual(false);
-
-    const second = getCsrfToken(
-      {
-        cookies: { "next-auth.csrf-token": "wrong-token!" },
-      },
-      {}
-    );
-
-    // The first token should not be reused.
-    expect(second.csrfToken === firstCall.csrfToken).toEqual(false);
-    expect(second.csrfTokenValid).toEqual(false);
-  });
-});

src/utils/api.ts

@@ -13,7 +13,12 @@ import {
   FormAPISubmission,
   AddressResponse,
 } from "../interfaces";
-import utils from "./utils";
+import {
+  generateNewToken,
+  setCsrfTokenCookie,
+  validateCsrfToken,
+  parseTokenFromPostRequestCookies,
+} from "./csrfUtils";
 
 // Initializing the cors middleware
 export const cors = Cors({
@@ -238,7 +243,12 @@ function invalidCsrfResponse(res) {
  */
 export async function validateAddress(req, res, appObj = app) {
   const tokenObject = appObj["tokenObject"];
-  const { csrfTokenValid } = utils.getCsrfToken(req, res);
+  const parsedTokenFromRequestCookies = parseTokenFromPostRequestCookies(req);
+  const csrfTokenValid = validateCsrfToken(req);
+  if (!parsedTokenFromRequestCookies) {
+    const newToken = generateNewToken();
+    setCsrfTokenCookie(res, newToken);
+  }
   if (!csrfTokenValid) {
     return invalidCsrfResponse(res);
   }
@@ -293,7 +303,7 @@ export async function validateUsername(
   appObj = app
 ) {
   const tokenObject = appObj["tokenObject"];
-  const { csrfTokenValid } = utils.getCsrfToken(req, res);
+  const csrfTokenValid = validateCsrfToken(req);
   if (!csrfTokenValid) {
     return invalidCsrfResponse(res);
   }
@@ -438,7 +448,7 @@ export async function createPatron(
   appObj = app
 ) {
   const data = req.body;
-  const { csrfTokenValid } = utils.getCsrfToken(req, res);
+  const csrfTokenValid = validateCsrfToken(req);
   if (!csrfTokenValid) {
     return invalidCsrfResponse(res);
   }