Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.3k
[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 commit82a24a4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Security hardening forwebbrowser 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 forwebbrowser.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""" | ||
| ifurlandurl.lstrip().startswith("-"): | ||
| raiseValueError(f"Invalid URL:{url}") | ||
CopilotAIMar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Becauseregister() can return controllers that don’t inherit fromBaseBrowser (and therefore won’t call_check_url()), invalid dash-prefixed URLs can still reach custom backends viawebbrowser.open(). To make the guarantee match the NEWS entry/title, consider validating once at the public API boundary (module-levelopen() /open_new*()) before dispatching to any controller.
| deftest_reject_dash_prefixes(self): | ||
| browser=self.browser_class(name=CMD_NAME) | ||
| withself.assertRaises(ValueError): | ||
| browser.open(f"--key=val{URL}") | ||
CopilotAIMar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This adds coverage forGenericBrowser, but the new URL validation is also invoked byBackgroundBrowser,UnixBrowser subclasses, and platform-specific controllers. Consider factoring this intoCommandTestMixin (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""" | ||
| ifurlandurl.lstrip().startswith("-"): | ||
| raiseValueError(f"Invalid URL:{url}") |
CopilotAIMar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
| raiseValueError(f"Invalid URL:{url}") | |
| raiseValueError(f"Invalid URL (leading dashes are not allowed):{url!r}") |
Uh oh!
There was an error while loading.Please reload this page.
(cherry picked from commit82a24a4)