Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ pub const DEV: &str = "dev";
pub const CANARY: &str = "canary";
pub const NIGHTLY: &str = "nightly";
pub const ESR: &str = "esr";
pub const BROWSER_VERSION_IGNORE: &str = "ignore";
pub const REG_VERSION_ARG: &str = "version";
pub const REG_PV_ARG: &str = "pv";
pub const DASH_VERSION: &str = "-v";
Expand Down Expand Up @@ -514,6 +515,7 @@ pub trait SeleniumManager {
}
if !download_browser && !self.is_electron() {
let major_browser_version = self.get_major_browser_version();
let original_browser_path = self.get_browser_path().to_string();
match self.discover_browser_version()? {
Some(discovered_version) => {
if !self.is_safari() {
Expand All @@ -523,9 +525,31 @@ pub trait SeleniumManager {
discovered_version
));
}
if self.is_browser_version_specific()
if self.is_browser_version_ignore() {
if !original_browser_path.is_empty() {
self.get_logger().warn(format!(
"Browser version check bypassed for {} at {}; using detected version {}",
self.get_browser_name(),
original_browser_path,
discovered_version
));
}
self.set_browser_version(discovered_version);
} else if self.is_browser_version_specific()
Comment thread
Delta456 marked this conversation as resolved.
&& !self.get_browser_version().eq(&discovered_version)
{
Comment on lines +538 to 540

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. --browser-path major mismatch allowed πŸ“Ž Requirement gap ≑ Correctness

The new mismatch error path in discover_local_browser() only triggers when
is_browser_version_specific() is true (i.e., the requested --browser-version contains a .), so
a major-only version like 114/105 can still mismatch the detected version at an explicit
--browser-path without failing fast. In that case, later major-version handling can set
download_browser=true and effectively ignore the user-provided path, violating the requirement to
clearly fail when --browser-path and --browser-version conflict.
Agent Prompt
## Issue description
When `--browser-path` is provided together with a major-only `--browser-version` (e.g., `114`/`105`), a mismatch between the detected browser version at that path and the requested major version does not trigger the new fail-fast mismatch error because it is gated by `is_browser_version_specific()` (versions containing `.`). This can allow later major-version mismatch handling to set `download_browser = true` and effectively ignore the explicit browser path, contrary to the requirement to clearly fail on `--browser-path`/`--browser-version` conflicts.

## Issue Context
Compliance ID 1 requires a clear failure when `--browser-path` and `--browser-version` conflict.
The current mismatch error path only runs when `is_browser_version_specific()` is true (implemented as `version.contains(".")`), so major-only version requests bypass it.
Major-version mismatch handling happens later during major comparisons/reconciliation and can still set `download_browser = true` without checking whether `original_browser_path` / an explicit `--browser-path` was provided.

## Fix Focus Areas
- rust/src/lib.rs[508-610]
- rust/src/lib.rs[855-867]

β“˜ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if !original_browser_path.is_empty() {
return Err(anyhow!(format!(
"The browser at {} has version {} but {} {} was requested; \
remove --browser-path to allow a browser download, or set \
--browser-version {} to use the detected browser version",
original_browser_path,
discovered_version,
self.get_browser_name(),
self.get_browser_version(),
BROWSER_VERSION_IGNORE
)));
}
download_browser = true;
} else {
let discovered_major_browser_version = self
Expand Down Expand Up @@ -591,6 +615,25 @@ pub trait SeleniumManager {
self.get_browser_name(),
self.get_browser_version_label()
));
if self.is_browser_version_ignore() {
let detail = if !original_browser_path.is_empty() {
format!(
"Could not detect {} version at {}; \
--browser-version {} requires a detectable local browser",
self.get_browser_name(),
original_browser_path,
BROWSER_VERSION_IGNORE
)
} else {
format!(
"No local {} found; --browser-version {} requires a local browser \
(use --browser-path to specify its location)",
self.get_browser_name(),
BROWSER_VERSION_IGNORE
)
};
return Err(anyhow!(detail));
}
download_browser = true;
}
}
Expand Down Expand Up @@ -817,6 +860,11 @@ pub trait SeleniumManager {
self.is_version_specific(self.get_browser_version())
}

