Merge pull request #548 from mo-tunn/fix/skill-upload-token-traversal

fix(skills): confine staged upload tokens

coworker/skills/store.py

@@ -31,6 +31,7 @@ from ..secrets import state_dir
 from .base import Skill, _parse_skill
 
 _NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$")
+_UPLOAD_TOKEN_RE = re.compile(r"^[0-9a-f]{32}$")
 _MAX_NAME = 64
 GLOBAL_SCOPE = "global"
 PROJECT_SCOPE = "project"
@@ -277,6 +278,22 @@ class SkillStore:
             )
 
     # -- uploads: stage → preview → confirm -----------------------------------------
+    def _staged_upload_path(self, token: str) -> Path:
+        """Resolve one server-minted upload token without leaving the staging root."""
+        token = str(token)
+        if not _UPLOAD_TOKEN_RE.fullmatch(token):
+            raise ValueError("Unknown or expired upload.")
+        try:
+            staging_root = self._staging_dir.resolve()
+            staged = (staging_root / token).resolve()
+        except (OSError, RuntimeError):
+            raise ValueError("Unknown or expired upload.") from None
+        # The shape check blocks path separators; this second check also rejects a
+        # valid-looking token whose staging entry is a symlink/junction to elsewhere.
+        if staged.parent != staging_root:
+            raise ValueError("Unknown or expired upload.")
+        return staged
+
     def stage_upload(self, data: bytes, filename: str = "") -> dict[str, Any]:
         """Stage an upload and return the parsed preview. Accepts a ``.zip`` (folder skill)
         or a bare ``SKILL.md`` with YAML frontmatter. Nothing is installed until
@@ -376,7 +393,7 @@ class SkillStore:
         scope: str = GLOBAL_SCOPE,
         workspace: Optional[str | Path] = None,
     ) -> dict[str, Any]:
-        staged = self._staging_dir / str(token)
+        staged = self._staged_upload_path(token)
         if not (staged / "SKILL.md").is_file():
             raise ValueError("Unknown or expired upload.")
         skill = _parse_skill(staged / "SKILL.md")
@@ -399,7 +416,7 @@ class SkillStore:
         return {"name": name, "scope": scope, "path": str(folder)}
 
     def discard_upload(self, token: str) -> None:
-        staged = self._staging_dir / str(token)
+        staged = self._staged_upload_path(token)
         shutil.rmtree(staged, ignore_errors=True)
 
 

tests/test_skills_api.py

@@ -207,6 +207,24 @@ def test_upload_preview_then_confirm(tmp_path):
     assert row["source"] == "uploaded"
 
 
+def test_upload_confirm_rejects_forged_absolute_token(tmp_path):
+    client, _m, _p = _client(tmp_path)
+    outside = tmp_path / "outside-staging"
+    outside.mkdir()
+    (outside / "SKILL.md").write_text(
+        "---\nname: forged\ndescription: d\n---\nbody\n", encoding="utf-8"
+    )
+    marker = outside / "keep.txt"
+    marker.write_text("must survive", encoding="utf-8")
+
+    result = client.post(
+        "/v1/skills/upload/confirm", json={"token": str(outside.resolve())}
+    ).json()
+
+    assert result["ok"] is False and "expired" in result["error"].lower()
+    assert marker.read_text(encoding="utf-8") == "must survive"
+
+
 def test_upload_invalid_archive_friendly(tmp_path):
     client, _m, _p = _client(tmp_path)
     bad = client.post(

tests/test_skills_store.py

@@ -291,6 +291,44 @@ def test_upload_confirm_saves_previewed_content(store):
         store.confirm_upload(preview["token"])  # token is one-shot
 
 
+@pytest.mark.parametrize("action", ["confirm", "discard"])
+def test_upload_token_cannot_escape_staging_dir(store, tmp_path, action):
+    outside = tmp_path / f"outside-{action}"
+    outside.mkdir()
+    (outside / "SKILL.md").write_text(SKILL_MD, encoding="utf-8")
+    marker = outside / "keep.txt"
+    marker.write_text("must survive", encoding="utf-8")
+    token = os.path.relpath(outside, store._staging_dir)
+
+    with pytest.raises(ValueError, match="expired"):
+        if action == "confirm":
+            store.confirm_upload(token)
+        else:
+            store.discard_upload(token)
+
+    assert marker.read_text(encoding="utf-8") == "must survive"
+
+
+def test_upload_token_symlink_cannot_escape_staging_dir(store, tmp_path):
+    outside = tmp_path / "outside-symlink"
+    outside.mkdir()
+    (outside / "SKILL.md").write_text(SKILL_MD, encoding="utf-8")
+    marker = outside / "keep.txt"
+    marker.write_text("must survive", encoding="utf-8")
+    token = "a" * 32  # valid token shape; containment must still be enforced
+    store._staging_dir.mkdir(parents=True, exist_ok=True)
+    try:
+        os.symlink(outside, store._staging_dir / token, target_is_directory=True)
+    except (OSError, NotImplementedError):
+        pytest.skip("symlinks unavailable on this platform/user")
+
+    with pytest.raises(ValueError, match="expired"):
+        store.confirm_upload(token)
+    with pytest.raises(ValueError, match="expired"):
+        store.discard_upload(token)
+    assert marker.read_text(encoding="utf-8") == "must survive"
+
+
 # -- disable state -------------------------------------------------------------------