From 2f6ea646e0123bc3efdcbe9f7d6a8833174516e0 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Thu, 28 Jul 2022 11:05:37 +0300 Subject: [PATCH 1/4] Use pathlib for path resolution --- sanic/mixins/routes.py | 23 +++++++++++++---------- tests/test_static.py | 3 +++ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/sanic/mixins/routes.py b/sanic/mixins/routes.py index 8a5f10cc9c..b8f2b3a173 100644 --- a/sanic/mixins/routes.py +++ b/sanic/mixins/routes.py @@ -3,8 +3,8 @@ from functools import partial, wraps from inspect import getsource, signature from mimetypes import guess_type -from os import path, sep -from pathlib import PurePath +from os import path +from pathlib import Path, PurePath from textwrap import dedent from time import gmtime, strftime from typing import ( @@ -806,22 +806,25 @@ async def _static_request_handler( __file_uri__=None, ): # Merge served directory and requested file if provided - root_path = file_path = path.abspath(unquote(file_or_directory)) + file_path_raw = Path(unquote(file_or_directory)) + root_path = file_path = file_path_raw.resolve() if __file_uri__: # Strip all / that in the beginning of the URL to help prevent # python from herping a derp and treating the uri as an # absolute path unquoted_file_uri = unquote(__file_uri__).lstrip("/") - - segments = unquoted_file_uri.split("/") - if ".." in segments or any(sep in segment for segment in segments): + file_path_raw = Path(file_or_directory, unquoted_file_uri) + file_path = file_path_raw.resolve() + if ( + file_path < root_path and not file_path_raw.is_symlink() + ) or file_path_raw.match("../**/*"): raise BadRequest("Invalid URL") - file_path = path.join(file_or_directory, unquoted_file_uri) - file_path = path.abspath(file_path) - - if not file_path.startswith(root_path): + if ( + not file_path.is_relative_to(root_path) + and not file_path_raw.is_symlink() + ): error_logger.exception( f"File not found: path={file_or_directory}, " f"relative_url={__file_uri__}" diff --git a/tests/test_static.py b/tests/test_static.py index aeb625b7b7..1126cbb61c 100644 --- a/tests/test_static.py +++ b/tests/test_static.py @@ -616,6 +616,9 @@ def test_dotted_dir_ok( def test_breakout(app: Sanic, static_file_directory: str): app.static("/foo", static_file_directory) + _, response = app.test_client.get("/foo/..%2Ffake/server.py") + assert response.status == 400 + _, response = app.test_client.get("/foo/..%2Fstatic/test.file") assert response.status == 400 From e68a0d8fe8735f28dba1cfdca1aedd3d97e068fa Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Thu, 28 Jul 2022 11:17:32 +0300 Subject: [PATCH 2/4] Only use 404 --- sanic/mixins/routes.py | 30 +++++++++++++----------------- tests/test_static.py | 8 ++++---- 2 files changed, 17 insertions(+), 21 deletions(-) diff --git a/sanic/mixins/routes.py b/sanic/mixins/routes.py index b8f2b3a173..6d27c12048 100644 --- a/sanic/mixins/routes.py +++ b/sanic/mixins/routes.py @@ -26,12 +26,7 @@ from sanic.compat import stat_async from sanic.constants import DEFAULT_HTTP_CONTENT_TYPE, HTTP_METHODS from sanic.errorpages import RESPONSE_MAPPING -from sanic.exceptions import ( - BadRequest, - FileNotFound, - HeaderNotFound, - RangeNotSatisfiable, -) +from sanic.exceptions import FileNotFound, HeaderNotFound, RangeNotSatisfiable from sanic.handlers import ContentRangeHandler from sanic.log import error_logger from sanic.models.futures import FutureRoute, FutureStatic @@ -808,6 +803,11 @@ async def _static_request_handler( # Merge served directory and requested file if provided file_path_raw = Path(unquote(file_or_directory)) root_path = file_path = file_path_raw.resolve() + not_found = FileNotFound( + "File not found", + path=file_or_directory, + relative_url=__file_uri__, + ) if __file_uri__: # Strip all / that in the beginning of the URL to help prevent @@ -819,7 +819,11 @@ async def _static_request_handler( if ( file_path < root_path and not file_path_raw.is_symlink() ) or file_path_raw.match("../**/*"): - raise BadRequest("Invalid URL") + error_logger.exception( + f"File not found: path={file_or_directory}, " + f"relative_url={__file_uri__}" + ) + raise not_found if ( not file_path.is_relative_to(root_path) @@ -829,11 +833,7 @@ async def _static_request_handler( f"File not found: path={file_or_directory}, " f"relative_url={__file_uri__}" ) - raise FileNotFound( - "File not found", - path=file_or_directory, - relative_url=__file_uri__, - ) + raise not_found try: headers = {} # Check if the client has been sent this file before @@ -901,11 +901,7 @@ async def _static_request_handler( except RangeNotSatisfiable: raise except FileNotFoundError: - raise FileNotFound( - "File not found", - path=file_or_directory, - relative_url=__file_uri__, - ) + raise not_found except Exception: error_logger.exception( f"Exception in static request handler: " diff --git a/tests/test_static.py b/tests/test_static.py index 1126cbb61c..c51e7e7e4f 100644 --- a/tests/test_static.py +++ b/tests/test_static.py @@ -617,10 +617,10 @@ def test_breakout(app: Sanic, static_file_directory: str): app.static("/foo", static_file_directory) _, response = app.test_client.get("/foo/..%2Ffake/server.py") - assert response.status == 400 + assert response.status == 404 _, response = app.test_client.get("/foo/..%2Fstatic/test.file") - assert response.status == 400 + assert response.status == 404 @pytest.mark.skipif( @@ -632,6 +632,6 @@ def test_double_backslash_prohibited_on_win32( app.static("/foo", static_file_directory) _, response = app.test_client.get("/foo/static/..\\static/test.file") - assert response.status == 400 + assert response.status == 404 _, response = app.test_client.get("/foo/static\\../static/test.file") - assert response.status == 400 + assert response.status == 404 From 1fac88686e6fd15f615eab283d3bd749f658ad72 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Thu, 28 Jul 2022 11:35:21 +0300 Subject: [PATCH 3/4] Do not use is_relative_to --- sanic/mixins/routes.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/sanic/mixins/routes.py b/sanic/mixins/routes.py index 6d27c12048..efb522f4d8 100644 --- a/sanic/mixins/routes.py +++ b/sanic/mixins/routes.py @@ -825,15 +825,15 @@ async def _static_request_handler( ) raise not_found - if ( - not file_path.is_relative_to(root_path) - and not file_path_raw.is_symlink() - ): - error_logger.exception( - f"File not found: path={file_or_directory}, " - f"relative_url={__file_uri__}" - ) - raise not_found + try: + file_path.relative_to(root_path) + except ValueError: + if not file_path_raw.is_symlink(): + error_logger.exception( + f"File not found: path={file_or_directory}, " + f"relative_url={__file_uri__}" + ) + raise not_found try: headers = {} # Check if the client has been sent this file before From d83229a75f407f96ee579d24d38612e9eb3cf035 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Thu, 28 Jul 2022 15:38:37 +0300 Subject: [PATCH 4/4] Informational patch codecov --- codecov.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/codecov.yml b/codecov.yml index 5905afd56c..f42568665c 100644 --- a/codecov.yml +++ b/codecov.yml @@ -4,6 +4,7 @@ coverage: default: target: auto threshold: 0.75 + informational: true project: default: target: auto