diff --git a/src/fast_agent/marketplace/source_urls.py b/src/fast_agent/marketplace/source_urls.py index f9ce7ee26..3e42502a0 100644 --- a/src/fast_agent/marketplace/source_urls.py +++ b/src/fast_agent/marketplace/source_urls.py @@ -4,7 +4,7 @@ from pathlib import Path, PurePosixPath from typing import TYPE_CHECKING, Any -from urllib.parse import urlparse +from urllib.parse import quote, unquote, urlparse from fast_agent.io.path_uri import file_uri_to_path from fast_agent.marketplace.source_models import MarketplaceRepoFields, ParsedGitHubUrl @@ -101,7 +101,7 @@ def github_raw_file_url( return None org, repo = repo_parts ref = repo_ref or "main" - return f"https://{GITHUB_RAW_HOST}/{org}/{repo}/{ref}/{repo_path}" + return f"https://{GITHUB_RAW_HOST}/{org}/{repo}/{ref}/{quote(repo_path, safe='/')}" def _github_raw_content_url(parsed_source: ParsedGitHubUrl | None) -> str | None: @@ -182,11 +182,14 @@ def candidate_marketplace_urls(url: str) -> list[str]: def _github_marketplace_candidates(org: str, repo: str, ref: str, base_path: str) -> list[str]: suffixes = _marketplace_path_candidates(base_path) - return [f"https://{GITHUB_RAW_HOST}/{org}/{repo}/{ref}/{suffix}" for suffix in suffixes] + return [ + f"https://{GITHUB_RAW_HOST}/{org}/{repo}/{ref}/{quote(suffix, safe='/')}" + for suffix in suffixes + ] def _marketplace_path_candidates(base_path: str) -> list[str]: - cleaned = base_path.strip().strip("/") + cleaned = base_path.strip("/") if not cleaned: return [".claude-plugin/marketplace.json", "marketplace.json"] @@ -283,10 +286,17 @@ def _parsed_github_source( return None org, repo = repo_parts[:2] ref, repo_path = split_ref + try: + decoded_path = unquote(repo_path, errors="strict") + except UnicodeDecodeError: + return None + if decoded_path != decoded_path.strip() or "\\" in decoded_path: + # Repository path normalization would select a different path. + return None return ParsedGitHubUrl( repo_url=f"https://github.com/{org}/{repo}", repo_ref=ref, - repo_path=repo_path, + repo_path=decoded_path, ) diff --git a/tests/unit/fast_agent/skills/test_github_encoded_paths.py b/tests/unit/fast_agent/skills/test_github_encoded_paths.py new file mode 100644 index 000000000..269ff788c --- /dev/null +++ b/tests/unit/fast_agent/skills/test_github_encoded_paths.py @@ -0,0 +1,128 @@ +from __future__ import annotations + +import shutil +import subprocess +from typing import TYPE_CHECKING +from urllib.parse import quote + +import pytest + +from fast_agent.marketplace import source_urls +from fast_agent.skills import operations +from fast_agent.skills.marketplace_parsing import parse_marketplace_payload +from fast_agent.skills.provenance import read_installed_skill_source + +if TYPE_CHECKING: + from pathlib import Path + + +@pytest.mark.parametrize("folder", ["hello world", "literal%20name", "hash#name", "snow 雪"]) +@pytest.mark.parametrize("kind", ["tree", "raw"]) +def test_install_from_encoded_github_skill_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, folder: str, kind: str +) -> None: + repo = tmp_path / "repo" + skill_dir = repo / "skills" / folder + skill_dir.mkdir(parents=True) + content = "---\nname: hello\ndescription: Example skill\n---\n\nHello.\n" + (skill_dir / "SKILL.md").write_text(content, encoding="utf-8") + for args in ( + ["init", "-b", "main"], + ["config", "user.email", "tests@example.com"], + ["config", "user.name", "Test User"], + ["add", "."], + ["commit", "-m", "initial"], + ): + subprocess.run( + ["git", "-c", "core.autocrlf=true", "-C", str(repo), *args], + check=True, + capture_output=True, + ) + + def clone_source( + *, + repo_url: str, + repo_ref: str | None, + repo_subdir: str, + destination_dir: Path, + checkout_ref: str | None, + ) -> None: + assert repo_url == "https://github.com/example/skills" + assert repo_ref == "main" + assert repo_subdir == f"skills/{folder}" + shutil.copytree(repo, destination_dir, dirs_exist_ok=True) + + monkeypatch.setattr(operations.marketplace_git_sources, "clone_sparse_checkout", clone_source) + path = quote(f"skills/{folder}", safe="/") + url = ( + f"https://github.com/example/skills/tree/main/{path}" + if kind == "tree" + else f"https://raw.githubusercontent.com/example/skills/main/{path}/SKILL.md" + ) + skills = parse_marketplace_payload([{"name": "hello", "repo_url": url}]) + installed = operations.install_marketplace_skill_sync(skills[0], tmp_path / "installed") + assert (installed / "SKILL.md").read_text(encoding="utf-8") == content + source = read_installed_skill_source(installed).source + assert source is not None + assert source.repo_path == f"skills/{folder}" + + +@pytest.mark.parametrize("ref", ["main", "release/v1", "release%2Fv1"]) +@pytest.mark.parametrize( + "path", + ["skills/hello world/SKILL.md", "skills/hash#name/SKILL.md", "skills/literal%20/SKILL.md"], +) +def test_github_raw_url_round_trip(ref: str, path: str) -> None: + raw = source_urls.github_raw_file_url( + repo_url="https://github.com/example/skills", repo_ref=ref, repo_path=path + ) + assert raw == f"https://raw.githubusercontent.com/example/skills/{ref}/{quote(path, safe='/')}" + parsed = source_urls.parse_github_url(raw) + assert parsed is not None + assert parsed.repo_ref == ref + assert parsed.repo_path == path + assert source_urls.normalize_marketplace_url(raw) == raw + + +def test_explicit_repository_path_is_not_url_decoded() -> None: + skill = parse_marketplace_payload( + [ + { + "name": "hello", + "repo_url": "https://github.com/example/skills", + "repo_path": "skills/literal%20", + } + ] + )[0] + assert skill.repo_path == "skills/literal%20" + + +@pytest.mark.parametrize("path", ["skills/trailing ", " leading/skill", "skills/back\\slash"]) +def test_marketplace_rejects_url_paths_that_normalization_would_change(path: str) -> None: + url = f"https://github.com/example/skills/tree/main/{quote(path, safe='/')}" + assert source_urls.parse_github_url(url) is None + assert parse_marketplace_payload([{"name": "hello", "repo_url": url}]) == [] + + +@pytest.mark.parametrize("encoded_path", ["skills/%FF", "skills/%C3%28"]) +def test_marketplace_rejects_invalid_utf8_url_paths(encoded_path: str) -> None: + url = f"https://github.com/example/skills/tree/main/{encoded_path}" + assert source_urls.parse_github_url(url) is None + assert parse_marketplace_payload([{"name": "hello", "repo_url": url}]) == [] + + +@pytest.mark.parametrize("kind", ["tree", "raw"]) +def test_marketplace_candidate_path_encoding_is_stable(kind: str) -> None: + encoded = "skills/hello%20world" + url = ( + f"https://github.com/example/skills/tree/main/{encoded}" + if kind == "tree" + else f"https://raw.githubusercontent.com/example/skills/main/{encoded}/marketplace.json" + ) + if kind == "tree": + assert source_urls.candidate_marketplace_urls(url) == [ + f"https://raw.githubusercontent.com/example/skills/main/{encoded}/.claude-plugin/marketplace.json", + f"https://raw.githubusercontent.com/example/skills/main/{encoded}/marketplace.json", + ] + else: + assert source_urls.normalize_marketplace_url(url) == url