diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 61d67c3cb..92d846bd8 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -135,11 +135,12 @@ def create_full_filename(basename, filename): Return: 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 @@ -160,8 +161,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). """ - encoding = sys.getfilesystemencoding() - suggested_filename = utils.force_encoding(suggested_filename, encoding) + suggested_filename = utils.sanitize_filename(suggested_filename) q = usertypes.Question(parent) q.title = "Save file to:" @@ -1260,8 +1260,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, diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index e9d4c3659..f2e19a743 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -500,10 +500,22 @@ def sanitize_filename(name, replacement='_'): """ if replacement is None: replacement = '' - # Bad characters taken from Windows, there are even fewer on Linux + + # 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) + # 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/helpers/fixtures.py b/tests/helpers/fixtures.py index 5bf841643..fac23ae1c 100644 --- a/tests/helpers/fixtures.py +++ b/tests/helpers/fixtures.py @@ -515,16 +515,35 @@ def mode_manager(win_registry, config_stub, key_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 @@ -533,10 +552,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 @@ -545,10 +561,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 @@ -557,10 +570,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 diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index b42caed00..94a11d552 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.""" @@ -92,3 +100,27 @@ class TestDownloadTarget: ]) def test_class_hierarchy(self, obj): assert isinstance(obj, downloads._DownloadTarget) + + +@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, platform, expected, config_stub, + download_tmpdir, monkeypatch): + is_platform(monkeypatch, platform) + manager = downloads.AbstractDownloadManager() + target = downloads.FileDownloadTarget(str(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) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index e8a96d448..3117b6a27 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): @@ -633,12 +641,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