fix(plugin): reject archive entry names the SDK would refuse to extract (#2757)

The host inspects a package to read manifest.yaml and requirements.txt before the
SDK extracts it. Inspection normalized entry names with only
replace('\\','/').strip('/'), so it accepted names its own extractor rejects
(absolute paths, '..' components, drive letters) and it silently kept the first
member when two normalized to the same path.

There was no traversal write, because the SDK's _extract_verified_zip validates
every member before writing and the host only reads members by ZipInfo. The bug
is the divergence: a package could pass inspection and then fail to install, and
the two rule sets could drift further as either side changes.

Inspection now applies the same rule set as the SDK, via
_normalize_archive_entry_name: reject empty/NUL names, absolute paths, any '..'
component and drive letters; reject a duplicate after normalization. '.' and
empty segments are still collapsed, so './manifest.yaml' and 'components//x.py'
keep working.

Tests: 13 cases covering each rejection, the duplicate, traversal taking
precedence over the duplicate check, and two positive cases (nested/unicode
layout, cosmetic separators).

Co-authored-by: multica-agent <[email protected]>

src/langbot/pkg/plugin/archive.py

@@ -17,6 +17,30 @@ _PLUGIN_METADATA_MAX_BYTES = 1024 * 1024
 _PLUGIN_REQUIREMENTS_MAX_ENTRIES = 1000
 
 
+def _normalize_archive_entry_name(filename: str) -> str:
+    """Return the archive entry name in portable form, or reject it.
+
+    The same rule set the SDK applies when it extracts an artifact
+    (``runtime/plugin/artifact.py``). Inspection must reject exactly what
+    extraction rejects: otherwise a package can pass inspection and then fail to
+    install, and the two layers can drift apart as either side changes.
+    """
+    if not filename or '\x00' in filename:
+        raise ValueError('Plugin archive contains an empty or NUL entry name')
+
+    portable = filename.replace('\\', '/')
+    parts = portable.split('/')
+    if portable.startswith('/') or any(part == '..' for part in parts):
+        raise ValueError(f'Plugin archive entry escapes the archive root: {filename}')
+    if len(parts[0]) >= 2 and parts[0][1] == ':':
+        raise ValueError(f'Plugin archive entry uses a drive letter: {filename}')
+
+    normalized = '/'.join(part for part in parts if part not in ('', '.'))
+    if not normalized:
+        raise ValueError(f'Plugin archive contains an empty entry path: {filename}')
+    return normalized
+
+
 class ArchiveCertificateState(str, Enum):
     """Syntactic certificate declaration state; this is not verification."""
 
@@ -108,9 +132,12 @@ def inspect_plugin_archive(file_bytes: bytes, *, require_manifest: bool = True)
             total_uncompressed += member.file_size
             if total_uncompressed > _PLUGIN_ARCHIVE_MAX_TOTAL_BYTES:
                 raise ValueError('Plugin archive exceeds the uncompressed size limit')
-            normalized = member.filename.replace('\\', '/').strip('/')
+            normalized = _normalize_archive_entry_name(member.filename)
             names.append(member.filename)
-            files.setdefault(normalized.lower(), member)
+            key = normalized.lower()
+            if key in files:
+                raise ValueError(f'Plugin archive contains a duplicate entry: {member.filename}')
+            files[key] = member
 
         manifest_member = files.get('manifest.yaml') or files.get('manifest.yml')
         if manifest_member is None:

tests/unit_tests/plugin/test_archive_entry_names.py

@@ -0,0 +1,93 @@
+"""Archive entry-name validation must match what the SDK will extract.
+
+The host inspects a package (to read manifest/requirements) before the SDK
+extracts it. If the two disagree, a package can pass inspection and then fail to
+install, and the rules can drift apart. These tests pin inspection to the same
+rejections the SDK applies in ``runtime/plugin/artifact.py``.
+"""
+
+import io
+import zipfile
+
+import pytest
+
+from langbot.pkg.plugin.archive import inspect_plugin_archive
+
+MANIFEST = 'apiVersion: v1\nkind: Plugin\nmetadata:\n  name: good\n  version: 0.0.1\n'
+
+
+def _package(entries: dict[str, str | bytes]) -> bytes:
+    buffer = io.BytesIO()
+    with zipfile.ZipFile(buffer, 'w') as archive:
+        for name, content in entries.items():
+            archive.writestr(name, content)
+    return buffer.getvalue()
+
+
+@pytest.mark.parametrize(
+    'bad_name',
+    [
+        '../escape.py',
+        '../../etc/passwd',
+        'a/../../manifest.yaml',
+        'components/../../outside.py',
+        '/absolute.py',
+        'C:/windows/system32/evil.py',
+        'c:evil.py',
+        'dir\\..\\..\\evil.py',
+    ],
+)
+def test_entries_escaping_the_root_are_rejected(bad_name):
+    package = _package({'manifest.yaml': MANIFEST, bad_name: 'x'})
+    with pytest.raises(ValueError):
+        inspect_plugin_archive(package)
+
+
+def test_nul_in_entry_name_is_rejected():
+    # zipfile truncates a name at NUL when writing, so the guard is exercised
+    # directly rather than through a crafted archive.
+    from langbot.pkg.plugin.archive import _normalize_archive_entry_name
+
+    with pytest.raises(ValueError):
+        _normalize_archive_entry_name('evil\x00.py')
+
+
+def test_duplicate_entry_after_normalization_is_rejected():
+    """Two members normalizing to one path make the inspected file ambiguous."""
+    package = _package({'manifest.yaml': MANIFEST, './manifest.yaml': MANIFEST})
+    with pytest.raises(ValueError, match='duplicate'):
+        inspect_plugin_archive(package)
+
+
+def test_traversal_is_rejected_before_any_duplicate_check():
+    """A '..' member is an escape, which is the more severe diagnosis."""
+    package = _package({'manifest.yaml': MANIFEST, 'sub/../manifest.yaml': MANIFEST})
+    with pytest.raises(ValueError, match='escapes the archive root'):
+        inspect_plugin_archive(package)
+
+
+def test_nested_and_unicode_paths_still_accepted():
+    """Normal plugin layouts must keep working."""
+    package = _package(
+        {
+            'manifest.yaml': MANIFEST,
+            'components/__init__.py': '',
+            'components/commands/帮助.py': '',
+            'requirements.txt': 'httpx>=0.27\n',
+        }
+    )
+    inspection = inspect_plugin_archive(package)
+    assert (inspection.manifest.get('metadata') or {}).get('name') == 'good'
+    assert any('httpx' in dep for dep in inspection.requirements)
+
+
+def test_leading_dot_slash_and_double_slashes_are_normalized_not_rejected():
+    """Cosmetic separators are common in real packages and must not be fatal."""
+    package = _package(
+        {
+            './manifest.yaml': MANIFEST,
+            'components//cmd.py': '',
+        }
+    )
+    inspection = inspect_plugin_archive(package)
+    assert (inspection.manifest.get('metadata') or {}).get('name') == 'good'