-
Notifications
You must be signed in to change notification settings - Fork 35
fix: fall back to sniffed encoding when a page omits an explicit charset - #127
Open
Smengerl wants to merge 1 commit into
Open
fix: fall back to sniffed encoding when a page omits an explicit charset #127Smengerl wants to merge 1 commit into
Smengerl wants to merge 1 commit into
Conversation
Smengerl
added a commit
to Smengerl/goosepaper-logicpuzzles
that referenced
this pull request
Aug 5, 2026
Backport of j6k4m8#127's own tests - the fix (response.apparent_encoding fallback in rss.py) was already in mainline, but its two tests had been lost somewhere along the way (likely during one of the several test_rss.py consolidation passes). Ported verbatim from PR j6k4m8#127.
This was referenced Aug 9, 2026
Smengerl
commented
Aug 9, 2026
Contributor
Author
j6k4m8
added a commit
that referenced
this pull request
Aug 11, 2026
...es (#135) ## What Absolutizes relative URLs (`<img src>`, `<source src>`, `<a href>`) in the HTML body that `readability-lxml` extracts from a fetched article, before it becomes a `Story`'s `body_html`. ## Why `readability.Document.summary()` returns the extracted article body more or less as-is from the source page's own markup. That markup can (and often does) use relative URLs - e.g. `<img src="/assets/img/foo.svg">` or a page-relative `<a href="../other-story/">`. goosepaper renders the whole newspaper - every story from every source, concatenated - as a single HTML document with one `base_url` for the entire thing (`Goosepaper.to_pdf`, which sets it to the local filesystem's `cwd`). There's no single correct base for a multi-origin document, so a relative URL that survives into `body_html` resolves against the wrong thing and silently fails. The most visible symptom is a missing image and a WeasyPrint log line like: ``` Failed to load image at 'file:///assets/img/foo.svg': ... No such file or directory ``` ## Fix Right after `doc.summary()` extracts the body, absolutize `src`/`href` on `<img>`, `<source>`, and `<a>` tags against the article's own final URL (`response.url`, i.e. after any redirects) - the one point where a single correct base is actually known, for that specific story. Already-absolute URLs, protocol-relative URLs (`//cdn.example.com/...`), and `data:` URIs are left untouched. ## Evidence Reproduced live against current, publicly reachable caranddriver.com review pages (fetched via the RSS feed, same code path this fix touches): - https://www.caranddriver.com/reviews/a73298183/2026-lexus-rx-350h-hybrid-f-sport-test/ - https://www.caranddriver.com/reviews/a73297467/2027-kia-telluride-x-pro-test/ Both extract body markup containing root-relative `src`/`href` values (site design-token icon SVGs, author profile links). Before the fix these are left relative in `body_html`; after the fix they resolve to absolute `https://www.caranddriver.com/...` URLs, and nothing relative is left behind. ## Testing - New `TestMakeUrlsAbsolute` test class (7 cases): root-relative image `src`, page-relative link `href`, already-absolute URLs left alone, `data:` URIs left alone, no synthetic `<body>` wrapper leaked into the result (bs4's `lxml` parser always wraps a bare fragment in `<html><body>`, so the implementation uses `.decode_contents()` rather than `str()` on the container), a true no-op returns the original string object unchanged, and empty/missing input handled. - Full existing test suite passes (85/85), including the `_FakeResponse` fixture update needed to give it a realistic `.url` now that `_story_from_response` reads it. ## Scope note This is deliberately independent of #134 (duplicate embedded headline) - different bug, different code path, no shared logic - so it's a separate PR rather than bundled in. ## Merge overlap note Verified by locally merging against `master` alongside my other currently open PRs. This one produces a mechanical (non-semantic) merge conflict with **#127, #134** - all touch `rss.py`/`test_rss.py` at nearby points (#134 at the exact same insertion line in `_story_from_response`, see its own description). No logic overlap in any case checked.
Smengerl
force-pushed
the
fix/rss-encoding-fallback
branch
from
August 14, 2026 20:21
bb54f26 to
78b9dcc
Compare
requests defaults undeclared text/* charsets to ISO-8859-1 per RFC 2616, mangling UTF-8 article pages that don't send a charset in Content-Type. Use response.apparent_encoding instead whenever the header didn't declare one.
Smengerl
force-pushed
the
fix/rss-encoding-fallback
branch
from
August 18, 2026 19:47
78b9dcc to
f40cf82
Compare
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
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.
Uh oh!
There was an error while loading. Please reload this page.
Summary
requestsdefaults undeclaredtext/*charsets to ISO-8859-1 per RFC 2616. When a fetched article page'sContent-Typeheader doesn't declare a charset, this mangles UTF-8 pages (accented characters etc. get decoded as the wrong encoding).response.apparent_encoding(requests' own content-sniffing) in that case, before the body is read - so_story_from_responsesees correctly decoded text.Test plan
pytest goosepaper/storyprovider/test_rss.py- 11 passed (9 existing + 2 new: an undeclared charset gets sniffed and decoded correctly; a declared charset is left untouched, even when sniffing would've suggested something different)pytest) - 80 passedMerge overlap note
Verified by locally merging every pairwise combination of my currently open PRs against
master. This one produces a mechanical (non-semantic) merge conflict with #121, #128, #135 - all touchrss.py/test_rss.pyat nearby insertion points. No logic overlap in any case checked.