fix: prevent log files being written to current directory on Windows (#46911)

* fix: prevent log files being written to current directory on Windows

Co-authored-by: Derek Cicerone <derekcicerone@gmail.com>

* Update shell/common/logging.cc

Co-authored-by: Robo <hop2deep@gmail.com>

Co-authored-by: Derek Cicerone <120135886+derekcicerone@users.noreply.github.com>

* chore: add test

Co-authored-by: deepak1556 <hop2deep@gmail.com>

* chore: update includes

Refs 6418805

Co-authored-by: deepak1556 <hop2deep@gmail.com>

* chore: address review feedback

Co-authored-by: deepak1556 <hop2deep@gmail.com>

* chore: update includes

Refs 6418805

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Derek Cicerone <derekcicerone@gmail.com>
Co-authored-by: Derek Cicerone <120135886+derekcicerone@users.noreply.github.com>
Co-authored-by: deepak1556 <hop2deep@gmail.com>

shell/common/api/electron_api_testing.cc

@@ -2,8 +2,10 @@
 // Use of this source code is governed by the MIT license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"
 #include "base/dcheck_is_on.h"
 #include "base/logging.h"
+#include "content/public/common/content_switches.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_includes.h"
 #include "v8/include/v8.h"
@@ -34,12 +36,18 @@ void Log(int severity, std::string text) {
   }
 }
 
+std::string GetLoggingDestination() {
+  const auto* command_line = base::CommandLine::ForCurrentProcess();
+  return command_line->GetSwitchValueASCII(switches::kEnableLogging);
+}
+
 void Initialize(v8::Local<v8::Object> exports,
                 v8::Local<v8::Value> unused,
                 v8::Local<v8::Context> context,
                 void* priv) {
   gin_helper::Dictionary dict(context->GetIsolate(), exports);
   dict.SetMethod("log", &Log);
+  dict.SetMethod("getLoggingDestination", &GetLoggingDestination);
 }
 
 }  // namespace

shell/common/logging.cc

@@ -17,11 +17,41 @@
 #include "chrome/common/chrome_paths.h"
 #include "content/public/common/content_switches.h"
 
