fix(parsers): sanitize image filenames against path traversal; skip redundant localize for local parser (#77)

Author: KylinMountain

openkb/converter.py

@@ -106,7 +106,11 @@ def convert_document(src: Path, kb_dir: Path, parser_override: str | None = None
         parser = LocalParser(doc_name=doc_name, images_dir=images_dir, source_dir=src.parent)
 
     parse_result = parser.parse(src)
-    markdown = localize_images(parse_result.markdown, parse_result.images, doc_name, images_dir)
+    if parser.name == "local":
+        # LocalParser already persisted images and produced canonical links.
+        markdown = parse_result.markdown
+    else:
+        markdown = localize_images(parse_result.markdown, parse_result.images, doc_name, images_dir)
 
     dest_md = sources_dir / f"{doc_name}.md"
     dest_md.write_text(markdown, encoding="utf-8")

openkb/images.py

@@ -229,12 +229,13 @@ def localize_images(
     images_dir.mkdir(parents=True, exist_ok=True)
     result = markdown
     for filename, data in images.items():
-        (images_dir / filename).write_bytes(data)
-        # Rewrite a bare ![alt](filename) reference to the canonical KB path.
-        # Use a replacement *function* (not a replacement string) so a filename
-        # containing regex-escape sequences (e.g. "\g<1>") can't corrupt the
-        # substitution — localize_images handles arbitrary parser-supplied names.
-        canonical = f"sources/images/{doc_name}/{filename}"
+        # Strip any directory components from parser-supplied names so a
+        # malicious/odd filename (e.g. "../x.png", "/abs/x.png") can never
+        # write outside images_dir. The markdown still references the original
+        # `filename`, so rewrite that ref to the sanitized canonical path.
+        safe_name = Path(filename).name or "image"
+        (images_dir / safe_name).write_bytes(data)
+        canonical = f"sources/images/{doc_name}/{safe_name}"
         pattern = re.compile(r"(!\[[^\]]*\]\()" + re.escape(filename) + r"(\))")
         result = pattern.sub(lambda m, c=canonical: m.group(1) + c + m.group(2), result)
     result = extract_base64_images(result, doc_name, images_dir)

tests/test_converter.py

@@ -164,3 +164,16 @@ def test_falls_back_to_local_for_unsupported_suffix(self, kb_dir):
             LP.return_value.parse.return_value = ParseResult(markdown="# md")
             convert_document(src, kb_dir)
         LP.assert_called_once()  # fell back to LocalParser
+
+    def test_local_parser_skips_redundant_localize(self, kb_dir):
+        src = kb_dir / "raw" / "notes.md"
+        src.write_text("# md", encoding="utf-8")
+        local = MagicMock()
+        local.name = "local"
+        local.supports.return_value = True
+        local.parse.return_value = ParseResult(markdown="# md final")
+        with patch("openkb.converter.get_parser", return_value=local), \
+             patch("openkb.converter.localize_images") as li:
+            result = convert_document(src, kb_dir)
+        li.assert_not_called()                      # local path skips localize_images
+        assert result.source_path.read_text(encoding="utf-8") == "# md final"

tests/test_images.py

@@ -203,3 +203,22 @@ def test_localize_images_filename_with_regex_metachars(tmp_path):
     out = localize_images(md, {weird: b"DATA"}, "doc", images_dir)
     assert f"sources/images/doc/{weird}" in out
     assert (images_dir / weird).read_bytes() == b"DATA"
+
+
+def test_localize_images_strips_path_traversal_in_filename(tmp_path):
+    images_dir = tmp_path / "wiki" / "sources" / "images" / "doc"
+    md = "![bad](../../evil.png)"
+    out = localize_images(md, {"../../evil.png": b"DATA"}, "doc", images_dir)
+    # bytes written INSIDE images_dir under the basename only — no escape
+    assert (images_dir / "evil.png").read_bytes() == b"DATA"
+    assert not (tmp_path / "evil.png").exists()
+    assert not (images_dir.parent.parent / "evil.png").exists()
+    # the original ref is rewritten to the sanitized canonical path
+    assert "sources/images/doc/evil.png" in out
+
+
+def test_localize_images_absolute_filename_stays_inside(tmp_path):
+    images_dir = tmp_path / "wiki" / "sources" / "images" / "doc"
+    out = localize_images("![x](/etc/x.png)", {"/etc/x.png": b"D"}, "doc", images_dir)
+    assert (images_dir / "x.png").read_bytes() == b"D"
+    assert "sources/images/doc/x.png" in out