Skip to content

Commit 912dd1a

Browse files
fix(hub): address PR #7 review issues
- Fix path traversal in local backend via _safe_repo_path validation - Rename client SyncPlan to ClientSyncPlan to avoid conflict with sync.SyncPlan - Use semver-aware sorting for local skill versions - Support GitHub organization repo creation in GitHub backend - Harden YAML/JSON manifest deserialization with fallback - Persist pulled skills via ctx.skill_lib in hub_pull_tool - Load actual local skills in hub_sync_tool - Avoid event-loop blocking during ModelScope file reads
1 parent 5081eb5 commit 912dd1a

6 files changed

Lines changed: 138 additions & 43 deletions

File tree

‎src/leapflow/hub/backends/github.py‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -224,25 +224,45 @@ async def delete_skill(self, repo_id: str) -> None:
224224
# ── Private Helpers ───────────────────────────────────────────────────
225225

226226
async def _ensure_repo(self, repo_id: str, visibility: Visibility) -> None:
227-
"""Create repository if it doesn't exist."""
227+
"""Create repository if it doesn't exist.
228+
229+
Handles both personal repos (POST /user/repos) and org repos
230+
(POST /orgs/{org}/repos) based on owner identity.
231+
"""
228232
parts = repo_id.split("/")
229233
if len(parts) != 2:
230234
raise ValueError(f"Invalid repo_id format: '{repo_id}' (expected 'owner/name')")
231-
_, name = parts
235+
owner, name = parts
232236

233237
# Check if repo exists
234238
resp = await self._http.get(f"/repos/{repo_id}")
235239
if resp.status_code == 200:
236240
return # Already exists
237241

238-
# Create repo
242+
# Determine if owner is the authenticated user or an org
239243
private = visibility != Visibility.PUBLIC
240-
create_resp = await self._http.post("/user/repos", json={
241-
"name": name,
242-
"private": private,
243-
"auto_init": True,
244-
"description": f"LeapFlow skill: {name}",
245-
})
244+
user_resp = await self._http.get("/user")
245+
is_personal = (
246+
user_resp.status_code == 200
247+
and user_resp.json().get("login", "") == owner
248+
)
249+
250+
if is_personal:
251+
create_resp = await self._http.post("/user/repos", json={
252+
"name": name,
253+
"private": private,
254+
"auto_init": True,
255+
"description": f"LeapFlow skill: {name}",
256+
})
257+
else:
258+
# Organization repo
259+
create_resp = await self._http.post(f"/orgs/{owner}/repos", json={
260+
"name": name,
261+
"private": private,
262+
"auto_init": True,
263+
"description": f"LeapFlow skill: {name}",
264+
})
265+
246266
if create_resp.status_code not in (201, 422):
247267
create_resp.raise_for_status()
248268

‎src/leapflow/hub/backends/local.py‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,19 @@
2727
logger = logging.getLogger(__name__)
2828

2929

