fix: support Node.js 20 #16

azu · 2023-10-20T01:04:23Z

fix Error when running tests on node 20 #15

TODO:

Cleanup
major updates? ~~or patch updates?~~
- delete applyRequestFilter

Bug Fixes

Support Node.js 20 - Error when running tests on node 20 #15

v2.0.0 fixes following error on Node.js 20+

Uncaught Error: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

BREAKING CHANGE

Version	Node.js 12	Node.js 14	Node.js 16	Node.js 18	Node.js 20
v1.x.x	Support	Support	Support	Support	Not Support
v2.0.0	No Support	No Support	No Support	Support	Support

Drop Node.js 12, 14, 16
Remove applyRequestFilter(agent) function
- It is incompatible way for Node.js 20+
- So, It is just dropped
Remove stopPortScanningByUrlRedirection option
- We already block the redirect attack pattern
- It is duplicated option and just drop stopPortScanningByUrlRedirection
- request-filtering-agent/test/request-filtering-agent.test.ts
  
  Line 195 in a0a348e
  
  `http://bit.ly/3z04dcF` // redirect to http://127.0.0.1.nip.io:12456

azu · 2023-10-20T01:11:20Z

test/request-filtering-agent.test.ts

            }
        }
    });

    it("should allow http://127.0.0.1, but other private ip is disallowed", async () => {
        const agent = new RequestFilteringHttpAgent({
-            allowIPAddressList: ["127.0.0.1"],
+            allowIPAddressList: ["127.0.0.1", "::1"],


localhost will be ::1 in some env.

azu · 2023-10-20T01:17:25Z

src/request-filtering-agent.ts

+                socket.end(() => {
+                    console.log("end 2");
+                    if (socket.destroyed) {
+                        return;
+                    }
+                    console.log("end 3 ", socket.destroyed);
+                    console.log("end errr", validationError?.message);
+                    socket.destroy(error);


avoid Node.js ERR_INTERNAL_ASSERTION: end -> destory.

Just call destory and Node.js throws ERR_INTERNAL_ASSERTION like #15

…direction` option

azu · 2023-10-21T05:43:19Z

test/request-filtering-agent.test.ts

        const privateIPs = [
-            `http://0.0.0.0:${TEST_PORT}`, // 0.0.0.0 is special


move it to IPv4: should not request because it is meta/unspecified IP

azu · 2023-10-21T05:44:53Z

src/request-filtering-agent.ts

+    // override http.Agent#createConnection
+    // https://nodejs.org/api/http.html#http_agent_createconnection_options_callback
+    // https://nodejs.org/api/net.html#net_net_createconnection_options_connectlistener
+    createConnection(options: TcpNetConnectOpts, connectionListener?: (error: Error | null, socket: Socket) => void) {
+        const { host } = options;
+        if (host !== undefined) {
+            // Direct ip address request without dns-lookup
+            // Example: http://127.0.0.1
+            // https://nodejs.org/api/net.html#net_socket_connect_options_connectlistener
+            const validationError = validateIPAddress({ address: host }, this.requestFilterOptions);
+            if (validationError) {
+                throw validationError;
+            }
+        }
+        // https://nodejs.org/api/net.html#net_socket_connect_options_connectlistener
+        // @ts-expect-error - @types/node does not defined createConnection
+        const socket: Socket = super.createConnection(options, connectionListener);
+        // Request with domain name
+        // Example: http://127.0.0.1.nip.io/
+        const onLookup = (err: Error, address: string, family: string | number, host: string): void => {
+            if (err) {
+                return;
+            }
+            const validationError = validateIPAddress({ address, family, host }, this.requestFilterOptions);
+            if (validationError) {
+                socket.removeListener("lookup", onLookup);
+                socket.end(() => {
+                    socket.destroy(validationError);
+                });
+            }
+        };
+        socket.addListener("lookup", onLookup);
+        return socket;


Implement validation as Agent subclass.
Overriding instance(applyRequestFilter) cause ERR_INTERNAL_ASSERTION on Node.js

Error [ERR_INTERNAL_ASSERTION] nodejs/node#48358

We need to avoid this error.

azu · 2023-10-21T05:46:30Z

src/request-filtering-agent.ts

+            const validationError = validateIPAddress({ address, family, host }, this.requestFilterOptions);
+            if (validationError) {
+                socket.removeListener("lookup", onLookup);
+                socket.end(() => {
+                    socket.destroy(validationError);
+                });
+            }


We need to call end before destory.

When just call destroy without end, Node.js 20 throws following error.

socket.destroy(validationError);

Error: This is caused by either a bug in Node.js or incorrect usage of Node.js internals. Please open an issue with this stack trace at https://github.com/nodejs/node/issues at new NodeError (node:internal/errors:406:5) at assert (node:internal/assert:14:11) at internalConnect (node:net:1037:3) at defaultTriggerAsyncIdScope (node:internal/async_hooks:464:18) at GetAddrInfoReqWrap.emitLookup [as callback] (node:net:1481:9) at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:dns:130:8)

azu · 2023-10-21T05:47:53Z

src/request-filtering-agent.ts

-    // prevent url redirection attack
-    // connection not made to private IP adresses where the port is closed
-    // Default: false
-    stopPortScanningByUrlRedirection?: boolean;


allowMetaIPAddress and allowPrivateIPAddress has same logics.

azu · 2023-10-21T06:16:31Z

test/request-filtering-agent.test.ts

-    it("should fail to make request when stopPortScanningByUrlRedirection option is set to true", async () => {
-        const closedPort = TEST_PORT + 1;
-        const privateIPs = [
-            `http://0.0.0.0:${closedPort}`, // 0.0.0.0 is special
-            `http://127.0.0.1:${closedPort}`,
-            `http://[email protected]:${closedPort}`
-        ];
-        const agent = new RequestFilteringHttpAgent({
-            stopPortScanningByUrlRedirection: true
-        });
-        for (const ipAddress of privateIPs) {
-            try {
-                await fetch(ipAddress, {
-                    agent,
-                    timeout: 2000
-                });
-                throw new ReferenceError("SHOULD NOT BE CALLED");
-            } catch (error) {
-                if (error instanceof ReferenceError) {
-                    assert.fail(error);
-                }
-                assert.match(
-                    error.message,
-                    /^DNS lookup (0\.0\.0\.0|127\.0\.0\.1|undefined)\(family:undefined, host:(0\.0\.0\.0|127\.0\.0\.1|undefined)\) is not allowed. Because, It is private IP address.$/g
-                );
-            }
-        }
-    });


This test case is coverred by other tests.

azu added 7 commits October 20, 2023 09:50

📝2度目のDNS loopupでdestroyするとエラーが起きる

f636e6c

↩️ f636e6c

6fad4f0

fix: Node.js 20 support

efa6f8e

CI: Node. 12+

5b951e9

CI: Node. 14+

67128bf

CI: Node. 14+

0c53b42

fix type

3dfbb3d

azu commented Oct 20, 2023

View reviewed changes

CI: Node.js 16+

6ae95db

azu mentioned this pull request Oct 20, 2023

Error when running tests on node 20 #15

Closed

azu added the Type: Bug Bug or Bug fixes label Oct 20, 2023

azu commented Oct 20, 2023

View reviewed changes

refactor!: remove applyRequestFilter() and `stopPortScanningByUrlRe…

a0a348e

…direction` option

azu added the Type: Breaking Change Includes breaking changes label Oct 21, 2023

azu added 4 commits October 21, 2023 14:36

CI: remove Node.js 16

55cd37d

fix comment

622720f

revert target

1088aff

Update yarn.lock

9d6e555

azu commented Oct 21, 2023

View reviewed changes

add comment

0e0e01c

azu marked this pull request as ready for review October 21, 2023 05:48

azu merged commit dbdeeb5 into master Oct 21, 2023

azu deleted the node20 branch October 21, 2023 05:51

azu commented Oct 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support Node.js 20 #16

fix: support Node.js 20 #16

azu commented Oct 20, 2023 •

edited

Loading

azu Oct 20, 2023 •

edited

Loading

azu Oct 20, 2023 •

edited

Loading

azu Oct 21, 2023

azu Oct 21, 2023

azu Oct 21, 2023

azu Oct 21, 2023

azu Oct 21, 2023

		const privateIPs = [
		`http://0.0.0.0:${TEST_PORT}`, // 0.0.0.0 is special

fix: support Node.js 20 #16

fix: support Node.js 20 #16

Conversation

azu commented Oct 20, 2023 • edited Loading

Bug Fixes

BREAKING CHANGE

azu Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

azu Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

azu Oct 21, 2023

Choose a reason for hiding this comment

azu Oct 21, 2023

Choose a reason for hiding this comment

azu Oct 21, 2023

Choose a reason for hiding this comment

azu Oct 21, 2023

Choose a reason for hiding this comment

azu Oct 21, 2023

Choose a reason for hiding this comment

azu commented Oct 20, 2023 •

edited

Loading

azu Oct 20, 2023 •

edited

Loading

azu Oct 20, 2023 •

edited

Loading