mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-09 07:09:11 +02:00
fix: harden Horizon state boundaries
This commit is contained in:
parent
760bef1ab8
commit
86cbf38ed3
6 changed files with 147 additions and 11 deletions
|
|
@ -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,6 +59,7 @@ class ConfigurationStore:
|
|||
credentials: CredentialStore,
|
||||
) -> HorizonConfiguration:
|
||||
"""Set the origin and clear credentials before an origin change."""
|
||||
with state_lock(self.path.parent):
|
||||
current = self.load()
|
||||
updated = HorizonConfiguration(schemaVersion=1, apiOrigin=api_origin)
|
||||
if updated.api_origin != current.api_origin:
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
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")
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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],
|
||||
|
|
@ -144,8 +196,13 @@ def write_state(path: Path, data: dict[str, Any]) -> None:
|
|||
|
||||
if os.name != "nt":
|
||||
directory_descriptor = os.open(path.parent, os.O_RDONLY)
|
||||
try:
|
||||
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:
|
||||
|
|
|
|||
|
|
@ -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={})
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue