Fix crash rewriting relative document URI on ..// in querystring - #314
Fix crash rewriting relative document URI on ..// in querystring#314goosfrabba wants to merge 1 commit into
Conversation
Sorry, something went wrong.
benoit74
left a comment
There was a problem hiding this comment.
Thank you for the valuable PR.
Two things to fix to move PR forward:
- you've fixed URL rewriting in Python which is good ; we need to have the same logic and fixes applied to URL rewriting in Javascript (see )
- your comments currently explain why you've done these changes ; they should be meant to help future maintainer which barely know about this PR ; for instance "it used to raide ValueError ..." is quite verbose and I don't feel like it help future maintainer, which probably does not care what the code used to misbehave at some point in the past.
Sorry, something went wrong.
benoit74
left a comment
There was a problem hiding this comment.
Reading this twice, the fix is significantly wrong.
If current page is http://kiwix.org/common/sub/page.html?css=../../prg, then its URL will in fact be http://kiwix.org/common/sub/page.html%3Fcss=../../prg to remove the query-string so that it does not causes issues with real query parameters in some URLs in some readers.
The problem is that while this convention allows to replace query-strings by paths, we end-up with a /../ in this item path, which will be interpreted legitimately as a relative path instruction and is the root-cause of the issue at hand.
Not transforming the query-string is wrong because it will cause issues in many readers.
Ignoring path segments in query-string is wrong because they are not query-strings anymore in the ZIM, but path segments.
@goosfrabba do you intend to work on this soon or should I take over?
Sorry, something went wrong.
|
I just rebased the branch to accomodate other changes which occured in parallel |
Sorry, something went wrong.
`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>
|
Thanks @benoit74, and sorry for the slow reply. You're right — I fixed the crash symptom but missed the root cause: once ? becomes %3F, the querystring is part of the ZIM entry path, so its / and .. are real path segments, not query data. Treating it as an opaque leaf was the wrong model and would break real readers as you said. You understand this corner far better than my patch did — please do take it over; I don't want to hold up the correct fix with another partial attempt. Happy to close this PR in favor of yours, or leave the branch for you to build on, whichever is easier for you. Thanks for the careful review. |
Sorry, something went wrong.
|
@goosfrabba thank you for the kind and probably honest reply. I'll take over once we've better understood what we wanna do with these special sequences in ZIM path (maybe we will simply have to forbid them). You can follow openzim/overview#90 if you are interested in the matter. Not an easy issue at all anyway |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
What
ArticleUrlRewriter.get_document_uri crashes 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_uri built the string path + "?" + querystring and handed it to PurePosixPath. PurePosixPath splits on / and treats .. as directory navigation, so any / or .. inside a querystring was interpreted as path structure. PurePosixPath.relative_to(..., walk_up=True) then raises ValueError as 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_segments covering .. 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.14 check + format clean.
Disclosure: implemented with the help of an AI coding assistant (Claude); verified locally with the included test.
Fixes #321