fix(markdown): escape referenced figures' filepaths and ensure cross-platform path handling (Windows / POSIX)

docling_core/transforms/serializer/markdown.py

@@ -5,8 +5,9 @@
 import re
 import textwrap
 from enum import Enum
-from pathlib import Path
+from pathlib import Path, PurePath
 from typing import Annotated, Any, Optional, Union
+from urllib.parse import quote, urlsplit, urlunsplit
 
 from pydantic import AnyUrl, BaseModel, Field, PositiveInt
 from tabulate import _column_type, tabulate
@@ -687,12 +688,79 @@ def _serialize_image_part(
             ):
                 text_res = image_placeholder
             else:
-                text_res = f"![Image]({item.image.uri!s})"
+                text_res = f"![Image]({self._escape_uri_path(item.image.uri)})"
         else:
             text_res = image_placeholder
 
         return create_ser_result(text=text_res, span_source=item)
 
+    @staticmethod
+    def _escape_uri_path(value: Union[AnyUrl, PurePath]) -> str:
+        """Encode a URL or filesystem path as a Markdown link destination.
+
+        Handles URLs of any scheme (https/s3/ftp/...) as well as POSIX and Windows
+        paths, keeps relative paths relative, and never double-encodes. A Windows path
+        is recognized by either flavour, so a document authored on Windows still
+        resolves when it is exported on POSIX.
+
+        The only destination this gives a ``file://`` scheme to is an absolute Windows
+        path, where it is the sole spelling a renderer cannot misread as a URL scheme
+        (``C:``) or as an authority (``//server``). A URL that already carries the
+        scheme is passed through, since dropping it would turn an absolute filesystem
+        reference into a root-relative URL.
+
+        Known limitation: behavior is unknown if value is a POSIX filename, containing
+        an actual backslash in the filename.
+
+        Args:
+            value: The URL or path to encode.
+
+        Returns:
+            A percent-encoded Markdown link destination.
+        """
+
+        # Characters that survive percent-encoding in a link destination.
+        # The RFC 3986 reserved characters that carry meaning in a URI, plus ``%`` so that an
+        # already-encoded destination is not encoded a second time. Whitespace and parentheses
+        # are deliberately absent: they would end a Markdown inline link.
+        _URI_KEEP_CHARS: str = "/%:@+,;=~$!&'*"
+
+        # Matches the drive prefix of an absolute Windows path, e.g. ``C:/``.
+        _WINDOWS_DRIVE_RE: re.Pattern = re.compile(r"[A-Za-z]:/")
+
+        keep = _URI_KEEP_CHARS
+        # A backslash is both the Windows separator and a Markdown escape character, and
+        # is read as a separator whatever flavour the path arrives in: a document authored
+        # on Windows keeps its backslashes once it is re-read on POSIX, where the flavour
+        # can no longer tell. A URL is unaffected, as pydantic normalizes backslashes away
+        # when parsing.
+        s = str(value).replace("\\", "/")
+
+        if s.startswith("//"):  # In case of a fileshare (//someserver/somefolder)
+            host, _, tail = s.lstrip("/").partition("/")  # get the end of the path
+            # file://<host>/<path>, with <host> being a possibly empty string.
+            return urlunsplit(("file", host, quote(f"/{tail}", safe=keep), "", ""))
+        if _WINDOWS_DRIVE_RE.match(s):  # In case of a Windows filename with drive letter
+            # file://<full_path_with_filename>
+            return urlunsplit(("file", "", quote(f"/{s}", safe=keep), "", ""))
+
+        # A URL keeps its scheme, authority and delimiters; only its components are
+        # encoded. A single-character scheme cannot be real, so it is read as a path.
+        parts = urlsplit(s)
+        if len(parts.scheme) > 1:
+            return urlunsplit(
+                (
+                    parts.scheme,
+                    parts.netloc,
+                    quote(parts.path, safe=keep),
+                    quote(parts.query, safe=keep + "="),
+                    quote(parts.fragment, safe=keep),
+                )
+            )
+
+        # A relative or root-relative local path.
+        return quote(s, safe=keep)
+
 
 class MarkdownKeyValueSerializer(BaseKeyValueSerializer):
     """Markdown-specific key-value item serializer."""

test/test_serialization.py

@@ -1,11 +1,13 @@
 """Test serialization."""
 
 import threading
-from pathlib import Path
+from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
+from typing import Union
 from unittest.mock import MagicMock, patch
 from xml.etree import ElementTree as ET
 
 import pytest
