Merge pull request #440 from ggozad/fix/webdav-follow-redirects
Follow HTTP redirects in the WebDAV source
This commit is contained in:
Yiorgis Gozadinos
GitHub
commit
6440698b80
4 changed files with 215 additions and 13 deletions
|
|
@ -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=<name>` URI; non-PDF attachments (e.g. `.joboptions`) are no longer misrouted to docling's PDF backend, and unsupported extensions are skipped.
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -34,6 +34,8 @@ _TAG_GETLASTMODIFIED = f"{{{_DAV_NS}}}getlastmodified"
|
|||
_TAG_GETCONTENTTYPE = f"{{{_DAV_NS}}}getcontenttype"
|
||||
|
||||
|
||||
_MAX_PROPFIND_REDIRECTS = 5
|
||||
|
||||
_PROPFIND_BODY = b"""<?xml version="1.0" encoding="utf-8"?>
|
||||
<propfind xmlns="DAV:">
|
||||
<prop>
|
||||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Reference in a new issue