chore: correct port validation

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
containers · May 6, 2024 · 6c3f417 · 6c3f417
1 parent fff043e
commit 6c3f417
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 157 deletions.
diff --git a/packages/main/src/plugin/index.ts b/packages/main/src/plugin/index.ts
@@ -163,7 +163,7 @@ import { Troubleshooting } from './troubleshooting.js';
 import type { IDisposable } from './types/disposable.js';
 import type { Deferred } from './util/deferred.js';
 import { Exec } from './util/exec.js';
-import { getFreePort, getFreePortRange, isFreePort } from './util/port.js';
+import { getFreePort, getFreePortRange, isFreePort, type PortStatus } from './util/port.js';
 import { ViewRegistry } from './view-registry.js';
 import { WebviewRegistry } from './webview/webview-registry.js';
 import { WelcomeInit } from './welcome/welcome-init.js';
@@ -1325,7 +1325,7 @@ export class PluginSystem {
       return getFreePortRange(rangeSize);
     });
 
-    this.ipcHandle('system:is-port-free', async (_, port: number): Promise<boolean> => {
+    this.ipcHandle('system:is-port-free', async (_, port: number): Promise<PortStatus> => {
       return isFreePort(port);
     });
 

diff --git a/packages/main/src/plugin/util/port.spec.ts b/packages/main/src/plugin/util/port.spec.ts
@@ -37,8 +37,8 @@ test('return valid port range', async () => {
   expect(isNaN(endRange)).toBe(false);
 
   expect(endRange + 1 - startRange).toBe(3);
-  expect(await port.isFreePort(startRange)).toBe(true);
-  expect(await port.isFreePort(endRange)).toBe(true);
+  expect(await port.isFreePort(startRange)).toStrictEqual({ available: true });
+  expect(await port.isFreePort(endRange)).toStrictEqual({ available: true });
 });
 
 test.each(hosts)(
@@ -75,8 +75,8 @@ test.each(hosts)(
 
     expect(startNewRange > endRange).toBe(true);
     expect(endNewRange + 1 - startNewRange).toBe(3);
-    expect(await port.isFreePort(startNewRange)).toBe(true);
-    expect(await port.isFreePort(endNewRange)).toBe(true);
+    expect(await port.isFreePort(startNewRange)).toStrictEqual({ available: true });
+    expect(await port.isFreePort(endNewRange)).toStrictEqual({ available: true });
   },
 );
 
@@ -85,7 +85,7 @@ test('return first empty port, no port is used', async () => {
   const freePort = await port.getFreePort(start);
 
   expect(freePort).toBe(start);
-  expect(await port.isFreePort(freePort)).toBe(true);
+  expect(await port.isFreePort(freePort)).toStrictEqual({ available: true });
 });
 
 test.each(hosts)(
@@ -104,7 +104,7 @@ test.each(hosts)(
     server.close();
 
     expect(freePort).toBe(port20001);
-    expect(await port.isFreePort(freePort)).toBe(true);
+    expect(await port.isFreePort(freePort)).toStrictEqual({ available: true });
   },
 );
 
@@ -129,18 +129,27 @@ test.each(hosts)(
     server2.close();
 
     expect(freePort).toBe(port20002);
-    expect(await port.isFreePort(freePort)).toBe(true);
+    expect(await port.isFreePort(freePort)).toStrictEqual({ available: true });
   },
 );
 
-test('fails with range error if trying to get a port which is over upper range', async () => {
-  await expect(port.getFreePort(200000)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
-});
-
 test('fails with range error if value is over upper range', async () => {
-  await expect(port.isFreePort(200000)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
+  expect(await port.isFreePort(200000)).toStrictEqual({
+    available: false,
+    message: 'The port must have an integer value within the range from 1025 to 65535.',
+  });
 });
 
 test('fails with range error if value is less lower range', async () => {
-  await expect(port.isFreePort(-1)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
+  expect(await port.isFreePort(-1)).toStrictEqual({
+    available: false,
+    message: 'The port must be greater than 1024.',
+  });
+});
+
+test('should return message that user is trying to check unprivileged port', async () => {
+  expect(await port.isFreePort(1)).toStrictEqual({
+    available: false,
+    message: 'The port must be greater than 1024.',
+  });
 });
diff --git a/packages/main/src/plugin/util/port.ts b/packages/main/src/plugin/util/port.ts
@@ -19,6 +19,11 @@
 import * as net from 'node:net';
 import { type NetworkInterfaceInfoIPv4, networkInterfaces } from 'node:os';
 
+export interface PortStatus {
+  available: boolean;
+  message?: string;
+}
+
 /**
  * Find a free port starting from the given port
  */
@@ -28,7 +33,7 @@ export async function getFreePort(port = 0): Promise<number> {
   }
   let isFree = false;
   while (!isFree) {
-    isFree = await isFreePort(port);
+    isFree = (await isFreePort(port)).available;
     if (!isFree) {
       port++;
     }
@@ -45,7 +50,8 @@ export async function getFreePortRange(rangeSize: number): Promise<string> {
   let startPort = port;
 
   do {
-    if (await isFreePort(port)) {
+    const portStatus = await isFreePort(port);
+    if (portStatus.available) {
       ++port;
     } else {
       ++port;
@@ -56,24 +62,62 @@ export async function getFreePortRange(rangeSize: number): Promise<string> {
   return `${startPort}-${port - 1}`;
 }
 
-function isFreeAddressPort(address: string, port: number): Promise<boolean> {
+function isFreeAddressPort(address: string, port: number): Promise<PortStatus> {
   const server = net.createServer();
-  return new Promise((resolve, reject) =>
+  return new Promise(resolve =>
     server
-      .on('error', (error: NodeJS.ErrnoException) => (error.code === 'EADDRINUSE' ? resolve(false) : reject(error)))
-      .on('listening', () => server.close(() => resolve(true)))
+      .on('error', (error: NodeJS.ErrnoException) => {
+        if (error.code === 'EADDRINUSE') {
+          resolve({
+            available: false,
+            message: `Port ${port} is already in use.`,
+          });
+        } else if (error.code === 'EACCES') {
+          resolve({
+            available: false,
+            message: 'Operation require administrative privileges.',
+          });
+        } else {
+          resolve({
+            available: false,
+            message: `Failed to check port status: ${error}`,
+          });
+        }
+      })
+      .on('listening', () => server.close(() => resolve({ available: true })))
       .listen(port, address),
   );
 }
 
-export async function isFreePort(port: number): Promise<boolean> {
+export async function isFreePort(port: number): Promise<PortStatus> {
+  if (isNaN(port) || port > 65535) {
+    return {
+      available: false,
+      message: 'The port must have an integer value within the range from 1025 to 65535.',
+    };
+  } else if (port < 1024) {
+    return {
+      available: false,
+      message: 'The port must be greater than 1024.',
+    };
+  }
+
   const intfs = getIPv4Interfaces();
   // Test this special interface separately, or it will interfere with other tests done in parallel
-  if (!(await isFreeAddressPort('0.0.0.0', port))) {
-    return false;
+  const portCheckStatus = await isFreeAddressPort('0.0.0.0', port);
+  if (!portCheckStatus.available) {
+    return portCheckStatus;
   }
   const checkInterfaces = await Promise.all(intfs.map(intf => isFreeAddressPort(intf, port)));
-  return checkInterfaces.every(element => element === true);
+  const combinedPortChecks = checkInterfaces.filter(check => !check.available);
+  if (combinedPortChecks && combinedPortChecks.length > 0) {
+    // Means, that some check was not successful and port is not available
+    return {
+      available: false,
+      message: combinedPortChecks.map(check => check.message).join('\n'),
+    };
+  }
+  return { available: true };
 }
 
 function getIPv4Interfaces(): string[] {

diff --git a/packages/preload/src/index.ts b/packages/preload/src/index.ts
@@ -100,6 +100,7 @@ import type { MessageBoxOptions, MessageBoxReturnValue } from '../../main/src/pl
 import type { ExtensionBanner, RecommendedRegistry } from '../../main/src/plugin/recommendations/recommendations-api';
 import type { StatusBarEntryDescriptor } from '../../main/src/plugin/statusbar/statusbar-registry';
 import type { IDisposable } from '../../main/src/plugin/types/disposable';
+import type { PortStatus } from '../../main/src/plugin/util/port';
 import { Deferred } from './util/deferred';
 
 export type DialogResultCallback = (openDialogReturnValue: Electron.OpenDialogReturnValue) => void;
@@ -1416,7 +1417,7 @@ export function initExposure(): void {
     return ipcInvoke('system:get-free-port-range', rangeSize);
   });
 
-  contextBridge.exposeInMainWorld('isFreePort', async (port: number): Promise<boolean> => {
+  contextBridge.exposeInMainWorld('isFreePort', async (port: number): Promise<PortStatus> => {
     return ipcInvoke('system:is-port-free', port);
   });
 

diff --git a/packages/renderer/src/lib/image/RunImage.spec.ts b/packages/renderer/src/lib/image/RunImage.spec.ts
@@ -332,6 +332,7 @@ describe('RunImage', () => {
   });
 
   test('Expect to see an error if the container/host ranges have different size', async () => {
+    (window.isFreePort as Mock).mockResolvedValue({ available: true });
     router.goto('/basic');
 
     await createRunImage(undefined, ['command1', 'command2']);
@@ -352,6 +353,9 @@ describe('RunImage', () => {
     await userEvent.clear(containerInput);
     await userEvent.keyboard('9000-9003');
 
+    // wait onPortInputTimeout (500ms) triggers
+    await new Promise(resolve => setTimeout(resolve, 600));
+
     const button = screen.getByRole('button', { name: 'Start Container' });
 
     await fireEvent.click(button);
@@ -464,86 +468,8 @@ describe('RunImage', () => {
     );
   });
 
-  test('Expect "start container" button to be disabled if user adds a port which is invalid (lower than 0)', async () => {
-    router.goto('/basic');
-
-    await createRunImage(undefined, ['command1', 'command2']);
-
-    const link1 = screen.getByRole('link', { name: 'Basic' });
-    await fireEvent.click(link1);
-
-    const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
-    await fireEvent.click(customMappingButton);
-
-    const hostInput = screen.getByLabelText('host port');
-    await userEvent.click(hostInput);
-    await userEvent.clear(hostInput);
-    // adds a negative port
-    await userEvent.keyboard('-1');
-
-    const containerInput = screen.getByLabelText('container port');
-    await userEvent.click(containerInput);
-    await userEvent.clear(containerInput);
-    await userEvent.keyboard('80');
-
-    const button = screen.getByRole('button', { name: 'Start Container' });
-    expect((button as HTMLButtonElement).disabled).toBeTruthy();
-  });
-
-  test('Expect "start container" button to be disabled if user adds a port which is invalid (over upper limit > 65535)', async () => {
-    router.goto('/basic');
-
-    await createRunImage(undefined, ['command1', 'command2']);
-
-    const link1 = screen.getByRole('link', { name: 'Basic' });
-    await fireEvent.click(link1);
-
-    const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
-    await fireEvent.click(customMappingButton);
-
-    const hostInput = screen.getByLabelText('host port');
-    await userEvent.click(hostInput);
-    await userEvent.clear(hostInput);
-    // adds a negative port
-    await userEvent.keyboard('71000');
-
-    const containerInput = screen.getByLabelText('container port');
-    await userEvent.click(containerInput);
-    await userEvent.clear(containerInput);
-    await userEvent.keyboard('80');
-
-    const button = screen.getByRole('button', { name: 'Start Container' });
-    expect((button as HTMLButtonElement).disabled).toBeTruthy();
-  });
-
-  test('Expect "start container" button to be disabled if user adds a port which is invalid (isNaN)', async () => {
-    router.goto('/basic');
-
-    await createRunImage(undefined, ['command1', 'command2']);
-
-    const link1 = screen.getByRole('link', { name: 'Basic' });
-    await fireEvent.click(link1);
-
-    const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
-    await fireEvent.click(customMappingButton);
-
-    const hostInput = screen.getByLabelText('host port');
-    await userEvent.click(hostInput);
-    await userEvent.clear(hostInput);
-    // adds a negative port
-    await userEvent.keyboard('test');
-
-    const containerInput = screen.getByLabelText('container port');
-    await userEvent.click(containerInput);
-    await userEvent.clear(containerInput);
-    await userEvent.keyboard('80');
-
-    const button = screen.getByRole('button', { name: 'Start Container' });
-    expect((button as HTMLButtonElement).disabled).toBeTruthy();
-  });
-
-  test('Expect "start container" button to be disabled if user adds a port which is NOT free', async () => {
-    (window.isFreePort as Mock).mockResolvedValue(false);
+  test('Expect "start container" button to be disabled when port is not free', async () => {
+    (window.isFreePort as Mock).mockResolvedValue({ available: false, message: 'Error message' });
     router.goto('/basic');
 
     await createRunImage(undefined, ['command1', 'command2']);
@@ -571,34 +497,4 @@ describe('RunImage', () => {
     const button = screen.getByRole('button', { name: 'Start Container' });
     expect((button as HTMLButtonElement).disabled).toBeTruthy();
   });
-
-  test('Expect "start container" button to be enabled if user adds a port which is valid and free', async () => {
-    (window.isFreePort as Mock).mockResolvedValue(true);
-    router.goto('/basic');
-
-    await createRunImage(undefined, ['command1', 'command2']);
-
-    const link1 = screen.getByRole('link', { name: 'Basic' });
-    await fireEvent.click(link1);
-
-    const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
-    await fireEvent.click(customMappingButton);
-
-    const hostInput = screen.getByLabelText('host port');
-    await userEvent.click(hostInput);
-    await userEvent.clear(hostInput);
-    // adds a negative port
-    await userEvent.keyboard('8080');
-
-    const containerInput = screen.getByLabelText('container port');
-    await userEvent.click(containerInput);
-    await userEvent.clear(containerInput);
-    await userEvent.keyboard('80');
-
-    // wait onPortInputTimeout (500ms) triggers
-    await new Promise(resolve => setTimeout(resolve, 600));
-
-    const button = screen.getByRole('button', { name: 'Start Container' });
-    expect((button as HTMLButtonElement).disabled).toBeFalsy();
-  });
 });
diff --git a/packages/renderer/src/lib/image/RunImage.svelte b/packages/renderer/src/lib/image/RunImage.svelte
@@ -599,33 +599,14 @@ function onPortInput(event: Event, portInfo: PortInfo, updateUI: () => void) {
   const target = event.currentTarget as HTMLInputElement;
   // convert string to number
   const _value: number = Number(target.value);
-  // if number is not valid (NaN or outside the value range), set the error
-  if (isNaN(_value) || _value < 0 || _value > 65535) {
-    portInfo.error = 'port should be >= 0 and < 65536';
-    updateUI();
-    return;
-  }
   onPortInputTimeout = setTimeout(() => {
-    isPortFree(_value).then(isFree => {
-      portInfo.error = isFree;
+    window.isFreePort(_value).then(portStatus => {
+      portInfo.error = !portStatus.available ? portStatus.message! : '';
       updateUI();
     });
   }, 500);
 }
 
-function isPortFree(port: number): Promise<string> {
-  return window
-    .isFreePort(port)
-    .then(isFree => {
-      if (!isFree) {
-        return `Port ${port} is already in use`;
-      } else {
-        return '';
-      }
-    })
-    .catch((_: unknown) => `Port ${port} is already in use`);
-}
-
 async function assertAllPortAreValid(): Promise<void> {
   const invalidHostPorts = hostContainerPortMappings.filter(pair => pair.hostPort.error);
   const invalidContainerPortMapping = containerPortMapping?.filter(port => port.error) || [];