-
Notifications
You must be signed in to change notification settings - Fork 662
feat(net/unstable): add ip utilities #6765
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.
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6765 +/- ##
==========================================
- Coverage 94.14% 94.14% -0.01%
==========================================
Files 586 589 +3
Lines 42465 42540 +75
Branches 6701 6717 +16
==========================================
+ Hits 39980 40049 +69
- Misses 2435 2442 +7
+ Partials 50 49 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
iuioiua
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick pass
http/file_server.ts
Outdated
| return html`<a href="${link}">${escape(path)}</a>`; | ||
| }) | ||
| .join("/")}/ | ||
| Index of ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems these formatting changes were made because your Deno version is out-of-date. Please update Deno and run deno fmt to revert these changes.
net/deno.json
Outdated
| "exports": { | ||
| ".": "./mod.ts", | ||
| "./get-available-port": "./get_available_port.ts", | ||
| "./ip-utils": "./ip_utils.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "./ip-utils": "./ip_utils.ts", | |
| "./ip": "./ip.ts", |
This is sufficient
net/ip_utils.ts
Outdated
| @@ -0,0 +1,137 @@ | |||
| // Copyright 2018-2025 the Deno authors. MIT license. | |||
|
|
|||
| const IPV4_REGEX = /^[0-9]{0,3}\.[0-9]{0,3}\.[0-9]{0,3}\.[0-9]{0,3}$/; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you get the regex? Genuine question. Do you have a reference that you can link to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it from https://github.com/denoland/fresh/pull/3035/files i thought it was clever
net/ip_utils.ts
Outdated
| export const distinctRemoteAddr = ( | ||
| remoteAddr: string, | ||
| ): AddressType | undefined => { | ||
| if (isIPv4(remoteAddr)) { | ||
| return "IPv4"; | ||
| } | ||
| if (isIPv6(remoteAddr)) { | ||
| return "IPv6"; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO I have a feeling this function simply isn't needed and adding it now might be premature. If it were up to me, I'd wait till there's sufficient demand for such a function before adding it to the API.
net/ip_utils.ts
Outdated
| * @param addr - IPv4 address in a string format (e.g., "192.168.0.1"). | ||
| * @returns a boolean indicating if the string is a IPv4 address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param addr - IPv4 address in a string format (e.g., "192.168.0.1"). | |
| * @returns a boolean indicating if the string is a IPv4 address | |
| * @param addr IPv4 address in a string format (e.g., "192.168.0.1"). | |
| * @returns A boolean indicating if the string is a IPv4 address |
Nits
net/ip_utils.ts
Outdated
| * @returns a boolean indicating if the string is a valid IPv4 address | ||
| */ | ||
| export function isValidIPv4(addr: string): boolean { | ||
| if (addr === "localhost") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localhost isn't an IP address.
net/ip_utils.ts
Outdated
| * @param addr - IPv4 address in a string format (e.g., "192.168.0.1"). | ||
| * @returns a boolean indicating if the string is a valid IPv4 address | ||
| */ | ||
| export function isValidIPv4(addr: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having both isValidIPv4() and isIPv4() is confusing. We should likely just have one, and I think it should be named isIPv4(). Ditto for the IPv6 function.
net/ip_utils.ts
Outdated
| * @returns a boolean indicating if the string is a IPv6 address | ||
| */ | ||
| export function isIPv6(addr: string): boolean { | ||
| return addr.includes(":"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct
net/ip_utils.ts
Outdated
| export function matchSubnet(subnet: string, address: string): boolean { | ||
| // 192.128.0.1 === 192.128.0.1 | ||
| if (subnet === address) return true; | ||
|
|
||
| if (subnet.includes("/")) { | ||
| const [range, bitStr] = subnet.split("/"); | ||
| if (range && bitStr) { | ||
| const bits = parseInt(bitStr, 0); | ||
| const zeroes = 32 - bits; | ||
| const mask = bits === 0 ? 0 : (~0 << zeroes) >>> 0; | ||
|
|
||
| return (ipToInt(address) & mask) === (ipToInt(range) & mask); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, this can be implemented in a follow up PR, as to get this one landed sooner.
net/ip_utils_test.ts
Outdated
| import { assert } from "../assert/assert.ts"; | ||
| import { assertEquals } from "../assert/equals.ts"; | ||
| import { assertFalse } from "../assert/false.ts"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import { assert } from "../assert/assert.ts"; | |
| import { assertEquals } from "../assert/equals.ts"; | |
| import { assertFalse } from "../assert/false.ts"; | |
| import { assert, assertEquals, assertFalse } from "@std/assert"; |
If you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax of valid IP addresses is described in the URL standard. Please follow that.
Also, I think it would be a waste to parse only to throw the result away, so I suggest adding functions that return the parsed address. They can internally share an implementation like this:
Details
function ipv4Parser(str: string, buf: Uint8Array): boolean {
// the actual implementation…
}
function ipv6Parser(str: string, buf: Uint8Array): boolean {
// maybe reuse `ipv4Parser` for IPv6 addresses in the third form…
}
const buf = new Uint8Array(16);
export function isIPv4(str: string): boolean {
return ipv4Parser(str, buf);
}
export function isIPv6(str: string): boolean {
return ipv6Parser(str, buf);
}
export function parseIPv4(str: string): Uint8Array | null {
return ipv4Parser(str, buf) ? buf.slice(0, 4) : null;
}
export function parseIPv6(str: string): Uint8Array | null {
return ipv6Parser(str, buf) ? buf.slice(0, 16) : null;
}
export function parseIPv4Into(str: string, buf: Uint8Array): boolean {
if (buf.length !== 4) {
throw new TypeError("IPv4 output buffer must have a length of 4");
}
return ipv4Parser(str, buf);
}
export function parseIPv6Into(str: string, buf: Uint8Array): boolean {
if (buf.length !== 16) {
throw new TypeError("IPv6 output buffer must have a length of 16");
}
return ipv6Parser(str, buf);
}
net/ip.ts
Outdated
| // Copyright 2018-2025 the Deno authors. MIT license. | ||
|
|
||
| /** | ||
| * Validates whether a given string is a valid IPv4 address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Validates whether a given string is a valid IPv4 address | |
| * Validates whether a given string is a valid IPv4 address. |
Nit: punctuation
net/ip.ts
Outdated
| * @returns A boolean indicating if the string is a valid IPv4 address | ||
| * | ||
| * @example Check if the address is a IPv4 | ||
| * ```ts no-assert ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * ```ts no-assert ignore | |
| * ```ts |
I don't see a reason why this should be ignored. Ditto for the other snippets.
net/ip.ts
Outdated
| * | ||
| * assert(isIPv4(correctIp)) | ||
| * assertFalse(isIPv4(incorrectIp)) | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * |
Nit
net/ip.ts
Outdated
| const parts = addr.split("."); | ||
|
|
||
| if (parts.length <= 0 || parts.length < 4) return false; | ||
|
|
||
| for (const part of parts) { | ||
| const n = parseInt(part); | ||
|
|
||
| // n >= 256 or is negative | ||
| if (n >= Math.pow(2, 8) || n < 0) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const parts = addr.split("."); | |
| if (parts.length <= 0 || parts.length < 4) return false; | |
| for (const part of parts) { | |
| const n = parseInt(part); | |
| // n >= 256 or is negative | |
| if (n >= Math.pow(2, 8) || n < 0) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| const octets = addr.split("."); | |
| return octets.length === 4 && octets.every((octet) => { | |
| const n = parseInt(octet); | |
| return n >= 0 && n <= 255; | |
| }); |
This is how I would've written it. What you had before allowed strings with 4 or more octets, which is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very clever, i didn´t know about this every function
net/mod.ts
Outdated
| */ | ||
|
|
||
| export * from "./get_available_port.ts"; | ||
| export * from "./ip.ts"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export * from "./ip.ts"; |
Unstable (new) modules aren't usually included in the mod.ts file.
net/deno.json
Outdated
| "exports": { | ||
| ".": "./mod.ts", | ||
| "./get-available-port": "./get_available_port.ts", | ||
| "./ip": "./ip.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "./ip": "./ip.ts", | |
| "./unstable-ip": "./unstable_ip.ts", |
I forgot that new modules need to have the "unstable" prefix.
net/ip.ts
Outdated
| // more than one use of :: | ||
| if ([...addr.matchAll(/::/g)].length > 1) { | ||
| return false; | ||
| } | ||
|
|
||
| const parts = addr.split(":"); | ||
|
|
||
| // insert "" to the sequence when encountering a :: to normalize the hextets | ||
| for (let i = 0; i < parts.length; i++) { | ||
| if (parts[i] == "" && parts.length < 8) { | ||
| parts.splice(i, 0, ""); | ||
| } | ||
| } | ||
|
|
||
| if (parts.length <= 0 || parts.length < 8) return false; | ||
|
|
||
| for (const part of parts) { | ||
| const n = part == "" ? 0 : parseInt(part, 16); | ||
|
|
||
| if (n >= Math.pow(2, 16) || n < 0 || isNaN(n)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // more than one use of :: | |
| if ([...addr.matchAll(/::/g)].length > 1) { | |
| return false; | |
| } | |
| const parts = addr.split(":"); | |
| // insert "" to the sequence when encountering a :: to normalize the hextets | |
| for (let i = 0; i < parts.length; i++) { | |
| if (parts[i] == "" && parts.length < 8) { | |
| parts.splice(i, 0, ""); | |
| } | |
| } | |
| if (parts.length <= 0 || parts.length < 8) return false; | |
| for (const part of parts) { | |
| const n = part == "" ? 0 : parseInt(part, 16); | |
| if (n >= Math.pow(2, 16) || n < 0 || isNaN(n)) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| // more than one use of :: | |
| if (addr.split("::").length > 2) return false; | |
| const hextets = addr.split(":"); | |
| // insert "" to the sequence when encountering a :: to normalize the hextets | |
| while (hextets.length < 8) { | |
| const idx = hextets.indexOf(""); | |
| if (idx === -1) break; | |
| hextets.splice(idx, 0, ""); | |
| } | |
| return hextets.length === 8 && hextets.every((hextet) => { | |
| const n = hextet == "" ? 0 : parseInt(hextet, 16); | |
| return n >= 0 && n <= 65535; | |
| }); |
net/ip_test.ts
Outdated
| import { isIPv4, isIPv6 } from "./ip.ts"; | ||
| import { assertEquals } from "@std/assert"; | ||
|
|
||
| Deno.test("isIPv4() returns boolean for list of IPv4s", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Deno.test("isIPv4() returns boolean for list of IPv4s", () => { | |
| Deno.test("isIPv4()", () => { |
This should suffice. Ditto for the other test.
|
IMO, the current approach is fine for now. |
net/ip.ts
Outdated
| if (parts.length <= 0 || parts.length < 4) return false; | ||
|
|
||
| for (const part of parts) { | ||
| const n = parseInt(part); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because parseInt returns 1 for "1a" for example, isIPv4("1a.1.1.1") is currently true
net/ip.ts
Outdated
| const n = parseInt(part); | ||
|
|
||
| // n >= 256 or is negative | ||
| if (n >= Math.pow(2, 8) || n < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check if n is NaN. Currently isIPv4("a.b.c.d") seems returning true
|
IPv6 has the form of See |
net/ip.ts
Outdated
| * | ||
| * ``` | ||
| */ | ||
| export function isIPv4(addr: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this name conforms with our naming convention. We usually avoid capitalized acronyms (like IP). ref https://docs.deno.com/runtime/contributing/style_guide/#naming-convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this too initially but ended up just thinking this looks fine.
That section states that this is an additional alternative form for IPv6 addresses. I used to be a network engineer and have never used that form or seen it used. I suggest not implementing for that form until there's sufficient demand for it, which, I predict there never will be. |
In Node.js, for example, Also URL constructor validates ip address in the hostname, and it recognizes the ipv6 address in the above form correctly (e.g. |
|
I think this one is good to go then, WDYT? @kt3k |
kt3k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your contribution! @WasixXD
Hopefully closes #6722
This PR adds the
isIPv4(addr: string): booleanandisIPv6(addr: string): booleanfunctions