nodejs
net: update net.blocklist to allow file save and file management
Getting very close! In order to land this will need, at a minimum, doc updates and tests. The doc update should indicate that it is initially experimental (search for the term Stability the doc files for examples of what I mean. I would prefer for IPv6 rules to also be supported rather than left as a todo. Once the documentation and tests are added I'll take another review pass.
@alphaleadership ... while this is very close, there are still a number of remaining issues with the type checking and test. This patch should cover everything:
diff --git a/lib/internal/blocklist.js b/lib/internal/blocklist.js
index fa0cdc66aa5..7c94c6fe4da 100644
--- a/lib/internal/blocklist.js
+++ b/lib/internal/blocklist.js
@@ -1,10 +1,9 @@
'use strict';
const {
+ ArrayIsArray,
Boolean,
-
JSONParse,
-
NumberParseInt,
ObjectSetPrototypeOf,
Symbol,
@@ -36,6 +35,7 @@ const { owner_symbol } = internalBinding('symbols');
const {
ERR_INVALID_ARG_VALUE,
+ ERR_INVALID_ARG_TYPE,
} = require('internal/errors').codes;
const { validateInt32, validateString } = require('internal/validators');
@@ -238,11 +238,29 @@ class BlockList {
}
fromJSON(data) {
- if (typeof data === 'string') {
- this.#parseIPInfo(JSONParse(data));
+ // The data argument must be a string, or an array of strings that
+ // is JSON parseable.
+ if (ArrayIsArray(data)) {
+ for (const n of data) {
+ if (typeof n !== 'string') {
+ throw new ERR_INVALID_ARG_TYPE('data', ['string', 'string[]'], data);
+ }
+ }
+ } else if (typeof data !== 'string') {
+ throw new ERR_INVALID_ARG_TYPE('data', ['string', 'string[]'], data);
} else {
- this.#parseIPInfo(data);
+ data = JSONParse(data);
+ if (!ArrayIsArray(data)) {
+ throw new ERR_INVALID_ARG_TYPE('data', ['string', 'string[]'], data);
+ }
+ for (const n of data) {
+ if (typeof n !== 'string') {
+ throw new ERR_INVALID_ARG_TYPE('data', ['string', 'string[]'], data);
+ }
+ }
}
+
+ this.#parseIPInfo(data);
}
diff --git a/test/parallel/test-blocklist.js b/test/parallel/test-blocklist.js
index 4b1bc78cd5a..6895efcc1c0 100644
--- a/test/parallel/test-blocklist.js
+++ b/test/parallel/test-blocklist.js
@@ -287,3 +287,75 @@ const util = require('util');
assert(BlockList.isBlockList(new BlockList()));
assert(!BlockList.isBlockList({}));
}
+
+// Test exporting and importing the rule list to/from JSON
+{
+ const ruleList = [
+ 'Address: IPv4 10.0.0.5',
+ 'Address: IPv6 ::',
+ 'Subnet: IPv4 192.168.1.0/24',
+ 'Subnet: IPv6 8592:757c:efae:4e45::/64',
+ ];
+
+ const test2 = new BlockList();
+ const test3 = new BlockList();
+ const test4 = new BlockList();
+ const test5 = new BlockList();
+
+ const bl = new BlockList();
+ bl.addAddress('10.0.0.5');
+ bl.addAddress('::', 'ipv6');
+ bl.addSubnet('192.168.1.0', 24);
+ bl.addSubnet('8592:757c:efae:4e45::', 64, 'ipv6');
+
+ // Test invalid inputs (input to fromJSON must be an array of
+ // string rules or a serialized json string of an array of
+ // string rules.
+ [
+ 1, null, Symbol(), [1, 2, 3], '123', [Symbol()], new Map(),
+ ].forEach((i) => {
+ assert.throws(() => test2.fromJSON(i), {
+ code: 'ERR_INVALID_ARG_TYPE',
+ });
+ });
+
+ // Invalid rules are ignored.
+ test2.fromJSON(['1', '2', '3']);
+ assert.deepStrictEqual(test2.rules, []);
+
+ // Direct output from toJSON method works
+ test2.fromJSON(bl.toJSON());
+ assert.deepStrictEqual(test2.rules.sort(), ruleList);
+
+ // JSON stringified output works
+ test3.fromJSON(JSON.stringify(bl));
+ assert.deepStrictEqual(test3.rules.sort(), ruleList);
+
+ // A raw array works
+ test4.fromJSON(ruleList);
+ assert.deepStrictEqual(test4.rules.sort(), ruleList);
+
+ // Individual rules work
+ ruleList.forEach((item) => {
+ test5.fromJSON([item]);
+ });
+ assert.deepStrictEqual(test5.rules.sort(), ruleList);
+
+ // Each of the created blocklists should handle the checks identically.
+ [
+ ['10.0.0.5', 'ipv4', true],
+ ['10.0.0.6', 'ipv4', false],
+ ['::', 'ipv6', true],
+ ['::1', 'ipv6', false],
+ ['192.168.1.0', 'ipv4', true],
+ ['193.168.1.0', 'ipv4', false],
+ ['8592:757c:efae:4e45::', 'ipv6', true],
+ ['1111:1111:1111:1111::', 'ipv6', false],
+ ].forEach((i) => {
+ assert.strictEqual(bl.check(i[0], i[1]), i[2]);
+ assert.strictEqual(test2.check(i[0], i[1]), i[2]);
+ assert.strictEqual(test3.check(i[0], i[1]), i[2]);
+ assert.strictEqual(test4.check(i[0], i[1]), i[2]);
+ assert.strictEqual(test5.check(i[0], i[1]), i[2]);
+ });
+}
diff --git a/test/parallel/test-net-blocklist.js b/test/parallel/test-net-blocklist.js
index 06591ead751..7048405aa97 100644
--- a/test/parallel/test-net-blocklist.js
+++ b/test/parallel/test-net-blocklist.js
@@ -3,7 +3,6 @@
const common = require('../common');
const net = require('net');
const assert = require('assert');
-const { BlockList } = net;
const blockList = new net.BlockList();
blockList.addAddress('127.0.0.1');
blockList.addAddress('127.0.0.2');
@@ -66,33 +65,3 @@ function check(err) {
socket.on('error', common.mustCall(check));
}
-
-const data = [
- 'Subnet: IPv4 192.168.1.0/24',
- 'Address: IPv4 10.0.0.5',
- 'Range: IPv4 192.168.2.1-192.168.2.10',
- 'Range: IPv4 10.0.0.1-10.0.0.10',
- 'Subnet: IPv6 2001:0db8:85a3:0000:0000:8a2e:0370:7334/64',
- 'Address: IPv6 2001:0db8:85a3:0000:0000:8a2e:0370:7334',
- 'Range: IPv6 2001:0db8:85a3:0000:0000:8a2e:0370:7334-2001:0db8:85a3:0000:0000:8a2e:0370:7335',
- 'Subnet: IPv6 2001:db8:1234::/48',
- 'Address: IPv6 2001:db8:1234::1',
- 'Range: IPv6 2001:db8:1234::1-2001:db8:1234::10',
-];
-const test = new BlockList();
-const test2 = new BlockList();
-const test3 = new BlockList();
-const test4 = new BlockList();
-test.addAddress('127.0.0.1');
-test.addAddress('192.168.0.1');
-test2.fromJSON(test.toJSON());
-test2.fromJSON(JSON.stringify(test.toJSON()));
-test3.fromJSON(data);
-data.forEach((item) => {
- test4.fromJSON([item]);
-});
-assert.deepStrictEqual(test2.rules, test.rules);
-assert.deepStrictEqual(test3.rules, data);
-data.map((t) => {
- return assert.strictEqual(test3.check(t), true);
-});
\ No newline at end of file
@alphaleadership ... I want to thank you for bearing with us in the review process for this. I know it has taken a while with a lot of back-and-forth, and I know this particular environment is new. I appreciate your patience and willingness to work through it all.
i just need @Ethan-Arrowood to accept the change and this is good for me (i have add a thing i need because i dont want to retro ingeniering express)
Codecov Report
Attention: Patch coverage is 80.80000% with 24 lines in your changes missing coverage. Please review.
Project coverage is 90.10%. Comparing base (
2244a09) to head (5460569). Report is 538 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lib/internal/blocklist.js | 80.80% | 24 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #58087 +/- ##
==========================================
- Coverage 92.26% 90.10% -2.17%
==========================================
Files 325 640 +315
Lines 126838 188551 +61713
Branches 20818 36981 +16163
==========================================
+ Hits 117032 169890 +52858
- Misses 9577 11368 +1791
- Partials 229 7293 +7064
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/blocklist.js | 91.30% <80.80%> (-7.56%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
The CI errors appear to be flaky (unrelated to this change). I'm running again and if it fails again I'll take another look.