Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ContentsHandler return 404 rather than raise exc #1357

Merged
merged 6 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions jupyter_server/services/contents/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Copyright (c) Jupyter Development Team.
# Distributed under the terms of the Modified BSD License.
import json
from http import HTTPStatus

try:
from jupyter_client.jsonutil import json_default
Expand Down Expand Up @@ -91,6 +92,12 @@ def _finish_model(self, model, location=True):
self.set_header("Content-Type", "application/json")
self.finish(json.dumps(model, default=json_default))

async def _finish_error(self, code, message):
"""Finish a JSON request with an error code and descriptive message"""
self.set_status(code)
self.write(message)
await self.finish()

@web.authenticated
@authorized
async def get(self, path=""):
Expand All @@ -116,18 +123,27 @@ async def get(self, path=""):
content = int(content_str or "")

if not cm.allow_hidden and await ensure_async(cm.is_hidden(path)):
raise web.HTTPError(404, f"file or directory {path!r} does not exist")

model = await ensure_async(
self.contents_manager.get(
path=path,
type=type,
format=format,
content=content,
await self._finish_error(
HTTPStatus.NOT_FOUND, f"file or directory {path!r} does not exist"
)
)
validate_model(model, expect_content=content)
self._finish_model(model, location=False)
try:
model = await ensure_async(
self.contents_manager.get(
path=path,
type=type,
format=format,
content=content,
)
)
validate_model(model, expect_content=content)
self._finish_model(model, location=False)
except web.HTTPError as exc:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest adding some comments here to explain why a web.HTTPError needs to be caught here, as it doesn't make any difference to the client

Mostly in #1328, we don't want a meaningless stack throw, which means that the situation here is fully under control.

# 404 is okay in this context, catch exception and return 404 code to prevent stack trace on client
if exc.status_code == HTTPStatus.NOT_FOUND:
await self._finish_error(
HTTPStatus.NOT_FOUND, f"file or directory {path!r} does not exist"
)
raise

@web.authenticated
@authorized
Expand Down
55 changes: 32 additions & 23 deletions tests/services/contents/test_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,35 +406,44 @@ async def test_400(jp_file_contents_manager_class, tmp_path): # noqa


async def test_404(jp_file_contents_manager_class, tmp_path):
# setup
td = str(tmp_path)
cm = jp_file_contents_manager_class(root_dir=td)

# Test visible file in hidden folder
with pytest.raises(HTTPError) as excinfo:
td = str(tmp_path)
cm = jp_file_contents_manager_class(root_dir=td)
hidden_dir = ".hidden"
file_in_hidden_path = os.path.join(hidden_dir, "visible.txt")
_make_dir(cm, hidden_dir)
model = await ensure_async(cm.new(path=file_in_hidden_path))
os_path = cm._get_os_path(model["path"])
cm.allow_hidden = True
hidden_dir = ".hidden"
file_in_hidden_path = os.path.join(hidden_dir, "visible.txt")
_make_dir(cm, hidden_dir)
model = await ensure_async(cm.new(path=file_in_hidden_path))
os_path = cm._get_os_path(model["path"])
cm.allow_hidden = False
Comment on lines +414 to +420
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the "setup" was contained within the 'with' an exception is thrown when creating the file since allow_hidden is False. The fix was to allow hidden while creating the test file, and then disallow during the actual method test.


try:
result = await ensure_async(cm.get(os_path, "w"))
except HTTPError as e:
assert e.status_code == 404
with pytest.raises(HTTPError) as excinfo:
await ensure_async(cm.get(os_path))
assert excinfo.value.status_code == 404

# Test hidden file in visible folder
cm.allow_hidden = True
hidden_dir = "visible"
file_in_hidden_path = os.path.join(hidden_dir, ".hidden.txt")
_make_dir(cm, hidden_dir)
model = await ensure_async(cm.new(path=file_in_hidden_path))
os_path = cm._get_os_path(model["path"])
cm.allow_hidden = False

with pytest.raises(HTTPError) as excinfo:
td = str(tmp_path)
cm = jp_file_contents_manager_class(root_dir=td)
hidden_dir = "visible"
file_in_hidden_path = os.path.join(hidden_dir, ".hidden.txt")
_make_dir(cm, hidden_dir)
model = await ensure_async(cm.new(path=file_in_hidden_path))
os_path = cm._get_os_path(model["path"])
await ensure_async(cm.get(os_path))
assert excinfo.value.status_code == 404

try:
result = await ensure_async(cm.get(os_path, "w"))
except HTTPError as e:
assert e.status_code == 404
# Test file not found
td = str(tmp_path)
cm = jp_file_contents_manager_class(root_dir=td)
not_a_file = "foo.bar"

with pytest.raises(HTTPError) as excinfo:
await ensure_async(cm.get(not_a_file))
assert excinfo.value.status_code == 404


async def test_escape_root(jp_file_contents_manager_class, tmp_path):
Expand Down
Loading