Register the 405 error handler properly. Enforce the appropriate methods for each route (GET or POST only, with OPTIONS disabled). Add tests for invalid methods. Add a friendlier 500 internal server error handler

author: Miguel Jacq <mig@mig5.net> 2021-05-10 11:23:44 +1000
committer: Miguel Jacq <mig@mig5.net> 2021-05-10 11:23:44 +1000
commit: 2618e89eda600184fb6f640d00528d7fc642bf60 (patch)
tree: 12d098596f96b2e021bd353a3f7868f82554c0da
parent: e067fc2963fb86afb4e51d816dea13f701cff70d (diff)
download: onionshare-2618e89eda600184fb6f640d00528d7fc642bf60.tar.gz
onionshare-2618e89eda600184fb6f640d00528d7fc642bf60.zip
11 files changed, 120 insertions, 13 deletions
diff --git a/cli/onionshare_cli/resources/templates/500.html b/cli/onionshare_cli/resources/templates/500.html
new file mode 100644
index 00000000..9f6727d2
--- /dev/null
+++ b/cli/onionshare_cli/resources/templates/500.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+  <title>OnionShare: An error occurred</title>
+  <meta charset="utf-8" />
+  <meta name="viewport" content="width=device-width, initial-scale=1"> 
+  <link href="{{ static_url_path }}/img/favicon.ico" rel="icon" type="image/x-icon">
+  <link rel="stylesheet" rel="subresource" type="text/css" href="{{ static_url_path }}/css/style.css" media="all">
+</head>
+
+<body>
+  <div class="info-wrapper">
+    <div class="info">
+      <p><img class="logo" src="{{ static_url_path }}/img/logo_large.png" title="OnionShare"></p>
+      <p class="info-header">Sorry, an unexpected error seems to have occurred, and your request didn't succeed.</p>
+    </div>
+  </div>
+</body>
+
+</html>
diff --git a/cli/onionshare_cli/web/chat_mode.py b/cli/onionshare_cli/web/chat_mode.py
index 8b2a5673..c772818d 100644
--- a/cli/onionshare_cli/web/chat_mode.py
+++ b/cli/onionshare_cli/web/chat_mode.py
@@ -46,7 +46,7 @@ class ChatModeWeb:
         The web app routes for chatting
         """
 
-        @self.web.app.route("/")
+        @self.web.app.route("/", methods=["GET"], provide_automatic_options=False)
         def index():
             history_id = self.cur_history_id
             self.cur_history_id += 1
diff --git a/cli/onionshare_cli/web/receive_mode.py b/cli/onionshare_cli/web/receive_mode.py
index f5aae296..b3a146e3 100644
--- a/cli/onionshare_cli/web/receive_mode.py
+++ b/cli/onionshare_cli/web/receive_mode.py
@@ -71,7 +71,7 @@ class ReceiveModeWeb:
         The web app routes for receiving files
         """
 
-        @self.web.app.route("/")
+        @self.web.app.route("/", methods=["GET"], provide_automatic_options=False)
         def index():
             history_id = self.cur_history_id
             self.cur_history_id += 1
@@ -93,7 +93,7 @@ class ReceiveModeWeb:
             )
             return self.web.add_security_headers(r)
 
-        @self.web.app.route("/upload", methods=["POST"])
+        @self.web.app.route("/upload", methods=["POST"], provide_automatic_options=False)
         def upload(ajax=False):
             """
             Handle the upload files POST request, though at this point, the files have
@@ -225,7 +225,7 @@ class ReceiveModeWeb:
                     )
                     return self.web.add_security_headers(r)
 
-        @self.web.app.route("/upload-ajax", methods=["POST"])
+        @self.web.app.route("/upload-ajax", methods=["POST"], provide_automatic_options=False)
         def upload_ajax_public():
             if not self.can_upload:
                 return self.web.error403()
diff --git a/cli/onionshare_cli/web/send_base_mode.py b/cli/onionshare_cli/web/send_base_mode.py
index 742f6f75..2f3e0bbd 100644
--- a/cli/onionshare_cli/web/send_base_mode.py
+++ b/cli/onionshare_cli/web/send_base_mode.py
@@ -208,10 +208,6 @@ class SendBaseModeWeb:
         history_id = self.cur_history_id
         self.cur_history_id += 1
 
-        # Only GET requests are allowed, any other method should fail
-        if request.method != "GET":
-            return self.web.error405(history_id)
-
         self.web.add_request(
             self.web.REQUEST_INDIVIDUAL_FILE_STARTED,
             path,
diff --git a/cli/onionshare_cli/web/share_mode.py b/cli/onionshare_cli/web/share_mode.py
index 95aec1ba..51ddd674 100644
--- a/cli/onionshare_cli/web/share_mode.py
+++ b/cli/onionshare_cli/web/share_mode.py
@@ -134,8 +134,8 @@ class ShareModeWeb(SendBaseModeWeb):
         The web app routes for sharing files
         """
 
