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

This commit is contained in:
Miguel Jacq 2021-05-10 11:23:44 +10:00
parent e067fc2963
commit 2618e89eda
No known key found for this signature in database
GPG Key ID: EEA4341C6D97A0B6
11 changed files with 120 additions and 13 deletions

View File

@ -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>

View File

@ -46,7 +46,7 @@ class ChatModeWeb:
The web app routes for chatting The web app routes for chatting
""" """
@self.web.app.route("/") @self.web.app.route("/", methods=["GET"], provide_automatic_options=False)
def index(): def index():
history_id = self.cur_history_id history_id = self.cur_history_id
self.cur_history_id += 1 self.cur_history_id += 1

View File

@ -71,7 +71,7 @@ class ReceiveModeWeb:
The web app routes for receiving files The web app routes for receiving files
""" """
@self.web.app.route("/") @self.web.app.route("/", methods=["GET"], provide_automatic_options=False)
def index(): def index():
history_id = self.cur_history_id history_id = self.cur_history_id
self.cur_history_id += 1 self.cur_history_id += 1
@ -93,7 +93,7 @@ class ReceiveModeWeb:
) )
return self.web.add_security_headers(r) 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): def upload(ajax=False):
""" """
Handle the upload files POST request, though at this point, the files have 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) 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(): def upload_ajax_public():
if not self.can_upload: if not self.can_upload:
return self.web.error403() return self.web.error403()

View File

@ -208,10 +208,6 @@ class SendBaseModeWeb:
history_id = self.cur_history_id history_id = self.cur_history_id
self.cur_history_id += 1 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.add_request(
self.web.REQUEST_INDIVIDUAL_FILE_STARTED, self.web.REQUEST_INDIVIDUAL_FILE_STARTED,
path, path,

View File

@ -134,8 +134,8 @@ class ShareModeWeb(SendBaseModeWeb):
The web app routes for sharing files The web app routes for sharing files
""" """
@self.web.app.route("/", defaults={"path": ""}) @self.web.app.route("/", defaults={"path": ""}, methods=["GET"], provide_automatic_options=False)
@self.web.app.route("/<path:path>") @self.web.app.route("/<path:path>", methods=["GET"], provide_automatic_options=False)
def index(path): def index(path):
""" """
Render the template for the onionshare landing page. Render the template for the onionshare landing page.
@ -160,7 +160,7 @@ class ShareModeWeb(SendBaseModeWeb):
return self.render_logic(path) return self.render_logic(path)
@self.web.app.route("/download") @self.web.app.route("/download", methods=["GET"], provide_automatic_options=False)
def download(): def download():
""" """
Download the zip file. Download the zip file.

View File

@ -229,6 +229,20 @@ class Web:
mode.cur_history_id += 1 mode.cur_history_id += 1
return self.error404(history_id) 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") @self.app.route("/<password_candidate>/shutdown")
def shutdown(password_candidate): def shutdown(password_candidate):
""" """
@ -305,6 +319,19 @@ class Web:
) )
return self.add_security_headers(r) 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): def add_security_headers(self, r):
""" """
Add security headers to a request Add security headers to a request

View File

@ -37,8 +37,8 @@ class WebsiteModeWeb(SendBaseModeWeb):
The web app routes for sharing a website The web app routes for sharing a website
""" """
@self.web.app.route("/", defaults={"path": ""}) @self.web.app.route("/", defaults={"path": ""}, methods=["GET", "POST"], provide_automatic_options=False)
@self.web.app.route("/<path:path>") @self.web.app.route("/<path:path>", methods=["GET", "POST"], provide_automatic_options=False)
def path_public(path): def path_public(path):
return path_logic(path) return path_logic(path)

View File

@ -452,6 +452,20 @@ class GuiBaseTest(unittest.TestCase):
# We should have timed out now # We should have timed out now
self.assertEqual(tab.get_mode().server_status.status, 0) 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 # Grouped tests follow from here
def run_all_common_setup_tests(self): def run_all_common_setup_tests(self):

View File

@ -286,3 +286,19 @@ class TestReceive(GuiBaseTest):
self.run_all_upload_non_writable_dir_tests(tab) self.run_all_upload_non_writable_dir_tests(tab)
self.close_all_tabs() 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()

View File

@ -608,3 +608,20 @@ class TestShare(GuiBaseTest):
self.hit_401(tab) self.hit_401(tab)
self.close_all_tabs() 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()

View File

@ -99,3 +99,19 @@ class TestWebsite(GuiBaseTest):
tab.get_mode().disable_csp_checkbox.click() tab.get_mode().disable_csp_checkbox.click()
self.run_all_website_mode_download_tests(tab) self.run_all_website_mode_download_tests(tab)
self.close_all_tabs() 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()