From 75df48f09eeee39bd7466d89238895cc41ebfb2a Mon Sep 17 00:00:00 2001 From: Aaron Perez Date: Mon, 6 Jul 2026 20:13:38 -0500 Subject: [PATCH] fix(SKY-11917): validate script deploy file paths (#7145) --- skyvern/cli/mcp_tools/scripts.py | 8 +- skyvern/forge/sdk/artifact/storage/azure.py | 8 +- skyvern/forge/sdk/artifact/storage/gcs.py | 8 +- skyvern/forge/sdk/artifact/storage/local.py | 8 +- skyvern/forge/sdk/artifact/storage/s3.py | 8 +- skyvern/schemas/scripts.py | 9 +- .../services/cached_script_deploy_service.py | 10 ++- skyvern/utils/script_file_paths.py | 49 +++++++++++ .../unit/test_cached_script_deploy_service.py | 14 +++- tests/unit/test_mcp_script_caching_live.py | 21 +++++ tests/unit/test_script_file_paths.py | 82 +++++++++++++++++++ 11 files changed, 212 insertions(+), 13 deletions(-) create mode 100644 skyvern/utils/script_file_paths.py create mode 100644 tests/unit/test_script_file_paths.py diff --git a/skyvern/cli/mcp_tools/scripts.py b/skyvern/cli/mcp_tools/scripts.py index b9259e00a..2e4aa8cc2 100644 --- a/skyvern/cli/mcp_tools/scripts.py +++ b/skyvern/cli/mcp_tools/scripts.py @@ -15,6 +15,7 @@ from pydantic import Field, ValidationError from skyvern.client.errors import NotFoundError from skyvern.client.types import ScriptFileCreate +from skyvern.utils.script_file_paths import normalize_script_file_path from ._common import ErrorCode, Timer, make_error, make_result, raw_http_get from ._session import get_skyvern @@ -247,8 +248,11 @@ async def skyvern_script_deploy( parsed_files = json.loads(files) if not isinstance(parsed_files, list): raise ValueError("files must be a JSON array") - typed_files = [ScriptFileCreate(**file_data) for file_data in parsed_files] - except (json.JSONDecodeError, TypeError, ValueError, ValidationError) as e: + typed_files = [ + ScriptFileCreate(**{**file_data, "path": normalize_script_file_path(file_data["path"])}) + for file_data in parsed_files + ] + except (json.JSONDecodeError, TypeError, ValueError, ValidationError, KeyError) as e: return make_result( "skyvern_script_deploy", ok=False, diff --git a/skyvern/forge/sdk/artifact/storage/azure.py b/skyvern/forge/sdk/artifact/storage/azure.py index 1495895bd..e4d0d1d6a 100644 --- a/skyvern/forge/sdk/artifact/storage/azure.py +++ b/skyvern/forge/sdk/artifact/storage/azure.py @@ -33,6 +33,7 @@ from skyvern.forge.sdk.schemas.ai_suggestions import AISuggestion from skyvern.forge.sdk.schemas.files import FileInfo from skyvern.forge.sdk.schemas.task_v2 import TaskV2, Thought from skyvern.forge.sdk.schemas.workflow_runs import WorkflowRunBlock +from skyvern.utils.script_file_paths import build_script_file_storage_uri LOG = structlog.get_logger() @@ -118,7 +119,12 @@ class AzureStorage(BaseStorage): self, *, organization_id: str, script_id: str, script_version: int, file_path: str ) -> str: """Build the Azure URI for a script file.""" - return f"{self._build_base_uri(organization_id)}/scripts/{script_id}/{script_version}/{file_path}" + return build_script_file_storage_uri( + self._build_base_uri(organization_id), + script_id=script_id, + script_version=script_version, + file_path=file_path, + ) async def store_artifact(self, artifact: Artifact, data: bytes) -> None: tier = await self._get_storage_tier_for_org(artifact.organization_id) diff --git a/skyvern/forge/sdk/artifact/storage/gcs.py b/skyvern/forge/sdk/artifact/storage/gcs.py index d8c1553f2..febb30f0a 100644 --- a/skyvern/forge/sdk/artifact/storage/gcs.py +++ b/skyvern/forge/sdk/artifact/storage/gcs.py @@ -38,6 +38,7 @@ from skyvern.forge.sdk.schemas.ai_suggestions import AISuggestion from skyvern.forge.sdk.schemas.files import FileInfo from skyvern.forge.sdk.schemas.task_v2 import TaskV2, Thought from skyvern.forge.sdk.schemas.workflow_runs import WorkflowRunBlock +from skyvern.utils.script_file_paths import build_script_file_storage_uri LOG = structlog.get_logger() @@ -122,7 +123,12 @@ class GcsStorage(BaseStorage): self, *, organization_id: str, script_id: str, script_version: int, file_path: str ) -> str: """Build the GCS URI for a script file.""" - return f"{self._build_base_uri(organization_id)}/scripts/{script_id}/{script_version}/{file_path}" + return build_script_file_storage_uri( + self._build_base_uri(organization_id), + script_id=script_id, + script_version=script_version, + file_path=file_path, + ) async def store_artifact(self, artifact: Artifact, data: bytes) -> None: storage_class = await self._get_storage_class_for_org(artifact.organization_id) diff --git a/skyvern/forge/sdk/artifact/storage/local.py b/skyvern/forge/sdk/artifact/storage/local.py index aa0db2ec1..29339a14c 100644 --- a/skyvern/forge/sdk/artifact/storage/local.py +++ b/skyvern/forge/sdk/artifact/storage/local.py @@ -22,6 +22,7 @@ from skyvern.forge.sdk.schemas.ai_suggestions import AISuggestion from skyvern.forge.sdk.schemas.files import FileInfo from skyvern.forge.sdk.schemas.task_v2 import TaskV2, Thought from skyvern.forge.sdk.schemas.workflow_runs import WorkflowRunBlock +from skyvern.utils.script_file_paths import build_script_file_storage_uri from skyvern.webeye.session_cookies import SESSION_COOKIES_FILENAME LOG = structlog.get_logger() @@ -122,7 +123,12 @@ class LocalStorage(BaseStorage): def build_script_file_uri( self, *, organization_id: str, script_id: str, script_version: int, file_path: str ) -> str: - return f"file://{self.artifact_path}/{settings.ENV}/{organization_id}/scripts/{script_id}/{script_version}/{file_path}" + return build_script_file_storage_uri( + f"file://{self.artifact_path}/{settings.ENV}/{organization_id}", + script_id=script_id, + script_version=script_version, + file_path=file_path, + ) async def store_artifact(self, artifact: Artifact, data: bytes) -> None: file_path = None diff --git a/skyvern/forge/sdk/artifact/storage/s3.py b/skyvern/forge/sdk/artifact/storage/s3.py index a9d809091..b3a1d2711 100644 --- a/skyvern/forge/sdk/artifact/storage/s3.py +++ b/skyvern/forge/sdk/artifact/storage/s3.py @@ -41,6 +41,7 @@ from skyvern.forge.sdk.schemas.ai_suggestions import AISuggestion from skyvern.forge.sdk.schemas.files import FileInfo from skyvern.forge.sdk.schemas.task_v2 import TaskV2, Thought from skyvern.forge.sdk.schemas.workflow_runs import WorkflowRunBlock +from skyvern.utils.script_file_paths import build_script_file_storage_uri from skyvern.webeye.video_utils import prepare_recording_for_upload LOG = structlog.get_logger() @@ -147,7 +148,12 @@ class S3Storage(BaseStorage): Returns: The S3 URI for the script file """ - return f"{self._build_base_uri(organization_id)}/scripts/{script_id}/{script_version}/{file_path}" + return build_script_file_storage_uri( + self._build_base_uri(organization_id), + script_id=script_id, + script_version=script_version, + file_path=file_path, + ) async def store_artifact(self, artifact: Artifact, data: bytes) -> None: # We compress HAR files with zstd level 3 to reduce storage size. diff --git a/skyvern/schemas/scripts.py b/skyvern/schemas/scripts.py index 7dbfe6780..e3acc947b 100644 --- a/skyvern/schemas/scripts.py +++ b/skyvern/schemas/scripts.py @@ -5,7 +5,9 @@ from datetime import datetime from enum import StrEnum from typing import Literal -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator + +from skyvern.utils.script_file_paths import normalize_script_file_path class FileEncoding(StrEnum): @@ -50,6 +52,11 @@ class ScriptFileCreate(BaseModel): encoding: FileEncoding = Field(default=FileEncoding.BASE64, description="Content encoding") mime_type: str | None = Field(default=None, description="MIME type (auto-detected if not provided)") + @field_validator("path") + @classmethod + def validate_path(cls, value: str) -> str: + return normalize_script_file_path(value) + class CreateScriptRequest(BaseModel): workflow_id: str | None = Field(default=None, description="Associated workflow ID") diff --git a/skyvern/services/cached_script_deploy_service.py b/skyvern/services/cached_script_deploy_service.py index 1c6619bcb..480d2ce65 100644 --- a/skyvern/services/cached_script_deploy_service.py +++ b/skyvern/services/cached_script_deploy_service.py @@ -26,6 +26,7 @@ from skyvern.schemas.scripts import ( WorkflowScript, ) from skyvern.services.workflow_script_service import CacheKeyResolutionError, resolve_cache_key_value +from skyvern.utils.script_file_paths import SCRIPT_FILE_PATH_ERROR, normalize_script_file_path _CODE_VERSION_STATIC = 1 _CODE_VERSION_ADAPTIVE = 2 @@ -87,12 +88,13 @@ def _decode_script_file_bytes(file: ScriptFileCreate) -> bytes: def _validate_script_file_path(file_path: str) -> None: - parts = file_path.split("/") - if file_path.startswith("/") or "\\" in file_path or any(part in ("", ".", "..") for part in parts): + try: + normalize_script_file_path(file_path) + except ValueError as exc: raise HTTPException( status_code=400, - detail=f"File path {file_path!r} must be a relative POSIX path without empty, '.', or '..' segments", - ) + detail=f"File path {file_path!r} is invalid: {SCRIPT_FILE_PATH_ERROR}", + ) from exc def _validate_script_files(files: list[ScriptFileCreate]) -> list[_ValidatedScriptFile]: diff --git a/skyvern/utils/script_file_paths.py b/skyvern/utils/script_file_paths.py new file mode 100644 index 000000000..5c5159b51 --- /dev/null +++ b/skyvern/utils/script_file_paths.py @@ -0,0 +1,49 @@ +from pathlib import PurePosixPath, PureWindowsPath +from urllib.parse import unquote + +SCRIPT_FILE_PATH_ERROR = ( + "Script file path must be a relative POSIX path without empty, '.', '..', absolute, " + "drive-qualified, backslash, or null-byte segments" +) +_MAX_SCRIPT_FILE_PATH_DECODE_ITERATIONS = 16 + + +def normalize_script_file_path(file_path: str) -> str: + normalized = _decode_storage_path(file_path) + if ( + not normalized + or "\x00" in normalized + or "\\" in normalized + or normalized.startswith(("/", "\\")) + or PurePosixPath(normalized).is_absolute() + or PureWindowsPath(normalized).drive + ): + raise ValueError(SCRIPT_FILE_PATH_ERROR) + + parts = normalized.split("/") + if any(part in ("", ".", "..") for part in parts): + raise ValueError(SCRIPT_FILE_PATH_ERROR) + return "/".join(parts) + + +def build_script_file_storage_uri( + storage_base_uri: str, + *, + script_id: str, + script_version: int, + file_path: str, +) -> str: + normalized_file_path = normalize_script_file_path(file_path) + return f"{storage_base_uri.rstrip('/')}/scripts/{script_id}/{script_version}/{normalized_file_path}" + + +def _decode_storage_path(file_path: str) -> str: + decoded = file_path + for _ in range(_MAX_SCRIPT_FILE_PATH_DECODE_ITERATIONS): + next_decoded = unquote(decoded) + if next_decoded == decoded: + return decoded + decoded = next_decoded + if unquote(decoded) != decoded: + raise ValueError(SCRIPT_FILE_PATH_ERROR) + return decoded diff --git a/tests/unit/test_cached_script_deploy_service.py b/tests/unit/test_cached_script_deploy_service.py index c4a19aeb8..9d3a0f0e2 100644 --- a/tests/unit/test_cached_script_deploy_service.py +++ b/tests/unit/test_cached_script_deploy_service.py @@ -726,7 +726,17 @@ async def run(parameters): @pytest.mark.asyncio @pytest.mark.parametrize( "file_path", - ["../outside.py", "/main.py", "dir//main.py", "dir/../main.py", "dir\\main.py", "./main.py"], + [ + "../outside.py", + "/main.py", + "dir//main.py", + "dir/../main.py", + "dir\\main.py", + "./main.py", + "%2e%2e/outside.py", + "main.py%00.txt", + "C:/main.py", + ], ) async def test_dry_run_rejects_unsafe_file_paths(file_path: str) -> None: source = """ @@ -742,7 +752,7 @@ async def run(parameters): encoding=FileEncoding.BASE64, mime_type="text/x-python", ), - ScriptFileCreate( + ScriptFileCreate.model_construct( path=file_path, content=_b64("x = 1\n"), encoding=FileEncoding.BASE64, diff --git a/tests/unit/test_mcp_script_caching_live.py b/tests/unit/test_mcp_script_caching_live.py index 21d940df6..09b6460c2 100644 --- a/tests/unit/test_mcp_script_caching_live.py +++ b/tests/unit/test_mcp_script_caching_live.py @@ -336,6 +336,27 @@ async def test_deploy_script_via_mcp(monkeypatch): assert called_files[0].path == "main.py" +@pytest.mark.asyncio +async def test_deploy_script_rejects_unsafe_file_path_via_mcp(monkeypatch): + fake_client = SimpleNamespace(deploy_script=AsyncMock()) + monkeypatch.setattr(script_tools, "get_skyvern", lambda: fake_client) + + files = json.dumps([{"path": "%2e%2e/main.py", "content": "ZA==", "encoding": "base64"}]) + + async with Client(mcp) as client: + result = await client.call_tool( + "skyvern_script_deploy", + { + "script_id": "s_abc", + "files": files, + }, + ) + + assert result.data["ok"] is False + assert "relative POSIX path" in result.data["error"]["message"] + fake_client.deploy_script.assert_not_awaited() + + # --------------------------------------------------------------------------- # Scenario 6: Workflow create surfaces caching fields when caller opts in # --------------------------------------------------------------------------- diff --git a/tests/unit/test_script_file_paths.py b/tests/unit/test_script_file_paths.py new file mode 100644 index 000000000..5e28dfa32 --- /dev/null +++ b/tests/unit/test_script_file_paths.py @@ -0,0 +1,82 @@ +from urllib.parse import quote + +import pytest +from pydantic import ValidationError + +from skyvern.schemas.scripts import FileEncoding, ScriptFileCreate +from skyvern.utils.script_file_paths import build_script_file_storage_uri, normalize_script_file_path + + +def _script_file(path: str) -> ScriptFileCreate: + return ScriptFileCreate(path=path, content="ZA==", encoding=FileEncoding.BASE64) + + +@pytest.mark.parametrize( + "file_path", + [ + "", + "../outside.py", + "src/../outside.py", + "/main.py", + "\\main.py", + "C:/main.py", + "src//main.py", + "src/./main.py", + "src\\main.py", + "main.py\x00.txt", + "main.py%00.txt", + "main.py%2500.txt", + "%2e%2e/outside.py", + "src/%2e%2e/outside.py", + "%2Fmain.py", + "src%5Cmain.py", + ], +) +def test_script_file_create_rejects_unsafe_paths_at_input_time(file_path: str) -> None: + with pytest.raises(ValidationError) as exc: + _script_file(file_path) + + assert "relative POSIX path" in str(exc.value) + + +def test_script_file_create_normalizes_encoded_storage_path() -> None: + script_file = _script_file("src%2Fmain.py") + + assert script_file.path == "src/main.py" + + +def test_script_file_create_rejects_deeply_encoded_traversal_path() -> None: + file_path = "%2e%2e/outside.py" + for _ in range(6): + file_path = quote(file_path, safe="") + + with pytest.raises(ValidationError) as exc: + _script_file(file_path) + + assert "relative POSIX path" in str(exc.value) + + +def test_build_script_file_storage_uri_pins_normalized_path_under_base() -> None: + uri = build_script_file_storage_uri( + "file:///tmp/artifacts/local/org/", + script_id="s_test", + script_version=2, + file_path="src%2Fmain.py", + ) + + assert uri == "file:///tmp/artifacts/local/org/scripts/s_test/2/src/main.py" + + +@pytest.mark.parametrize("file_path", ["../main.py", "%2e%2e/main.py", "main.py%00.txt", "C:/main.py"]) +def test_build_script_file_storage_uri_rejects_unsafe_paths(file_path: str) -> None: + with pytest.raises(ValueError): + build_script_file_storage_uri( + "s3://bucket/v1/local/org", + script_id="s_test", + script_version=2, + file_path=file_path, + ) + + +def test_normalize_script_file_path_preserves_safe_posix_path() -> None: + assert normalize_script_file_path("src/helpers/main.py") == "src/helpers/main.py"