-        @self.web.app.route("/", defaults={"path": ""})
-        @self.web.app.route("/<path:path>")
+        @self.web.app.route("/", defaults={"path": ""}, methods=["GET"], provide_automatic_options=False)
+        @self.web.app.route("/<path:path>", methods=["GET"], provide_automatic_options=False)
         def index(path):
             """
             Render the template for the onionshare landing page.
@@ -160,7 +160,7 @@ class ShareModeWeb(SendBaseModeWeb):
 
             return self.render_logic(path)
 
-        @self.web.app.route("/download")
+        @self.web.app.route("/download", methods=["GET"], provide_automatic_options=False)
         def download():
             """
             Download the zip file.
diff --git a/cli/onionshare_cli/web/web.py b/cli/onionshare_cli/web/web.py
index d88a7e4e..f190d94d 100644
--- a/cli/onionshare_cli/web/web.py
+++ b/cli/onionshare_cli/web/web.py
@@ -229,6 +229,20 @@ class Web:
             mode.cur_history_id += 1
             return self.error404(history_id)
 
+        @self.app.errorhandler(405)
+        def method_not_allowed(e):
+            mode = self.get_mode()
+            history_id = mode.cur_history_id
+            mode.cur_history_id += 1
+            return self.error405(history_id)
+
+        @self.app.errorhandler(500)
+        def method_not_allowed(e):
+            mode = self.get_mode()
+            history_id = mode.cur_history_id
+            mode.cur_history_id += 1
+            return self.error500(history_id)
+
         @self.app.route("/<password_candidate>/shutdown")
         def shutdown(password_candidate):
             """
@@ -305,6 +319,19 @@ class Web:
         )
         return self.add_security_headers(r)
 
+    def error500(self, history_id):
+        self.add_request(
+            self.REQUEST_INDIVIDUAL_FILE_STARTED,
+            request.path,
+            {"id": history_id, "status_code": 500},
+        )
+
+        self.add_request(Web.REQUEST_OTHER, request.path)
+        r = make_response(
+            render_template("500.html", static_url_path=self.static_url_path), 405
+        )
+        return self.add_security_headers(r)
+
     def add_security_headers(self, r):
         """
         Add security headers to a request
