src: add cli option to preserve env vars on dr

PR-URL: nodejs#55697 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>
tpoisseau · Nov 21, 2024 · 20498c4 · 20498c4
1 parent c18a3b8
commit 20498c4
Show file tree

Hide file tree

Showing 12 changed files with 199 additions and 14 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -2056,6 +2056,15 @@ Enables report to be generated upon receiving the specified (or predefined)
 signal to the running Node.js process. The signal to trigger the report is
 specified through `--report-signal`.
 
+### `--report-exclude-env`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+When `--report-exclude-env` is passed the diagnostic report generated will not
+contain the `environmentVariables` data.
+
 ### `--report-signal=signal`
 
 <!-- YAML
@@ -3115,6 +3124,7 @@ one is included in the list below.
 * `--redirect-warnings`
 * `--report-compact`
 * `--report-dir`, `--report-directory`
+* `--report-exclude-env`
 * `--report-exclude-network`
 * `--report-filename`
 * `--report-on-fatalerror`

diff --git a/doc/api/process.md b/doc/api/process.md
@@ -3494,6 +3494,16 @@ const { report } = require('node:process');
 console.log(`Report on exception: ${report.reportOnUncaughtException}`);
 ```
 
+### `process.report.excludeEnv`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* {boolean}
+
+If `true`, a diagnostic report is generated without the environment variables.
+
 ### `process.report.signal`
 
 <!-- YAML

diff --git a/doc/api/report.md b/doc/api/report.md
@@ -10,6 +10,9 @@
 
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55697
+    description: Added `--report-exclude-env` option for excluding environment variables from report generation.
   - version:
     - v22.0.0
     - v20.13.0
@@ -465,6 +468,10 @@ meaning of `SIGUSR2` for the said purposes.
   diagnostic report. By default this is not set and the network interfaces
   are included.
 
+* `--report-exclude-env` Exclude `environmentVariables` from the
+  diagnostic report. By default this is not set and the environment
+  variables are included.
+
 A report can also be triggered via an API call from a JavaScript application:
 
 ```js

diff --git a/lib/internal/process/report.js b/lib/internal/process/report.js
@@ -105,6 +105,13 @@ const report = {
 
     nr.setReportOnUncaughtException(trigger);
   },
