Skip to content

Record the original for every rewritten URL attribute #338

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.

Sign up for GitHub

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

Draft
epheterson wants to merge 1 commit into
base: main
Choose a base branch
from
+89 −60
Draft

Record the original for every rewritten URL attribute #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
94 changes: 46 additions & 48 deletions src/zimscraperlib/rewriting/html.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,55 +54,55 @@ def get_attr_value_from(
return default


# The attribute wombat reads a script's pre-rewrite src back out of.
# The attributes wombat asks for a pre-rewrite value of.
#
# wombat already overrides Element.prototype.getAttribute and, for a <script>,
# checks this attribute before anything else (retrieveWBOSRC). It exists for
# exactly this situation and nothing was filling it in.
WB_ORIG_SRC_ATTRIBUTE = "__wb_orig_src"
# wombat overrides Element.prototype.getAttribute, and for any attribute it
# considers a URL it first looks for the original the server-side rewriter
# recorded (retrieveWBOSRC), falling back to extractOriginalURL. That fallback
# can only reverse rewrites WOMBAT performed; a value rewritten here, at scrape
# time, is unrecognisable to it, and it ends up prepending the original scheme
# to an already-relative path.
#
# These are the attributes wombat treats as a URL on every tag, and so the ones
# it will come asking about.
WB_ORIG_ATTRIBUTE_PREFIX = "__wb_orig_"
WB_ORIG_ATTRIBUTES = ("src", "href", "xlink:href")


def get_script_original_src(
tag: str, attrs: AttrsList, rewritten_attrs: list[AttrNameAndValue]
) -> str | None:
"""The src a <script> had before rewriting, when rewriting changed it.

Bundlers identify their chunks by the raw ``getAttribute("src")``, not by
the ``.src`` property, and they expect the string the server sent. Rewriting
an absolute path to a relative one therefore breaks them silently: Next.js
with Turbopack strips a leading ``/_next/`` to get a chunk's name, the strip
no longer matches, every chunk registers under a name nothing is waiting
for, and the page never hydrates. Vite is reported to be affected the same
way.

wombat cannot repair this on its own. Its getAttribute override un-rewrites
URLs that WOMBAT rewrote, and this one was rewritten here, at scrape time —
so extractOriginalURL sees no prefix it knows and hands the value straight
back. What it does have is ``__wb_orig_src``, which it checks first for
script elements and which only the rewriter can fill in.

None when the tag is not a script, when it had no src, when rewriting left
the src alone, or when the tag already carries the attribute — there is
nothing to remember in those cases, and an attribute that merely repeats
the src is noise in every captured page.

The already-carries case is HTML being rewritten a second time. What is
recorded then is the src of the FIRST pass, which is already a rewritten
value; keeping the attribute that is there preserves the true original, and
appending a second one would both duplicate the attribute and let the stale
value win, since a browser takes the first.
def wb_orig_attribute(attr_name: str) -> str:
"""The attribute wombat reads ``attr_name``'s pre-rewrite value out of."""
return WB_ORIG_ATTRIBUTE_PREFIX + attr_name


def get_original_attr_values(
attrs: AttrsList, rewritten_attrs: list[AttrNameAndValue]
) -> list[AttrNameAndValue]:
"""The pre-rewrite values worth recording, as (attribute, value) pairs.

Two symptoms, one cause. A bundler identifies its chunks by the raw
``getAttribute("src")`` and expects the string the server sent, so a
rewritten src stops matching and the page never hydrates. And a rewritten
href comes back with the original scheme glued onto a relative path
(openzim/warc2zim#413).

Nothing is recorded for an attribute that had no value, that rewriting left
alone, or that already carries its original. That last is HTML rewritten
twice: the attribute already present is the true original, and a second
would both duplicate it and win, since a browser takes the first.
"""
if tag != "script":
return None
if get_attr_value_from(attrs, WB_ORIG_SRC_ATTRIBUTE) is not None:
return None
original = get_attr_value_from(attrs, "src")
if not original:
return None
rewritten = get_attr_value_from(rewritten_attrs, "src")
if rewritten is None or rewritten == original:
return None
return original
recorded: list[AttrNameAndValue] = []
for attr_name in WB_ORIG_ATTRIBUTES:
orig_name = wb_orig_attribute(attr_name)
if get_attr_value_from(attrs, orig_name) is not None:
continue
original = get_attr_value_from(attrs, attr_name)
if not original:
continue
rewritten = get_attr_value_from(rewritten_attrs, attr_name)
if rewritten is None or rewritten == original:
continue
recorded.append((orig_name, original))
return recorded


def format_attr(name: str, value: str | None) -> str:
Expand Down Expand Up @@ -263,9 +263,7 @@ def handle_starttag(self, tag: str, attrs: AttrsList, *, auto_close: bool = Fals
tag=tag, attr_name=attr_name, attr_value=attr_value, attrs=attrs
)
]
original_src = get_script_original_src(tag, attrs, rewritten_attrs)
if original_src is not None:
rewritten_attrs.append((WB_ORIG_SRC_ATTRIBUTE, original_src))
rewritten_attrs.extend(get_original_attr_values(attrs, rewritten_attrs))
self.send(" ".join(format_attr(*attr) for attr in rewritten_attrs))