diff --git a/cli/onionshare_cli/web/website_mode.py b/cli/onionshare_cli/web/website_mode.py
index 6badd399..29b2cc9b 100644
--- a/cli/onionshare_cli/web/website_mode.py
+++ b/cli/onionshare_cli/web/website_mode.py
@@ -37,8 +37,8 @@ class WebsiteModeWeb(SendBaseModeWeb):
         The web app routes for sharing a website
         """
 
-        @self.web.app.route("/", defaults={"path": ""})
-        @self.web.app.route("/<path:path>")
+        @self.web.app.route("/", defaults={"path": ""}, methods=["GET", "POST"], provide_automatic_options=False)
+        @self.web.app.route("/<path:path>", methods=["GET", "POST"], provide_automatic_options=False)
         def path_public(path):
             return path_logic(path)
 
diff --git a/desktop/tests/gui_base_test.py b/desktop/tests/gui_base_test.py
index c6a5da2f..d630cdf0 100644
--- a/desktop/tests/gui_base_test.py
+++ b/desktop/tests/gui_base_test.py
@@ -452,6 +452,20 @@ class GuiBaseTest(unittest.TestCase):
         # We should have timed out now
         self.assertEqual(tab.get_mode().server_status.status, 0)
 
+    def hit_405(self, url, expected_resp, data = {}, methods = [] ):
+        """Test various HTTP methods and the response"""
+        for method in methods:
+            if method == "put":
+                r = requests.put(url, data = data)
+            if method == "post":
+                r = requests.post(url, data = data)
+            if method == "delete":
+                r = requests.delete(url)
+            if method == "options":
+                r = requests.options(url)
+            self.assertTrue(expected_resp in r.text)
+            self.assertFalse('Werkzeug' in r.headers)
+
     # Grouped tests follow from here
 
     def run_all_common_setup_tests(self):
diff --git a/desktop/tests/test_gui_receive.py b/desktop/tests/test_gui_receive.py
index 6e14ae67..40bebc12 100644
--- a/desktop/tests/test_gui_receive.py
+++ b/desktop/tests/test_gui_receive.py
@@ -286,3 +286,19 @@ class TestReceive(GuiBaseTest):
         self.run_all_upload_non_writable_dir_tests(tab)
 
         self.close_all_tabs()
+
+    def test_405_page_returned_for_invalid_methods(self):
+        """
+        Our custom 405 page should return for invalid methods
+        """
+        tab = self.new_receive_tab()
+
+        tab.get_mode().mode_settings_widget.public_checkbox.click()
+
+        self.run_all_common_setup_tests()
+        self.run_all_receive_mode_setup_tests(tab)
+        self.run_all_receive_mode_tests(tab)
+        url = f"http://127.0.0.1:{tab.app.port}/"
+        self.hit_405(url, expected_resp="OnionShare: 405 Method Not Allowed", data = {'foo':'bar'}, methods = ["put", "post", "delete", "options"])
+
+        self.close_all_tabs()
diff --git a/desktop/tests/test_gui_share.py b/desktop/tests/test_gui_share.py
index 380d63f6..531e456f 100644
--- a/desktop/tests/test_gui_share.py
+++ b/desktop/tests/test_gui_share.py
@@ -608,3 +608,20 @@ class TestShare(GuiBaseTest):
         self.hit_401(tab)
 
         self.close_all_tabs()
+
+    def test_405_page_returned_for_invalid_methods(self):
+        """
+        Our custom 405 page should return for invalid methods
+        """
+        tab = self.new_share_tab()
+
+        tab.get_mode().autostop_sharing_checkbox.click()
+        tab.get_mode().mode_settings_widget.public_checkbox.click()
+
+        self.run_all_common_setup_tests()
+        self.run_all_share_mode_setup_tests(tab)
+        self.run_all_share_mode_started_tests(tab)
+        url = f"http://127.0.0.1:{tab.app.port}/"
+        self.hit_405(url, expected_resp="OnionShare: 405 Method Not Allowed", data = {'foo':'bar'}, methods = ["put", "post", "delete", "options"])
+        self.history_widgets_present(tab)
+        self.close_all_tabs()
diff --git a/desktop/tests/test_gui_website.py b/desktop/tests/test_gui_website.py
index a838cb96..6bb6bb7a 100644
--- a/desktop/tests/test_gui_website.py
+++ b/desktop/tests/test_gui_website.py
@@ -99,3 +99,19 @@ class TestWebsite(GuiBaseTest):
         tab.get_mode().disable_csp_checkbox.click()
         self.run_all_website_mode_download_tests(tab)
         self.close_all_tabs()
+
+    def test_405_page_returned_for_invalid_methods(self):
+        """
+        Our custom 405 page should return for invalid methods
+        """
+        tab = self.new_website_tab()
+
+        tab.get_mode().mode_settings_widget.public_checkbox.click()
+
+        self.run_all_common_setup_tests()
+        self.run_all_website_mode_setup_tests(tab)
+        self.run_all_website_mode_started_tests(tab)
+        url = f"http://127.0.0.1:{tab.app.port}/"
+        self.hit_405(url, expected_resp="OnionShare: 405 Method Not Allowed", data = {'foo':'bar'}, methods = ["put", "delete", "options"])
+
+        self.close_all_tabs()
author	Miguel Jacq <mig@mig5.net>	2021-05-10 11:23:44 +1000
committer	Miguel Jacq <mig@mig5.net>	2021-05-10 11:23:44 +1000
commit	2618e89eda600184fb6f640d00528d7fc642bf60 (patch)
tree	12d098596f96b2e021bd353a3f7868f82554c0da
parent	e067fc2963fb86afb4e51d816dea13f701cff70d (diff)
download	onionshare-2618e89eda600184fb6f640d00528d7fc642bf60.tar.gz onionshare-2618e89eda600184fb6f640d00528d7fc642bf60.zip