+  get excludeEnv() {
+    return nr.getExcludeEnv();
+  },
+  set excludeEnv(b) {
+    validateBoolean(b, 'excludeEnv');
+    nr.setExcludeEnv(b);
+  },
 };
 
 function addSignalHandler(sig) {

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -890,6 +890,10 @@ inline bool Environment::is_in_heapsnapshot_heap_limit_callback() const {
   return is_in_heapsnapshot_heap_limit_callback_;
 }
 
+inline bool Environment::report_exclude_env() const {
+  return options_->report_exclude_env;
+}
+
 inline void Environment::AddHeapSnapshotNearHeapLimitCallback() {
   DCHECK(!heapsnapshot_near_heap_limit_callback_added_);
   heapsnapshot_near_heap_limit_callback_added_ = true;

diff --git a/src/env.h b/src/env.h
@@ -1044,6 +1044,8 @@ class Environment final : public MemoryRetainer {
   inline void set_heap_snapshot_near_heap_limit(uint32_t limit);
   inline bool is_in_heapsnapshot_heap_limit_callback() const;
 
+  inline bool report_exclude_env() const;
+
   inline void AddHeapSnapshotNearHeapLimitCallback();
 
   inline void RemoveHeapSnapshotNearHeapLimitCallback(size_t heap_limit);

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -878,6 +878,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             &EnvironmentOptions::tls_max_v1_3,
             kAllowedInEnvvar);
 
+  AddOption("--report-exclude-env",
+            "Exclude environment variables when generating report"
+            " (default: false)",
+            &EnvironmentOptions::report_exclude_env,
+            kAllowedInEnvvar);
   AddOption("--report-exclude-network",
             "exclude network interface diagnostics."
             " (default: false)",

diff --git a/src/node_options.h b/src/node_options.h
@@ -249,6 +249,7 @@ class EnvironmentOptions : public Options {
 
   std::vector<std::string> user_argv;
 
+  bool report_exclude_env = false;
   bool report_exclude_network = false;
 
   inline DebugOptions* get_debug_options() { return &debug_options_; }

diff --git a/src/node_report.cc b/src/node_report.cc
@@ -61,7 +61,8 @@ static void WriteNodeReport(Isolate* isolate,
                             std::ostream& out,
                             Local<Value> error,
                             bool compact,
-                            bool exclude_network = false);
+                            bool exclude_network = false,
+                            bool exclude_env = false);
 static void PrintVersionInformation(JSONWriter* writer,
                                     bool exclude_network = false);
 static void PrintJavaScriptErrorStack(JSONWriter* writer,
@@ -78,6 +79,7 @@ static void PrintJavaScriptErrorProperties(JSONWriter* writer,
 static void PrintNativeStack(JSONWriter* writer);
 static void PrintResourceUsage(JSONWriter* writer);
 static void PrintGCStatistics(JSONWriter* writer, Isolate* isolate);
+static void PrintEnvironmentVariables(JSONWriter* writer);
 static void PrintSystemInformation(JSONWriter* writer);
 static void PrintLoadedLibraries(JSONWriter* writer);
 static void PrintComponentVersions(JSONWriter* writer);
@@ -95,7 +97,8 @@ static void WriteNodeReport(Isolate* isolate,
                             std::ostream& out,
                             Local<Value> error,
                             bool compact,
-                            bool exclude_network) {
+                            bool exclude_network,
+                            bool exclude_env) {
   // Obtain the current time and the pid.
   TIME_TYPE tm_struct;
   DiagnosticFilename::LocalTime(&tm_struct);
@@ -249,6 +252,9 @@ static void WriteNodeReport(Isolate* isolate,
   writer.json_arrayend();
 
   // Report operating system information
+  if (exclude_env == false) {
+    PrintEnvironmentVariables(&writer);
+  }
   PrintSystemInformation(&writer);
 
   writer.json_objectend();
@@ -694,8 +700,7 @@ static void PrintResourceUsage(JSONWriter* writer) {
 #endif  // RUSAGE_THREAD
 }
 
-// Report operating system information.
-static void PrintSystemInformation(JSONWriter* writer) {
+static void PrintEnvironmentVariables(JSONWriter* writer) {
   uv_env_item_t* envitems;
   int envcount;
   int r;
@@ -715,7 +720,10 @@ static void PrintSystemInformation(JSONWriter* writer) {
   }
 
   writer->json_objectend();
+}
 
+// Report operating system information.
+static void PrintSystemInformation(JSONWriter* writer) {
 #ifndef _WIN32
   static struct {
     const char* description;
@@ -915,6 +923,10 @@ std::string TriggerNodeReport(Isolate* isolate,
   bool exclude_network = env != nullptr ? env->options()->report_exclude_network
                                         : per_process::cli_options->per_isolate
                                               ->per_env->report_exclude_network;
+  bool exclude_env =
+      env != nullptr
+          ? env->report_exclude_env()
+          : per_process::cli_options->per_isolate->per_env->report_exclude_env;
 
   report::WriteNodeReport(isolate,
                           env,
@@ -924,7 +936,8 @@ std::string TriggerNodeReport(Isolate* isolate,
                           *outstream,
                           error,
                           compact,
-                          exclude_network);
+                          exclude_network,
+                          exclude_env);
 
   // Do not close stdout/stderr, only close files we opened.
   if (outfile.is_open()) {
@@ -978,8 +991,20 @@ void GetNodeReport(Isolate* isolate,
   bool exclude_network = env != nullptr ? env->options()->report_exclude_network
                                         : per_process::cli_options->per_isolate
                                               ->per_env->report_exclude_network;
-  report::WriteNodeReport(
-      isolate, env, message, trigger, "", out, error, false, exclude_network);
+  bool exclude_env =
+      env != nullptr
+          ? env->report_exclude_env()
+          : per_process::cli_options->per_isolate->per_env->report_exclude_env;
+  report::WriteNodeReport(isolate,
+                          env,
+                          message,
+                          trigger,
+                          "",
+                          out,
+                          error,
+                          false,
+                          exclude_network,
+                          exclude_env);
 }
 
 // External function to trigger a report, writing to a supplied stream.
@@ -995,8 +1020,20 @@ void GetNodeReport(Environment* env,
   bool exclude_network = env != nullptr ? env->options()->report_exclude_network
                                         : per_process::cli_options->per_isolate
                                               ->per_env->report_exclude_network;
-  report::WriteNodeReport(
-      isolate, env, message, trigger, "", out, error, false, exclude_network);
+  bool exclude_env =
+      env != nullptr
+          ? env->report_exclude_env()
+          : per_process::cli_options->per_isolate->per_env->report_exclude_env;
+  report::WriteNodeReport(isolate,
+                          env,
+                          message,
+                          trigger,
+                          "",
+                          out,
+                          error,
+                          false,
+                          exclude_network,
+                          exclude_env);
 }
 
 }  // namespace node
diff --git a/src/node_report_module.cc b/src/node_report_module.cc
@@ -95,6 +95,17 @@ static void SetExcludeNetwork(const FunctionCallbackInfo<Value>& info) {
   env->options()->report_exclude_network = info[0]->IsTrue();
 }
 
+static void GetExcludeEnv(const FunctionCallbackInfo<Value>& info) {
+  Environment* env = Environment::GetCurrent(info);
+  info.GetReturnValue().Set(env->report_exclude_env());
+}
+
+static void SetExcludeEnv(const FunctionCallbackInfo<Value>& info) {
+  Environment* env = Environment::GetCurrent(info);
+  CHECK(info[0]->IsBoolean());
+  env->options()->report_exclude_env = info[0]->IsTrue();
+}
+
 static void GetDirectory(const FunctionCallbackInfo<Value>& info) {
   Mutex::ScopedLock lock(per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
@@ -187,6 +198,8 @@ static void Initialize(Local<Object> exports,
   SetMethod(context, exports, "setCompact", SetCompact);
   SetMethod(context, exports, "getExcludeNetwork", GetExcludeNetwork);
   SetMethod(context, exports, "setExcludeNetwork", SetExcludeNetwork);
+  SetMethod(context, exports, "getExcludeEnv", GetExcludeEnv);
+  SetMethod(context, exports, "setExcludeEnv", SetExcludeEnv);
   SetMethod(context, exports, "getDirectory", GetDirectory);
   SetMethod(context, exports, "setDirectory", SetDirectory);
   SetMethod(context, exports, "getFilename", GetFilename);
@@ -215,6 +228,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(SetCompact);
   registry->Register(GetExcludeNetwork);
   registry->Register(SetExcludeNetwork);
+  registry->Register(GetExcludeEnv);
+  registry->Register(SetExcludeEnv);
   registry->Register(GetDirectory);
   registry->Register(SetDirectory);
   registry->Register(GetFilename);

diff --git a/test/common/report.js b/test/common/report.js
@@ -59,7 +59,12 @@ function _validateContent(report, fields = []) {
 
   // Verify that all sections are present as own properties of the report.
   const sections = ['header', 'nativeStack', 'javascriptStack', 'libuv',
-                    'environmentVariables', 'sharedObjects', 'resourceUsage', 'workers'];
+                    'sharedObjects', 'resourceUsage', 'workers'];
+
+  if (!process.report.excludeEnv) {
+    sections.push('environmentVariables');
+  }
+
   if (!isWindows)
     sections.push('userLimits');
 
@@ -294,10 +299,12 @@ function _validateContent(report, fields = []) {
                        resource.type === 'loop' ? 'undefined' : 'boolean');
   });
 
-  // Verify the format of the environmentVariables section.
-  for (const [key, value] of Object.entries(report.environmentVariables)) {
-    assert.strictEqual(typeof key, 'string');
-    assert.strictEqual(typeof value, 'string');
+  if (!process.report.excludeEnv) {
+    // Verify the format of the environmentVariables section.
+    for (const [key, value] of Object.entries(report.environmentVariables)) {
+      assert.strictEqual(typeof key, 'string');
+      assert.strictEqual(typeof value, 'string');
+    }
   }
 
   // Verify the format of the userLimits section on non-Windows platforms.

diff --git a/test/report/test-report-writereport-exclude-env.js b/test/report/test-report-writereport-exclude-env.js
@@ -0,0 +1,80 @@
+// Flags: --report-exclude-env
+'use strict';
+
+// Test producing a report via API call, using the no-hooks/no-signal interface.
+require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const helper = require('../common/report');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+process.report.directory = tmpdir.path;
+
+function validate() {
+  const reports = helper.findReports(process.pid, tmpdir.path);
+  assert.strictEqual(reports.length, 1);
+  helper.validate(reports[0], arguments[0]);
+  fs.unlinkSync(reports[0]);
+  return reports[0];
+}
+
+{
+  // Test with no arguments.
+  process.report.writeReport();
+  validate();
+}
+
+{
+  // Test with an error argument.
+  process.report.writeReport(new Error('test error'));
+  validate();
+}
+
+{
+  // Test with an error with one line stack
+  const error = new Error();
+  error.stack = 'only one line';
+  process.report.writeReport(error);
+  validate();
+}
+
+{
+  const error = new Error();
+  error.foo = 'goo';
+  process.report.writeReport(error);
+  validate([['javascriptStack.errorProperties.foo', 'goo']]);
+}
+
+{
+  // Test with a file argument.
+  const file = process.report.writeReport('custom-name-1.json');
+  const absolutePath = tmpdir.resolve(file);
+  assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
+  assert.strictEqual(file, 'custom-name-1.json');
+  helper.validate(absolutePath);
+  fs.unlinkSync(absolutePath);
+}
+
+{
+  // Test with file and error arguments.
+  const file = process.report.writeReport('custom-name-2.json',
+                                          new Error('test error'));
+  const absolutePath = tmpdir.resolve(file);
+  assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
+  assert.strictEqual(file, 'custom-name-2.json');
+  helper.validate(absolutePath);
+  fs.unlinkSync(absolutePath);
+}
+
+{
+  // Test with a filename option.
+  process.report.filename = 'custom-name-3.json';
+  const file = process.report.writeReport();
+  assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
+  const filename = path.join(process.report.directory, 'custom-name-3.json');
+  assert.strictEqual(file, process.report.filename);
+  helper.validate(filename);
+  fs.unlinkSync(filename);
+}