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

@@ -72,8 +72,7 @@ class Settings(object):
             'public_mode': False,
             'slug': '',
             'hidservauth_string': '',
-            'downloads_dir': self.build_default_downloads_dir(),
+            'downloads_dir': self.build_default_downloads_dir()
-            'receive_allow_receiver_shutdown': True
         }
         self._settings = {}
         self.fill_in_defaults()

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,
+                upload_action=upload_action))
-                close_action=close_action,
-                receive_allow_receiver_shutdown=self.common.settings.get('receive_allow_receiver_shutdown')))
             return self.add_security_headers(r)
 
         @self.app.route("/<slug_candidate>")
@@ -344,130 +343,102 @@ class Web(object):
             """
             Upload files.
             """
-            while self.can_upload:
+            # Make sure downloads_dir exists
-                # Make sure downloads_dir exists
+            valid = True
-                valid = True
+            try:
-                try:
+                self.common.validate_downloads_dir()
-                    self.common.validate_downloads_dir()
+            except DownloadsDirErrorCannotCreate:
-                except DownloadsDirErrorCannotCreate:
+                self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_CANNOT_CREATE, request.path)
-                    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')))
-                    print(strings._('error_cannot_create_downloads_dir').format(self.common.settings.get('downloads_dir')))
+                valid = False
-                    valid = False
+            except DownloadsDirErrorNotWritable:
-                except DownloadsDirErrorNotWritable:
+                self.add_request(Web.REQUEST_ERROR_DOWNLOADS_DIR_NOT_WRITABLE, request.path)
-                    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')))
-                    print(strings._('error_downloads_dir_not_writable').format(self.common.settings.get('downloads_dir')))
+                valid = False
-                    valid = False
+            if not valid:
-                if not valid:
+                flash('Error uploading, please inform the OnionShare user', 'error')
-                    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))
 
-        @self.app.route("/<slug_candidate>/upload", methods=['POST'])
+            files = request.files.getlist('file[]')
-        def upload(slug_candidate):
+            filenames = []
-            self.check_slug_candidate(slug_candidate)
+            print('')
-            if self.can_upload:
+            for f in files:
-                return upload_logic(slug_candidate)
+                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'])
+            if self.common.settings.get('public_mode'):
-        def upload_public():
+                return redirect('/')
-            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)
             else:
                 return redirect('/{}'.format(slug_candidate))
 
-        @self.app.route("/<slug_candidate>/close", methods=['POST'])
+        @self.app.route("/<slug_candidate>/upload", methods=['POST'])
-        def close(slug_candidate):
+        def upload(slug_candidate):
+            if not self.can_upload:
+                return self.error403()
             self.check_slug_candidate(slug_candidate)
-            if self.can_upload:
+            return upload_logic(slug_candidate)
-                return close_logic(slug_candidate)
-            else:
-                return self.error404()
 
-        @self.app.route("/close", methods=['POST'])
+        @self.app.route("/upload", methods=['POST'])
-        def close_public():
+        def upload_public():
-            if not self.common.settings.get('public_mode') or not self.can_upload:
+            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.

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

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

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",

share/templates/403.html

@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html>
+  <head>
+      <title>OnionShare: 403 Forbidden</title>
+      <link href="/static/img/favicon.ico" rel="icon" type="image/x-icon" />
+      <link href="/static/css/style.css" rel="stylesheet" type="text/css" />
+  </head>
+  <body>
+      <div class="info-wrapper">
+          <div class="info">
+              <p><img class="logo" src="/static/img/logo_large.png" title="OnionShare"></p>
+              <p class="info-header">You are not allowed to perform that action at this time.</p>
+	  </div>
+      </div>
+  </body>
+</html>

share/templates/receive.html

@@ -34,14 +34,5 @@
         </form>
       </div>
     </div>
-    {% if receive_allow_receiver_shutdown %}
-      {% with messages = get_flashed_messages() %}
-        {% if messages %}
-          <form method="post" action="{{ close_action }}">
-            <input type="submit" class="close-button" value="I'm Finished Sending" />
-          </form>
-        {% endif %}
-      {% endwith %}
-    {% endif %}
   </body>
 </html>

test/test_onionshare_settings.py

@@ -63,8 +63,7 @@ class TestSettings:
             'private_key': '',
             'slug': '',
             'hidservauth_string': '',
-            'downloads_dir': os.path.expanduser('~/OnionShare'),
+            'downloads_dir': os.path.expanduser('~/OnionShare')
-            'receive_allow_receiver_shutdown': True,
             'public_mode': False
         }
 

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)