inspector: add --inspect-allow-host option #31071
Conversation
nodejs-github-bot
added
c++
legendecas
added
the
inspector
legendecas
force-pushed
the
pr/28470
branch
3 times, most recently
from
11ee4a4
to
43ba813
Compare
|
My main concern is that this increases the attack surface. Is the SSH tunnel (as described on this page - https://nodejs.org/de/docs/guides/debugging-getting-started/) not enough? |
|
@eugeneo Take a look over at #28470 for more information as to why it's not enough. I'm not sure how this "increases the attack surface". There was a whole debate about this in the other PR which originally was just to allow all addresses that are ".local" which was argued did increase the attack vector and this was made as a suggested alternative that satisfied critics. |
|
With no additional configuration (i.e. the default state), this does not change the functionality of inspector at all. However it allows for a configuration where you specify the hostname inspector is allowed to receive connections on. This prevents the DNS rebind attacks which caused the change to inspector that prevented all connections using hostnames other than localhost. |
legendecas
force-pushed
the
pr/28470
branch
2 times, most recently
from
01f12b9
to
8140657
Compare
|
@bnoordhuis @nodejs/security @nodejs/inspector |
doc/api/cli.md
Outdated
| added: REPLACEME | ||
| --> | ||
|
|
||
| Specify allowed HTTP GET host on the inspector websocket port. |
There was a problem hiding this comment.
This sentence is confusingly worded, IMO.
| Specify allowed HTTP GET host on the inspector websocket port. | |
| Have the HTTP endpoint accept HTTP GET requests addressed to this host name. | |
| This option can be specified more than once. |
It's still no Hemingway but hey.
src/inspector_agent.cc
Outdated
| const std::string& path, | ||
| const DebugOptions& options, | ||
| std::shared_ptr<HostPort> host_port, | ||
| std::shared_ptr<std::vector<std::string>> allowed_http_get_hosts, |
There was a problem hiding this comment.
I think you can obtain this from DebugOptions instead of passing it in explicitly? Of course explicit > implicit but then again DRY > redundancy.
There was a problem hiding this comment.
Yes, it can be obtained from the DebugOptions.
But if we'd like to support changing the allow list at runtime from JavaScript API, it would be better to not directly accessing the one in DebugOptions.
There was a problem hiding this comment.
But changing it programmatically is not what's being asked. I'm not even sure that's a good idea, there might be security implications.
This comment has been minimized.
This comment has been minimized.
|
I can reproduce the failed inspector ipv6 case locally. I'll dig into it these days. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bnoordhuis may I have your review on the PR again? |
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff
to
2935f72
Compare
|
Ping @bnoordhuis @legendecas ... looks like this stalled out and needs a rebase and further review to move forward |
|
@legendecas Sorry, missed your ping. Can you rebase and add me as a reviewer? Cheers. |
aduh95
added
stalled
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
|
Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
|
@legendecas sad to see this not come to fruition. would have made attaching to pods running in a locally running k8s cluster so much easier. |
Related #28470
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes