inspector: added --inspect-publish-uid #27741

alexkozy · 2019-05-16T22:43:46Z

This flag specifies how inspector websocket url should be exposed.
Supported options:

stderr - reports websocket as a message to stderr,
http - exposes /json/list endpoint that contains inspector websocket
url,
binding - require('inspector').url().

Related discussion: nodejs/diagnostics#303

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-05-16T22:43:48Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3544

src/inspector_socket_server.cc

src/inspector_io.h

src/inspector_socket_server.cc

alexkozy · 2019-05-17T23:08:40Z

I addressed comments and added another option - binding. When this option is not presented, require('inspector').url() returns undefined.

src/node_options.cc

src/inspector_js_api.cc

eugeneo · 2019-05-17T23:18:31Z

I addressed comments and added another option - binding. When this option is not presented, require('inspector').url() returns undefined.

IMHO, that's unnecessary. Code that can access inspector API can already do pretty much anything.

eugeneo

Approved modulo other review comments.

alexkozy · 2019-05-17T23:35:51Z

I addressed comments and added another option - binding. When this option is not presented, require('inspector').url() returns undefined.

IMHO, that's unnecessary. Code that can access inspector API can already do pretty much anything.

I removed binding from this PR and I'd like to create separate one and when binding is not passed - disable inspector JS bindings all together. WDYT?

eugeneo · 2019-05-17T23:43:13Z

I removed binding from this PR and I'd like to create separate one and when binding is not passed - disable inspector JS bindings all together. WDYT?

I think that would warrant more discussions. I do not see utility of disabling the bindings. It would not help with the security (JS bindings are only accessible from inside the Node runtime).

My main concern is that people would disable JS bindings "for security" and end up confused why some ecosystem tools stop working.

alexkozy · 2019-05-18T01:23:20Z

I think that would warrant more discussions. I do not see utility of disabling the bindings. It would not help with the security (JS bindings are only accessible from inside the Node runtime).

Imagine for a second compromised superpopular package that uses inspector to get anything from any package around violating any module/function scopes. It can access any other package information, it can open inspector websocket url and send it somewhere.

In general if I can inject some code to Node process using compromised dependency or some other way, I get access to part of JS heap but not to everything. As soon as I can use inspector in this snippet - I get access to everything.

My main concern is that people would disable JS bindings "for security" and end up confused why some ecosystem tools stop working.

If people need some tool to work then they need to allow inspection. And some ecosystem tool can easily check that inspection flag was not passed and ask user to pass it - it is not a big deal.

nodejs-github-bot · 2019-05-30T15:30:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23499/

Trott · 2019-05-30T15:31:38Z

@eugeneo @ak239 Can/should this land? Or do you two still need to work out stuff?

alexkozy · 2019-05-30T17:10:41Z

I need to fix a test first.

Trott · 2019-06-01T08:13:57Z

I need to fix a test first.

OK, thanks! I've added the blocked label. (When the test is fixed, remove it!)

nodejs-github-bot · 2019-06-03T07:56:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23582/

nodejs-github-bot · 2019-06-03T08:48:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23583/

This flag specifies how inspector websocket url should be reported. Tthre options are supported: - stderr - reports websocket as a message to stderr, - http - exposes /json/list endpoint that contains inspector websocket url, - binding - require('inspector').url(). Related discussion: nodejs/diagnostics#303

nodejs-github-bot · 2019-06-03T09:02:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23585/

nodejs-github-bot · 2019-06-03T09:55:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23586/

nodejs-github-bot · 2019-06-03T10:23:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23587/

nodejs-github-bot · 2019-06-03T11:11:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23588/

alexkozy · 2019-06-03T15:16:16Z

Landed in f0018a5

This flag specifies how inspector websocket url should be reported. Tthre options are supported: - stderr - reports websocket as a message to stderr, - http - exposes /json/list endpoint that contains inspector websocket url, - binding - require('inspector').url(). Related discussion: nodejs/diagnostics#303 PR-URL: #27741 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

BridgeAR · 2019-06-20T16:50:29Z

doc/api/cli.md

@@ -980,6 +987,7 @@ Node.js options that are allowed are:
 - `--inspect`
 - `--inspect-brk`
 - `--inspect-port`
+- `--inspect-publish-uid`


This should also be documented in ./doc/node.1.

BridgeAR · 2019-06-20T16:52:48Z

Should this actually be semver-minor?

alexkozy · 2019-06-20T17:07:34Z

@BridgeAR this pull request does not change default behavior so I believe we do not need semver-minor for it.

BridgeAR · 2019-06-20T17:10:37Z

@ak239 if it would change the default, I'd expect it to be semver-major? We normally declare all new features as semver-minor.

alexkozy · 2019-06-20T17:43:04Z

@BridgeAR I see, thank you for clarification. It is new feature so semver-minor sounds good to me.

