diff --git a/src/langbot/pkg/plugin/archive.py b/src/langbot/pkg/plugin/archive.py index 28b23cbff..19b3cda61 100644 --- a/src/langbot/pkg/plugin/archive.py +++ b/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: diff --git a/tests/unit_tests/plugin/test_archive_entry_names.py b/tests/unit_tests/plugin/test_archive_entry_names.py new file mode 100644 index 000000000..74a8144f9 --- /dev/null +++ b/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'