From 0134b3bcce112d584d89e975665e471549b49054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?FabioLeit=C3=A3o?= Date: Mon, 24 Aug 2026 13:43:09 -0300 Subject: [PATCH] fix(security): validate Postman collection/environment ids, block path traversal postman:// targets took collection_uid/environment_uid as raw netloc+path with no format check. requests collapses ".." in a URL path before sending the request, so fetch_postman_collection/fetch_postman_environment built `{POSTMAN_API_BASE}/collections/{uid}` (and /environments/{uid}) with an attacker-controlled string that could traverse to an arbitrary Postman API endpoint -- authenticated with the caller's own POSTMAN_API_KEY via X-Api-Key. Confirmed with requests.Request(...).prepare().url: 'https://api.getpostman.com/collections/x/../../workspaces' -> 'https://api.getpostman.com/workspaces' Also: a successfully-fetched collection was written with the default umask (world-readable on a shared host) even though it can carry saved auth headers/tokens, while this codebase already has write_secret_text (0600) for exactly this kind of data in three other call sites. Fix: - validate_postman_uid() (letters/digits/hyphens only, matching real Postman uid shape) enforced both at target-parse time (fail fast, clear error) and inside fetch_postman_collection/fetch_postman_environment (defense in depth for any other caller). - write_fetched_collection now uses write_secret_text instead of Path.write_text. Verified after rebasing onto upstream main (fde37a14): the 5 test files that import the changed modules pass (97 tests), ruff format and ruff check are clean on the touched files, mypy Success: no issues found in 4 source files (note: unused pyproject override sections for cvss.*, pypdf.*, reportlab.*). The full suite was not re-run. 6 new tests cover the traversal rejection (asserting requests.get is never called), an @-authority variant, and the file-permission fix. --- strix/interface/utils.py | 14 ++++++++++--- strix/utils/api_spec.py | 21 ++++++++++++++++++++ tests/test_api_spec.py | 26 ++++++++++++++++++++++++ tests/test_api_spec_targets.py | 36 +++++++++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/strix/interface/utils.py b/strix/interface/utils.py index 7ab55d8ee..232a7e139 100644 --- a/strix/interface/utils.py +++ b/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://)" ) + # 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) diff --git a/strix/utils/api_spec.py b/strix/utils/api_spec.py index db3a77388..e2e94e03a 100644 --- a/strix/utils/api_spec.py +++ b/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, diff --git a/tests/test_api_spec.py b/tests/test_api_spec.py index d7979d358..177ee6b33 100644 --- a/tests/test_api_spec.py +++ b/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"): diff --git a/tests/test_api_spec_targets.py b/tests/test_api_spec_targets.py index bba0aba55..84dff58f4 100644 --- a/tests/test_api_spec_targets.py +++ b/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"