From c795611fb00aabb67b18e118e7d9946ebcd67fc6 Mon Sep 17 00:00:00 2001 From: benoit74 Date: Thu, 23 Jul 2026 14:29:54 +0000 Subject: [PATCH] Consider equal and comma are safe in URLs for proper Youtube JS player operation --- CHANGELOG.md | 4 +++ docs/technical_architecture.md | 2 +- javascript/src/wombatSetup.js | 6 +++- javascript/test/wombatUrlRewriting.js | 38 ++++++++++++++------ src/zimscraperlib/rewriting/url_rewriting.py | 8 +++-- src/zimscraperlib/zim/items.py | 2 +- tests/conftest.py | 2 +- tests/rewriting/test_css_rewriting.py | 2 +- tests/rewriting/test_html_rewriting.py | 2 +- tests/rewriting/test_url_rewriting.py | 28 +++++++++++---- 10 files changed, 68 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1b3c4c0..856cc47c 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 + +- Do not URL-encode `=` and `,` characters when rewriting document URIs, fixing YouTube JS player breakage (#316) + ## [5.4.0] - 2026-05-28 ### Added diff --git a/docs/technical_architecture.md b/docs/technical_architecture.md index d3b9a394..a6244c98 100644 --- a/docs/technical_architecture.md +++ b/docs/technical_architecture.md @@ -44,7 +44,7 @@ Below is an example case of the rewrite operation on an image URL found in an HT - Document original URL: `https://kiwix.org/a/article/document.html` - Document ZIM path: `kiwix.org/a/article/document.html` - Image original URL: `//xn--exmple-cva.com/a/resource/image.png?foo=bar` -- Image rewritten URL: `../../../ex%C3%A9mple.com/a/resource/image.png%3Ffoo%3Dbar` +- Image rewritten URL: `../../../ex%C3%A9mple.com/a/resource/image.png%3Ffoo=bar` - Image ZIM Path: `exémple.com/a/resource/image.png?foo=bar` ## JS Rewriting diff --git a/javascript/src/wombatSetup.js b/javascript/src/wombatSetup.js index d14761ec..9aa84ca5 100644 --- a/javascript/src/wombatSetup.js +++ b/javascript/src/wombatSetup.js @@ -194,7 +194,11 @@ export function urlRewriteFunction( // Reencode everything but '/' (we decode it afterwards for simplicity) const finalUrl = - prefix + encodeURIComponent(fuzzifiedPath).replaceAll('%2F', '/'); + prefix + + encodeURIComponent(fuzzifiedPath) + .replaceAll('%2F', '/') + .replaceAll('%3D', '=') + .replaceAll('%2C', ','); console.debug( 'urlRewriten:\n\t- current_url: ' + diff --git a/javascript/test/wombatUrlRewriting.js b/javascript/test/wombatUrlRewriting.js index e047f842..65a92486 100644 --- a/javascript/test/wombatUrlRewriting.js +++ b/javascript/test/wombatUrlRewriting.js @@ -335,7 +335,7 @@ test('contentWithEncodedQuestionMarkAndQueryParam', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/cont%3Fnt.txt%3Fquery%3Dvalue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/cont%3Fnt.txt%3Fquery=value', ); }); @@ -454,7 +454,24 @@ test('contentWithSimpleQueryString', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery%3Dvalue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery=value', + ); +}); + +test('contentWithSimplePathWithEqualAndComma', (t) => { + t.is( + urlRewriteFunction( + t.context.currentUrl, + t.context.originalHost, + t.context.originalScheme, + t.context.originalUrl, + t.context.prefix, + 'https://www.example.com/javascript/content.txt/query1=value1,param2=value2', + undefined, + undefined, + undefined, + ), + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt/query1=value1,param2=value2', ); }); @@ -471,7 +488,7 @@ test('contentWithQueryValueEqualSign', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery%3Dval%3Deue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery=val=eue', ); }); @@ -488,7 +505,7 @@ test('contentWithQueryValuePercentSign', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery%3Dval%25eue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery=val%25eue', ); }); @@ -505,7 +522,7 @@ test('contentWithQueryParamPercentSign', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fque%25ry%3Dvaleue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fque%25ry=valeue', ); }); @@ -522,7 +539,7 @@ test('contentWithQueryParamPlusSign', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fparam%3Dval%20ue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fparam=val%20ue', ); }); @@ -811,7 +828,7 @@ test('youtubeFuzzyNotEncoded', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/path1/youtube.fuzzy.replayweb.page/get_video_info%3Fvideo_id%3D123ah', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/path1/youtube.fuzzy.replayweb.page/get_video_info%3Fvideo_id=123ah', ); }); @@ -828,7 +845,7 @@ test('youtubeFuzzyEncoded', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/path1/youtube.fuzzy.replayweb.page/get_video_info%3Fvideo_id%3D12%3D3ah', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/path1/youtube.fuzzy.replayweb.page/get_video_info%3Fvideo_id=12=3ah', ); }); @@ -931,7 +948,7 @@ test('relAlreadyEncoded', (t) => { undefined, undefined, ), - 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery%3Dvalue', + 'http://library.kiwix.org/content/myzim_yyyy-mm/www.example.com/javascript/content.txt%3Fquery=value', ); }); @@ -1001,7 +1018,8 @@ test('relAnotherHostAlreadyRewrittenEmptyPath', (t) => { }); // this is an edge case where the URL has already been statically rewritten and is located -// on a different fuzzified domain name => we do not touch it at all +// on a different fuzzified domain name => we do not touch it at all (for instance it keeps +// %3D even if it should not have them) test('relAnotherFuzzifiedHostAlreadyRewritten', (t) => { t.is( urlRewriteFunction( diff --git a/src/zimscraperlib/rewriting/url_rewriting.py b/src/zimscraperlib/rewriting/url_rewriting.py index 337fc2c8..fffe5e04 100644 --- a/src/zimscraperlib/rewriting/url_rewriting.py +++ b/src/zimscraperlib/rewriting/url_rewriting.py @@ -34,8 +34,10 @@ start by resolving it into an absolute URL (based on the containing document absolute URI), applying the transformation to compute the corresponding ZIM path and we url-encode the whole ZIM path, so that readers will have one single blob to process, -url-decode and find corresponding ZIM entry. Only '/' separators are considered safe -and not url-encoded. +url-decode and find corresponding ZIM entry. Only '/', '=' and ',' are considered safe +and not url-encoded. '=' and ',' are kept unencoded because some sites (e.g. YouTube) +embed scripts that parse their own script URL and expect these characters to appear +literally in it. """ import re @@ -332,7 +334,7 @@ def get_document_uri(self, item_path: ZimPath, item_fragment: str) -> str: relative_path += "/" return ( - f"{quote(relative_path, safe='/')}" + f"{quote(relative_path, safe='/=,')}" f"{'#' + item_fragment if item_fragment else ''}" ) diff --git a/src/zimscraperlib/zim/items.py b/src/zimscraperlib/zim/items.py index 767747e3..ba18dd16 100644 --- a/src/zimscraperlib/zim/items.py +++ b/src/zimscraperlib/zim/items.py @@ -270,7 +270,7 @@ def __init__( if self.headers.get("Content-Encoding", "identity") != "identity": raise ValueError("Can't trust Content-Length for size") # non-html, non-compressed data. - self.size = int(self.headers["Content-Length"]) + self.size = int(self.headers["Content-Length"]) # pragma: no cover except Exception: # we couldn't retrieve size so we have to download resource to target, self.size = self.download_for_size( diff --git a/tests/conftest.py b/tests/conftest.py index 510ff3d8..57ec280c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -74,7 +74,7 @@ def gzip_html_url() -> str: @pytest.fixture(scope="module") def gzip_nonhtml_url() -> str: - return "http://mirror.download.kiwix.org/robots.txt" + return "http://lb.download.kiwix.org/robots.txt" def file_src(fname: str) -> pathlib.Path: diff --git a/tests/rewriting/test_css_rewriting.py b/tests/rewriting/test_css_rewriting.py index a43ce849..3d7e094b 100644 --- a/tests/rewriting/test_css_rewriting.py +++ b/tests/rewriting/test_css_rewriting.py @@ -190,7 +190,7 @@ def test_rewrite(): expected = """ /* A comment with a link : http://foo.com */ - @import url(../fonts.googleapis.com/icon%3Ffamily%3DMaterial%20Icons); + @import url(../fonts.googleapis.com/icon%3Ffamily=Material%20Icons); p, input { color: rbg(1, 2, 3); diff --git a/tests/rewriting/test_html_rewriting.py b/tests/rewriting/test_html_rewriting.py index 2650492a..32cb8773 100644 --- a/tests/rewriting/test_html_rewriting.py +++ b/tests/rewriting/test_html_rewriting.py @@ -787,7 +787,7 @@ def test_rewrite_base_href(rewrite_base_href_content: ContentForTests): ), pytest.param( """""", - """""", + """""", id="badly_escaped_src", ), ], diff --git a/tests/rewriting/test_url_rewriting.py b/tests/rewriting/test_url_rewriting.py index 8375f27d..2d9e2e77 100644 --- a/tests/rewriting/test_url_rewriting.py +++ b/tests/rewriting/test_url_rewriting.py @@ -170,7 +170,7 @@ def test_missing_zim_paths( "foo.html?foo=bar", RewriteResult( "https://kiwix.org/a/article/foo.html?foo=bar", - "foo.html%3Ffoo%3Dbar", + "foo.html%3Ffoo=bar", ZimPath("kiwix.org/a/article/foo.html?foo=bar"), ), ["kiwix.org/a/article/foo.html?foo=bar"], @@ -181,7 +181,7 @@ def test_missing_zim_paths( "foo.html?foo=b%24ar", RewriteResult( "https://kiwix.org/a/article/foo.html?foo=b%24ar", - "foo.html%3Ffoo%3Db%24ar", + "foo.html%3Ffoo=b%24ar", ZimPath("kiwix.org/a/article/foo.html?foo=b$ar"), ), ["kiwix.org/a/article/foo.html?foo=b$ar"], @@ -192,7 +192,7 @@ def test_missing_zim_paths( "foo.html?foo=b%3Far", # a query string with an encoded ? char in value RewriteResult( "https://kiwix.org/a/article/foo.html?foo=b%3Far", - "foo.html%3Ffoo%3Db%3Far", + "foo.html%3Ffoo=b%3Far", ZimPath("kiwix.org/a/article/foo.html?foo=b?ar"), ), ["kiwix.org/a/article/foo.html?foo=b?ar"], @@ -316,7 +316,7 @@ def test_missing_zim_paths( "foo?param=value", RewriteResult( "https://kiwix.org/a/article/foo?param=value", - "foo%3Fparam%3Dvalue", + "foo%3Fparam=value", ZimPath("kiwix.org/a/article/foo?param=value"), ), ["kiwix.org/a/article/foo?param=value"], @@ -327,7 +327,7 @@ def test_missing_zim_paths( "foo?param=value%2F", RewriteResult( "https://kiwix.org/a/article/foo?param=value%2F", - "foo%3Fparam%3Dvalue/", + "foo%3Fparam=value/", ZimPath("kiwix.org/a/article/foo?param=value/"), ), ["kiwix.org/a/article/foo?param=value/"], @@ -338,12 +338,26 @@ def test_missing_zim_paths( "foo?param=value%2Fend", RewriteResult( "https://kiwix.org/a/article/foo?param=value%2Fend", - "foo%3Fparam%3Dvalue/end", + "foo%3Fparam=value/end", ZimPath("kiwix.org/a/article/foo?param=value/end"), ), ["kiwix.org/a/article/foo?param=value/end"], False, ), + ( + # regression test for #316: '=' and ',' must not be encoded so that + # e.g. the YouTube JS player can parse its own script URL properly, + # while other reserved characters (e.g. '?') are still encoded + "https://kiwix.org/a/article/document.html", + "foo?k=ytembeds.base.en_US,foo.bar", + RewriteResult( + "https://kiwix.org/a/article/foo?k=ytembeds.base.en_US,foo.bar", + "foo%3Fk=ytembeds.base.en_US,foo.bar", + ZimPath("kiwix.org/a/article/foo?k=ytembeds.base.en_US,foo.bar"), + ), + ["kiwix.org/a/article/foo?k=ytembeds.base.en_US,foo.bar"], + False, + ), ( "https://kiwix.org/a/article/document.html", "foo/", @@ -393,7 +407,7 @@ def test_missing_zim_paths( "/foo.html?param=val+ue", RewriteResult( "https://kiwix.org/foo.html?param=val+ue", - "../../foo.html%3Fparam%3Dval%20ue", + "../../foo.html%3Fparam=val%20ue", ZimPath("kiwix.org/foo.html?param=val ue"), ), ["kiwix.org/foo.html?param=val ue"],