Skip to content

Commit be4f7e2

Browse files
CLDSRV-623: Perf of checkIp for bucket policy
Move the validateIpRegex function out of the loop to avoid recreating it multiple time for perf.
1 parent 3c09b1a commit be4f7e2

File tree

1 file changed

+35
-12
lines changed

1 file changed

+35
-12
lines changed

lib/api/apiUtils/authorization/permissionChecks.js

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
const { evaluators, actionMaps, RequestContext, requestUtils } = require('arsenal').policies;
22
const { errorInstances } = require('arsenal');
3-
const { parseCIDR, isValid } = require('ipaddr.js');
3+
// const { parseCIDR } = require('ipaddr.js');
44
const constants = require('../../../../constants');
55
const { config } = require('../../../Config');
66

@@ -494,6 +494,17 @@ function validatePolicyResource(bucketName, policy) {
494494
});
495495
}
496496

497+
// Apply the existing IP validation logic to each element
498+
function validateIpRegex(ip) {
499+
if (constants.ipv4Regex.test(ip)) {
500+
return ip.split('.').every(part => parseInt(part, 10) <= 255);
501+
}
502+
if (constants.ipv6Regex.test(ip)) {
503+
return ip.split(':').every(part => part.length <= 4);
504+
}
505+
return false;
506+
}
507+
497508
function checkIp(value) {
498509
const errString = 'Invalid IP address in Conditions';
499510

@@ -507,6 +518,28 @@ function checkIp(value) {
507518
// notation (e.g., xxx.xxx.xxx.xxx/xx for IPv4). Otherwise,
508519
// we would accept different ip formats, which is not
509520
// standard in an AWS use case.
521+
// discard to fix tests ?
522+
523+
/*
524+
As seen in https://scality.atlassian.net/browse/ARSN-477
525+
the `isValid` does the same parsing as `parseCIDR` but
526+
encapsulate in try catch to return a boolean.
527+
The `isValid` in catch is useless and the double try
528+
discards completely any error from ipaddr.parseCIDR.
529+
530+
Meaning in case of invalid ip there will be 2 ipaddr.js parsing,
531+
reducing perf for an ignored error.
532+
533+
Fixing this breaks the tests.
534+
535+
I'm not even sure of the purpose of using ipaddr.js
536+
and using our regex after as our regex seems to be
537+
more restrictive.
538+
539+
ipaddr.js accepts valid ip like:
540+
- 0010.0xa5.1.1
541+
- ::ffff:222.1.41.900
542+
510543
try {
511544
try {
512545
parseCIDR(values[i]);
@@ -516,17 +549,7 @@ function checkIp(value) {
516549
} catch (err) {
517550
return errString;
518551
}
519-
520-
// Apply the existing IP validation logic to each element
521-
const validateIpRegex = (ip) => {
522-
if (constants.ipv4Regex.test(ip)) {
523-
return ip.split('.').every(part => parseInt(part, 10) <= 255);
524-
}
525-
if (constants.ipv6Regex.test(ip)) {
526-
return ip.split(':').every(part => part.length <= 4);
527-
}
528-
return false;
529-
};
552+
*/
530553

531554
if (validateIpRegex(values[i]) !== true) {
532555
return errString;

0 commit comments

Comments
 (0)