Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 29, 2025

User description

💥 What does this PR do?

  • SM was looking for installation in the wrong place for non-stable versions of Edge on Windows
  • Updates logging messages for confusing behavior with Edge on Windows

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Fix Edge beta/dev/canary installation path detection on Windows

  • Use correct Program Files environment variable for unstable Edge versions

  • Improve logging messages to clarify Edge system path behavior on Windows

  • Simplify browser path resolution logic for Windows Edge installations


Diagram Walkthrough

flowchart LR
  A["Edge Version Detection"] -->|"Unstable Beta/Dev/Canary"| B["Use Program Files"]
  A -->|"Stable"| C["Use Program Files x86"]
  B --> D["Correct MSI Installation Path"]
  C --> D
  E["Improved Logging"] -->|"System Path Clarity"| F["Better User Feedback"]
Loading

File Walkthrough

Relevant files
Bug fix
edge.rs
Fix Edge Windows installation path selection logic             

rust/src/edge.rs

  • Added ENV_PROGRAM_FILES import for unstable Edge versions
  • Fixed browser path resolution to use correct Program Files directory
    based on Edge version stability
  • Refactored Windows path logic to determine environment variable
    dynamically instead of always using x86 variant
  • Simplified path construction by removing redundant OS check
+19/-15 
lib.rs
Improve logging clarity for Edge Windows behavior               

rust/src/lib.rs

  • Enhanced debug logging to show actual browser binary path location
  • Added clarifying notes for Edge on Windows about system path vs cache
    behavior
  • Improved log messages when browser already exists or is being
    downloaded
  • Restructured control flow to return early when browser exists,
    reducing nesting
+60/-39 

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Dec 29, 2025
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Relative path execution

Description: The code uses env::var(program_files_env).unwrap_or_default() and then
Path::new(&env_value).join(&browser_path), which can fall back to an empty string and
produce a relative path (e.g., .\Microsoft\Edge...), potentially allowing execution of a
malicious msedge.exe from the current working directory if the expected ProgramFiles env
var is missing/misconfigured or controlled in a hostile environment.
edge.rs [494-513]

Referred Code
// Stable Edge MSI installs to Program Files (x86) for compatibility
// Beta/Dev/Canary MSIs install to Program Files
let (browser_path, program_files_env) = if self.is_unstable(browser_version) {
    (
        self.get_browser_path_from_version(browser_version)
            .to_string(),
        ENV_PROGRAM_FILES,
    )
} else {
    (
        format!(
            r"Microsoft\Edge\Application\{}\msedge.exe",
            self.get_browser_version()
        ),
        ENV_PROGRAM_FILES_X86,
    )
};
let env_value = env::var(program_files_env).unwrap_or_default();
let full_browser_path = Path::new(&env_value).join(&browser_path);
Ok((&path_to_string(&full_browser_path)).into())
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Silent env fallback: The new Windows Edge path resolution silently falls back to an empty Program Files
environment variable value via unwrap_or_default(), which can produce an incorrect
relative path without surfacing an actionable error.

Referred Code
let env_value = env::var(program_files_env).unwrap_or_default();
let full_browser_path = Path::new(&env_value).join(&browser_path);
Ok((&path_to_string(&full_browser_path)).into())

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Potential sensitive logging: New debug logs include full local filesystem paths and full download URLs, which may
expose sensitive host details (e.g., user directory paths or URL query parameters)
depending on runtime values.

Referred Code
// Checking if browser version already in expected location
let browser_binary_path = self.get_browser_binary_path(original_browser_version)?;
if browser_binary_path.exists() {
    let location_note = if WINDOWS.is(self.get_os()) && self.is_edge() {
        " (Edge uses system path, not cache)"
    } else {
        ""
    };
    self.get_logger().debug(format!(
        "{} {} already exists at {}{}",
        self.get_browser_name(),
        browser_version,
        browser_binary_path.display(),
        location_note
    ));
    self.set_browser_path(path_to_string(&browser_binary_path));
    return Ok(Some(browser_binary_path));
}

// Browser not available, download it
if WINDOWS.is(self.get_os()) && self.is_edge() && !self.is_windows_admin() {


 ... (clipped 35 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Validate environment variable presence

Replace env::var(...).unwrap_or_default() with a check that returns an error if
the environment variable is not set, preventing silent failures from creating
invalid paths.

rust/src/edge.rs [511-512]

-let env_value = env::var(program_files_env).unwrap_or_default();
-let full_browser_path = Path::new(&env_value).join(&browser_path);
+let env_value = env::var_os(program_files_env)
+    .ok_or_else(|| Error::msg(format!("Environment variable {} not set", program_files_env)))?;
+let full_browser_path = PathBuf::from(env_value).join(&browser_path);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that unwrap_or_default() can hide a missing environment variable, leading to silent failures, and proposes a robust, idiomatic Rust solution to handle this critical error case.

Medium
Learned
best practice
Make lock release error-safe

Ensure the lock is released even when get_browser_url_for_download,
download_to_tmp_folder, or uncompress returns an error by using an RAII guard
(e.g., Drop/scopeguard) or a match/finally-style pattern that always releases on
exit.

rust/src/lib.rs [344-385]

 let mut lock = Lock::acquire(self.get_logger(), &browser_path_in_cache, None)?;
+let _release_lock = scopeguard::guard((), |_| lock.release());
+
 // If lock file was deleted, another process held it and released (deleting the file).
 // Check if that process already downloaded the browser.
 if !lock.exists() && browser_binary_path.exists() {
     self.get_logger().debug(format!(
         "{} {} now available at {}",
         self.get_browser_name(),
         browser_version,
         browser_binary_path.display()
     ));
     self.set_browser_path(path_to_string(&browser_binary_path));
     return Ok(Some(browser_binary_path.clone()));
 }
 
 let browser_url = self.get_browser_url_for_download(original_browser_version)?;
 // Edge on Windows: MSI installs to system path, not cache
 let install_note = if WINDOWS.is(self.get_os()) && self.is_edge() {
     " (will install to system path via MSI)"
 } else {
     ""
 };
 self.get_logger().debug(format!(
     "Downloading {} {} from {}{}",
     self.get_browser_name(),
     self.get_browser_version(),
     browser_url,
     install_note
 ));
 let (_tmp_folder, driver_zip_file) =
     download_to_tmp_folder(self.get_http_client(), browser_url, self.get_logger())?;
 
 let browser_label_for_download =
     self.get_browser_label_for_download(original_browser_version)?;
 uncompress(
     &driver_zip_file,
     &browser_path_in_cache,
     self.get_logger(),
     self.get_os(),
     None,
     browser_label_for_download,
 )?;
-lock.release();
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Always ensure cleanup of external resources (e.g., locks, temp resources) using RAII/try-finally patterns so errors don’t leak resources.

Low
  • More

Copy link
Member

@bonigarcia bonigarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it, but the code LGTM.

@titusfortner titusfortner merged commit c39a0b2 into trunk Dec 31, 2025
108 of 110 checks passed
@titusfortner titusfortner deleted the sm_edge branch December 31, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants