Skip to content

Fix for #6320 #7878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix for #6320 #7878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
006bb13
Fix for #6320
grogi Jun 14, 2025
bf11113
Added comment why we are rethrowing the caught exception.
grogi Jun 14, 2025
b59ff8c
Updates for code review
grogi Jun 22, 2025
28ed05c
Removed invalid import.
grogi Jun 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 127 additions & 6 deletions client/src/__tests__/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { describe, expect, test, afterEach, vi, beforeEach, it } from 'vitest';

import { sortIp, countClientsStatistics, findAddressType, subnetMaskToBitMask } from '../helpers/helpers';
import { sortIp, countClientsStatistics, findAddressType, subnetMaskToBitMask, sortAddress } from '../helpers/helpers';
import { ADDRESS_TYPES } from '../helpers/constants';

describe('sortIp', () => {
function testBaseSortFunction(sortIp: (a: string, b: string) => number) {
describe('ipv4', () => {
test('one octet differ', () => {
const arr = ['127.0.2.0', '127.0.3.0', '127.0.1.0'];
Expand Down Expand Up @@ -257,6 +257,63 @@ describe('sortIp', () => {
});
});

describe('mixed', () => {
test('ipv4, ipv6 in short and long forms and cidr', () => {
const arr = [
'2001:db8:11a3:9d7:0:0:0:1/32',
'192.168.1.2',
'127.0.0.2',
'2001:db8:11a3:9d7::1/128',
'2001:db8:11a3:9d7:0:0:0:1',
'127.0.0.1/12',
'192.168.1.1',
'2001:db8::/32',
'2001:db8:11a3:9d7::1/24',
'192.168.1.2/12',
'2001:db7::/32',
'127.0.0.1',
'2001:db8:11a3:9d7:0:0:0:2',
'192.168.1.1/24',
'2001:db7::/64',
'2001:db7::',
'2001:db8::',
'2001:db8:11a3:9d7:0:0:0:1/128',
'192.168.1.1/12',
'127.0.0.1/32',
'::1',
];
const sortedArr = [
'127.0.0.1/12',
'127.0.0.1/32',
'127.0.0.1',
'127.0.0.2',
'192.168.1.1/12',
'192.168.1.1/24',
'192.168.1.1',
'192.168.1.2/12',
'192.168.1.2',
'::1',
'2001:db7::/32',
'2001:db7::/64',
'2001:db7::',
'2001:db8::/32',
'2001:db8::',
'2001:db8:11a3:9d7::1/24',
'2001:db8:11a3:9d7:0:0:0:1/32',
'2001:db8:11a3:9d7::1/128',
'2001:db8:11a3:9d7:0:0:0:1/128',
'2001:db8:11a3:9d7:0:0:0:1',
'2001:db8:11a3:9d7:0:0:0:2',
];

expect(arr.sort(sortIp)).toStrictEqual(sortedArr);
});
});
}

describe('sortIp', () => {
testBaseSortFunction(sortIp);

describe('invalid input', () => {
const originalWarn = console.warn;

Expand Down Expand Up @@ -293,16 +350,75 @@ describe('sortIp', () => {
expect(arr.sort(sortIp)).toStrictEqual(arr);
});
});
})

describe('mixed', () => {
test('ipv4, ipv6 in short and long forms and cidr', () => {
describe('sortAddress', () => {
testBaseSortFunction(sortAddress);

describe('domain_names_sorting', () => {
test('widcard before other names', () => {
const arr = [
'home.fritz.box',
'*.home.fritz.box',
'adguard-home.fritz.box'
]

const sortedArr = [
'*.home.fritz.box',
'adguard-home.fritz.box',
'home.fritz.box'
];

expect(arr.sort(sortAddress)).toStrictEqual(sortedArr);
})

test('only non-ip strings', () => {
const arr = [
'fritz.box',
'adguard-home.fritz.box',
'foo.bar',
'example.com',
'my.router.local',
'office.lan',
'server.example.org',
'mail.google.com',
'desktop.home',
'printer.office.lan',
'web.internal',
'host123.domain.net',
'api.service.company',
];

const sortedArr = [
'adguard-home.fritz.box',
'api.service.company',
'desktop.home',
'example.com',
'foo.bar',
'fritz.box',
'host123.domain.net',
'mail.google.com',
'my.router.local',
'office.lan',
'printer.office.lan',
'server.example.org',
'web.internal',
];

expect(arr.sort(sortAddress)).toStrictEqual(sortedArr);
});

test('ipv4, ipv6 in short and long forms and cidr, and a few domain names', () => {
const arr = [
'2001:db8:11a3:9d7:0:0:0:1/32',
'192.168.1.2',
'127.0.0.2',
'foo.bar',
'2001:db8:11a3:9d7::1/128',
'2001:db8:11a3:9d7:0:0:0:1',
'127.0.0.1/12',
'fritz.box',
'adguard-home.fritz.box',
'192.168.1.1',
'2001:db8::/32',
'2001:db8:11a3:9d7::1/24',
Expand All @@ -317,6 +433,7 @@ describe('sortIp', () => {
'2001:db8:11a3:9d7:0:0:0:1/128',
'192.168.1.1/12',
'127.0.0.1/32',
'*.home.fritz.net',
'::1',
];
const sortedArr = [
Expand All @@ -341,12 +458,16 @@ describe('sortIp', () => {
'2001:db8:11a3:9d7:0:0:0:1/128',
'2001:db8:11a3:9d7:0:0:0:1',
'2001:db8:11a3:9d7:0:0:0:2',
'*.home.fritz.net',
'adguard-home.fritz.box',
'foo.bar',
'fritz.box',
Comment on lines +461 to +464
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wildcard pattern '*.home.fritz.net' is placed before other domain names in the sorted result, which seems inconsistent with the alphabetical sorting of other non-IP strings. Consider either sorting wildcard patterns alphabetically along with other domain names or documenting the special ordering rule if this is intentional behavior.

];

expect(arr.sort(sortIp)).toStrictEqual(sortedArr);
expect(arr.sort(sortAddress)).toStrictEqual(sortedArr);
});
});
});
})

describe('findAddressType', () => {
it('should return IP type for IP addresses', () => {
Expand Down
4 changes: 2 additions & 2 deletions client/src/components/Filters/Rewrites/Table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React, { Component } from 'react';
import ReactTable from 'react-table';
import { withTranslation } from 'react-i18next';

import { sortIp } from '../../../helpers/helpers';
import { sortAddress } from '../../../helpers/helpers';
import { MODAL_TYPE, TABLES_MIN_ROWS } from '../../../helpers/constants';
import { LocalStorageHelper, LOCAL_STORAGE_KEYS } from '../../../helpers/localStorageHelper';

Expand Down Expand Up @@ -37,7 +37,7 @@ class Table extends Component<TableProps> {
{
Header: this.props.t('answer'),
accessor: 'answer',
sortMethod: sortIp,
sortMethod: sortAddress,
Cell: this.cellWrap,
},
{
Expand Down
107 changes: 66 additions & 41 deletions client/src/helpers/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,69 +771,94 @@ export const getObjectKeysSorted = <T extends Record<string, NestedObject>, K ex
};

/**
* @param ip
* @returns {[IPv4|IPv6, 33|129]}
* Helper function for IP and CIDR comparison (supports both v4 and v6). Creates an array of chunks that can be used for comparison.
* @param address - ip, cidr or domain name to create comparison chunks from
* @returns { (string | number)[] } The array of chunks to compare. Those are either bytes for IPs, or domain name parts.
*/
const getParsedIpWithPrefixLength = (ip: any) => {
const MAX_PREFIX_LENGTH_V4 = 32;
const MAX_PREFIX_LENGTH_V6 = 128;

const parsedIp = ipaddr.parse(ip);
const prefixLength = parsedIp.kind() === 'ipv4' ? MAX_PREFIX_LENGTH_V4 : MAX_PREFIX_LENGTH_V6;

// Increment prefix length to always put IP after CIDR, e.g. 127.0.0.1/32, 127.0.0.1
return [parsedIp, prefixLength + 1];
};

/**
* Helper function for IP and CIDR comparison (supports both v4 and v6)
* @param item - ip or cidr
* @returns {number[]}
*/
const getAddressesComparisonBytes = (item: any) => {
// Sort ipv4 before ipv6
const getAddressesComparisonChunks = (address: string, allowOnlyIpAddresses: boolean) => {
// Sort ipv4 before ipv6. Strings that are not IP addresses will be sorted after both.
const IP_V4_COMPARISON_CODE = 0;
const IP_V6_COMPARISON_CODE = 1;
const NOT_IP_ADDRESS = 2;

const [parsedIp, cidr] = ipaddr.isValid(item) ? getParsedIpWithPrefixLength(item) : ipaddr.parseCIDR(item);
try {
const [parsedIp, cidr] = ipaddr.isValid(address) ? [ ipaddr.parse(address), Number.MAX_SAFE_INTEGER ] : ipaddr.parseCIDR(address);

const [normalizedBytes, ipVersionComparisonCode] =
(parsedIp as IPv4 | IPv6).kind() === 'ipv4'
? [(parsedIp as IPv4).toIPv4MappedAddress().parts, IP_V4_COMPARISON_CODE]
: [(parsedIp as IPv6).parts, IP_V6_COMPARISON_CODE];
const [normalizedBytes, ipVersionComparisonCode] =
parsedIp.kind() === 'ipv4'
? [(parsedIp as IPv4).toIPv4MappedAddress().parts, IP_V4_COMPARISON_CODE]
: [(parsedIp as IPv6).parts, IP_V6_COMPARISON_CODE];

return [ipVersionComparisonCode, ...normalizedBytes, cidr];
return [ipVersionComparisonCode, ...normalizedBytes, cidr];
}
catch (e) {
if (allowOnlyIpAddresses) {
throw new Error(`Invalid address: ${address}. Only IP addresses and CIDRs are allowed.`, { cause: e });
}

return [NOT_IP_ADDRESS, address];
}
};

const getAsElementForSorting = (item: any) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name getAsElementForSorting is misleading since it doesn't return an element for sorting but rather converts the input to a string. Consider renaming to something like getStringForSorting or normalizeItemToString to better reflect its purpose.

// If item is an array, take the first element for sorting
if (Array.isArray(item)) {
item = item[0];
}

// String is expected downstream by ipaddr.isValid and .parseCIDR, we can validate it here
if (typeof item !== 'string') {
console.warn('Expected a string. Got', item, `of type ${typeof item} instead.`);
}

// Make sure we always return a string
return "" + item;
}
Comment on lines +815 to +816
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon at the end of the function.

Suggested change
return "" + item;
}
return "" + item;
}


/**
* Compare function for IP and CIDR sort in ascending order (supports both v4 and v6)
* @param a
* @param b
* @returns {number} -1 | 0 | 1
* Creates a sort function for IP addresses, CIDRs and domain names, with given restrictions.
* Don't use this function directly - use sortIp or sortAddress. react-table occasionally passes `true` as third parameter when it is doing reverse sorting.
* @param {boolean} allowOnlyIpAddresses - if true, only IP addresses and CIDRs will be compared. Invalid IP address would throw an exception, leaving the items in their original order.
* @returns A compare function that can be used in Array.prototype.sort() to sort IP addresses, CIDRs and domain names.
*/
export const sortIp = (a: any, b: any) => {
const sortFunction = (a: any, b: any, allowOnlyIpAddresses: boolean) => {
try {
const comparisonBytesA = Array.isArray(a) ? getAddressesComparisonBytes(a[0]) : getAddressesComparisonBytes(a);
const comparisonBytesB = Array.isArray(b) ? getAddressesComparisonBytes(b[0]) : getAddressesComparisonBytes(b);
const comparisonChunksA = getAddressesComparisonChunks(getAsElementForSorting(a), allowOnlyIpAddresses);
const comparisonChunksB = getAddressesComparisonChunks(getAsElementForSorting(b), allowOnlyIpAddresses);

for (let i = 0; i < comparisonBytesA.length; i += 1) {
const byteA = comparisonBytesA[i];
const byteB = comparisonBytesB[i];
for (let i = 0; i < Math.min(comparisonChunksA.length, comparisonChunksB.length); i++) {
const byteA = comparisonChunksA[i];
const byteB = comparisonChunksB[i];

if (byteA === byteB) {
// eslint-disable-next-line no-continue
continue;
if (byteA !== byteB) {
return byteA > byteB ? 1 : -1;
}
return byteA > byteB ? 1 : -1;
}

return 0;
// If all compared chunks are equal, compare by length. Shorter array should come first.
return comparisonChunksA.length - comparisonChunksB.length;
} catch (e) {
console.warn(e);
return 0;
}
};

/**
* Compare function for IP, CIDR name sort in ascending order (supports both v4 and v6).
* @param a
* @param b
* @returns {number} -1 | 0 | 1
*/
export const sortIp = (a: any, b: any) => sortFunction(a, b, true);

/**
* Compare function for IP, CIDR (supports both v4 and v6) and domain name sort in ascending order.
* @param a
* @param b
* @returns {number} -1 | 0 | 1
*/
export const sortAddress = (a: any, b: any) => sortFunction(a, b, false);

/**
* @param {number} filterId
* @returns {string}
Expand Down