fixup! src: add cli option to preserve env vars on dr

RafaelGSS · RafaelGSS · commit d4460ea1f53a · 2024-11-05T16:55:56.000-03:00
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -2058,13 +2058,13 @@ 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-preserve-env`
+### `--report-exclude-env`
 
 <!-- YAML
 added: REPLACEME
 -->
 
-When `--report-preserve-env` is passed the diagnostic report generated will not
+When `--report-exclude-env` is passed the diagnostic report generated will not
 contain the `environmentVariables` data.
 
 ### `--report-signal=signal`
@@ -3126,6 +3126,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,7 +3494,7 @@ const { report } = require('node:process');
 console.log(`Report on exception: ${report.reportOnUncaughtException}`);
 ```
 
-### `process.report.preserveEnv`
+### `process.report.excludeEnv`
 
 <!-- YAML
 added: REPLACEME
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
@@ -16,6 +16,8 @@ const {
 } = require('internal/validators');
 const nr = internalBinding('report');
 
+let excludeEnv;
+
 const report = {
   writeReport(file, err) {
     if (typeof file === 'object' && file !== null) {
@@ -105,8 +107,11 @@ const report = {
 
     nr.setReportOnUncaughtException(trigger);
   },
-  get preserveEnv() {
-    return !nr.shouldPreserveEnvironmentVariables();
+  get excludeEnv() {
+    if (excludeEnv === undefined) {
+      excludeEnv = nr.shouldExcludeEnvironmentVariables();
+    }
+    return excludeEnv;
   },
 };
 
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;
@@ -904,10 +908,6 @@ inline void Environment::RemoveHeapSnapshotNearHeapLimitCallback(
                                         heap_limit);
 }
 
-inline bool Environment::ShouldPreserveEnvOnReport() const {
-  return options_->report_preserve_env;
-}
-
 inline void Environment::SetAsyncResourceContextFrame(
     std::uintptr_t async_resource_handle,
     v8::Global<v8::Value>&& context_frame) {
diff --git a/src/env.h b/src/env.h
@@ -1044,12 +1044,12 @@ 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);
 
-  inline bool ShouldPreserveEnvOnReport() const;
-
   // Field identifiers for exit_info_
   enum ExitInfoField {
     kExiting = 0,
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -383,10 +383,6 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             " (default: current working directory)",
             &EnvironmentOptions::diagnostic_dir,
             kAllowedInEnvvar);
-  AddOption("--report-preserve-env",
-            "Preserve environment variables when generating report",
-            &EnvironmentOptions::report_preserve_env,
-            kAllowedInEnvvar);
   AddOption("--dns-result-order",
             "set default value of verbatim in dns.lookup. Options are "
             "'ipv4first' (IPv4 addresses are placed before IPv6 addresses) "
@@ -881,6 +877,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
@@ -177,7 +177,6 @@ class EnvironmentOptions : public Options {
 #endif  // HAVE_INSPECTOR
   std::string redirect_warnings;
   std::string diagnostic_dir;
-  bool report_preserve_env = false;
   std::string env_file;
   std::string optional_env_file;
   bool has_env_file_string = false;
@@ -250,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,
@@ -96,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);
@@ -250,7 +252,7 @@ static void WriteNodeReport(Isolate* isolate,
   writer.json_arrayend();
 
   // Report operating system information
-  if (env->ShouldPreserveEnvOnReport()) {
+  if (exclude_env == false) {
     PrintEnvironmentVariables(&writer);
   }
   PrintSystemInformation(&writer);
@@ -921,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,
@@ -930,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()) {
@@ -984,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.
@@ -1001,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
@@ -170,10 +170,10 @@ static void ShouldReportOnUncaughtException(
       env->isolate_data()->options()->report_uncaught_exception);
 }
 
-static void ShouldPreserveEnvironmentVariables(
+static void ShouldExcludeEnvironmentVariables(
     const FunctionCallbackInfo<Value>& info) {
   Environment* env = Environment::GetCurrent(info);
-  info.GetReturnValue().Set(env->ShouldPreserveEnvOnReport());
+  info.GetReturnValue().Set(env->report_exclude_env());
 }
 
 static void SetReportOnUncaughtException(
@@ -210,8 +210,8 @@ static void Initialize(Local<Object> exports,
             ShouldReportOnUncaughtException);
   SetMethod(context,
             exports,
-            "shouldPreserveEnvironmentVariables",
-            ShouldPreserveEnvironmentVariables);
+            "shouldExcludeEnvironmentVariables",
+            ShouldExcludeEnvironmentVariables);
   SetMethod(context,
             exports,
             "setReportOnUncaughtException",
@@ -236,7 +236,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(ShouldReportOnSignal);
   registry->Register(SetReportOnSignal);
   registry->Register(ShouldReportOnUncaughtException);
-  registry->Register(ShouldPreserveEnvironmentVariables);
+  registry->Register(ShouldExcludeEnvironmentVariables);
   registry->Register(SetReportOnUncaughtException);
 }
 
diff --git a/test/common/report.js b/test/common/report.js
@@ -61,8 +61,9 @@ function _validateContent(report, fields = []) {
   const sections = ['header', 'nativeStack', 'javascriptStack', 'libuv',
                     'sharedObjects', 'resourceUsage', 'workers'];
 
-  if (!process.report.preserveEnv)
+  if (!process.report.excludeEnv) {
     sections.push('environmentVariables');
+  }
 
   if (!isWindows)
     sections.push('userLimits');
@@ -298,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
@@ -1,4 +1,4 @@
-// Flags: --report-preserve-env
+// Flags: --report-exclude-env
 'use strict';
 
 // Test producing a report via API call, using the no-hooks/no-signal interface.
