fix(rust): prevent empty driver version from being cached in metadata#17757
Open
MathiasPaulenko wants to merge 5 commits into
Open
fix(rust): prevent empty driver version from being cached in metadata#17757MathiasPaulenko wants to merge 5 commits into
MathiasPaulenko wants to merge 5 commits into
Conversation
Edge and Firefox cached empty driver_version in metadata when a
version-less HTTP response was received (e.g. misconfigured
driver-mirror-url or intercepting proxy). Chrome already guarded
against this with `!driver_version.is_empty()`. This change applies
the same guard to Edge and Firefox.
Also adds unit tests for previously untested pure logic:
- parse_version (version string parsing, error short-circuit, ESR/snap)
- get_index_version (major/minor/patch extraction, error cases)
- Version predicates (is_stable, is_beta, is_dev, is_nightly, is_esr,
is_version_specific)
- get_major_version, get_major_browser_version
- str_to_os, OS::is, ARCH::is (config parsing)
- get_env_name (env var name generation)
- find_latest_from_cache, collect_files_from_cache
- is_driver_and_matches_browser_version (including empty-major bug)
- find_best_driver_from_cache (match, fallback, empty cache)
Tests document latent bugs: lexical sort misorders across digit
boundaries (9 vs 10), and starts_with("") matches all cached
drivers when major version is empty.
Fixes SeleniumHQ#17641
|
|
selenium-ci
added
C-rust
Contributor
PR Summary by Qodofix(rust): avoid caching empty driver versions for Edge/Firefox
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Selenium Manager cache-poisoning bug where Edge/Firefox could cache an empty driver_version in metadata, aligning their caching guard with Chrome’s behavior. It also adds a set of Rust unit tests to improve deterministic coverage of version parsing, config parsing, and cache selection logic.
Changes:
- Prevent caching empty
driver_versionvalues inedge.rsandfirefox.rsmetadata updates. - Add new Rust unit test suites under
rust/tests/for versions, config parsing, and cache logic. - Add inline unit tests in
rust/src/lib.rsandrust/src/config.rsfor helper functions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/edge.rs | Adds !driver_version.is_empty() guard before caching driver metadata. |
| rust/src/firefox.rs | Adds !driver_version.is_empty() guard before caching driver metadata. |
| rust/tests/version_unit_tests.rs | Adds unit tests for parse_version and version helper/predicate behavior. |
| rust/tests/config_unit_tests.rs | Adds unit tests for OS/ARCH parsing and matching helpers. |
| rust/tests/cache_unit_tests.rs | Adds unit tests for cache scanning and cache selection helpers. |
| rust/src/lib.rs | Adds inline unit tests for get_index_version. |
| rust/src/config.rs | Adds inline unit tests for get_env_name. |
Comment on lines
+27
to
+35
| fn platform_label() -> (&'static str, &'static str, &'static str) { | ||
| if cfg!(windows) { | ||
| ("win64", "windows", ".exe") | ||
| } else if cfg!(target_os = "macos") { | ||
| ("mac-x64", "macos", "") | ||
| } else { | ||
| ("linux64", "linux", "") | ||
| } | ||
| } |
Author
There was a problem hiding this comment.
Fixed in commit 015b4b3 — create_driver_in_cache now uses the manager's actual get_platform_label() and get_os() instead of hardcoded values, so tests work correctly on Apple Silicon (mac-arm64) and 32-bit Windows (win32).
Comment on lines
+159
to
+164
| #[test] | ||
| fn get_major_version_errors_on_garbage() { | ||
| let manager = get_manager_by_browser("chrome".to_string()).unwrap(); | ||
| let result = manager.get_major_version("not-a-version"); | ||
| assert_eq!(result.unwrap(), "not-a-version"); | ||
| } |
Author
There was a problem hiding this comment.
Fixed in commit 015b4b3 — renamed the test to get_major_version_returns_first_component_for_non_numeric to accurately reflect that get_index_version only splits by . and does not validate numeric content.
- Use manager's actual get_platform_label() instead of hardcoded values, so tests work on Apple Silicon (mac-arm64) and 32-bit Windows (win32) - Rename get_major_version_errors_on_garbage to get_major_version_returns_first_component_for_non_numeric since the test asserts Ok, not Err
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
14 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
14 rules 1. should_cache_driver_version lacks docs
|
… guard, fix i686 arch classification - Add empty_driver_version_is_not_cached_in_metadata regression test that fails if the !driver_version.is_empty() guard is removed from edge.rs/firefox.rs - Move i686 from ARCH::X64 to ARCH::X32 aliases in config.rs so get_normalized_arch() correctly returns ARCH_X86 for 32-bit systems - Update config_unit_tests.rs to expect i686 under X32 and verify X64 does not match i686
Contributor
|
Code review by qodo was updated up to the latest commit c7e86d0 |
Comment on lines
+184
to
+190
| pub fn should_cache_driver_version( | ||
| driver_ttl: u64, | ||
| major_browser_version: &str, | ||
| driver_version: &str, | ||
| ) -> bool { | ||
| driver_ttl > 0 && !major_browser_version.is_empty() && !driver_version.is_empty() | ||
| } |
Contributor
There was a problem hiding this comment.
1. should_cache_driver_version lacks docs 📘 Rule violation ✧ Quality
The new public function should_cache_driver_version has no Rust doc comment (///) even though it is part of the crate’s public API surface. This violates the requirement to document public API items with structured doc comments, including # Arguments and # Returns where applicable.
Agent Prompt
## Issue description
`pub fn should_cache_driver_version(...) -> bool` is a public API item but has no `///` documentation. Per the checklist, public functions should have a brief summary plus structured sections when applicable.
## Issue Context
This function takes multiple parameters and returns a non-`()` value, so the doc comment should include `# Arguments` and `# Returns`.
## Fix Focus Areas
- rust/src/metadata.rs[184-190]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+256
to
+257
| if should_cache_driver_version(driver_ttl, major_browser_version, &driver_version) | ||
| { |
Contributor
There was a problem hiding this comment.
2. String passed to helper 🐞 Bug ≡ Correctness
EdgeManager::request_driver_version passes major_browser_version (a String) into should_cache_driver_version, which requires &str, causing a compile-time type mismatch and breaking the build.
Agent Prompt
### Issue description
`should_cache_driver_version(driver_ttl, major_browser_version, &driver_version)` passes a `String` where the helper requires `&str`, which is a compile error in Rust.
### Issue Context
In `edge.rs`, `major_browser_version` is a `String` returned from `get_major_browser_version()`, and the helper signature is `(&str, &str)` for the version parameters.
### Fix Focus Areas
- rust/src/edge.rs[252-264]
### Suggested change
Update the call to explicitly pass a string slice, e.g.:
- `should_cache_driver_version(driver_ttl, major_browser_version.as_str(), &driver_version)`
(or `should_cache_driver_version(driver_ttl, &major_browser_version, &driver_version)` relying on deref coercion)
Keep the rest of the logic unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit e6529d0 |
Contributor
|
Code review by qodo was updated up to the latest commit 15ef2a7 |
bonigarcia
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a caching bug in Selenium Manager where Edge and Firefox cached an empty
driver_versionin metadata when a version-less HTTP response was received (e.g. misconfigured driver-mirror-url or intercepting proxy). Chrome already guarded against this with!driver_version.is_empty()— this PR applies the same guard to Edge and Firefox.Root cause
In
edge.rsandfirefox.rs, the driver-version caching condition was:Chrome (
chrome.rs) had the correct guard:Without the
!driver_version.is_empty()check, a version-less HTTP 200 response would push an empty string intometadata.drivers, poisoning the cache for the entire TTL window (1 hour). Subsequent runs would find the empty version in metadata and fail to resolve a driver.Changes
Bug fix
rust/src/edge.rs— added&& !driver_version.is_empty()to caching guardrust/src/firefox.rs— same fixUnit tests (new coverage for issue #17641)
Before this PR, Selenium Manager had no unit tests for its pure logic — only integration tests requiring network and the built binary.
New test files:
rust/tests/version_unit_tests.rsparse_version(version extraction, error short-circuit, ESR/snap suffixes, trailing dot, garbage input), version predicates (is_stable,is_beta,is_dev,is_nightly,is_esr,is_version_specific),get_major_version,get_major_browser_versionrust/tests/config_unit_tests.rsstr_to_os(valid/invalid OS parsing),OS::is(positive/negative matching),ARCH::is(all arch aliases)rust/tests/cache_unit_tests.rsfind_latest_from_cache(empty, single, multiple, lexical sort bug),collect_files_from_cache,is_driver_and_matches_browser_version(major match, empty-major bug),find_best_driver_from_cache(match, fallback, empty)Inline
#[cfg(test)]modules:rust/src/lib.rs—get_index_version(major/minor/patch extraction, out-of-bounds, empty string)rust/src/config.rs—get_env_name(simple key, dash-to-underscore, mixed case, empty suffix)Tests documenting latent bugs (not fixed in this PR)
Two tests intentionally document existing bugs for future fixes:
find_latest_from_cache—sort_by_file_name()orders9.0.0after10.0.0(lexical, not semver)starts_with("")inis_driver_and_matches_browser_version— whenmajor_browser_versionis empty,starts_with("")matches every cached driverThese tests pass today and serve as regression guards when the bugs are fixed.
Fixes #17641