diff --git a/CHANGELOG.md b/CHANGELOG.md index a1b3c4c..648c2aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix crash in `ArticleUrlRewriter.get_document_uri` when the document or a linked item has a `..` (or `/`) segment in its querystring ; querystrings are part of the ZIM entry name and must not be interpreted as navigable path segments (openzim/warc2zim#380) + ## [5.4.0] - 2026-05-28 ### Added diff --git a/src/zimscraperlib/rewriting/url_rewriting.py b/src/zimscraperlib/rewriting/url_rewriting.py index 337fc2c..ec6772d 100644 --- a/src/zimscraperlib/rewriting/url_rewriting.py +++ b/src/zimscraperlib/rewriting/url_rewriting.py @@ -311,25 +311,33 @@ def get_document_uri(self, item_path: ZimPath, item_fragment: str) -> str: """ item_parts = urlsplit(item_path.value) - - # item_path is both path + querystring, both will be url-encoded in the document - # so that readers consider them as a whole and properly pass them to libzim - item_url = item_parts.path - if item_parts.query: - item_url += "?" + item_parts.query + article_parts = urlsplit(self.article_path.value) + + # The relative path is computed using only the path components. A querystring is + # part of the ZIM entry name (a leaf), it is not a navigable directory, so its + # content (which may contain `/` or even `..` segments) must not be interpreted + # as path navigation when computing the relative path ; doing so crashes on `..` + # segments (see https://github.com/openzim/warc2zim/issues/380). The querystring + # is re-appended and url-encoded together with the path below so that readers + # consider path + querystring as a single whole entry. relative_path = str( - PurePosixPath(item_url).relative_to( + PurePosixPath(item_parts.path).relative_to( ( - PurePosixPath(self.article_path.value) - if self.article_path.value.endswith("/") - else PurePosixPath(self.article_path.value).parent + PurePosixPath(article_parts.path) + if article_parts.path.endswith("/") + else PurePosixPath(article_parts.path).parent ), walk_up=True, ) ) - # relative_to removes a potential last '/' in the path, we add it back - if item_path.value.endswith("/"): + # relative_to removes a potential last '/' in the path, we add it back before + # appending the querystring + if item_parts.path.endswith("/"): relative_path += "/" + # re-append the querystring (part of the ZIM entry name) now that the relative + # path has been computed + if item_parts.query: + relative_path += "?" + item_parts.query return ( f"{quote(relative_path, safe='/')}" diff --git a/tests/rewriting/test_url_rewriting.py b/tests/rewriting/test_url_rewriting.py index 8375f27..da4479c 100644 --- a/tests/rewriting/test_url_rewriting.py +++ b/tests/rewriting/test_url_rewriting.py @@ -532,6 +532,46 @@ def test_relative_url( == expected_rewrite_result ) + @pytest.mark.parametrize( + "article_url, item_url, expected_document_uri", + [ + # `..` in the querystring of the document being rewritten must not be + # interpreted as a navigable path segment (see + # https://github.com/openzim/warc2zim/issues/380 ; it used to raise + # `ValueError: '..' segment ... cannot be walked`) + pytest.param( + "http://kiwix.org/common/sub/page.html?css=../../prg", + "http://kiwix.org/_zim_static/wombat.js", + "../../_zim_static/wombat.js", + id="dotdot_in_article_querystring", + ), + # `..` in the querystring of the linked item is kept as-is in the (encoded) + # entry name and must not trigger path navigation either + pytest.param( + "http://kiwix.org/common/page.html", + "http://kiwix.org/common/other.html?css=../../prg", + "other.html%3Fcss%3D../../prg", + id="dotdot_in_item_querystring", + ), + # a `/` inside the querystring is part of the entry name, not a directory + pytest.param( + "http://kiwix.org/a/b/page.html", + "http://kiwix.org/a/b/img.png?x=1/2", + "img.png%3Fx%3D1/2", + id="slash_in_item_querystring", + ), + ], + ) + def test_get_document_uri_querystring_segments( + self, + article_url: str, + item_url: str, + expected_document_uri: str, + ): + rewriter = ArticleUrlRewriter(article_url=HttpUrl(article_url)) + item_path = ArticleUrlRewriter.normalize(HttpUrl(item_url)) + assert rewriter.get_document_uri(item_path, "") == expected_document_uri + @pytest.mark.parametrize( "article_url, original_content_url, expected_rewrite_result, know_paths, " "rewrite_all_url",