From 92027345d06f97b133987aa17563bcf8fa9ed812 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 10 May 2021 11:23:44 +1000 Subject: [PATCH] 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 --- .../resources/templates/500.html | 21 +++++++++++++++ cli/onionshare_cli/web/chat_mode.py | 2 +- cli/onionshare_cli/web/receive_mode.py | 6 ++--- cli/onionshare_cli/web/send_base_mode.py | 4 --- cli/onionshare_cli/web/share_mode.py | 6 ++--- cli/onionshare_cli/web/web.py | 27 +++++++++++++++++++ cli/onionshare_cli/web/website_mode.py | 4 +-- desktop/tests/gui_base_test.py | 14 ++++++++++ desktop/tests/test_gui_receive.py | 16 +++++++++++ desktop/tests/test_gui_share.py | 17 ++++++++++++ desktop/tests/test_gui_website.py | 16 +++++++++++ 11 files changed, 120 insertions(+), 13 deletions(-) create mode 100644 cli/onionshare_cli/resources/templates/500.html 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 @@ + + + + + OnionShare: An error occurred + + + + + + + +
+
+

+

Sorry, an unexpected error seems to have occurred, and your request didn't succeed.

+
+
+ + + 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("/") + @self.web.app.route("/", defaults={"path": ""}, methods=["GET"], provide_automatic_options=False) + @self.web.app.route("/", 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("//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("/") + @self.web.app.route("/", defaults={"path": ""}, methods=["GET", "POST"], provide_automatic_options=False) + @self.web.app.route("/", 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()