From 25899c6080398b84e5ed5a9a3ad24c5054ba1018 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Sun, 27 Jun 2021 18:20:11 +0430 Subject: [PATCH 1/8] handle-oserror-exceptions --- starlette/staticfiles.py | 35 ++++++++++++++++++++-------------- tests/test_staticfiles.py | 40 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index 33ea0b033..a7f3e4e94 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -111,7 +111,25 @@ async def get_response(self, path: str, scope: Scope) -> Response: if scope["method"] not in ("GET", "HEAD"): return PlainTextResponse("Method Not Allowed", status_code=405) - full_path, stat_result = await self.lookup_path(path) + try: + full_path, stat_result = await self.lookup_path(path) + except (FileNotFoundError, NotADirectoryError): + if self.html: + # Check for '404.html' if we're in HTML mode. + full_path, stat_result = await self.lookup_path("404.html") + if stat_result is not None and stat.S_ISREG(stat_result.st_mode): + return FileResponse( + full_path, + stat_result=stat_result, + method=scope["method"], + status_code=404, + ) + return PlainTextResponse("Not Found", status_code=404) + except PermissionError: + return PlainTextResponse("Permission denied", status_code=401) + except OSError: + return PlainTextResponse("Internal server error", status_code=500) + if stat_result and stat.S_ISREG(stat_result.st_mode): # We have a static file to serve. @@ -130,17 +148,6 @@ async def get_response(self, path: str, scope: Scope) -> Response: return RedirectResponse(url=url) return self.file_response(full_path, stat_result, scope) - if self.html: - # Check for '404.html' if we're in HTML mode. - full_path, stat_result = await self.lookup_path("404.html") - if stat_result is not None and stat.S_ISREG(stat_result.st_mode): - return FileResponse( - full_path, - stat_result=stat_result, - method=scope["method"], - status_code=404, - ) - return PlainTextResponse("Not Found", status_code=404) async def lookup_path( @@ -156,8 +163,8 @@ async def lookup_path( try: stat_result = await anyio.to_thread.run_sync(os.stat, full_path) return full_path, stat_result - except FileNotFoundError: - pass + except OSError: + raise return "", None def file_response( diff --git a/tests/test_staticfiles.py b/tests/test_staticfiles.py index 3c8ff240e..c6bb0b44b 100644 --- a/tests/test_staticfiles.py +++ b/tests/test_staticfiles.py @@ -1,5 +1,6 @@ import os import pathlib +import stat import time import anyio @@ -279,3 +280,42 @@ def test_staticfiles_cache_invalidation_for_deleted_file_html_mode(tmpdir): ) assert resp_deleted.status_code == 404 assert resp_deleted.text == "

404 file

" + + +def test_staticfiles_with_invalid_dir_permissions_returns_401(tmpdir): + path = os.path.join(tmpdir, "example.txt") + with open(path, "w") as file: + file.write("") + + os.chmod(tmpdir, stat.S_IRWXO) + + app = StaticFiles(directory=tmpdir) + client = TestClient(app) + response = client.get("/example.txt") + assert response.status_code == 401 + assert response.text == "Permission denied" + + +def test_staticfiles_with_missing_dir_returns_404(tmpdir): + path = os.path.join(tmpdir, "example.txt") + with open(path, "w") as file: + file.write("") + + app = StaticFiles(directory=tmpdir) + client = TestClient(app) + response = client.get("/foo/example.txt") + assert response.status_code == 404 + assert response.text == "Not Found" + + +def test_staticfiles_access_file_as_dir_returns_404(tmpdir): + path = os.path.join(tmpdir, "example.txt") + with open(path, "w") as file: + file.write("") + + app = StaticFiles(directory=tmpdir) + client = TestClient(app) + response = client.get("/example.txt/foo") + assert response.status_code == 404 + assert response.text == "Not Found" + From 5d5664b0b32c6de69eb56d556aefc398dca71d82 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Sun, 27 Jun 2021 18:50:16 +0430 Subject: [PATCH 2/8] increase coverage --- starlette/staticfiles.py | 1 - tests/test_staticfiles.py | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index a7f3e4e94..9f8fa3ec0 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -130,7 +130,6 @@ async def get_response(self, path: str, scope: Scope) -> Response: except OSError: return PlainTextResponse("Internal server error", status_code=500) - if stat_result and stat.S_ISREG(stat_result.st_mode): # We have a static file to serve. return self.file_response(full_path, stat_result, scope) diff --git a/tests/test_staticfiles.py b/tests/test_staticfiles.py index c6bb0b44b..0d6a889fa 100644 --- a/tests/test_staticfiles.py +++ b/tests/test_staticfiles.py @@ -2,6 +2,7 @@ import pathlib import stat import time +from unittest import mock import anyio import pytest @@ -319,3 +320,20 @@ def test_staticfiles_access_file_as_dir_returns_404(tmpdir): assert response.status_code == 404 assert response.text == "Not Found" + +def test_staticfiles_unhandled_os_error_returns_500(tmpdir): + path = os.path.join(tmpdir, "example.txt") + with open(path, "w") as file: + file.write("") + + app = StaticFiles(directory=tmpdir) + client = TestClient(app) + + with mock.patch( + "starlette.staticfiles.StaticFiles.lookup_path" + ) as mock_lookup_path: + mock_lookup_path.side_effect = TimeoutError() + + response = client.get("/example.txt") + assert response.status_code == 500 + assert response.text == "Internal server error" From 93803e3a09f5013f355134023f73fc62bbf57b32 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Tue, 29 Jun 2021 08:47:26 +0430 Subject: [PATCH 3/8] change lookup_path to sync method --- starlette/staticfiles.py | 20 +++++++++++--------- tests/test_staticfiles.py | 21 +++++++++++++-------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index 9f8fa3ec0..4c8e33cea 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -112,11 +112,15 @@ async def get_response(self, path: str, scope: Scope) -> Response: return PlainTextResponse("Method Not Allowed", status_code=405) try: - full_path, stat_result = await self.lookup_path(path) + full_path, stat_result = await anyio.to_thread.run_sync( + self.lookup_path, path + ) except (FileNotFoundError, NotADirectoryError): if self.html: # Check for '404.html' if we're in HTML mode. - full_path, stat_result = await self.lookup_path("404.html") + full_path, stat_result = await anyio.to_thread.run_sync( + self.lookup_path, "404.html" + ) if stat_result is not None and stat.S_ISREG(stat_result.st_mode): return FileResponse( full_path, @@ -138,7 +142,9 @@ async def get_response(self, path: str, scope: Scope) -> Response: # We're in HTML mode, and have got a directory URL. # Check if we have 'index.html' file to serve. index_path = os.path.join(path, "index.html") - full_path, stat_result = await self.lookup_path(index_path) + full_path, stat_result = await anyio.to_thread.run_sync( + self.lookup_path, index_path + ) if stat_result is not None and stat.S_ISREG(stat_result.st_mode): if not scope["path"].endswith("/"): # Directory URLs should redirect to always end in "/". @@ -149,7 +155,7 @@ async def get_response(self, path: str, scope: Scope) -> Response: return PlainTextResponse("Not Found", status_code=404) - async def lookup_path( + def lookup_path( self, path: str ) -> typing.Tuple[str, typing.Optional[os.stat_result]]: for directory in self.all_directories: @@ -159,11 +165,7 @@ async def lookup_path( # Don't allow misbehaving clients to break out of the static files # directory. continue - try: - stat_result = await anyio.to_thread.run_sync(os.stat, full_path) - return full_path, stat_result - except OSError: - raise + return full_path, os.stat(full_path) return "", None def file_response( diff --git a/tests/test_staticfiles.py b/tests/test_staticfiles.py index b43abf521..858ea44fb 100644 --- a/tests/test_staticfiles.py +++ b/tests/test_staticfiles.py @@ -288,7 +288,9 @@ def test_staticfiles_cache_invalidation_for_deleted_file_html_mode( assert resp_deleted.text == "

