fix(core): revoke active JWT tokens after logout

[Sourav-kashyap]

GH-2570

JWT Logout Security Fix

Problem Statement

Security Vulnerability: JWT Tokens Remain Valid After Logout

Issue: When a user logs out of the system, their JWT access token remains valid until its natural expiration time, allowing the token to be used for authenticated requests even after logout.

Impact: This creates a security vulnerability where:

1. Logged-out users retain access to protected APIs until their JWT naturally expires
2. Session invalidation through logout is ineffective
3. In case of security incidents, compromised tokens cannot be immediately revoked
4. Access token lifetime becomes the effective session lifetime, regardless of logout

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• Performed a self-review of my own code
• npm test passes on your machine

Build:

Test:

[samarpan-b]

where are we revoking this token actually ?

[Sourav-kashyap]

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.

[rohit-sourcefuse]

Thanks for the explanation — agreed that the set() side is the existing logout code and this PR adds the missing get() check on the service verifiers. Two things worth resolving before this goes in, both raised inline:

1. The RevokedTokenRepository injection on these service verifiers is required, which forces an AuthCache datasource onto every type: service consumer — that's a backward-incompatible change (the reason the two test-helpers needed patching in this same PR).
2. The check itself fails open on any store error, so during an AuthCache/Redis outage revoked tokens are accepted — which is the opposite of what a revocation control should do, and inconsistent with the auth-service verifier that fails closed.

[rohit-sourcefuse]

This is a fail-open security control. Any error from revokedTokenRepo.get() (AuthCache/Redis down, timeout, connection-pool exhaustion, failover) is caught, logged, and verification continues — so a token the user explicitly logged out is accepted for the entire duration of a cache incident. That defeats the purpose of the check, and it is inconsistent with the authentication-service verifier (services/authentication-service/src/modules/auth/providers/bearer-token-verify.provider.ts), which does the same revokedTokenRepository.get() lookup with no try/catch and lets the error propagate — i.e. it fails closed.

That auth-service provider is the established ARC precedent, and it's the simpler pattern: no catch, no discriminator. The current util instead throws the intentional TokenRevoked inside the try and then relies on HttpErrors.HttpError.prototype.isPrototypeOf(error) to re-surface it. That couples "token is revoked" and "cache call failed" into one catch whose default branch is accept; any misclassification (a non-object throw, a wrapped error, or a duplicate http-errors copy in the monorepo giving a different HttpError.prototype identity) silently turns a hard reject into a pass.

Suggestion: match the auth-service verifier — do the revoked check without wrapping the store call in a catch, so a store error propagates and the request is denied:

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);
  }
}

A revocation control should fail closed: if we can't prove a token is not revoked, reject it. This also removes the throw-inside-try / isPrototypeOf discrimination entirely, so there's no path where a revoked token leaks through a classification miss. The two facade verifiers (facades-bearer-token-verify.provider.ts, and the private _checkIfTokenRevoked in facades-bearer-asym-token-verify.provider.ts) still carry the same fail-open logic un-refactored — whichever posture we land on should be applied consistently, ideally by routing all of them through this shared util.

If fail-open is a deliberate, product-approved availability trade-off (this is packages/core, so fail-closed does mean an AuthCache outage takes down authenticated traffic), it should be an explicit, default-off, documented flag — not the silent default.

[Sourav-kashyap]

resolved

[rohit-sourcefuse]

@repository(RevokedTokenRepository) here is a required injection, and that repository hard-requires the AuthCache datasource (@inject('datasources.AuthCache')). BearerVerifierComponent selects these two providers for every type: BearerVerifierType.service consumer. Before this PR the service path never injected this repo, so the AuthCache datasource was never resolved on that path; now it is resolved when the provider is constructed, so any consumer without an AuthCache datasource breaks with a ResolutionError on every authenticated request. The test-helper edits in this PR (task-service, user-tenant-service) are that same breakage being patched for two in-repo apps.

One correction to flag on the fix: making the injection {optional: true} will NOT prevent this. BearerVerifierComponent already binds RevokedTokenRepository unconditionally (component.ts this.repositories = [RevokedTokenRepository, ...]), so the repositories.RevokedTokenRepository binding exists in every consumer. optional only yields undefined when the binding key is not found — here it is found, and resolving it then fails one level deeper on the missing datasources.AuthCache. That ResolutionError propagates regardless of optional.

