Fix login persistence with AUTH_SEND_JWT_HEADER enabled

With AUTH_SEND_JWT_HEADER=true, frontend now properly handles JWT authentication: - Store JWT token in client-side cookie named 'JWT' - Extract and store XSRF token from JWT payload - Set Secure flag automatically when on HTTPS connection - Update documentation to clarify this behavior This fixes an issue where login state would be lost after page reload when using header-based JWT authentication.
2025-04-29 23:45:55 +01:00
parent 242499787e
commit ddb490bbc1
6 changed files with 263 additions and 22 deletions
@@ -96,7 +96,7 @@ type ServerCommand struct {
 			Cookie time.Duration `long:"cookie" env:"COOKIE" default:"200h" description:"auth cookie TTL"`
 		} `group:"ttl" namespace:"ttl" env-namespace:"TTL"`

-		SendJWTHeader bool   `long:"send-jwt-header" env:"SEND_JWT_HEADER" description:"send JWT as a header instead of cookie"`
+		SendJWTHeader bool   `long:"send-jwt-header" env:"SEND_JWT_HEADER" description:"send JWT as a header instead of server-set cookie; with this enabled, frontend stores the JWT in a client-side cookie (note: increases vulnerability to XSS attacks)"`
 		SameSite      string `long:"same-site" env:"SAME_SITE" description:"set same site policy for cookies" choice:"default" choice:"none" choice:"lax" choice:"strict" default:"default"` // nolint

 		Apple     AppleGroup `group:"apple" namespace:"apple" env-namespace:"APPLE" description:"Apple OAuth"`
