diff --git a/packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py b/packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py index 90d1c5e4c..339f2de45 100644 --- a/packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py +++ b/packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py @@ -411,37 +411,26 @@ async def skill_upload( is_archive = zipfile.is_zipfile(buf) if is_archive: - buf = io.BytesIO(content_bytes) - with zipfile.ZipFile(buf, "r") as zf: - for name in zf.namelist(): - if ".." in name or name.startswith("/"): - return Result.failed( - code="E4002", - msg=f"Unsafe path in archive: {name}", - ) - - top_dirs = {n.split("/")[0] for n in zf.namelist() if "/" in n} - if len(top_dirs) == 1: - dest_name = top_dirs.pop() - else: - dest_name = stem - + # Reuse the robust _extract_skill_from_zip helper (same one used + # by the GitHub import endpoint) to avoid the nested-directory bug + # that the old inline extractall logic suffered from. + # + # strict=False: uploaded packages may not contain a SKILL.md yet. + tmp_zip = upload_dir / f"{uuid.uuid4().hex}.zip" + tmp_zip.write_bytes(content_bytes) + try: with tempfile.TemporaryDirectory(dir=upload_dir) as tmp_extract: - tmp_extract_path = Path(tmp_extract) - if len(top_dirs) <= 1 and all( - n.startswith(dest_name + "/") or n == dest_name - for n in zf.namelist() - if n - ): - zf.extractall(tmp_extract_path) - src_dir = tmp_extract_path / dest_name - else: - skill_tmp = tmp_extract_path / dest_name - skill_tmp.mkdir(parents=True, exist_ok=True) - zf.extractall(skill_tmp) - src_dir = skill_tmp + dest_in_tmp = Path(tmp_extract) / "skill" + try: + dest_name = _extract_skill_from_zip( + tmp_zip, subpath=None, dest_dir=dest_in_tmp, strict=False + ) + except ValueError as exc: + return Result.failed(code="E4002", msg=str(exc)) - rel_path = _install_skill_from_dir(src_dir, dest_name, user_dir) + rel_path = _install_skill_from_dir(dest_in_tmp, dest_name, user_dir) + finally: + tmp_zip.unlink(missing_ok=True) else: dest = user_dir / stem @@ -544,10 +533,19 @@ def _construct_download_url(owner: str, repo: str, branch: str) -> str: return f"https://github.com/{owner}/{repo}/archive/refs/heads/{branch}.zip" +def _is_macos_junk(name: str) -> bool: + """Return True if the archive entry is a macOS metadata artifact.""" + parts = name.split("/") + return any(p == "__MACOSX" or p.startswith("._") for p in parts) + + def _extract_skill_from_zip( - zip_path: "Path", subpath: "Optional[str]", dest_dir: "Path" + zip_path: "Path", + subpath: "Optional[str]", + dest_dir: "Path", + strict: bool = True, ) -> str: - """Extract a skill from a GitHub-style ZIP archive into ``dest_dir``. + """Extract a skill from a ZIP archive into ``dest_dir``. The ZIP is expected to have a single top-level directory (e.g. ``repo-main/``). That top-level directory is stripped when extracting so @@ -556,6 +554,10 @@ def _extract_skill_from_zip( When ``subpath`` is given, only the files under ``{top_dir}/{subpath}/`` are extracted (again, stripped to ``dest_dir``). + macOS metadata entries (``__MACOSX/`` directories and ``._*`` files) are + automatically filtered out before any directory-structure analysis so they + do not cause spurious nested directories. + Args: zip_path (Path): Path to the ZIP file on disk. subpath (Optional[str]): Relative sub-directory inside the archive @@ -564,6 +566,10 @@ def _extract_skill_from_zip( dest_dir (Path): Directory into which the skill files are extracted. It is created if it does not exist; if it already exists its contents are removed before extraction. + strict (bool): When ``True`` (default), raise ``ValueError`` if no + ``SKILL.md`` is found in the archive. When ``False``, skip the + ``SKILL.md`` validation — useful for uploading generic skill + packages that may not yet contain a ``SKILL.md``. Returns: str: The skill name derived from ``subpath`` (last component) or from @@ -571,7 +577,8 @@ def _extract_skill_from_zip( Raises: ValueError: If the archive contains path-traversal sequences. - ValueError: If no ``SKILL.md`` is found after extraction. + ValueError: If no ``SKILL.md`` is found after extraction (only when + ``strict=True``). ValueError: If the archive root contains multiple sub-directories with ``SKILL.md`` files and no ``subpath`` was specified (the error message lists the available sub-directory names). @@ -585,8 +592,11 @@ def _extract_skill_from_zip( if normalized.startswith("..") or ".." in normalized.split(os.sep): raise ValueError(f"Unsafe path in archive: {name!r}") + # Filter out macOS metadata artifacts before analysing structure + valid_names = [n for n in all_names if not _is_macos_junk(n)] + # Detect the single top-level directory (GitHub archives always have one) - top_dirs = {n.split("/")[0] for n in all_names if "/" in n} + top_dirs = {n.split("/")[0] for n in valid_names if "/" in n} archive_root: Optional[str] = top_dirs.pop() if len(top_dirs) == 1 else None # Build the prefix inside the archive that maps to dest_dir @@ -601,14 +611,14 @@ def _extract_skill_from_zip( # Check whether SKILL.md exists under the chosen prefix skill_md_entry = next( - (n for n in all_names if n == skill_prefix + "SKILL.md"), + (n for n in valid_names if n == skill_prefix + "SKILL.md"), None, ) if skill_md_entry is None and not subpath: # No SKILL.md at root — scan one level of subdirectories subdirs_with_skill = [] - for name in all_names: + for name in valid_names: if not name.startswith(skill_prefix): continue rel = name[len(skill_prefix) :] @@ -616,13 +626,20 @@ def _extract_skill_from_zip( if len(parts) == 2 and parts[1] == "SKILL.md": subdirs_with_skill.append(parts[0]) - if subdirs_with_skill: + if len(subdirs_with_skill) > 1: raise ValueError( "Multiple skills found. Specify a subpath. " "Available: " + ", ".join(sorted(subdirs_with_skill)) ) - if skill_md_entry is None: + # If exactly one sub-directory has SKILL.md, use it automatically + if len(subdirs_with_skill) == 1: + only_subdir = subdirs_with_skill[0] + skill_prefix = f"{skill_prefix}{only_subdir}/" + skill_name = only_subdir + skill_md_entry = skill_prefix + "SKILL.md" + + if strict and skill_md_entry is None: raise ValueError( "No SKILL.md found in the archive" + (f" under '{subpath}'" if subpath else "") @@ -634,8 +651,8 @@ def _extract_skill_from_zip( shutil.rmtree(dest_dir) dest_dir.mkdir(parents=True, exist_ok=True) - # Extract members individually (no extractall) for security - for member in all_names: + # Extract valid members individually (no extractall) for security + for member in valid_names: if not member.startswith(skill_prefix) or member == skill_prefix: continue rel = member[len(skill_prefix) :] diff --git a/packages/dbgpt-app/src/dbgpt_app/tests/test_import_github_extract.py b/packages/dbgpt-app/src/dbgpt_app/tests/test_import_github_extract.py index 55906daf9..c4cbacb32 100644 --- a/packages/dbgpt-app/src/dbgpt_app/tests/test_import_github_extract.py +++ b/packages/dbgpt-app/src/dbgpt_app/tests/test_import_github_extract.py @@ -15,9 +15,13 @@ import pytest from dbgpt_app.openapi.api_v1.agentic_data_api import _install_skill_from_dir try: - from dbgpt_app.openapi.api_v1.agentic_data_api import _extract_skill_from_zip + from dbgpt_app.openapi.api_v1.agentic_data_api import ( + _extract_skill_from_zip, + _is_macos_junk, + ) except ImportError: _extract_skill_from_zip = None # type: ignore[assignment] + _is_macos_junk = None # type: ignore[assignment] def _make_zip(files: dict) -> bytes: @@ -188,3 +192,133 @@ class TestInstallSkillFromDir: new_content = (user_dir / "my-skill" / "SKILL.md").read_text() assert "version: 2" in new_content assert "version: 1" not in new_content + + +class TestIsMacosJunk: + """Tests for _is_macos_junk helper.""" + + _needs = pytest.mark.skipif( + _is_macos_junk is None, + reason="_is_macos_junk not available", + ) + + @_needs + def test_macosx_dir(self): + assert _is_macos_junk("__MACOSX/foo/bar") is True + + @_needs + def test_dot_underscore_file(self): + assert _is_macos_junk("my-skill/._SKILL.md") is True + + @_needs + def test_normal_file(self): + assert _is_macos_junk("my-skill/SKILL.md") is False + + @_needs + def test_nested_macosx(self): + assert _is_macos_junk("repo-main/__MACOSX/my-skill/SKILL.md") is True + + +class TestExtractSkillFromZipEnhancements: + """Tests for the enhanced _extract_skill_from_zip (strict, __MACOSX filter).""" + + _needs = pytest.mark.skipif( + _extract_skill_from_zip is None, + reason="_extract_skill_from_zip not available", + ) + + @_needs + def test_macosx_filtered_single_top_dir(self, tmp_path): + """ZIP with __MACOSX alongside skill dir should not create nested dirs.""" + zip_bytes = _make_zip( + { + "my-skill/SKILL.md": "name: my-skill\n", + "my-skill/scripts/run.sh": "echo hello\n", + "__MACOSX/my-skill/._SKILL.md": "binary junk", + } + ) + zip_path = tmp_path / "pkg.zip" + zip_path.write_bytes(zip_bytes) + dest = tmp_path / "target" + + skill_name = _extract_skill_from_zip(zip_path, subpath=None, dest_dir=dest) + + assert skill_name == "my-skill" + assert (dest / "SKILL.md").exists() + assert (dest / "scripts" / "run.sh").exists() + # __MACOSX content must NOT be extracted + assert not (dest / "__MACOSX").exists() + assert not (dest / "._SKILL.md").exists() + + @_needs + def test_strict_false_no_skill_md(self, tmp_path): + """With strict=False, missing SKILL.md should NOT raise.""" + zip_bytes = _make_zip( + { + "my-tool/config.json": '{"key": "value"}\n', + "my-tool/README.md": "# readme\n", + } + ) + zip_path = tmp_path / "pkg.zip" + zip_path.write_bytes(zip_bytes) + dest = tmp_path / "target" + + skill_name = _extract_skill_from_zip( + zip_path, subpath=None, dest_dir=dest, strict=False + ) + + assert skill_name == "my-tool" + assert (dest / "config.json").exists() + assert (dest / "README.md").exists() + + @_needs + def test_strict_true_no_skill_md_raises(self, tmp_path): + """With strict=True (default), missing SKILL.md MUST raise.""" + zip_bytes = _make_zip({"my-tool/config.json": '{"key": "value"}\n'}) + zip_path = tmp_path / "pkg.zip" + zip_path.write_bytes(zip_bytes) + dest = tmp_path / "target" + + with pytest.raises(ValueError, match="No SKILL.md found"): + _extract_skill_from_zip(zip_path, subpath=None, dest_dir=dest) + + @_needs + def test_single_subdir_with_skill_md_auto_selected(self, tmp_path): + """When exactly one sub-directory has SKILL.md, select it automatically.""" + zip_bytes = _make_zip( + { + "repo-main/my-skill/SKILL.md": "name: my-skill\n", + "repo-main/my-skill/scripts/run.sh": "echo hi\n", + } + ) + zip_path = tmp_path / "pkg.zip" + zip_path.write_bytes(zip_bytes) + dest = tmp_path / "target" + + skill_name = _extract_skill_from_zip(zip_path, subpath=None, dest_dir=dest) + + assert skill_name == "my-skill" + assert (dest / "SKILL.md").exists() + assert (dest / "scripts" / "run.sh").exists() + + @_needs + def test_flat_zip_no_top_dir(self, tmp_path): + """ZIP with files at root (no wrapping dir), strict=False.""" + zip_bytes = _make_zip( + { + "config.json": '{"key": "value"}\n', + "README.md": "# Hi\n", + } + ) + zip_path = tmp_path / "my-pkg.zip" + zip_path.write_bytes(zip_bytes) + dest = tmp_path / "target" + + skill_name = _extract_skill_from_zip( + zip_path, subpath=None, dest_dir=dest, strict=False + ) + + # skill_name falls back to dest_dir.name when no archive_root + assert skill_name == "target" + assert (dest / "config.json").exists() + assert (dest / "README.md").exists()