30+
def _semver_key(version: str) -> tuple:
31+
"""Parse version string to comparable tuple for correct semver sorting.
32+
33+
Handles formats: '0.1.0', 'v1.2.3', '1.0'.
34+
Returns (0, 0, 0) on parse failure for safe fallback.
35+
"""
36+
cleaned = version.strip().lstrip("v")
37+
try:
38+
return tuple(int(x) for x in cleaned.split("."))
39+
except (ValueError, AttributeError):
40+
return (0, 0, 0)
41+
42+
3043
class LocalBackend:
3144
"""HubBackend using local filesystem as storage.
3245
@@ -54,6 +67,26 @@ def base_dir(self) -> Path:
5467
"""Return the base directory for local storage."""
5568
return self._base_dir
5669

70+
def _safe_repo_path(self, repo_id: str) -> Path:
71+
"""Validate repo_id and resolve to safe path under base_dir.
72+
73+
Prevents path traversal attacks via '..' segments or absolute paths.
74+
"""
75+
from pathlib import PurePosixPath
76+
77+
parts = PurePosixPath(repo_id).parts
78+
if not parts:
79+
raise ValueError(f"Empty repo_id")
80+
if any(p == ".." for p in parts):
81+
raise ValueError(f"Path traversal detected in repo_id: '{repo_id}'")
82+
if PurePosixPath(repo_id).is_absolute():
83+
raise ValueError(f"Absolute path not allowed in repo_id: '{repo_id}'")
84+
85+
resolved = (self._base_dir / repo_id).resolve()
86+
if not str(resolved).startswith(str(self._base_dir.resolve())):
87+
raise ValueError(f"repo_id escapes base directory: '{repo_id}'")
88+
return resolved
89+
5790
async def authenticate(self) -> UserInfo:
5891
"""Return local user info (no actual authentication needed)."""
5992
username = os.environ.get("USER", os.environ.get("USERNAME", "local"))
@@ -65,13 +98,9 @@ async def push_skill(
6598
repo_id: str,
6699
visibility: Visibility = Visibility.PRIVATE,
67100
) -> PushResult:
68-
"""Write skill bundle to local filesystem.
69-
70-
Creates a versioned directory structure:
71-
base_dir/repo_id/version/files...
72-
"""
101+
"""Write skill bundle to local filesystem."""
73102
version = bundle.manifest.version or "0.1.0"
74-
repo_path = self._base_dir / repo_id / version
103+
repo_path = self._safe_repo_path(repo_id) / version
75104
repo_path.mkdir(parents=True, exist_ok=True)
76105

77106
# Write files
@@ -95,20 +124,18 @@ async def pull_skill(
95124
version: Optional[str] = None,
96125
) -> SkillBundle:
97126
"""Read skill bundle from local filesystem."""
98-
repo_base = self._base_dir / repo_id
127+
repo_base = self._safe_repo_path(repo_id)
99128

100129
if not repo_base.exists():
101130
raise FileNotFoundError(f"Skill repository not found: {repo_id}")
102131

103132
# Resolve version
104133
if version is None:
105-
# Get latest version (sorted lexicographically)
106-
versions = sorted(
107-
[d.name for d in repo_base.iterdir() if d.is_dir()],
108-
reverse=True,
109-
)
110-
if not versions:
134+
# Get latest version (semantic version sort, not lexicographic)
135+
version_dirs = [d.name for d in repo_base.iterdir() if d.is_dir()]
136+
if not version_dirs:
111137
raise FileNotFoundError(f"No versions found for: {repo_id}")
138+
versions = sorted(version_dirs, key=_semver_key, reverse=True)
112139
version = versions[0]
113140

114141
version_path = repo_base / version
@@ -221,7 +248,7 @@ async def delete_skill(self, repo_id: str) -> None:
221248
"""Delete a skill from local storage."""
222249
import shutil
223250

224-
repo_base = self._base_dir / repo_id
251+
repo_base = self._safe_repo_path(repo_id)
225252

226253
if not repo_base.exists():
227254
raise FileNotFoundError(f"Skill repository not found: {repo_id}")

‎src/leapflow/hub/backends/modelscope.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,13 +142,17 @@ async def pull_skill(
142142
**kwargs,
143143
)
144144

145-
# Read all files from downloaded directory
145+
# Read all files in a thread to avoid blocking the event loop
146+
def _read_files(local_path: Path) -> dict:
147+
files: dict[str, str] = {}
148+
for file_path in local_path.rglob("*"):
149+
if file_path.is_file() and not file_path.name.startswith("."):
150+
rel = file_path.relative_to(local_path)
151+
files[str(rel)] = file_path.read_text(encoding="utf-8")
152+
return files
153+
146154
local_path = Path(local_dir)
147-
files: dict[str, str] = {}
148-
for file_path in local_path.rglob("*"):
149-
if file_path.is_file() and not file_path.name.startswith("."):
150-
rel = file_path.relative_to(local_path)
151-
files[str(rel)] = file_path.read_text(encoding="utf-8")
155+
files = await asyncio.to_thread(_read_files, local_path)
152156

153157
return self._serializer.files_to_bundle(files)
154158

‎src/leapflow/hub/client.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,11 @@
2626
# ─── Sync Plan ───────────────────────────────────────────────────────────────
2727

2828

29-
class SyncPlan:
30-
"""Plan describing what needs to happen during a sync operation."""
29+
class ClientSyncPlan:
30+
"""Lightweight sync plan for HubClient.sync_skills() preview.
31+
32+
Distinct from leapflow.hub.sync.SyncPlan (full engine-level plan).
33+
"""
3134

3235
def __init__(
3336
self,
@@ -325,7 +328,7 @@ async def sync_skills(
325328
if name not in local_by_name:
326329
to_pull.append(remote)
327330

328-
return SyncPlan(to_push=to_push, to_pull=to_pull, conflicts=conflicts)
331+
return ClientSyncPlan(to_push=to_push, to_pull=to_pull, conflicts=conflicts)
329332

330333
async def login(self) -> UserInfo:
331334
"""Authenticate with the hub backend.

