From 53b6dcc1c460187bfa6d664d25ea14e33f9bbdd0 Mon Sep 17 00:00:00 2001 From: Roland Date: Thu, 8 Oct 2026 19:59:23 -0600 Subject: [PATCH] Scrape: resolve relative links against the page's directory resolve_url appended a relative href to the page URL, so a link to "a.txt" on ".../dir/page.html" became ".../dir/page.html/a.txt" and the file download 404'd. Use urljoin, and resolve against the final URL after redirects so a directory URL without a trailing slash still works. Co-Authored-By: Claude Opus 5.5 --- wrolpi/files/test/test_scrape_downloader.py | 34 +++++++++++++++++++-- wrolpi/scrape_downloader.py | 32 ++++++++----------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/wrolpi/files/test/test_scrape_downloader.py b/wrolpi/files/test/test_scrape_downloader.py index c37d789a..3c434009 100644 --- a/wrolpi/files/test/test_scrape_downloader.py +++ b/wrolpi/files/test/test_scrape_downloader.py @@ -11,6 +11,7 @@ from wrolpi.scrape_downloader import ( ExecutedScrape, PreparedScrape, ScrapeHTMLDownloader, + resolve_url, scrape_html_downloader, ) @@ -29,7 +30,8 @@ EXAMPLE_HTTP = ''' async def fake_fetch_html(self, url: str): - return EXAMPLE_HTTP + # Servers redirect a directory URL to its trailing-slash form. + return f'{url.rstrip("/")}/', EXAMPLE_HTTP async def fake_file_do_download(*a, **kwargs): @@ -275,7 +277,7 @@ async def test_fetch_html_decodes_declared_charset(test_directory, simple_file_s (test_directory / 'page.html').write_bytes(html.encode(encoding)) host, port = simple_file_server.server_address - content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/page.html') + _, content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/page.html') assert 'Zurück' in content @@ -300,3 +302,31 @@ async def test_execute_collects_multiple_suffixes(test_directory): 'https://example.com/dir/three.pdf', 'https://example.com/dir/other.html', ]) + + +@pytest.mark.parametrize('page_url,href,expected', [ + # A link on a page resolves against the page's directory, not the page itself. + ('http://example.com/archive/archive.html', 'bc-a02.txt', 'http://example.com/archive/bc-a02.txt'), + ('http://example.com/archive/', 'bc-a02.txt', 'http://example.com/archive/bc-a02.txt'), + ('http://example.com/archive/archive.html', '../index.html', 'http://example.com/index.html'), + ('http://example.com/archive/archive.html', './a.txt', 'http://example.com/archive/a.txt'), + ('http://example.com/archive/archive.html', '/root.txt', 'http://example.com/root.txt'), + ('http://example.com/archive/archive.html', 'https://other.com/x.txt', 'https://other.com/x.txt'), + ('https://example.com/archive/archive.html', '//cdn.example.com/x.txt', 'https://cdn.example.com/x.txt'), + ('http://example.com/archive/archive.html', 'a.txt?v=1', 'http://example.com/archive/a.txt?v=1'), +]) +def test_resolve_url(page_url, href, expected): + assert resolve_url(page_url, href) == expected + + +@pytest.mark.asyncio +async def test_fetch_html_returns_redirected_url(test_directory, simple_file_server): + """Links in a directory listing resolve against the redirected URL (with trailing slash).""" + (test_directory / 'archive').mkdir() + (test_directory / 'archive' / 'a.txt').write_text('a') + host, port = simple_file_server.server_address + + final_url, content = await ScrapeHTMLDownloader.fetch_html(f'http://127.0.0.1:{port}/archive') + + assert final_url == f'http://127.0.0.1:{port}/archive/' + assert resolve_url(final_url, 'a.txt') == f'http://127.0.0.1:{port}/archive/a.txt' diff --git a/wrolpi/scrape_downloader.py b/wrolpi/scrape_downloader.py index c4d65238..3558401d 100644 --- a/wrolpi/scrape_downloader.py +++ b/wrolpi/scrape_downloader.py @@ -1,8 +1,8 @@ import pathlib from copy import copy from dataclasses import dataclass -from typing import List -from urllib.parse import urlparse +from typing import List, Tuple +from urllib.parse import urljoin from bs4.dammit import UnicodeDammit from sqlalchemy.orm import Session @@ -14,20 +14,9 @@ from wrolpi.errors import UnrecoverableDownloadError logger = logger.getChild(__name__) -def resolve_url(parent_url: str, url: str): - parsed_domain = urlparse(parent_url) - - parsed = urlparse(url) - if parsed.scheme: - # URL is complete or external. - return url - elif parsed.path.startswith('/'): - # URL is absolute from domain. - return f'{parsed_domain.scheme}://{parsed_domain.netloc}{url}' - - # Assume URL is relative to the parent URL page. - parent_url = parent_url.rstrip('/') - return f'{parent_url}/{url}' +def resolve_url(page_url: str, url: str) -> str: + """Resolve a link the way a browser does: relative to the page's directory (or absolute/external).""" + return urljoin(page_url, url) @dataclass @@ -64,13 +53,16 @@ class ScrapeHTMLDownloader(Downloader): return '' @staticmethod - async def fetch_html(url: str) -> str: + async def fetch_html(url: str) -> Tuple[str, str]: """Fetch and decode a page. A charset in the Content-Type header wins, then the page's - charset; aiohttp alone ignores and assumes UTF-8.""" + charset; aiohttp alone ignores and assumes UTF-8. + + Returns the final URL after redirects (links resolve against it) and the page content.""" async with aiohttp_get(url, timeout=60 * 5) as response: body = await response.read() known_encodings = [response.charset] if response.charset else [] - return UnicodeDammit(body, known_definite_encodings=known_encodings, is_html=True).unicode_markup + final_url = str(response.url) + return final_url, UnicodeDammit(body, known_definite_encodings=known_encodings, is_html=True).unicode_markup def prepare_download(self, session: Session, download: Download) -> PreparedScrape: """Validate settings and ensure the destination directory exists. @@ -124,7 +116,7 @@ class ScrapeHTMLDownloader(Downloader): break # Get the HTML of the URL. - content = await self.fetch_html(url) + url, content = await self.fetch_html(url) page_count += 1 if not content: logger.error(f'Failed to download url: {url}')