From 3da21a32d2b31c4f486dac6f42fdadd0656ddf44 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 23 Mar 2017 14:59:22 +0100 Subject: [PATCH 1/7] treat E: and E:\ the same when downloading Fixes #2305 --- qutebrowser/browser/downloads.py | 2 ++ qutebrowser/browser/webkit/mhtml.py | 2 ++ qutebrowser/utils/utils.py | 19 +++++++++++++++++++ tests/unit/utils/test_utils.py | 13 +++++++++++++ 4 files changed, 36 insertions(+) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 51ccdbfeb..8820ffb9c 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -583,6 +583,8 @@ class AbstractDownloadItem(QObject): """ global last_used_directory filename = os.path.expanduser(filename) + if sys.platform == "win32": + filename = utils.expand_windows_drive(filename) self._ensure_can_set_filename(filename) self._filename = create_full_filename(self.basename, filename) diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index bc9ea2695..79b0dd320 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -554,6 +554,8 @@ def start_download_checked(target, tab): dest = utils.force_encoding(target.filename, encoding) dest = os.path.expanduser(dest) + if sys.platform == "win32": + dest = utils.expand_windows_drive(dest) # See if we already have an absolute path path = downloads.create_full_filename(default_name, dest) diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index fd4466e2a..2aca78168 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -20,6 +20,7 @@ """Other utilities which don't fit anywhere else.""" import io +import re import sys import enum import json @@ -845,3 +846,21 @@ def open_file(filename, cmdline=None): def unused(_arg): """Function which does nothing to avoid pylint complaining.""" pass + + +def expand_windows_drive(path): + r"""Expand a drive-path like E: into E:\. + + Does nothing for other paths. + + Args: + path: The path to expand. + """ + # Usually, "E:" on Windows refers to the current working directory on drive + # E:\. The correct way to specifify drive E: is "E:\", but most users + # probably don't use the "multiple working directories" feature and expect + # "E:" and "E:\" to be equal. + if re.match(r'[A-Z]:$', path, re.IGNORECASE): + return path + "\\" + else: + return path diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index 12330aa73..c60b640d6 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -916,3 +916,16 @@ class TestOpenFile: def test_unused(): utils.unused(None) + + +@pytest.mark.parametrize('path, expected', [ + ('E:', 'E:\\'), + ('e:', 'e:\\'), + ('E:foo', 'E:foo'), + ('E:\\', 'E:\\'), + ('E:\\foo', 'E:\\foo'), + ('foo:', 'foo:'), + ('foo:bar', 'foo:bar'), +]) +def test_expand_windows_drive(path, expected): + assert utils.expand_windows_drive(path) == expected From 9d905ebb5c4ae167eb890a1e0fec5636f8818478 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 11:04:20 +0100 Subject: [PATCH 2/7] disallow filenames like E:filename Per-drive working directories are not really supported. --- qutebrowser/browser/downloads.py | 2 -- qutebrowser/browser/webkit/mhtml.py | 2 -- qutebrowser/mainwindow/prompt.py | 32 +++++++++++++++++++++++++++-- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 8820ffb9c..51ccdbfeb 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -583,8 +583,6 @@ class AbstractDownloadItem(QObject): """ global last_used_directory filename = os.path.expanduser(filename) - if sys.platform == "win32": - filename = utils.expand_windows_drive(filename) self._ensure_can_set_filename(filename) self._filename = create_full_filename(self.basename, filename) diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index 79b0dd320..bc9ea2695 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -554,8 +554,6 @@ def start_download_checked(target, tab): dest = utils.force_encoding(target.filename, encoding) dest = os.path.expanduser(dest) - if sys.platform == "win32": - dest = utils.expand_windows_drive(dest) # See if we already have an absolute path path = downloads.create_full_filename(default_name, dest) diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index afe8a72dd..b8b842057 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -19,15 +19,18 @@ """Showing prompts above the statusbar.""" +import re +import sys import os.path import html import collections import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QTimer, QDir, QModelIndex, - QItemSelectionModel, QObject, QEventLoop) + QItemSelectionModel, QObject, QEventLoop, QPoint) from PyQt5.QtWidgets import (QWidget, QGridLayout, QVBoxLayout, QLineEdit, - QLabel, QFileSystemModel, QTreeView, QSizePolicy) + QLabel, QFileSystemModel, QTreeView, QSizePolicy, + QToolTip) from qutebrowser.browser import downloads from qutebrowser.config import style, config @@ -642,8 +645,29 @@ class FilenamePrompt(_BasePrompt): self._file_model.directoryLoaded.connect( lambda: self._file_model.sort(0)) + def _transform_path(self, path): + r"""Do platform-specific transformations, like changing E: to E:\. + + Returns None if the path is invalid on the current platform. + """ + if sys.platform != "win32": + return path + path = utils.expand_windows_drive(path) + # Drive dependent working directories are not supported, e.g. + # E:filename is invalid + if re.match(r'[A-Z]:[^\\]', path, re.IGNORECASE): + return None + return path + + def _show_error(self, msg): + QToolTip.showText(self._lineedit.mapToGlobal(QPoint(0, 0)), msg) + def accept(self, value=None): text = value if value is not None else self._lineedit.text() + text = self._transform_path(text) + if text is None: + self._show_error("Invalid filename") + return False self.question.answer = text return True @@ -695,6 +719,10 @@ class DownloadFilenamePrompt(FilenamePrompt): def accept(self, value=None): text = value if value is not None else self._lineedit.text() + text = self._transform_path(text) + if text is None: + self._show_error("Invalid filename") + return False self.question.answer = downloads.FileDownloadTarget(text) return True From bc4430e5d9863cc30c3b807d37ec7a32c64f4d10 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 11:36:19 +0100 Subject: [PATCH 3/7] prevent reserved filenames on Windows Fixes #82 Prevents filenames like COM1, ... --- qutebrowser/mainwindow/prompt.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index b8b842057..ff676146d 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -23,6 +23,7 @@ import re import sys import os.path import html +import pathlib import collections import sip @@ -657,6 +658,10 @@ class FilenamePrompt(_BasePrompt): # E:filename is invalid if re.match(r'[A-Z]:[^\\]', path, re.IGNORECASE): return None + # Paths like COM1, ... + # See https://github.com/qutebrowser/qutebrowser/issues/82 + if pathlib.Path(path).is_reserved(): + return None return path def _show_error(self, msg): From 07b3a7db7c0fd7f5f66aaa53c9f475532bb84410 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 11:55:17 +0100 Subject: [PATCH 4/7] add integration tests for reserved filenames --- qutebrowser/mainwindow/prompt.py | 1 + tests/end2end/features/downloads.feature | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index ff676146d..2f833c3a3 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -665,6 +665,7 @@ class FilenamePrompt(_BasePrompt): return path def _show_error(self, msg): + log.prompt.error(msg) QToolTip.showText(self._lineedit.mapToGlobal(QPoint(0, 0)), msg) def accept(self, value=None): diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 444cf8721..b47edf8d8 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -126,6 +126,20 @@ Feature: Downloading things from a website. Then the downloaded file ../foo should not exist And the downloaded file foo should exist + @windows + Scenario: Downloading a file to a reserved path + When I open data/downloads/download.bin + And I run :prompt-accept COM1 + And I run :prompt-cancel + Then "Invalid filename" should be logged + + @windows + Scenario: Downloading a file to a drive-relative working directory + When I open data/downloads/download.bin + And I run :prompt-accept C:foobar + And I run :prompt-cancel + Then "Invalid filename" should be logged + ## :download-retry Scenario: Retrying a failed download From df83f7aa993045be5d5c5e71c76a5775f6cd94e6 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 12:30:29 +0100 Subject: [PATCH 5/7] also add path transformations to :download --- qutebrowser/browser/commands.py | 3 +++ qutebrowser/browser/downloads.py | 21 +++++++++++++++++++++ qutebrowser/mainwindow/prompt.py | 24 ++---------------------- tests/end2end/features/downloads.feature | 10 ++++++++++ 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 2f5f617fe..a2cdbdbbe 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1334,6 +1334,9 @@ class CommandDispatcher: scope='window', window=self._win_id) target = None if dest is not None: + dest = downloads.transform_path(dest) + if dest is None: + raise cmdexc.CommandError("Invalid target filename") target = downloads.FileDownloadTarget(dest) tab = self._current_widget() diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 51ccdbfeb..f9b246d3a 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -19,11 +19,13 @@ """Shared QtWebKit/QtWebEngine code for downloads.""" +import re import sys import html import os.path import collections import functools +import pathlib import tempfile import sip @@ -161,6 +163,25 @@ def get_filename_question(*, suggested_filename, url, parent=None): return q +def transform_path(path): + r"""Do platform-specific transformations, like changing E: to E:\. + + Returns None if the path is invalid on the current platform. + """ + if sys.platform != "win32": + return path + path = utils.expand_windows_drive(path) + # Drive dependent working directories are not supported, e.g. + # E:filename is invalid + if re.match(r'[A-Z]:[^\\]', path, re.IGNORECASE): + return None + # Paths like COM1, ... + # See https://github.com/qutebrowser/qutebrowser/issues/82 + if pathlib.Path(path).is_reserved(): + return None + return path + + class NoFilenameError(Exception): """Raised when we can't find out a filename in DownloadTarget.""" diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index 2f833c3a3..b50bcb323 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -19,11 +19,9 @@ """Showing prompts above the statusbar.""" -import re import sys import os.path import html -import pathlib import collections import sip @@ -646,31 +644,13 @@ class FilenamePrompt(_BasePrompt): self._file_model.directoryLoaded.connect( lambda: self._file_model.sort(0)) - def _transform_path(self, path): - r"""Do platform-specific transformations, like changing E: to E:\. - - Returns None if the path is invalid on the current platform. - """ - if sys.platform != "win32": - return path - path = utils.expand_windows_drive(path) - # Drive dependent working directories are not supported, e.g. - # E:filename is invalid - if re.match(r'[A-Z]:[^\\]', path, re.IGNORECASE): - return None - # Paths like COM1, ... - # See https://github.com/qutebrowser/qutebrowser/issues/82 - if pathlib.Path(path).is_reserved(): - return None - return path - def _show_error(self, msg): log.prompt.error(msg) QToolTip.showText(self._lineedit.mapToGlobal(QPoint(0, 0)), msg) def accept(self, value=None): text = value if value is not None else self._lineedit.text() - text = self._transform_path(text) + text = downloads.transform_path(text) if text is None: self._show_error("Invalid filename") return False @@ -725,7 +705,7 @@ class DownloadFilenamePrompt(FilenamePrompt): def accept(self, value=None): text = value if value is not None else self._lineedit.text() - text = self._transform_path(text) + text = downloads.transform_path(text) if text is None: self._show_error("Invalid filename") return False diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index b47edf8d8..38c4cc627 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -140,6 +140,16 @@ Feature: Downloading things from a website. And I run :prompt-cancel Then "Invalid filename" should be logged + @windows + Scenario: Downloading a file to a reserved path with :download + When I run :download data/downloads/download.bin --dest=COM1 + Then the error "Invalid target filename" should be shown + + @windows + Scenario: Download a file to a drive-relative working directory with :download + When I run :download data/downloads/download.bin --dest=C:foobar + Then the error "Invalid target filename" should be shown + ## :download-retry Scenario: Retrying a failed download From a011034ff710c3f368fc23e2416d07fd60ab1e05 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 12:57:53 +0100 Subject: [PATCH 6/7] fix tests --- tests/end2end/features/downloads.feature | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 38c4cc627..1e4587740 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -128,16 +128,20 @@ Feature: Downloading things from a website. @windows Scenario: Downloading a file to a reserved path - When I open data/downloads/download.bin + When I set storage -> prompt-download-directory to true + And I open data/downloads/download.bin + And I wait for "Asking question text='Please enter a location for http://localhost:*/data/downloads/download.bin' title='Save file to:'>, *" in the log And I run :prompt-accept COM1 - And I run :prompt-cancel + And I run :leave-mode Then "Invalid filename" should be logged @windows Scenario: Downloading a file to a drive-relative working directory - When I open data/downloads/download.bin + When I set storage -> prompt-download-directory to true + And I open data/downloads/download.bin + And I wait for "Asking question text='Please enter a location for http://localhost:*/data/downloads/download.bin' title='Save file to:'>, *" in the log And I run :prompt-accept C:foobar - And I run :prompt-cancel + And I run :leave-mode Then "Invalid filename" should be logged @windows From 9dccd00ebb2483fd0bb6c6447f57c76a4b40d335 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 24 Mar 2017 14:49:30 +0100 Subject: [PATCH 7/7] fix unused import --- qutebrowser/mainwindow/prompt.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index b50bcb323..8a469cb00 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -19,7 +19,6 @@ """Showing prompts above the statusbar.""" -import sys import os.path import html import collections