From efee8df1fac413c8de7356cf4c47000b17d967af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 10:52:08 +0100 Subject: [PATCH 1/8] bpo-36021: WindowsDefault does not execute os.startfile() for a local file --- Lib/test/test_webbrowser.py | 13 +++++++++++++ Lib/webbrowser.py | 5 ++++- .../2019-02-19-10-50-22.bpo-36021.T83KHF.rst | 2 ++ 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2019-02-19-10-50-22.bpo-36021.T83KHF.rst diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index 519a9432abe012..90a1eed370e5a7 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -328,5 +328,18 @@ def test_environment_preferred(self): self.assertEqual(webbrowser.get().name, sys.executable) +@unittest.skipIf(sys.platform[:3] != 'win', 'requires Windows') +class TestWindowsDefault(unittest.TestCase): + @mock.patch('os.startfile') + def test_do_not_run_startfile_for_local(self, mock_startfile): + webbrowser.WindowsDefault().open('file:///tmp/test.txt') + self.assertFalse(mock_startfile.assert_called()) + + @mock.patch('os.startfile') + def test_do_run_startfile_with_external_resource(self, mock_startfile): + webbrowser.WindowsDefault().open('https://pythontest.net') + self.assertTrue(mock_startfile.assert_called()) + + if __name__=='__main__': unittest.main() diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index 82bff835fdd0be..5db0751b421e21 100755 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -8,6 +8,7 @@ import sys import subprocess import threading +from urllib.parse import urlparse __all__ = ["Error", "open", "open_new", "open_new_tab", "get", "register"] @@ -578,7 +579,9 @@ def register_standard_browsers(): class WindowsDefault(BaseBrowser): def open(self, url, new=0, autoraise=True): try: - os.startfile(url) + parsed_url = urlparse(url) + if parsed_url.scheme != 'file': + os.startfile(url) except OSError: # [Error 22] No application is associated with the specified # file for this operation: '' diff --git a/Misc/NEWS.d/next/Library/2019-02-19-10-50-22.bpo-36021.T83KHF.rst b/Misc/NEWS.d/next/Library/2019-02-19-10-50-22.bpo-36021.T83KHF.rst new file mode 100644 index 00000000000000..f8887ffcdbe2ee --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-02-19-10-50-22.bpo-36021.T83KHF.rst @@ -0,0 +1,2 @@ +WindowsDefault does not use os.startfile() when the location of the resource is +local file. Contributed by Stéphane Wirtel From 4bdb38e6b6ec79292661c8b55578f36c230fd945 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 12:05:11 +0100 Subject: [PATCH 2/8] Allow some schemes --- Lib/test/test_webbrowser.py | 5 +++++ Lib/webbrowser.py | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index 90a1eed370e5a7..ad6dc9acf38a84 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -335,6 +335,11 @@ def test_do_not_run_startfile_for_local(self, mock_startfile): webbrowser.WindowsDefault().open('file:///tmp/test.txt') self.assertFalse(mock_startfile.assert_called()) + @mock.patch('os.startfile') + def test_do_not_run_startfile_for_local_calc(self, mock_startfile): + webbrowser.WindowsDefault().open('c:\\windows\\system32\\calc.exe') + self.assertFalse(mock_startfile.assert_called()) + @mock.patch('os.startfile') def test_do_run_startfile_with_external_resource(self, mock_startfile): webbrowser.WindowsDefault().open('https://pythontest.net') diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index 5db0751b421e21..3b971cd125b7c5 100755 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -8,7 +8,7 @@ import sys import subprocess import threading -from urllib.parse import urlparse +import urllib.parse __all__ = ["Error", "open", "open_new", "open_new_tab", "get", "register"] @@ -579,8 +579,13 @@ def register_standard_browsers(): class WindowsDefault(BaseBrowser): def open(self, url, new=0, autoraise=True): try: - parsed_url = urlparse(url) - if parsed_url.scheme != 'file': + allowed_schemes = set( + urllib.parse.uses_relative + + urllib.parse.uses_netloc + + urllib.parse.uses_params) - set(['file', '']) + + parsed_url = urllib.parse.urlparse(url) + if parsed_url.scheme in allowed_schemes: os.startfile(url) except OSError: # [Error 22] No application is associated with the specified From 7ac915fc6a14359ec8e58108fa29395d7a4eddbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 12:32:08 +0100 Subject: [PATCH 3/8] Fix the tests, sorry --- Lib/test/test_webbrowser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index ad6dc9acf38a84..8d62b3c7e2dbf8 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -333,17 +333,17 @@ class TestWindowsDefault(unittest.TestCase): @mock.patch('os.startfile') def test_do_not_run_startfile_for_local(self, mock_startfile): webbrowser.WindowsDefault().open('file:///tmp/test.txt') - self.assertFalse(mock_startfile.assert_called()) + mock_startfile.assert_not_called() @mock.patch('os.startfile') def test_do_not_run_startfile_for_local_calc(self, mock_startfile): webbrowser.WindowsDefault().open('c:\\windows\\system32\\calc.exe') - self.assertFalse(mock_startfile.assert_called()) + mock_startfile.assert_not_called() @mock.patch('os.startfile') def test_do_run_startfile_with_external_resource(self, mock_startfile): webbrowser.WindowsDefault().open('https://pythontest.net') - self.assertTrue(mock_startfile.assert_called()) + mock_startfile.assert_called() if __name__=='__main__': From dfd2fa020193cf9eb46866d01323109936e43119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 12:58:06 +0100 Subject: [PATCH 4/8] Allow the non-executable files --- Lib/webbrowser.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index 3b971cd125b7c5..cab676f8b57818 100755 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -576,6 +576,9 @@ def register_standard_browsers(): # if sys.platform[:3] == "win": + def is_executable(path): + return os.path.isfile(path) and os.access(path, os.X_OK) + class WindowsDefault(BaseBrowser): def open(self, url, new=0, autoraise=True): try: @@ -585,8 +588,14 @@ def open(self, url, new=0, autoraise=True): urllib.parse.uses_params) - set(['file', '']) parsed_url = urllib.parse.urlparse(url) - if parsed_url.scheme in allowed_schemes: + if parsed_url.scheme not in allowed_schemes: + path = parsed_url.path if parsed_url.scheme == 'file' else url + + if not is_executable(path): + os.startfile(url) + else: os.startfile(url) + except OSError: # [Error 22] No application is associated with the specified # file for this operation: '' From f0cc80c6758adc2c6076261f3d52ef3d2aa15462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 13:03:22 +0100 Subject: [PATCH 5/8] Use the sys.executable for the tests, and a normal file should be called --- Lib/test/test_webbrowser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index 8d62b3c7e2dbf8..f65f4a11e9fa15 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -331,13 +331,13 @@ def test_environment_preferred(self): @unittest.skipIf(sys.platform[:3] != 'win', 'requires Windows') class TestWindowsDefault(unittest.TestCase): @mock.patch('os.startfile') - def test_do_not_run_startfile_for_local(self, mock_startfile): + def test_do_run_startfile_for_local(self, mock_startfile): webbrowser.WindowsDefault().open('file:///tmp/test.txt') - mock_startfile.assert_not_called() + mock_startfile.assert_called() @mock.patch('os.startfile') def test_do_not_run_startfile_for_local_calc(self, mock_startfile): - webbrowser.WindowsDefault().open('c:\\windows\\system32\\calc.exe') + webbrowser.WindowsDefault().open(sys.executable) mock_startfile.assert_not_called() @mock.patch('os.startfile') From 978a3c106fd9aaf5261d5e03a117eac3b81de855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 13:07:12 +0100 Subject: [PATCH 6/8] Add mocks --- Lib/test/test_webbrowser.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index f65f4a11e9fa15..8bb713b74e21bc 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -331,7 +331,9 @@ def test_environment_preferred(self): @unittest.skipIf(sys.platform[:3] != 'win', 'requires Windows') class TestWindowsDefault(unittest.TestCase): @mock.patch('os.startfile') - def test_do_run_startfile_for_local(self, mock_startfile): + @mock.patch('os.access', return_value=False) + @mock.patch('os.path.isfile', return_value=True) + def test_do_run_startfile_for_local(self, mock_isfile, mock_access, mock_startfile): webbrowser.WindowsDefault().open('file:///tmp/test.txt') mock_startfile.assert_called() From 8054098b4dd494113875b113cc360ef113f42c7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 14:32:26 +0100 Subject: [PATCH 7/8] Read the content of the file and check if it starts with 'MZ' --- Lib/webbrowser.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index cab676f8b57818..8f6f5769f33669 100755 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -577,7 +577,14 @@ def register_standard_browsers(): if sys.platform[:3] == "win": def is_executable(path): - return os.path.isfile(path) and os.access(path, os.X_OK) + import struct + + is_exe = False + with open(path, 'rb') as fp: + s = fp.read(2) + is_exe = s != b'MZ' + + return os.path.isfile(path) and is_exe class WindowsDefault(BaseBrowser): def open(self, url, new=0, autoraise=True): From 815f748f5b3a56818561e3704cd7e52b43b2d612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Wirtel?= Date: Tue, 19 Feb 2019 15:11:24 +0100 Subject: [PATCH 8/8] Use a whitelist instead of an executable --- Lib/test/test_webbrowser.py | 2 +- Lib/webbrowser.py | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_webbrowser.py b/Lib/test/test_webbrowser.py index 8bb713b74e21bc..8ea851b32709da 100644 --- a/Lib/test/test_webbrowser.py +++ b/Lib/test/test_webbrowser.py @@ -334,7 +334,7 @@ class TestWindowsDefault(unittest.TestCase): @mock.patch('os.access', return_value=False) @mock.patch('os.path.isfile', return_value=True) def test_do_run_startfile_for_local(self, mock_isfile, mock_access, mock_startfile): - webbrowser.WindowsDefault().open('file:///tmp/test.txt') + webbrowser.WindowsDefault().open('file:///tmp/test.html') mock_startfile.assert_called() @mock.patch('os.startfile') diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index 8f6f5769f33669..7acb2498ded580 100755 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -576,17 +576,14 @@ def register_standard_browsers(): # if sys.platform[:3] == "win": - def is_executable(path): - import struct - - is_exe = False - with open(path, 'rb') as fp: - s = fp.read(2) - is_exe = s != b'MZ' - - return os.path.isfile(path) and is_exe - class WindowsDefault(BaseBrowser): + def _is_html(self, path): + HTML_EXTENSIONS = ( + '.html', '.htm', '.mht', '.mhtml', '.shtml', + '.xht', '.xhtml' + ) + return os.path.splitext(path)[-1] in HTML_EXTENSIONS + def open(self, url, new=0, autoraise=True): try: allowed_schemes = set( @@ -598,7 +595,7 @@ def open(self, url, new=0, autoraise=True): if parsed_url.scheme not in allowed_schemes: path = parsed_url.path if parsed_url.scheme == 'file' else url - if not is_executable(path): + if self._is_html(path): os.startfile(url) else: os.startfile(url)