begin rewriting path to resource resolver

this code was getting too messy and scattered, and I realized that Flask
wasn't doing as much as I thought it was here, so now we need more
safety and sanity checks

incorporealcms/pages.py

@@ -19,14 +19,18 @@ bp = Blueprint('pages', __name__, url_prefix='/')
 @bp.route('/<path:path>')
 def display_page(path):
     """Get the file contents of the requested path and render the file."""
-    if is_file_path_actually_dir_path(path):
+    try:
-        return redirect(f'{path}/', code=301)
+        resolved_path = request_path_to_instance_resource(path)
-
-    resolved_path = resolve_page_file(path)
         logger.debug("received request for path '%s', resolved to '%s'", path, resolved_path)
+    except PermissionError:
+        abort(400)
+    except IsADirectoryError:
+        return redirect(f'{path}/', code=301)
+    except FileNotFoundError:
+        abort(404)
+
     try:
         with app.open_instance_resource(resolved_path, 'r') as entry_file:
-            logger.debug("file '%s' found", resolved_path)
             parent_navs = generate_parent_navs(path)
             mtime = datetime.datetime.fromtimestamp(os.path.getmtime(entry_file.name), get_localzone())
             entry = entry_file.read()
@@ -67,6 +71,45 @@ def render(template_name_or_list, **context):
     return resp
 
 
+def request_path_to_instance_resource(path):
+    """Turn a request URL path to the full page path.
+
+    flask.Flask.open_instance_resource will open a file like /etc/hosts if you tell it to,
+    which sucks, so we do a lot of work here to make sure we have a valid request to
+    something inside the pages dir.
+    """
+    # check if the path is allowed
+    base_dir = os.path.realpath(f'{app.instance_path}/pages/')
+    resolved_path = os.path.realpath(os.path.join(base_dir, path))
+    logger.debug("base_dir: %s, constructed resolved_path: %s", base_dir, resolved_path)
+
+    # bail if the requested real path isn't inside the base directory
+    if base_dir != os.path.commonpath((base_dir, resolved_path)):
+        logger.warning("client tried to request a path '%s' outside of the base_dir!", path)
+        raise PermissionError
+
+    # if this is a file-like requset but actually a directory, redirect the user
+    if os.path.isdir(resolved_path) and not path.endswith('/'):
+        logger.info("client requested a path '%s' that is actually a directory", path)
+        raise IsADirectoryError
+
+    # derive the proper markdown file depending on if this is a dir or file
+    if os.path.isdir(resolved_path):
+        absolute_resource = os.path.join(resolved_path, 'index.md')
+    else:
+        absolute_resource = f'{resolved_path}.md'
+
+    logger.info("final path = '%s' for request '%s'", absolute_resource, path)
+
+    # does the final file actually exist?
+    if not os.path.exists(absolute_resource):
+        logger.warning("requested final path '%s' does not exist!", absolute_resource)
+        raise FileNotFoundError
+
+    # we checked that the file exists via absolute path, but now we need to give the path relative to instance dir
+    return absolute_resource.replace(f'{app.instance_path}{os.path.sep}', '')
+
+
 def resolve_page_file(path):
     """Manipulate the request path to find appropriate page file.
 
@@ -81,22 +124,6 @@ def resolve_page_file(path):
     return path
 
 
-def is_file_path_actually_dir_path(path):
-    """Check if requested path which looks like a file is actually a directory.
-
-    If, for example, /foo used to be a file (foo.md) which later became a directory,
-    foo/, this returns True. Useful for when I make my structure more complicated
-    than it originally was, or if users are just weird.
-    """
-    if not path.endswith('/'):
-        logger.debug("requested path '%s' looks like a file", path)
-        if os.path.isdir(f'{app.instance_path}/pages/{path}'):
-            logger.debug("...and it's actually a dir")
-            return True
-
-    return False
-
-
 def generate_parent_navs(path):
     """Create a series of paths/links to navigate up from the given path."""
     # derive additional path/location stuff based on path

tests/instance/unreachable.md

@@ -0,0 +1 @@
+this file exists but the app should not serve it.

tests/test_pages.py

@@ -1,7 +1,8 @@
 """Unit test helper methods."""
+import pytest
 from werkzeug.http import dump_cookie
 
-from incorporealcms.pages import generate_parent_navs, is_file_path_actually_dir_path, render, resolve_page_file
+from incorporealcms.pages import generate_parent_navs, render, request_path_to_instance_resource,  resolve_page_file
 
 
 def test_resolve_page_file_dir_to_index():
@@ -54,30 +55,6 @@ def test_generate_page_navs_subdir_with_title_parsing_real_page(app):
         ]
 
 
-def test_is_file_path_actually_dir_path_valid_file_is_yes(app):
-    """Test that a file request for what's actually a directory is detected as such."""
-    with app.app_context():
-        assert is_file_path_actually_dir_path('/subdir')
-
-
-def test_is_file_path_actually_dir_path_valid_dir_is_no(app):
-    """Test that a directory request is still a directory request."""
-    with app.app_context():
-        assert not is_file_path_actually_dir_path('/subdir/')
-
-
-def test_is_file_path_actually_dir_path_nonsense_file_is_no(app):
-    """Test that requests to nonsense file-looking paths aren't treated as dirs."""
-    with app.app_context():
-        assert not is_file_path_actually_dir_path('/antphnathpnthapnthsnthax')
-
-
-def test_is_file_path_actually_dir_path_nonsense_dir_is_no(app):
-    """Test that a directory request is a directory request even if the dir doesn't exist."""
-    with app.app_context():
-        assert not is_file_path_actually_dir_path('/antphnathpnthapnthsnthax/')
-
-
 def test_render_with_user_dark_theme(app):
     """Test that a request with the dark theme selected renders the dark theme."""
     cookie = dump_cookie("user-style", 'dark')
