From fac62cb347aeea2d0db1060da161b2c9394820ac Mon Sep 17 00:00:00 2001 From: Yiorgis Gozadinos Date: Mon, 15 Jun 2026 09:02:51 +0300 Subject: [PATCH] Follow HTTP redirects in the WebDAV source --- CHANGELOG.md | 1 + docs/ingester.md | 9 ++ .../haiku/rag/ingester/sources/webdav.py | 77 ++++++++-- tests/ingester/test_webdav_source.py | 141 ++++++++++++++++++ 4 files changed, 215 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fabc94d..8db62f30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### Fixed +- WebDAV source follows HTTP redirects (`PROPFIND` and `GET`); front-ended servers that 301/302 on trailing-slash or virtual-host rewrites no longer error. - All in-process pdfium access (page slicing and embedded-attachment scanning) is serialized under a single shared lock. Concurrent ingester workers no longer corrupt libpdfium's global state, which previously failed valid PDFs with "Data format error". - Embedded PDF attachment extension is derived from the attachment filename, not the parent's synthetic `...#attachment=` URI; non-PDF attachments (e.g. `.joboptions`) are no longer misrouted to docling's PDF backend, and unsupported extensions are skipped. diff --git a/docs/ingester.md b/docs/ingester.md index f8d53751..b25dc09c 100644 --- a/docs/ingester.md +++ b/docs/ingester.md @@ -146,6 +146,15 @@ collection are emitted as DELETE. Fetches are plain HTTP GETs — any WebDAV server already supports them. +Redirects are followed for both `PROPFIND` and `GET`, so front-ended +servers that 30x on trailing-slash normalisation or scheme upgrades (e.g. +Plone) work without extra configuration. Discovered URIs stay anchored to +`base_url` (the `GET` fetch follows redirects to the bytes). A same-host +scheme upgrade (`http`→`https`) is transparent; a redirect that moves the +collection to a different path or host makes discovery raise so you can +point `base_url` at the new location rather than silently dropping every +file. Credentials are never replayed to a different host on a redirect. + Bearer-token auth can replace HTTP Basic via the standard `headers` map: ```yaml diff --git a/haiku_rag_slim/haiku/rag/ingester/sources/webdav.py b/haiku_rag_slim/haiku/rag/ingester/sources/webdav.py index 18282b76..4437de31 100644 --- a/haiku_rag_slim/haiku/rag/ingester/sources/webdav.py +++ b/haiku_rag_slim/haiku/rag/ingester/sources/webdav.py @@ -34,6 +34,8 @@ _TAG_GETLASTMODIFIED = f"{{{_DAV_NS}}}getlastmodified" _TAG_GETCONTENTTYPE = f"{{{_DAV_NS}}}getcontenttype" +_MAX_PROPFIND_REDIRECTS = 5 + _PROPFIND_BODY = b""" @@ -197,8 +199,18 @@ class WebDAVSource: if self.username is not None and self.password is not None else None ) + # Follow redirects: Plone and other front-ended WebDAV servers 30x on + # trailing-slash normalisation and virtual-host rewrites; httpx defaults + # follow_redirects to False, which would surface those as errors. GET + # and HEAD (fetch) follow at the client level. PROPFIND (head/discover) + # is followed by hand in `_propfind` because httpx downgrades PROPFIND to + # GET on 302/303, which the final endpoint would reject or answer with a + # non-multistatus body. self._http = httpx.AsyncClient( - auth=auth, headers=self.headers, transport=transport + auth=auth, + headers=self.headers, + transport=transport, + follow_redirects=True, ) def supports(self, uri: str) -> bool: @@ -207,13 +219,43 @@ class WebDAVSource: async def aclose(self) -> None: await self._http.aclose() - async def head(self, uri: str) -> str | None: - response = await self._http.request( - "PROPFIND", - uri, - headers={"Depth": "0", "Content-Type": "application/xml"}, - content=_PROPFIND_BODY, + async def _propfind(self, url: str, *, depth: str) -> tuple[httpx.Response, str]: + """Issue a PROPFIND, following redirects by hand so the method and body + are preserved (httpx turns a redirected PROPFIND into a GET on 302/303). + Returns the final response and the URL it was served from. + + Each hop re-issues through the client, which re-applies `auth`, so a + cross-host redirect is refused *before* the request is sent — otherwise + we would leak Basic credentials to the redirect target (httpx only + strips auth cross-host for its own auto-followed redirects).""" + host = urlparse(url).netloc + current = url + for _ in range(_MAX_PROPFIND_REDIRECTS): + response = await self._http.request( + "PROPFIND", + current, + headers={"Depth": depth, "Content-Type": "application/xml"}, + content=_PROPFIND_BODY, + follow_redirects=False, + ) + location = response.headers.get("location") + if response.is_redirect and location: + current = urljoin(current, location) + if urlparse(current).netloc != host: + raise ValueError( + f"WebDAV PROPFIND {url!r} redirected to a different host " + f"{current!r}; refusing to send credentials. Update " + f"base_url to the new host." + ) + continue + return response, current + raise httpx.TooManyRedirects( + f"Exceeded {_MAX_PROPFIND_REDIRECTS} redirects for PROPFIND {url}", + request=response.request, ) + + async def head(self, uri: str) -> str | None: + response, _ = await self._propfind(uri, depth="0") if response.is_error: return None entries = _parse_multistatus(response.content) @@ -267,13 +309,22 @@ class WebDAVSource: now = datetime.now(UTC) seen: set[str] = set() - response = await self._http.request( - "PROPFIND", - self.base_url, - headers={"Depth": "infinity", "Content-Type": "application/xml"}, - content=_PROPFIND_BODY, - ) + response, final_url = await self._propfind(self.base_url, depth="infinity") response.raise_for_status() + # hrefs are resolved and filtered against base_url, and the worker + # resolves URIs back to this source by base_url too. A redirect that + # moves the collection to a different path would make every href fall + # outside base_url — silently emitting DELETEs for all known docs. Fail + # loudly instead so the operator points base_url at the new location. + # (_propfind already rejects cross-host redirects, so only the path can + # differ here; a same-path scheme upgrade stays transparent.) + if urlparse(final_url).path.rstrip("/") != urlparse(self.base_url).path.rstrip( + "/" + ): + raise ValueError( + f"WebDAV collection {self.base_url!r} redirected to a different " + f"path {final_url!r}; update base_url to the new location." + ) entries = _parse_multistatus(response.content) for entry in entries: diff --git a/tests/ingester/test_webdav_source.py b/tests/ingester/test_webdav_source.py index ce4fcf44..060ddb06 100644 --- a/tests/ingester/test_webdav_source.py +++ b/tests/ingester/test_webdav_source.py @@ -434,6 +434,147 @@ async def test_discover_emits_upsert_for_unknown_uri_without_revision(): assert non_delete[0].kind is SourceEventKind.UPSERT +@pytest.mark.asyncio +async def test_fetch_follows_redirect(): + """Plone commonly 301s (trailing-slash normalisation, VHM rewrites); the + client must follow to fetch the real bytes instead of returning the 3xx.""" + body = b"redirected body" + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/dav/a.md": + return httpx.Response( + 301, headers={"location": "https://nc.example.com/dav/final.md"} + ) + assert request.url.path == "/dav/final.md" + return httpx.Response( + 200, + content=body, + headers={"content-type": "text/markdown", "etag": '"rev-1"'}, + ) + + src = WebDAVSource( + source_id="nc", + base_url="https://nc.example.com/dav/", + transport=_transport(handler), + ) + result = await src.fetch("https://nc.example.com/dav/a.md") + assert result.body == body + assert result.revision == "rev-1" + + +@pytest.mark.asyncio +async def test_discover_follows_redirect_preserving_propfind(): + """A 302 on PROPFIND is followed with the method preserved (httpx would + downgrade it to GET). A same-path scheme upgrade is transparent, and hrefs + stay anchored to base_url so stored URIs remain stable for the worker.""" + methods: list[str] = [] + multistatus = _multistatus( + {"href": "/dav/a.md", "etag": '"rev-a"', "content_type": "text/markdown"}, + ) + + def handler(request: httpx.Request) -> httpx.Response: + methods.append(request.method) + if request.url.scheme == "http": + return httpx.Response( + 302, headers={"location": "https://nc.example.com/dav/"} + ) + return httpx.Response(207, content=multistatus) + + src = WebDAVSource( + source_id="nc", + base_url="http://nc.example.com/dav/", + transport=_transport(handler), + ) + events = [event async for event in src.discover()] + assert methods == ["PROPFIND", "PROPFIND"] + assert [e.uri for e in events] == ["http://nc.example.com/dav/a.md"] + + +@pytest.mark.asyncio +async def test_discover_raises_when_collection_relocates(): + """If the collection root redirects to a different path, the multistatus + hrefs fall outside base_url. Resolving them against base_url would skip + every file and DELETE all known docs — fail loudly instead.""" + multistatus = _multistatus( + {"href": "/dav2/a.md", "etag": '"rev-a"', "content_type": "text/markdown"}, + ) + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/dav/": + return httpx.Response( + 301, headers={"location": "https://nc.example.com/dav2/"} + ) + return httpx.Response(207, content=multistatus) + + src = WebDAVSource( + source_id="nc", + base_url="https://nc.example.com/dav/", + transport=_transport(handler), + ) + with pytest.raises(ValueError, match="redirected to a different path"): + [event async for event in src.discover()] + + +@pytest.mark.asyncio +async def test_discover_refuses_cross_host_redirect_without_sending_credentials(): + """A PROPFIND redirected to another host must not have the configured + credentials replayed to that host (httpx only strips auth cross-host for + its own auto-followed redirects, not our manual loop).""" + seen_hosts: list[str | None] = [] + + def handler(request: httpx.Request) -> httpx.Response: + seen_hosts.append(request.url.host) + return httpx.Response( + 302, headers={"location": "https://evil.example.com/dav/"} + ) + + src = WebDAVSource( + source_id="nc", + base_url="https://nc.example.com/dav/", + username="alice", + password="hunter2", + transport=_transport(handler), + ) + with pytest.raises(ValueError, match="different host"): + [event async for event in src.discover()] + assert "evil.example.com" not in seen_hosts + + +@pytest.mark.asyncio +async def test_head_follows_redirect_preserving_propfind(): + methods: list[str] = [] + + def handler(request: httpx.Request) -> httpx.Response: + methods.append(request.method) + if request.url.path == "/dav/a.md": + return httpx.Response( + 302, headers={"location": "https://nc.example.com/dav/final.md"} + ) + return httpx.Response(207, content=_multistatus({"href": "/x", "etag": '"r"'})) + + src = WebDAVSource( + source_id="nc", + base_url="https://nc.example.com/dav/", + transport=_transport(handler), + ) + assert await src.head("https://nc.example.com/dav/a.md") == "r" + assert methods == ["PROPFIND", "PROPFIND"] + + +@pytest.mark.asyncio +async def test_propfind_redirect_loop_is_bounded(): + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(302, headers={"location": "https://nc.example.com/loop/"}) + + src = WebDAVSource( + source_id="nc", + base_url="https://nc.example.com/dav/", + transport=_transport(handler), + ) + with pytest.raises(httpx.TooManyRedirects): + [event async for event in src.discover()] + + @pytest.mark.asyncio async def test_fetch_rejects_file_exceeding_max_size(): def handler(request: httpx.Request) -> httpx.Response: