From c335cb183948ab67cf6d1b2eb0fa1fd4de3ced20 Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 15:54:35 -0400 Subject: [PATCH 1/9] Fix: [M3-6800] improve FW custom ports validation --- .../Rules/FirewallRuleDrawer.utils.ts | 4 +- packages/validation/src/firewalls.schema.ts | 90 +++++++++++++++++-- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts index fad2eb3658b..5351b3fe68b 100644 --- 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, + runCustomPortsValidation, } 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 && !runCustomPortsValidation(ports)) { errors.ports = CUSTOM_PORTS_ERROR_MESSAGE; } diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index b9aa0d36946..38754ca85e1 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -5,8 +5,8 @@ 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_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 => { @@ -42,10 +42,88 @@ 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 = + 'Ports must be an integer, range of integers, or a comma-separated list of integers.'; + +const validatePort = (port: string): boolean => { + 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 (String(convertedPort) !== port) { + CUSTOM_PORTS_ERROR_MESSAGE = 'Port must not have leading zeroes'; + return false; + } + + return true; +}; + +export const runCustomPortsValidation = (value: string): boolean => { + const portList = value?.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 { + runCustomPortsValidation(value); + } catch (err) { + return false; + } + return true; + }, +}); const validFirewallRuleProtocol = ['ALL', 'TCP', 'UDP', 'ICMP', 'IPENCAP']; export const FirewallRuleTypeSchema = object().shape({ From ffd6a142089b0465d6f1397e5033ce3d7775a768 Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 16:05:56 -0400 Subject: [PATCH 2/9] Fix: [M3-6800] increased test coverage --- .../Rules/FirewallRuleDrawer.test.tsx | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.test.tsx b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.test.tsx index 124d37fddb7..2715d6b6de8 100644 --- 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', () => { From 55e61665ca4ee4fd628cd5a95264c0f7a4b3d496 Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 16:09:55 -0400 Subject: [PATCH 3/9] Added changeset: Firewall custom ports validation --- packages/manager/.changeset/pr-9336-fixed-1687982995866.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 packages/manager/.changeset/pr-9336-fixed-1687982995866.md diff --git a/packages/manager/.changeset/pr-9336-fixed-1687982995866.md b/packages/manager/.changeset/pr-9336-fixed-1687982995866.md new file mode 100644 index 00000000000..af3816b78e1 --- /dev/null +++ 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)) From c45465f69e2c454ca9826a2b50c5f28a51e710ad Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 16:10:41 -0400 Subject: [PATCH 4/9] Added changeset: Firewall custom port validation --- .../validation/.changeset/pr-9336-fixed-1687983041491.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 packages/validation/.changeset/pr-9336-fixed-1687983041491.md diff --git a/packages/validation/.changeset/pr-9336-fixed-1687983041491.md b/packages/validation/.changeset/pr-9336-fixed-1687983041491.md new file mode 100644 index 00000000000..69266a068bb --- /dev/null +++ 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)) From fd75dc58051b7dd9fd1b2da0b8fa9d1e706b59cd Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 16:11:21 -0400 Subject: [PATCH 5/9] Fix: [M3-6800] cleanup --- packages/validation/src/firewalls.schema.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index 38754ca85e1..9c73e59639a 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -5,8 +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 => { From 559663aa42585ae3209edd7d2c4a06f6652cef7a Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 16:15:47 -0400 Subject: [PATCH 6/9] Fix: [M3-6800] moaaar cleanup --- packages/validation/src/firewalls.schema.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index 9c73e59639a..f5cc6718a8c 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -5,7 +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_VALIDATION_REGEX = /^(?:\d+|\d+-\d+|(?:\d+,\s*)*\d+)$/; export const validateIP = (ipAddress?: string | null): boolean => { if (!ipAddress) { From d1bed3fd4e38c7553bf53c41a3dda9aa8fe0313d Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Wed, 28 Jun 2023 17:15:45 -0400 Subject: [PATCH 7/9] Fix: [M3-6800] better naming conventions and JSDoc --- .../Rules/FirewallRuleDrawer.utils.ts | 4 ++-- packages/validation/src/firewalls.schema.ts | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts index 5351b3fe68b..e19971e62a5 100644 --- a/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts +++ b/packages/manager/src/features/Firewalls/FirewallDetail/Rules/FirewallRuleDrawer.utils.ts @@ -13,7 +13,7 @@ import { } from 'src/features/Firewalls/shared'; import { CUSTOM_PORTS_ERROR_MESSAGE, - runCustomPortsValidation, + 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 && !runCustomPortsValidation(ports)) { + } else if (ports && !isCustomPortsValid(ports)) { errors.ports = CUSTOM_PORTS_ERROR_MESSAGE; } diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index f5cc6718a8c..df25c4d2a32 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -42,6 +42,11 @@ export const ipAddress = string().test({ export let CUSTOM_PORTS_ERROR_MESSAGE = 'Ports must be an integer, range of integers, or a comma-separated list of integers.'; +/** + * @param port + * @returns boolean + * @description Validates a single port or port range and sets the error message + */ const validatePort = (port: string): boolean => { if (!port) { CUSTOM_PORTS_ERROR_MESSAGE = 'Must be 1-65535'; @@ -62,8 +67,13 @@ const validatePort = (port: string): boolean => { return true; }; -export const runCustomPortsValidation = (value: string): boolean => { - const portList = value?.split(',') || []; +/** + * @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) { @@ -114,7 +124,7 @@ const validateFirewallPorts = string().test({ } try { - runCustomPortsValidation(value); + isCustomPortsValid(value); } catch (err) { return false; } From 17ae8a69abeee330e017933b68eb49d3625b078e Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Thu, 29 Jun 2023 12:16:51 -0400 Subject: [PATCH 8/9] Fix: [M3-6800] Improved feedback --- packages/validation/src/firewalls.schema.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index df25c4d2a32..e1a2eeb6e4f 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -39,8 +39,7 @@ export const ipAddress = string().test({ test: validateIP, }); -export let CUSTOM_PORTS_ERROR_MESSAGE = - 'Ports must be an integer, range of integers, or a comma-separated list of integers.'; +export let CUSTOM_PORTS_ERROR_MESSAGE = ''; /** * @param port @@ -48,6 +47,9 @@ export let CUSTOM_PORTS_ERROR_MESSAGE = * @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; @@ -59,11 +61,15 @@ const validatePort = (port: string): boolean => { return false; } - if (String(convertedPort) !== port) { + if (port.startsWith('0')) { CUSTOM_PORTS_ERROR_MESSAGE = 'Port must not have leading zeroes'; return false; } + if (String(convertedPort) !== port) { + return false; + } + return true; }; From 1e16579ae70beb9f704220008299734553c32201 Mon Sep 17 00:00:00 2001 From: Alban Bailly Date: Thu, 29 Jun 2023 15:15:38 -0400 Subject: [PATCH 9/9] remove unecessary changesets --- packages/manager/.changeset/pr-9336-fixed-1687982995866.md | 5 ----- .../validation/.changeset/pr-9336-fixed-1687983041491.md | 5 ----- 2 files changed, 10 deletions(-) delete mode 100644 packages/manager/.changeset/pr-9336-fixed-1687982995866.md delete mode 100644 packages/validation/.changeset/pr-9336-fixed-1687983041491.md diff --git a/packages/manager/.changeset/pr-9336-fixed-1687982995866.md b/packages/manager/.changeset/pr-9336-fixed-1687982995866.md deleted file mode 100644 index af3816b78e1..00000000000 --- a/packages/manager/.changeset/pr-9336-fixed-1687982995866.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@linode/manager": Fixed ---- - -Firewall custom ports validation ([#9336](https://github.com/linode/manager/pull/9336)) diff --git a/packages/validation/.changeset/pr-9336-fixed-1687983041491.md b/packages/validation/.changeset/pr-9336-fixed-1687983041491.md deleted file mode 100644 index 69266a068bb..00000000000 --- a/packages/validation/.changeset/pr-9336-fixed-1687983041491.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@linode/validation": Fixed ---- - -Firewall custom port validation ([#9336](https://github.com/linode/manager/pull/9336))