[3.10] gh-143930: Reject leading dashes in webbrowser URLs (GH-143931)#146359
[3.10] gh-143930: Reject leading dashes in webbrowser URLs (GH-143931)#146359tomcruiseqi wants to merge 1 commit intopython:3.10from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR backports a security hardening change to webbrowser to prevent option-injection style issues by rejecting URLs that begin with - (after leading whitespace) before passing them to browser launchers.
Changes:
- Add
BaseBrowser._check_url()to reject leading-dash URLs with aValueError. - Invoke
_check_url()from multipleopen()implementations that launch browsers. - Add a regression test and a Security NEWS blurb.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
Misc/NEWS.d/next/Security/2026-01-16-12-04-49.gh-issue-143930.zYC5x3.rst |
Documents the security behavior change in webbrowser.open(). |
Lib/webbrowser.py |
Introduces URL validation and applies it to several browser controllers. |
Lib/test/test_webbrowser.py |
Adds a test ensuring leading-dash inputs are rejected (currently for GenericBrowser). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _check_url(url): | ||
| """Ensures that the URL is safe to pass to subprocesses as a parameter""" | ||
| if url and url.lstrip().startswith("-"): | ||
| raise ValueError(f"Invalid URL: {url}") |
There was a problem hiding this comment.
The ValueError message interpolates the raw URL, which can include control characters/newlines and makes logs/debug output ambiguous. Consider using a safer/clearer representation (e.g., url!r) and/or a message that specifically calls out “URLs must not start with '-' after leading whitespace”.
| raise ValueError(f"Invalid URL: {url}") | |
| raise ValueError( | |
| f"Invalid URL {url!r}: URLs must not start with '-' after leading whitespace" | |
| ) |
| @staticmethod | ||
| def _check_url(url): | ||
| """Ensures that the URL is safe to pass to subprocesses as a parameter""" | ||
| if url and url.lstrip().startswith("-"): | ||
| raise ValueError(f"Invalid URL: {url}") |
There was a problem hiding this comment.
_check_url() is only effective when each browser implementation calls it. Currently MacOSXOSAScript.open() and Grail.open() do not call _check_url(), so on macOS (default controller) and when Grail is available, webbrowser.open() can still accept leading-dash values. If the intent is to reject leading dashes for all webbrowser.open() calls, add _check_url() calls to those implementations (or enforce the check once in the module-level open() function).
| def test_reject_dash_prefixes(self): | ||
| browser = self.browser_class(name=CMD_NAME) | ||
| with self.assertRaises(ValueError): | ||
| browser.open(f"--key=val {URL}") | ||
|
|
There was a problem hiding this comment.
The new test only exercises GenericBrowser.open(), but this change also adds _check_url() calls in several other controllers (e.g., BackgroundBrowser, UnixBrowser, Konqueror, and platform-specific implementations). Consider adding a small parametrized test (or a mixin-based loop) to assert that each affected browser_class.open() rejects leading-dash inputs, so the security behavior is covered across implementations.
(cherry picked from commit 82a24a4) Co-authored-by: Seth Michael Larson <seth@python.org>
69fd15b to
c84b32d
Compare
(cherry picked from commit 82a24a4)