fix(authentication-service): make authorization code one-time-use

[piyushsinghgaur1]

GH-2495

Description

This pull request enhances the security of the authorization code flow by enforcing one-time use of authorization codes and refactors time-to-live (TTL) calculations for tokens and keys to use a single class-level constant. The main improvements are grouped below:

Security Enhancements:

• Authorization codes are now checked for prior use before processing, and are revoked immediately after successful verification to prevent reuse, enforcing one-time-use as required by the OAuth2 specification.
  https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2

Code Consistency and Refactoring:

• Introduced a single msInSecond class property to consistently handle TTL calculations across methods, replacing repeated inline ms = 1000 declarations.
• Updated all TTL settings for refresh tokens, access tokens, and public keys to use the new msInSecond property for improved maintainability and readability. [1] [2] [3] [4]

Code Cleanup:

• Removed redundant local ms variable declarations in several methods, relying on the new class property for TTL calculations. [1] [2] [3]

Before Fix

AuthCode.SrceenRecord.Before.Fix.mov

After Fix

AuthCode.SrceenRecord.After.Fix.mov

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

[Copilot]

Pull request overview

This PR aims to harden the OAuth2 authorization-code token exchange in the authentication service by enforcing one-time-use authorization codes, while also refactoring token/key TTL calculations to use a shared milliseconds-per-second constant. In addition, it updates several sandbox/root dependency overrides and Trivy configuration/workflow files.

Changes:

• Enforce “single redemption” for authorization codes by checking a revocation store and revoking the code after successful verification.
• Refactor TTL computations to use a single msInSecond constant across refresh tokens, access tokens, and key caching.
• Update various package.json/package-lock.json overrides (e.g., undici, underscore, lodash) and adjust Trivy ignore list/workflow action version.

Reviewed changes

Copilot reviewed 13 out of 23 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
services/authentication-service/src/services/idp-login.service.ts	Adds auth-code revocation checks and replaces inline 1000 with a class constant for TTL conversions.
sandbox/telemed-app/backend/video-conferencing-service/package.json	Adds/adjusts dependency overrides for sandbox service.
sandbox/telemed-app/backend/video-conferencing-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/telemed-app/backend/notification-service/package.json	Adds/adjusts dependency overrides for sandbox service.
sandbox/telemed-app/backend/notification-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/telemed-app/backend/authentication-service/package.json	Adds/adjusts dependency overrides for sandbox service.
sandbox/chat-notification-socketio-example/services/notifications-service/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-socketio-example/services/notifications-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/chat-notification-socketio-example/services/chat-service/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-socketio-example/services/chat-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/chat-notification-socketio-example/facade/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-socketio-example/facade/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/chat-notification-pubnub-example/services/notifications-service/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-pubnub-example/services/notifications-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/chat-notification-pubnub-example/services/chat-service/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-pubnub-example/services/chat-service/package-lock.json	Lockfile updates reflecting override/dependency changes.
sandbox/chat-notification-pubnub-example/facade/package.json	Adds/adjusts dependency overrides for sandbox example.
sandbox/chat-notification-pubnub-example/facade/package-lock.json	Lockfile updates reflecting override/dependency changes.
package.json	Updates root-level overrides (incl. fast-xml-parser/handlebars/undici/lodash/axios).
.trivyignore	Adds additional CVEs to the ignore list.
.github/workflows/trivy.yml	Bumps Trivy GitHub Action version and adjusts YAML quoting.

Files not reviewed (9)

• sandbox/chat-notification-pubnub-example/facade/package-lock.json: Language not supported
• sandbox/chat-notification-pubnub-example/services/chat-service/package-lock.json: Language not supported
• sandbox/chat-notification-pubnub-example/services/notifications-service/package-lock.json: Language not supported
• sandbox/chat-notification-socketio-example/facade/package-lock.json: Language not supported
• sandbox/chat-notification-socketio-example/services/chat-service/package-lock.json: Language not supported
• sandbox/chat-notification-socketio-example/services/notifications-service/package-lock.json: Language not supported
• sandbox/telemed-app/backend/authentication-service/package-lock.json: Language not supported
• sandbox/telemed-app/backend/notification-service/package-lock.json: Language not supported
• sandbox/telemed-app/backend/video-conferencing-service/package-lock.json: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The current one-time-use check is not race-safe: two concurrent requests with the same authorization code can both pass the get() check before either set() executes, resulting in the same code being redeemed multiple times. To enforce true single-use, perform an atomic check-and-set (e.g., Redis SET key value NX PX <ttl> via the datasource connector), or persist codes in a store that can mark them as consumed with a conditional update/unique constraint before issuing tokens.

