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

[antsmartian]

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

1. Not sure util.js is the right place for inserting this check. This should be fine I guess. Please let me know otherwise.
2. 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]

can exposes

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

[antsmartian]

@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]

fwiw, this is likely also true of http2

[antsmartian]

@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]

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

[richardlau]

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]

@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]

/cc @nodejs/collaborators

[Trott]

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]

@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]

@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]

@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]

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

[antsmartian]

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

[mcollina]

LGTM

[mcollina]

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

[SomeoneWeird]

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

[mcollina]

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

[mcollina]

Landed in 980877f

[ChALkeR]

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

[antsmartian]

Thanks everyone !

[mcollina]

I’ve used ncu.

[ChALkeR]

@mcollina Thanks!