Error on invalid toolkit file (#510)

Changes toolkit loading and deployments to error if there are syntax
errors in the file

libs/arcade-cli/arcade_cli/deployment.py

@@ -9,6 +9,8 @@ from pathlib import Path
 from typing import Any
 
 import toml
+from arcade_core import Toolkit
+from arcade_core.toolkit import valid_path
 from arcadepy import Arcade, NotFoundError
 from httpx import Client, ConnectError, HTTPStatusError, TimeoutException
 from packaging.requirements import Requirement
@@ -228,18 +230,7 @@ class Worker(BaseModel):
 
         def exclude_filter(tarinfo: tarfile.TarInfo) -> tarfile.TarInfo | None:
             """Filter for files/directories to exclude from the compressed package"""
-            basename = os.path.basename(tarinfo.name)
+            if not valid_path(tarinfo.name):
-
-            # Exclude all hidden directories/files
-            if basename.startswith("."):
-                return None
-
-            # Exclude specific directories/files
-            if basename in {"dist", "build", "__pycache__", "venv", "coverage.xml"}:
-                return None
-
-            # Exclude lock files
-            if basename.endswith(".lock"):
                 return None
 
             return tarinfo
@@ -262,6 +253,9 @@ class Worker(BaseModel):
                     f"package '{package_path}' must contain a pyproject.toml or setup.py file"
                 )
 
+            # Validate that we are able to load the package
+            Toolkit.tools_from_directory(package_dir=package_path, package_name=package_path.name)
+
             # Compress the package into a byte stream and tar
             byte_stream = io.BytesIO()
             with tarfile.open(fileobj=byte_stream, mode="w:gz") as tar:

libs/arcade-core/arcade_core/toolkit.py

@@ -4,7 +4,7 @@ import logging
 import os
 import types
 from collections import defaultdict
-from pathlib import Path
+from pathlib import Path, PurePosixPath, PureWindowsPath
 
 from pydantic import BaseModel, ConfigDict, field_validator
 
@@ -87,14 +87,6 @@ class Toolkit(BaseModel):
         except (ImportError, AttributeError) as e:
             raise ToolkitLoadError(f"Failed to locate package directory for '{package}'.") from e
 
-        # Get all python files in the package directory
-        try:
-            modules = [f for f in package_dir.glob("**/*.py") if f.is_file()]
-        except OSError as e:
-            raise ToolkitLoadError(
-                f"Failed to locate Python files in package directory for '{package}'."
-            ) from e
-
         toolkit = cls(
             name=name,
             package_name=package_name,
@@ -105,14 +97,7 @@ class Toolkit(BaseModel):
             repository=repo,
         )
 
-        for module_path in modules:
+        toolkit.tools = cls.tools_from_directory(package_dir, package_name)
-            relative_path = module_path.relative_to(package_dir)
-            import_path = ".".join(relative_path.with_suffix("").parts)
-            import_path = f"{package_name}.{import_path}"
-            toolkit.tools[import_path] = get_tools_from_file(str(module_path))
-
-        if not toolkit.tools:
-            raise ToolkitLoadError(f"No tools found in package {package}")
 
         return toolkit
 
@@ -229,6 +214,61 @@ class Toolkit(BaseModel):
 
         return all_toolkits
 