@@ -98,3 +75,73 @@ def test_render_with_no_user_theme(app):
     with app.test_request_context():
         assert b'light.css' in render('base.html').data
         assert b'dark.css' not in render('base.html').data
+
+
+def test_request_path_to_instance_resource(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('index') == 'pages/index.md'
+
+
+def test_request_path_to_instance_resource_direct_file(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('no-title') == 'pages/no-title.md'
+
+
+def test_request_path_to_instance_resource_in_subdir(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('subdir/page') == 'pages/subdir/page.md'
+
+
+def test_request_path_to_instance_resource_subdir_index(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('subdir/') == 'pages/subdir/index.md'
+
+
+def test_request_path_to_instance_resource_relatives_walked(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('subdir/more-subdir/../../more-metadata') == 'pages/more-metadata.md'
+
+
+def test_request_path_to_instance_resource_relatives_walked_indexes_work_too(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('subdir/more-subdir/../../') == 'pages/index.md'
+
+
+def test_request_path_to_instance_resource_relatives_walked_into_subdirs_also_fine(app):
+    """Test a normal URL request is transformed into the file path."""
+    with app.test_request_context():
+        assert request_path_to_instance_resource('subdir/more-subdir/../../subdir/page') == 'pages/subdir/page.md'
+
+
+def test_request_path_to_instance_resource_permission_error_on_ref_above_pages(app):
+    """Test that attempts to get above the base dir ("/../../foo") fail."""
+    with app.test_request_context():
+        with pytest.raises(PermissionError):
+            assert request_path_to_instance_resource('../unreachable')
+
+
+def test_request_path_to_instance_resource_isadirectory_on_file_like_req_for_dir(app):
+    """Test that a request for e.g. '/foo' when foo is a dir indicate to redirect."""
+    with app.test_request_context():
+        with pytest.raises(IsADirectoryError):
+            assert request_path_to_instance_resource('subdir')
+
+
+def test_request_path_to_instance_resource_nonexistant_file_errors(app):
+    """Test that a request for something not on disk errors."""
+    with app.test_request_context():
+        with pytest.raises(FileNotFoundError):
+            assert request_path_to_instance_resource('nthanpthpnh')
+
+
+def test_request_path_to_instance_resource_absolute_file_errors(app):
+    """Test that a request for something not on disk errors."""
+    with app.test_request_context():
+        with pytest.raises(PermissionError):
+            assert request_path_to_instance_resource('/etc/hosts')