DC-150 Add: Address stale PKCE scenario in sessionStorage

Author: noxmwalsh

src/SEBT.Portal.Web/src/lib/oidc-pkce.test.ts

@@ -1,10 +1,14 @@
-import { describe, expect, it } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 import {
   buildAuthorizationUrl,
+  clearPkceStorage,
   generateCodeChallenge,
   generateCodeVerifier,
-  generateState
+  generateState,
+  getPkceFromStorage,
+  PKCE_STORAGE_MAX_AGE_MS,
+  savePkceForCallback
 } from './oidc-pkce'
 
 describe('oidc-pkce', () => {
@@ -90,4 +94,70 @@ describe('oidc-pkce', () => {
       expect(challenge).not.toContain('=')
     })
   })
+
+  describe('savePkceForCallback / getPkceFromStorage', () => {
+    beforeEach(() => {
+      sessionStorage.clear()
+      vi.useRealTimers()
+    })
+
+    afterEach(() => {
+      sessionStorage.clear()
+      vi.useRealTimers()
+    })
+
+    it('round-trips PKCE with storedAtMs and returns it while fresh', () => {
+      const before = Date.now()
+      savePkceForCallback('st', 'verifier', {
+        redirectUri: 'https://app/cb',
+        tokenEndpoint: 'https://auth/token',
+        clientId: 'cid'
+      })
+      const got = getPkceFromStorage()
+      expect(got).not.toBeNull()
+      expect(got!.state).toBe('st')
+      expect(got!.code_verifier).toBe('verifier')
+      expect(got!.storedAtMs).toBeGreaterThanOrEqual(before)
+      expect(got!.storedAtMs).toBeLessThanOrEqual(Date.now())
+    })
+
+    it('returns null and clears storage when older than max age', () => {
+      vi.useFakeTimers()
+      const t0 = Date.now()
+      vi.setSystemTime(t0)
+      savePkceForCallback('st', 'verifier', {
+        redirectUri: 'https://app/cb',
+        tokenEndpoint: 'https://auth/token',
+        clientId: 'cid'
+      })
+      vi.setSystemTime(t0 + PKCE_STORAGE_MAX_AGE_MS + 1)
+      expect(getPkceFromStorage()).toBeNull()
+      expect(sessionStorage.getItem('oidc_co_pkce')).toBeNull()
+    })
+
+    it('returns null and clears storage when storedAtMs is missing (legacy blob)', () => {
+      sessionStorage.setItem(
+        'oidc_co_pkce',
+        JSON.stringify({
+          state: 's',
+          code_verifier: 'v',
+          redirect_uri: 'https://r',
+          token_endpoint: 'https://t',
+          client_id: 'c'
+        })
+      )
+      expect(getPkceFromStorage()).toBeNull()
+      expect(sessionStorage.getItem('oidc_co_pkce')).toBeNull()
+    })
+
+    it('clearPkceStorage removes the key', () => {
+      savePkceForCallback('st', 'verifier', {
+        redirectUri: 'https://app/cb',
+        tokenEndpoint: 'https://auth/token',
+        clientId: 'cid'
+      })
+      clearPkceStorage()
+      expect(sessionStorage.getItem('oidc_co_pkce')).toBeNull()
+    })
+  })
 })

src/SEBT.Portal.Web/src/lib/oidc-pkce.ts

@@ -4,9 +4,15 @@
 
 const CO_PKCE_STORAGE_KEY = 'oidc_co_pkce'
 
+/**
+ * Discard PKCE blobs older than this so a long-lived tab cannot complete a flow with stale state.
+ * Aligned with typical authorization-code lifetime plus a small buffer.
+ */
+export const PKCE_STORAGE_MAX_AGE_MS = 15 * 60 * 1000
+
 function randomBase64Url(length: number): string {
   const bytes = new Uint8Array(length)
-  // crypto.getRandomValues is required for PKCE security — Math.random is not cryptographically secure.
+  // crypto.getRandomValues is required for PKCE security; Math.random is not cryptographically secure.
   // All modern browsers support this (since 2013), and generateCodeChallenge already requires crypto.subtle.
   crypto.getRandomValues(bytes)
   return btoa(String.fromCharCode(...bytes))
@@ -86,6 +92,8 @@ export interface StoredPkce {
   isStepUp?: boolean
   /** URL to redirect to after step-up completes. */
   returnUrl?: string
+  /** When this blob was written (`Date.now()`), for max-age checks. */
+  storedAtMs: number
 }
 
 export function savePkceForCallback(
@@ -106,32 +114,45 @@ export function savePkceForCallback(
     redirect_uri: config.redirectUri,
     token_endpoint: config.tokenEndpoint,
     client_id: config.clientId,
+    storedAtMs: Date.now(),
     ...(config.isStepUp !== undefined && { isStepUp: config.isStepUp }),
     ...(config.returnUrl !== undefined &&
       config.returnUrl !== '' && { returnUrl: config.returnUrl })
   }
-  // PKCE data is stored in sessionStorage only — not localStorage.
-  // localStorage would persist across tabs/sessions, which could allow stale PKCE to be accepted.
+  // PKCE data is stored in sessionStorage only, not localStorage:
+  // localStorage would persist across tabs/sessions and could allow stale PKCE to be accepted.
   sessionStorage.setItem(CO_PKCE_STORAGE_KEY, JSON.stringify(payload))
 }
 
+function parseStoredAtMs(value: unknown): number | null {
+  if (typeof value !== 'number' || !Number.isFinite(value) || value <= 0) return null
+  return value
+}
+
 export function getPkceFromStorage(): StoredPkce | null {
   if (typeof window === 'undefined') return null
   const raw = sessionStorage.getItem(CO_PKCE_STORAGE_KEY)
   if (!raw) return null
   try {
     const payload = JSON.parse(raw) as StoredPkce
     if (
-      payload?.state &&
-      payload?.code_verifier &&
-      payload?.redirect_uri &&
-      payload?.token_endpoint &&
-      payload?.client_id
+      !payload?.state ||
+      !payload?.code_verifier ||
+      !payload?.redirect_uri ||
+      !payload?.token_endpoint ||
+      !payload?.client_id
     ) {
-      return payload
+      sessionStorage.removeItem(CO_PKCE_STORAGE_KEY)
+      return null
+    }
+    const storedAtMs = parseStoredAtMs(payload.storedAtMs)
+    if (storedAtMs === null || Date.now() - storedAtMs > PKCE_STORAGE_MAX_AGE_MS) {
+      sessionStorage.removeItem(CO_PKCE_STORAGE_KEY)
+      return null
     }
+    return { ...payload, storedAtMs }
   } catch {
-    // ignore
+    sessionStorage.removeItem(CO_PKCE_STORAGE_KEY)
   }
   return null
 }