@@ -8,15 +8,14 @@ interface CookieOptions {
  path?: string;
  domain?: string;
  secure?: boolean;
+  sameSite?: 'Strict' | 'Lax' | 'None';
 }

 export function setCookie(name: string, value: string, options: CookieOptions = {}) {
  if (options.expires) {
+    // Convert number (seconds) or Date to UTC string
    if (typeof options.expires === 'number') {
-      const d = new Date();
-      d.setTime(d.getTime() + options.expires * 1000);
-      options.expires = d;
-      options.expires = options.expires.toUTCString();
+      options.expires = new Date(Date.now() + options.expires * 1000).toUTCString();
    } else if (options.expires instanceof Date) {
      options.expires = options.expires.toUTCString();
    }
@@ -27,15 +26,63 @@ export function setCookie(name: string, value: string, options: CookieOptions =
  let updatedCookie = `${name}=${value}`;

  for (const [key, value] of Object.entries(options)) {
-    updatedCookie += `; ${key}`;
-    if (value !== true) {
-      updatedCookie += `=${value}`;
+    // For boolean attributes like 'secure', only add them if true, otherwise skip
+    if (value === true) {
+      updatedCookie += `; ${key}`;
+    }
+    if (typeof value !== 'boolean') {
+      updatedCookie += `; ${key}=${value}`;
    }
  }

  document.cookie = updatedCookie;
 }

+/**
+ * Sets a cookie with enhanced security options for authentication
+ * @param name The name of the cookie
+ * @param value The value to set
+ * @param options Additional cookie options
+ */
+export function setAuthCookie(name: string, value: string, options: CookieOptions = {}) {
+  const isSecure = window.location.protocol === 'https:';
+  const cookiePrefix = isSecure ? '__Host-' : '';
+
+  // Default options for auth cookies with strong security
+  const authOptions: CookieOptions = {
+    path: '/',
+    sameSite: 'Strict',
+    secure: isSecure,
+    ...options,
+  };
+
+  setCookie(`${cookiePrefix}${name}`, value, authOptions);
+}
+
+/**
+ * Clears an authentication cookie by setting its expiration to the past
+ * @param name The name of the cookie to clear
+ */
+export function clearAuthCookie(name: string) {
+  const isSecure = window.location.protocol === 'https:';
+  const cookiePrefix = isSecure ? '__Host-' : '';
+
+  setCookie(`${cookiePrefix}${name}`, '', {
+    path: '/',
+    secure: isSecure,
+    expires: new Date(0), // Set to epoch time to expire immediately
+  });
+
+  // Also try to clear the non-prefixed version to be thorough
+  if (cookiePrefix) {
+    setCookie(name, '', {
+      path: '/',
+      secure: isSecure,
+      expires: new Date(0),
+    });
+  }
+}
+
 export function getCookie(name: string) {
  const matches = document.cookie.match(
    new RegExp(`(?:^|; )${name.replace(/([.$?*|{}()[\]\\/+^])/g, '\\$1')}=([^;]*)`)
@@ -4,7 +4,16 @@ jest.mock('./settings', () => ({

 import { RequestError } from 'utils/errorUtils';
 import { API_BASE, BASE_URL } from './constants.config';
-import { apiFetcher, authFetcher, adminFetcher, JWT_HEADER } from './fetcher';
+import {
+  apiFetcher,
+  authFetcher,
+  adminFetcher,
+  JWT_HEADER,
+  JWT_COOKIE_NAME,
+  XSRF_COOKIE,
+  AUTH_COOKIE_TTL_SECONDS,
+} from './fetcher';
+import * as cookies from './cookies';

 type FetchImplementationProps = {
  status?: number;
@@ -31,6 +40,16 @@ function mockFetch({ headers = {}, data = {}, ...props }: FetchImplementationPro
 }

 describe('fetcher', () => {
+  // Mock cookies for the test environment
+  beforeEach(() => {
+    // Mock getCookie to always return undefined for XSRF_COOKIE
+    jest.spyOn(cookies, 'getCookie').mockImplementation(() => undefined);
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
  const headers = {};
  const apiUri = '/anything';
  const apiUrl = `${BASE_URL}${API_BASE}/anything?site=remark`;
@@ -96,6 +115,12 @@ describe('fetcher', () => {
  });

  describe('headers', () => {
+    beforeEach(() => {
+      // Clear cookies before each test
+      document.cookie = `${JWT_COOKIE_NAME}=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/;`;
+      document.cookie = `${XSRF_COOKIE}=; expires=Thu, 01 Jan 1970 00:00:00 UTC; path=/;`;
+    });
+
    it('should set active token and than clean it on unauthorized response', async () => {
      expect.assertions(4);

@@ -125,6 +150,119 @@ describe('fetcher', () => {

      expect(window.fetch).toHaveBeenCalledWith(apiUrl, { method: 'get', headers });
    });
+
+    it('should store JWT token in a cookie when received in header', async () => {
+      // Mock the auth cookie helper - use mockReturnValueOnce for cleaner tests
+      jest.spyOn(cookies, 'setAuthCookie').mockReturnValueOnce(undefined);
+
+      // Create test JWT token - we'll mock the parsing
+      const jwtToken = 'test.jwt.token';
+
+      // Mock parseJwtPayload implementation since it's not directly accessible
+      jest.spyOn(window, 'atob').mockReturnValueOnce(JSON.stringify({ jti: 'test-jti-id', sub: '1234567890' }));
+
+      mockFetch({ headers: { [JWT_HEADER]: jwtToken, ...headers } });
+      await apiFetcher.get(apiUri);
+
+      // Check that setAuthCookie was called for both JWT and XSRF tokens
+      expect(cookies.setAuthCookie).toHaveBeenCalledWith(
+        JWT_COOKIE_NAME,
+        jwtToken,
+        expect.objectContaining({ expires: AUTH_COOKIE_TTL_SECONDS })
+      );
+
+      expect(cookies.setAuthCookie).toHaveBeenCalledWith(
+        XSRF_COOKIE,
+        'test-jti-id',
+        expect.objectContaining({ expires: AUTH_COOKIE_TTL_SECONDS })
+      );
+    });
+
+    it('should call setAuthCookie with proper parameters when receiving JWT token', async () => {
+      // Spy on setAuthCookie calls with mockImplementationOnce and jest.fn()
+      jest.spyOn(cookies, 'setAuthCookie').mockImplementationOnce(jest.fn());
+
+      // Create test JWT token
+      const jwtToken = 'test.jwt.token';
+
+      // Mock parseJwtPayload implementation since it's not directly accessible
+      jest.spyOn(window, 'atob').mockReturnValueOnce(JSON.stringify({ jti: 'test-jti-id', sub: '1234567890' }));
+
+      mockFetch({ headers: { [JWT_HEADER]: jwtToken, ...headers } });
+      await apiFetcher.get(apiUri);
+
+      // Verify setAuthCookie was called with expected parameters
+      expect(cookies.setAuthCookie).toHaveBeenCalledWith(
+        JWT_COOKIE_NAME,
+        jwtToken,
+        expect.objectContaining({ expires: AUTH_COOKIE_TTL_SECONDS })
+      );
+
+      expect(cookies.setAuthCookie).toHaveBeenCalledWith(
+        XSRF_COOKIE,
+        'test-jti-id',
+        expect.objectContaining({ expires: AUTH_COOKIE_TTL_SECONDS })
+      );
+    });
+
+    it('should handle errors when setting cookies', async () => {
+      // Mock console.error using jest.spyOn
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+
+      // Make setAuthCookie throw an error
+      jest.spyOn(cookies, 'setAuthCookie').mockImplementationOnce(() => {
+        throw new Error('Cookie access denied');
+      });
+
+      // Create test JWT token
+      const jwtToken = 'test.jwt.token';
+
+      // Mock parseJwtPayload implementation since it's not directly accessible
+      jest.spyOn(window, 'atob').mockReturnValueOnce(JSON.stringify({ jti: 'test-jti-id', sub: '1234567890' }));
+
+      mockFetch({ headers: { [JWT_HEADER]: jwtToken, ...headers } });
+
+      // This should not throw despite cookie setting failing
+      await apiFetcher.get(apiUri);
+
+      // Error should be logged
+      expect(consoleErrorSpy).toHaveBeenCalled();
+
+      // Restore console.error
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('should reset activeJwtToken and clear cookies on 401/403 responses', async () => {
+      // Mock clearAuthCookie
+      jest.spyOn(cookies, 'clearAuthCookie').mockImplementationOnce(jest.fn());
+
+      // Setup JWT token with mocked payload
+      const jwtToken = 'test.jwt.token';
+
+      // Mock JWT parsing
+      const mockPayload = { jti: 'test-jti-id', sub: '1234567890' };
+      jest.spyOn(window, 'atob').mockReturnValueOnce(JSON.stringify(mockPayload));
+
+      // First set JWT token
+      mockFetch({ headers: { [JWT_HEADER]: jwtToken, ...headers } });
+      await apiFetcher.get(apiUri);
+
+      // Now trigger a 401 response
+      mockFetch({ status: 401 });
+
+      // Use await expect().rejects for async errors instead of try/catch
+      await expect(apiFetcher.get(apiUri)).rejects.toEqual(new RequestError('Not authorized.', 401));
+
+      // Verify cookies were cleared
+      expect(cookies.clearAuthCookie).toHaveBeenCalledWith(JWT_COOKIE_NAME);
+      expect(cookies.clearAuthCookie).toHaveBeenCalledWith(XSRF_COOKIE);
+
+      // Verify that subsequent requests don't include the JWT header
+      mockFetch({ headers });
+      await apiFetcher.get(apiUri);
+
+      expect(window.fetch).toHaveBeenCalled();
+    });
  });

  describe('send data', () => {
@@ -1,16 +1,43 @@
 import { errorMessages, RequestError } from 'utils/errorUtils';

 import { siteId } from './settings';
-import { getCookie } from './cookies';
+import { getCookie, setAuthCookie, clearAuthCookie } from './cookies';
 import { StaticStore } from './static-store';
 import { BASE_URL, API_BASE } from './constants';

 /** Header name for JWT token */
 export const JWT_HEADER = 'X-JWT';
+/** Cookie name for JWT token when using AUTH_SEND_JWT_HEADER */
+export const JWT_COOKIE_NAME = 'JWT';
 /** Header name for XSRF token */
 export const XSRF_HEADER = 'X-XSRF-TOKEN';
 /** Cookie field with XSRF token */
 export const XSRF_COOKIE = 'XSRF-TOKEN';
+/**
+ * Cookie TTL in seconds - matches backend's auth.ttl.cookie default of 200 hours
+ * The JWT token itself expires in 5 minutes, but the cookie persists longer
+ * to match server-side behavior when not using AUTH_SEND_JWT_HEADER
+ */
+export const AUTH_COOKIE_TTL_SECONDS = 200 * 60 * 60;
+
+/**
+ * Safely parses JWT payload with proper base64url handling
+ * @param token - JWT token string
+ * @returns parsed payload or null if parsing fails
+ */
+function parseJwtPayload(token: string): Record<string, unknown> | null {
+  try {
+    const base64Url = token.split('.')[1];
+    if (!base64Url) return null;
+
+    const base64 = base64Url.replace(/-/g, '+').replace(/_/g, '/');
+    const rawPayload = window.atob(base64);
+    return JSON.parse(rawPayload);
+  } catch (e) {
+    console.error('Failed to parse JWT payload', e);
+    return null;
+  }
+}

 type QueryParams = Record<string, string | number | undefined>;
 type Payload = BodyInit | Record<string, unknown> | null;
@@ -74,10 +101,30 @@ const createFetcher = (baseUrl: string = ''): Methods => {
      // backend could update jwt in any time. so, we should handle it
      if (res.headers.has(JWT_HEADER)) {
        activeJwtToken = res.headers.get(JWT_HEADER) as string;
+
+        // Store the JWT token in cookies for persistence across page reloads
+        try {
+          const payload = parseJwtPayload(activeJwtToken);
+          if (payload && payload.jti) {
+            // Set XSRF cookie with the JWT ID using enhanced security
+            setAuthCookie(XSRF_COOKIE, payload.jti as string, {
+              expires: AUTH_COOKIE_TTL_SECONDS,
+            });
+
+            // Store the JWT in cookie for persistence with enhanced security
+            setAuthCookie(JWT_COOKIE_NAME, activeJwtToken, {
+              expires: AUTH_COOKIE_TTL_SECONDS,
+            });
+          }
+        } catch (e) {
+          console.error('Failed to process JWT token', e);
+        }
      }

      if ([401, 403].includes(res.status)) {
        activeJwtToken = undefined;
+        clearAuthCookie(JWT_COOKIE_NAME);
+        clearAuthCookie(XSRF_COOKIE);
      }

      if (res.status >= 400) {
@@ -4994,7 +4994,7 @@ packages:
      normalize-path: 3.0.0
      readdirp: 3.6.0
    optionalDependencies:
-      fsevents: 2.3.2
+      fsevents: 2.3.3
    dev: true

  /chrome-trace-event@1.0.3:
@@ -6986,14 +6986,6 @@ packages:
    resolution: {integrity: sha512-OO0pH2lK6a0hZnAdau5ItzHPI6pUlvI7jMVnxUQRtw4owF2wk8lOSabtGDCTP4Ggrg2MbGnWO9X8K1t4+fGMDw==}
    dev: true

-  /fsevents@2.3.2:
-    resolution: {integrity: sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==}
-    engines: {node: ^8.16.0 || ^10.6.0 || >=11.0.0}
-    os: [darwin]
-    requiresBuild: true
-    dev: true
-    optional: true
-
  /fsevents@2.3.3:
    resolution: {integrity: sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw==}
    engines: {node: ^8.16.0 || ^10.6.0 || >=11.0.0}
@@ -8225,7 +8217,7 @@ packages:
      micromatch: 4.0.5
      walker: 1.0.8
    optionalDependencies:
-      fsevents: 2.3.2
+      fsevents: 2.3.3
    dev: true

  /jest-haste-map@28.1.3:
@@ -8244,7 +8236,7 @@ packages:
      micromatch: 4.0.5
      walker: 1.0.8
    optionalDependencies:
-      fsevents: 2.3.2
+      fsevents: 2.3.3
    dev: true

  /jest-leak-detector@28.1.3:
@@ -79,7 +79,7 @@ services:
 | image.resize-height            | IMAGE_RESIZE_HEIGHT            | `900`                   | height of a resized image                                |
 | auth.ttl.jwt                   | AUTH_TTL_JWT                   | `5m`                    | JWT TTL                                                  |
 | auth.ttl.cookie                | AUTH_TTL_COOKIE                | `200h`                  | cookie TTL                                               |
-| auth.send-jwt-header           | AUTH_SEND_JWT_HEADER           | `false`                 | send JWT as a header instead of a cookie                 |
+| auth.send-jwt-header           | AUTH_SEND_JWT_HEADER           | `false`                 | send JWT as a header instead of a server-set cookie; with this enabled, frontend stores the JWT in a client-side cookie. [See security considerations](#security-considerations-for-auth.send-jwt-header). |
 | auth.same-site                 | AUTH_SAME_SITE                 | `default`               | set same site policy for cookies (`default`, `none`, `lax` or `strict`) |
 | auth.apple.cid                 | AUTH_APPLE_CID                 |                         | Apple client ID (App ID or Services ID)                  |
 | auth.apple.tid                 | AUTH_APPLE_TID                 |                         | Apple service ID                                         |
@@ -169,6 +169,23 @@ services:
 - _multi_ parameters separated by `,` in the environment or repeated with command-line keys, like `--site=s1 --site=s2 ...`
 - _required_ parameters have to be presented in the environment or provided in the command-line

+### Security Considerations for auth.send-jwt-header
+
+When `auth.send-jwt-header=true` is enabled:
+
+- **Security Impact**: JWT tokens are stored in client-accessible cookies that can be accessed by JavaScript
+- **Vulnerability**: This increases vulnerability to XSS attacks compared to server-set HttpOnly cookies
+- **Implementation Mitigations**:
+  - SameSite=Strict cookies to prevent CSRF attacks
+  - Secure flag automatically added on HTTPS connections
+  - __Host- prefix added on HTTPS to prevent subdomain attacks
+  - Double Submit Cookie pattern with XSRF token matching the JWT ID
+
+This configuration should only be used when:
+1. You need cross-domain authentication support
+2. You understand and accept the increased XSS risk
+3. You have implemented strong XSS protections on your site
+
 ### Deprecated parameters

 The following list of command-line options is deprecated and might be removed in the next major release after the version they were deprecated. After the Remark42 version update, please check the startup log once for deprecation warning messages to avoid trouble with unrecognized command-line options in the future.