diff --git a/src/server/notifications/KeyValueChannelStorage.ts b/src/server/notifications/KeyValueChannelStorage.ts index b15684278..073f65837 100644 --- a/src/server/notifications/KeyValueChannelStorage.ts +++ b/src/server/notifications/KeyValueChannelStorage.ts @@ -10,6 +10,7 @@ type StorageValue = string | string[] | NotificationChannel; /** * Stores all the {@link NotificationChannel} in a {@link KeyValueStorage}. + * Encodes IDs/topics before storing them in the KeyValueStorage. * * Uses a {@link ReadWriteLocker} to prevent internal race conditions. */ @@ -25,7 +26,7 @@ export class KeyValueChannelStorage implements NotificationChannelStorage { } public async get(id: string): Promise { - const channel = await this.storage.get(id); + const channel = await this.storage.get(encodeURIComponent(id)); if (channel && this.isChannel(channel)) { if (typeof channel.endAt === 'number' && channel.endAt < Date.now()) { this.logger.info(`Notification channel ${id} has expired.`); @@ -40,7 +41,7 @@ export class KeyValueChannelStorage implements NotificationChannelStorage { } public async getAll(topic: ResourceIdentifier): Promise { - const channels = await this.storage.get(topic.path); + const channels = await this.storage.get(encodeURIComponent(topic.path)); if (Array.isArray(channels)) { return channels; } @@ -51,15 +52,15 @@ export class KeyValueChannelStorage implements NotificationChannelStorage { const target = { path: channel.topic }; return this.locker.withWriteLock(this.getLockKey(target), async(): Promise => { const channels = await this.getAll(target); - await this.storage.set(channel.id, channel); + await this.storage.set(encodeURIComponent(channel.id), channel); channels.push(channel.id); - await this.storage.set(channel.topic, channels); + await this.storage.set(encodeURIComponent(channel.topic), channels); }); } public async update(channel: NotificationChannel): Promise { return this.locker.withWriteLock(this.getLockKey(channel.id), async(): Promise => { - const oldChannel = await this.storage.get(channel.id); + const oldChannel = await this.storage.get(encodeURIComponent(channel.id)); if (oldChannel) { if (!this.isChannel(oldChannel)) { @@ -70,7 +71,7 @@ export class KeyValueChannelStorage implements NotificationChannelStorage { } } - await this.storage.set(channel.id, channel); + await this.storage.set(encodeURIComponent(channel.id), channel); }); } @@ -100,12 +101,12 @@ export class KeyValueChannelStorage implements NotificationChannelStorage { } else { channels.splice(idx, 1); if (channels.length > 0) { - await this.storage.set(channel.topic, channels); + await this.storage.set(encodeURIComponent(channel.topic), channels); } else { - await this.storage.delete(channel.topic); + await this.storage.delete(encodeURIComponent(channel.topic)); } } - await this.storage.delete(channel.id); + await this.storage.delete(encodeURIComponent(channel.id)); }); } diff --git a/test/unit/server/notifications/KeyValueChannelStorage.test.ts b/test/unit/server/notifications/KeyValueChannelStorage.test.ts index bd1dbab75..9e64492a4 100644 --- a/test/unit/server/notifications/KeyValueChannelStorage.test.ts +++ b/test/unit/server/notifications/KeyValueChannelStorage.test.ts @@ -1,4 +1,3 @@ -import { v4 } from 'uuid'; import type { ResourceIdentifier } from '../../../../src/http/representation/ResourceIdentifier'; import type { Logger } from '../../../../src/logging/Logger'; import { getLoggerFor } from '../../../../src/logging/LogUtil'; @@ -8,7 +7,6 @@ import type { KeyValueStorage } from '../../../../src/storage/keyvalue/KeyValueS import type { ReadWriteLocker } from '../../../../src/util/locking/ReadWriteLocker'; import resetAllMocks = jest.resetAllMocks; -jest.mock('uuid', (): any => ({ v4: (): string => '4c9b88c1-7502-4107-bb79-2a3a590c7aa3' })); jest.mock('../../../../src/logging/LogUtil', (): any => { const logger: Logger = { info: jest.fn(), error: jest.fn() } as any; return { getLoggerFor: (): Logger => logger }; @@ -17,7 +15,10 @@ jest.mock('../../../../src/logging/LogUtil', (): any => { describe('A KeyValueChannelStorage', (): void => { const logger = getLoggerFor('mock'); const topic = 'http://example.com/foo'; + const encodedTopic = encodeURIComponent(topic); const identifier = { path: topic }; + const id = 'http://example.com/.notifications/123465'; + const encodedId = encodeURIComponent(id); let channel: NotificationChannel; let internalMap: Map; let internalStorage: KeyValueStorage; @@ -27,7 +28,7 @@ describe('A KeyValueChannelStorage', (): void => { beforeEach(async(): Promise => { resetAllMocks(); channel = { - id: `WebSocketChannel2023:${v4()}:http://example.com/foo`, + id, topic, type: 'WebSocketChannel2023', }; @@ -36,7 +37,7 @@ describe('A KeyValueChannelStorage', (): void => { internalStorage = internalMap as any; locker = { - withWriteLock: jest.fn(async (id: ResourceIdentifier, whileLocked: () => T | Promise): + withWriteLock: jest.fn(async (rid: ResourceIdentifier, whileLocked: () => T | Promise): Promise => whileLocked()), withReadLock: jest.fn(), }; @@ -52,6 +53,7 @@ describe('A KeyValueChannelStorage', (): void => { it('returns the matching channel.', async(): Promise => { await storage.add(channel); await expect(storage.get(channel.id)).resolves.toEqual(channel); + expect(internalMap.get(encodedId)).toEqual(channel); }); it('deletes expired channel.', async(): Promise => { @@ -77,9 +79,9 @@ describe('A KeyValueChannelStorage', (): void => { it('adds the channel and adds its id to the topic collection.', async(): Promise => { await expect(storage.add(channel)).resolves.toBeUndefined(); expect(internalMap.size).toBe(2); - expect([ ...internalMap.values() ]).toEqual(expect.arrayContaining([ - [ channel.id ], - channel, + expect([ ...internalMap.entries() ]).toEqual(expect.arrayContaining([ + [ encodedTopic, [ channel.id ]], + [ encodedId, channel ], ])); }); }); @@ -110,14 +112,12 @@ describe('A KeyValueChannelStorage', (): void => { it('rejects update request targeting a non-channel value.', async(): Promise => { await storage.add(channel); - // Looking for the key so this test doesn't depend on the internal keys used - const id = [ ...internalMap.entries() ].find((entry): boolean => Array.isArray(entry[1]))![0]; const newChannel = { ...channel, - id, + id: topic, }; await expect(storage.update(newChannel)).rejects - .toThrow(`Trying to update ${id} which is not a NotificationChannel.`); + .toThrow(`Trying to update ${topic} which is not a NotificationChannel.`); }); }); @@ -125,16 +125,16 @@ describe('A KeyValueChannelStorage', (): void => { it('removes the channel and its reference.', async(): Promise => { const channel2 = { ...channel, - id: 'differentId', + id: 'http://example.com/.notifications/9999999', }; await storage.add(channel); await storage.add(channel2); expect(internalMap.size).toBe(3); await expect(storage.delete(channel.id)).resolves.toBe(true); expect(internalMap.size).toBe(2); - expect([ ...internalMap.values() ]).toEqual(expect.arrayContaining([ - [ channel2.id ], - channel2, + expect([ ...internalMap.entries() ]).toEqual(expect.arrayContaining([ + [ encodedTopic, [ channel2.id ]], + [ encodeURIComponent('http://example.com/.notifications/9999999'), channel2 ], ])); }); @@ -150,9 +150,7 @@ describe('A KeyValueChannelStorage', (): void => { it('logs an error if the target can not be found in the list of references.', async(): Promise => { await storage.add(channel); - // Looking for the key so this test doesn't depend on the internal keys used - const id = [ ...internalMap.entries() ].find((entry): boolean => Array.isArray(entry[1]))![0]; - internalMap.set(id, []); + internalMap.set(encodedTopic, []); await expect(storage.delete(channel.id)).resolves.toBe(true); expect(logger.error).toHaveBeenCalledTimes(2); });