So this is effectively a new required datasource dependency for all service-type consumers. Two honest options:

1. Treat it as a documented breaking change (major version bump + migration note): consumers must add an AuthCache datasource. This mirrors the facade path, which already requires it. The scheduler-example sandbox datasource is the template — connector: 'kv-redis' reading datasources.config.AuthCache.

2. If backward compatibility must be preserved, the optionality has to live at the datasource level, not the repository level — e.g. inject @inject('datasources.AuthCache', {optional: true}) and skip the revocation check when it's absent. That keeps existing consumers booting, at the cost of silently no-op'ing revocation where the cache isn't configured (which should then be logged/documented).

Same applies to services-bearer-asym-token-verifier.ts.

[rohit-sourcefuse]

datasources.AuthCache is bound to connector: 'memory', but RevokedTokenRepository is a DefaultKeyValueRepository and needs a KV connector (kv-memory). Note this hard binding also overrides the datasources.config.AuthCache → kv-memory config just below it, since the repo injects datasources.AuthCache directly. Against the plain memory connector the KV .get() won't behave as a key-value lookup, and any error it raises is swallowed by the fail-open catch in checkIfTokenRevoked — so this test can never prove a revoked token is rejected. The same applies to the user-tenant-service test datasource (AuthenticationCacheDbDataSource is also connector: 'memory').

This is why CI stays green: revocation is effectively a no-op in the test apps, so nothing here would fail if the revocation logic broke.

Suggestion: bind datasources.AuthCache to connector: 'kv-memory' (the connector already used in datasources.config.AuthCache here, and the one auth-service and the other sandbox tests use), then add a test that logs a token out and asserts the next request with that token is rejected with TokenRevoked.

[Sourav-kashyap]

resolved

[rohit-sourcefuse]

Right idea and a real gap closed — the service-level verifiers were missing the revoked-token check that the facade and auth-service verifiers already have, and extracting it into a shared util is the correct shape. A few things need addressing before merge (details inline).

Must-fix before merge:

1. The new @repository(RevokedTokenRepository) injection on both service verifiers turns the AuthCache datasource into a hard runtime dependency for every type: service consumer. BearerVerifierComponent wires these providers for all such consumers, so any service without an AuthCache datasource now breaks with a ResolutionError on every authenticated request. The two test-helper edits in this PR are that same breakage being patched in-repo. Note {optional: true} on the repository injection does not fix this — the component already binds RevokedTokenRepository, so the binding exists and resolution fails one level deeper on the missing datasource, which optional doesn't catch. Either ship this as a documented breaking change (major bump + migration note to add an AuthCache datasource, per the scheduler-example sandbox: connector: 'kv-redis'), or, if backward compat is required, make the datasource injection optional and skip the check when it's absent.

2. The revocation check fails open: any store error is swallowed and verification continues, so revoked (logged-out) tokens are accepted during any AuthCache/Redis outage. That undermines the security goal of the PR and is inconsistent with the auth-service verifier, which fails closed (no catch, lets the error propagate). Please make the posture a conscious, documented decision and keep it consistent across the core verifiers — the two facade verifiers still carry the same fail-open logic un-refactored.

3. No test exercises any of checkIfTokenRevoked's branches, and neither modified verifier has a test asserting a revoked token is rejected. The existing repositories.revoked-token.repository.unit.ts tests a different method (setIfNotExists) with tautological assertions, so it isn't coverage for this change. The test datasources also bind AuthCache to a plain memory connector (not kv-memory), so the KV lookup is effectively a no-op there — combined with the fail-open catch, that's why CI stays green without validating revocation. Please add:

   • revoked record returned → rejects with Unauthorized/TokenRevoked
   • store throws (Redis down) → asserts the chosen fail posture
   • not-revoked → passes and a valid token still authenticates
   • per provider: a revoked token stubbed in the store is rejected before JWT verification runs

Nice-to-have later:

• Route the two facade verifiers through the shared util so the revoke logic can't diverge (the facade asym provider still has its own private _checkIfTokenRevoked).
• error instanceof HttpErrors.HttpError over isPrototypeOf (or drop it entirely with the fail-closed rewrite).
• Switch the task-service test datasource to a kv-memory connector so it actually covers the revocation path.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud