Fix TOTP validation ignoring the configured tolerance window (#23632)

## Problem Entering a 2FA code right after it rotates fails with "Invalid OTP". A tolerance window was configured (`TOTP_DEFAULT_CONFIGURATION.window`) but never applied: `TotpStrategy` validated its options with Zod and then discarded them, and `validate()` called the global otplib `authenticator`, which defaults to `window: 0`. Only the current 30-second code was ever accepted, and clock drift between the device and the server made it feel even stricter. ## Changes - `TotpStrategy` now clones the otplib authenticator with the validated options (window, step, digits, algorithm, encoding, epoch) and uses that instance for both `initiate` and `validate`, so the configured tolerance actually applies. - Default window set to 1: the previous and next codes are accepted alongside the current one, so a code stays valid for up to 30 extra seconds after rotation and forward clock skew is tolerated. This follows the RFC 6238 recommendation of one time step, kept deliberately tight since `getAuthTokensFromOTP` has no rate limiting beyond the optional captcha guard. - Replaced the placeholder strategy tests with deterministic assertions using fixed epochs: previous and next tokens accepted within the window, a token two steps old rejected, and no-window strategies still reject the previous token. ## Testing - All 2FA module unit tests pass (105), including 19 for the strategy. - `lint:diff-with-main` and `typecheck` pass for twenty-server. --- _Generated by [Claude Code](https://claude.ai/code/session_01Va3ESUmkp14Wu65sAXkL7k)_  <a href="https://cubic.dev/pr/twentyhq/twenty/pull/23632?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
2026-07-31 14:11:06 +02:00
parent fec266a5ae
commit 57ecab8571
6 changed files with 348 additions and 63 deletions
@@ -21,7 +21,7 @@ export const TOTP_DEFAULT_CONFIGURATION = {
  algorithm: TOTPHashAlgorithms.SHA1,
  digits: 6,
  encodings: TOTPKeyEncodings.HEX, // Keep as hex - this is correct for @otplib/core
-  window: 3,
+  window: 1,
  step: 30,
 };

@@ -2,6 +2,7 @@ import { authenticator } from 'otplib';

 import { type PlaintextString } from 'src/engine/core-modules/secret-encryption/branded-strings/plaintext-string.type';
 import { OTPStatus } from 'src/engine/core-modules/two-factor-authentication/strategies/otp/otp.constants';
+import { TwoFactorAuthenticationException } from 'src/engine/core-modules/two-factor-authentication/two-factor-authentication.exception';

 import { TotpStrategy } from './totp.strategy';

@@ -10,23 +11,21 @@ import {
  type TotpContext,
 } from './constants/totp.strategy.constants';

-const RESYNCH_WINDOW = 3;
+const FIXED_EPOCH_MS = 1_700_000_000_000;
+const STEP_DURATION_MS = 30 * 1000;
+
+const generateTokenAtEpoch = (secret: string, epochMs: number): string =>
+  authenticator.clone({ epoch: epochMs }).generate(secret);

 describe('TOTPStrategy Configuration', () => {
  let strategy: TotpStrategy;
  let secret: string;
  let context: TotpContext;
-  let warnSpy: jest.SpyInstance;

  beforeEach(() => {
-    warnSpy = jest.spyOn(console, 'warn').mockImplementation();
    secret = authenticator.generateSecret();
  });

-  afterEach(() => {
-    warnSpy.mockRestore();
-  });
-
  describe('Valid Configurations', () => {
    it('should create a strategy with default options', () => {
      expect(() => new TotpStrategy()).not.toThrow();
@@ -43,27 +42,22 @@ describe('TOTPStrategy Configuration', () => {
      expect(() => new TotpStrategy(validOptions)).not.toThrow();
    });

-    it('should warn when all custom options are valid but not recommended', () => {
-      // Since we simplified the implementation, this test no longer applies
-      // as we don't have custom configuration warnings
+    it('should accept a large window', () => {
      expect(() => new TotpStrategy({ window: 10 })).not.toThrow();
-      // Remove the warning expectation since our simplified implementation doesn't warn
+    });
+  });
+
+  describe('Invalid Configurations', () => {
+    it('should throw for a negative window', () => {
+      expect(() => new TotpStrategy({ window: -1 })).toThrow(
+        TwoFactorAuthenticationException,
+      );
    });

-    it('should correctly set the window property', () => {
-      // Since we simplified the implementation to use otplib defaults,
-      // we can't directly access internal configuration
-      const strategy = new TotpStrategy({ window: 10 });
-
-      expect(strategy).toBeDefined();
-    });
-
-    it('should default window to 0 if not provided', () => {
-      // Since we simplified the implementation to use otplib defaults,
-      // we can't directly access internal configuration
-      const strategy = new TotpStrategy();
-
-      expect(strategy).toBeDefined();
+    it('should throw for digits below the minimum', () => {
+      expect(() => new TotpStrategy({ digits: 4 })).toThrow(
+        TwoFactorAuthenticationException,
+      );
    });
  });

@@ -92,9 +86,7 @@ describe('TOTPStrategy Configuration', () => {

  describe('validate', () => {
    beforeEach(() => {
-      strategy = new TotpStrategy({
-        window: RESYNCH_WINDOW,
-      });
+      strategy = new TotpStrategy({ window: 1, epoch: FIXED_EPOCH_MS });

      context = {
        status: OTPStatus.VERIFIED,
@@ -103,12 +95,9 @@ describe('TOTPStrategy Configuration', () => {
    });

    it('should return true for a valid token at the current counter', () => {
-      // Use the initiate method to generate a proper secret
-      const initResult = strategy.initiate('test@example.com', 'TestApp');
-      // Use authenticator.generate to match what authenticator.check expects
-      const token = authenticator.generate(initResult.context.secret);
+      const token = generateTokenAtEpoch(secret, FIXED_EPOCH_MS);

-      const result = strategy.validate(token, initResult.context);
+      const result = strategy.validate(token, context);

      expect(result.isValid).toBe(true);
    });
@@ -120,23 +109,49 @@ describe('TOTPStrategy Configuration', () => {
      expect(result.isValid).toBe(false);
    });

-    it('should succeed if the token is valid within the window', () => {
-      // Use the initiate method to generate a proper secret
-      const initResult = strategy.initiate('test@example.com', 'TestApp');
-      // Use authenticator.generate to match what authenticator.check expects
-      const futureToken = authenticator.generate(initResult.context.secret);
+    it('should accept the previous token within the window', () => {
+      const previousToken = generateTokenAtEpoch(
+        secret,
+        FIXED_EPOCH_MS - STEP_DURATION_MS,
+      );

-      const result = strategy.validate(futureToken, initResult.context);
+      const result = strategy.validate(previousToken, context);

      expect(result.isValid).toBe(true);
    });

-    it('should fail if the token is valid but outside the window', () => {
-      // For this test, we'll use a completely invalid token since we can't easily
-      // generate tokens outside the window with the simplified implementation
-      const invalidToken = '000000';
+    it('should accept the next token within the window', () => {
+      const nextToken = generateTokenAtEpoch(
+        secret,
+        FIXED_EPOCH_MS + STEP_DURATION_MS,
+      );

-      const result = strategy.validate(invalidToken, context);
+      const result = strategy.validate(nextToken, context);
+
+      expect(result.isValid).toBe(true);
+    });
+
+    it('should reject a token generated outside the window', () => {
+      const staleToken = generateTokenAtEpoch(
+        secret,
+        FIXED_EPOCH_MS - 2 * STEP_DURATION_MS,
+      );
+
+      const result = strategy.validate(staleToken, context);
+
+      expect(result.isValid).toBe(false);
+    });
+
+    it('should reject the previous token when no window is configured', () => {
+      const strategyWithoutWindow = new TotpStrategy({
+        epoch: FIXED_EPOCH_MS,
+      });
+      const previousToken = generateTokenAtEpoch(
+        secret,
+        FIXED_EPOCH_MS - STEP_DURATION_MS,
+      );
+
+      const result = strategyWithoutWindow.validate(previousToken, context);

      expect(result.isValid).toBe(false);
    });
@@ -147,8 +162,6 @@ describe('TOTPStrategy Configuration', () => {
        secret: 'invalid-secret' as PlaintextString,
      };

-      // The authenticator.check method doesn't throw for invalid secrets,
-      // it just returns false
      const result = strategy.validate('123456', invalidContext);

      expect(result.isValid).toBe(false);
@@ -160,18 +173,17 @@ describe('TOTPStrategy Configuration', () => {
        secret: '' as PlaintextString,
      };

-      // The authenticator.check method doesn't throw for empty secrets,
-      // it just returns false
      const result = strategy.validate('123456', invalidContext);

      expect(result.isValid).toBe(false);
    });

    it('should return the original context on validation success', () => {
-      // Use the initiate method to generate a proper secret
      const initResult = strategy.initiate('test@example.com', 'TestApp');
-      // Use authenticator.generate to match what authenticator.check expects
-      const token = authenticator.generate(initResult.context.secret);
+      const token = generateTokenAtEpoch(
+        initResult.context.secret,
+        FIXED_EPOCH_MS,
+      );

      const result = strategy.validate(token, initResult.context);

@@ -194,24 +206,27 @@ describe('TOTPStrategy Configuration', () => {
    });

    it('should handle empty token gracefully', () => {
-      const context = {
+      const errorHandlingContext = {
        status: OTPStatus.VERIFIED,
        secret: secret as PlaintextString,
      };

-      const result = strategy.validate('', context);
+      const result = strategy.validate('', errorHandlingContext);

      expect(result.isValid).toBe(false);
      expect(result.context.status).toBe(OTPStatus.VERIFIED);
    });

    it('should handle null token gracefully', () => {
-      const context = {
+      const errorHandlingContext = {
        status: OTPStatus.VERIFIED,
        secret: secret as PlaintextString,
      };

-      const result = strategy.validate(null as any, context);
+      const result = strategy.validate(
+        null as unknown as string,
+        errorHandlingContext,
+      );

      expect(result.isValid).toBe(false);
      expect(result.context.status).toBe(OTPStatus.VERIFIED);
@@ -3,7 +3,6 @@ import { Injectable } from '@nestjs/common';
 import { authenticator } from 'otplib';
 import { TwoFactorAuthenticationStrategy } from 'twenty-shared/types';
 import { isDefined } from 'twenty-shared/utils';
-import { type ZodSafeParseResult } from 'zod';

 import { type OTPAuthenticationStrategyInterface } from 'src/engine/core-modules/two-factor-authentication/strategies/otp/interfaces/otp.strategy.interface';
 import { type PlaintextString } from 'src/engine/core-modules/secret-encryption/branded-strings/plaintext-string.type';
@@ -20,15 +19,22 @@ import {
  TOTPStrategyConfig,
 } from './constants/totp.strategy.constants';

+type TotpAuthenticator = typeof authenticator;
+type TotpAuthenticatorOptions = NonNullable<
+  Parameters<TotpAuthenticator['clone']>[0]
+>;
+
@Injectable()
 export class TotpStrategy implements OTPAuthenticationStrategyInterface {
  public readonly name = TwoFactorAuthenticationStrategy.TOTP;

+  private readonly totpAuthenticator: TotpAuthenticator;
+
  constructor(options?: TOTPStrategyConfig) {
-    let result: ZodSafeParseResult<TOTPStrategyConfig> | undefined;
+    let validatedOptions: TOTPStrategyConfig | undefined;

    if (isDefined(options)) {
-      result = TOTP_STRATEGY_CONFIG_SCHEMA.safeParse(options);
+      const result = TOTP_STRATEGY_CONFIG_SCHEMA.safeParse(options);

      if (!result.success) {
        const errorMessages = Object.entries(result.error.flatten().fieldErrors)
@@ -43,9 +49,51 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface {
          TwoFactorAuthenticationExceptionCode.INVALID_CONFIGURATION,
        );
      }
+
+      validatedOptions = result.data;
    }

-    // otplib will use its defaults: sha1, 6 digits, 30 second step, etc.
+    this.totpAuthenticator = authenticator.clone(
+      this.buildAuthenticatorOptions(validatedOptions),
+    );
+  }
+
+  private buildAuthenticatorOptions(
+    config?: TOTPStrategyConfig,
+  ): TotpAuthenticatorOptions {
+    const authenticatorOptions: TotpAuthenticatorOptions = {};
+
+    if (!isDefined(config)) {
+      return authenticatorOptions;
+    }
+
+    if (isDefined(config.algorithm)) {
+      authenticatorOptions.algorithm =
+        config.algorithm as unknown as TotpAuthenticatorOptions['algorithm'];
+    }
+
+    if (isDefined(config.encodings)) {
+      authenticatorOptions.encoding =
+        config.encodings as unknown as TotpAuthenticatorOptions['encoding'];
+    }
+
+    if (isDefined(config.digits)) {
+      authenticatorOptions.digits = config.digits;
+    }
+
+    if (isDefined(config.window)) {
+      authenticatorOptions.window = config.window;
+    }
+
+    if (isDefined(config.step)) {
+      authenticatorOptions.step = config.step;
+    }
+
+    if (isDefined(config.epoch)) {
+      authenticatorOptions.epoch = config.epoch;
+    }
+
+    return authenticatorOptions;
  }

  public initiate(
@@ -55,8 +103,8 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface {
    uri: string;
    context: TotpContext;
  } {
-    const secret = authenticator.generateSecret() as PlaintextString;
-    const uri = authenticator.keyuri(accountName, issuer, secret);
+    const secret = this.totpAuthenticator.generateSecret() as PlaintextString;
+    const uri = this.totpAuthenticator.keyuri(accountName, issuer, secret);

    return {
      uri,
@@ -74,7 +122,7 @@ export class TotpStrategy implements OTPAuthenticationStrategyInterface {
    isValid: boolean;
    context: TotpContext;
  } {
-    const isValid = authenticator.check(token, context.secret);
+    const isValid = this.totpAuthenticator.check(token, context.secret);

    return {
      isValid,
@@ -0,0 +1,117 @@
+import { authenticator } from 'otplib';
+import { initiateOtpProvisioningForAuthenticatedUser } from 'test/integration/graphql/utils/initiate-otp-provisioning-for-authenticated-user.util';
+import { verifyTwoFactorAuthenticationMethod } from 'test/integration/graphql/utils/verify-two-factor-authentication-method.util';
+
+import { USER_WORKSPACE_DATA_SEED_IDS } from 'src/engine/workspace-manager/dev-seeder/core/utils/seed-user-workspaces.util';
+
+const TOTP_STEP_DURATION_MS = 30_000;
+const MINIMUM_STEP_REMAINING_MS = 5_000;
+
+const generateTokenAtEpoch = (secret: string, epochMs: number): string =>
+  authenticator.clone({ epoch: epochMs }).generate(secret);
+
+// A previous-step token generated right before a step boundary would already be
+// two steps old once the server validates it, so wait out the boundary first
+const waitUntilSafelyInsideTotpStep = async (): Promise<void> => {
+  const millisecondsRemainingInStep =
+    TOTP_STEP_DURATION_MS - (Date.now() % TOTP_STEP_DURATION_MS);
+
+  if (millisecondsRemainingInStep < MINIMUM_STEP_REMAINING_MS) {
+    await new Promise((resolve) =>
+      setTimeout(resolve, millisecondsRemainingInStep + 100),
+    );
+  }
+};
+
+const deleteJonyTwoFactorAuthenticationMethods = async (): Promise<void> => {
+  await global.testDataSource.query(
+    `DELETE FROM core."twoFactorAuthenticationMethod" WHERE "userWorkspaceId" = $1`,
+    [USER_WORKSPACE_DATA_SEED_IDS.JONY],
+  );
+};
+
+describe('Two-factor authentication TOTP verification (integration)', () => {
+  let secret: string;
+
+  beforeAll(async () => {
+    await deleteJonyTwoFactorAuthenticationMethods();
+
+    const { data, errors } = await initiateOtpProvisioningForAuthenticatedUser({
+      accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN,
+      expectToFail: false,
+    });
+
+    expect(errors).toBeUndefined();
+
+    const uri = data.initiateOTPProvisioningForAuthenticatedUser.uri;
+
+    expect(uri).toMatch(/^otpauth:\/\/totp\//);
+
+    const secretFromUri = uri.match(/[?&]secret=([^&]+)/)?.[1];
+
+    if (secretFromUri === undefined || secretFromUri === '') {
+      throw new Error('Expected the otpauth URI to contain a secret');
+    }
+
+    secret = secretFromUri;
+  });
+
+  afterAll(async () => {
+    await deleteJonyTwoFactorAuthenticationMethods();
+  });
+
+  it('should reject a code that is more than one step old', async () => {
+    const staleToken = generateTokenAtEpoch(
+      secret,
+      Date.now() - 2 * TOTP_STEP_DURATION_MS,
+    );
+
+    const { data, errors } = await verifyTwoFactorAuthenticationMethod({
+      otp: staleToken,
+      accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN,
+      expectToFail: true,
+    });
+
+    expect(errors).toBeDefined();
+    expect(
+      data?.verifyTwoFactorAuthenticationMethodForAuthenticatedUser,
+    ).toBeFalsy();
+  });
+
+  it('should accept the current code', async () => {
+    await waitUntilSafelyInsideTotpStep();
+
+    const currentToken = generateTokenAtEpoch(secret, Date.now());
+
+    const { data, errors } = await verifyTwoFactorAuthenticationMethod({
+      otp: currentToken,
+      accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN,
+      expectToFail: false,
+    });
+
+    expect(errors).toBeUndefined();
+    expect(
+      data.verifyTwoFactorAuthenticationMethodForAuthenticatedUser.success,
+    ).toBe(true);
+  });
+
+  it('should accept the code from the previous step', async () => {
+    await waitUntilSafelyInsideTotpStep();
+
+    const previousStepToken = generateTokenAtEpoch(
+      secret,
+      Date.now() - TOTP_STEP_DURATION_MS,
+    );
+
+    const { data, errors } = await verifyTwoFactorAuthenticationMethod({
+      otp: previousStepToken,
+      accessToken: APPLE_JONY_MEMBER_ACCESS_TOKEN,
+      expectToFail: false,
+    });
+
+    expect(errors).toBeUndefined();
+    expect(
+      data.verifyTwoFactorAuthenticationMethodForAuthenticatedUser.success,
+    ).toBe(true);
+  });
+});
@@ -0,0 +1,50 @@
+import gql from 'graphql-tag';
+import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util';
+import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type';
+import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util';
+import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util';
+
+import { type InitiateTwoFactorAuthenticationProvisioningDTO } from 'src/engine/core-modules/two-factor-authentication/dto/initiate-two-factor-authentication-provisioning.dto';
+
+type InitiateOtpProvisioningForAuthenticatedUserUtilArgs = {
+  accessToken: string;
+  expectToFail?: boolean;
+};
+
+export const initiateOtpProvisioningForAuthenticatedUser = async ({
+  accessToken,
+  expectToFail,
+}: InitiateOtpProvisioningForAuthenticatedUserUtilArgs): CommonResponseBody<{
+  initiateOTPProvisioningForAuthenticatedUser: InitiateTwoFactorAuthenticationProvisioningDTO;
+}> => {
+  const mutation = gql`
+    mutation InitiateOTPProvisioningForAuthenticatedUser {
+      initiateOTPProvisioningForAuthenticatedUser {
+        uri
+      }
+    }
+  `;
+
+  const response = await makeMetadataAPIRequest(
+    {
+      query: mutation,
+    },
+    accessToken,
+  );
+
+  if (expectToFail === true) {
+    warnIfNoErrorButExpectedToFail({
+      response,
+      errorMessage: 'OTP provisioning should have failed but did not',
+    });
+  }
+
+  if (expectToFail === false) {
+    warnIfErrorButNotExpectedToFail({
+      response,
+      errorMessage: 'OTP provisioning has failed but should not',
+    });
+  }
+
+  return { data: response.body.data, errors: response.body.errors };
+};
@@ -0,0 +1,55 @@
+import gql from 'graphql-tag';
+import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util';
+import { type CommonResponseBody } from 'test/integration/metadata/types/common-response-body.type';
+import { warnIfErrorButNotExpectedToFail } from 'test/integration/metadata/utils/warn-if-error-but-not-expected-to-fail.util';
+import { warnIfNoErrorButExpectedToFail } from 'test/integration/metadata/utils/warn-if-no-error-but-expected-to-fail.util';
+
+import { type VerifyTwoFactorAuthenticationMethodDTO } from 'src/engine/core-modules/two-factor-authentication/dto/verify-two-factor-authentication-method.dto';
+
+type VerifyTwoFactorAuthenticationMethodUtilArgs = {
+  otp: string;
+  accessToken: string;
+  expectToFail?: boolean;
+};
+
+export const verifyTwoFactorAuthenticationMethod = async ({
+  otp,
+  accessToken,
+  expectToFail,
+}: VerifyTwoFactorAuthenticationMethodUtilArgs): CommonResponseBody<{
+  verifyTwoFactorAuthenticationMethodForAuthenticatedUser: VerifyTwoFactorAuthenticationMethodDTO;
+}> => {
+  const mutation = gql`
+    mutation VerifyTwoFactorAuthenticationMethodForAuthenticatedUser(
+      $otp: String!
+    ) {
+      verifyTwoFactorAuthenticationMethodForAuthenticatedUser(otp: $otp) {
+        success
+      }
+    }
+  `;
+
+  const response = await makeMetadataAPIRequest(
+    {
+      query: mutation,
+      variables: { otp },
+    },
+    accessToken,
+  );
+
+  if (expectToFail === true) {
+    warnIfNoErrorButExpectedToFail({
+      response,
+      errorMessage: 'OTP verification should have failed but did not',
+    });
+  }
+
+  if (expectToFail === false) {
+    warnIfErrorButNotExpectedToFail({
+      response,
+      errorMessage: 'OTP verification has failed but should not',
+    });
+  }
+
+  return { data: response.body.data, errors: response.body.errors };
+};