From b942f74d1a5a914bbfd3a8604de3708911a5739e Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Fri, 12 Jun 2026 22:29:57 +0530 Subject: [PATCH] =?UTF-8?q?security:=20validate=20env-controlled=20inputs?= =?UTF-8?q?=20+=20harden=20release=20pipeline=20(PER-8514=E2=80=938518)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves five High-severity AppSec findings (and the C-003..C-006 attack chains built on them): PER-8516 (CWE-306) & PER-8518 (CWE-918) — PERCY_CLI_API was used verbatim as the target for credential-bearing POSTs (capabilities can carry the BrowserStack accessKey) and healthcheck GETs, enabling SSRF and credential exfiltration to an attacker-controlled host. Add _resolve_cli_api_address(): loopback-only by default; a remote host is allowed only over HTTPS with an explicit PERCY_ALLOW_REMOTE_CLI_API opt-in. Otherwise we warn and fall back to http://localhost:5338. PER-8517 (CWE-22) — PERCY_TMP_DIR was passed straight to Path().mkdir() and tempfile.mkstemp(), allowing arbitrary file writes via path traversal. Add _resolve_tmp_dir(): realpath the value and require it to stay within an approved temp root (system tmpdir, /tmp, /var/tmp); fall back to mkdtemp() otherwise. PER-8514 (CWE-829) — release.yml pinned actions to mutable tags and ran with write-all GITHUB_TOKEN. Pin checkout/setup-python/cache to commit SHAs and add permissions: contents: read. PER-8515 (CWE-829) — twine was unpinned in development.txt, allowing a publishing-tool substitution. Pin to twine==5.1.1. Adds unit tests for both validators. Full suite: 140 passing; pylint 10/10. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/release.yml | 10 +++++--- development.txt | 2 +- percy/lib/cli_wrapper.py | 37 +++++++++++++++++++++++++++-- percy/providers/generic_provider.py | 30 +++++++++++++++++++---- tests/test_cli_wrapper.py | 36 ++++++++++++++++++++++++++++ tests/test_generic_provider.py | 19 +++++++++++++++ 6 files changed, 124 insertions(+), 10 deletions(-) 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 b5f2e73..9d7155b 100644 --- a/development.txt +++ b/development.txt @@ -1,3 +1,3 @@ httpretty==1.1.* pylint==2.* -twine +twine==5.1.1 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 77c4436..1a03e23 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 8a5b818..5b67b44 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,24 @@ 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_path(self): count = 1000 # Generate count unique paths and check their uniqueness png_paths = [