Rename REFRESH_TOKEN_COOL_DOWN to REFRESH_TOKEN_REUSE_GRACE_PERIOD and anchor the grace window (#17782)
## Summary - Renames `REFRESH_TOKEN_COOL_DOWN` to `REFRESH_TOKEN_REUSE_GRACE_PERIOD` — the old name was misleading and suggested a security mechanism rather than what it actually is: a grace period for concurrent refresh token use (e.g. two browser tabs refreshing simultaneously). - Makes the token revocation in `renew-token.service.ts` conditional (`revokedAt: IsNull()`), so if the token was already revoked by a concurrent request, the original `revokedAt` timestamp is preserved and the grace window stays anchored. - Updates comments and config description to clarify intent. ## Test plan - [x] Existing unit tests updated and passing (`refresh-token.service.spec.ts`, `renew-token.service.spec.ts`) - [x] Lint clean Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
+32
-23
@@ -34,7 +34,9 @@ export class RefreshTokenService {
|
||||
) {}
|
||||
|
||||
async verifyRefreshToken(refreshToken: string) {
|
||||
const coolDown = this.twentyConfigService.get('REFRESH_TOKEN_COOL_DOWN');
|
||||
const reuseGracePeriod = this.twentyConfigService.get(
|
||||
'REFRESH_TOKEN_REUSE_GRACE_PERIOD',
|
||||
);
|
||||
|
||||
await this.jwtWrapperService.verifyJwtToken(refreshToken);
|
||||
const jwtPayload =
|
||||
@@ -70,29 +72,36 @@ export class RefreshTokenService {
|
||||
);
|
||||
}
|
||||
|
||||
// Check if revokedAt is less than coolDown
|
||||
if (
|
||||
token.revokedAt &&
|
||||
token.revokedAt.getTime() <= Date.now() - ms(coolDown)
|
||||
) {
|
||||
// Revoke all user refresh tokens
|
||||
await Promise.all(
|
||||
user.appTokens.map(async ({ id, type }) => {
|
||||
if (type === AppTokenType.RefreshToken) {
|
||||
await this.appTokenRepository.update(
|
||||
{ id },
|
||||
{
|
||||
revokedAt: new Date(),
|
||||
},
|
||||
);
|
||||
}
|
||||
}),
|
||||
);
|
||||
if (token.revokedAt) {
|
||||
const wasRevokedBeforeGracePeriod =
|
||||
token.revokedAt.getTime() <= Date.now() - ms(reuseGracePeriod);
|
||||
|
||||
throw new AuthException(
|
||||
'Suspicious activity detected, this refresh token has been revoked. All tokens have been revoked.',
|
||||
AuthExceptionCode.FORBIDDEN_EXCEPTION,
|
||||
);
|
||||
if (wasRevokedBeforeGracePeriod) {
|
||||
// Token was revoked long ago and is being reused -- suspicious.
|
||||
// Revoke all user refresh tokens as a safety measure.
|
||||
await Promise.all(
|
||||
user.appTokens.map(async ({ id, type }) => {
|
||||
if (type === AppTokenType.RefreshToken) {
|
||||
await this.appTokenRepository.update(
|
||||
{ id },
|
||||
{
|
||||
revokedAt: new Date(),
|
||||
},
|
||||
);
|
||||
}
|
||||
}),
|
||||
);
|
||||
|
||||
throw new AuthException(
|
||||
'Suspicious activity detected, this refresh token has been revoked. All tokens have been revoked.',
|
||||
AuthExceptionCode.FORBIDDEN_EXCEPTION,
|
||||
);
|
||||
}
|
||||
|
||||
// Token was revoked recently (within grace period). This is expected
|
||||
// when concurrent requests (e.g. two browser tabs) race to refresh
|
||||
// at the same time. Allow it but don't reset the original revokedAt
|
||||
// timestamp so the grace window stays anchored and cannot be extended.
|
||||
}
|
||||
|
||||
return {
|
||||
|
||||
+2
-2
@@ -1,7 +1,7 @@
|
||||
import { Test, type TestingModule } from '@nestjs/testing';
|
||||
import { getRepositoryToken } from '@nestjs/typeorm';
|
||||
|
||||
import { Repository } from 'typeorm';
|
||||
import { IsNull, Repository } from 'typeorm';
|
||||
|
||||
import { AppTokenEntity } from 'src/engine/core-modules/app-token/app-token.entity';
|
||||
import { AuthException } from 'src/engine/core-modules/auth/auth.exception';
|
||||
@@ -110,7 +110,7 @@ describe('RenewTokenService', () => {
|
||||
mockRefreshToken,
|
||||
);
|
||||
expect(appTokenRepository.update).toHaveBeenCalledWith(
|
||||
{ id: mockTokenId },
|
||||
{ id: mockTokenId, revokedAt: IsNull() },
|
||||
{ revokedAt: expect.any(Date) },
|
||||
);
|
||||
expect(accessTokenService.generateAccessToken).toHaveBeenCalledWith(
|
||||
|
||||
+6
-2
@@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common';
|
||||
import { InjectRepository } from '@nestjs/typeorm';
|
||||
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
import { Repository } from 'typeorm';
|
||||
import { IsNull, Repository } from 'typeorm';
|
||||
|
||||
import { AppTokenEntity } from 'src/engine/core-modules/app-token/app-token.entity';
|
||||
import {
|
||||
@@ -47,10 +47,14 @@ export class RenewTokenService {
|
||||
impersonatedUserWorkspaceId,
|
||||
} = await this.refreshTokenService.verifyRefreshToken(token);
|
||||
|
||||
// Revoke old refresh token
|
||||
// Revoke old refresh token only if not already revoked.
|
||||
// If it was already revoked (concurrent race condition within grace
|
||||
// period), we preserve the original revokedAt timestamp so the grace
|
||||
// window stays anchored and cannot be extended by repeated reuse.
|
||||
await this.appTokenRepository.update(
|
||||
{
|
||||
id,
|
||||
revokedAt: IsNull(),
|
||||
},
|
||||
{
|
||||
revokedAt: new Date(),
|
||||
|
||||
@@ -277,12 +277,13 @@ export class ConfigVariables {
|
||||
|
||||
@ConfigVariablesMetadata({
|
||||
group: ConfigVariablesGroup.TOKENS_DURATION,
|
||||
description: 'Cooldown period for refreshing tokens',
|
||||
description:
|
||||
'Grace period allowing concurrent refresh token use (e.g. two tabs refreshing simultaneously). Reuse after this window triggers suspicious activity detection.',
|
||||
type: ConfigVariableType.STRING,
|
||||
})
|
||||
@IsDuration()
|
||||
@IsOptional()
|
||||
REFRESH_TOKEN_COOL_DOWN = '1m';
|
||||
REFRESH_TOKEN_REUSE_GRACE_PERIOD = '1m';
|
||||
|
||||
@ConfigVariablesMetadata({
|
||||
group: ConfigVariablesGroup.TOKENS_DURATION,
|
||||
|
||||
Reference in New Issue
Block a user