fix: session cookie is signed

README.md

@@ -88,11 +88,10 @@ Some super helpful references to keep handy:
     - [ ] Ensure that accounts are linked by sessionid (rather than email address)
       - [x] Store `sub` claim as part of token.
       - [x] redirect handler should lookup by `sub` claim not `email` claim.
-      - [ ] fix: sign the session id cookie
-    - [ ] should be tested with at least two (for the same user)
+      - [x] fix: session cookie is signed
     - [ ] Ensures User's ID is preserved with multiple providers (multiple tokens for a single user)
-    - [ ] Ensure `email_verified` claim: https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
-    - [ ] Make response_mode a environment variable (this removes more apple- SIWA dependency)
+  - [ ] Ensure `email_verified` claim: https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
+  - [ ] Make response_mode a environment variable (this removes more apple- SIWA dependency)
   - [ ] feat: logout endpoint (clears the session)
 
 - [ ] feat: CSRF token middleware in all state-changing APIs:

server/jest.config.js

@@ -7,7 +7,8 @@ module.exports = {
   globalTeardown: "./test/support/globalTeardown.ts",
   globals: {
     "ts-jest": {
-      tsconfig: "tsconfig.prod.json",
+      tsconfig: "tsconfig.jest.json",
+      isolatedModules: false
     },
   },
   collectCoverageFrom: ["src/**/*.ts", "!src/react-app/**"],

server/src/http/get-auth-login-000provider/index.ts

@@ -1,3 +1,4 @@
-import login from "@architect/shared/lambda/oauth/handlers/login"
+import loginHandlerFactory from "@architect/shared/lambda/oauth/handlers/login"
+import userRepositoryFactory from "@architect/shared/lambda/oauth/repository/UserRepository"
 
-export const handler = login
+export const handler = loginHandlerFactory(userRepositoryFactory())

server/src/shared/lambda/middleware/csrf.spec.ts

@@ -1,16 +1,80 @@
+import sinon from "sinon"
+import { randomInt } from "../../../../test/support"
 import { LambdaHttpResponse } from "../../lambda"
-import { addCsrfTokenToResponse, CSRF_HEADER_NAME } from "./csrf"
+import {
+  addCsrfTokenToResponse,
+  createCSRFToken,
+  CSRF_HEADER_NAME,
+  isTokenValid,
+} from "./csrf"
 
 describe("csrf", () => {
-  describe("response middleware", () => {
-    it("should write csrf token to response", async () => {
-      const res: LambdaHttpResponse = {}
-      await addCsrfTokenToResponse("foo", res)
-      expect(res).toHaveProperty("headers")
-      expect(res.headers).toHaveProperty(CSRF_HEADER_NAME)
+  // preserve environment
+  const OLD_ENV = process.env
+  afterAll(() => {
+    process.env = OLD_ENV
+  })
+
+  afterEach(() => {
+    process.env = OLD_ENV
+  })
+
+  afterEach(() => {
+    sinon.restore()
+  })
+
+  it("should write csrf token to response", async () => {
+    const res: LambdaHttpResponse = {}
+    await addCsrfTokenToResponse("foo", res)
+    expect(res).toHaveProperty("headers")
+    expect(res.headers).toHaveProperty(CSRF_HEADER_NAME)
+  })
+
+  it("should require response", async () => {
+    const res: LambdaHttpResponse = {}
+    expect(
+      addCsrfTokenToResponse("foo", (null as unknown) as LambdaHttpResponse)
+    ).rejects.toThrowError(/response/)
+  })
+
+  describe("isTokenValid", () => {
+    it("should accept valid token", async () => {
+      const testSessionID = `test-${randomInt()}`
+      const tok = await createCSRFToken(testSessionID)
+      expect(isTokenValid(tok, testSessionID)).toBeTruthy()
+    })
+
+    it("should reject invalid token", async () => {
+      const testSessionID = `test-${randomInt()}`
+      const tok = await createCSRFToken(testSessionID)
+      expect(isTokenValid(tok, testSessionID + "foo")).toBeFalsy()
+      expect(isTokenValid(tok, "foo" + testSessionID)).toBeFalsy()
+    })
+
+    it("should warn if CSRF_TOKEN_WARNING_DISABLE is not present", async () => {
+      const consoleMock = sinon.mock(console)
+
+      delete process.env.CSRF_TOKEN_WARNING_DISABLE
+      consoleMock.expects("warn").once()
+      const testSessionID = `test-${randomInt()}`
+      const tok = await createCSRFToken(testSessionID)
+      expect(isTokenValid(tok, "foo")).toBeFalsy()
+
+      sinon.verifyAndRestore()
+    })
+
+    it("should not warn if CSRF_TOKEN_WARNING_DISABLE is not present", async () => {
+      const consoleMock = sinon.mock(console)
+
+      process.env.CSRF_TOKEN_WARNING_DISABLE = "anything"
+      consoleMock.expects("warn").never()
+      const testSessionID = `test-${randomInt()}`
+      const tok = await createCSRFToken(testSessionID)
+      expect(isTokenValid(tok, "foo")).toBeFalsy()
+
+      sinon.verifyAndRestore()
     })
 
-    it.todo("should test the csrfResponseMiddleware func")
   })
 
   describe("request middleware", () => {

server/src/shared/lambda/middleware/csrf.ts

@@ -4,6 +4,7 @@ import {
   LambdaHttpRequest,
   LambdaHttpResponse,
 } from "../../lambda"
+import { secretFromEnvironment } from "../../secretEnvironment"
 import Tokenater from "../../Tokenater"
 import { readSessionID } from "./session"
 
@@ -26,18 +27,13 @@ export async function createCSRFToken(sessionID: string): Promise<string> {
 export function isTokenValid(token: string, sessionID: string): boolean {
   const ater = createTokenater()
   if (!ater.isValid(token)) {
-    // don't bother with warnings if we're inside jest
-    if (!("CSRF_TOKEN_WARNING_DISABLE" in process.env)) {
-      // eslint-disable-next-line no-console
-      console.warn("CSRF token is expired or has been tampered with")
-    }
+    warn("CSRF token is expired or has been tampered with")
     return false
   }
   // our CSRF token has the session id in it. Now that we've validated the token, extract the session id and make sure that it matches
   const csrfSessionID = ater.getTokenValue(token)
   if (csrfSessionID != sessionID) {
-    // eslint-disable-next-line no-console
-    console.warn(
+    warn(
       "CSRF token does not match session:",
       csrfSessionID,
       "!=",
@@ -48,6 +44,13 @@ export function isTokenValid(token: string, sessionID: string): boolean {
   return true
 }
 
+function warn(message?: any, ...optionalParams: any[]): void {
+  if (!("CSRF_TOKEN_WARNING_DISABLE" in process.env)) {
+    // eslint-disable-next-line no-console
+    console.warn(message, optionalParams)
+  }
+}
+
 /**
  * Response middleware to add a CSRF token to the response that can be read/validated with the @see expectCsrfTokenWithRequest request middleware function.
  * @param handler Your HTTP handler that should run before this middleware adds the CSRF token header to the response.
@@ -59,17 +62,19 @@ export function csrfResponseMiddleware(
     const response = await handler(req)
     assert(response, "response expected from handler")
     // get the current session id:
-    const sessionID = readSessionID(req)
-    if (!sessionID) {
-      throw new Error("sessionID not on request session!")
+    const session = readSessionID(req)
+    if (!session) {
+      throw new Error("session not on request session!")
     }
     // add the CSRF token:
-    await addCsrfTokenToResponse(sessionID, response)
+    await addCsrfTokenToResponse(session.userID, response)
     return response
   }
   return thunk
 }
 
+type HttpResponseLike = Pick<LambdaHttpResponse, "headers">
+
 /**
  * Adds a CSRF token to the specified response object according to the [HMAC Based Token Pattern](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#hmac-based-token-pattern)
  * The token can be validated in a subsequent request with `expectCsrfTokenWithRequest`.
@@ -78,7 +83,7 @@ export function csrfResponseMiddleware(
  */
 export async function addCsrfTokenToResponse(
   sessionID: string,
-  response: LambdaHttpResponse
+  response: HttpResponseLike
 ): Promise<void> {
   if (!response) {
     throw new Error("response must be provided")
@@ -91,18 +96,8 @@ const createTokenater = (): Tokenater =>
   new Tokenater(getSecret(), Tokenater.DAYS_IN_MS * 1)
 
 function getSecret(): string {
-  let secret = process.env.CSRF_SECRET
-  if (!secret) {
-    if (process.env.NODE_ENV == "production") {
-      throw new Error(
-        "CSRF_SECRET environment variable MUST be provided in production environments"
-      )
-    }
-    // eslint-disable-next-line no-console
-    console.warn(
-      "CSRF_SECRET environment variable SHOULD be provided in pre-production environments"
-    )
-    secret = `${process.env.AWS_SECRET_ACCESS_KEY} not so secret`
-  }
-  return secret
+  return secretFromEnvironment(
+    "WAS_CSRF_SECRET",
+    `${process.env.NODE_ENV}`
+  ).padEnd(32, ".")
 }

server/src/shared/lambda/middleware/session.spec.ts

@@ -1,24 +1,24 @@
-import { writeSessionID, readSessionID } from "./session"
+import { writeSessionID, readSessionID, UserSession, createAnonymousSessionID } from "./session"
 import { createMockRequest } from "../../../../test/support/lambda"
-import { randomInt } from "../../../../test/support"
 
 describe("session", () => {
-  let testSessionID = ""
+  let testSesson: UserSession
+
   beforeEach(() => {
-    testSessionID = "session-id-" + randomInt().toString()
+    testSesson = createAnonymousSessionID()
   })
 
   it("should add and read the same session id", async () => {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const response: any = {}
-    writeSessionID(response, testSessionID)
+    writeSessionID(response, testSesson)
     expect(response).toHaveProperty("cookies")
     // steal the cookie for the read test:
     const stolenCookie = response.cookies[0]
 
     // read it:
     const req = createMockRequest({ cookies: [stolenCookie] })
     const found = readSessionID(req)
-    expect(found).toStrictEqual(testSessionID)
+    expect(found).toStrictEqual(testSesson)
   })
 })