+#if BUILDFLAG(IS_WIN)
+#include <windows.h>
+#include "base/win/scoped_handle.h"
+#include "base/win/win_util.h"
+#include "sandbox/policy/switches.h"
+#endif
+
 namespace logging {
 
 constexpr std::string_view kLogFileName{"ELECTRON_LOG_FILE"};
 constexpr std::string_view kElectronEnableLogging{"ELECTRON_ENABLE_LOGGING"};
 
+#if BUILDFLAG(IS_WIN)
+base::win::ScopedHandle GetLogInheritedHandle(
+    const base::CommandLine& command_line) {
+  auto handle_str = command_line.GetSwitchValueNative(::switches::kLogFile);
+  uint32_t handle_value = 0;
+  if (!base::StringToUint(handle_str, &handle_value)) {
+    return {};
+  }
+  // Duplicate the handle from the command line so that different things can
+  // init logging. This means the handle from the parent is never closed, but
+  // there will only be one of these in the process.
+  HANDLE log_handle = nullptr;
+  if (!::DuplicateHandle(::GetCurrentProcess(),
+                         base::win::Uint32ToHandle(handle_value),
+                         ::GetCurrentProcess(), &log_handle, 0,
+                         /*bInheritHandle=*/FALSE, DUPLICATE_SAME_ACCESS)) {
+    return {};
+  }
+  // Transfer ownership to the caller.
+  return base::win::ScopedHandle(log_handle);
+}
+#endif
+
 base::FilePath GetLogFileName(const base::CommandLine& command_line) {
   std::string filename = command_line.GetSwitchValueASCII(switches::kLogFile);
   if (filename.empty())
@@ -47,9 +77,9 @@ bool HasExplicitLogFile(const base::CommandLine& command_line) {
   return !filename.empty();
 }
 
-LoggingDestination DetermineLoggingDestination(
+std::pair<LoggingDestination, bool /* filename_is_handle */>
-    const base::CommandLine& command_line,
+DetermineLoggingDestination(const base::CommandLine& command_line,
-    bool is_preinit) {
+                            bool is_preinit) {
   bool enable_logging = false;
   std::string logging_destination;
   if (command_line.HasSwitch(::switches::kEnableLogging)) {
@@ -64,7 +94,7 @@ LoggingDestination DetermineLoggingDestination(
     }
   }
   if (!enable_logging)
-    return LOG_NONE;
+    return {LOG_NONE, false};
 
   bool also_log_to_stderr = false;
 #if !defined(NDEBUG)
@@ -75,6 +105,16 @@ LoggingDestination DetermineLoggingDestination(
     also_log_to_stderr = true;
 #endif
 
+#if BUILDFLAG(IS_WIN)
+  if (logging_destination == "handle" &&
+      command_line.HasSwitch(::switches::kProcessType) &&
+      command_line.HasSwitch(::switches::kLogFile)) {
+    // Child processes can log to a handle duplicated from the parent, and
+    // provided in the log-file switch value.
+    return {LOG_TO_FILE, true};
+  }
+#endif  // BUILDFLAG(IS_WIN)
+
   // --enable-logging logs to stderr, --enable-logging=file logs to a file.
   // NB. this differs from Chromium, in which --enable-logging logs to a file
   // and --enable-logging=stderr logs to stderr, because that's how Electron
@@ -90,8 +130,8 @@ LoggingDestination DetermineLoggingDestination(
   // given.
   if (HasExplicitLogFile(command_line) ||
       (logging_destination == "file" && !is_preinit))
-    return LOG_TO_FILE | (also_log_to_stderr ? LOG_TO_STDERR : 0);
+    return {LOG_TO_FILE | (also_log_to_stderr ? LOG_TO_STDERR : 0), false};
-  return LOG_TO_SYSTEM_DEBUG_LOG | LOG_TO_STDERR;
+  return {LOG_TO_SYSTEM_DEBUG_LOG | LOG_TO_STDERR, false};
 }
 
 }  // namespace
@@ -100,10 +140,13 @@ void InitElectronLogging(const base::CommandLine& command_line,
                          bool is_preinit) {
   const std::string process_type =
       command_line.GetSwitchValueASCII(::switches::kProcessType);
-  LoggingDestination logging_dest =
+  auto [logging_dest, filename_is_handle] =
       DetermineLoggingDestination(command_line, is_preinit);
   LogLockingState log_locking_state = LOCK_LOG_FILE;
   base::FilePath log_path;
+#if BUILDFLAG(IS_WIN)
+  base::win::ScopedHandle log_handle;
+#endif
 
   if (command_line.HasSwitch(::switches::kLoggingLevel) &&
       GetMinLogLevel() >= 0) {
@@ -121,7 +164,19 @@ void InitElectronLogging(const base::CommandLine& command_line,
   // Don't resolve the log path unless we need to. Otherwise we leave an open
   // ALPC handle after sandbox lockdown on Windows.
   if ((logging_dest & LOG_TO_FILE) != 0) {
-    log_path = GetLogFileName(command_line);
+    if (filename_is_handle) {
+#if BUILDFLAG(IS_WIN)
+      // Child processes on Windows are provided a file handle if logging is
+      // enabled as sandboxed processes cannot open files.
+      log_handle = GetLogInheritedHandle(command_line);
+      if (!log_handle.is_valid()) {
+        LOG(ERROR) << "Unable to initialize logging from handle.";
+        return;
+      }
+#endif
+    } else {
+      log_path = GetLogFileName(command_line);
+    }
   } else {
     log_locking_state = DONT_LOCK_LOG_FILE;
   }
@@ -133,6 +188,13 @@ void InitElectronLogging(const base::CommandLine& command_line,
   LoggingSettings settings;
   settings.logging_dest = logging_dest;
   settings.log_file_path = log_path.value().c_str();
+#if BUILDFLAG(IS_WIN)
+  // Avoid initializing with INVALID_HANDLE_VALUE.
+  // This handle is owned by the logging framework and is closed when the
+  // process exits.
+  // TODO(crbug.com/328285906) Use a ScopedHandle in logging settings.
+  settings.log_file = log_handle.is_valid() ? log_handle.release() : nullptr;
+#endif
   settings.lock_log = log_locking_state;
   // If we're logging to an explicit file passed with --log-file, we don't want
   // to delete the log file on our second initialization.

spec/fixtures/log-test.js

@@ -0,0 +1,3 @@
+const binding = process._linkedBinding('electron_common_testing');
+binding.log(1, 'CHILD_PROCESS_TEST_LOG');
+binding.log(1, `CHILD_PROCESS_DESTINATION_${binding.getLoggingDestination()}`);

spec/logging-spec.ts

@@ -7,7 +7,7 @@ import { once } from 'node:events';
 import * as fs from 'node:fs/promises';
 import * as path from 'node:path';
 
-import { startRemoteControlApp, ifdescribe } from './lib/spec-helpers';
+import { startRemoteControlApp, ifdescribe, ifit } from './lib/spec-helpers';
 
 function isTestingBindingAvailable () {
   try {
@@ -127,6 +127,34 @@ ifdescribe(isTestingBindingAvailable())('logging', () => {
     expect(contents).to.match(/TEST_LOG/);
   });
 
+  ifit(process.platform === 'win32')('child process logs to the given file when --log-file is passed', async () => {
+    const logFilePath = path.join(app.getPath('temp'), 'test-log-file-' + uuid.v4());
+    const preloadPath = path.resolve(__dirname, 'fixtures', 'log-test.js');
+    const rc = await startRemoteControlApp(['--enable-logging', `--log-file=${logFilePath}`, `--boot-eval=preloadPath=${JSON.stringify(preloadPath)}`]);
+    rc.remotely(() => {
+      process._linkedBinding('electron_common_testing').log(0, 'MAIN_PROCESS_TEST_LOG');
+      const { app, BrowserWindow } = require('electron');
+      const w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          preload: preloadPath,
+          additionalArguments: ['--unsafely-expose-electron-internals-for-testing']
+        }
+      });
+      w.loadURL('about:blank');
+      w.webContents.once('did-finish-load', () => {
+        setTimeout(() => { app.quit(); });
+      });
+    });
+    await once(rc.process, 'exit');
+    const stat = await fs.stat(logFilePath);
+    expect(stat.isFile()).to.be.true();
+    const contents = await fs.readFile(logFilePath, 'utf8');
+    expect(contents).to.match(/MAIN_PROCESS_TEST_LOG/);
+    expect(contents).to.match(/CHILD_PROCESS_TEST_LOG/);
+    expect(contents).to.match(/CHILD_PROCESS_DESTINATION_handle/);
+  });
+
   it('logs to the given file when ELECTRON_LOG_FILE is set', async () => {
     const logFilePath = path.join(app.getPath('temp'), 'test-log-file-' + uuid.v4());
     const rc = await startRemoteControlApp([], { env: { ...process.env, ELECTRON_ENABLE_LOGGING: '1', ELECTRON_LOG_FILE: logFilePath } });