-
Notifications
You must be signed in to change notification settings - Fork 78
fix(core): revoke active JWT tokens after logout #2571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| // Copyright (c) 2023 Sourcefuse Technologies | ||
| // Copyright (c) 2023 Sourcefuse Technologies | ||
| // | ||
| // This software is released under the MIT License. | ||
| // https://opensource.org/licenses/MIT | ||
| import {Constructor, inject, Provider} from '@loopback/context'; | ||
| import {repository} from '@loopback/repository'; | ||
| import {HttpErrors} from '@loopback/rest'; | ||
| import {verify} from 'jsonwebtoken'; | ||
| import { | ||
|
|
@@ -12,18 +13,25 @@ import { | |
| VerifyFunction, | ||
| } from 'loopback4-authentication'; | ||
| import moment from 'moment-timezone'; | ||
| import {RevokedTokenRepository} from '../../../repositories'; | ||
| import {ILogger, LOGGER} from '../../logger-extension'; | ||
| import {IAuthUserWithPermissions} from '../keys'; | ||
| import {checkIfTokenRevoked} from './utils/revoked-token-checker.util'; | ||
|
|
||
| export class ServicesBearerTokenVerifyProvider implements Provider<VerifyFunction.BearerFn> { | ||
| constructor( | ||
| @inject(LOGGER.LOGGER_INJECT) public logger: ILogger, | ||
| @repository(RevokedTokenRepository) | ||
| public revokedTokenRepo: RevokedTokenRepository, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One correction to flag on the fix: making the injection So this is effectively a new required datasource dependency for all
Same applies to |
||
| @inject(AuthenticationBindings.USER_MODEL, {optional: true}) | ||
| public authUserModel?: Constructor<EntityWithIdentifier & IAuthUser>, | ||
| ) {} | ||
|
|
||
| value(): VerifyFunction.BearerFn { | ||
| return async (token: string) => { | ||
| // Check if token has been revoked | ||
| await checkIfTokenRevoked(token, this.revokedTokenRepo, this.logger); | ||
|
|
||
| let user: IAuthUserWithPermissions; | ||
|
|
||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // Copyright (c) 2023 Sourcefuse Technologies | ||
| // | ||
| // This software is released under the MIT License. | ||
| // https://opensource.org/licenses/MIT | ||
| import {HttpErrors} from '@loopback/rest'; | ||
| import {RevokedTokenRepository} from '../../../../repositories'; | ||
| import {AuthenticateErrorKeys} from '../../../../enums/auth-error-keys.enum'; | ||
| import {ILogger} from '../../../../components/logger-extension'; | ||
|
|
||
| /** | ||
| * Checks if a token has been revoked and throws an error if it has. | ||
| * | ||
| * This function queries the RevokedTokenRepository to determine if the given token | ||
| * has been revoked. If the token is found in the revoked list, an Unauthorized | ||
| * error is thrown, preventing the use of previously logged-out tokens. | ||
| * | ||
| * **Security posture (fail-closed)**: If the revoked token repository is unavailable | ||
| * (Redis down, timeout, connection errors), the error propagates and the request is | ||
| * denied. This ensures that logout always takes effect - if we cannot verify a token | ||
| * is not revoked, we reject it. This matches the established pattern from | ||
| * authentication-service's bearer-token-verify.provider.ts. | ||
| * | ||
| * @param token - The JWT token to check for revocation | ||
| * @param revokedTokenRepo - The repository to check for revoked tokens | ||
| * @param logger - Logger instance for security logging | ||
| * @throws {HttpErrors.Unauthorized} When the token has been revoked | ||
| * @throws When the revoked token repository is unavailable | ||
| */ | ||
| export async function checkIfTokenRevoked( | ||
| token: string, | ||
| revokedTokenRepo: RevokedTokenRepository, | ||
| logger: ILogger, | ||
| ): Promise<void> { | ||
| const isRevoked = await revokedTokenRepo.get(token); | ||
| if (isRevoked?.token) { | ||
| logger.warn(`[SECURITY] Attempt to use revoked token detected`); | ||
| throw new HttpErrors.Unauthorized(AuthenticateErrorKeys.TokenRevoked); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are we revoking this token actually ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sir the token is revoked using the existing RevokedTokenRepository
on logout (idp-login.service.ts) : token is stored via revokedTokensRepo.set()
on api request (services-bearer-asym-token-verifier.ts) : token is checked via checkIfTokenRevoked() which calls revokedTokenRepo.get(token) and throws TokenRevoked error if found
The same mechanism that the authentication service and facade services already use - we just added the same check to the service-level verifiers which were missing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation — agreed that the
set()side is the existing logout code and this PR adds the missingget()check on the service verifiers. Two things worth resolving before this goes in, both raised inline:RevokedTokenRepositoryinjection on these service verifiers is required, which forces anAuthCachedatasource onto everytype: serviceconsumer — that's a backward-incompatible change (the reason the two test-helpers needed patching in this same PR).