if auto_close:
Expand Down
55 changes: 43 additions & 12 deletions tests/rewriting/test_html_rewriting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,17 @@ def test_escaped_content(escaped_content: ContentForTests):
'<link rel="preload" src="https://cdnjs.cloudflare.com/jquery.min.js"'
' integrity="sha512-3gJwYpMe3QewGELv8k/BX9vcqhryRdzRMxVfq6ngyWXwo03GFEzjsUm'
'8Q7RZcHPHksttq7/GFoxjCVUjkjvPdw=="></link>',
'<link rel="preload" src="../cdnjs.cloudflare.com/jquery.min.js"></link>',
'<link rel="preload" src="../cdnjs.cloudflare.com/jquery.min.js"'
' __wb_orig_src="https://cdnjs.cloudflare.com/jquery.min.js"></link>',
),
ContentForTests(
'<link rel="preload" as="script"'
'src="https://cdnjs.cloudflare.com/jquery.min.js"'
' integrity="sha512-3gJwYpMe3QewGELv8k/BX9vcqhryRdzRMxVfq6ngyWXwo03GFEzjsUm'
'8Q7RZcHPHksttq7/GFoxjCVUjkjvPdw=="></link>',
'<link rel="preload" as="script" '
'src="../cdnjs.cloudflare.com/jquery.min.js"></link>',
'src="../cdnjs.cloudflare.com/jquery.min.js"'
' __wb_orig_src="https://cdnjs.cloudflare.com/jquery.min.js"></link>',
),
]
)
Expand All @@ -261,8 +263,16 @@ def test_js_rewrites(js_rewrites: ContentForTests):
assert transformed == js_rewrites.expected_str


LONG_PATH = "http://exemple.com/a/long/path"


def long_path_replace_test_content(input_: str, rewriten_url: str, article_url: str):
expected = input_.replace("http://exemple.com/a/long/path", rewriten_url)
expected = input_.replace(LONG_PATH, rewriten_url)
if rewriten_url != LONG_PATH:
# A rewritten href records the value the server sent, for wombat.
expected = expected.replace(
'">A link', f'" __wb_orig_href="{LONG_PATH}">A link'
)
return ContentForTests(input_, expected, article_url)


Expand Down Expand Up @@ -394,12 +404,12 @@ def test_rewrite_attributes():

assert (
rewriter.rewrite("<a href='https://kiwix.org/foo'>A link</a>").content
== '<a href="foo">A link</a>'
== '<a href="foo" __wb_orig_href="https://kiwix.org/foo">A link</a>'
)

assert (
rewriter.rewrite("<img src='https://kiwix.org/foo'></img>").content
== '<img src="foo"></img>'
== '<img src="foo" __wb_orig_src="https://kiwix.org/foo"></img>'
)

assert (
Expand Down Expand Up @@ -676,7 +686,8 @@ def test_extract_base_href(html_content: str, expected_base_href: str):
ContentForTests(
'<html><head><base href="../"></head>'
'<body><a href="foo.html"></a></body></html>',
'<html><head></head><body><a href="../foo.html"></a></body></html>',
"<html><head></head><body>"
'<a href="../foo.html" __wb_orig_href="foo.html"></a></body></html>',
"kiwix.org/a/index.html",
),
ContentForTests(
Expand Down Expand Up @@ -727,8 +738,8 @@ def test_extract_base_href(html_content: str, expected_base_href: str):
ContentForTests(
'<html><head> <link rel="shortcut icon" href="favicon.ico">'
'<base href="../"></head><body></body></html>',
'<html><head> <link rel="shortcut icon" href="../favicon.ico">'
"</head><body></body></html>",
'<html><head> <link rel="shortcut icon" href="../favicon.ico"'
' __wb_orig_href="favicon.ico"></head><body></body></html>',
"kiwix.org/a/index.html",
),
]
Expand Down Expand Up @@ -790,7 +801,8 @@ def test_rewrite_base_href(rewrite_base_href_content: ContentForTests):
),
pytest.param(
"""<img src="image.png?param1=value1&param2=value2">""",
"""<img src="image.png%3Fparam1=value1%26param2=value2">""",
'<img src="image.png%3Fparam1=value1%26param2=value2"'
' __wb_orig_src="image.png?param1=value1&amp;param2=value2">',
id="badly_escaped_src",
),
],
Expand Down Expand Up @@ -1621,8 +1633,25 @@ def test_rewrite_meta_http_equiv_redirect_rule(
),
pytest.param(
'<img src="/img/a.png">',
'<img src="../img/a.png">',
id="only_scripts_get_it",
'<img src="../img/a.png" __wb_orig_src="/img/a.png">',
id="an_image_gets_one_too",
),
pytest.param(
'<a href="/page?ref=noted.lol">z</a>',
'<a href="https://kiwix.org/page?ref=noted.lol"'
' __wb_orig_href="/page?ref=noted.lol">z</a>',
id="a_link_gets_one_too",
),
pytest.param(
'<a href="#anchor">x</a>',
'<a href="#anchor">x</a>',
id="an_unchanged_href_adds_nothing",
),
pytest.param(
'<a href="/b.html" __wb_orig_href="/original/b.html">x</a>',
'<a href="https://kiwix.org/b.html"'
' __wb_orig_href="/original/b.html">x</a>',
id="rewriting_twice_keeps_the_first_original_for_any_attribute",
),
pytest.param(
'<script src="/_next/a.js" __wb_orig_src="/original/a.js"></script>',
Expand All @@ -1631,7 +1660,9 @@ def test_rewrite_meta_http_equiv_redirect_rule(
),
],
)
def test_script_keeps_its_original_src(input_str: str, expected_str: str):
def test_rewritten_url_attributes_keep_their_original(
input_str: str, expected_str: str
):
"""A rewritten <script> remembers the src the server sent, for wombat.

Bundlers identify a chunk by the raw ``getAttribute("src")`` rather than by
Expand Down