★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/27741Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@addaleax addaleax

@BridgeAR BridgeAR

@eugeneo eugeneo

@benjamingr benjamingr

Assignees
No one assigned
Labels
C++ inspector lib / src semver-minor
Projects
None yet
Milestone
No milestone
7 participants
@ak239 @nodejs-github-bot @eugeneo @Trott @BridgeAR @addaleax @benjamingr
@ak239
Copy link
Member

commented May 16, 2019 •
edited

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

@ak239 ak239 requested a review from eugeneo May 16, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 16, 2019

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

@nodejs-github-bot nodejs-github-bot added C++ lib / src labels May 16, 2019

@ak239 ak239 added the inspector label May 16, 2019

@eugeneo

eugeneo reviewed May 17, 2019
View changes

Show resolved Hide resolved src/inspector_socket_server.cc Outdated
@eugeneo

eugeneo requested changes May 17, 2019
View changes

Show resolved Hide resolved 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

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch 2 times, most recently from 379d809 to 5070b54 May 17, 2019

@ak239

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 17, 2019 •
edited

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

addaleax reviewed May 17, 2019
View changes

Show resolved Hide resolved src/node_options.cc Outdated
Show resolved Hide resolved src/inspector_js_api.cc Outdated
@eugeneo

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 17, 2019

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

eugeneo approved these changes May 17, 2019
View changes

Copy link
Contributor

left a comment

Approved modulo other review comments.

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch from 5070b54 to d48d6b4 May 17, 2019

@ak239

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 17, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 17, 2019

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.

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch from d48d6b4 to 177866f May 18, 2019

@ak239

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 18, 2019

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.
@benjamingr

benjamingr approved these changes May 30, 2019
View changes

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 30, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 30, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented May 30, 2019

I need to fix a test first.

@Trott Trott added the blocked label Jun 1, 2019

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 1, 2019

I need to fix a test first.

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

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch from 177866f to 7129b9e Jun 3, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

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

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch from 7129b9e to 106f4bf Jun 3, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23583/
@ak239
inspector: added --inspect-publish-uid 
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
Loading status checks…
ccc51ca

@ak239 ak239 force-pushed the ak239:inspect-publish-uid branch from 106f4bf to ccc51ca Jun 3, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

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

This comment has been minimized.

Sign in to view
Copy link

commented Jun 3, 2019

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

@ak239 ak239 removed the blocked label Jun 3, 2019

@ak239

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 3, 2019

Landed in f0018a5

@ak239 ak239 closed this Jun 3, 2019

ak239 added a commit that referenced this pull request Jun 3, 2019

@ak239
inspector: added --inspect-publish-uid 
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 <eostroukhov@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Loading status checks…
f0018a5

BridgeAR added a commit that referenced this pull request Jun 17, 2019

@ak239 @BridgeAR
inspector: added --inspect-publish-uid 
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 <eostroukhov@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
f8c9a58

@BridgeAR BridgeAR referenced this pull request Jun 17, 2019

Merged

v12.5.0 proposal #28268

@BridgeAR

BridgeAR reviewed Jun 20, 2019
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@BridgeAR

BridgeAR Jun 20, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 20, 2019

Should this actually be semver-minor?

@BridgeAR BridgeAR added the semver-minor label Jun 20, 2019

@ak239 ak239 deleted the ak239:inspect-publish-uid branch Jun 20, 2019

@ak239 ak239 restored the ak239:inspect-publish-uid branch Jun 20, 2019

@ak239

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 20, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 20, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 20, 2019

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

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-18, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
4360273

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-18, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
65829a2

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-26, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
f5e0b17

BridgeAR added a commit that referenced this pull request Jun 27, 2019

@BridgeAR
2019-06-27, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
01b404f

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019

@BridgeAR
2019-06-27, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
af3cc71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.