fn is_browser_version_ignore(&self) -> bool {
self.get_browser_version()
.eq_ignore_ascii_case(BROWSER_VERSION_IGNORE)
}

fn is_driver_version_specific(&self) -> bool {
self.is_version_specific(self.get_driver_version())
}
Expand Down
119 changes: 119 additions & 0 deletions rust/tests/browser_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use crate::common::{assert_output, get_selenium_manager, get_stdout};
use exitcode::DATAERR;
use rstest::rstest;
use std::env::consts::OS;
#[cfg(unix)]
use std::os::unix::fs::PermissionsExt;
use std::path::Path;

mod common;
Expand Down Expand Up @@ -160,3 +162,120 @@ fn invalid_browser_path_test() {
.code(DATAERR)
.failure();
}

#[cfg(unix)]
fn create_fake_browser(version: &str) -> std::path::PathBuf {
let tmp = std::env::temp_dir().join(format!("fake-chrome-{}", version.replace('.', "-")));
let script = format!("#!/bin/sh\necho 'Google Chrome {}'\n", version);
std::fs::write(&tmp, script).expect("Unable to write fake browser script");
std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755))
.expect("Unable to set executable bit");
tmp
}

#[cfg(unix)]
fn create_silent_browser() -> std::path::PathBuf {
let tmp = std::env::temp_dir().join("fake-chrome-silent");
std::fs::write(&tmp, "#!/bin/sh\n# prints nothing\n")
.expect("Unable to write silent browser script");
std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755))
.expect("Unable to set executable bit");
tmp
}

#[test]
#[cfg(unix)]
fn browser_version_ignore_no_detectable_browser_test() {
let silent_browser = create_silent_browser();
let mut cmd = get_selenium_manager();
let stdout = cmd
.args([
"--browser",
"chrome",
"--browser-path",
silent_browser.to_str().unwrap(),
"--browser-version",
"ignore",
"--debug",
])
.assert()
.get_output()
.stdout
.clone();
let stdout_str = std::str::from_utf8(&stdout).unwrap();
assert!(
!stdout_str.contains("not available for download"),
"Should not show misleading download-version error; got: {}",
stdout_str
);
assert!(
stdout_str.contains("Could not detect") || stdout_str.contains("No local"),
"Should explain that no detectable local browser was found; got: {}",
stdout_str
);
}
Comment thread
Delta456 marked this conversation as resolved.

#[test]
#[cfg(unix)]
fn browser_path_version_mismatch_test() {
let fake_browser = create_fake_browser("131.0.6778.264");
let mut cmd = get_selenium_manager();
let stdout = cmd
.args([
"--browser",
"chrome",
"--browser-path",
fake_browser.to_str().unwrap(),
"--browser-version",
"999.0.0.0",
"--debug",
])
.assert()
.get_output()
.stdout
.clone();
let stdout_str = std::str::from_utf8(&stdout).unwrap();
// The mismatch is always reported, either as ERROR (no cached driver) or WARN (fallback used)
assert!(
stdout_str.contains("131.0.6778.264"),
"Should mention detected version"
);
assert!(
stdout_str.contains("999.0.0.0"),
"Should mention requested version"
);
assert!(
stdout_str.contains("ignore"),
"Should mention the ignore escape hatch"
);
}

#[test]
#[cfg(unix)]
fn browser_path_version_ignore_test() {
let fake_browser = create_fake_browser("131.0.6778.264");
let mut cmd = get_selenium_manager();
let stdout = cmd
.args([
"--browser",
"chrome",
"--browser-path",
fake_browser.to_str().unwrap(),
"--browser-version",
"ignore",
"--debug",
])
.assert()
.get_output()
.stdout
.clone();
let stdout_str = std::str::from_utf8(&stdout).unwrap();
assert!(
stdout_str.contains("bypassed"),
"Warning about version check bypass should appear"
);
assert!(
stdout_str.contains("131.0.6778.264"),
"Should log the detected browser version"
);
}
Loading