Fix: [M3-6800] Firewall custom port validation (#9336)

* Fix: [M3-6800] improve FW custom ports validation * Fix: [M3-6800] increased test coverage * Added changeset: Firewall custom ports validation * Added changeset: Firewall custom port validation * Fix: [M3-6800] cleanup * Fix: [M3-6800] moaaar cleanup * Fix: [M3-6800] better naming conventions and JSDoc * Fix: [M3-6800] Improved feedback
linode · Jun 29, 2023 · 0dbd58e · 0dbd58e
1 parent a83ec7a
commit 0dbd58e
Show file tree

Hide file tree

Showing 5 changed files with 143 additions and 23 deletions.
diff --git a/packages/manager/.changeset/pr-9336-fixed-1687982995866.md b/packages/manager/.changeset/pr-9336-fixed-1687982995866.md
@@ -0,0 +1,5 @@
+---
+"@linode/manager": Fixed
+---
+
+Firewall custom ports validation ([#9336](https://github.com/linode/manager/pull/9336))
diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.test.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.test.tsx
@@ -121,7 +121,8 @@ describe('utilities', () => {
         label: 'Firewalllabel',
         addresses: 'All IPv4',
       };
-      expect(validateForm({ protocol: 'TCP', ports: '1', ...rest })).toEqual(
+      // SUCCESS CASES
+      expect(validateForm({ protocol: 'TCP', ports: '1234', ...rest })).toEqual(
         {}
       );
       expect(
@@ -134,28 +135,46 @@ describe('utilities', () => {
         {}
       );
       expect(
-        validateForm({ protocol: 'TCP', ports: 'abc', ...rest })
-      ).toHaveProperty(
-        'ports',
-        'Ports must be an integer, range of integers, or a comma-separated list of integers.'
-      );
-      expect(
-        validateForm({ protocol: 'TCP', ports: '1--20', ...rest })
-      ).toHaveProperty(
-        'ports',
-        'Ports must be an integer, range of integers, or a comma-separated list of integers.'
-      );
+        validateForm({
+          protocol: 'TCP',
+          ports: '1,2,3,4,5,6,7,8,9,10,11,12,13,14,15',
+          ...rest,
+        })
+      ).toEqual({});
       expect(
         validateForm({ protocol: 'TCP', ports: '1-2,3-4', ...rest })
+      ).toEqual({});
+      expect(
+        validateForm({ protocol: 'TCP', ports: '1,5-12', ...rest })
+      ).toEqual({});
+      // FAILURE CASES
+      expect(
+        validateForm({ protocol: 'TCP', ports: '1,21-12', ...rest })
       ).toHaveProperty(
         'ports',
-        'Ports must be an integer, range of integers, or a comma-separated list of integers.'
+        'Range must start with a smaller number and end with a larger number'
       );
+      expect(
+        validateForm({ protocol: 'TCP', ports: '1-21-45', ...rest })
+      ).toHaveProperty('ports', 'Ranges must have 2 values');
+      expect(
+        validateForm({ protocol: 'TCP', ports: 'abc', ...rest })
+      ).toHaveProperty('ports', 'Must be 1-65535');
+      expect(
+        validateForm({ protocol: 'TCP', ports: '1--20', ...rest })
+      ).toHaveProperty('ports', 'Must be 1-65535');
       expect(
         validateForm({ protocol: 'TCP', ports: '-20', ...rest })
+      ).toHaveProperty('ports', 'Must be 1-65535');
+      expect(
+        validateForm({
+          protocol: 'TCP',
+          ports: '1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16',
+          ...rest,
+        })
       ).toHaveProperty(
         'ports',
-        'Ports must be an integer, range of integers, or a comma-separated list of integers.'
+        'Number of ports or port range endpoints exceeded. Max allowed is 15'
       );
     });
     it('validates label', () => {

diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts
@@ -12,8 +12,8 @@ import {
   predefinedFirewallFromRule,
 } from 'src/features/Firewalls/shared';
 import {
-  CUSTOM_PORTS_VALIDATION_REGEX,
   CUSTOM_PORTS_ERROR_MESSAGE,
+  isCustomPortsValid,
 } from '@linode/validation';
 import type {
   FirewallRuleProtocol,
@@ -332,7 +332,7 @@ export const validateForm = ({
 
   if ((protocol === 'ICMP' || protocol === 'IPENCAP') && ports) {
     errors.ports = `Ports are not allowed for ${protocol} protocols.`;
-  } else if (ports && !ports.match(CUSTOM_PORTS_VALIDATION_REGEX)) {
+  } else if (ports && !isCustomPortsValid(ports)) {
     errors.ports = CUSTOM_PORTS_ERROR_MESSAGE;
   }
 

diff --git a/packages/validation/.changeset/pr-9336-fixed-1687983041491.md b/packages/validation/.changeset/pr-9336-fixed-1687983041491.md
@@ -0,0 +1,5 @@
+---
+"@linode/validation": Fixed
+---
+
+Firewall custom port validation ([#9336](https://github.com/linode/manager/pull/9336))
diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts
@@ -5,9 +5,6 @@ import { array, mixed, number, object, string } from 'yup';
 
 export const IP_ERROR_MESSAGE =
   'Must be a valid IPv4 or IPv6 address or range.';
-export const CUSTOM_PORTS_ERROR_MESSAGE =
-  'Ports must be an integer, range of integers, or a comma-separated list of integers.';
-export const CUSTOM_PORTS_VALIDATION_REGEX = /^(?:\d+|\d+-\d+|(?:\d+,\s*)*\d+)$/;
 
 export const validateIP = (ipAddress?: string | null): boolean => {
   if (!ipAddress) {
@@ -42,10 +39,104 @@ export const ipAddress = string().test({
   test: validateIP,
 });
 
-export const validateFirewallPorts = string().matches(
-  CUSTOM_PORTS_VALIDATION_REGEX,
-  CUSTOM_PORTS_ERROR_MESSAGE
-);
+export let CUSTOM_PORTS_ERROR_MESSAGE = '';
+
+/**
+ * @param port
+ * @returns boolean
+ * @description Validates a single port or port range and sets the error message
+ */
+const validatePort = (port: string): boolean => {
+  CUSTOM_PORTS_ERROR_MESSAGE =
+    'Ports must be an integer, range of integers, or a comma-separated list of integers.';
+
+  if (!port) {
+    CUSTOM_PORTS_ERROR_MESSAGE = 'Must be 1-65535';
+    return false;
+  }
+
+  const convertedPort = parseInt(port, 10);
+  if (!(1 <= convertedPort && convertedPort <= 65535)) {
+    CUSTOM_PORTS_ERROR_MESSAGE = 'Must be 1-65535';
+    return false;
+  }
+
+  if (port.startsWith('0')) {
+    CUSTOM_PORTS_ERROR_MESSAGE = 'Port must not have leading zeroes';
+    return false;
+  }
+
+  if (String(convertedPort) !== port) {
+    return false;
+  }
+
+  return true;
+};
+
+/**
+ * @param ports
+ * @returns boolean
+ * @description Validates a comma-separated list of ports and port ranges and sets the error message
+ */
+export const isCustomPortsValid = (ports: string): boolean => {
+  const portList = ports?.split(',') || [];
+  let portLimitCount = 0;
+
+  for (const port of portList) {
+    const cleanedPort = port.trim();
+
+    if (cleanedPort.includes('-')) {
+      const portRange = cleanedPort.split('-');
+
+      if (!validatePort(portRange[0]) || !validatePort(portRange[1])) {
+        return false;
+      }
+
+      if (portRange.length !== 2) {
+        CUSTOM_PORTS_ERROR_MESSAGE = 'Ranges must have 2 values';
+        return false;
+      }
+
+      if (parseInt(portRange[0], 10) >= parseInt(portRange[1], 10)) {
+        CUSTOM_PORTS_ERROR_MESSAGE =
+          'Range must start with a smaller number and end with a larger number';
+        return false;
+      }
+
+      portLimitCount += 2;
+    } else {
+      if (!validatePort(cleanedPort)) {
+        return false;
+      }
+      portLimitCount++;
+    }
+  }
+
+  if (portLimitCount > 15) {
+    CUSTOM_PORTS_ERROR_MESSAGE =
+      'Number of ports or port range endpoints exceeded. Max allowed is 15';
+    return false;
+  }
+
+  return true;
+};
+
+const validateFirewallPorts = string().test({
+  name: 'firewall-ports',
+  message: CUSTOM_PORTS_ERROR_MESSAGE,
+  test: (value) => {
+    if (!value) {
+      return false;
+    }
+
+    try {
+      isCustomPortsValid(value);
+    } catch (err) {
+      return false;
+    }
+    return true;
+  },
+});
 
 const validFirewallRuleProtocol = ['ALL', 'TCP', 'UDP', 'ICMP', 'IPENCAP'];
 export const FirewallRuleTypeSchema = object().shape({