[Copilot]

This PR description focuses on OAuth authorization-code single-use + TTL refactors in the authentication service, but this change also updates root-level overrides (e.g., fast-xml-parser/handlebars/lodash/undici/axios). If these dependency changes are required, please document the motivation (e.g., specific CVEs) or split them into a dedicated dependency/security PR to keep review scope clear.

[samarpanB]

I agree

[samarpanB]

test cases update missing

[samarpanB]

why this added ?

[piyushsinghgaur1]

trivy vulnerability fix

[samarpanB]

I agree

[samarpanB]

why ignored ? this is not good

[piyushsinghgaur1]

Earlier, these CVEs were temporarily added to .trivyignore due to a vulnerability in the tar dependency that was being pulled in via lerna. At that time, overriding the tar version during release was failing due to compatibility issues with the existing lerna version.

I have now upgraded lerna to a version that resolves this dependency issue, and the vulnerable tar version is no longer being used. As a result, these CVEs are no longer applicable and the .trivyignore entries can be safely removed.

Going forward, we will avoid suppressing vulnerabilities unless absolutely necessary and ensure proper remediation instead of long-term ignores.

[rohit-sourcefuse]

The explanation makes sense — lerna upgrade resolves the tar vulnerability, so the CVE suppressions are no longer needed. Good to remove them properly rather than carry them indefinitely.

[samarpanB]

we should have a new key for this.

[piyushsinghgaur1]

Now using auth_code: instead of the raw code to avoid mixing with other revoked tokens and to keep key space isolated.

[rohit-sourcefuse]

The revocation check here fails open. If revokedTokensRepo.get(...) throws — Redis down, network timeout, connection exhaustion — the exception is swallowed by the outer catch block and rethrown as AuthErrorKeys.InvalidCredentials. The revocation check is silently bypassed, the code is treated as unused, and the request proceeds.

Security controls must fail closed. Wrap the get call in its own try-catch:

// Check if the authorization code has already been used (one-time-use enforcement)
const authCodeKeyPrefix = 'revoked:auth_code';
const revokedAuthCodeKey = `${authCodeKeyPrefix}:${resultCode}`;

let isCodeRevoked: RevokedToken | null;
try {
  isCodeRevoked = await this.revokedTokensRepo.get(revokedAuthCodeKey);
} catch (repoError) {
  this.logger.error(
    `[AUTH] Revocation store unavailable — denying auth code exchange for safety. Key: ${revokedAuthCodeKey}. Error: ${(repoError as Error).message}`,
  );
  throw new HttpErrors.ServiceUnavailable(
    'Authentication service temporarily unavailable. Please retry.',
  );
}

if (isCodeRevoked) {
  this.logger.warn(
    `[AUTH][SECURITY] Auth code replay attempt detected. Key: ${revokedAuthCodeKey}, clientId: ${request.clientId}`,
  );
  throw new HttpErrors.Unauthorized(AuthErrorKeys.CodeExpired);
}

This also adds a security log on replay detection — currently nothing is logged when a reuse attempt occurs, which makes replay attack investigation impossible.

[rohit-sourcefuse]

Race condition (TOCTOU) — concurrent replay is not prevented

Two simultaneous requests carrying the same auth code will both call revokedTokensRepo.get(...) and receive null (not revoked), both pass jwtVerifier, and both reach revokedTokensRepo.set(...) — issuing two valid tokens for a single authorization code. This is a realistic attack: an intercepted code replayed in parallel with the legitimate exchange will succeed for both.

The fix requires an atomic set-if-not-exists operation — a Redis SET key value NX EX ttl that marks the code as used and checks whether it was already used in a single step.

Add this method to RevokedTokenRepository:

// In services/authentication-service/src/repositories/revoked-token.repository.ts

/**
 * Atomically marks the key as used if not already present.
 * Returns true if the key was set (first use), false if it already existed (replay).
 * Uses Redis SET NX EX for atomicity.
 */
async setIfNotExists(
  key: string,
  value: RevokedToken,
  options: {ttl: number},
): Promise<boolean> {
  // ttl from LoopBack is in milliseconds; Redis EX is in seconds
  const ttlSeconds = Math.ceil(options.ttl / 1000);
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  const connector = (this.dataSource.connector as any);
  const result = await connector.execute(
    'SET',
    [key, JSON.stringify(value), 'NX', 'EX', ttlSeconds],
  );
  // Redis SET NX returns 'OK' on success, null if key already existed
  return result !== null;
}

