util: Adding warnings when NODE_DEBUG is set as http/http2 #21914

antsmartian · 2018-07-20T18:11:13Z

I have fixed the issue #21774 (related to NODE_DEBUG=http part). There are couple of things I wanted to understand here:

Not sure util.js is the right place for inserting this check. This should be fine I guess. Please let me know otherwise.
The test case, that I had written is actually failing. I debugged it, looks like for some reason debugEnvRegex test is not working when we set the NODE_DEBUG from code, so warning is not getting triggered. I did make -j4 and tested manually with build, and I could able to see warning like the following:

(node:75788) Warning: Setting the NODE_DEBUG environment variable to 'http' can exposes sensitive data of your application.

So somewhere, I'm making a mistake in my test.

Thanks for your time on this.

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

ChALkeR

See inline comment — we need to make sure that the warning does not get emitted multiple times.

Disregard my comment, the flag is not required here, it already gets cached.

ChALkeR · 2018-07-23T17:14:07Z

lib/util.js

+function emitWarningIfNeeded(set) {
+  if ('HTTP' === set) {
+    process.emitWarning('Setting the NODE_DEBUG ' +
+      'environment variable to \'http\' can exposes sensitive data ' +


can exposes

I am not a native English speaker, but this needs rewording.

antsmartian · 2018-07-24T01:29:09Z

@ChALkeR Thanks, have made the correction. As I have mentioned on my PR, the test case is failing. Not sure where is the mistake. Thanks for your help on this.

jasnell · 2018-07-27T13:57:40Z

lib/util.js

+// emits warning, when user sets
+// NODE_DEBUG=http.
+function emitWarningIfNeeded(set) {
+  if ('HTTP' === set) {


fwiw, this is likely also true of http2

antsmartian · 2018-07-27T16:38:12Z

@jasnell Thanks for reviewing the code. Taken care for http2 too. Should be good too go I guess.

Also made the test case pass for http logging. Just for my education purpose, I tried to test these changes for http debug log like:

// doesn't work
process.env.NODE_DEBUG = 'http';
common.expectWarning(
   'Warning',
   'Setting the NODE_DEBUG ' +
   'environment variable to \'http\' can expose sensitive data ' +
   'of your application.',
   common.noWarnCode
 );

another method:

// doesn't work
process.env.NODE_DEBUG = 'http';
process.on('warning', ({ name, message }) => {
    // assert here
});

I tried adding these assertions in test-http-client-get-url. But no luck. Finally I went ahead and created a separate test case as seen in the PR.

ChALkeR · 2018-07-27T16:48:32Z

lib/util.js

+  if ('HTTP' === set || 'HTTP2' === set) {
+    process.emitWarning('Setting the NODE_DEBUG environment variable ' +
+      'to \'' + set.toLowerCase() + '\' can expose sensitive data ' +
+      'of your application.');


Imo, this needs to be made more explicit, something along «… can expose sensitive data (including passwords, tokens, authentication headers) in the resulting log.»

richardlau · 2018-07-27T17:02:59Z

The test case, that I had written is actually failing. I debugged it, looks like for some reason debugEnvRegex test is not working when we set the NODE_DEBUG from code,

debugEnvRegex is set when util is loaded, so NODE_DEBUG would need to be set before any require('util'); calls (including those from other required modules, e.g. require('http');).

node/lib/util.js

Lines 298 to 307 in 28a3e28

    
           const debugs = {}; 
        
           let debugEnvRegex = /^$/; 
        
           if (process.env.NODE_DEBUG) { 
        
             let debugEnv = process.env.NODE_DEBUG; 
        
             debugEnv = debugEnv.replace(/[|\\{}()[\]^$+?.]/g, '\\$&') 
        
                 .replace(/\*/g, '.*') 
        
                 .replace(/,/g, '$|^') 
        
                 .toUpperCase(); 
        
             debugEnvRegex = new RegExp(`^${debugEnv}$`, 'i'); 
        
           }

antsmartian · 2018-07-27T17:30:42Z

@richardlau Thanks, that make sense.
@ChALkeR ping, addressed your comments.

Let me know if all good.

ChALkeR

Rubber-stump LGTM. I have not tested this locally, though, but code and message look good to me.

I would prefer to wait a bit more for more comments from others (re: message and other things) though.

ChALkeR · 2018-07-27T23:00:55Z

/cc @nodejs/collaborators

Trott · 2018-07-27T23:10:37Z

lib/util.js

+  if ('HTTP' === set || 'HTTP2' === set) {
+    process.emitWarning('Setting the NODE_DEBUG environment variable ' +
+      'to \'' + set.toLowerCase() + '\' can expose sensitive ' +
+      'data (including passwords, tokens, authentication headers) ' +


Optional suggested text changes:

s/including/such as/

s/tokens, authentication/tokens, and authentication/

Tests will need to be updated to reflect any changes here, of course.

Trott · 2018-07-27T23:14:11Z

@nodejs/tsc What's the semver-ness of adding a warning? We treat runtime deprecation warnings as semver-major and I'm not sure there's a good justification for treating the introduction of other runtime warnings as any less severe.

Adding semver-major label out of caution, but feel free to remove it if I'm the only person who feels that way.

ChALkeR · 2018-07-27T23:35:13Z

@antsmartian Won't this trigger the warning twice on NODE_DEBUG=http,http2? If yes, perhaps there needs to be a flag to check if the warning has been already emitted.

antsmartian · 2018-07-28T06:19:31Z

@ChALkeR: Yes it will if the same code imports both http and http2:

require('http')
require('http2')

will give us two warnings:

(node:87392) Warning: Setting the NODE_DEBUG environment variable to 'http' can expose sensitive data (including passwords, tokens, authentication headers) in the resulting log.
(node:87392) Warning: Setting the NODE_DEBUG environment variable to 'http2' can expose sensitive data (including passwords, tokens, authentication headers) in the resulting log.

one for http and another for http2. I guess it should be ok for showing the warning for http and http2 if used as above. Let me know if you feel otherwise.

ChALkeR · 2018-07-28T06:30:33Z

Ah, in fact it's ok if the message is different in that aspect.
@antsmartian Thanks! Disregard my comment above.

antsmartian · 2018-07-28T06:45:32Z

@Trott Addressed your comments. Also thanks for fixing minor punctuation issues in comment section.

mcollina

LGTM

mcollina · 2018-07-30T14:57:35Z

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

SomeoneWeird

LGTM. Much needed. Have seen this go very wrong before.

mcollina · 2018-08-03T14:14:37Z

I'm planning to land next Monday morning (CEST), please comment before then.

mcollina · 2018-08-06T07:25:23Z

Landed in 980877f

PR-URL: #21914 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

ChALkeR · 2018-08-06T08:09:27Z

@mcollina Was that landed manually or with help of node-core-utils?

antsmartian · 2018-08-06T08:12:21Z

Thanks everyone !

mcollina · 2018-08-06T08:18:08Z

I’ve used ncu.

ChALkeR · 2018-08-06T08:20:51Z

@mcollina Thanks!

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [nodejs#21316](nodejs#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [nodejs#21649](nodejs#21649) * `console.time()` will no longer reset a timer if it already exists. [nodejs#20442](nodejs#20442) * Dependencies * V8 has been updated to 7.0. [nodejs#22754](nodejs#22754) * `fs` * The `fs.read()` method now requires a callback. [nodejs#22146](nodejs#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[nodejs#20735](nodejs#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [nodejs#20270](nodejs#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [nodejs#22951](nodejs#22951) * Internal * Windows performance-counter support has been removed. [nodejs#22485](nodejs#22485) * The `--expose-http2` command-line option has been removed. [nodejs#20887](nodejs#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [nodejs#20002](nodejs#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [nodejs#22281](nodejs#22281) * `util.inspect()` output size is limited to 128 MB by default. [nodejs#22756](nodejs#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [nodejs#21914](nodejs#21914)

Notable changes: * Build * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](nodejs/node#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](nodejs/node#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](nodejs/node#20442) * Dependencies * V8 has been updated to 7.0. [#22754](nodejs/node#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](nodejs/node#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](nodejs/node#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](nodejs/node#20270) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed. * An experimental implementation of `queueMicrotask()` has been added. [#22951](nodejs/node#22951) * Internal * Windows performance-counter support has been removed. [#22485](nodejs/node#22485) * The `--expose-http2` command-line option has been removed. [#20887](nodejs/node#20887) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](nodejs/node#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](nodejs/node#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](nodejs/node#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](nodejs/node#21914)

ChALkeR previously requested changes Jul 23, 2018

View reviewed changes

ChALkeR reviewed Jul 23, 2018

View reviewed changes

antsmartian force-pushed the 21774 branch from eaa4be0 to 03c0a78 Compare July 24, 2018 01:28

jasnell reviewed Jul 27, 2018

View reviewed changes

jasnell approved these changes Jul 27, 2018

View reviewed changes

antsmartian force-pushed the 21774 branch from 03c0a78 to e0f143c Compare July 27, 2018 16:21

ChALkeR reviewed Jul 27, 2018

View reviewed changes

antsmartian changed the title ~~util: Adding warnings when NODE_DEBUG is set as http~~ util: Adding warnings when NODE_DEBUG is set as http/http2 Jul 27, 2018

antsmartian force-pushed the 21774 branch from e0f143c to f5e9048 Compare July 27, 2018 17:29

ChALkeR approved these changes Jul 27, 2018

View reviewed changes

Trott reviewed Jul 27, 2018

View reviewed changes

Trott added semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels Jul 27, 2018

util: Adding warnings when NODE_DEBUG is set as http/http2

d248a6f

antsmartian force-pushed the 21774 branch from 8b4378e to d248a6f Compare July 28, 2018 06:43

mcollina approved these changes Jul 29, 2018

View reviewed changes

SomeoneWeird approved these changes Aug 1, 2018

View reviewed changes

mcollina closed this Aug 6, 2018

ChALkeR mentioned this pull request Aug 6, 2018

git-node does not include reviews from other teams nodejs/node-core-utils#267

Closed

richardlau mentioned this pull request Aug 9, 2018

test: add a common.debuglog function #22206

Closed

4 tasks

antsmartian deleted the 21774 branch September 4, 2018 05:02

jasnell mentioned this pull request Oct 2, 2018

11.0.0 proposal #23230

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Adding warnings when NODE_DEBUG is set as http/http2 #21914

util: Adding warnings when NODE_DEBUG is set as http/http2 #21914

antsmartian commented Jul 20, 2018 •

edited

Loading

ChALkeR left a comment

ChALkeR Jul 23, 2018

antsmartian commented Jul 24, 2018 •

edited

Loading

jasnell Jul 27, 2018

antsmartian commented Jul 27, 2018

ChALkeR Jul 27, 2018 •

edited

Loading

richardlau commented Jul 27, 2018

antsmartian commented Jul 27, 2018

ChALkeR left a comment

ChALkeR commented Jul 27, 2018

Trott Jul 27, 2018

Trott commented Jul 27, 2018

ChALkeR commented Jul 27, 2018

antsmartian commented Jul 28, 2018 •

edited

Loading

ChALkeR commented Jul 28, 2018

antsmartian commented Jul 28, 2018

mcollina left a comment

mcollina commented Jul 30, 2018

SomeoneWeird left a comment

mcollina commented Aug 3, 2018

mcollina commented Aug 6, 2018

ChALkeR commented Aug 6, 2018

antsmartian commented Aug 6, 2018

mcollina commented Aug 6, 2018

ChALkeR commented Aug 6, 2018

util: Adding warnings when NODE_DEBUG is set as http/http2 #21914

util: Adding warnings when NODE_DEBUG is set as http/http2 #21914

Conversation

antsmartian commented Jul 20, 2018 • edited Loading

ChALkeR left a comment

Choose a reason for hiding this comment

ChALkeR Jul 23, 2018

Choose a reason for hiding this comment

antsmartian commented Jul 24, 2018 • edited Loading

jasnell Jul 27, 2018

Choose a reason for hiding this comment

antsmartian commented Jul 27, 2018

ChALkeR Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

richardlau commented Jul 27, 2018

antsmartian commented Jul 27, 2018

ChALkeR left a comment

Choose a reason for hiding this comment

ChALkeR commented Jul 27, 2018

Trott Jul 27, 2018

Choose a reason for hiding this comment

Trott commented Jul 27, 2018

ChALkeR commented Jul 27, 2018

antsmartian commented Jul 28, 2018 • edited Loading

ChALkeR commented Jul 28, 2018

antsmartian commented Jul 28, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jul 30, 2018

SomeoneWeird left a comment

Choose a reason for hiding this comment

mcollina commented Aug 3, 2018

mcollina commented Aug 6, 2018

ChALkeR commented Aug 6, 2018

antsmartian commented Aug 6, 2018

mcollina commented Aug 6, 2018

ChALkeR commented Aug 6, 2018

antsmartian commented Jul 20, 2018 •

edited

Loading

antsmartian commented Jul 24, 2018 •

edited

Loading

ChALkeR Jul 27, 2018 •

edited

Loading

antsmartian commented Jul 28, 2018 •

edited

Loading