+from pydantic import AnyUrl
 
 from docling_core.transforms.serializer.common import _DEFAULT_LABELS
 from docling_core.transforms.serializer.html import (
@@ -18,13 +20,14 @@
 from docling_core.transforms.serializer.markdown import (
     MarkdownDocSerializer,
     MarkdownParams,
+    MarkdownPictureSerializer,
     MarkdownTableSerializer,
     OrigListItemMarkerMode,
     _cell_content_has_table,
 )
 from docling_core.transforms.serializer.webvtt import WebVTTDocSerializer, WebVTTParams
 from docling_core.transforms.visualizer.layout_visualizer import LayoutVisualizer
-from docling_core.types.doc import DoclingDocument
+from docling_core.types.doc import DoclingDocument, ImageRef, Size
 from docling_core.types.doc.base import ImageRefMode
 from docling_core.types.doc.document import (
     BaseMeta,
@@ -1103,3 +1106,149 @@ def test_html_meta_emits_xhtml_compatible_attributes():
     assert 'data-meta-name="entities"' in html_out
     # Output must be parseable by a strict XML parser.
     ET.fromstring(html_out)
+
+
+# A link destination to encode, paired with its expected encoding.
+_EscapeCase = tuple[Union[AnyUrl, PurePath], str]
+
+_ESCAPE_PATH_CASES: list[_EscapeCase] = [
+    # A relative path with nothing to encode: the hash-based names generated by
+    # `_with_pictures_refs` must pass through untouched.
+    (PurePosixPath("doc_artifacts/image_000001_ab12.png"), "doc_artifacts/image_000001_ab12.png"),
+    # A relative artifacts dir is derived from the output filename, so it can hold
+    # spaces. An unencoded space ends the link destination.
+    (PurePosixPath("My Report_artifacts/img.png"), "My%20Report_artifacts/img.png"),
+    # Parentheses are only valid in a destination while balanced; encoding them keeps
+    # an odd one in a filename from ending the link early.
+    (PurePosixPath("artifacts/img (1).png"), "artifacts/img%20%281%29.png"),
+    # In a path these are literal characters, not URI delimiters. "%" is kept as-is so
+    # that an already-encoded URI is not encoded twice.
+    (PurePosixPath("100%_scale/a#b?c.png"), "100%_scale/a%23b%3Fc.png"),
+    # A root-relative POSIX path stays root-relative.
+    (PurePosixPath("/home/a b/img.png"), "/home/a%20b/img.png"),
+    # Backslash separators must become forward slashes: "\" is an escape character in
+    # Markdown, so "dir\img.png" would render as "dirimg.png".
+    (PureWindowsPath("My Report_artifacts/img.png"), "My%20Report_artifacts/img.png"),
+    # An absolute Windows path becomes an RFC 8089 file:// URL, so that "C:" cannot be
+    # read as a URL scheme.
+    (PureWindowsPath("C:/Users/me/My Docs/img.png"), "file:///C:/Users/me/My%20Docs/img.png"),
+    # A UNC share becomes the authority of the file:// URL, so that the leading "//"
+    # cannot be read as a scheme-relative URL.
+    (PureWindowsPath("//server/share/My Docs/img.png"), "file://server/share/My%20Docs/img.png"),
+]
+
+_ESCAPE_URL_CASES: list[_EscapeCase] = [
+    # A URL keeps its scheme, authority, and delimiters; only the components get
+    # encoded. Escapes pydantic already applied must not become "%2520".
+    (AnyUrl("file:///home/a b/img.png"), "file:///home/a%20b/img.png"),
+    (AnyUrl("s3://bucket/My Report_artifacts/img.png"), "s3://bucket/My%20Report_artifacts/img.png"),
+    (AnyUrl("https://example.com:8080/a b.png?w=1&h=2#frag"), "https://example.com:8080/a%20b.png?w=1&h=2#frag"),
+    # pydantic leaves parentheses as-is, so the encoding still has work to do here.
+    (AnyUrl("https://example.com/img (1).png"), "https://example.com/img%20%281%29.png"),
+]
+
+
+def _as_destination(dest: str) -> Union[AnyUrl, PurePath]:
+    """Re-read an encoded destination the way a caller would supply it."""
+    return AnyUrl(dest) if "://" in dest else PurePosixPath(dest)
+
+
+@pytest.mark.parametrize(("value", "expected"), _ESCAPE_PATH_CASES + _ESCAPE_URL_CASES)
+def test_escape_uri_path(value: Union[AnyUrl, PurePath], expected: str):
+    """Test encoding of link destinations for both URLs and local paths."""
+    assert MarkdownPictureSerializer._escape_uri_path(value) == expected
+
+
+@pytest.mark.parametrize(("value", "expected"), _ESCAPE_PATH_CASES + _ESCAPE_URL_CASES)
+def test_escape_uri_path_is_idempotent(value: Union[AnyUrl, PurePath], expected: str):
+    """Test that re-encoding an encoded destination is a no-op (no double-encoding)."""
+    assert MarkdownPictureSerializer._escape_uri_path(_as_destination(expected)) == expected
+
+
+@pytest.mark.parametrize(
+    "posix_spelling",
+    [
+        "doc_artifacts/image_000001_ab12.png",
+        "My Report_artifacts/img.png",
+        "C:/Users/me/My Docs/img.png",
+        "//server/share/My Docs/img.png",
+    ],
+)
+def test_escape_uri_path_is_flavour_independent(posix_spelling: str):
+    """Test that a path encodes the same way however its separators arrive.
+
+    A document authored on Windows is re-read as a POSIX path on another host, and PRs
+    #641 and #663 make `_with_pictures_refs` store the POSIX spelling on Windows. Neither
+    may change the destination.
+    """
+    escape = MarkdownPictureSerializer._escape_uri_path
+    windows = PureWindowsPath(posix_spelling)
+    assert "\\" in str(windows)  # guard: the fixture really has separators to convert
+
+    assert escape(PurePosixPath(posix_spelling)) == escape(windows)
+    assert escape(PurePosixPath(str(windows))) == escape(windows)
+
+
+def test_escape_uri_path_file_scheme_is_only_for_absolute_windows_paths():
+    """Test that only an absolute Windows path is given a file:// scheme."""
+    escape = MarkdownPictureSerializer._escape_uri_path
+
+    # A drive letter and a UNC host are the two absolute Windows forms.
+    assert escape(PureWindowsPath("C:/Users/me/img.png")) == "file:///C:/Users/me/img.png"
+    assert escape(PureWindowsPath("//server/share/img.png")) == "file://server/share/img.png"
+
+    # Nothing else acquires the scheme, a root-relative POSIX path included.
+    for value in (
+        PurePosixPath("artifacts/img.png"),
+        PurePosixPath("/home/me/img.png"),
+        PureWindowsPath("artifacts/img.png"),
+        AnyUrl("https://example.com/img.png"),
+        AnyUrl("s3://bucket/img.png"),
+    ):
+        assert not escape(value).startswith("file:")
+
+    # A URL that already carries the scheme keeps it: dropping it would turn an absolute
+    # filesystem reference into a root-relative URL.
+    assert escape(AnyUrl("file:///home/me/img.png")) == "file:///home/me/img.png"
+
+
+@pytest.mark.parametrize(
+    ("uri", "expected"),
+    [
+        (Path("doc_artifacts/image_000001_ab12.png"), "doc_artifacts/image_000001_ab12.png"),
+        (Path("My Report (final)_artifacts/img.png"), "My%20Report%20%28final%29_artifacts/img.png"),
+        # A Windows-authored separator resolves the same way on either exporting host.
+        (Path("My Report_artifacts\\img.png"), "My%20Report_artifacts/img.png"),
+        # An AnyUrl was already percent-encoded by pydantic on parse: its escapes must
+        # not be encoded a second time, nor its scheme mangled into "file%3A".
+        (AnyUrl("file:///home/a b/img.png"), "file:///home/a%20b/img.png"),
+        # `_save_image_and_resolve_uri` returns an AnyUrl for remote artifact dirs.
+        (AnyUrl("s3://bucket/My Report_artifacts/img.png"), "s3://bucket/My%20Report_artifacts/img.png"),
+        # Query delimiters must stay functional.
+        (AnyUrl("https://example.com/img.png?w=1&h=2"), "https://example.com/img.png?w=1&h=2"),
+    ],
+)
+def test_referenced_image_uri_is_encoded(uri, expected: str):
+    """Test that `_serialize_image_part` encodes the URI it emits."""
+    doc = DoclingDocument(name="x")
+    doc.add_picture(image=ImageRef(mimetype="image/png", dpi=72, size=Size(width=10, height=10), uri=uri))
+
+    assert doc.export_to_markdown(image_mode=ImageRefMode.REFERENCED) == f"![Image]({expected})"
+
+
+def test_referenced_image_uri_encoding_only_applies_to_referenced_mode():
+    """Test that PLACEHOLDER mode still emits the placeholder, not an encoded URI."""
+    doc = DoclingDocument(name="x")
+    uri = Path("My Report_artifacts/img.png")
+    doc.add_picture(image=ImageRef(mimetype="image/png", dpi=72, size=Size(width=10, height=10), uri=uri))
+
+    assert doc.export_to_markdown(image_mode=ImageRefMode.PLACEHOLDER) == "<!-- image -->"
+
+
+def test_referenced_image_data_uri_is_not_encoded():
+    """Test that a data URI still falls back to the placeholder in REFERENCED mode."""
+    doc = DoclingDocument(name="x")
+    uri = AnyUrl("data:image/png;base64,iVBORw0KGgo=")
+    doc.add_picture(image=ImageRef(mimetype="image/png", dpi=72, size=Size(width=10, height=10), uri=uri))
+
+    assert doc.export_to_markdown(image_mode=ImageRefMode.REFERENCED) == "<!-- image -->"