-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: inspector security warning for changing host to a public IP #23640
Conversation
c48bfaa
to
e552d34
Compare
doc/api/cli.md
Outdated
either the IP is not public, or the port is properly firewalled to disallow | ||
unwanted connections. | ||
|
||
**More specifially, `--inspect=0.0.0.0` is insecure if the port (`9229` by |
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 might be worthwhile linking to this page that talks about things in more detail: https://nodejs.org/en/docs/guides/debugging-getting-started/#security-implications
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.
Done, thanks!
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: Specifically typo
doc/api/cli.md
Outdated
#### Warning: binding inspector to a public IP:port combination is insecure | ||
|
||
Binding the inspector to a public IP (including `0.0.0.0`) with an open port is | ||
insecure, as it allows anyone of the outside could connect to the inspector and |
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.
"anyone of the outside" isn't grammatically correct - perhaps "as it allows other hosts to connect to the inspector"
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.
Done, with s/other/third-party/.
doc/api/cli.md
Outdated
perform a [remote code execution][] attack. | ||
|
||
If you specify a host, make sure that at least one of the following is true: | ||
either the IP is not public, or the port is properly firewalled to disallow |
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.
s/IP/host/
- use the same word in both places
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.
Done, thanks!
e552d34
to
9897f34
Compare
doc/api/cli.md
Outdated
@@ -144,6 +144,8 @@ Useful when activating the inspector by sending the `SIGUSR1` signal. | |||
|
|||
Default host is `127.0.0.1`. | |||
|
|||
See the [security warning](#inspector_security) below. |
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.
security warning about providing a different `host`
, or something similar to the added text in the other file? That would give people an indication about the circumstances in which there are security concerns upfront
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.
Changed to
See the security warning below regarding the
host
parameter usage.
and
See the security warning regarding the
host
parameter usage.
in two files.
Thanks!
9897f34
to
825dbdb
Compare
doc/api/cli.md
Outdated
#### Warning: binding inspector to a public IP:port combination is insecure | ||
|
||
Binding the inspector to a public IP (including `0.0.0.0`) with an open port is | ||
insecure, as it allows third-party hosts to connect to the inspector and perform |
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.
Wouldn't external hosts be better than third-party here? We don't even have a second party here.
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.
Will fix, thanks!
doc/api/cli.md
Outdated
either the IP is not public, or the port is properly firewalled to disallow | ||
unwanted connections. | ||
|
||
**More specifially, `--inspect=0.0.0.0` is insecure if the port (`9229` by |
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: Specifically typo
Landed in 90be286 |
Refs: nodejs#23444 Refs: nodejs#21774 PR-URL: nodejs#23640 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Refs: #23444 Refs: #21774 PR-URL: #23640 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Refs: #23444 Refs: #21774 PR-URL: #23640 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Refs: #23444 Refs: #21774 PR-URL: #23640 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Refs: #23444 Refs: #21774 PR-URL: #23640 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
This is the documentation part of #23444.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes/cc @nodejs/documentation @nodejs/security-wg