net: do not manipulate potential user code by BridgeAR · Pull Request #26751 · nodejs/node (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

BridgeAR

The error provided in this function could come from user code. Thus
the error should not be manipulated in any way. The added properties
do not seem to provide any actual value either as can not be part
of the error. The hostname is already set on the error and adding
the host property with the identical value does not seem right in
this case.

Checklist

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 18, 2019

@nodejs-github-bot

@joyeecheung

I am not sure why the existing properties have to be removed...they seem to exist for a very long time, so that at least warrants a CITGM run? At least the port could be sometimes useful IMO when looking at server logs (e.g. when you recognize certain well-known ports, you'll know what the request is for, even when the port is not the cause of the error).

Also if it's just for avoid manipulating user errors, we could just branch out when we know that lookup is dns.lookup.

@BridgeAR

jasnell

@addaleax

The error provided in this function could come from user code. Thus
the error should not be manipulated in any way.

I don’t think that’s necessarily the case. I’d be okay with only adding these properties if they aren’t already set (i.e. no overriding), but I don’t see why these properties wouldn’t generally be useful?

@lpinca

lpinca

@lpinca

hostname is available, port is not very useful for this error in my opinion. LGTM.

@BridgeAR

@addaleax @joyeecheung as @lpinca pointed out, the hostname is still set and the port is not involved in this case. I personally would actually say that having the port included in the error message would make me think that it has something to do with it, while that is not true.

@BridgeAR

mcollina

Choose a reason for hiding this comment

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

LGTM

@BridgeAR

@addaleax

@BridgeAR I think this needs another TSC approval, but apart from that, yes, I think so

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 20, 2019

@BridgeAR

The error provided in this function could come from user code. Thus the error should not be manipulated in any way. The added properties do not seem to provide any actual value either as can not be part of the error. The hostname is already set on the error and adding the host property with the identical value does not seem right in this case.

@BridgeAR

@BridgeAR

@nodejs/tsc PTAL. This has a couple of LGs but it misses one more LG from a TSC member and is otherwise ready.

Trott

Choose a reason for hiding this comment

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

LGTM if CITGM passes.

mcollina

Choose a reason for hiding this comment

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

LGTM

@BridgeAR

@BridgeAR

CITGM looked good to me.

Landed in 96204c3 🎉

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Mar 25, 2019

@BridgeAR

The error provided in this function could come from user code. Thus the error should not be manipulated in any way. The added properties do not seem to provide any actual value either as can not be part of the error. The hostname is already set on the error and adding the host property with the identical value does not seem right in this case.

PR-URL: nodejs#26751 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Matteo Collina matteo.collina@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

ankon added a commit to Collaborne/kafkajs that referenced this pull request

Nov 5, 2019

@ankon

NodeJS 12.x removed the (useless) 'host:port' part of the connection errors with nodejs/node#26751, adapt the code here to check with a pattern that handles both NodeJS 12.x and earlier versions.

@ankon ankon mentioned this pull request

Nov 5, 2019

@BridgeAR BridgeAR deleted the do-not-manipulate-errors branch

January 20, 2020 11:55

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

@apapirovski apapirovski Awaiting requested review from apapirovski

@ofrobots ofrobots Awaiting requested review from ofrobots

@rvagg rvagg Awaiting requested review from rvagg

@danbev danbev Awaiting requested review from danbev

@gibfahn gibfahn Awaiting requested review from gibfahn

@ChALkeR ChALkeR Awaiting requested review from ChALkeR

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

net

Issues and PRs related to the net subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.