404 file

" -def test_staticfiles_with_invalid_dir_permissions_returns_401(tmpdir): +def test_staticfiles_with_invalid_dir_permissions_returns_401( + tmpdir, test_client_factory +): path = os.path.join(tmpdir, "example.txt") with open(path, "w") as file: file.write("") @@ -296,43 +298,46 @@ def test_staticfiles_with_invalid_dir_permissions_returns_401(tmpdir): os.chmod(tmpdir, stat.S_IRWXO) app = StaticFiles(directory=tmpdir) - client = TestClient(app) + client = test_client_factory(app) + response = client.get("/example.txt") assert response.status_code == 401 assert response.text == "Permission denied" -def test_staticfiles_with_missing_dir_returns_404(tmpdir): +def test_staticfiles_with_missing_dir_returns_404(tmpdir, test_client_factory): path = os.path.join(tmpdir, "example.txt") with open(path, "w") as file: file.write("") app = StaticFiles(directory=tmpdir) - client = TestClient(app) + client = test_client_factory(app) + response = client.get("/foo/example.txt") assert response.status_code == 404 assert response.text == "Not Found" -def test_staticfiles_access_file_as_dir_returns_404(tmpdir): +def test_staticfiles_access_file_as_dir_returns_404(tmpdir, test_client_factory): path = os.path.join(tmpdir, "example.txt") with open(path, "w") as file: file.write("") app = StaticFiles(directory=tmpdir) - client = TestClient(app) + client = test_client_factory(app) + response = client.get("/example.txt/foo") assert response.status_code == 404 assert response.text == "Not Found" -def test_staticfiles_unhandled_os_error_returns_500(tmpdir): +def test_staticfiles_unhandled_os_error_returns_500(tmpdir, test_client_factory): path = os.path.join(tmpdir, "example.txt") with open(path, "w") as file: file.write("") app = StaticFiles(directory=tmpdir) - client = TestClient(app) + client = test_client_factory(app) with mock.patch( "starlette.staticfiles.StaticFiles.lookup_path" From 0bcbd054e7513bf18b45eaa4d54d38f247eaea2c Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Sat, 14 Aug 2021 23:03:16 +0430 Subject: [PATCH 4/8] use pytest monkeypatch instead of mock --- tests/test_staticfiles.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/test_staticfiles.py b/tests/test_staticfiles.py index 858ea44fb..0cd04634a 100644 --- a/tests/test_staticfiles.py +++ b/tests/test_staticfiles.py @@ -2,7 +2,6 @@ import pathlib import stat import time -from unittest import mock import anyio import pytest @@ -331,7 +330,12 @@ def test_staticfiles_access_file_as_dir_returns_404(tmpdir, test_client_factory) assert response.text == "Not Found" -def test_staticfiles_unhandled_os_error_returns_500(tmpdir, test_client_factory): +def test_staticfiles_unhandled_os_error_returns_500( + tmpdir, test_client_factory, monkeypatch +): + def mock_timeout(*args, **kwargs): + raise TimeoutError + path = os.path.join(tmpdir, "example.txt") with open(path, "w") as file: file.write("") @@ -339,11 +343,8 @@ def test_staticfiles_unhandled_os_error_returns_500(tmpdir, test_client_factory) app = StaticFiles(directory=tmpdir) client = test_client_factory(app) - with mock.patch( - "starlette.staticfiles.StaticFiles.lookup_path" - ) as mock_lookup_path: - mock_lookup_path.side_effect = TimeoutError() + monkeypatch.setattr("starlette.staticfiles.StaticFiles.lookup_path", mock_timeout) - response = client.get("/example.txt") - assert response.status_code == 500 - assert response.text == "Internal server error" + response = client.get("/example.txt") + assert response.status_code == 500 + assert response.text == "Internal server error" From 88b7eef4748edc3b158a485ddf37a3330df82c86 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Tue, 17 Aug 2021 10:40:42 +0430 Subject: [PATCH 5/8] raise `HTTPException` instead of `PlainTextResponse` --- starlette/staticfiles.py | 18 ++++++-------- tests/test_staticfiles.py | 50 ++++++++++++++++++++++++++------------- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index 4c8e33cea..fa87e0a6e 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -7,12 +7,8 @@ import anyio from starlette.datastructures import URL, Headers -from starlette.responses import ( - FileResponse, - PlainTextResponse, - RedirectResponse, - Response, -) +from starlette.exceptions import HTTPException +from starlette.responses import FileResponse, RedirectResponse, Response from starlette.types import Receive, Scope, Send PathLike = typing.Union[str, "os.PathLike[str]"] @@ -109,7 +105,7 @@ async def get_response(self, path: str, scope: Scope) -> Response: Returns an HTTP response, given the incoming path, method and request headers. """ if scope["method"] not in ("GET", "HEAD"): - return PlainTextResponse("Method Not Allowed", status_code=405) + raise HTTPException(status_code=405) try: full_path, stat_result = await anyio.to_thread.run_sync( @@ -128,11 +124,11 @@ async def get_response(self, path: str, scope: Scope) -> Response: method=scope["method"], status_code=404, ) - return PlainTextResponse("Not Found", status_code=404) + raise HTTPException(status_code=404) except PermissionError: - return PlainTextResponse("Permission denied", status_code=401) + raise HTTPException(status_code=401) except OSError: - return PlainTextResponse("Internal server error", status_code=500) + raise if stat_result and stat.S_ISREG(stat_result.st_mode): # We have a static file to serve. @@ -153,7 +149,7 @@ async def get_response(self, path: str, scope: Scope) -> Response: return RedirectResponse(url=url) return self.file_response(full_path, stat_result, scope) - return PlainTextResponse("Not Found", status_code=404) + raise HTTPException(status_code=404) def lookup_path( self, path: str diff --git a/tests/test_staticfiles.py b/tests/test_staticfiles.py index 0cd04634a..48fdaf1a5 100644 --- a/tests/test_staticfiles.py +++ b/tests/test_staticfiles.py @@ -7,6 +7,7 @@ import pytest from starlette.applications import Starlette +from starlette.exceptions import HTTPException from starlette.requests import Request from starlette.routing import Mount from starlette.staticfiles import StaticFiles @@ -72,8 +73,10 @@ def test_staticfiles_post(tmpdir, test_client_factory): with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) + response = client.post("/example.txt") assert response.status_code == 405 assert response.text == "Method Not Allowed" @@ -84,8 +87,10 @@ def test_staticfiles_with_directory_returns_404(tmpdir, test_client_factory): with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) + response = client.get("/") assert response.status_code == 404 assert response.text == "Not Found" @@ -96,8 +101,10 @@ def test_staticfiles_with_missing_file_returns_404(tmpdir, test_client_factory): with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) + response = client.get("/404.txt") assert response.status_code == 404 assert response.text == "Not Found" @@ -137,11 +144,15 @@ def test_staticfiles_config_check_occurs_only_once(tmpdir, test_client_factory): app = StaticFiles(directory=tmpdir) client = test_client_factory(app) assert not app.config_checked - client.get("/") - assert app.config_checked - client.get("/") + + with pytest.raises(HTTPException): + client.get("/") + assert app.config_checked + with pytest.raises(HTTPException): + client.get("/") + def test_staticfiles_prevents_breaking_out_of_directory(tmpdir): directory = os.path.join(tmpdir, "foo") @@ -155,9 +166,12 @@ def test_staticfiles_prevents_breaking_out_of_directory(tmpdir): # We can't test this with 'requests', so we test the app directly here. path = app.get_path({"path": "/../example.txt"}) scope = {"method": "GET"} - response = anyio.run(app.get_response, path, scope) - assert response.status_code == 404 - assert response.body == b"Not Found" + + with pytest.raises(HTTPException) as exc_info: + anyio.run(app.get_response, path, scope) + + assert exc_info.value.status_code == 404 + assert exc_info.value.detail == "Not Found" def test_staticfiles_never_read_file_for_head_method(tmpdir, test_client_factory): @@ -296,12 +310,13 @@ def test_staticfiles_with_invalid_dir_permissions_returns_401( os.chmod(tmpdir, stat.S_IRWXO) - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) response = client.get("/example.txt") assert response.status_code == 401 - assert response.text == "Permission denied" + assert response.text == "Unauthorized" def test_staticfiles_with_missing_dir_returns_404(tmpdir, test_client_factory): @@ -309,7 +324,8 @@ def test_staticfiles_with_missing_dir_returns_404(tmpdir, test_client_factory): with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) response = client.get("/foo/example.txt") @@ -322,7 +338,8 @@ def test_staticfiles_access_file_as_dir_returns_404(tmpdir, test_client_factory) with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) client = test_client_factory(app) response = client.get("/example.txt/foo") @@ -340,11 +357,12 @@ def mock_timeout(*args, **kwargs): with open(path, "w") as file: file.write("") - app = StaticFiles(directory=tmpdir) - client = test_client_factory(app) + routes = [Mount("/", app=StaticFiles(directory=tmpdir), name="static")] + app = Starlette(routes=routes) + client = test_client_factory(app, raise_server_exceptions=False) monkeypatch.setattr("starlette.staticfiles.StaticFiles.lookup_path", mock_timeout) response = client.get("/example.txt") assert response.status_code == 500 - assert response.text == "Internal server error" + assert response.text == "Internal Server Error" From f17f693a1494ea0a178bda63b20ada06cc231c20 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Fri, 1 Oct 2021 09:32:43 +0330 Subject: [PATCH 6/8] Update starlette/staticfiles.py Co-authored-by: Marcelo Trylesinski --- starlette/staticfiles.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index fa87e0a6e..39a697260 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -117,7 +117,7 @@ async def get_response(self, path: str, scope: Scope) -> Response: full_path, stat_result = await anyio.to_thread.run_sync( self.lookup_path, "404.html" ) - if stat_result is not None and stat.S_ISREG(stat_result.st_mode): + if stat_result and stat.S_ISREG(stat_result.st_mode): return FileResponse( full_path, stat_result=stat_result, From fc544f0c8612e33333234a63f616c0ff32bb6ac6 Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Fri, 1 Oct 2021 09:37:25 +0330 Subject: [PATCH 7/8] Update staticfiles.py --- starlette/staticfiles.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index 39a697260..b0ad367dd 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -118,12 +118,7 @@ async def get_response(self, path: str, scope: Scope) -> Response: self.lookup_path, "404.html" ) if stat_result and stat.S_ISREG(stat_result.st_mode): - return FileResponse( - full_path, - stat_result=stat_result, - method=scope["method"], - status_code=404, - ) + return self.file_response(full_path, stat_result, scope, 404) raise HTTPException(status_code=404) except PermissionError: raise HTTPException(status_code=401) From 200f684b5097ff1844117bd045bb61fbf52af57c Mon Sep 17 00:00:00 2001 From: Amin Alaee Date: Mon, 4 Oct 2021 18:47:15 +0330 Subject: [PATCH 8/8] revert 404 FileResponse --- starlette/staticfiles.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/starlette/staticfiles.py b/starlette/staticfiles.py index b0ad367dd..39a697260 100644 --- a/starlette/staticfiles.py +++ b/starlette/staticfiles.py @@ -118,7 +118,12 @@ async def get_response(self, path: str, scope: Scope) -> Response: self.lookup_path, "404.html" ) if stat_result and stat.S_ISREG(stat_result.st_mode): - return self.file_response(full_path, stat_result, scope, 404) + return FileResponse( + full_path, + stat_result=stat_result, + method=scope["method"], + status_code=404, + ) raise HTTPException(status_code=404) except PermissionError: raise HTTPException(status_code=401)