Repository: lektor/lektor
Pull request #1053 (2022-08-12 03:34:46)

Admin preview view fails to track links with anchors
<!---
Remember to please check that your code passes tests locally. Test with the Makefile
commands `make test`, or if you only need to test just Python / JS `make test-python`
or `make test-js`.
--->

In the _preview_ view of the Admin UI, normally when one clicks on links to other pages in the site, the admin interface tracks the navigation and updates appropriately for the page that one navigated to.

Currently, that tracking fails if the link followed includes a fragment (#anchor) (or a query string.)

Here's a fix.  This makes [`Pad.resolve_url`](https://github.com/lektor/lektor/blob/6d7bd4db43584ffa14158ccf5f4f5211a2567f14/lektor/db.py#L1656) ignore any query or fragment on any URL paths passed to it.


### Related Issues / Links

<!---
Are there any similar or related issues or pull requests?
Did you make a pull request to update the docs?
--->

### Description of Changes

- [x] Wrote at least one-line docstrings (for any new functions)
- [x] Added unit test(s) covering the changes (if testable)


<!--- Explain what you've done and why --->

## Breaking change in `cleanup_path`

This also makes a *conceivably breaking* change in `lektor.utils.cleanup_path` (which is used to normalize Lektor DB paths.)

While mucking about to fix the primary issue, I noticed that `lektor.utils.cleanup_path` currently does not handle double-dots (`..`) in paths in the manner that most of us would expect.
 also makes a *conceivable breaking* change in `lektor.utils.cleanup_path` (which is used to normalize Lektor DB paths.)

Until now, `cleanup_path` was treating path components consisting of two dots (`..`) the same as a single dot and just stripping them from the path.  This is quite different than the normal interpretation of two dots, which indicates traversal to the parent directory.

I've added a commit to this PR which fixes that so that double-dots are now interpreted as a traversal to the parent.

In practice, this change is unlikely to affect many users.  They would notice it if they were doing something like:
```py
pad.get("/parent/child/../child2")
```
which, currently would retrieve the record at `/parent/child/child2`, but after this PR will retrieve `/parent/child2` (as one would naïvely expect in the first place.)

Note that relative URL resolution uses `lektor.utils.join_path` (rather than `cleanup_path`) and `join_path` already handles double-dots in an expected manner.  So things like `{{ "../child2" | url }}` already work as expected (if invoked from `/parent/child`, it will compute the URL to `/parent/child2`). Such uses are unaffected by this PR.

<!--- Thanks for your help making Lektor better for everyone! --->



Patch:
diff --git a/lektor/db.py b/lektor/db.py
index bae4b54..a414c18 100644
--- a/lektor/db.py
+++ b/lektor/db.py
@@ -35,6 +35,7 @@ from lektor.imagetools import ThumbnailMode
 from lektor.sourceobj import SourceObject
 from lektor.sourceobj import VirtualSourceObject
 from lektor.utils import cleanup_path
+from lektor.utils import cleanup_url_path
 from lektor.utils import fs_enc
 from lektor.utils import locate_executable
 from lektor.utils import make_relative_url
@@ -1660,7 +1661,10 @@ class Pad:
         might be an attachment.  If a record cannot be found or is unexposed
         the return value will be `None`.
         """
-        pieces = clean_path = cleanup_path(url_path).strip("/")
+        try:
+            clean_path = cleanup_url_path(url_path).strip("/")
+        except ValueError:
+            return None
 
         # Split off the alt and if no alt was found, point it to the
         # primary alternative.  If the clean path comes back as `None`
diff --git a/lektor/utils.py b/lektor/utils.py
index b686a4a..a989409 100644
--- a/lektor/utils.py
+++ b/lektor/utils.py
@@ -13,6 +13,7 @@ from contextlib import contextmanager
 from datetime import datetime
 from functools import lru_cache
 from pathlib import PurePosixPath
+from urllib.parse import urlsplit
 
 from jinja2 import is_undefined
 from markupsafe import Markup
@@ -26,7 +27,6 @@ is_windows = os.name == "nt"
 
 _slash_escape = "\\/" not in json.dumps("/")
 
-_slashes_re = re.compile(r"(/\.{1,2}(/|$))|/")
 _last_num_re = re.compile(r"^(.*)(\d+)(.*?)$")
 _list_marker = object()
 _value_marker = object()
@@ -74,7 +74,29 @@ def join_path(a, b):
 
 
 def cleanup_path(path):
-    return "/" + _slashes_re.sub("/", path).strip("/")
+    # NB: POSIX allows for two leading slashes in a pathname, so we have to
+    # deal with the possiblity of leading double-slash ourself.
+    return posixpath.normpath("/" + path.lstrip("/"))
+
+
+def cleanup_url_path(url_path):
+    """Clean up a URL path.
+
+    This strips any query, and/or fragment that may be present in the
+    input path.
+
+    Raises ValueError if the path contains a _scheme_
+    which is neither ``http`` nor ``https``, or a _netloc_.
+    """
+    scheme, netloc, path, _, _ = urlsplit(url_path, scheme="http")
+    if scheme not in ("http", "https"):
+        raise ValueError(f"Invalid scheme: {url_path!r}")
+    if netloc:
+        raise ValueError(f"Invalid netloc: {url_path!r}")
+
+    # NB: POSIX allows for two leading slashes in a pathname, so we have to
+    # deal with the possiblity of leading double-slash ourself.
+    return posixpath.normpath("/" + path.lstrip("/"))
 
 
 def parse_path(path):


Test patch:
diff --git a/tests/test_db.py b/tests/test_db.py
index b6c434d..389c675 100644
--- a/tests/test_db.py
+++ b/tests/test_db.py
@@ -82,8 +82,12 @@ def test_url_matching_with_customized_slug_in_alt(pad):
     "path",
     [
         "/",
-        "/extra/container/a",  # child if hidden page explicit marked as non-hidden
+        "/extra/container/a",  # child of hidden page explicit marked as non-hidden
         "/extra/container/hello.txt",  # attachment of hidden page
+        "/#fragment",  # fragment should be ignored
+        "/?query",  # query should be ignored
+        "http:/",  # http scheme should be ignored
+        "https:/",  # https scheme should be ignored
     ],
 )
 def test_resolve_url(pad, path):
@@ -100,6 +104,17 @@ def test_resolve_url_asset(pad):
     assert pad.resolve_url_path("/static/demo.css", include_assets=False) is None
 
 
+@pytest.mark.parametrize(
+    "path",
+    [
+        "ftp:///",  # bad scheme
+        "//localhost/",  # path should not have a netloc
+    ],
+)
+def test_resolve_url_invalid_path(pad, path):
+    assert pad.resolve_url_path(path) is None
+
+
 def test_basic_alts(pad):
     with Context(pad=pad):
         assert get_alts() == ["en", "de"]
diff --git a/tests/test_urls.py b/tests/test_urls.py
index 1240d73..156e19c 100644
--- a/tests/test_urls.py
+++ b/tests/test_urls.py
@@ -5,18 +5,73 @@ import pytest
 from lektor.context import _ctx_stack
 from lektor.context import Context
 from lektor.utils import cleanup_path
+from lektor.utils import cleanup_url_path
 
 
-def test_cleanup_path():
-    assert cleanup_path("/") == "/"
-    assert cleanup_path("/foo") == "/foo"
-    assert cleanup_path("/foo/") == "/foo"
-    assert cleanup_path("/////foo/") == "/foo"
-    assert cleanup_path("/////foo////") == "/foo"
-    assert cleanup_path("/////foo/.///") == "/foo"
-    assert cleanup_path("/////foo/..///") == "/foo"
-    assert cleanup_path("/foo/./bar/") == "/foo/bar"
-    assert cleanup_path("/foo/../bar/") == "/foo/bar"
+@pytest.mark.parametrize(
+    "path, expected",
+    [
+        ("", "/"),
+        ("/", "/"),
+        ("/foo", "/foo"),
+        ("//foo", "/foo"),
+        ("///foo", "/foo"),
+        ("/foo/", "/foo"),
+        ("/////foo/", "/foo"),
+        ("/////foo////", "/foo"),
+        ("/////foo/.///", "/foo"),
+        ("/////foo/..///", "/"),
+        (".", "/"),
+        ("/foo/./bar/", "/foo/bar"),
+        ("/foo/../bar/", "/bar"),
+    ],
+)
+def test_cleanup_path(path, expected):
+    assert cleanup_path(path) == expected
+
+
+@pytest.mark.parametrize(
+    "url_path, expected",
+    [
+        ("", "/"),
+        ("foo", "/foo"),
+        ("foo/", "/foo"),
+        ("/", "/"),
+        ("//", "/"),
+        ("/foo", "/foo"),
+        ("/foo/", "/foo"),
+        ("///foo", "/foo"),
+        ("////foo", "/foo"),
+        ("/////foo", "/foo"),
+        ("foo///bar", "/foo/bar"),
+        ("./foo/.", "/foo"),
+        ("/foo/./bar", "/foo/bar"),
+        ("../foo", "/foo"),
+        ("/../foo/", "/foo"),
+        ("foo/../bar", "/bar"),
+        ("foo/../../bar/", "/bar"),
+        ("foo#frag", "/foo"),  # fragment gets stripped
+        ("/foo?query", "/foo"),  # query get stripped
+        ("///foo", "/foo"),  # empty netloc
+        ("http:foo", "/foo"),  # explicit scheme
+        ("HTTP:FOO", "/FOO"),  # explicit scheme get case normalized
+        ("https:///foo", "/foo"),  # explicit scheme
+    ],
+)
+def test_cleanup_url_path(url_path, expected):
+    assert cleanup_url_path(url_path) == expected
+
+
+@pytest.mark.parametrize(
+    "url_path",
+    [
+        "file:///foo",  # bad scheme
+        "//example.net/bar",  # netloc not allowed
+    ],
+)
+def test_cleanup_url_path_raises_value_error(url_path):
+    with pytest.raises(ValueError):
+        cleanup_url_path(url_path)
 
 
 @pytest.mark.parametrize(
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 0f62384..4dee2d1 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -88,7 +88,7 @@ def test_parse_path():
     assert parse_path("/foo/") == ["foo"]
     assert parse_path("/foo/bar") == ["foo", "bar"]
     assert parse_path("/foo/bar/") == ["foo", "bar"]
-    assert parse_path("/foo/bar/../stuff") == ["foo", "bar", "stuff"]
+    assert parse_path("/foo/bar/../stuff") == ["foo", "stuff"]
 
 
 @pytest.mark.parametrize(