From 2e6bd74fa81a5e5d6738f68cf43ae43fd7e64597 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 17 Sep 2018 17:42:04 +1000 Subject: [PATCH 01/15] Hold a share open if its timer hsa expired but a file is still uploading. Don't allow other uploads during this time --- onionshare/web.py | 176 +++++++++++++----------- onionshare_gui/receive_mode/__init__.py | 11 ++ share/locale/en.json | 1 + 3 files changed, 106 insertions(+), 82 deletions(-) diff --git a/onionshare/web.py b/onionshare/web.py index 221c2c53..9c3d52ba 100644 --- a/onionshare/web.py +++ b/onionshare/web.py @@ -118,6 +118,7 @@ class Web(object): self.download_in_progress = False self.done = False + self.can_upload = True # If the client closes the OnionShare window while a download is in progress, # it should immediately stop serving the file. The client_cancel global is @@ -343,96 +344,104 @@ class Web(object): """ Upload files. """ - # Make sure downloads_dir exists - valid = True - try: - self.common.validate_downloads_dir() - except DownloadsDirErrorCannotCreate: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) - print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) - valid = False - except DownloadsDirErrorNotWritable: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) - print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) - valid = False - if not valid: - flash('Error uploading, please inform the OnionShare user', 'error') + while self.can_upload: + # Make sure downloads_dir exists + valid = True + try: + self.common.validate_downloads_dir() + except DownloadsDirErrorCannotCreate: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) + print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) + valid = False + except DownloadsDirErrorNotWritable: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) + print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) + valid = False + if not valid: + flash('Error uploading, please inform the OnionShare user', 'error') + if self.common.settings.get('public_mode'): + return redirect('/') + else: + return redirect('/{}'.format(slug_candidate)) + + files = request.files.getlist('file[]') + filenames = [] + print('') + for f in files: + if f.filename != '': + # Automatically rename the file, if a file of the same name already exists + filename = secure_filename(f.filename) + filenames.append(filename) + local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) + if os.path.exists(local_path): + if '.' in filename: + # Add "-i", e.g. change "foo.txt" to "foo-2.txt" + parts = filename.split('.') + name = parts[:-1] + ext = parts[-1] + + i = 2 + valid = False + while not valid: + new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + else: + # If no extension, just add "-i", e.g. change "foo" to "foo-2" + i = 2 + valid = False + while not valid: + new_filename = '{}-{}'.format(filename, i) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + + basename = os.path.basename(local_path) + if f.filename != basename: + # Tell the GUI that the file has changed names + self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { + 'id': request.upload_id, + 'old_filename': f.filename, + 'new_filename': basename + }) + + self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) + print(strings._('receive_mode_received_file').format(local_path)) + f.save(local_path) + + # Note that flash strings are on English, and not translated, on purpose, + # to avoid leaking the locale of the OnionShare user + if len(filenames) == 0: + flash('No files uploaded', 'info') + else: + for filename in filenames: + flash('Sent {}'.format(filename), 'info') + + + # Register that uploads were sent (for shutdown timer) + self.done = True + if self.common.settings.get('public_mode'): return redirect('/') else: return redirect('/{}'.format(slug_candidate)) - files = request.files.getlist('file[]') - filenames = [] - print('') - for f in files: - if f.filename != '': - # Automatically rename the file, if a file of the same name already exists - filename = secure_filename(f.filename) - filenames.append(filename) - local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) - if os.path.exists(local_path): - if '.' in filename: - # Add "-i", e.g. change "foo.txt" to "foo-2.txt" - parts = filename.split('.') - name = parts[:-1] - ext = parts[-1] - - i = 2 - valid = False - while not valid: - new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - else: - # If no extension, just add "-i", e.g. change "foo" to "foo-2" - i = 2 - valid = False - while not valid: - new_filename = '{}-{}'.format(filename, i) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - - basename = os.path.basename(local_path) - if f.filename != basename: - # Tell the GUI that the file has changed names - self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { - 'id': request.upload_id, - 'old_filename': f.filename, - 'new_filename': basename - }) - - self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) - print(strings._('receive_mode_received_file').format(local_path)) - f.save(local_path) - - # Note that flash strings are on English, and not translated, on purpose, - # to avoid leaking the locale of the OnionShare user - if len(filenames) == 0: - flash('No files uploaded', 'info') - else: - for filename in filenames: - flash('Sent {}'.format(filename), 'info') - - if self.common.settings.get('public_mode'): - return redirect('/') - else: - return redirect('/{}'.format(slug_candidate)) - @self.app.route("//upload", methods=['POST']) def upload(slug_candidate): self.check_slug_candidate(slug_candidate) - return upload_logic(slug_candidate) + if self.can_upload: + return upload_logic(slug_candidate) + else: + return self.error404() @self.app.route("/upload", methods=['POST']) def upload_public(): - if not self.common.settings.get('public_mode'): + if not self.common.settings.get('public_mode') or not self.can_upload: return self.error404() return upload_logic() @@ -449,11 +458,14 @@ class Web(object): @self.app.route("//close", methods=['POST']) def close(slug_candidate): self.check_slug_candidate(slug_candidate) - return close_logic(slug_candidate) + if self.can_upload: + return close_logic(slug_candidate) + else: + return self.error404() @self.app.route("/close", methods=['POST']) def close_public(): - if not self.common.settings.get('public_mode'): + if not self.common.settings.get('public_mode') or not self.can_upload: return self.error404() return close_logic() @@ -745,7 +757,7 @@ class ReceiveModeRequest(Request): # Is this a valid upload request? self.upload_request = False - if self.method == 'POST': + if self.method == 'POST' and self.web.can_upload: if self.path == '/{}/upload'.format(self.web.slug): self.upload_request = True else: diff --git a/onionshare_gui/receive_mode/__init__.py b/onionshare_gui/receive_mode/__init__.py index 623d3986..a9f15749 100644 --- a/onionshare_gui/receive_mode/__init__.py +++ b/onionshare_gui/receive_mode/__init__.py @@ -98,6 +98,17 @@ class ReceiveMode(Mode): The shutdown timer expired, should we stop the server? Returns a bool """ # TODO: wait until the final upload is done before stoppign the server? + # If there were no attempts to upload files, or all uploads are done, we can stop + if self.web.upload_count == 0 or self.web.done: + self.server_status.stop_server() + self.server_status_label.setText(strings._('close_on_timeout', True)) + return True + # An upload is probably still running - hold off on stopping the share, but block new shares. + else: + self.server_status_label.setText(strings._('timeout_upload_still_running', True)) + self.web.can_upload = False + return False + return True def start_server_custom(self): diff --git a/share/locale/en.json b/share/locale/en.json index cc39bba7..beebac77 100644 --- a/share/locale/en.json +++ b/share/locale/en.json @@ -15,6 +15,7 @@ "close_on_timeout": "Stopped because timer expired", "closing_automatically": "Stopped because download finished", "timeout_download_still_running": "Waiting for download to complete", + "timeout_upload_still_running": "Waiting for upload to complete", "large_filesize": "Warning: Sending large files could take hours", "systray_menu_exit": "Quit", "systray_download_started_title": "OnionShare Download Started", From 551e7e97ca3efabe20aa5f0ce846886e193df22c Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 18 Sep 2018 08:35:58 +1000 Subject: [PATCH 02/15] Remove unnecessary loop. Remove the Close route/setting which can DoS another running upload. Fix detecting whether any uploads are still in progress before terminating the service after timer expires. Don't register 404s for uploads after expiry has finished (throw a 403 instead)" --- onionshare/settings.py | 3 +- onionshare/web.py | 210 +++++++++++------------- onionshare_gui/receive_mode/__init__.py | 2 +- onionshare_gui/settings_dialog.py | 13 -- share/locale/en.json | 1 - share/templates/403.html | 16 ++ share/templates/receive.html | 9 - test/test_onionshare_settings.py | 3 +- test/test_onionshare_web.py | 30 ---- 9 files changed, 115 insertions(+), 172 deletions(-) create mode 100644 share/templates/403.html diff --git a/onionshare/settings.py b/onionshare/settings.py index ef75be2f..6cc70954 100644 --- a/onionshare/settings.py +++ b/onionshare/settings.py @@ -72,8 +72,7 @@ class Settings(object): 'public_mode': False, 'slug': '', 'hidservauth_string': '', - 'downloads_dir': self.build_default_downloads_dir(), - 'receive_allow_receiver_shutdown': True + 'downloads_dir': self.build_default_downloads_dir() } self._settings = {} self.fill_in_defaults() diff --git a/onionshare/web.py b/onionshare/web.py index 9c3d52ba..c197caf1 100644 --- a/onionshare/web.py +++ b/onionshare/web.py @@ -119,6 +119,7 @@ class Web(object): self.done = False self.can_upload = True + self.uploads_in_progress = [] # If the client closes the OnionShare window while a download is in progress, # it should immediately stop serving the file. The client_cancel global is @@ -323,9 +324,7 @@ class Web(object): r = make_response(render_template( 'receive.html', - upload_action=upload_action, - close_action=close_action, - receive_allow_receiver_shutdown=self.common.settings.get('receive_allow_receiver_shutdown'))) + upload_action=upload_action)) return self.add_security_headers(r) @self.app.route("/") @@ -344,130 +343,102 @@ class Web(object): """ Upload files. """ - while self.can_upload: - # Make sure downloads_dir exists - valid = True - try: - self.common.validate_downloads_dir() - except DownloadsDirErrorCannotCreate: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) - print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) - valid = False - except DownloadsDirErrorNotWritable: - self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) - print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) - valid = False - if not valid: - flash('Error uploading, please inform the OnionShare user', 'error') - if self.common.settings.get('public_mode'): - return redirect('/') - else: - return redirect('/{}'.format(slug_candidate)) - - files = request.files.getlist('file[]') - filenames = [] - print('') - for f in files: - if f.filename != '': - # Automatically rename the file, if a file of the same name already exists - filename = secure_filename(f.filename) - filenames.append(filename) - local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) - if os.path.exists(local_path): - if '.' in filename: - # Add "-i", e.g. change "foo.txt" to "foo-2.txt" - parts = filename.split('.') - name = parts[:-1] - ext = parts[-1] - - i = 2 - valid = False - while not valid: - new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - else: - # If no extension, just add "-i", e.g. change "foo" to "foo-2" - i = 2 - valid = False - while not valid: - new_filename = '{}-{}'.format(filename, i) - local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) - if os.path.exists(local_path): - i += 1 - else: - valid = True - - basename = os.path.basename(local_path) - if f.filename != basename: - # Tell the GUI that the file has changed names - self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { - 'id': request.upload_id, - 'old_filename': f.filename, - 'new_filename': basename - }) - - self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) - print(strings._('receive_mode_received_file').format(local_path)) - f.save(local_path) - - # Note that flash strings are on English, and not translated, on purpose, - # to avoid leaking the locale of the OnionShare user - if len(filenames) == 0: - flash('No files uploaded', 'info') - else: - for filename in filenames: - flash('Sent {}'.format(filename), 'info') - - - # Register that uploads were sent (for shutdown timer) - self.done = True - + # Make sure downloads_dir exists + valid = True + try: + self.common.validate_downloads_dir() + except DownloadsDirErrorCannotCreate: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path) + print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir'))) + valid = False + except DownloadsDirErrorNotWritable: + self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path) + print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir'))) + valid = False + if not valid: + flash('Error uploading, please inform the OnionShare user', 'error') if self.common.settings.get('public_mode'): return redirect('/') else: return redirect('/{}'.format(slug_candidate)) - @self.app.route("//upload", methods=['POST']) - def upload(slug_candidate): - self.check_slug_candidate(slug_candidate) - if self.can_upload: - return upload_logic(slug_candidate) + files = request.files.getlist('file[]') + filenames = [] + print('') + for f in files: + if f.filename != '': + # Automatically rename the file, if a file of the same name already exists + filename = secure_filename(f.filename) + filenames.append(filename) + local_path = os.path.join(self.common.settings.get('downloads_dir'), filename) + if os.path.exists(local_path): + if '.' in filename: + # Add "-i", e.g. change "foo.txt" to "foo-2.txt" + parts = filename.split('.') + name = parts[:-1] + ext = parts[-1] + + i = 2 + valid = False + while not valid: + new_filename = '{}-{}.{}'.format('.'.join(name), i, ext) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + else: + # If no extension, just add "-i", e.g. change "foo" to "foo-2" + i = 2 + valid = False + while not valid: + new_filename = '{}-{}'.format(filename, i) + local_path = os.path.join(self.common.settings.get('downloads_dir'), new_filename) + if os.path.exists(local_path): + i += 1 + else: + valid = True + + basename = os.path.basename(local_path) + if f.filename != basename: + # Tell the GUI that the file has changed names + self.add_request(Web.REQUEST_UPLOAD_FILE_RENAMED, request.path, { + 'id': request.upload_id, + 'old_filename': f.filename, + 'new_filename': basename + }) + self.common.log('Web', 'receive_routes', '/upload, uploaded {}, saving to {}'.format(f.filename, local_path)) + print(strings._('receive_mode_received_file').format(local_path)) + f.save(local_path) + + # Note that flash strings are on English, and not translated, on purpose, + # to avoid leaking the locale of the OnionShare user + if len(filenames) == 0: + flash('No files uploaded', 'info') else: - return self.error404() + for filename in filenames: + flash('Sent {}'.format(filename), 'info') - @self.app.route("/upload", methods=['POST']) - def upload_public(): - if not self.common.settings.get('public_mode') or not self.can_upload: - return self.error404() - return upload_logic() - - - def close_logic(slug_candidate=''): - if self.common.settings.get('receive_allow_receiver_shutdown'): - self.force_shutdown() - r = make_response(render_template('closed.html')) - self.add_request(Web.REQUEST_CLOSE_SERVER, request.path) - return self.add_security_headers(r) + if self.common.settings.get('public_mode'): + return redirect('/') else: return redirect('/{}'.format(slug_candidate)) - @self.app.route("//close", methods=['POST']) - def close(slug_candidate): + @self.app.route("//upload", methods=['POST']) + def upload(slug_candidate): + if not self.can_upload: + return self.error403() self.check_slug_candidate(slug_candidate) - if self.can_upload: - return close_logic(slug_candidate) - else: - return self.error404() + return upload_logic(slug_candidate) - @self.app.route("/close", methods=['POST']) - def close_public(): - if not self.common.settings.get('public_mode') or not self.can_upload: + @self.app.route("/upload", methods=['POST']) + def upload_public(): + if not self.common.settings.get('public_mode'): return self.error404() - return close_logic() + if not self.can_upload: + return self.error403() + return upload_logic() + def common_routes(self): """ @@ -504,6 +475,12 @@ class Web(object): r = make_response(render_template('404.html'), 404) return self.add_security_headers(r) + def error403(self): + self.add_request(Web.REQUEST_OTHER, request.path) + + r = make_response(render_template('403.html'), 403) + return self.add_security_headers(r) + def add_security_headers(self, r): """ Add security headers to a request @@ -783,6 +760,8 @@ class ReceiveModeRequest(Request): datetime.now().strftime("%b %d, %I:%M%p"), strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) )) + # append to self.uploads_in_progress + self.web.uploads_in_progress.append(self.upload_id) # Tell the GUI self.web.add_request(Web.REQUEST_STARTED, self.path, { @@ -816,6 +795,9 @@ class ReceiveModeRequest(Request): 'id': self.upload_id }) + # remove from self.uploads_in_progress + self.web.uploads_in_progress.remove(self.upload_id) + def file_write_func(self, filename, length): """ This function gets called when a specific file is written to. diff --git a/onionshare_gui/receive_mode/__init__.py b/onionshare_gui/receive_mode/__init__.py index a9f15749..8c360d19 100644 --- a/onionshare_gui/receive_mode/__init__.py +++ b/onionshare_gui/receive_mode/__init__.py @@ -99,7 +99,7 @@ class ReceiveMode(Mode): """ # TODO: wait until the final upload is done before stoppign the server? # If there were no attempts to upload files, or all uploads are done, we can stop - if self.web.upload_count == 0 or self.web.done: + if self.web.upload_count == 0 or not self.web.uploads_in_progress: self.server_status.stop_server() self.server_status_label.setText(strings._('close_on_timeout', True)) return True diff --git a/onionshare_gui/settings_dialog.py b/onionshare_gui/settings_dialog.py index c4e6b752..c23f8ad8 100644 --- a/onionshare_gui/settings_dialog.py +++ b/onionshare_gui/settings_dialog.py @@ -101,15 +101,9 @@ class SettingsDialog(QtWidgets.QDialog): downloads_layout.addWidget(self.downloads_dir_lineedit) downloads_layout.addWidget(downloads_button) - # Allow the receiver to shutdown the server - self.receive_allow_receiver_shutdown_checkbox = QtWidgets.QCheckBox() - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Checked) - self.receive_allow_receiver_shutdown_checkbox.setText(strings._("gui_settings_receive_allow_receiver_shutdown_checkbox", True)) - # Receiving options layout receiving_group_layout = QtWidgets.QVBoxLayout() receiving_group_layout.addLayout(downloads_layout) - receiving_group_layout.addWidget(self.receive_allow_receiver_shutdown_checkbox) receiving_group = QtWidgets.QGroupBox(strings._("gui_settings_receiving_label", True)) receiving_group.setLayout(receiving_group_layout) @@ -421,12 +415,6 @@ class SettingsDialog(QtWidgets.QDialog): downloads_dir = self.old_settings.get('downloads_dir') self.downloads_dir_lineedit.setText(downloads_dir) - receive_allow_receiver_shutdown = self.old_settings.get('receive_allow_receiver_shutdown') - if receive_allow_receiver_shutdown: - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Checked) - else: - self.receive_allow_receiver_shutdown_checkbox.setCheckState(QtCore.Qt.Unchecked) - public_mode = self.old_settings.get('public_mode') if public_mode: self.public_mode_checkbox.setCheckState(QtCore.Qt.Checked) @@ -802,7 +790,6 @@ class SettingsDialog(QtWidgets.QDialog): # Also unset the HidServAuth if we are removing our reusable private key settings.set('hidservauth_string', '') settings.set('downloads_dir', self.downloads_dir_lineedit.text()) - settings.set('receive_allow_receiver_shutdown', self.receive_allow_receiver_shutdown_checkbox.isChecked()) settings.set('public_mode', self.public_mode_checkbox.isChecked()) settings.set('use_stealth', self.stealth_checkbox.isChecked()) # Always unset the HidServAuth if Stealth mode is unset diff --git a/share/locale/en.json b/share/locale/en.json index beebac77..98addae9 100644 --- a/share/locale/en.json +++ b/share/locale/en.json @@ -168,7 +168,6 @@ "gui_settings_receiving_label": "Receiving options", "gui_settings_downloads_label": "Save files to", "gui_settings_downloads_button": "Browse", - "gui_settings_receive_allow_receiver_shutdown_checkbox": "Receive mode can be stopped by the sender", "gui_settings_public_mode_checkbox": "OnionShare is open to the public\n(don't prevent people from guessing the OnionShare address)", "systray_close_server_title": "OnionShare Server Closed", "systray_close_server_message": "A user closed the server", diff --git a/share/templates/403.html b/share/templates/403.html new file mode 100644 index 00000000..df81f3e7 --- /dev/null +++ b/share/templates/403.html @@ -0,0 +1,16 @@ + + + + OnionShare: 403 Forbidden + + + + +
+
+

+

You are not allowed to perform that action at this time.

+
+
+ + diff --git a/share/templates/receive.html b/share/templates/receive.html index d8b02f73..e85b6ff9 100644 --- a/share/templates/receive.html +++ b/share/templates/receive.html @@ -34,14 +34,5 @@ - {% if receive_allow_receiver_shutdown %} - {% with messages = get_flashed_messages() %} - {% if messages %} -
- -
- {% endif %} - {% endwith %} - {% endif %} diff --git a/test/test_onionshare_settings.py b/test/test_onionshare_settings.py index 1521492d..fb9e1f69 100644 --- a/test/test_onionshare_settings.py +++ b/test/test_onionshare_settings.py @@ -63,8 +63,7 @@ class TestSettings: 'private_key': '', 'slug': '', 'hidservauth_string': '', - 'downloads_dir': os.path.expanduser('~/OnionShare'), - 'receive_allow_receiver_shutdown': True, + 'downloads_dir': os.path.expanduser('~/OnionShare') 'public_mode': False } diff --git a/test/test_onionshare_web.py b/test/test_onionshare_web.py index 2209a0fd..d523ea96 100644 --- a/test/test_onionshare_web.py +++ b/test/test_onionshare_web.py @@ -138,36 +138,6 @@ class TestWeb: res.get_data() assert res.status_code == 200 - def test_receive_mode_allow_receiver_shutdown_on(self, common_obj): - web = web_obj(common_obj, True) - - common_obj.settings.set('receive_allow_receiver_shutdown', True) - - assert web.running == True - - with web.app.test_client() as c: - # Load close page - res = c.post('/{}/close'.format(web.slug)) - res.get_data() - # Should return ok, and server should stop - assert res.status_code == 200 - assert web.running == False - - def test_receive_mode_allow_receiver_shutdown_off(self, common_obj): - web = web_obj(common_obj, True) - - common_obj.settings.set('receive_allow_receiver_shutdown', False) - - assert web.running == True - - with web.app.test_client() as c: - # Load close page - res = c.post('/{}/close'.format(web.slug)) - res.get_data() - # Should redirect to index, and server should still be running - assert res.status_code == 302 - assert web.running == True - def test_public_mode_on(self, common_obj): web = web_obj(common_obj, True) common_obj.settings.set('public_mode', True) From 2c14575c1034206c9ecf4e09fde2aca13e59f66a Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 18 Sep 2018 09:59:06 +1000 Subject: [PATCH 03/15] Fix test --- test/test_onionshare_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_onionshare_settings.py b/test/test_onionshare_settings.py index fb9e1f69..937288d1 100644 --- a/test/test_onionshare_settings.py +++ b/test/test_onionshare_settings.py @@ -63,7 +63,7 @@ class TestSettings: 'private_key': '', 'slug': '', 'hidservauth_string': '', - 'downloads_dir': os.path.expanduser('~/OnionShare') + 'downloads_dir': os.path.expanduser('~/OnionShare'), 'public_mode': False } From dddf5a9a092e01530cc37ddc313b569d121f1a9c Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Thu, 20 Sep 2018 11:33:37 +1000 Subject: [PATCH 04/15] Throw a 403 on the index pages if the timer has run out but an upload is in progress --- onionshare/web.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/onionshare/web.py b/onionshare/web.py index 1c285ea7..f52d9dd6 100644 --- a/onionshare/web.py +++ b/onionshare/web.py @@ -340,10 +340,14 @@ class Web(object): @self.app.route("/") def index(slug_candidate): self.check_slug_candidate(slug_candidate) + if not self.can_upload: + return self.error403() return index_logic() @self.app.route("/") def index_public(): + if not self.can_upload: + return self.error403() if not self.common.settings.get('public_mode'): return self.error404() return index_logic() From d267cc597dfc7c111c1c8e18f5957548eb4eb351 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 1 Oct 2018 18:42:53 +1000 Subject: [PATCH 05/15] Fix logic for handling an upload still in progress when timer runs out. Show thankyou page for last uploader post-timer expiry --- onionshare/web/receive_mode.py | 74 ++++++++++++------- onionshare_gui/receive_mode/__init__.py | 8 +- .../templates/{closed.html => thankyou.html} | 0 3 files changed, 53 insertions(+), 29 deletions(-) rename share/templates/{closed.html => thankyou.html} (100%) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 73762573..5035f68a 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -19,6 +19,7 @@ class ReceiveModeWeb(object): self.web = web self.can_upload = True + self.can_stop_share_now = False self.upload_count = 0 self.uploads_in_progress = [] @@ -39,6 +40,7 @@ class ReceiveModeWeb(object): r = make_response(render_template( 'receive.html', upload_action=upload_action)) + return self.web.add_security_headers(r) @self.web.app.route("/") @@ -138,10 +140,24 @@ class ReceiveModeWeb(object): for filename in filenames: flash('Sent {}'.format(filename), 'info') - if self.common.settings.get('public_mode'): - return redirect('/') + if self.can_upload: + if self.common.settings.get('public_mode'): + path = '/' + else: + path = '/{}'.format(slug_candidate) + + return redirect('{}'.format(path)) else: - return redirect('/{}'.format(slug_candidate)) + # It was the last upload and the timer ran out + if self.common.settings.get('public_mode'): + return thankyou_logic(slug_candidate) + else: + return thankyou_logic() + + def thankyou_logic(slug_candidate=''): + r = make_response(render_template( + 'thankyou.html')) + return self.web.add_security_headers(r) @self.web.app.route("//upload", methods=['POST']) def upload(slug_candidate): @@ -231,39 +247,36 @@ class ReceiveModeRequest(Request): if self.path == '/upload': self.upload_request = True - if self.upload_request: + if self.upload_request and self.web.receive_mode.can_upload: # A dictionary that maps filenames to the bytes uploaded so far self.progress = {} # Create an upload_id, attach it to the request self.upload_id = self.web.receive_mode.upload_count - if self.web.receive_mode.can_upload: - self.web.receive_mode.upload_count += 1 + self.web.receive_mode.upload_count += 1 - # Figure out the content length - try: - self.content_length = int(self.headers['Content-Length']) - except: - self.content_length = 0 + # Figure out the content length + try: + self.content_length = int(self.headers['Content-Length']) + except: + self.content_length = 0 - print("{}: {}".format( - datetime.now().strftime("%b %d, %I:%M%p"), - strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) - )) + print("{}: {}".format( + datetime.now().strftime("%b %d, %I:%M%p"), + strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) + )) - # append to self.uploads_in_progress - self.web.receive_mode.uploads_in_progress.append(self.upload_id) + # append to self.uploads_in_progress + self.web.receive_mode.uploads_in_progress.append(self.upload_id) - # Tell the GUI - self.web.add_request(self.web.REQUEST_STARTED, self.path, { - 'id': self.upload_id, - 'content_length': self.content_length - }) + # Tell the GUI + self.web.add_request(self.web.REQUEST_STARTED, self.path, { + 'id': self.upload_id, + 'content_length': self.content_length + }) - self.previous_file = None - else: - self.upload_rejected = True + self.previous_file = None def _get_file_stream(self, total_content_length, content_type, filename=None, content_length=None): """ @@ -284,14 +297,19 @@ class ReceiveModeRequest(Request): """ super(ReceiveModeRequest, self).close() if self.upload_request: - if not self.upload_rejected: + try: + upload_id = self.upload_id # Inform the GUI that the upload has finished self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { - 'id': self.upload_id + 'id': upload_id }) # remove from self.uploads_in_progress - self.web.receive_mode.uploads_in_progress.remove(self.upload_id) + self.web.receive_mode.uploads_in_progress.remove(upload_id) + + except AttributeError: + # We may not have got an upload_id (e.g uploads were rejected) + pass def file_write_func(self, filename, length): """ diff --git a/onionshare_gui/receive_mode/__init__.py b/onionshare_gui/receive_mode/__init__.py index faa65ffe..c10622e4 100644 --- a/onionshare_gui/receive_mode/__init__.py +++ b/onionshare_gui/receive_mode/__init__.py @@ -51,6 +51,7 @@ class ReceiveMode(Mode): self.uploads_in_progress = 0 self.uploads_completed = 0 self.new_upload = False # For scrolling to the bottom of the uploads list + self.can_stop_server = False # for communicating to the auto-stop timer # Information about share, and show uploads button self.info_in_progress_uploads_count = QtWidgets.QLabel() @@ -93,7 +94,7 @@ class ReceiveMode(Mode): The shutdown timer expired, should we stop the server? Returns a bool """ # If there were no attempts to upload files, or all uploads are done, we can stop - if self.web.receive_mode.upload_count == 0 or not self.web.receive_mode.uploads_in_progress: + if self.web.receive_mode.upload_count == 0 or self.can_stop_server: self.server_status.stop_server() self.server_status_label.setText(strings._('close_on_timeout', True)) return True @@ -110,7 +111,9 @@ class ReceiveMode(Mode): Starting the server. """ # Reset web counters + self.can_stop_server = False self.web.receive_mode.upload_count = 0 + self.web.receive_mode.can_upload = True self.web.error404_count = 0 # Hide and reset the uploads if we have previously shared @@ -144,6 +147,7 @@ class ReceiveMode(Mode): self.uploads.add(event["data"]["id"], event["data"]["content_length"]) self.uploads_in_progress += 1 self.update_uploads_in_progress() + self.can_stop_server = False self.system_tray.showMessage(strings._('systray_upload_started_title', True), strings._('systray_upload_started_message', True)) @@ -177,6 +181,8 @@ class ReceiveMode(Mode): # Update the 'in progress uploads' info self.uploads_in_progress -= 1 self.update_uploads_in_progress() + if self.uploads_in_progress == 0: + self.can_stop_server = True def on_reload_settings(self): """ diff --git a/share/templates/closed.html b/share/templates/thankyou.html similarity index 100% rename from share/templates/closed.html rename to share/templates/thankyou.html From eeedd3279382d4a2c7a881d18994b89dab61e37f Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 1 Oct 2018 19:15:58 +1000 Subject: [PATCH 06/15] remove unused variable, whitespace --- onionshare/web/receive_mode.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 5035f68a..7fa274d5 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -19,7 +19,6 @@ class ReceiveModeWeb(object): self.web = web self.can_upload = True - self.can_stop_share_now = False self.upload_count = 0 self.uploads_in_progress = [] @@ -40,7 +39,6 @@ class ReceiveModeWeb(object): r = make_response(render_template( 'receive.html', upload_action=upload_action)) - return self.web.add_security_headers(r) @self.web.app.route("/") From c8fc6d7f8546c7b7041d1acbd61c2e8f7233deca Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 1 Oct 2018 19:17:50 +1000 Subject: [PATCH 07/15] Another unused variable --- onionshare/web/receive_mode.py | 1 - 1 file changed, 1 deletion(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 7fa274d5..8602e98a 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -236,7 +236,6 @@ class ReceiveModeRequest(Request): # Is this a valid upload request? self.upload_request = False - self.upload_rejected = False if self.method == 'POST': if self.path == '/{}/upload'.format(self.web.slug): self.upload_request = True From 7fc4f97b2ae6d8f83c0297795608406e75220c84 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 1 Oct 2018 19:18:50 +1000 Subject: [PATCH 08/15] remove uploads_in_progress list from web side --- onionshare/web/receive_mode.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 8602e98a..bc5e1734 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -20,7 +20,6 @@ class ReceiveModeWeb(object): self.can_upload = True self.upload_count = 0 - self.uploads_in_progress = [] self.define_routes() @@ -264,9 +263,6 @@ class ReceiveModeRequest(Request): strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) )) - # append to self.uploads_in_progress - self.web.receive_mode.uploads_in_progress.append(self.upload_id) - # Tell the GUI self.web.add_request(self.web.REQUEST_STARTED, self.path, { 'id': self.upload_id, @@ -301,9 +297,6 @@ class ReceiveModeRequest(Request): 'id': upload_id }) - # remove from self.uploads_in_progress - self.web.receive_mode.uploads_in_progress.remove(upload_id) - except AttributeError: # We may not have got an upload_id (e.g uploads were rejected) pass From 80a70a6fc68791b69ca1a8dfdbaa7d05f2ff8099 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 2 Oct 2018 07:33:13 +1000 Subject: [PATCH 09/15] remove unused variable --- onionshare/web/web.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/onionshare/web/web.py b/onionshare/web/web.py index 2885e87f..45d021c0 100644 --- a/onionshare/web/web.py +++ b/onionshare/web/web.py @@ -66,8 +66,6 @@ class Web(object): # Use a custom Request class to track upload progess self.app.request_class = ReceiveModeRequest - self.can_upload = True - # Starting in Flask 0.11, render_template_string autoescapes template variables # by default. To prevent content injection through template variables in # earlier versions of Flask, we force autoescaping in the Jinja2 template From 509d3134646d78c307bb6a4b2158b4494e75394c Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 2 Oct 2018 08:22:08 +1000 Subject: [PATCH 10/15] Try to fix logic handling last upload after timer expiry --- onionshare/web/receive_mode.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index bc5e1734..60909a23 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -243,7 +243,11 @@ class ReceiveModeRequest(Request): if self.path == '/upload': self.upload_request = True - if self.upload_request and self.web.receive_mode.can_upload: + # Prevent new uploads if we've said so (timer expired) + if not self.web.receive_mode.can_upload: + self.upload_request = False + + if self.upload_request: # A dictionary that maps filenames to the bytes uploaded so far self.progress = {} @@ -290,16 +294,11 @@ class ReceiveModeRequest(Request): """ super(ReceiveModeRequest, self).close() if self.upload_request: - try: - upload_id = self.upload_id - # Inform the GUI that the upload has finished - self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { - 'id': upload_id - }) + # Inform the GUI that the upload has finished + self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { + 'id': self.upload_id + }) - except AttributeError: - # We may not have got an upload_id (e.g uploads were rejected) - pass def file_write_func(self, filename, length): """ From b27007b20d6a3c768ba1733a3c24e92d3966c792 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 2 Oct 2018 15:41:29 +1000 Subject: [PATCH 11/15] Make auto-stop timer work on CLI when an upload is still in progress on expiry --- onionshare/__init__.py | 7 +++++++ onionshare/web/receive_mode.py | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/onionshare/__init__.py b/onionshare/__init__.py index 715c5571..42294ec1 100644 --- a/onionshare/__init__.py +++ b/onionshare/__init__.py @@ -198,6 +198,13 @@ def main(cwd=None): print(strings._("close_on_timeout")) web.stop(app.port) break + if mode == 'receive': + if web.receive_mode.upload_count == 0 or not web.receive_mode.uploads_in_progress: + print(strings._("close_on_timeout")) + web.stop(app.port) + break + else: + web.receive_mode.can_upload = False # Allow KeyboardInterrupt exception to be handled with threads # https://stackoverflow.com/questions/3788208/python-threading-ignores-keyboardinterrupt-exception time.sleep(0.2) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 60909a23..4ea95201 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -20,6 +20,7 @@ class ReceiveModeWeb(object): self.can_upload = True self.upload_count = 0 + self.uploads_in_progress = [] self.define_routes() @@ -273,6 +274,8 @@ class ReceiveModeRequest(Request): 'content_length': self.content_length }) + self.web.receive_mode.uploads_in_progress.append(self.upload_id) + self.previous_file = None def _get_file_stream(self, total_content_length, content_type, filename=None, content_length=None): @@ -298,6 +301,7 @@ class ReceiveModeRequest(Request): self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { 'id': self.upload_id }) + self.web.receive_mode.uploads_in_progress.remove(self.upload_id) def file_write_func(self, filename, length): From 005d2d0af7368da8c379a689d48d27b6712cd8b0 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 13 Nov 2018 14:42:26 +1100 Subject: [PATCH 12/15] Try and fix closing the request for a valid upload post-timer expiry, whilst still rejecting subsequent uploads --- onionshare/web/receive_mode.py | 56 ++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 4ea95201..6ac96f8e 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -244,39 +244,38 @@ class ReceiveModeRequest(Request): if self.path == '/upload': self.upload_request = True - # Prevent new uploads if we've said so (timer expired) - if not self.web.receive_mode.can_upload: - self.upload_request = False - if self.upload_request: # A dictionary that maps filenames to the bytes uploaded so far self.progress = {} - # Create an upload_id, attach it to the request - self.upload_id = self.web.receive_mode.upload_count + # Prevent new uploads if we've said so (timer expired) + if self.web.receive_mode.can_upload: - self.web.receive_mode.upload_count += 1 + # Create an upload_id, attach it to the request + self.upload_id = self.web.receive_mode.upload_count - # Figure out the content length - try: - self.content_length = int(self.headers['Content-Length']) - except: - self.content_length = 0 + self.web.receive_mode.upload_count += 1 - print("{}: {}".format( - datetime.now().strftime("%b %d, %I:%M%p"), - strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) - )) + # Figure out the content length + try: + self.content_length = int(self.headers['Content-Length']) + except: + self.content_length = 0 - # Tell the GUI - self.web.add_request(self.web.REQUEST_STARTED, self.path, { - 'id': self.upload_id, - 'content_length': self.content_length - }) + print("{}: {}".format( + datetime.now().strftime("%b %d, %I:%M%p"), + strings._("receive_mode_upload_starting").format(self.web.common.human_readable_filesize(self.content_length)) + )) - self.web.receive_mode.uploads_in_progress.append(self.upload_id) + # Tell the GUI + self.web.add_request(self.web.REQUEST_STARTED, self.path, { + 'id': self.upload_id, + 'content_length': self.content_length + }) - self.previous_file = None + self.web.receive_mode.uploads_in_progress.append(self.upload_id) + + self.previous_file = None def _get_file_stream(self, total_content_length, content_type, filename=None, content_length=None): """ @@ -296,13 +295,16 @@ class ReceiveModeRequest(Request): Closing the request. """ super(ReceiveModeRequest, self).close() - if self.upload_request: + try: + upload_id = self.upload_id + self.web.common.log('ReceiveModeWeb', 'We finished our upload') # Inform the GUI that the upload has finished self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { - 'id': self.upload_id + 'id': upload_id }) - self.web.receive_mode.uploads_in_progress.remove(self.upload_id) - + self.web.receive_mode.uploads_in_progress.remove(upload_id) + except AttributeError: + pass def file_write_func(self, filename, length): """ From 3a879fb22fa66b5fb068a44d5275cfd9ece31f23 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 13 Nov 2018 14:59:29 +1100 Subject: [PATCH 13/15] remove obsolete settings in test that related to allowing receiver to shutdown service --- tests/local_onionshare_settings_dialog_test.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/local_onionshare_settings_dialog_test.py b/tests/local_onionshare_settings_dialog_test.py index 6d8923b6..c1e48122 100644 --- a/tests/local_onionshare_settings_dialog_test.py +++ b/tests/local_onionshare_settings_dialog_test.py @@ -85,11 +85,6 @@ class SettingsGuiTest(unittest.TestCase, SettingsGuiBaseTest): # receive mode self.gui.downloads_dir_lineedit.setText('/tmp/OnionShareSettingsTest') - # allow receiver shutdown is on - self.assertTrue(self.gui.receive_allow_receiver_shutdown_checkbox.isChecked()) - # disable receiver shutdown - QtTest.QTest.mouseClick(self.gui.receive_allow_receiver_shutdown_checkbox, QtCore.Qt.LeftButton, pos=QtCore.QPoint(2,self.gui.receive_allow_receiver_shutdown_checkbox.height()/2)) - self.assertFalse(self.gui.receive_allow_receiver_shutdown_checkbox.isChecked()) # bundled mode is enabled @@ -168,7 +163,6 @@ class SettingsGuiTest(unittest.TestCase, SettingsGuiBaseTest): self.assertTrue(data["save_private_key"]) self.assertTrue(data["use_stealth"]) self.assertEqual(data["downloads_dir"], "/tmp/OnionShareSettingsTest") - self.assertFalse(data["receive_allow_receiver_shutdown"]) self.assertFalse(data["close_after_first_download"]) self.assertEqual(data["connection_type"], "bundled") self.assertFalse(data["tor_bridges_use_obfs4"]) From 7151874dad68b8d13bc56d6c7e7664121d3164de Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Tue, 13 Nov 2018 15:06:28 +1100 Subject: [PATCH 14/15] remove debug log --- onionshare/web/receive_mode.py | 1 - 1 file changed, 1 deletion(-) diff --git a/onionshare/web/receive_mode.py b/onionshare/web/receive_mode.py index 274ee138..6985f38a 100644 --- a/onionshare/web/receive_mode.py +++ b/onionshare/web/receive_mode.py @@ -305,7 +305,6 @@ class ReceiveModeRequest(Request): super(ReceiveModeRequest, self).close() try: upload_id = self.upload_id - self.web.common.log('ReceiveModeWeb', 'We finished our upload') # Inform the GUI that the upload has finished self.web.add_request(self.web.REQUEST_UPLOAD_FINISHED, self.path, { 'id': upload_id From 195df0499d5f570fba258b605a7051c3787e51d0 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Wed, 5 Dec 2018 18:19:35 +1100 Subject: [PATCH 15/15] Keep the upload running in the GUI if the timer has run out --- onionshare_gui/mode/receive_mode/__init__.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/onionshare_gui/mode/receive_mode/__init__.py b/onionshare_gui/mode/receive_mode/__init__.py index d6c0c351..c53f1ea1 100644 --- a/onionshare_gui/mode/receive_mode/__init__.py +++ b/onionshare_gui/mode/receive_mode/__init__.py @@ -96,8 +96,16 @@ class ReceiveMode(Mode): """ The shutdown timer expired, should we stop the server? Returns a bool """ - # TODO: wait until the final upload is done before stoppign the server? - return True + # If there were no attempts to upload files, or all uploads are done, we can stop + if self.web.receive_mode.upload_count == 0 or not self.web.receive_mode.uploads_in_progress: + self.server_status.stop_server() + self.server_status_label.setText(strings._('close_on_timeout')) + return True + # An upload is probably still running - hold off on stopping the share, but block new shares. + else: + self.server_status_label.setText(strings._('timeout_upload_still_running')) + self.web.receive_mode.can_upload = False + return False def start_server_custom(self): """