Allow inline # comments in whitelist and blacklist import lists. Allow importing IPs and not just CIDR

[p1r473]

The current firewall import whitelist and firewall import blacklist logic only accepts lines that are only a CIDR.

Any line with a trailing inline comment or no CIDR range is silently discarded.

Example entries that fail silently today:

170.114.0.0/16 #Zoom
137.221.104.0/22 #Battle.net / Blizzard
166.117.0.0/16 #Battle.net
166.117.0.0

This leads to:
Import reporting success
Entries appearing partially imported
Some expected CIDRs missing from the active ipset
Traffic still being blocked unexpectedly

Root Cause
Import logic uses a strict anchored regex:

^...$

This requires the entire line to match an IP/CIDR and does not tolerate trailing content such as comments.

Solution
Strip inline comments and blank lines before validation.

This:

• Removes trailing # comments
• Removes empty lines
• Preserves existing IP/CIDR validation logic

Behavior After Change
These entries now import correctly:

170.114.0.0/16 #Zoom
137.221.104.0/22 #Battle.net / Blizzard
166.117.0.0/16 #Battle.net / Hybrid Networks
166.117.0.0

Entries appear in whitelist view imported
Entries populate the active ipset
Matching traffic is no longer blocked

Notes: the whitelist I am importing is my own here https://github.com/p1r473/ipblocklist/blob/main/ipwhitelist.txt

[p1r473]

Allow inline comments in the whitelist import.

Added:
Allow to import cidr or ip address, not just cidr.

[jumpsmm7]

@p1r473 you have to be careful here with how much you are proposing to change. part of skynet relies heavy on skynet blocked ranges which relies on CIDR notation specifically ranges that are greater than /32. It takes those sets and imports them into separate ipsets specifically configured for handling netsets versus ipsets which is better on ram memory resources. Understand that you cannot just propose a pull request putting in your specifications that could potentially break the nature of this capability. Skynet does allow individual IP addresses into a separate pool but you need to notate with /32 like typical ipset/netset lists do.

[p1r473]

@jumpsmm7 should I remove part where I add in non-CIDR support? And keep it to just allowing for inline comments on the imported list?
Cc @Adamm00

[jumpsmm7]

It is really up to @Adamm00 , but it is pretty much a corner stone feature for how the ipsets, netsets, and the stats and what not are set up. There is something bound to break down the line like a trickle down effect. I one time sent a major pull request to repair the features so I have first hand account of how the down hill impact works with changes to these parts of the code.

Here is what I mean with my deep adventures into the skynet abyss.

https://github.com/jumpsmm7/IPSet_ASUS/compare/4c8dbd706c8e04a77e0c3962495204884af0c7f7...d19c538acf85a8b2d11eae2cce475fa3d94f559d
and my earlier ones:
https://github.com/jumpsmm7/IPSet_ASUS/compare/2d4eee8f3e368dc08bbd7ee59ff871542145c243...f7eed021ba449cd781205983f5785e028a50a6ce

[p1r473]

Thank you for your responses and wisdom, I will wait for Adam and I am happy to remove support for the non-CIDR (I can easily just put /32 in my whitelist on individual IPs) but would love at least for the inline comments and bigger blacklist to be approved 🙏🏻✌🏻

[jumpsmm7]

Thank you for your responses and wisdom, I will wait for Adam and I am happy to remove support for the non-CIDR (I can easily just put /24 in my whitelist on individual IPs) but would love at least for the inline comments and bigger blacklist to be approved 🙏🏻✌🏻

I definitely agree with you about the bigger blocklists. As everybody moves to the newer routers it may be more feasable. Maybe we should place a router model check in place for older generation routers versus newer generation routers so older gen get the lesser value, and newer gen get the higher value.

[ -z "$(nvram get odmpid)" ] && ROUTER_MODEL="$(nvram get productid)" || ROUTER_MODEL="$(nvram get odmpid)"
or you could do it based on router arch.

eg.

ROUTER_ARCH="$(uname -m)"
case "${ROUTER_ARCH}" in
"aarch64" | "arm64")
BLK_VALUE="32"
;;
"armv7l")
BLK_VALUE="16"
;;
esac

then it will become

"$((65536 * BLK_VALUE))"
@p1r473 that will make it be compatible for newer and older generation routers. I think 16 was chosen based on the older generation models.

[p1r473]

I used AI to help review the relevant firewall paths, so please take with a grain of salt

I double checked the import behavior, and I do not think this PR makes the import path less optimized.

For whitelist imports, bare IPs are not left as bare entries. The PR appends /32 before ipset restore:

if(ip~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$/) ip=ip"/32";

So:

1.2.3.4

is restored as:

1.2.3.4/32

For blacklist imports, the existing split remains intact. Bare IPs and /32 entries continue through Skynet-Blacklist, while /1 through /31 CIDR ranges continue through Skynet-BlockedRanges.

So the behavior remains:

1.2.3.4        -> single host
1.2.3.4/32     -> single host
1.2.3.0/24     -> network range
170.114.0.0/16 -> network range

The PR is not trying to collapse netsets into ipsets or change Skynet’s range optimization model. It only makes imports more forgiving by allowing inline comments and bare host IPs, while preserving the existing host-vs-range handling.

CC @jumpsmm7

[jumpsmm7]

I used AI to help review the relevant firewall paths, so please take with a grain of salt

I double checked the import behavior, and I do not think this PR makes the import path less optimized.

For whitelist imports, bare IPs are not left as bare entries. The PR appends /32 before ipset restore:

if(ip~/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$/) ip=ip"/32";

So:

1.2.3.4

is restored as:

1.2.3.4/32

For blacklist imports, the existing split remains intact. Bare IPs and /32 entries continue through Skynet-Blacklist, while /1 through /31 CIDR ranges continue through Skynet-BlockedRanges.

So the behavior remains:

1.2.3.4        -> single host
1.2.3.4/32     -> single host
1.2.3.0/24     -> network range
170.114.0.0/16 -> network range

The PR is not trying to collapse netsets into ipsets or change Skynet’s range optimization model. It only makes imports more forgiving by allowing inline comments and bare host IPs, while preserving the existing host-vs-range handling.

CC @jumpsmm7
@p1r473 It is already forgiving enough because it is fed straight to

grep -E '^(((25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9]).){3}(25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9]))$' which allows any IP without considering the CIDR

Specifically

Filter_IP() {
grep -E '^(((25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9]).){3}(25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9]))$'
}

By you adding a regex into the awk command you would be only slowing down the processing of the list with no real advantage that isn't already covered.

[p1r473]

Thanks, let me see if I can optimize

[jumpsmm7]

@p1r473
check this out
| awk -v desc="Imported: $imptime" -v reg="^(((25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9])\.){3}(25[0-5]|(2[0-4]|1[0-9]|[1-9]|)[0-9]))$" '{ip=$1; if(ip~/reg/) ip=ip"/32"; printf "add Skynet-Whitelist %s comment \"%s\"\n", ip, desc }' > /tmp/skynet/iplist-filtered.txt this would be an optimized use. It ensures only "real" ip addresses can be used or parsed which prevents firewall restart and skynet restart glitches. otherwise skynet will fail to restart properly because the ipsets will fail to load with a false IP present in the list.

[p1r473]

@jumpsmm7 Thanks, I took another stab at updating the PR based on your feedback.

I also found one issue in the suggested awk example:

if(ip~/reg/)

That matches the literal text reg, not the reg variable. I changed it to:

if(ip ~ reg)

[p1r473]

@Adamm00 fyi! for your review. thank you!