-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
gh-137586: Replace 'osascript' with 'open' on macOS in webbrowser #146439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
00fa6c7
f697cd5
7781033
6066221
d54293f
080197e
fdd6649
8e1eef4
e193626
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -491,10 +491,13 @@ def register_standard_browsers(): | |||||
| _tryorder = [] | ||||||
|
|
||||||
| if sys.platform == 'darwin': | ||||||
| register("MacOSX", None, MacOSXOSAScript('default')) | ||||||
| register("chrome", None, MacOSXOSAScript('google chrome')) | ||||||
| register("firefox", None, MacOSXOSAScript('firefox')) | ||||||
| register("safari", None, MacOSXOSAScript('safari')) | ||||||
| register("MacOSX", None, MacOSX('default')) | ||||||
| register("chrome", None, MacOSX('google chrome')) | ||||||
| register("chromium", None, MacOSX('chromium')) | ||||||
| register("firefox", None, MacOSX('firefox')) | ||||||
| register("safari", None, MacOSX('safari')) | ||||||
| register("opera", None, MacOSX('opera')) | ||||||
| register("microsoft-edge", None, MacOSX('microsoft edge')) | ||||||
| # macOS can use below Unix support (but we prefer using the macOS | ||||||
| # specific stuff) | ||||||
|
|
||||||
|
|
@@ -613,8 +616,131 @@ def open(self, url, new=0, autoraise=True): | |||||
| # | ||||||
|
|
||||||
| if sys.platform == 'darwin': | ||||||
| def _macos_default_browser_bundle_id(): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function leaks memory due to not performing objective-c reference count updates. Also: This introduces a new dependency on an Apple system framework, which in the past has caused problems for folks starting new worker processes using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that hardcoding The core motivation is that On |
||||||
| """Return the bundle ID of the default web browser via NSWorkspace. | ||||||
|
|
||||||
| Uses the Objective-C runtime directly to call | ||||||
| NSWorkspace.sharedWorkspace().URLForApplicationToOpenURL() with a | ||||||
| probe https:// URL, then reads the bundle identifier from the | ||||||
| resulting NSBundle. Returns None if ctypes is unavailable or the | ||||||
| lookup fails for any reason. | ||||||
| """ | ||||||
| try: | ||||||
| from ctypes import cdll, c_void_p, c_char_p | ||||||
| from ctypes.util import find_library | ||||||
|
|
||||||
| # NSWorkspace is an AppKit class; load AppKit to register it. | ||||||
| cdll.LoadLibrary( | ||||||
| '/System/Library/Frameworks/AppKit.framework/AppKit' | ||||||
| ) | ||||||
| objc = cdll.LoadLibrary(find_library('objc')) | ||||||
| objc.objc_getClass.restype = c_void_p | ||||||
| objc.sel_registerName.restype = c_void_p | ||||||
| objc.objc_msgSend.restype = c_void_p | ||||||
|
|
||||||
| def cls(name): | ||||||
| return objc.objc_getClass(name) | ||||||
|
|
||||||
| def sel(name): | ||||||
| return objc.sel_registerName(name) | ||||||
|
|
||||||
| # Build probe NSURL for "https://python.org" | ||||||
| NSString = cls(b'NSString') | ||||||
| objc.objc_msgSend.argtypes = [c_void_p, c_void_p, c_char_p] | ||||||
| ns_str = objc.objc_msgSend( | ||||||
| NSString, sel(b'stringWithUTF8String:'), b'https://python.org' | ||||||
| ) | ||||||
|
|
||||||
| NSURL = cls(b'NSURL') | ||||||
| objc.objc_msgSend.argtypes = [c_void_p, c_void_p, c_void_p] | ||||||
| probe_url = objc.objc_msgSend(NSURL, sel(b'URLWithString:'), ns_str) | ||||||
|
|
||||||
| # Ask NSWorkspace which app handles https:// | ||||||
| NSWorkspace = cls(b'NSWorkspace') | ||||||
| objc.objc_msgSend.argtypes = [c_void_p, c_void_p] | ||||||
| workspace = objc.objc_msgSend(NSWorkspace, sel(b'sharedWorkspace')) | ||||||
| if not workspace: | ||||||
| return None | ||||||
|
|
||||||
| objc.objc_msgSend.argtypes = [c_void_p, c_void_p, c_void_p] | ||||||
| app_url = objc.objc_msgSend( | ||||||
| workspace, sel(b'URLForApplicationToOpenURL:'), probe_url | ||||||
| ) | ||||||
| if not app_url: | ||||||
| return None | ||||||
|
|
||||||
| # Get bundle identifier from that app's NSBundle | ||||||
| NSBundle = cls(b'NSBundle') | ||||||
| bundle = objc.objc_msgSend(NSBundle, sel(b'bundleWithURL:'), app_url) | ||||||
| if not bundle: | ||||||
| return None | ||||||
|
|
||||||
| objc.objc_msgSend.argtypes = [c_void_p, c_void_p] | ||||||
| bundle_id_ns = objc.objc_msgSend(bundle, sel(b'bundleIdentifier')) | ||||||
| if not bundle_id_ns: | ||||||
| return None | ||||||
|
|
||||||
| objc.objc_msgSend.restype = c_char_p | ||||||
| bundle_id_bytes = objc.objc_msgSend(bundle_id_ns, sel(b'UTF8String')) | ||||||
| return bundle_id_bytes.decode() if bundle_id_bytes else None | ||||||
| except Exception: | ||||||
| return None | ||||||
|
|
||||||
| class MacOSX(BaseBrowser): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been macOS for 10 years, shall we use drop the X for the new class?
Suggested change
|
||||||
| """Launcher class for macOS browsers, using /usr/bin/open. | ||||||
|
|
||||||
| For http/https URLs with the default browser, /usr/bin/open is called | ||||||
| directly; macOS routes these to the registered browser. | ||||||
|
|
||||||
| For all other URL schemes (e.g. file://) and for named browsers, | ||||||
| /usr/bin/open -b <bundle-id> is used so that the URL is always passed | ||||||
| to a browser application rather than dispatched by the OS file handler. | ||||||
| This prevents file injection attacks where a file:// URL pointing to an | ||||||
| executable bundle could otherwise be launched by the OS. | ||||||
|
|
||||||
| Named browsers with known bundle IDs use -b; unknown names fall back | ||||||
| to -a. | ||||||
| """ | ||||||
|
|
||||||
| _BUNDLE_IDS = { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a |
||||||
| 'google chrome': 'com.google.Chrome', | ||||||
| 'firefox': 'org.mozilla.firefox', | ||||||
| 'safari': 'com.apple.Safari', | ||||||
| 'chromium': 'org.chromium.Chromium', | ||||||
| 'opera': 'com.operasoftware.Opera', | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have Opera installed on macOS, and confirm this is the correct bundle ID in the plist, but whilst this works: import webbrowser
web = webbrowser.get("firefox")
web.open("https://www.python.org/")This doesn't: import webbrowser
web = webbrowser.get("opera")
web.open("https://www.python.org/")Traceback (most recent call last):
File "/Users/hugo/github/python/cpython/main/1.py", line 2, in <module>
web = webbrowser.get("opera")
File "/Users/hugo/github/python/cpython/main/Lib/webbrowser.py", line 68, in get
raise Error("could not locate runnable browser")
webbrowser.Error: could not locate runnable browserDo these new ones need registering too, or removing from here? |
||||||
| 'microsoft edge': 'com.microsoft.Edge', | ||||||
| } | ||||||
|
|
||||||
| def open(self, url, new=0, autoraise=True): | ||||||
| sys.audit("webbrowser.open", url) | ||||||
| self._check_url(url) | ||||||
| if self.name == 'default': | ||||||
| proto, sep, _ = url.partition(':') | ||||||
| if sep and proto.lower() in {'http', 'https'}: | ||||||
| cmd = ['/usr/bin/open', url] | ||||||
| else: | ||||||
| bundle_id = _macos_default_browser_bundle_id() | ||||||
| if bundle_id: | ||||||
| cmd = ['/usr/bin/open', '-b', bundle_id, url] | ||||||
| else: | ||||||
| cmd = ['/usr/bin/open', url] | ||||||
| else: | ||||||
| bundle_id = self._BUNDLE_IDS.get(self.name.lower()) | ||||||
| if bundle_id: | ||||||
| cmd = ['/usr/bin/open', '-b', bundle_id, url] | ||||||
| else: | ||||||
| cmd = ['/usr/bin/open', '-a', self.name, url] | ||||||
| proc = subprocess.run(cmd, stderr=subprocess.DEVNULL) | ||||||
| return proc.returncode == 0 | ||||||
|
|
||||||
| class MacOSXOSAScript(BaseBrowser): | ||||||
| def __init__(self, name='default'): | ||||||
| import warnings | ||||||
| warnings.warn( | ||||||
| "MacOSXOSAScript is deprecated, use MacOSX instead.", | ||||||
| DeprecationWarning, | ||||||
| stacklevel=2, | ||||||
| ) | ||||||
| super().__init__(name) | ||||||
|
|
||||||
| def open(self, url, new=0, autoraise=True): | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Add :class:`!MacOSX` to :mod:`webbrowser` for macOS, which opens URLs via | ||
| ``/usr/bin/open`` instead of piping AppleScript to ``osascript``. | ||
| Deprecate :class:`!MacOSXOSAScript` in favour of :class:`!MacOSX`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Fix a PATH-injection vulnerability in :mod:`webbrowser` on macOS where | ||
| ``osascript`` was invoked without an absolute path. The new :class:`!MacOSX` | ||
| class uses ``/usr/bin/open`` directly, eliminating the dependency on | ||
| ``osascript`` entirely. |
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.
3.15
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.
And precede this
deprecatedblock with aversionaddedfor the new class, and move both afterversionchanged:: 3.13below.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.
Also the
MacOSXOSAScripts in the table above need updating, and should we also addchromeandfirefox(with note (3)) as also returning the new class?