diff --git a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts index 3e2149e3abff..7debd649db0b 100644 --- a/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts +++ b/apps/services/bff/src/app/modules/auth/auth.controller.spec.ts @@ -2,6 +2,7 @@ import { ConfigType } from '@island.is/nest/config' import { LOGIN_ATTEMPT_FAILED_ACTIVE_SESSION } from '@island.is/shared/constants' import { CACHE_MANAGER } from '@nestjs/cache-manager' import { HttpStatus, INestApplication } from '@nestjs/common' +import type { Request, Response } from 'express' import jwt from 'jsonwebtoken' import request from 'supertest' import { setupTestServer } from '../../../../test/setupTestServer' @@ -13,6 +14,7 @@ import { mockedTokensResponse as tokensResponse, } from '../../../../test/sharedConstants' import { BffConfig } from '../../bff.config' +import { SessionCookieService } from '../../services/sessionCookie.service' import { IdsService } from '../ids/ids.service' import { ParResponse } from '../ids/ids.types' @@ -70,6 +72,19 @@ describe('AuthController', () => { let server: request.SuperTest let mockConfig: ConfigType let baseUrlWithKey: string + let sessionCookieService: SessionCookieService + let hashedSid: string + + // Mock Response object + const mockResponse: Partial = { + cookie: jest.fn(), + clearCookie: jest.fn(), + } + + // Mock Request object + const mockRequest: Partial = { + cookies: {}, + } beforeAll(async () => { const app = await setupTestServer({ @@ -81,9 +96,25 @@ describe('AuthController', () => { .useValue(mockIdsService), }) + sessionCookieService = app.get(SessionCookieService) mockConfig = app.get>(BffConfig.KEY) + baseUrlWithKey = `${mockConfig.clientBaseUrl}${mockConfig.clientBasePath}` + // Set the hashed SID + sessionCookieService.set({ + res: mockResponse as Response, + value: SID_VALUE, + }) + + // Capture the hashed value from the mock cookie call + hashedSid = (mockResponse.cookie as jest.Mock).mock.calls[0][1] + + // Set up the mock request cookies for subsequent get() calls + mockRequest.cookies = { + [SESSION_COOKIE_NAME]: hashedSid, + } + server = request(app.getHttpServer()) }) @@ -311,7 +342,7 @@ describe('AuthController', () => { const loginAttempt = setCacheSpy.mock.calls[0] // Assert - First request should cache the login attempt - expect(setCacheSpy.mock.calls[0]).toContain( + expect(loginAttempt[0]).toContain( `attempt::${mockConfig.name}::${SID_VALUE}`, ) expect(loginAttempt[1]).toMatchObject({ @@ -322,16 +353,15 @@ describe('AuthController', () => { // Then make a callback to the login endpoint const res = await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code, state: SID_VALUE }) const currentLogin = setCacheSpy.mock.calls[1] // Assert expect(setCacheSpy).toHaveBeenCalled() - - expect(currentLogin[0]).toContain( - `current::${mockConfig.name}::${SID_VALUE}`, + expect(currentLogin[0]).toBe( + `current::${mockConfig.name}::${hashedSid}`, ) // Check if the cache contains the correct values for the current login expect(currentLogin[1]).toMatchObject(tokensResponse) @@ -357,7 +387,7 @@ describe('AuthController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server.get('/logout') @@ -410,13 +440,13 @@ describe('AuthController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .get('/logout') .query({ sid: SID_VALUE }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(revokeRefreshTokenSpy).toHaveBeenCalled() @@ -445,7 +475,7 @@ describe('AuthController', () => { it('should throw 400 if logout_token is missing from body', async () => { // Act - const res = await await server.post('/callbacks/logout') + const res = await server.post('/callbacks/logout') // Assert expect(res.status).toEqual(HttpStatus.BAD_REQUEST) @@ -514,7 +544,7 @@ describe('AuthController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server @@ -545,7 +575,7 @@ describe('AuthController', () => { // First login - create initial session with attempt data await server.get('/login') - const currentKey = `current::${mockConfig.name}::${SID_VALUE}` + const currentKey = `current::${mockConfig.name}::${hashedSid}` mockCacheManagerValue.set(currentKey, tokensResponse) @@ -563,7 +593,7 @@ describe('AuthController', () => { // Act - Try to complete second login const res = await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code, state: SID_VALUE }) // Assert diff --git a/apps/services/bff/src/app/modules/auth/auth.module.ts b/apps/services/bff/src/app/modules/auth/auth.module.ts index 9785e7063252..8b4281c77d37 100644 --- a/apps/services/bff/src/app/modules/auth/auth.module.ts +++ b/apps/services/bff/src/app/modules/auth/auth.module.ts @@ -1,13 +1,22 @@ import { Module } from '@nestjs/common' +import { CryptoService } from '../../services/crypto.service' +import { CryptoKeyService } from '../../services/cryptoKey.service' +import { PKCEService } from '../../services/pkce.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { IdsService } from '../ids/ids.service' import { AuthController } from './auth.controller' import { AuthService } from './auth.service' -import { PKCEService } from '../../services/pkce.service' -import { CryptoService } from '../../services/crypto.service' @Module({ controllers: [AuthController], - providers: [AuthService, PKCEService, IdsService, CryptoService], + providers: [ + AuthService, + PKCEService, + IdsService, + CryptoService, + SessionCookieService, + CryptoKeyService, + ], exports: [AuthService], }) export class AuthModule {} diff --git a/apps/services/bff/src/app/modules/auth/auth.service.ts b/apps/services/bff/src/app/modules/auth/auth.service.ts index b592915ebdfd..0c9381206700 100644 --- a/apps/services/bff/src/app/modules/auth/auth.service.ts +++ b/apps/services/bff/src/app/modules/auth/auth.service.ts @@ -18,15 +18,14 @@ import { IdTokenClaims } from '@island.is/shared/types' import { Algorithm, decode, verify } from 'jsonwebtoken' import { v4 as uuid } from 'uuid' import { BffConfig } from '../../bff.config' -import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { FIVE_SECONDS_IN_MS } from '../../constants/time' import { CryptoService } from '../../services/crypto.service' import { PKCEService } from '../../services/pkce.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { CreateErrorQueryStrArgs, createErrorQueryStr, } from '../../utils/create-error-query-str' -import { getCookieOptions } from '../../utils/get-cookie-options' import { validateUri } from '../../utils/validate-uri' import { CacheService } from '../cache/cache.service' import { IdsService } from '../ids/ids.service' @@ -52,6 +51,7 @@ export class AuthService { private readonly cacheService: CacheService, private readonly idsService: IdsService, private readonly cryptoService: CryptoService, + private readonly sessionCookieService: SessionCookieService, ) { this.baseUrl = this.config.ids.issuer } @@ -125,7 +125,10 @@ export class AuthService { // Save the tokenResponse to the cache await this.cacheService.save({ - key: this.cacheService.createSessionKeyType('current', userProfile.sid), + key: this.cacheService.createSessionKeyType( + 'current', + this.sessionCookieService.hash(userProfile.sid), + ), value, ttl: this.config.cacheUserProfileTTLms, }) @@ -263,7 +266,7 @@ export class AuthService { res: Response loginAttemptCacheKey: string }) { - const sid = req.cookies[SESSION_COOKIE_NAME] + const sid = this.sessionCookieService.get(req) // Check if older session exists if (sid) { @@ -356,25 +359,28 @@ export class AuthService { // Clear any existing session cookie first // This prevents multiple session cookies being set. - res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + this.sessionCookieService.clear(res) // Create session cookie with successful login session id - res.cookie( - SESSION_COOKIE_NAME, - updatedTokenResponse.userProfile.sid, - getCookieOptions(), - ) + this.sessionCookieService.set({ + res, + value: updatedTokenResponse.userProfile.sid, + }) - // Check if there is an old session cookie and clean up the cache - const oldSessionCookie = req.cookies[SESSION_COOKIE_NAME] + // Check if there is an old session cookie + const oldHashedSessionCookie = this.sessionCookieService.get(req) if ( - oldSessionCookie && - oldSessionCookie !== updatedTokenResponse.userProfile.sid + oldHashedSessionCookie && + // Check if the old session cookie is different from the new one + !this.sessionCookieService.verify( + req, + updatedTokenResponse.userProfile.sid, + ) ) { const oldSessionCacheKey = this.cacheService.createSessionKeyType( 'current', - oldSessionCookie, + oldHashedSessionCookie, ) const oldSessionData = await this.cacheService.get( @@ -422,7 +428,7 @@ export class AuthService { res: Response query: LogoutDto }) { - const sidCookie = req.cookies[SESSION_COOKIE_NAME] + const sidCookie = this.sessionCookieService.get(req) if (!sidCookie) { this.logger.error('Logout failed: No session cookie found') @@ -430,9 +436,13 @@ export class AuthService { return res.redirect(this.config.logoutRedirectUri) } - if (sidCookie !== query.sid) { + const hashedQuerySid = this.sessionCookieService.hash(query.sid) + + if (sidCookie !== hashedQuerySid) { this.logger.error( - `Logout failed: Cookie sid "${sidCookie}" does not match the session id in query param "${query.sid}"`, + `Logout failed: Cookie "${this.cacheService.createKeyError( + sidCookie, + )}" does not match the session id in query param "sid"`, ) return this.redirectWithError(res, { @@ -443,7 +453,7 @@ export class AuthService { const currentLoginCacheKey = this.cacheService.createSessionKeyType( 'current', - query.sid, + hashedQuerySid, ) const cachedTokenResponse = @@ -470,7 +480,7 @@ export class AuthService { * - Delete the current login from the cache * - Clear the session cookie */ - res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + this.sessionCookieService.clear(res) this.cacheService .delete(currentLoginCacheKey) @@ -553,7 +563,7 @@ export class AuthService { // Create cache key and retrieve cached token response const cacheKey = this.cacheService.createSessionKeyType( 'current', - payload.sid, + this.sessionCookieService.hash(payload.sid), ) const cachedTokenResponse = await this.cacheService.get( diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts index 84d091fb0b75..6fa7ce44bb40 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts @@ -46,10 +46,10 @@ describe('TokenRefreshService', () => { let authService: AuthService let idsService: IdsService let cacheService: CacheService - const testSid = 'test-sid' + const testCacheKey = 'test-sid' const testRefreshToken = 'test-refresh-token' const refreshInProgressPrefix = 'refresh_token_in_progress' - const refreshInProgressKey = `${refreshInProgressPrefix}:${testSid}` + const refreshInProgressKey = `${refreshInProgressPrefix}:${testCacheKey}` beforeEach(async () => { const module = await Test.createTestingModule({ @@ -83,7 +83,9 @@ describe('TokenRefreshService', () => { delete: jest .fn() .mockImplementation(async (key) => mockCacheStore.delete(key)), - createSessionKeyType: jest.fn((type, sid) => `${type}_${sid}`), + createSessionKeyType: jest.fn( + (type, cackeKey) => `${type}_${cackeKey}`, + ), }, }, ], @@ -104,7 +106,7 @@ describe('TokenRefreshService', () => { it('should successfully refresh token when no refresh is in progress', async () => { // Act const result = await service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }) @@ -128,7 +130,7 @@ describe('TokenRefreshService', () => { setTimeout(async () => { await cacheService.delete(refreshInProgressKey) await cacheService.save({ - key: `current_${testSid}`, + key: `current_${testCacheKey}`, value: mockTokenResponse, ttl: 3600, }) @@ -136,7 +138,7 @@ describe('TokenRefreshService', () => { // Act const result = await service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }) @@ -155,7 +157,7 @@ describe('TokenRefreshService', () => { // Act const result = await service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }) @@ -172,15 +174,13 @@ describe('TokenRefreshService', () => { // Act const cachedTokenResponse = await service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }) // expect(cachedTokenResponse).toBe(null) - expect(mockLogger.warn).toHaveBeenCalledWith( - `Token refresh failed for sid: ${testSid}`, - ) + expect(mockLogger.warn).toHaveBeenCalledWith('Token refresh failed') }) it('should prevent concurrent refresh token requests', async () => { @@ -217,7 +217,7 @@ describe('TokenRefreshService', () => { // First request refreshPromises.push( service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }), ) @@ -229,7 +229,7 @@ describe('TokenRefreshService', () => { for (let i = 1; i < refreshCount; i++) { refreshPromises.push( service.refreshToken({ - sid: testSid, + cacheKey: testCacheKey, encryptedRefreshToken: testRefreshToken, }), ) diff --git a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts index d2cfad0b426a..9567f680b50c 100644 --- a/apps/services/bff/src/app/modules/auth/token-refresh.service.ts +++ b/apps/services/bff/src/app/modules/auth/token-refresh.service.ts @@ -98,9 +98,9 @@ export class TokenRefreshService { * Waits for an ongoing refresh operation to complete * Uses polling with a maximum wait time * - * @param sid Session ID + * @param cacheKey Cached key for tracking refresh status */ - private async waitForRefreshCompletion(sid: string): Promise { + private async waitForRefreshCompletion(cacheKey: string): Promise { let attempts = 0 // Calculate how many attempts we should make // maxAttempts = 3000 / 200 = ~15 ish attempts @@ -109,7 +109,7 @@ export class TokenRefreshService { while (attempts < maxAttempts) { const refreshTokenInProgress = await this.cacheService.get( - this.createRefreshTokenKey(sid), + this.createRefreshTokenKey(cacheKey), false, ) @@ -126,7 +126,7 @@ export class TokenRefreshService { // We've made all attempts (~15 attempts in 3 seconds total) and still no success this.logger.warn( - `Polling timed out for token refresh completion for session ${sid}`, + 'Polling timed out for token refresh completion for session id (sid)', ) return false @@ -179,14 +179,14 @@ export class TokenRefreshService { */ public async refreshToken({ - sid, + cacheKey, encryptedRefreshToken, }: { - sid: string + cacheKey: string encryptedRefreshToken: string }): Promise { - const refreshTokenKey = this.createRefreshTokenKey(sid) - const tokenResponseKey = this.createTokenResponseKey(sid) + const refreshTokenKey = this.createRefreshTokenKey(cacheKey) + const tokenResponseKey = this.createTokenResponseKey(cacheKey) // Check if refresh is already in progress const refreshTokenInProgress = await this.cacheService.get( @@ -195,7 +195,7 @@ export class TokenRefreshService { ) if (refreshTokenInProgress) { - const refreshCompleted = await this.waitForRefreshCompletion(sid) + const refreshCompleted = await this.waitForRefreshCompletion(cacheKey) if (refreshCompleted) { const cachedToken = await this.getTokenFromCache(tokenResponseKey) @@ -215,7 +215,7 @@ export class TokenRefreshService { }) if (!updatedTokenResponse) { - this.logger.warn(`Token refresh failed for sid: ${sid}`) + this.logger.warn('Token refresh failed') } return updatedTokenResponse diff --git a/apps/services/bff/src/app/modules/cache/cache.service.spec.ts b/apps/services/bff/src/app/modules/cache/cache.service.spec.ts new file mode 100644 index 000000000000..a4de2a1d7e14 --- /dev/null +++ b/apps/services/bff/src/app/modules/cache/cache.service.spec.ts @@ -0,0 +1,208 @@ +import { CACHE_MANAGER } from '@nestjs/cache-manager' +import { Test, TestingModule } from '@nestjs/testing' +import { Cache as CacheManager } from 'cache-manager' +import { BffConfig } from '../../bff.config' +import { CacheService } from './cache.service' + +const mockConfig = { + name: 'test-bff', +} + +class TestCacheService extends CacheService { + public testKeyWithoutSid(key: string): string { + return this['keyWithoutSid'](key) + } +} + +describe('CacheService', () => { + let service: TestCacheService + let mockCacheManager: jest.Mocked + + beforeEach(async () => { + mockCacheManager = { + set: jest.fn(), + get: jest.fn(), + del: jest.fn(), + } as unknown as jest.Mocked + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + { + provide: CacheService, + useClass: TestCacheService, + }, + { + provide: CACHE_MANAGER, + useValue: mockCacheManager, + }, + { + provide: BffConfig.KEY, + useValue: mockConfig, + }, + ], + }).compile() + + service = module.get(CacheService) + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + describe('keyWithoutSid', () => { + it('should keep only first two parts of key regardless of total parts', () => { + // Arrange + const key = 'attempt::test-bff::1234::extra::more' + + // Act + const result = service.testKeyWithoutSid(key) + + // Assert + expect(result).toBe('attempt::test-bff') + }) + + it('should return original key if it has less than two parts', () => { + // Arrange + const key = 'attempt' + + // Act + const result = service.testKeyWithoutSid(key) + + // Assert + expect(result).toBe('attempt') + }) + + it('should return original key if it has exactly two parts', () => { + // Arrange + const key = 'attempt::test-bff' + + // Act + const result = service.testKeyWithoutSid(key) + + // Assert + expect(result).toBe('attempt::test-bff') + }) + }) + + describe('createKeyError', () => { + it('should create error message with sanitized key', () => { + // Arrange + const key = 'attempt::test-bff::1234' + + // Act + const result = service.createKeyError(key) + + // Assert + expect(result).toBe('Cache key "attempt::test-bff" not found.') + }) + }) + + describe('createSessionKeyType', () => { + it('should create attempt key', () => { + // Act + const result = service.createSessionKeyType('attempt', '1234') + + // Assert + expect(result).toBe('attempt::test-bff::1234') + }) + + it('should create current key', () => { + // Act + const result = service.createSessionKeyType('current', '1234') + + // Assert + expect(result).toBe('current::test-bff::1234') + }) + }) + + describe('save', () => { + it('should save value with ttl', async () => { + // Arrange + const key = 'test-key' + const value = { data: 'test' } + const ttl = 1000 + + // Act + await service.save({ key, value, ttl }) + + // Assert + expect(mockCacheManager.set).toHaveBeenCalledWith(key, value, ttl) + }) + + it('should save value without ttl', async () => { + // Arrange + const key = 'test-key' + const value = { data: 'test' } + + // Act + await service.save({ key, value }) + + // Assert + expect(mockCacheManager.set).toHaveBeenCalledWith(key, value, undefined) + }) + }) + + describe('get', () => { + it('should return cached value when exists', async () => { + // Arrange + const key = 'test-key' + const value = { data: 'test' } + mockCacheManager.get.mockResolvedValue(value) + + // Act + const result = await service.get<{ data: string }>(key) + + // Assert + expect(result).toEqual(value) + expect(mockCacheManager.get).toHaveBeenCalledWith(key) + }) + + it('should throw error when value does not exist and throwError is true', async () => { + // Arrange + const key = 'test-key' + mockCacheManager.get.mockResolvedValue(null) + + // Act & Assert + await expect(service.get(key)).rejects.toThrow( + 'Cache key "test-key" not found.', + ) + }) + + it('should return null when value does not exist and throwError is false', async () => { + // Arrange + const key = 'test-key' + mockCacheManager.get.mockResolvedValue(null) + + // Act + const result = await service.get(key, false) + + // Assert + expect(result).toBeNull() + }) + }) + + describe('delete', () => { + it('should delete cache entry', async () => { + // Arrange + const key = 'test-key' + mockCacheManager.del.mockResolvedValue(undefined) + + // Act + await service.delete(key) + + // Assert + expect(mockCacheManager.del).toHaveBeenCalledWith(key) + }) + + it('should throw error with sanitized key when deletion fails', async () => { + // Arrange + const key = 'attempt::test-bff::1234' + mockCacheManager.del.mockRejectedValue(new Error('Deletion failed')) + + // Act & Assert + await expect(service.delete(key)).rejects.toThrow( + 'Failed to delete key "attempt::test-bff" from cache.', + ) + }) + }) +}) diff --git a/apps/services/bff/src/app/modules/cache/cache.service.ts b/apps/services/bff/src/app/modules/cache/cache.service.ts index e5888f9b2c93..3d654bb0c209 100644 --- a/apps/services/bff/src/app/modules/cache/cache.service.ts +++ b/apps/services/bff/src/app/modules/cache/cache.service.ts @@ -7,6 +7,8 @@ import { BffConfig } from '../../bff.config' @Injectable() export class CacheService { + private separator = '::' + constructor( @Inject(CACHE_MANAGER) private readonly cacheManager: CacheManager, @@ -15,8 +17,25 @@ export class CacheService { private readonly config: ConfigType, ) {} + /** + * If the key contains multiple parts separated by separator, + * it will keep only the first two parts to ensure no session id or other sensitive data is leaked to logs. + * If the key has less than two parts, it will return the key as is. + * + * @example + * keyWithoutSid('attempt::some_name::1234') // attempt::some_name + * keyWithoutSid('attempt::some_name::1234::extra') // attempt::some_name + * keyWithoutSid('some_name::1234') // some_name::1234 + * keyWithoutSid('some_name') // some_name + */ + private keyWithoutSid(key: string): string { + const parts = key.split(this.separator) + + return parts.length >= 2 ? parts.slice(0, 2).join(this.separator) : key + } + public createKeyError(key: string) { - return `Cache key "${key}" not found.` + return `Cache key "${this.keyWithoutSid(key)}" not found.` } /** @@ -30,7 +49,7 @@ export class CacheService { * createSessionKeyType('current', '1234') // current::{bffName}::1234 */ public createSessionKeyType(type: 'attempt' | 'current', sid: string) { - return `${type}::${this.config.name}::${sid}` + return `${type}${this.separator}${this.config.name}${this.separator}${sid}` } public async save({ @@ -68,7 +87,9 @@ export class CacheService { try { await this.cacheManager.del(key) } catch (error) { - throw new Error(`Failed to delete key "${key}" from cache.`) + throw new Error( + `Failed to delete key "${this.keyWithoutSid(key)}" from cache.`, + ) } } } diff --git a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts index c5d4ba8e89e3..5d461988435e 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts @@ -1,6 +1,7 @@ import { ConfigType } from '@island.is/nest/config' import { CACHE_MANAGER } from '@nestjs/cache-manager' import { HttpStatus, INestApplication } from '@nestjs/common' +import type { Request, Response } from 'express' import request from 'supertest' import { setupTestServer } from '../../../../test/setupTestServer' @@ -11,6 +12,7 @@ import { mockedTokensResponse as tokensResponse, } from '../../../../test/sharedConstants' import { BffConfig } from '../../bff.config' +import { SessionCookieService } from '../../services/sessionCookie.service' import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { IdsService } from '../ids/ids.service' import { TokenResponse } from '../ids/ids.types' @@ -60,6 +62,19 @@ describe('ProxyController', () => { let server: request.SuperTest let capturedArgs: ExecuteStreamRequestArgs let mockConfig: ConfigType + let sessionCookieService: SessionCookieService + let hashedSid: string + + // Mock Response object + const mockResponse: Partial = { + cookie: jest.fn(), + clearCookie: jest.fn(), + } + + // Mock Request object + const mockRequest: Partial = { + cookies: {}, + } beforeAll(async () => { const app = await setupTestServer({ @@ -78,7 +93,22 @@ describe('ProxyController', () => { ), }) + sessionCookieService = app.get(SessionCookieService) mockConfig = app.get>(BffConfig.KEY) + + // Set the hashed SID + sessionCookieService.set({ + res: mockResponse as Response, + value: SID_VALUE, + }) + + // Capture the hashed value from the mock cookie call + hashedSid = (mockResponse.cookie as jest.Mock).mock.calls[0][1] + + // Set up the mock request cookies for subsequent get() calls + mockRequest.cookies = { + [SESSION_COOKIE_NAME]: hashedSid, + } server = request(app.getHttpServer()) }) @@ -123,7 +153,7 @@ describe('ProxyController', () => { const res = await server .get('/api') .query({ url: allowedExternalApiUrl }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.UNAUTHORIZED) @@ -135,13 +165,13 @@ describe('ProxyController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .get('/api') .query({ url: allowedExternalApiUrl }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.OK) @@ -169,12 +199,12 @@ describe('ProxyController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .post('/api/graphql') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.OK) @@ -187,12 +217,12 @@ describe('ProxyController', () => { await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .post('/api/graphql?op=test') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.OK) @@ -217,12 +247,12 @@ describe('ProxyController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .post('/api/graphql') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockIdsService.refreshToken).toHaveBeenCalled() @@ -251,12 +281,12 @@ describe('ProxyController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .post('/api/graphql') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockIdsService.refreshToken).toHaveBeenCalled() @@ -297,12 +327,12 @@ describe('ProxyController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) const res = await server .post('/api/graphql') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockIdsService.refreshToken).not.toHaveBeenCalled() diff --git a/apps/services/bff/src/app/modules/proxy/proxy.module.ts b/apps/services/bff/src/app/modules/proxy/proxy.module.ts index 6cbeba5c5733..2e871bfeddd8 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.module.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.module.ts @@ -1,6 +1,8 @@ import { Module } from '@nestjs/common' import { CryptoService } from '../../services/crypto.service' +import { CryptoKeyService } from '../../services/cryptoKey.service' import { ErrorService } from '../../services/error.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { AuthModule } from '../auth/auth.module' import { TokenRefreshService } from '../auth/token-refresh.service' import { IdsService } from '../ids/ids.service' @@ -16,6 +18,8 @@ import { ProxyService } from './proxy.service' TokenRefreshService, ErrorService, IdsService, + SessionCookieService, + CryptoKeyService, ], }) export class ProxyModule {} diff --git a/apps/services/bff/src/app/modules/proxy/proxy.service.ts b/apps/services/bff/src/app/modules/proxy/proxy.service.ts index 27c6536db887..59f76e10a678 100644 --- a/apps/services/bff/src/app/modules/proxy/proxy.service.ts +++ b/apps/services/bff/src/app/modules/proxy/proxy.service.ts @@ -18,8 +18,8 @@ import { AGENT_DEFAULT_FREE_SOCKET_TIMEOUT, AGENT_DEFAULT_MAX_SOCKETS, } from '@island.is/shared/constants' -import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { validateUri } from '../../utils/validate-uri' import { CachedTokenResponse } from '../auth/auth.types' @@ -69,6 +69,7 @@ export class ProxyService { private readonly cryptoService: CryptoService, private readonly tokenRefreshService: TokenRefreshService, private readonly errorService: ErrorService, + private readonly sessionCookieService: SessionCookieService, ) {} /** @@ -83,7 +84,7 @@ export class ProxyService { req: Request res: Response }): Promise { - const sid = req.cookies[SESSION_COOKIE_NAME] + const sid = this.sessionCookieService.get(req) if (!sid) { throw new UnauthorizedException() @@ -106,7 +107,7 @@ export class ProxyService { hasTimestampExpiredInMS(cachedTokenResponse.accessTokenExp) ) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ - sid, + cacheKey: sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, }) } diff --git a/apps/services/bff/src/app/modules/user/user.controller.spec.ts b/apps/services/bff/src/app/modules/user/user.controller.spec.ts index a6dfc9d72ef6..ebc935c589fe 100644 --- a/apps/services/bff/src/app/modules/user/user.controller.spec.ts +++ b/apps/services/bff/src/app/modules/user/user.controller.spec.ts @@ -1,6 +1,7 @@ import { ConfigType } from '@island.is/nest/config' import { CACHE_MANAGER } from '@nestjs/cache-manager' import { HttpStatus, INestApplication } from '@nestjs/common' +import type { Request, Response } from 'express' import request from 'supertest' import { setupTestServer } from '../../../../test/setupTestServer' @@ -11,6 +12,7 @@ import { mockedTokensResponse as tokensResponse, } from '../../../../test/sharedConstants' import { BffConfig } from '../../bff.config' +import { SessionCookieService } from '../../services/sessionCookie.service' import { TokenRefreshService } from '../auth/token-refresh.service' import { IdsService } from '../ids/ids.service' @@ -70,6 +72,19 @@ describe('UserController', () => { let app: INestApplication let server: request.SuperTest let mockConfig: ConfigType + let sessionCookieService: SessionCookieService + let hashedSid: string + + // Mock Response object + const mockResponse: Partial = { + cookie: jest.fn(), + clearCookie: jest.fn(), + } + + // Mock Request object + const mockRequest: Partial = { + cookies: {}, + } beforeAll(async () => { app = await setupTestServer({ @@ -83,13 +98,31 @@ describe('UserController', () => { .useValue(mockIdsService), }) + sessionCookieService = app.get(SessionCookieService) mockConfig = app.get>(BffConfig.KEY) + + // Set the hashed SID + sessionCookieService.set({ + res: mockResponse as Response, + value: SID_VALUE, + }) + + // Capture the hashed value from the mock cookie call + hashedSid = (mockResponse.cookie as jest.Mock).mock.calls[0][1] + + // Set up the mock request cookies for subsequent get() calls + mockRequest.cookies = { + [SESSION_COOKIE_NAME]: hashedSid, + } + server = request(app.getHttpServer()) }) afterEach(() => { mockCacheStore.clear() jest.clearAllMocks() + // Reset mocks + mockRequest.cookies = {} }) afterAll(async () => { @@ -111,7 +144,7 @@ describe('UserController', () => { // Act const res = await server .get('/user') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.UNAUTHORIZED) @@ -128,7 +161,7 @@ describe('UserController', () => { await server.get('/login') const callbackRes = await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) expect(callbackRes.status).toBe(HttpStatus.FOUND) @@ -136,7 +169,7 @@ describe('UserController', () => { // Act - Get user data const res = await server .get('/user') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(res.status).toEqual(HttpStatus.OK) @@ -161,7 +194,7 @@ describe('UserController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) // Set expired token in cache @@ -170,7 +203,7 @@ describe('UserController', () => { accessTokenExp: Date.now() - 1000, // Expired token } mockCacheStore.set( - `current::${mockConfig.name}::${SID_VALUE}`, + `current::${mockConfig.name}::${hashedSid}`, expiredTokenResponse, ) @@ -178,11 +211,11 @@ describe('UserController', () => { const res = await server .get('/user') .query({ refresh: 'true' }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockTokenRefreshService.refreshToken).toHaveBeenCalledWith({ - sid: SID_VALUE, + cacheKey: hashedSid, encryptedRefreshToken: expiredTokenResponse.encryptedRefreshToken, }) expect(res.status).toEqual(HttpStatus.OK) @@ -203,7 +236,7 @@ describe('UserController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) // Set expired token in cache @@ -212,7 +245,7 @@ describe('UserController', () => { accessTokenExp: Date.now() - 1000, // Expired token } mockCacheStore.set( - `current::${mockConfig.name}::${SID_VALUE}`, + `current::${mockConfig.name}::${hashedSid}`, expiredTokenResponse, ) @@ -220,7 +253,7 @@ describe('UserController', () => { const res = await server .get('/user') .query({ refresh: 'false' }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockTokenRefreshService.refreshToken).not.toHaveBeenCalled() @@ -242,7 +275,7 @@ describe('UserController', () => { await server.get('/login') await server .get('/callbacks/login') - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) .query({ code: 'some_code', state: SID_VALUE }) // Set valid (not expired) token in cache @@ -251,7 +284,7 @@ describe('UserController', () => { accessTokenExp: Date.now() + 1000, // Future expiration } mockCacheStore.set( - `current::${mockConfig.name}::${SID_VALUE}`, + `current::${mockConfig.name}::${hashedSid}`, validTokenResponse, ) @@ -259,7 +292,7 @@ describe('UserController', () => { const res = await server .get('/user') .query({ refresh: 'true' }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert expect(mockTokenRefreshService.refreshToken).not.toHaveBeenCalled() @@ -317,7 +350,7 @@ describe('UserController', () => { : Date.now() + 1000, // Not expired } mockCacheStore.set( - `current::${mockConfig.name}::${SID_VALUE}`, + `current::${mockConfig.name}::${hashedSid}`, tokenResponse, ) } @@ -326,7 +359,7 @@ describe('UserController', () => { const res = await server .get('/user') .query({ refresh: testCase.refresh.toString() }) - .set('Cookie', [`${SESSION_COOKIE_NAME}=${SID_VALUE}`]) + .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) // Assert if (testCase.shouldCallRefresh) { diff --git a/apps/services/bff/src/app/modules/user/user.module.ts b/apps/services/bff/src/app/modules/user/user.module.ts index d95c6b1cd7cc..e2070eaf6cf6 100644 --- a/apps/services/bff/src/app/modules/user/user.module.ts +++ b/apps/services/bff/src/app/modules/user/user.module.ts @@ -1,6 +1,8 @@ import { Module } from '@nestjs/common' import { CryptoService } from '../../services/crypto.service' +import { CryptoKeyService } from '../../services/cryptoKey.service' import { ErrorService } from '../../services/error.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { AuthModule } from '../auth/auth.module' import { TokenRefreshService } from '../auth/token-refresh.service' import { IdsService } from '../ids/ids.service' @@ -16,6 +18,8 @@ import { UserService } from './user.service' CryptoService, TokenRefreshService, ErrorService, + SessionCookieService, + CryptoKeyService, ], }) export class UserModule {} diff --git a/apps/services/bff/src/app/modules/user/user.service.ts b/apps/services/bff/src/app/modules/user/user.service.ts index 74f576a5d8aa..45aac9caf009 100644 --- a/apps/services/bff/src/app/modules/user/user.service.ts +++ b/apps/services/bff/src/app/modules/user/user.service.ts @@ -2,9 +2,9 @@ import { Injectable, UnauthorizedException } from '@nestjs/common' import type { Request, Response } from 'express' import { BffUser } from '@island.is/shared/types' -import { SESSION_COOKIE_NAME } from '../../constants/cookies' import { ErrorService } from '../../services/error.service' +import { SessionCookieService } from '../../services/sessionCookie.service' import { hasTimestampExpiredInMS } from '../../utils/has-timestamp-expired-in-ms' import { CachedTokenResponse } from '../auth/auth.types' import { TokenRefreshService } from '../auth/token-refresh.service' @@ -16,6 +16,7 @@ export class UserService { private readonly cacheService: CacheService, private readonly tokenRefreshService: TokenRefreshService, private readonly errorService: ErrorService, + private readonly sessionCookieService: SessionCookieService, ) {} /** @@ -40,7 +41,7 @@ export class UserService { res: Response refresh: boolean }): Promise { - const sid = req.cookies[SESSION_COOKIE_NAME] + const sid = this.sessionCookieService.get(req) if (!sid) { throw new UnauthorizedException() @@ -65,7 +66,7 @@ export class UserService { refresh ) { cachedTokenResponse = await this.tokenRefreshService.refreshToken({ - sid, + cacheKey: sid, encryptedRefreshToken: cachedTokenResponse.encryptedRefreshToken, }) } diff --git a/apps/services/bff/src/app/services/crypto.service.spec.ts b/apps/services/bff/src/app/services/crypto.service.spec.ts index 87528e308640..e95a55b66ad1 100644 --- a/apps/services/bff/src/app/services/crypto.service.spec.ts +++ b/apps/services/bff/src/app/services/crypto.service.spec.ts @@ -2,8 +2,10 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { Test, TestingModule } from '@nestjs/testing' import crypto from 'crypto' -import { CryptoService } from './crypto.service' +import { tokenSecretBase64 } from '../../../test/sharedConstants' import { BffConfig } from '../bff.config' +import { CryptoService } from './crypto.service' +import { CryptoKeyService } from './cryptoKey.service' const DECRYPTED_TEXT = 'Hello, World!' @@ -18,8 +20,7 @@ const invalidConfig = { } const validConfig = { - // A valid 32-byte base64 key - tokenSecretBase64: 'ABHlmq6Ic6Ihip4OnTa1MeUXtHFex8IT/mFZrjhsme0=', + tokenSecretBase64, } const mockLogger = { @@ -30,6 +31,7 @@ const createModule = async (config = validConfig): Promise => { return Test.createTestingModule({ providers: [ CryptoService, + CryptoKeyService, { provide: LOGGER_PROVIDER, useValue: mockLogger }, { provide: BffConfig.KEY, useValue: config }, ], diff --git a/apps/services/bff/src/app/services/crypto.service.ts b/apps/services/bff/src/app/services/crypto.service.ts index f5b39526b7c1..ffeb6f200c16 100644 --- a/apps/services/bff/src/app/services/crypto.service.ts +++ b/apps/services/bff/src/app/services/crypto.service.ts @@ -1,32 +1,18 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { Inject, Injectable } from '@nestjs/common' -import { ConfigType } from '@nestjs/config' import * as crypto from 'crypto' -import { BffConfig } from '../bff.config' +import { CryptoKeyService } from './cryptoKey.service' @Injectable() export class CryptoService { private readonly algorithm = 'aes-256-cbc' - private readonly key: Buffer constructor( @Inject(LOGGER_PROVIDER) - private logger: Logger, - - @Inject(BffConfig.KEY) - private readonly config: ConfigType, - ) { - // Decode from base64 to binary - this.key = Buffer.from(this.config.tokenSecretBase64, 'base64') - - // Ensure the key is exactly 32 bytes (256 bits) long - if (this.key.length !== 32) { - throw new Error( - '"tokenSecretBase64" secret must be exactly 32 bytes (256 bits) long.', - ) - } - } + private readonly logger: Logger, + private readonly cryptoKeyService: CryptoKeyService, + ) {} /** * Encrypts a given text using the AES-256-CBC encryption algorithm. @@ -45,7 +31,11 @@ export class CryptoService { const iv = crypto.randomBytes(16) // Create a Cipher object using the algorithm, encryption key, and initialization vector (IV) - const cipher = crypto.createCipheriv(this.algorithm, this.key, iv) + const cipher = crypto.createCipheriv( + this.algorithm, + this.cryptoKeyService.cryptoKey, + iv, + ) // Encrypt the text in 'utf8' format and encode the result as base64 let encrypted = cipher.update(text, 'utf8', 'base64') @@ -88,7 +78,11 @@ export class CryptoService { const iv = Buffer.from(ivBase64, 'base64') // Create a Decipher object using the same algorithm, key, and IV - const decipher = crypto.createDecipheriv(this.algorithm, this.key, iv) + const decipher = crypto.createDecipheriv( + this.algorithm, + this.cryptoKeyService.cryptoKey, + iv, + ) // Decrypt the encrypted text from base64 to utf8 format let decrypted = decipher.update(encrypted, 'base64', 'utf8') diff --git a/apps/services/bff/src/app/services/cryptoKey.service.ts b/apps/services/bff/src/app/services/cryptoKey.service.ts new file mode 100644 index 000000000000..51dd4768ac2d --- /dev/null +++ b/apps/services/bff/src/app/services/cryptoKey.service.ts @@ -0,0 +1,49 @@ +import { Inject, Injectable } from '@nestjs/common' +import { ConfigType } from '@nestjs/config' +import { BffConfig } from '../bff.config' + +/** + * Service that handles the initialization and validation of the 256-bit cryptographic key + * used for both AES-256-CBC encryption and HMAC-SHA256 hashing operations. + * + * The key is derived from a base64 encoded secret configured in the application settings. + * It must be exactly 32 bytes (256 bits) when decoded. + */ +@Injectable() +export class CryptoKeyService { + private readonly _cryptoKey: Buffer + + constructor( + @Inject(BffConfig.KEY) + private readonly config: ConfigType, + ) { + this._cryptoKey = this.initializeCryptoKey() + } + + /** + * Initializes and validates the cryptographic key. + * The key must be a base64 encoded string that decodes to exactly 32 bytes (256 bits). + * + * @throws Error if the decoded key is not exactly 32 bytes + * @returns Buffer containing the 256-bit key + */ + private initializeCryptoKey(): Buffer { + const key = Buffer.from(this.config.tokenSecretBase64, 'base64') + + if (key.length !== 32) { + throw new Error( + '"tokenSecretBase64" secret must be exactly 32 bytes (256 bits) long.', + ) + } + + return key + } + + /** + * Returns the 256-bit cryptographic key used for AES-256-CBC encryption + * and HMAC-SHA256 hashing operations. + */ + get cryptoKey(): Buffer { + return this._cryptoKey + } +} diff --git a/apps/services/bff/src/app/services/error.service.ts b/apps/services/bff/src/app/services/error.service.ts index 8e4ad8efd551..27f5ac76ac7c 100644 --- a/apps/services/bff/src/app/services/error.service.ts +++ b/apps/services/bff/src/app/services/error.service.ts @@ -2,9 +2,8 @@ import type { Logger } from '@island.is/logging' import { LOGGER_PROVIDER } from '@island.is/logging' import { Inject, Injectable, UnauthorizedException } from '@nestjs/common' import type { Response } from 'express' -import { SESSION_COOKIE_NAME } from '../constants/cookies' import { CacheService } from '../modules/cache/cache.service' -import { getCookieOptions } from '../utils/get-cookie-options' +import { SessionCookieService } from './sessionCookie.service' /** * Standard OAuth2 error codes returned by Identity Server @@ -34,6 +33,7 @@ export class ErrorService { private logger: Logger, private readonly cacheService: CacheService, + private readonly sessionCookieService: SessionCookieService, ) {} /** @@ -79,7 +79,7 @@ export class ErrorService { error, ) - res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + this.sessionCookieService.clear(res) await this.cacheService.delete(tokenResponseKey) throw new UnauthorizedException() diff --git a/apps/services/bff/src/app/services/sessionCookie.service.spec.ts b/apps/services/bff/src/app/services/sessionCookie.service.spec.ts new file mode 100644 index 000000000000..295380ccfad6 --- /dev/null +++ b/apps/services/bff/src/app/services/sessionCookie.service.spec.ts @@ -0,0 +1,189 @@ +import type { Logger } from '@island.is/logging' +import { LOGGER_PROVIDER } from '@island.is/logging' +import { Test, TestingModule } from '@nestjs/testing' +import type { Request, Response } from 'express' +import { tokenSecretBase64 } from '../../../test/sharedConstants' +import { BffConfig } from '../bff.config' +import { SESSION_COOKIE_NAME } from '../constants/cookies' +import { getCookieOptions } from '../utils/get-cookie-options' +import { CryptoKeyService } from './cryptoKey.service' +import { SessionCookieService } from './sessionCookie.service' + +const mockLogger = { + error: jest.fn(), +} as unknown as Logger + +const validConfig = { + tokenSecretBase64, +} + +describe('SessionCookieService', () => { + let service: SessionCookieService + let mockResponse: Partial + let mockRequest: Partial + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + SessionCookieService, + CryptoKeyService, + { provide: LOGGER_PROVIDER, useValue: mockLogger }, + { provide: BffConfig.KEY, useValue: validConfig }, + ], + }).compile() + + service = module.get(SessionCookieService) + + mockResponse = { + cookie: jest.fn(), + clearCookie: jest.fn(), + } + + mockRequest = { + cookies: {}, + } + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + describe('get', () => { + it('should return undefined when cookie is not present', () => { + // Act + const result = service.get(mockRequest as Request) + + // Assert + expect(result).toBeUndefined() + }) + + it('should return cookie value when present', () => { + // Arrange + const cookieValue = 'test-cookie-value' + mockRequest.cookies = { [SESSION_COOKIE_NAME]: cookieValue } + + // Act + const result = service.get(mockRequest as Request) + + // Assert + expect(result).toBe(cookieValue) + }) + }) + + describe('set', () => { + it('should set cookie with hashed value', () => { + // Arrange + const value = 'test-value' + const hashedValue = service.hash(value) + + // Act + service.set({ res: mockResponse as Response, value }) + + // Assert + expect(mockResponse.cookie).toHaveBeenCalledWith( + SESSION_COOKIE_NAME, + hashedValue, + getCookieOptions(), + ) + }) + + it('should throw error if hashing fails', () => { + // Arrange + jest.spyOn(service, 'hash').mockImplementation(() => { + throw new Error('Hashing failed') + }) + + // Act & Assert + expect(() => + service.set({ res: mockResponse as Response, value: 'test-value' }), + ).toThrow('Hashing failed') + + expect(mockLogger.error).toHaveBeenCalledWith( + 'Error hashing session cookie: ', + { message: 'Hashing failed' }, + ) + }) + }) + + describe('verify', () => { + it('should return false when cookie is not present', () => { + // Act + const result = service.verify(mockRequest as Request, 'test-value') + + // Assert + expect(result).toBe(false) + }) + + it('should return true when value matches hashed cookie', () => { + // Arrange + const value = 'test-value' + const hashedValue = service.hash(value) + mockRequest.cookies = { [SESSION_COOKIE_NAME]: hashedValue } + + // Act + const result = service.verify(mockRequest as Request, value) + + // Assert + expect(result).toBe(true) + }) + + it('should return false when value does not match hashed cookie', () => { + // Arrange + const hashedValue = service.hash('original-value') + mockRequest.cookies = { [SESSION_COOKIE_NAME]: hashedValue } + + // Act + const result = service.verify(mockRequest as Request, 'different-value') + + // Assert + expect(result).toBe(false) + }) + }) + + describe('clear', () => { + it('should clear the session cookie', () => { + // Act + service.clear(mockResponse as Response) + + // Assert + expect(mockResponse.clearCookie).toHaveBeenCalledWith( + SESSION_COOKIE_NAME, + expect.any(Object), + ) + }) + }) + + describe('hash', () => { + it('should create consistent hashes for the same value', () => { + // Arrange + const value = 'test-value' + + // Act + const hash1 = service.hash(value) + const hash2 = service.hash(value) + + // Assert + expect(hash1).toBe(hash2) + }) + + it('should create different hashes for different values', () => { + // Act + const hash1 = service.hash('value1') + const hash2 = service.hash('value2') + + // Assert + expect(hash1).not.toBe(hash2) + }) + + it('should return a hex string', () => { + // Arrange + const value = 'test-value' + + // Act + const hash = service.hash(value) + + // Assert + expect(hash).toMatch(/^[0-9a-f]+$/) + }) + }) +}) diff --git a/apps/services/bff/src/app/services/sessionCookie.service.ts b/apps/services/bff/src/app/services/sessionCookie.service.ts new file mode 100644 index 000000000000..89b5264f17ef --- /dev/null +++ b/apps/services/bff/src/app/services/sessionCookie.service.ts @@ -0,0 +1,76 @@ +import type { Logger } from '@island.is/logging' +import { LOGGER_PROVIDER } from '@island.is/logging' +import { Inject, Injectable } from '@nestjs/common' +import * as crypto from 'crypto' +import type { Request, Response } from 'express' +import { SESSION_COOKIE_NAME } from '../constants/cookies' +import { getCookieOptions } from '../utils/get-cookie-options' +import { CryptoKeyService } from './cryptoKey.service' + +@Injectable() +export class SessionCookieService { + constructor( + @Inject(LOGGER_PROVIDER) + private readonly logger: Logger, + private readonly cryptoKeyService: CryptoKeyService, + ) {} + + /** + * Gets the hashed session cookie value. + * + * Note: With hashing, we cannot recover the original value. + * You'll need to compare hashed values to verify matches. + */ + get(req: Request): string | undefined { + return req.cookies[SESSION_COOKIE_NAME] ?? undefined + } + + /** + * Sets an encrypted session cookie with the given value + */ + set({ res, value }: { res: Response; value: string }): void { + try { + const hashedValue = this.hash(value) + + res.cookie(SESSION_COOKIE_NAME, hashedValue, getCookieOptions()) + } catch (error) { + this.logger.error('Error hashing session cookie: ', { + message: error.message, + }) + + throw error + } + } + + /** + * Verifies if a given value matches the hashed cookie + */ + verify(req: Request, value: string): boolean { + const hashedCookie = this.get(req) + + if (!hashedCookie) { + return false + } + + const hashedValue = this.hash(value) + + return hashedCookie === hashedValue + } + + /** + * Clears the session cookie + */ + clear(res: Response): void { + res.clearCookie(SESSION_COOKIE_NAME, getCookieOptions()) + } + + /** + * Helper method to create consistent hashes using HMAC + */ + hash(value: string): string { + return crypto + .createHmac('sha256', this.cryptoKeyService.cryptoKey) + .update(value) + .digest('hex') + } +} diff --git a/apps/services/bff/test/sharedConstants.ts b/apps/services/bff/test/sharedConstants.ts index 99ec6d196209..df1f4264ac85 100644 --- a/apps/services/bff/test/sharedConstants.ts +++ b/apps/services/bff/test/sharedConstants.ts @@ -1,7 +1,7 @@ import jwt from 'jsonwebtoken' import { - TokenResponse, GetLoginSearchParamsReturnValue, + TokenResponse, } from '../src/app/modules/ids/ids.types' export const SESSION_COOKIE_NAME = 'sid' @@ -10,6 +10,9 @@ export const SID_VALUE = 'fake_uuid' const ONE_HOUR_EXPIRATION = Math.floor(Date.now() / 1000) + 3600 +// A valid 32-byte base64 key +export const tokenSecretBase64 = 'ABHlmq6Ic6Ihip4OnTa1MeUXtHFex8IT/mFZrjhsme0=' + export const mockedTokensResponse: TokenResponse = { access_token: jwt.sign( {