+    @classmethod
+    def tools_from_directory(cls, package_dir: Path, package_name: str) -> dict[str, list[str]]:
+        """
+        Load a Toolkit from a directory.
+        """
+        # Get all python files in the package directory
+        try:
+            modules = [f for f in package_dir.glob("**/*.py") if f.is_file() and valid_path(f)]
+        except OSError as e:
+            raise ToolkitLoadError(
+                f"Failed to locate Python files in package directory for '{package_name}'."
+            ) from e
+
+        tools: dict[str, list[str]] = {}
+
+        for module_path in modules:
+            relative_path = module_path.relative_to(package_dir)
+            cls.validate_file(module_path)
+            import_path = ".".join(relative_path.with_suffix("").parts)
+            import_path = f"{package_name}.{import_path}"
+            tools[import_path] = get_tools_from_file(str(module_path))
+
+        if not tools:
+            raise ToolkitLoadError(f"No tools found in package {package_name}")
+
+        return tools
+
+    @classmethod
+    def validate_file(cls, file_path: str | Path) -> None:
+        """
+        Validate that the Python code in the given file is syntactically correct.
+
+        Args:
+            file_path: Path to the Python file to validate
+        """
+        # Convert string path to Path object if needed
+        path = Path(file_path) if isinstance(file_path, str) else file_path
+
+        # Check if file exists
+        if not path.exists():
+            raise ValueError(f"❌ File not found: {path}")
+
+        # Check if it's a Python file
+        if not path.suffix == ".py":
+            raise ValueError(f"❌ Not a Python file: {path}")
+
+        try:
+            # Try to compile the code to check for syntax errors
+            with open(path, encoding="utf-8") as f:
+                source = f.read()
+
+            compile(source, str(path), "exec")
+        except Exception as e:
+            raise SyntaxError(f"{path}: {e}")
+
 
 def get_package_directory(package_name: str) -> str:
     """
@@ -247,3 +287,25 @@ def get_package_directory(package_name: str) -> str:
         return spec.submodule_search_locations[0]
     else:
         raise ImportError(f"Package {package_name} does not have a file path associated with it")
+
+
+def valid_path(path: str) -> bool:
+    """
+    Validate if a path is valid to be served or deployed.
+    """
+    # Check both POSIX and Windows interpretations
+    posix_path = PurePosixPath(path)
+    windows_path = PureWindowsPath(path)
+
+    # Get all possible parts from both interpretations
+    all_parts = set(posix_path.parts) | set(windows_path.parts)
+
+    for part in all_parts:
+        if part.startswith("."):
+            return False
+        if part in {"dist", "build", "__pycache__", "venv", "coverage.xml"}:
+            return False
+        if part.endswith(".lock"):
+            return False
+
+    return True

libs/arcade-core/pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "arcade-core"
-version = "2.2.0"
+version = "2.2.1"
 description = "Arcade Core - Core library for Arcade platform"
 readme = "README.md"
 license = {text = "MIT"}

libs/tests/core/test_toolkit.py

@@ -1,8 +1,9 @@
+from pathlib import Path
 from unittest.mock import MagicMock, patch
 
 import pytest
 from arcade_core.errors import ToolkitLoadError
-from arcade_core.toolkit import Toolkit
+from arcade_core.toolkit import Toolkit, valid_path
 
 
 class TestToolkit:
@@ -342,3 +343,224 @@ class TestToolkitIntegration:
         assert names == {"custom", "utils", "legacy"}
         utils_toolkit = next(t for t in toolkits if t.name == "utils")
         assert utils_toolkit.version == "2.0.0"
+
+
+@pytest.fixture
+def cleanup_test_files():
+    test_files = ["valid.py", "invalid.py", "test.txt"]
+
+    # Run the test
+    yield
+
+    # Clean up test files after the test
+    for file in test_files:
+        try:  # noqa: SIM105
+            Path(file).unlink(missing_ok=True)
+        except Exception:  # noqa: S110
+            pass
+
+
+class TestValidateFile:
+    @pytest.mark.usefixtures("cleanup_test_files")
+    def test_validate_file(self):
+        """Test validation of Python files with valid syntax."""
+        # Create a temporary valid Python file
+        valid_file = Path("valid.py")
+        valid_file.write_text("def test(): return True")
+
+        # Should not raise any exceptions
+        Toolkit.validate_file(valid_file)
+        # Test with string path
+        Toolkit.validate_file(str(valid_file))
+
+    def test_validate_tools_nonexistent_file(self):
+        """Test validation with non-existent file."""
+        nonexistent = Path("nonexistent.py")
+
+        with pytest.raises(ValueError, match="File not found"):
+            Toolkit.validate_file(nonexistent)
+
+    @pytest.mark.usefixtures("cleanup_test_files")
+    def test_validate_tools_non_python_file(self):
+        """Test validation with non-Python file."""
+        txt_file = Path("test.txt")
+        txt_file.write_text("Not a Python file")
+
+        with pytest.raises(ValueError, match="Not a Python file"):
+            Toolkit.validate_file(txt_file)
+
+    @pytest.mark.usefixtures("cleanup_test_files")
+    def test_validate_tools_syntax_error(self):
+        """Test validation with Python file containing syntax errors."""
+        invalid_file = Path("invalid.py")
+        invalid_file.write_text("def test(): return True:")  # Invalid syntax
+
+        with pytest.raises(SyntaxError):
+            Toolkit.validate_file(invalid_file)
+
+
+class TestValidPath:
+    """Test the valid_path function for path validation during deployment and serving."""
+
+    @pytest.mark.parametrize(
+        "path_input",
+        [
+            # Simple valid paths
+            "file.py",
+            "module.py",
+            "utils.py",
+            "main.py",
+            "README.md",
+            "config.json",
+            # Valid nested paths
+            "src/main.py",
+            "lib/utils/helper.py",
+            "package/subpackage/module.py",
+            "tools/scripts/deploy.py",
+            "docs/api/reference.md",
+            "tests/unit/test_module.py",
+            # Deep nested paths
+            "very/deep/nested/directory/structure/file.py",
+            "a/b/c/d/e/f/g/module.py",
+            # Edge cases
+            "",  # Path("") creates current directory path "."
+            "a",
+            "1",
+            # Files containing but not matching restricted patterns
+            "my_dist_file.py",  # contains "dist" but doesn't match exactly
+            "build_utils.py",  # contains "build" but doesn't match exactly
+            "lockfile.py",  # contains "lock" but doesn't end with .lock
+            "unlock.py",  # contains "lock" but doesn't end with .lock
+            # Case sensitivity - these should be valid because case doesn't match exactly
+            "DIST/file.py",  # "DIST" != "dist"
+            "Build/file.py",  # "Build" != "build"
+            "VENV/file.py",  # "VENV" != "venv"
+            "Package.LOCK",  # ".LOCK" != ".lock"
+            # Windows-style paths
+            "src\\module.py",
+            "lib\\utils\\helper.py",
+            # Absolute paths that should be valid
+            "/home/user/project/file.py",
+            # Unicode and special characters
+            "файл.py",  # Cyrillic
+            "文件.py",  # Chinese
+            "módulo.py",  # Accented characters
+            "file with spaces.py",
+            "file-with-dashes.py",
+            "file_with_underscores.py",
+            # Path objects
+            Path("file.py"),
+            Path("src/module.py"),
+        ],
+        ids=[
+            "simple_file_py",
+            "simple_module_py",
+            "simple_utils_py",
+            "simple_main_py",
+            "simple_readme_md",
+            "simple_config_json",
+            "nested_src_main",
+            "nested_lib_utils",
+            "nested_package_subpackage",
+            "nested_tools_scripts",
+            "nested_docs_api",
+            "nested_tests_unit",
+            "deep_nested_very_deep",
+            "deep_nested_a_b_c",
+            "edge_case_empty_string",
+            "edge_case_single_a",
+            "edge_case_single_1",
+            "contains_dist_but_valid",
+            "contains_build_but_valid",
+            "contains_lock_but_valid",
+            "contains_unlock_valid",
+            "case_sensitive_DIST",
+            "case_sensitive_Build",
+            "case_sensitive_VENV",
+            "case_sensitive_LOCK",
+            "windows_src_module",
+            "windows_lib_utils",
+            "absolute_home_user",
+            "unicode_cyrillic",
+            "unicode_chinese",
+            "unicode_accented",
+            "spaces_in_filename",
+            "dashes_in_filename",
+            "underscores_in_filename",
+            "path_object_file",
+            "path_object_nested",
+        ],
+    )
+    def test_valid_paths(self, path_input):
+        """Test that valid paths are accepted."""
+        assert valid_path(path_input) is True
+
+    @pytest.mark.parametrize(
+        "path_input",
+        [
+            # Hidden files (starting with .)
+            ".hidden",
+            ".gitignore",
+            ".env",
+            ".DS_Store",
+            ".vscode",
+            ".pytest_cache",
+            # Paths containing hidden directories
+            ".hidden/file.py",
+            "src/.cache/data.py",
+            "lib/.git/config",
+            ".vscode/settings.json",
+            "deep/.hidden/nested/file.py",
+            "normal/.DS_Store",
+            # Excluded directories (exact matches)
+            "dist",
+            "build",
+            "__pycache__",
+            "venv",
+            "coverage.xml",
+            # Paths containing excluded directories
+            "dist/bundle.js",
+            "build/output/file.py",
+            "src/__pycache__/module.cpython-39.pyc",
+            "venv/lib/python3.9/site-packages/module.py",
+            "project/build/artifacts/file.py",
+            "lib/dist/package/module.py",
+            "tools/venv/bin/python",
+            # Lock files (ending with .lock)
+            "package.lock",
+            "poetry.lock",
+            "requirements.lock",
+            "Pipfile.lock",
+            "yarn.lock",
+            "npm.lock",
+            "custom.lock",
+            # Lock files in nested paths
+            "src/package.lock",
+            "frontend/yarn.lock",
+            "backend/poetry.lock",
+            "deep/nested/path/file.lock",
+            # Multiple violations
+            ".hidden/dist/file.py",  # hidden + excluded dir
+            "build/.cache/file.py",  # excluded dir + hidden
+            "venv/package.lock",  # excluded dir + lock file
+            ".git/hooks/file.lock",  # hidden + lock file
+            # Windows-style paths that should be invalid
+            "dist\\bundle.js",
+            ".hidden\\file.py",
+            "src\\package.lock",
+            # Absolute paths that should be invalid
+            "/home/user/project/.hidden/file.py",
+            "/home/user/project/dist/file.py",
+            "/opt/project/package.lock",
+            # Unicode with exclusion patterns
+            ".файл",  # Hidden Cyrillic file
+            "文件.lock",  # Chinese lock file
+            # Path objects
+            Path(".hidden/file.py"),
+            Path("dist/bundle.js"),
+            Path("package.lock"),
+        ],
+    )
+    def test_invalid_paths(self, path_input):
+        """Test that invalid paths are rejected."""
+        assert valid_path(path_input) is False

pyproject.toml

@@ -21,7 +21,7 @@ requires-python = ">=3.10"
 
 dependencies = [
     # CLI dependencies
-    "arcade-core>=2.0.0,<3.0.0",
+    "arcade-core>=2.2.1,<3.0.0",
     "typer==0.10.0",
     "rich==13.9.4",
     "Jinja2==3.1.6",