Then replace the current get + separate set with a single atomic call in generateToken:

// Replace the isCodeRevoked block AND the later revokedTokensRepo.set block with:
const authCodeKeyPrefix = 'revoked:auth_code';
const revokedAuthCodeKey = `${authCodeKeyPrefix}:${resultCode}`;

let codeWasNew: boolean;
try {
  codeWasNew = await this.revokedTokensRepo.setIfNotExists(
    revokedAuthCodeKey,
    {token: resultCode},
    {ttl: authClient.authCodeExpiration * this.msInSecond},
  );
} catch (repoError) {
  this.logger.error(
    `[AUTH] Revocation store error during auth code exchange. Key: ${revokedAuthCodeKey}. Error: ${(repoError as Error).message}`,
  );
  throw new HttpErrors.ServiceUnavailable(
    'Authentication service temporarily unavailable. Please retry.',
  );
}

if (!codeWasNew) {
  this.logger.warn(
    `[AUTH][SECURITY] Auth code replay attempt. Key: ${revokedAuthCodeKey}, clientId: ${request.clientId}`,
  );
  throw new HttpErrors.Unauthorized(AuthErrorKeys.CodeExpired);
}

// No separate set() call needed — code is already atomically marked above
const payload = (await this.jwtVerifier(resultCode, {
  audience: request.clientId,
})) as ClientAuthCode<User, typeof User.prototype.id>;

Note: moving the atomic mark before jwtVerifier also avoids the window where a JWT verification failure leaves the code permanently revoked without a token ever being issued.

[rohit-sourcefuse]

If revokedTokensRepo.set(...) throws here — after jwtVerifier has already verified the JWT — the outer catch block will rethrow it as AuthErrorKeys.InvalidCredentials (401). The caller gets an auth failure and will retry with the same code. Because the set never succeeded, the code is still not in the revocation store, so the retry passes the revocation check and issues a token.

A transient Redis write failure therefore causes: first attempt → 401, retry → 200 with a token. The auth code has been used without being revoked.

If adopting the atomic setIfNotExists approach above, this call is eliminated entirely (the mark happens atomically before jwtVerifier). Otherwise, wrap this set explicitly:

try {
  await this.revokedTokensRepo.set(
    revokedAuthCodeKey,
    {token: resultCode},
    {ttl: authClient.authCodeExpiration * this.msInSecond},
  );
} catch (setError) {
  this.logger.error(
    `[AUTH] Failed to revoke auth code — cannot safely issue token. Key: ${revokedAuthCodeKey}. Error: ${(setError as Error).message}`,
  );
  // Throw 500, not 401 — a 401 causes the client to retry with the same
  // code, which will succeed because revocation was never written.
  throw new HttpErrors.InternalServerError(
    'Token issuance failed due to an internal error. Please restart the authorization flow.',
  );
}

[rohit-sourcefuse]

authCodeKeyPrefix is a magic string defined inline as a local const. The same revokedTokensRepo is used by logoutUser to revoke access tokens (with no prefix — raw token string as key). There is no central registry of key patterns, so future callers have no way to discover what namespaces exist in this store without scanning every call site.

Extract to a class-level constant:

// Add near msInSecond, at the top of the class body:
private readonly msInSecond = 1000;
private static readonly REVOKED_AUTH_CODE_PREFIX = 'revoked:auth_code' as const;

Use it as:

const revokedAuthCodeKey = `${IdpLoginService.REVOKED_AUTH_CODE_PREFIX}:${resultCode}`;

The revoked: outer namespace also helps distinguish these keys from access-token revocation entries (which use raw token strings with no prefix), reducing the chance of a collision if other code ever stores keys in the same Redis store.

[piyushsinghgaur1]

fixed

[rohit-sourcefuse]

The assertion expect(replayResponse.body.error.message).to.equal(AuthErrorKeys.CodeExpired) may be vacuous. Looking at other 401 assertions in this file (e.g. lines ~214, ~373), some access .body.error.message.message (nested) rather than .body.error.message (flat). If HttpErrors.Unauthorized wraps the error key in a nested object, .body.error.message resolves to an object (not a string), and .to.equal(AuthErrorKeys.CodeExpired) will fail for the wrong reason — or pass vacuously if mocha/chai is not strict about type equality here.

Verify the actual response shape and tighten the assertion:

