Repository: discordjs/voice
Pull request #82 (2021-03-17 11:59:39)

test(VoiceUDPSocket): unit tests
**Please describe the changes this PR makes and why it should be merged:**

Adds unit tests to VoiceUDPSocket, and also fixes an issue raised in testing. Previously, if an unexpected message was received on the socket during IP discovery, then an unhandled error would be thrown. This has been fixed, the caller of the parsing function now handles errors by discarding bad packets. The parsing function also validates the IP address.


**Status and versioning classification:**

- Code changes have been tested against the Discord API, or there are no code changes

codecov[bot]: # [Codecov](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=h1) Report
> Merging [#82](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=desc) (08d0d20) into [main](https://codecov.io/gh/discordjs/voice/commit/f140bd4a25e00feab2950007d559746492a42052?el=desc) (f140bd4) will **increase** coverage by `3.59%`.
> The diff coverage is `88.88%`.

[![Impacted file tree graph](https://codecov.io/gh/discordjs/voice/pull/82/graphs/tree.svg?width=650&height=150&src=pr&token=u7oQ23UoxX)](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   44.01%   47.61%   +3.59%     
==========================================
  Files          15       15              
  Lines         602      609       +7     
  Branches      133      135       +2     
==========================================
+ Hits          265      290      +25     
+ Misses        335      317      -18     
  Partials        2        2              
```


| [Impacted Files](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/networking/VoiceUDPSocket.ts](https://codecov.io/gh/discordjs/voice/pull/82/diff?src=pr&el=tree#diff-c3JjL25ldHdvcmtpbmcvVm9pY2VVRFBTb2NrZXQudHM=) | `92.59% <88.88%> (+92.59%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=footer). Last update [f140bd4...08d0d20](https://codecov.io/gh/discordjs/voice/pull/82?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

amishshah: Addressed code review @kyranet. As for the malformed packet test, I realised this situation could never actually occur, so I removed the check.

Patch:
diff --git a/src/networking/VoiceUDPSocket.ts b/src/networking/VoiceUDPSocket.ts
index 606bb8b..cfc6854 100644
--- a/src/networking/VoiceUDPSocket.ts
+++ b/src/networking/VoiceUDPSocket.ts
@@ -1,11 +1,12 @@
 import { createSocket, Socket } from 'dgram';
 import { EventEmitter } from 'events';
+import { isIPv4 } from 'net';
 
 /**
  * Stores an IP address and port. Used to store socket details for the local client as well as
  * for Discord.
  */
-interface SocketConfig {
+export interface SocketConfig {
 	ip: string;
 	port: number;
 }
@@ -60,8 +61,11 @@ export class VoiceUDPSocket extends EventEmitter {
 	public performIPDiscovery(ssrc: number): Promise<SocketConfig> {
 		return new Promise((resolve, reject) => {
 			const listener = (message: Buffer) => {
-				resolve(parseLocalPacket(message));
-				this.socket.removeListener('message', listener);
+				try {
+					const packet = parseLocalPacket(message);
+					this.socket.off('message', listener);
+					resolve(packet);
+				} catch {}
 			};
 
 			this.socket.on('message', listener);
@@ -79,11 +83,16 @@ export class VoiceUDPSocket extends EventEmitter {
  *
  * @param message - The received message
  */
-function parseLocalPacket(message: Buffer): SocketConfig {
-	// todo, this function can throw
+export function parseLocalPacket(message: Buffer): SocketConfig {
 	const packet = Buffer.from(message);
 	let ip = '';
 	for (let i = 4; i < packet.indexOf(0, i); i++) ip += String.fromCharCode(packet[i]);
+
+	if (!isIPv4(ip)) {
+		throw new Error('Malformed IP address');
+	}
+
 	const port = parseInt(packet.readUIntLE(packet.length - 2, 2).toString(10), 10);
+
 	return { ip, port };
 }


Test patch:
diff --git a/src/networking/__tests__/VoiceUDPSocket.test.ts b/src/networking/__tests__/VoiceUDPSocket.test.ts
new file mode 100644
index 0000000..8f988fb
--- /dev/null
+++ b/src/networking/__tests__/VoiceUDPSocket.test.ts
@@ -0,0 +1,83 @@
+/* eslint-disable @typescript-eslint/no-empty-function */
+/* eslint-disable @typescript-eslint/no-unused-vars */
+import { createSocket as _createSocket } from 'dgram';
+import EventEmitter from 'events';
+import { parseLocalPacket, VoiceUDPSocket } from '../VoiceUDPSocket';
+jest.mock('dgram');
+
+const createSocket = (_createSocket as unknown) as jest.Mock<typeof _createSocket>;
+
+beforeEach(() => {
+	createSocket.mockReset();
+});
+
+class FakeSocket extends EventEmitter {
+	public send(buffer: Buffer, port: number, address: string) {}
+	public close() {
+		this.emit('close');
+	}
+}
+
+// ip = 94.195.157.2, port = 42381
+const VALID_RESPONSE = Buffer.concat([
+	Buffer.from([0x02, 0x47, 0x1e, 0x00, 0x39, 0x34, 0x2e, 0x31, 0x39, 0x35, 0x2e, 0x31, 0x35, 0x37, 0x2e, 0x32]),
+	Buffer.alloc(52),
+	Buffer.from([0x8d, 0xa5]),
+]);
+
+describe('VoiceUDPSocket#performIPDiscovery', () => {
+	/*
+		Ensures that the UDP socket sends data and parses the response correctly
+	*/
+	test('Resolves and cleans up with a successful flow', async () => {
+		const fake = new FakeSocket();
+		fake.send = jest.fn().mockImplementation((buffer: Buffer, port: number, address: string) => {
+			fake.emit('message', VALID_RESPONSE);
+		});
+		createSocket.mockImplementation((type) => fake as any);
+		const socket = new VoiceUDPSocket({ ip: '1.2.3.4', port: 25565 });
+
+		expect(createSocket).toHaveBeenCalledWith('udp4');
+		await expect(socket.performIPDiscovery(1234)).resolves.toEqual({
+			ip: '94.195.157.2',
+			port: 42381,
+		});
+		// Ensure clean up occurs
+		expect(fake.listenerCount('message')).toBe(0);
+	});
+
+	/*
+		In the case where an unrelated message is received before the IP discovery buffer,
+		the UDP socket should wait indefinitely until the correct buffer arrives.
+	*/
+	test('Waits for a valid response in an unexpected flow', async () => {
+		const fake = new FakeSocket();
+		const fakeResponse = Buffer.from([1, 2, 3, 4, 5]);
+		fake.send = jest.fn().mockImplementation((buffer: Buffer, port: number, address: string) => {
+			fake.emit('message', fakeResponse);
+			setImmediate(() => fake.emit('message', VALID_RESPONSE));
+		});
+		createSocket.mockImplementation((type) => fake as any);
+		const socket = new VoiceUDPSocket({ ip: '1.2.3.4', port: 25565 });
+
+		expect(createSocket).toHaveBeenCalledWith('udp4');
+		await expect(socket.performIPDiscovery(1234)).resolves.toEqual({
+			ip: '94.195.157.2',
+			port: 42381,
+		});
+		// Ensure clean up occurs
+		expect(fake.listenerCount('message')).toBe(0);
+	});
+
+	test('Rejects if socket closes before IP discovery can be completed', async () => {
+		const fake = new FakeSocket();
+		fake.send = jest.fn().mockImplementation((buffer: Buffer, port: number, address: string) => {
+			setImmediate(() => fake.close());
+		});
+		createSocket.mockImplementation((type) => fake as any);
+		const socket = new VoiceUDPSocket({ ip: '1.2.3.4', port: 25565 });
+
+		expect(createSocket).toHaveBeenCalledWith('udp4');
+		await expect(socket.performIPDiscovery(1234)).rejects.toThrowError();
+	});
+});