diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1a6984c..98a5659 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2,15 +2,19 @@ name: Release on: release: types: [published] +permissions: + contents: read jobs: publish: runs-on: ubuntu-latest + permissions: + contents: read steps: - - uses: actions/checkout@v3 - - uses: actions/setup-python@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 with: python-version: 3.x - - uses: actions/cache@v3 + - uses: actions/cache@1bd1e32a3bdc45362d1e726936510720a7c30a57 # v4.2.0 with: path: ~/.cache/pip key: v1/${{ runner.os }}/pip/${{ hashFiles('{requirements,development}.txt') }} diff --git a/development.txt b/development.txt index b5ca3b9..6531b68 100644 --- a/development.txt +++ b/development.txt @@ -1,4 +1,4 @@ httpretty==1.1.* pylint==2.* -twine +twine==5.1.1 coverage==7.* diff --git a/percy/lib/cli_wrapper.py b/percy/lib/cli_wrapper.py index dee7481..33fb167 100644 --- a/percy/lib/cli_wrapper.py +++ b/percy/lib/cli_wrapper.py @@ -1,4 +1,5 @@ from functools import lru_cache +from urllib.parse import urlparse import os import requests @@ -6,8 +7,40 @@ from percy.common import log from percy.environment import Environment -# Maybe get the CLI API address from the environment -PERCY_CLI_API = os.environ.get('PERCY_CLI_API') or 'http://localhost:5338' +DEFAULT_PERCY_CLI_API = 'http://localhost:5338' + +# Hostnames that resolve to the local machine. The Percy CLI runs locally, so +# the SDK only ever needs to talk to a loopback address. +_LOOPBACK_HOSTS = frozenset({'localhost', '127.0.0.1', '::1'}) + + +def _resolve_cli_api_address(): + # The CLI API address can be overridden via the environment. Because the SDK + # POSTs request bodies that may carry BrowserStack credentials (capabilities) + # to this address, an attacker-controlled value would enable SSRF and + # credential exfiltration to an arbitrary host (CWE-306 / CWE-918). Restrict + # it to loopback by default; allow a remote host only over HTTPS and with an + # explicit opt-in. + raw = os.environ.get('PERCY_CLI_API') or DEFAULT_PERCY_CLI_API + host = (urlparse(raw).hostname or '').lower() + + if host in _LOOPBACK_HOSTS: + return raw + + allow_remote = os.environ.get('PERCY_ALLOW_REMOTE_CLI_API', '').lower() in ('1', 'true', 'yes') + if allow_remote and urlparse(raw).scheme == 'https': + return raw + + log( + f"Ignoring non-loopback PERCY_CLI_API '{raw}'; falling back to {DEFAULT_PERCY_CLI_API}. " + "To target a remote Percy CLI, use an https:// URL and set PERCY_ALLOW_REMOTE_CLI_API=true.", + error=True, + ) + return DEFAULT_PERCY_CLI_API + + +# Maybe get the CLI API address from the environment (validated to loopback) +PERCY_CLI_API = _resolve_cli_api_address() class CLIWrapper: def __init__(self) -> None: diff --git a/percy/providers/generic_provider.py b/percy/providers/generic_provider.py index 68f27be..288c37f 100644 --- a/percy/providers/generic_provider.py +++ b/percy/providers/generic_provider.py @@ -180,11 +180,33 @@ def get_device_name(self): return '' def _get_dir(self): + return self._resolve_tmp_dir() + + @staticmethod + def _resolve_tmp_dir(): dir_path = os.environ.get('PERCY_TMP_DIR') or None - if dir_path: - Path(dir_path).mkdir(parents=True, exist_ok=True) - return dir_path - return tempfile.mkdtemp() + if not dir_path: + return tempfile.mkdtemp() + + # PERCY_TMP_DIR is attacker-influenceable. Resolve symlinks and relative + # segments, then confirm the result stays within an approved temp root so + # it cannot be used to write screenshots to arbitrary locations such as + # web-served directories or dotfiles (CWE-22). + resolved = os.path.realpath(dir_path) + allowed_bases = [] + for base in (tempfile.gettempdir(), '/tmp', '/var/tmp'): + try: + allowed_bases.append(os.path.realpath(base)) + except OSError: + continue + + if not any(resolved == base or resolved.startswith(base + os.sep) for base in allowed_bases): + log(f"Ignoring unsafe PERCY_TMP_DIR '{dir_path}' (outside the system temp directory); " + "using a secure temporary directory instead.", error=True) + return tempfile.mkdtemp() + + Path(resolved).mkdir(parents=True, exist_ok=True) + return resolved def _get_path(self, directory): suffix = '.png' diff --git a/tests/test_cli_wrapper.py b/tests/test_cli_wrapper.py index e89fb3c..3c52506 100644 --- a/tests/test_cli_wrapper.py +++ b/tests/test_cli_wrapper.py @@ -1,4 +1,5 @@ # pylint: disable=[protected-access] +import os import unittest from unittest.mock import Mock, patch from percy.errors import CLIException @@ -7,6 +8,41 @@ from percy.lib.tile import Tile +class ResolveCliApiAddressTestCase(unittest.TestCase): + @patch.dict(os.environ, {}, clear=True) + def test_defaults_to_localhost(self): + self.assertEqual(cli_wrapper._resolve_cli_api_address(), cli_wrapper.DEFAULT_PERCY_CLI_API) + + @patch.dict(os.environ, {"PERCY_CLI_API": "http://127.0.0.1:5338"}, clear=True) + def test_allows_loopback(self): + self.assertEqual(cli_wrapper._resolve_cli_api_address(), "http://127.0.0.1:5338") + + @patch.dict(os.environ, {"PERCY_CLI_API": "http://attacker.example.com/x"}, clear=True) + def test_rejects_remote_host(self): + # Falls back to the safe default instead of POSTing credentials off-box. + self.assertEqual(cli_wrapper._resolve_cli_api_address(), cli_wrapper.DEFAULT_PERCY_CLI_API) + + @patch.dict(os.environ, { + "PERCY_CLI_API": "http://169.254.169.254/latest/meta-data", + }, clear=True) + def test_rejects_link_local_ssrf_target(self): + self.assertEqual(cli_wrapper._resolve_cli_api_address(), cli_wrapper.DEFAULT_PERCY_CLI_API) + + @patch.dict(os.environ, { + "PERCY_CLI_API": "https://percy-cli.internal:5338", + "PERCY_ALLOW_REMOTE_CLI_API": "true", + }, clear=True) + def test_allows_remote_https_with_opt_in(self): + self.assertEqual(cli_wrapper._resolve_cli_api_address(), "https://percy-cli.internal:5338") + + @patch.dict(os.environ, { + "PERCY_CLI_API": "http://percy-cli.internal:5338", + "PERCY_ALLOW_REMOTE_CLI_API": "true", + }, clear=True) + def test_remote_opt_in_still_requires_https(self): + self.assertEqual(cli_wrapper._resolve_cli_api_address(), cli_wrapper.DEFAULT_PERCY_CLI_API) + + class CLIWrapperTestCase(unittest.TestCase): def setUp(self): self.cli_wrapper = cli_wrapper.CLIWrapper() diff --git a/tests/test_generic_provider.py b/tests/test_generic_provider.py index aa33cb4..79f722b 100644 --- a/tests/test_generic_provider.py +++ b/tests/test_generic_provider.py @@ -2,6 +2,7 @@ # pylint: disable=R0904 import shutil import os +import tempfile import unittest from unittest.mock import MagicMock, patch, PropertyMock from appium.webdriver.webdriver import WebDriver @@ -52,6 +53,42 @@ def test_get_dir_with_env_var(self): self.assertEqual(os.listdir(dir_created), []) os.removedirs(dir_created) + def test_get_dir_rejects_path_outside_temp(self): + # A PERCY_TMP_DIR that resolves outside the system temp root must be + # rejected to prevent arbitrary file writes via path traversal (CWE-22). + escape_dir = os.path.join(os.getcwd(), "percy-escape-test") + with patch.dict(os.environ, {"PERCY_TMP_DIR": escape_dir}): + dir_created = self.generic_provider._get_dir() + self.assertTrue(os.path.realpath(dir_created).startswith(os.path.realpath(tempfile.gettempdir()))) + self.assertFalse(os.path.exists(escape_dir)) + os.removedirs(dir_created) + + def test_get_dir_rejects_dotdot_traversal(self): + traversal = os.path.join(tempfile.gettempdir(), "..", "..", "etc", "percy-evil") + with patch.dict(os.environ, {"PERCY_TMP_DIR": traversal}): + dir_created = self.generic_provider._get_dir() + self.assertTrue(os.path.realpath(dir_created).startswith(os.path.realpath(tempfile.gettempdir()))) + self.assertFalse(os.path.exists(os.path.realpath(traversal))) + os.removedirs(dir_created) + + def test_get_dir_skips_unresolvable_allowed_base(self): + # If resolving an allowed temp base raises OSError it must be skipped + # rather than crash; with no resolvable base the env dir is rejected and + # a secure temporary directory is used instead. + real_realpath = os.path.realpath + fake_dir = os.path.join(tempfile.gettempdir(), "percy-base-error") + + def flaky_realpath(path): + if path in (tempfile.gettempdir(), "/tmp", "/var/tmp"): + raise OSError("cannot resolve base") + return real_realpath(path) + + with patch.dict(os.environ, {"PERCY_TMP_DIR": fake_dir}): + with patch("percy.providers.generic_provider.os.path.realpath", side_effect=flaky_realpath): + dir_created = self.generic_provider._get_dir() + self.assertTrue(os.path.realpath(dir_created).startswith(os.path.realpath(tempfile.gettempdir()))) + os.removedirs(dir_created) + def test_get_path(self): count = 1000 # Generate count unique paths and check their uniqueness png_paths = [