From ed872b7c5ed12268984fb1090c09a0a20603b26b Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 16 Feb 2024 16:25:23 +0100 Subject: [PATCH 1/2] CLDSRV-505: update ip check for arrays --- .../authorization/permissionChecks.js | 60 ++++++++++--------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index f31949ecaa..220d414c6c 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -480,40 +480,44 @@ function validatePolicyResource(bucketName, policy) { function checkIp(value) { const errString = 'Invalid IP address in Conditions'; - // these preliminary checks are validating the provided - // ip address against ipaddr.js, the library we use when - // evaluating IP condition keys. It ensures compatibility, - // but additional checks are required to enforce the right - // notation (e.g., xxx.xxx.xxx.xxx/xx for IPv4). Otherwise, - // we would accept different ip formats, which is not - // standard in an AWS use case. - try { + const values = Array.isArray(value) ? value : [value]; + + for (let i = 0; i < values.length; i++) { + // these preliminary checks are validating the provided + // ip address against ipaddr.js, the library we use when + // evaluating IP condition keys. It ensures compatibility, + // but additional checks are required to enforce the right + // notation (e.g., xxx.xxx.xxx.xxx/xx for IPv4). Otherwise, + // we would accept different ip formats, which is not + // standard in an AWS use case. try { - parseCIDR(value); + try { + parseCIDR(values[i]); + } catch (err) { + isValid(values[i]); + } } catch (err) { - isValid(value); + return errString; // Return immediately if an invalid IP is found } - } catch (err) { - return errString; - } - // credit to Theodore John.S - // Medium article: Validating IPv4 and IPv6 Addresses with Ease - // — Unveiling the Power of Validation in JavaScript - const validateIpRegex = (ip) => { - if (constants.ipv4Regex.test(ip)) { - return ip.split('.').every(part => parseInt(part, 10) <= 255); - } - if (constants.ipv6Regex.test(ip)) { - return ip.split(':').every(part => part.length <= 4); - } - return false; - }; + // Apply the existing IP validation logic to each element + const validateIpRegex = (ip) => { + if (constants.ipv4Regex.test(ip)) { + return ip.split('.').every(part => parseInt(part, 10) <= 255); + } + if (constants.ipv6Regex.test(ip)) { + return ip.split(':').every(part => part.length <= 4); + } + return false; + }; - if (validateIpRegex(value) === true) { - return null; + if (validateIpRegex(values[i]) !== true) { + return errString; // Return immediately if an invalid IP is found + } } - return errString; + + // If the function hasn't returned by now, all elements are valid + return null; } // This function checks all bucket policy conditions if the values provided From 3d41397f40c199349400cc5b8a368a7f5d058d14 Mon Sep 17 00:00:00 2001 From: Will Toozs Date: Fri, 16 Feb 2024 16:25:38 +0100 Subject: [PATCH 2/2] CLDSRV-505: update ip check tests for arrays --- tests/unit/auth/permissionChecks.js | 39 ++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/tests/unit/auth/permissionChecks.js b/tests/unit/auth/permissionChecks.js index 35ed7d7c65..788d560797 100644 --- a/tests/unit/auth/permissionChecks.js +++ b/tests/unit/auth/permissionChecks.js @@ -382,17 +382,50 @@ describe('validatePolicyConditions', () => { expected: 'Invalid IP address in Conditions', }, { - description: 'Should return a relevant error message if the ' + - 'condition value type does not match the expected type', + description: 'Should accept arrays of IPs', inputPolicy: { Statement: [{ Condition: { - IpAddress: { 'aws:SourceIp': [] }, // IP should be a string, not an array + IpAddress: { + 'aws:SourceIp': [ + '10.0.11.0/24', + '10.0.1.0/24', + ], + }, + }, + }], + }, + expected: null, + }, + { + description: 'Should return relevant error if one of the IPs in the array is invalid', + inputPolicy: { + Statement: [{ + Condition: { + IpAddress: { + 'aws:SourceIp': [ + '10.0.11.0/24', + '123', + ], + }, }, }], }, expected: 'Invalid IP address in Conditions', }, + { + description: 'Should not return error if array value in IP condition is empty', // this is AWS behavior + inputPolicy: { + Statement: [{ + Condition: { + IpAddress: { + 'aws:SourceIp': [], + }, + }, + }], + }, + expected: null, + }, { description: 'Should return null or a relevant error message ' + 'if multiple conditions are provided in a single statement',