// Log what you actually get, then assert precisely
console.log('replay error body:', JSON.stringify(replayResponse.body.error));

// Once confirmed, use:
expect(replayResponse.body.error).to.not.be.undefined();
expect(replayResponse.body.error.message).to.be.a('string');
expect(replayResponse.body.error.message).to.equal(AuthErrorKeys.CodeExpired);
// Or, if the structure is nested like other tests in this file:
// expect(replayResponse.body.error.message.message).to.equal(AuthErrorKeys.CodeExpired);

[rohit-sourcefuse]

aquasecurity/trivy-action@0.35.0 is a mutable tag — same concern flagged across other repos in this org. The upstream maintainer can force-push this tag to a different commit at any time.

Also, note that the SHA 57a97c7e7821a5776cebc9bb87c984fa69cba8f1 referenced in earlier PR comments does not resolve — CI confirmed this with the error:

Unable to resolve action aquasecurity/trivy-action@57a97c7e7821a5776cebc9bb87c984fa69cba8f1

To get the correct SHA for v0.35.0:

git ls-remote https://github.com/aquasecurity/trivy-action refs/tags/v0.35.0
# Use the dereferenced commit (the ^{} line), not the tag object SHA

Then pin:

uses: aquasecurity/trivy-action@<correct-sha-here> # v0.35.0

[piyushsinghgaur1]

fixed

[rohit-sourcefuse]

The security goal here is correct and the OAuth2 one-time-use enforcement is a real requirement (RFC 6749 §4.1.2). The msInSecond refactor is also a clean, worthwhile change. That said, the implementation of the core security feature has two critical correctness flaws that defeat the guarantee under realistic conditions, plus a few package-level issues that need attention before this can merge.

---

Must fix before merge

1. Race condition (TOCTOU) — inline comments
Two concurrent requests with the same auth code both pass the revokedTokensRepo.get(...) check before either has written the revocation entry. The fix requires an atomic SET NX EX operation (Redis "set if not exists") on RevokedTokenRepository. Full working code for both the repository method and the generateToken call site is in the inline comment on line 192.

2. Fail-open on Redis error — inline comment on line 178
If revokedTokensRepo.get(...) throws, the outer catch swallows it and the request proceeds as if the code was unused. Security controls must fail closed — wrap the get call separately and throw a 503 if the revocation store is unreachable.

3. revokedTokensRepo.set failure causes retryable code reuse — inline comment on line 192
If the set throws after jwtVerifier succeeds, the outer catch rethrows as a 401. The client retries with the same (never-revoked) code and succeeds. Addressed by the atomic approach above, or by wrapping the set and throwing a 500 explicitly. A 500 signals "restart the flow"; a 401 signals "retry the same request."

4. lodash: "^4.18.0" override breaks npm install — inline comment on line 61 of package.json
Lodash 4.18.0 does not exist; the latest 4.x is 4.17.21. A ^4.18.0 range will fail to resolve. Change to ^4.17.21.

5. package-lock.json — 4,025 integrity hashes removed
This PR strips every integrity SHA-512 and resolved URL from package-lock.json. These are what npm ci uses to verify downloaded tarballs match expected content. Without them, the install pipeline fetches packages without verifying their content — a supply-chain regression that is particularly ironic in a security-hardening PR. This looks like an artifact of regenerating the lockfile with a different npm version. Please revert to a lockfile that includes integrity hashes, or explain in the PR why stripping them is intentional and safe.

---

.trivyignore deletion

The file was suppressing 4 CVEs (CVE-2026-23745, CVE-2026-23950, CVE-2026-24842, CVE-2026-26960). Please add a note in the description confirming which dependency version bump in this PR addresses each CVE — or re-add the entries with inline comments if they are false positives. Without that context, reviewers cannot verify whether Trivy CI will pass or what the actual exposure is.

---

Nice-to-have (follow-up)

• Add unit tests for generateToken with a mocked revokedTokensRepo covering the store-failure path and, once the atomic approach is in place, the concurrent-request scenario
• Extract authCodeKeyPrefix to a private static readonly class constant (inline comment on line 173)
• Verify the error message assertion in the new acceptance test against the actual response body structure (inline comment on line 594)
• Pin trivy-action to an immutable commit SHA (inline comment on .github/workflows/trivy.yml line 25 — note that the SHA previously suggested in this repo's PR comments does not resolve; run git ls-remote to get the correct one for v0.35.0)

[sonarqubecloud]

SonarQube reviewer guide

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud