8000 dgram: tighten `address` validation in `socket.send` by VoltrexKeyva · Pull Request #39190 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

dgram: tighten address validation in socket.send #39190

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

Jump to bottom
Closed
VoltrexKeyva wants to merge 3 commits into from
Closed

dgram: tighten address validation in socket.send #39190

VoltrexKeyva wants to merge 3 commits into from

Conversation

@VoltrexKeyva
Copy link
Member
@VoltrexKeyva VoltrexKeyva commented Jun 29, 2021 •
edited
Loading

We don't mention a value being "falsy" in validation, its
better to use a validator here to keep consistency,
this change makes the address parameter only
accept a string, null or undefined.

@github-actions github-actions bot added dgram Issues and PRs related to the dgram subsystem / UDP. needs-ci PRs that need a full CI run. labels Jun 29, 2021
aduh95
aduh95 requested changes Jun 29, 2021
View reviewed changes
Copy link
Contributor
@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

You'd need to update the docs here:

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

The `address` argument is a string. If the value of `address` is a host name,
DNS will be used to resolve the address of the host. If `address` is not
provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
(for `udp6` sockets) will be used by default.

Also doesn't that make the method throw if address is nullish? undefined and null should still be valid value IMHO.

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 29, 2021
@VoltrexKeyva
Copy link
Member Author
VoltrexKeyva commented Jun 29, 2021 •
edited
Loading

You'd need to update the docs here:

node/doc/api/dgram.md

Lines 518 to 521 in f4d0a6a

The `address` argument is a string. If the value of `address` is a host name,
DNS will be used to resolve the address of the host. If `address` is not
provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
(for `udp6` sockets) will be used by default.

Also doesn't that make the method throw if address is nullish? undefined and null should still be valid value IMHO.

No, it doesn't make it throw if the value is "falsy" (or "nullish" if you will) as it only validates the value as a string if the value is not "falsy" or "nullish".

@aduh95
Copy link
Contributor
aduh95 commented Jun 29, 2021

How do you mean? Doesn't validateString simply check the typeof?

node/lib/internal/validators.js

Lines 117 to 120 in 46a7e61

function validateString(value, name) {
if (typeof value !== 'string')
throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
}

@VoltrexKeyva
Copy link
Member Author
VoltrexKeyva commented Jun 29, 2021 •
edited
Loading

@aduh95 Here's how:

The current code:

  • Check if address is not falsy or nullish
    • Check if type of address doesn't equal to string
      • Throw
  • Otherwise ignore

This change:

  • Check if address is not falsy or nullish
    • Validate address to check if its a type of string with validateString()
      • validateString(): If type of the passed value is not string, then throw
  • Otherwise ignore

Both leading to the exact same result but current change tends to keep consistency with the usage of validators and improve readability a little.

aduh95
aduh95 reviewed Jun 30, 2021
View reviewed changes
Copy link
Contributor
@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Oh right, I guess I missed the else if part, my bad.
I think I'd prefer to stricken the validation to allow only nullish or strings, and making this a semver-major, to make this function more consistent with how Node.js usually validates arguments. wdyt?

@VoltrexKeyva
Copy link
Member Author
VoltrexKeyva commented Jun 30, 2021

@aduh95 I'm fine with making the validation more strict, if this actually becomes semver-major do we need to add any change entries to the method's documentation?