Merge 0134b3bcce into dfdbe4b2ab

strix/interface/utils.py

@@ -20,7 +20,8 @@ from rich.text import Text
 
 from strix.config import load_settings
 from strix.telemetry import report_error
-from strix.utils.api_spec import detect_spec_format
+from strix.utils.api_spec import detect_spec_format, validate_postman_uid
+from strix.utils.secret_files import write_secret_text
 
 
 logger = logging.getLogger(__name__)
@@ -1149,6 +1150,10 @@ def infer_target_type(target: str) -> tuple[str, dict[str, str]]:  # noqa: PLR09
             raise ValueError(
                 f"Missing Postman collection id in '{target}' (expected postman://<collection-uid>)"
             )
+        # Reject here too, not just inside fetch_postman_collection: a malformed
+        # id (e.g. containing "..") must never reach the point of being turned
+        # into an HTTP request against the Postman API with a real API key.
+        validate_postman_uid(collection_uid, "collection")
         details = {
             "target_spec": target,
             "spec_format": "postman",
@@ -1158,6 +1163,7 @@ def infer_target_type(target: str) -> tuple[str, dict[str, str]]:  # noqa: PLR09
         query = parse_qs(parsed.query)
         env_uid = (query.get("env") or query.get("environment") or [""])[0].strip()
         if env_uid:
+            validate_postman_uid(env_uid, "environment")
             details["environment_uid"] = env_uid
         return "api_spec", details
 
@@ -1508,9 +1514,11 @@ def write_fetched_collection(collection: dict[str, Any], collection_uid: str) ->
     spec file from here on and the API key never leaves the host.
     """
     staging = Path(tempfile.gettempdir()) / "strix_api_specs" / "fetched"
-    staging.mkdir(parents=True, exist_ok=True)
     path = staging / f"{sanitize_name(collection_uid)}.postman_collection.json"
-    path.write_text(json.dumps(collection, indent=2), encoding="utf-8")
+    # A fetched collection can carry auth headers/tokens saved in Postman requests;
+    # write it 0600 like the other secret-bearing files in this codebase instead of
+    # the default umask (world-readable on a shared host).
+    write_secret_text(path, json.dumps(collection, indent=2))
     return str(path)
 
 

strix/utils/api_spec.py

@@ -234,6 +234,25 @@ def spec_base_urls(
 POSTMAN_API_BASE = "https://api.getpostman.com"
 _POSTMAN_FETCH_TIMEOUT = 30
 
+# Postman collection/environment uids are `{ownerId}-{uuid}` (digits, hex, hyphens).
+# collection_uid/environment_uid ultimately come from a user-supplied `postman://`
+# target (see interface/utils.py) with no upstream format check. requests collapses
+# `..` in a URL path before the request is sent, so an unvalidated uid containing
+# `../..` turns `GET /collections/{uid}` into a confused-deputy request against an
+# arbitrary Postman API endpoint (e.g. `/workspaces`, `/users/me`), authenticated
+# with the caller's own POSTMAN_API_KEY. Enforced here too (not just at the target
+# parser) so this stays safe regardless of caller.
+_POSTMAN_UID_PATTERN = re.compile(r"^[A-Za-z0-9-]+$")
+
+
+def validate_postman_uid(uid: str, label: str) -> None:
+    if not _POSTMAN_UID_PATTERN.fullmatch(uid):
+        raise SpecParseError(
+            f"Invalid Postman {label} id {uid!r}: expected only letters, digits, "
+            "and hyphens (a real Postman collection/environment id never contains "
+            "'/', '.', or other characters).",
+        )
+
 
 def _postman_api_json(url: str, api_key: str, label: str) -> dict[str, Any]:
     """GET a Postman API resource and return the parsed JSON payload.
@@ -279,6 +298,7 @@ def fetch_postman_collection(collection_uid: str, api_key: str) -> dict[str, Any
     wraps the collection under a ``collection`` key, unwrapped here so the result
     matches an exported collection file.
     """
+    validate_postman_uid(collection_uid, "collection")
     payload = _postman_api_json(
         f"{POSTMAN_API_BASE}/collections/{collection_uid}",
         api_key,
@@ -296,6 +316,7 @@ def fetch_postman_environment(environment_uid: str, api_key: str) -> dict[str, s
     Disabled values are skipped, matching how Postman resolves an environment at
     request time.
     """
+    validate_postman_uid(environment_uid, "environment")
     payload = _postman_api_json(
         f"{POSTMAN_API_BASE}/environments/{environment_uid}",
         api_key,

tests/test_api_spec.py

@@ -234,6 +234,32 @@ def test_fetch_postman_missing_key_raises() -> None:
         fetch_postman_collection("abc-123", "")
 
 
+def test_fetch_postman_collection_rejects_path_traversal_uid(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    # Security regression: requests collapses ".." in a URL path before sending
+    # the request, so this used to turn into a confused-deputy GET against an
+    # arbitrary Postman API endpoint, authenticated with the caller's API key.
+    # Assert requests.get is never even called -- rejected before any network I/O.
+    def fail_if_called(*_a: Any, **_k: Any) -> _FakeResponse:
+        raise AssertionError("requests.get must not be called for an invalid uid")
+
+    monkeypatch.setattr(requests, "get", fail_if_called)
+    with pytest.raises(SpecParseError, match="Invalid Postman collection id"):
+        fetch_postman_collection("x/../../workspaces", "PMAK-xyz")
+
+
+def test_fetch_postman_environment_rejects_path_traversal_uid(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    def fail_if_called(*_a: Any, **_k: Any) -> _FakeResponse:
+        raise AssertionError("requests.get must not be called for an invalid uid")
+
+    monkeypatch.setattr(requests, "get", fail_if_called)
+    with pytest.raises(SpecParseError, match="Invalid Postman environment id"):
+        fetch_postman_environment("../../users/me", "PMAK-xyz")
+
+
 def test_fetch_postman_404_raises(monkeypatch: pytest.MonkeyPatch) -> None:
     monkeypatch.setattr(requests, "get", lambda *_a, **_k: _FakeResponse(404, {}))
     with pytest.raises(SpecParseError, match="not found"):

tests/test_api_spec_targets.py

@@ -11,7 +11,7 @@ import pytest
 
 from strix.core.inputs import build_root_task, build_scope_context
 from strix.interface.scan_setup import build_targets_info
-from strix.interface.utils import infer_target_type, stage_api_specs
+from strix.interface.utils import infer_target_type, stage_api_specs, write_fetched_collection
 
 
 OPENAPI = {
@@ -78,6 +78,27 @@ def test_infer_target_type_rejects_empty_postman_uri() -> None:
         infer_target_type("postman://")
 
 
+def test_infer_target_type_rejects_path_traversal_in_postman_collection_uid() -> None:
+    # Security regression: requests collapses ".." in a URL path before sending
+    # the request, so an unvalidated collection_uid turned `GET
+    # /collections/{uid}` into a confused-deputy request against an arbitrary
+    # Postman API endpoint, authenticated with the caller's own POSTMAN_API_KEY.
+    with pytest.raises(ValueError, match="Invalid Postman collection id"):
+        infer_target_type("postman://x/../../workspaces")
+
+
+def test_infer_target_type_rejects_path_traversal_in_postman_environment_uid() -> None:
+    with pytest.raises(ValueError, match="Invalid Postman environment id"):
+        infer_target_type("postman://coll-uid?env=../../users/me")
+
+
+def test_infer_target_type_rejects_at_sign_in_postman_uid() -> None:
+    # Guards against a uid engineered to change the request's authority/host
+    # component if this ever ends up interpolated somewhere URL-relative.
+    with pytest.raises(ValueError, match="Invalid Postman collection id"):
+        infer_target_type("postman://evil.example@trusted.example")
+
+
 def test_infer_target_type_parses_postman_environment() -> None:
     _ttype, details = infer_target_type("postman://coll-uid?env=env-uid")
     assert details["collection_uid"] == "coll-uid"
@@ -89,6 +110,19 @@ def test_infer_target_type_postman_without_env_omits_key() -> None:
     assert "environment_uid" not in details
 
 
+def test_write_fetched_collection_is_not_world_readable() -> None:
+    # Security regression: a fetched Postman collection can carry saved auth
+    # headers/tokens. It used to be written with the default umask (typically
+    # world-readable on a shared host); it must be 0600 like the codebase's
+    # other secret-bearing files.
+    path_str = write_fetched_collection({"info": {"name": "x"}}, "world-readable-test-uid")
+    try:
+        mode = Path(path_str).stat().st_mode & 0o777
+        assert mode == 0o600, f"expected 0600, got {oct(mode)}"
+    finally:
+        Path(path_str).unlink(missing_ok=True)
+
+
 def test_build_targets_info_records_title_and_base_urls(tmp_path: Path) -> None:
     (target,) = _resolved_targets(_write_spec(tmp_path))
     assert target["details"]["spec_title"] == "Shop API"