Fix crash rewriting relative document URI on ..// in querystring#314
Open
goosfrabba wants to merge 1 commit into
Open
Fix crash rewriting relative document URI on ..// in querystring#314goosfrabba wants to merge 1 commit into
..// in querystring#314goosfrabba wants to merge 1 commit into
Conversation
`ArticleUrlRewriter.get_document_uri` computed the relative path between two ZIM entries by feeding `path + "?" + querystring` to `PurePosixPath`, which splits on `/` and interprets `..` segments as directory navigation. When the document being rewritten (or a linked item) had a querystring containing `..` (e.g. `xtree.html?css=../../prg`), `PurePosixPath.relative_to(..., walk_up=True)` raised `ValueError: '..' segment ... cannot be walked`, aborting the scrape. A querystring is part of the ZIM entry name (a leaf), not a navigable directory, so its content must not take part in the relative-path walk. Compute the relative path from the path components only, then re-append the querystring (url-encoded together with the path) once the relative path is known. Existing outputs are unchanged for querystrings without `/` or `..` segments. Fixes openzim/warc2zim#380 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ArticleUrlRewriter.get_document_uricrashes when the document being rewritten (or a linked item) has a querystring that contains a..segment, e.g.xtree.html?css=../../prg:This is the bug reported (and confirmed as legitimate by @benoit74) in openzim/warc2zim#380 — the rewriting code has since moved into this library, so the fix belongs here.
Why it happened
get_document_uribuilt the stringpath + "?" + querystringand handed it toPurePosixPath.PurePosixPathsplits on/and treats..as directory navigation, so any/or..inside a querystring was interpreted as path structure.PurePosixPath.relative_to(..., walk_up=True)then raisesValueErroras soon as it has to walk up through a..segment coming from the querystring.Fix
A querystring is part of the ZIM entry name (a leaf), not a navigable directory, so its content must not take part in the relative-path walk. The relative path is now computed from the path components only, and the querystring is re-appended (url-encoded together with the path) once the relative path is known.
Output is unchanged for querystrings that don't contain
/or..segments (verified by the existing test suite).Tests
Added
test_get_document_uri_querystring_segmentscovering..in the document querystring (the crash case),..in a linked item's querystring, and/in a querystring.Before the fix (source reverted, test kept):
After the fix:
Full
tests/rewriting/suite passes;ruff==0.15.14check + format clean.Disclosure: implemented with the help of an AI coding assistant (Claude); verified locally with the included test.
Fixes openzim/warc2zim#380