From 5f4efced7b17d6538b02caf4e1f22d51e90b6f1d Mon Sep 17 00:00:00 2001 From: Jimmy Date: Sun, 27 May 2018 18:03:43 +1200 Subject: [PATCH 1/8] Sanitize generated filenames for downloads. Replace characters that Windows doesn't like from generated and suggested filenames for downloads. Does not apply to filenames that a user inters via the downloads location prompt. Fixes #3922 --- qutebrowser/browser/downloads.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index dd112e00a..421b8c08b 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -134,6 +134,7 @@ def create_full_filename(basename, filename): Return: The full absolute path, or None if filename creation was not possible. """ + basename = utils.sanitize_filename(basename) # Remove chars which can't be encoded in the filename encoding. # See https://github.com/qutebrowser/qutebrowser/issues/427 encoding = sys.getfilesystemencoding() @@ -159,6 +160,7 @@ def get_filename_question(*, suggested_filename, url, parent=None): url: The URL the download originated from. parent: The parent of the question (a QObject). """ + suggested_filename = utils.sanitize_filename(suggested_filename) encoding = sys.getfilesystemencoding() suggested_filename = utils.force_encoding(suggested_filename, encoding) From 75c6e087c78bd344f416eb629f22df5b46e28279 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Mon, 4 Jun 2018 10:54:02 +1200 Subject: [PATCH 2/8] Refactor individual tmpdir fixtures. I was adding a downloads one and saw the opportunity to refactor. Didn't end up saving that much because I couldn't figure out how to get the method that does the work to use the fixtures while also taking an argument, so they have to be passed through from the calling ones. Oh well. --- tests/helpers/fixtures.py | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/tests/helpers/fixtures.py b/tests/helpers/fixtures.py index ec562c3b4..caf5a18e0 100644 --- a/tests/helpers/fixtures.py +++ b/tests/helpers/fixtures.py @@ -459,16 +459,35 @@ def mode_manager(win_registry, config_stub, qapp): objreg.delete('mode-manager', scope='window', window=0) +def standarddir_tmpdir(folder, monkeypatch, tmpdir): + """Set tmpdir/config as the configdir. + + Use this to avoid creating a 'real' config dir (~/.config/qute_test). + """ + confdir = tmpdir / folder + confdir.ensure(dir=True) + if hasattr(standarddir, folder): + monkeypatch.setattr(standarddir, folder, + lambda **_kwargs: str(confdir)) + return confdir + + +@pytest.fixture +def download_tmpdir(monkeypatch, tmpdir): + """Set tmpdir/download as the downloaddir. + + Use this to avoid creating a 'real' download dir (~/.config/qute_test). + """ + return standarddir_tmpdir('download', monkeypatch, tmpdir) + + @pytest.fixture def config_tmpdir(monkeypatch, tmpdir): """Set tmpdir/config as the configdir. Use this to avoid creating a 'real' config dir (~/.config/qute_test). """ - confdir = tmpdir / 'config' - confdir.ensure(dir=True) - monkeypatch.setattr(standarddir, 'config', lambda auto=False: str(confdir)) - return confdir + return standarddir_tmpdir('config', monkeypatch, tmpdir) @pytest.fixture @@ -477,10 +496,7 @@ def data_tmpdir(monkeypatch, tmpdir): Use this to avoid creating a 'real' data dir (~/.local/share/qute_test). """ - datadir = tmpdir / 'data' - datadir.ensure(dir=True) - monkeypatch.setattr(standarddir, 'data', lambda system=False: str(datadir)) - return datadir + return standarddir_tmpdir('data', monkeypatch, tmpdir) @pytest.fixture @@ -489,10 +505,7 @@ def runtime_tmpdir(monkeypatch, tmpdir): Use this to avoid creating a 'real' runtime dir. """ - runtimedir = tmpdir / 'runtime' - runtimedir.ensure(dir=True) - monkeypatch.setattr(standarddir, 'runtime', lambda: str(runtimedir)) - return runtimedir + return standarddir_tmpdir('runtime', monkeypatch, tmpdir) @pytest.fixture @@ -501,10 +514,7 @@ def cache_tmpdir(monkeypatch, tmpdir): Use this to avoid creating a 'real' cache dir (~/.cache/qute_test). """ - cachedir = tmpdir / 'cache' - cachedir.ensure(dir=True) - monkeypatch.setattr(standarddir, 'cache', lambda: str(cachedir)) - return cachedir + return standarddir_tmpdir('cache', monkeypatch, tmpdir) @pytest.fixture From 2510fde80fa051c7cbaf9dee9e6095a7ecc5eb54 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Mon, 4 Jun 2018 10:57:16 +1200 Subject: [PATCH 3/8] Add unit test for sanitized download filenames. I think this covers the right path despite not using either of the backend specific download code. They both either call `set_target()` directly with a guessed filename or from the prompt question. When called directly though that basename set by `_init_item()` is used. It's a little high level but we already know that sanitize_filenames does, I wanted to test that it gets called. --- tests/unit/browser/webkit/test_downloads.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index 571e21704..f14c417f9 100644 --- a/tests/unit/browser/webkit/test_downloads.py +++ b/tests/unit/browser/webkit/test_downloads.py @@ -96,3 +96,24 @@ class TestDownloadTarget: ]) def test_class_hierarchy(self, obj): assert isinstance(obj, downloads._DownloadTarget) + + +@pytest.mark.parametrize('raw, expected', [ + ('http://foo/bar', 'bar'), + ('A *|<>\\: bear!', 'A ______ bear!') +]) +def test_sanitized_filenames(raw, expected, config_stub, download_tmpdir): + manager = downloads.AbstractDownloadManager() + target = downloads.FileDownloadTarget(download_tmpdir) + item = downloads.AbstractDownloadItem() + + # Don't try to start a timer outside of a QThread + manager._update_timer.isActive = lambda: True + + # Abstract methods + item._ensure_can_set_filename = lambda *args: True + item._after_set_filename = lambda *args: True + + manager._init_item(item, True, raw) + item.set_target(target) + assert item._filename.endswith(expected) From d31fa5229dd995041c4369cecf978eab3b0bc1b4 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Tue, 5 Jun 2018 20:36:07 +1200 Subject: [PATCH 4/8] Stringify py.path object for py3.5 https://www.python.org/dev/peps/pep-0519/ Huh, I didn't realise cpython and the stdlib look up "special" methods on the class only. --- tests/unit/browser/webkit/test_downloads.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index f14c417f9..68573002c 100644 --- a/tests/unit/browser/webkit/test_downloads.py +++ b/tests/unit/browser/webkit/test_downloads.py @@ -104,7 +104,7 @@ class TestDownloadTarget: ]) def test_sanitized_filenames(raw, expected, config_stub, download_tmpdir): manager = downloads.AbstractDownloadManager() - target = downloads.FileDownloadTarget(download_tmpdir) + target = downloads.FileDownloadTarget(str(download_tmpdir)) item = downloads.AbstractDownloadItem() # Don't try to start a timer outside of a QThread From ffd6ffef45d04ee0cea99365ce5a5f908451cab2 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Tue, 26 Jun 2018 15:38:01 +1200 Subject: [PATCH 5/8] Add force_encoding call to sanitize_filename. So that callers don't have to call both. --- qutebrowser/browser/downloads.py | 4 +--- qutebrowser/utils/utils.py | 6 ++++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 421b8c08b..27408ffd6 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -135,11 +135,11 @@ def create_full_filename(basename, filename): The full absolute path, or None if filename creation was not possible. """ basename = utils.sanitize_filename(basename) + # Filename can be a full path so don't use sanitize_filename on it. # Remove chars which can't be encoded in the filename encoding. # See https://github.com/qutebrowser/qutebrowser/issues/427 encoding = sys.getfilesystemencoding() filename = utils.force_encoding(filename, encoding) - basename = utils.force_encoding(basename, encoding) if os.path.isabs(filename) and (os.path.isdir(filename) or filename.endswith(os.sep)): # We got an absolute directory from the user, so we save it under @@ -161,8 +161,6 @@ def get_filename_question(*, suggested_filename, url, parent=None): parent: The parent of the question (a QObject). """ suggested_filename = utils.sanitize_filename(suggested_filename) - encoding = sys.getfilesystemencoding() - suggested_filename = utils.force_encoding(suggested_filename, encoding) q = usertypes.Question(parent) q.title = "Save file to:" diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index 4554aef2e..8beb21279 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -499,6 +499,12 @@ def sanitize_filename(name, replacement='_'): """ if replacement is None: replacement = '' + + # Remove chars which can't be encoded in the filename encoding. + # See https://github.com/qutebrowser/qutebrowser/issues/427 + encoding = sys.getfilesystemencoding() + name = force_encoding(name, encoding) + # Bad characters taken from Windows, there are even fewer on Linux # See also # https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words From 1febcb9fce933a4045395b9b7164c3c0051511a3 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Tue, 26 Jun 2018 15:39:16 +1200 Subject: [PATCH 6/8] Also call sanitize_filename in TempDownloadManager Just in case the suggested name used for the temp file suffix somehow has an illegal character in it. --- qutebrowser/browser/downloads.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 27408ffd6..517d93638 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -1224,8 +1224,7 @@ class TempDownloadManager: A tempfile.NamedTemporaryFile that should be used to save the file. """ tmpdir = self._get_tmpdir() - encoding = sys.getfilesystemencoding() - suggested_name = utils.force_encoding(suggested_name, encoding) + suggested_name = utils.sanitize_filename(suggested_name) # Make sure that the filename is not too long suggested_name = utils.elide_filename(suggested_name, 50) fobj = tempfile.NamedTemporaryFile(dir=tmpdir.name, delete=False, From 747acfe7fad3a0fa70d88b6fa8d2891fd0eecb15 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Tue, 26 Jun 2018 16:24:37 +1200 Subject: [PATCH 7/8] Makes sanitize_filenames platform dependant. Hopefully having the posix bad_chars list so minimal won't ruin anyones day. --- qutebrowser/utils/utils.py | 10 ++++++++-- tests/unit/browser/webkit/test_downloads.py | 19 +++++++++++++++---- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index 8beb21279..f3ab6b5ee 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -505,10 +505,16 @@ def sanitize_filename(name, replacement='_'): encoding = sys.getfilesystemencoding() name = force_encoding(name, encoding) - # Bad characters taken from Windows, there are even fewer on Linux # See also # https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words - bad_chars = '\\/:*?"<>|' + if is_windows: + bad_chars = '\\/:*?"<>|' + elif is_mac: + # Colons can be confusing in finder https://superuser.com/a/326627 + bad_chars = '/:' + else: + bad_chars = '/' + for bad_char in bad_chars: name = name.replace(bad_char, replacement) return name diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index 68573002c..8dc80f92c 100644 --- a/tests/unit/browser/webkit/test_downloads.py +++ b/tests/unit/browser/webkit/test_downloads.py @@ -22,6 +22,14 @@ import pytest from qutebrowser.browser import downloads, qtnetworkdownloads +@pytest.fixture(autouse=True) +def is_platform(monkeypatch, platform="windows"): + for p in ["mac", "linux", "posix", "windows"]: + monkeypatch.setattr( + 'qutebrowser.utils.utils.is_{}'.format(p), + p == platform) + + def test_download_model(qapp, qtmodeltester, config_stub, cookiejar_and_cache, fake_args): """Simple check for download model internals.""" @@ -98,11 +106,14 @@ class TestDownloadTarget: assert isinstance(obj, downloads._DownloadTarget) -@pytest.mark.parametrize('raw, expected', [ - ('http://foo/bar', 'bar'), - ('A *|<>\\: bear!', 'A ______ bear!') +@pytest.mark.parametrize('raw, platform, expected', [ + ('http://foo/bar', 'windows', 'bar'), + ('A *|<>\\: bear!', 'windows', 'A ______ bear!'), + ('A *|<>\\: bear!', 'posix', 'A *|<>\\: bear!') ]) -def test_sanitized_filenames(raw, expected, config_stub, download_tmpdir): +def test_sanitized_filenames(raw, platform, expected, config_stub, + download_tmpdir, monkeypatch): + is_platform(monkeypatch, platform) manager = downloads.AbstractDownloadManager() target = downloads.FileDownloadTarget(str(download_tmpdir)) item = downloads.AbstractDownloadItem() From 680ae89ffd82795c52c7b2322f19bdd5819de613 Mon Sep 17 00:00:00 2001 From: Jimmy Date: Tue, 26 Jun 2018 17:47:10 +1200 Subject: [PATCH 8/8] Also mock platform to windows for utils tests. This is the same fixture I added in tests/unit/browser/webkit/test_downloads.py --- tests/unit/utils/test_utils.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index ef2b6c8d4..b8f9b932e 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -113,6 +113,14 @@ class TestElidingFilenames: assert utils.elide_filename(filename, length) == expected +@pytest.fixture(autouse=True) +def is_platform(monkeypatch, platform="windows"): + for p in ["mac", "linux", "posix", "windows"]: + monkeypatch.setattr( + 'qutebrowser.utils.utils.is_{}'.format(p), + p == platform) + + @pytest.fixture(params=[True, False]) def freezer(request, monkeypatch): if request.param and not getattr(sys, 'frozen', False): @@ -629,12 +637,15 @@ def test_force_encoding(inp, enc, expected): assert utils.force_encoding(inp, enc) == expected -@pytest.mark.parametrize('inp, expected', [ - ('normal.txt', 'normal.txt'), - ('user/repo issues.mht', 'user_repo issues.mht'), - (' - "*?:|', '_Test_File_ - _____'), +@pytest.mark.parametrize('inp, platform, expected', [ + ('normal.txt', 'windows', 'normal.txt'), + ('user/repo issues.mht', 'windows', 'user_repo issues.mht'), + (' - "*?:|', 'windows', '_Test_File_ - _____'), + (' - "*?:|', 'mac', ' - "*?_|'), + (' - "*?:|', 'posix', ' - "*?:|'), ]) -def test_sanitize_filename(inp, expected): +def test_sanitize_filename(inp, platform, expected, monkeypatch): + is_platform(monkeypatch, platform) assert utils.sanitize_filename(inp) == expected