[3.11] gh-143930: Reject leading dashes in webbrowser URLs (GH-143931)#146364
[3.11] gh-143930: Reject leading dashes in webbrowser URLs (GH-143931)#146364tomcruiseqi wants to merge 1 commit intopython:3.11from
Conversation
(cherry picked from commit 82a24a4)
tomcruiseqi
changed the title
tomcruiseqi
changed the title
tomcruiseqi
changed the title
There was a problem hiding this comment.
Pull request overview
Security hardening for webbrowser to prevent option-injection style issues when URLs are forwarded to external browser commands.
Changes:
- Add URL validation that rejects inputs whose first non-whitespace character is
-. - Invoke this validation across built-in browser controller implementations before launching/dispatching.
- Add a regression test for the new rejection behavior and a Security NEWS entry.
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 for webbrowser.open(). |
| Lib/webbrowser.py | Introduces _check_url() and applies it to multiple controllers before executing browser actions. |
| Lib/test/test_webbrowser.py | Adds a regression test ensuring dash-prefixed inputs are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @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.
Because register() can return controllers that don’t inherit from BaseBrowser (and therefore won’t call _check_url()), invalid dash-prefixed URLs can still reach custom backends via webbrowser.open(). To make the guarantee match the NEWS entry/title, consider validating once at the public API boundary (module-level open() / open_new*()) before dispatching to any controller.
| 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.
This adds coverage for GenericBrowser, but the new URL validation is also invoked by BackgroundBrowser, UnixBrowser subclasses, and platform-specific controllers. Consider factoring this into CommandTestMixin (or adding equivalent tests per class) so the rejection behavior is enforced across the other command-based browser implementations too.
| 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 includes the raw URL; using repr (or otherwise escaping/quoting) would make the exception unambiguous and avoid embedding control characters/newlines in error output. Consider formatting the URL with !r and/or using a more specific message (e.g., that leading dashes are rejected).
| raise ValueError(f"Invalid URL: {url}") | |
| raise ValueError(f"Invalid URL (leading dashes are not allowed): {url!r}") |
(cherry picked from commit 82a24a4)