User description
💥 What does this PR do?
This PR makes a slight change to the logic for finding the Selenium Manager binary, so it doesn't give a false positive if there is a directory named selenium-manager where the executable should be if you build/install from source or a local sdist package. This also adds a check to verify the Selenium Manager binary exists if its location is specified in an environment variable, so it's clear why it is failing.
It also converts some old string formatting to use f-strings and adds information to the docstring about where we look for the binary.
🔄 Types of changes
- Cleanup (formatting, renaming)
- Bug fix (backwards compatible)
PR Type
Bug fix, Tests
Description
-
Fix false positive when directory named selenium-manager exists instead of executable
-
Change compiled_path.exists() to compiled_path.is_file() for proper verification
-
Convert string formatting to f-strings for consistency
-
Enhance docstring with binary search order and error conditions
Diagram Walkthrough
flowchart LR
A["Binary Detection Logic"] -->|Check env variable| B["SE_MANAGER_PATH"]
A -->|Check compiled path| C["compiled_path.is_file"]
C -->|Old: exists| D["False Positive<br/>on directory"]
C -->|New: is_file| E["Correct verification<br/>of executable"]
A -->|Check wheel package| F["Platform-specific binary"]
A -->|Fail| G["WebDriverException"]
File Walkthrough
| Relevant files |
|---|
| Bug fix |
selenium_manager.pyFix binary detection and modernize string formatting
py/selenium/webdriver/common/selenium_manager.py
- Changed
compiled_path.exists() to compiled_path.is_file() to properly
verify executable instead of accepting directories
- Converted three logger calls from old string formatting (
%s) to
f-strings
- Enhanced docstring with numbered list of binary search locations and
additional error condition documentation
- Added blank line after license header for PEP 8 compliance
|
+14/-6 |
|
PR Compliance Guide 🔍
Below is a summary of compliance checks for this PR:
| Security Compliance |
| 🟢 | No security concerns identified
No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
|
| Ticket Compliance |
| ⚪ | 🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->
|
| Codebase Duplication Compliance |
| ⚪ | Codebase context is not defined
Follow the guide to enable codebase context checks.
|
| Custom Compliance |
| 🟢 |
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: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status: Passed
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: 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: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
Action Logging: The new code paths that determine and validate the Selenium Manager binary location do not
add or update any audit logging for critical actions, which may be acceptable for library
code but cannot be verified from this diff alone.
Referred Code
if (env_path := os.getenv("SE_MANAGER_PATH")) is not None:
logger.debug(f"Selenium Manager set by env SE_MANAGER_PATH to: {env_path}")
path = Path(env_path)
elif compiled_path.is_file():
path = compiled_path
else:
allowed = {
("darwin", "any"): "macos/selenium-manager",
("win32", "any"): "windows/selenium-manager.exe",
("cygwin", "any"): "windows/selenium-manager.exe",
("linux", "x86_64"): "linux/selenium-manager",
("freebsd", "x86_64"): "linux/selenium-manager",
("openbsd", "x86_64"): "linux/selenium-manager",
}
arch = platform.machine() if sys.platform in ("linux", "freebsd", "openbsd") else "any"
if sys.platform in ["freebsd", "openbsd"]:
logger.warning(f"Selenium Manager binary may not be compatible with {sys.platform}; verify settings")
location = allowed.get((sys.platform, arch))
if location is None:
... (clipped 10 lines)
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:
Error Detail: Raised WebDriverException messages include platform and path details which are likely
acceptable for internal exceptions but could be sensitive if surfaced to end users;
visibility cannot be determined from this diff.
Referred Code
location = allowed.get((sys.platform, arch))
if location is None:
raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}")
path = Path(__file__).parent.joinpath(location)
if path is None or not path.is_file():
raise WebDriverException(f"Unable to obtain working Selenium Manager binary; {path}")
Learn more about managing compliance generic rules or creating your own custom rules
|
- [ ] Update <!-- /compliance --update_compliance=true -->
|
Compliance status legend
🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | Impact |
Learned
best practice |
✅ Validate environment variable path
Suggestion Impact:The commit changed the code to create a Path from SE_MANAGER_PATH and only use it if it is a file; otherwise it sets path to None, effectively validating the environment variable.
code diff:
if (env_path := os.getenv("SE_MANAGER_PATH")) is not None:
logger.debug(f"Selenium Manager set by env SE_MANAGER_PATH to: {env_path}")
- path = Path(env_path)
+ path_candidate = Path(env_path)
+ path = path_candidate if path_candidate.is_file() else None
Validate that SE_MANAGER_PATH points to an existing executable file and ignore
or error out if it does not.
py/selenium/webdriver/common/selenium_manager.py [81-83]
if (env_path := os.getenv("SE_MANAGER_PATH")) is not None:
logger.debug(f"Selenium Manager set by env SE_MANAGER_PATH to: {env_path}")
- path = Path(env_path)
+ env_path = env_path.strip()
+ candidate = Path(env_path)
+ if candidate.is_file():
+ path = candidate
+ else:
+ logger.warning(f"SE_MANAGER_PATH does not point to a file: {env_path}")
[Suggestion processed]
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Validate and sanitize external inputs such as environment variables before use to avoid invalid paths or injection issues.
| Low
|
- [ ] Update <!-- /improve_multi --more_suggestions=true -->
| |
SeleniumHQ