‎src/leapflow/hub/serializer.py‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,20 @@ def _serialize_manifest(manifest: SkillManifest) -> str:
7575

7676

7777
def _deserialize_manifest(text: str) -> SkillManifest:
78-
"""Deserialize manifest from YAML or JSON."""
78+
"""Deserialize manifest from YAML or JSON with graceful fallback."""
79+
data = None
7980
if _YAML_AVAILABLE:
80-
data = yaml.safe_load(text)
81-
else:
82-
# Try JSON first, fallback to basic parsing
83-
data = json.loads(text)
84-
return _dict_to_manifest(data if data else {})
81+
try:
82+
data = yaml.safe_load(text)
83+
except Exception:
84+
logger.debug("YAML parse failed for manifest; trying JSON fallback")
85+
if data is None:
86+
try:
87+
data = json.loads(text)
88+
except (json.JSONDecodeError, ValueError):
89+
logger.warning("Failed to parse manifest as YAML or JSON")
90+
data = None
91+
return _dict_to_manifest(data if isinstance(data, dict) else {})
8592

8693

8794
# ─── SkillSerializer ─────────────────────────────────────────────────────────

‎src/leapflow/tools/hub_tool.py‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,33 @@ async def hub_pull_tool(
146146
serializer = SkillSerializer()
147147
skill_data = serializer.import_skill(bundle)
148148

149+
# Attempt to save to local skill library if context available
150+
ctx = kwargs.get("ctx")
151+
if ctx is not None and hasattr(ctx, "skill_lib") and ctx.skill_lib is not None:
152+
if high_risk:
153+
return (
154+
f"Pulled '{bundle.manifest.name}' v{bundle.manifest.version} "
155+
f"from {repo_id} but NOT installed due to {len(high_risk)} "
156+
f"high-risk finding(s).{finding_text}\n"
157+
f"Use CLI 'leap hub pull {repo_id} --trust' to install with risks accepted."
158+
)
159+
try:
160+
ctx.skill_lib.save_from_hub(skill_data)
161+
return (
162+
f"Pulled and installed '{bundle.manifest.name}' "
163+
f"v{bundle.manifest.version} from {repo_id}.{finding_text}"
164+
)
165+
except Exception as e:
166+
return (
167+
f"Pulled '{bundle.manifest.name}' v{bundle.manifest.version} "
168+
f"from {repo_id} but install failed: {e}.{finding_text}"
169+
)
170+
149171
return (
150172
f"Pulled '{bundle.manifest.name}' v{bundle.manifest.version} "
151173
f"from {repo_id}.{finding_text}\n"
152-
f"Skill is ready for local installation. "
153-
f"Use 'hub pull {repo_id} --trust' in CLI to install directly."
174+
f"Skill library context not available; skill was not installed. "
175+
f"Use 'leap hub pull {repo_id}' in CLI to install."
154176
)
155177

156178

@@ -219,10 +241,22 @@ async def hub_sync_tool(
219241
repo_prefix=settings.hub_repo_prefix,
220242
)
221243

222-
# For tool context we use empty manifests as placeholder
223-
# Real implementation would access skill_lib from ctx
244+
# Load actual local skills from context if available
245+
ctx = kwargs.get("ctx")
246+
local_manifests: list = []
247+
if ctx is not None and hasattr(ctx, "skill_lib") and ctx.skill_lib is not None:
248+
try:
249+
stored_skills = ctx.skill_lib.load_all_active()
250+
for s in stored_skills:
251+
local_manifests.append(SkillManifest(
252+
name=getattr(s, "title", ""),
253+
version=str(getattr(s, "version", "0.1.0")),
254+
))
255+
except Exception:
256+
pass # Fall through to empty manifests
257+
224258
try:
225-
plan = await client.sync_skills([])
259+
plan = await client.sync_skills(local_manifests)
226260
except Exception as e:
227261
return f"Sync failed: {type(e).__name__}: {e}"
228262

0 commit comments

Comments
 (0)