Notable changes: This release contains `semver-major` commits. These are in fact not `semver-major` due to follow-up commits that remove all breaking changes. * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default nodejs#28181 * deps: * Updated `V8` to 7.5.288.22 nodejs#27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c nodejs#28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure nodejs#27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant nodejs#27851 * report: * The cpu info got added to the report output nodejs#28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode nodejs#24260 * tools,gyp: * Introduce MSVS 2019 nodejs#27375 * util: * inspect: * Array grouping became more compact and uses more columns than before nodejs#28059 nodejs#28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines nodejs#28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated nodejs#28021 PR-URL: nodejs#28268

alexkozy requested a review from eugeneo May 16, 2019 22:43

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 16, 2019

alexkozy added the inspector Issues and PRs related to the V8 inspector protocol label May 16, 2019

eugeneo reviewed May 17, 2019

View reviewed changes

src/inspector_socket_server.cc Outdated Show resolved Hide resolved

eugeneo suggested changes May 17, 2019

View reviewed changes

src/inspector_io.h Outdated Show resolved Hide resolved

src/inspector_socket_server.cc Outdated Show resolved Hide resolved

src/inspector_socket_server.cc Outdated Show resolved Hide resolved

alexkozy force-pushed the inspect-publish-uid branch 2 times, most recently from 379d809 to 5070b54 Compare May 17, 2019 23:05

addaleax reviewed May 17, 2019

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

src/inspector_js_api.cc Outdated Show resolved Hide resolved

eugeneo approved these changes May 17, 2019

View reviewed changes

alexkozy force-pushed the inspect-publish-uid branch from 5070b54 to d48d6b4 Compare May 17, 2019 23:31

alexkozy force-pushed the inspect-publish-uid branch from d48d6b4 to 177866f Compare May 18, 2019 00:32

benjamingr approved these changes May 30, 2019

View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Jun 1, 2019

alexkozy force-pushed the inspect-publish-uid branch from 177866f to 7129b9e Compare June 3, 2019 07:55

alexkozy force-pushed the inspect-publish-uid branch from 7129b9e to 106f4bf Compare June 3, 2019 08:47

alexkozy force-pushed the inspect-publish-uid branch from 106f4bf to ccc51ca Compare June 3, 2019 09:02

alexkozy removed the blocked PRs that are blocked by other issues or PRs. label Jun 3, 2019

alexkozy closed this Jun 3, 2019

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

BridgeAR reviewed Jun 20, 2019

View reviewed changes

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 20, 2019

alexkozy deleted the inspect-publish-uid branch June 20, 2019 17:07

alexkozy restored the inspect-publish-uid branch June 20, 2019 17:07

BethGriggs added the lts-watch-v10.x label Sep 3, 2019

BethGriggs added the backport-requested-v10.x label Oct 17, 2019

bnoordhuis mentioned this pull request Jul 7, 2020

Allow to suppress --inspect hint #12665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: added --inspect-publish-uid #27741

inspector: added --inspect-publish-uid #27741

alexkozy commented May 16, 2019 •

edited

Loading

nodejs-github-bot commented May 16, 2019

alexkozy commented May 17, 2019 •

edited

Loading

eugeneo commented May 17, 2019

eugeneo left a comment

alexkozy commented May 17, 2019

eugeneo commented May 17, 2019

alexkozy commented May 18, 2019

nodejs-github-bot commented May 30, 2019

Trott commented May 30, 2019

alexkozy commented May 30, 2019

Trott commented Jun 1, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

alexkozy commented Jun 3, 2019

BridgeAR Jun 20, 2019

BridgeAR commented Jun 20, 2019

alexkozy commented Jun 20, 2019

BridgeAR commented Jun 20, 2019

alexkozy commented Jun 20, 2019

inspector: added --inspect-publish-uid #27741

inspector: added --inspect-publish-uid #27741

Conversation

alexkozy commented May 16, 2019 • edited Loading

Checklist

nodejs-github-bot commented May 16, 2019

alexkozy commented May 17, 2019 • edited Loading

eugeneo commented May 17, 2019

eugeneo left a comment

Choose a reason for hiding this comment

alexkozy commented May 17, 2019

eugeneo commented May 17, 2019

alexkozy commented May 18, 2019

nodejs-github-bot commented May 30, 2019

Trott commented May 30, 2019

alexkozy commented May 30, 2019

Trott commented Jun 1, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

nodejs-github-bot commented Jun 3, 2019

alexkozy commented Jun 3, 2019

BridgeAR Jun 20, 2019

Choose a reason for hiding this comment

BridgeAR commented Jun 20, 2019

alexkozy commented Jun 20, 2019

BridgeAR commented Jun 20, 2019

alexkozy commented Jun 20, 2019

alexkozy commented May 16, 2019 •

edited

Loading

alexkozy commented May 17, 2019 •

edited

Loading