From 86cbf38ed3157aa9668f3a392daeeaf97d908fb2 Mon Sep 17 00:00:00 2001 From: Edward Park Date: Fri, 7 Aug 2026 23:49:32 -0700 Subject: [PATCH] fix: harden Horizon state boundaries --- .../fastmcp/cli/deploy/configuration.py | 15 ++--- .../fastmcp/cli/deploy/credentials.py | 25 +++++++- .../fastmcp/cli/deploy/horizon_client.py | 4 ++ fastmcp_slim/fastmcp/cli/deploy/state.py | 61 ++++++++++++++++++- tests/cli/deploy/test_credentials.py | 51 ++++++++++++++++ tests/cli/deploy/test_horizon_client.py | 2 + 6 files changed, 147 insertions(+), 11 deletions(-) diff --git a/fastmcp_slim/fastmcp/cli/deploy/configuration.py b/fastmcp_slim/fastmcp/cli/deploy/configuration.py index 9f2f41ca1..750bb8e83 100644 --- a/fastmcp_slim/fastmcp/cli/deploy/configuration.py +++ b/fastmcp_slim/fastmcp/cli/deploy/configuration.py @@ -12,7 +12,7 @@ from fastmcp.cli.deploy.horizon_client import ( DEFAULT_HORIZON_API_ORIGIN, normalize_api_origin, ) -from fastmcp.cli.deploy.state import read_state, write_state +from fastmcp.cli.deploy.state import read_state, state_lock, write_state class HorizonConfiguration(BaseModel): @@ -59,9 +59,10 @@ class ConfigurationStore: credentials: CredentialStore, ) -> HorizonConfiguration: """Set the origin and clear credentials before an origin change.""" - current = self.load() - updated = HorizonConfiguration(schemaVersion=1, apiOrigin=api_origin) - if updated.api_origin != current.api_origin: - credentials.clear() - self.save(updated) - return updated + with state_lock(self.path.parent): + current = self.load() + updated = HorizonConfiguration(schemaVersion=1, apiOrigin=api_origin) + if updated.api_origin != current.api_origin: + credentials.clear() + self.save(updated) + return updated diff --git a/fastmcp_slim/fastmcp/cli/deploy/credentials.py b/fastmcp_slim/fastmcp/cli/deploy/credentials.py index c70dbd3f8..bd129abee 100644 --- a/fastmcp_slim/fastmcp/cli/deploy/credentials.py +++ b/fastmcp_slim/fastmcp/cli/deploy/credentials.py @@ -17,11 +17,12 @@ from pydantic import ( field_validator, ) -from fastmcp.cli.deploy.horizon_client import HorizonClient +from fastmcp.cli.deploy.horizon_client import HorizonClient, normalize_api_origin from fastmcp.cli.deploy.state import ( StateFileError, read_state, remove_state, + state_lock, write_state, ) @@ -79,6 +80,22 @@ class CredentialStore: }, ) + def save_for_origin( + self, + api_key: SecretStr | str, + *, + expected_api_origin: str, + ) -> None: + """Save a key only while its issuing Horizon origin is active.""" + from fastmcp.cli.deploy.configuration import ConfigurationStore + + expected_api_origin = normalize_api_origin(expected_api_origin) + with state_lock(self.path.parent): + active_api_origin = ConfigurationStore(self.path.parent).load().api_origin + if active_api_origin != expected_api_origin: + raise StateFileError("The Horizon host changed during login") + self.save(api_key) + def clear(self) -> None: remove_state(self.path) @@ -88,6 +105,7 @@ async def resolve_credential( *, environ: Mapping[str, str] | None = None, authorize: Callable[[], Awaitable[SecretStr]] | None = None, + expected_api_origin: str | None = None, ) -> ResolvedCredential: """Resolve environment, stored, then interactive credentials.""" environ = os.environ if environ is None else environ @@ -106,7 +124,10 @@ async def resolve_credential( raise AuthenticationRequiredError("Horizon authentication is required") api_key = await authorize() - store.save(api_key) + if expected_api_origin is None: + store.save(api_key) + else: + store.save_for_origin(api_key, expected_api_origin=expected_api_origin) return ResolvedCredential(api_key=api_key, source="interactive") diff --git a/fastmcp_slim/fastmcp/cli/deploy/horizon_client.py b/fastmcp_slim/fastmcp/cli/deploy/horizon_client.py index f3f01553c..26a66a10f 100644 --- a/fastmcp_slim/fastmcp/cli/deploy/horizon_client.py +++ b/fastmcp_slim/fastmcp/cli/deploy/horizon_client.py @@ -129,6 +129,10 @@ class DeviceTokenPoll: def normalize_api_origin(value: str) -> str: """Validate and normalize a Horizon API origin.""" parts = urlsplit(value) + try: + _ = parts.port + except ValueError: + raise ValueError("The Horizon API origin must be an HTTP origin") from None if ( parts.scheme not in {"http", "https"} or not parts.hostname diff --git a/fastmcp_slim/fastmcp/cli/deploy/state.py b/fastmcp_slim/fastmcp/cli/deploy/state.py index c037ce450..55cad4d74 100644 --- a/fastmcp_slim/fastmcp/cli/deploy/state.py +++ b/fastmcp_slim/fastmcp/cli/deploy/state.py @@ -2,11 +2,13 @@ from __future__ import annotations +import errno import json import os import subprocess import tempfile -from contextlib import suppress +from collections.abc import Iterator +from contextlib import contextmanager, suppress from pathlib import Path from typing import Any, TypeVar @@ -90,6 +92,56 @@ def _prepare_directory(path: Path) -> None: _restrict_access(path, directory=True) +@contextmanager +def state_lock(directory: Path) -> Iterator[None]: + """Lock related CLI state changes across processes.""" + _prepare_directory(directory) + lock_path = directory / ".state.lock" + if lock_path.is_symlink(): + raise StateFileError("The CLI state lock must not be a symbolic link") + + lock_file = None + try: + lock_file = lock_path.open("a+b") + _restrict_access(lock_path) + if os.name == "nt": + import msvcrt + + if lock_path.stat().st_size == 0: + lock_file.write(b"\0") + lock_file.flush() + lock_file.seek(0) + msvcrt.locking(lock_file.fileno(), msvcrt.LK_LOCK, 1) + else: + import fcntl + + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + except (OSError, StateFileError) as exc: + if lock_file is not None: + with suppress(OSError): + lock_file.close() + if isinstance(exc, StateFileError): + raise + raise StateFileError("Could not lock CLI state") from exc + + try: + yield + finally: + if os.name == "nt": + import msvcrt + + with suppress(OSError): + lock_file.seek(0) + msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1) + else: + import fcntl + + with suppress(OSError): + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + with suppress(OSError): + lock_file.close() + + def read_state( path: Path, model: type[ModelT], @@ -145,7 +197,12 @@ def write_state(path: Path, data: dict[str, Any]) -> None: if os.name != "nt": directory_descriptor = os.open(path.parent, os.O_RDONLY) try: - os.fsync(directory_descriptor) + try: + os.fsync(directory_descriptor) + except OSError as exc: + unsupported = {errno.EINVAL, errno.ENOTSUP} + if exc.errno not in unsupported: + raise finally: os.close(directory_descriptor) except StateFileError: diff --git a/tests/cli/deploy/test_credentials.py b/tests/cli/deploy/test_credentials.py index a4ea55b45..23e98ca62 100644 --- a/tests/cli/deploy/test_credentials.py +++ b/tests/cli/deploy/test_credentials.py @@ -1,7 +1,9 @@ from __future__ import annotations +import errno import json import os +import stat import subprocess import traceback from pathlib import Path @@ -11,6 +13,10 @@ import httpx2 import pytest from pydantic import SecretStr +from fastmcp.cli.deploy.configuration import ( + ConfigurationStore, + HorizonConfiguration, +) from fastmcp.cli.deploy.credentials import ( AuthenticationRequiredError, CredentialStore, @@ -63,6 +69,26 @@ def test_credential_store_restricts_an_existing_secret_file(tmp_path: Path) -> N assert store.path.stat().st_mode & 0o777 == 0o600 +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory fsync") +def test_atomic_write_ignores_unsupported_directory_fsync( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + store = CredentialStore(tmp_path) + original_fsync = os.fsync + + def fsync(descriptor: int) -> None: + if stat.S_ISDIR(os.fstat(descriptor).st_mode): + raise OSError(errno.EINVAL, "directory sync is not supported") + original_fsync(descriptor) + + monkeypatch.setattr("fastmcp.cli.deploy.state.os.fsync", fsync) + + store.save("fmcp_secret") + + assert load_secret(store).get_secret_value() == "fmcp_secret" + + def test_atomic_write_preserves_previous_state_on_replace_failure( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -132,6 +158,31 @@ async def test_interactive_credential_is_persisted(tmp_path: Path) -> None: assert load_secret(store).get_secret_value() == "fmcp_interactive" +async def test_interactive_credential_rejects_an_origin_change( + tmp_path: Path, +) -> None: + store = CredentialStore(tmp_path) + ConfigurationStore(tmp_path).save( + HorizonConfiguration( + schemaVersion=1, + apiOrigin="https://dev.horizon.prefect.io", + ) + ) + + async def authorize() -> SecretStr: + return SecretStr("fmcp_old_origin") + + with pytest.raises(StateFileError, match="host changed"): + await resolve_credential( + store, + environ={}, + authorize=authorize, + expected_api_origin="https://horizon.prefect.io", + ) + + assert store.load() is None + + async def test_missing_noninteractive_credential_is_explicit(tmp_path: Path) -> None: with pytest.raises(AuthenticationRequiredError): await resolve_credential(CredentialStore(tmp_path), environ={}) diff --git a/tests/cli/deploy/test_horizon_client.py b/tests/cli/deploy/test_horizon_client.py index a0f065400..aa0e4dee0 100644 --- a/tests/cli/deploy/test_horizon_client.py +++ b/tests/cli/deploy/test_horizon_client.py @@ -249,6 +249,8 @@ async def test_invalid_responses_do_not_include_response_bodies() -> None: "https://user@example.com", "https://horizon.prefect.io/path", "https://horizon.prefect.io?query=value", + "https://horizon.prefect.io:abc", + "https://horizon.prefect.io:99999", ], ) def test_api_origin_rejects_values_that_are_not_origins(value: str) -> None: