diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index ef80779..35ef5cc 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -2,10 +2,15 @@ name: Changelog on: push: branches: [master] +permissions: + contents: read jobs: update_draft: + permissions: + contents: write # create/update the draft release + pull-requests: read runs-on: ubuntu-latest steps: - - uses: release-drafter/release-drafter@v5 + - uses: release-drafter/release-drafter@09c613e259eb8d4e7c81c2cb00618eb5fc4575a7 # v5.25.0 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index a180105..7deb8bd 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,15 +1,17 @@ name: Lint on: push +permissions: + contents: read jobs: lint: name: Lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-python@v4 + - uses: actions/checkout@a37ce9120846195fa4ece8f58b268e6043cb2f26 # v3.7.0 + - uses: actions/setup-python@7f4fc3e22c37d6ff65e88745f38bd3157c663f7c # v4.9.1 with: python-version: 3.11 - - uses: actions/cache@v3 + - uses: actions/cache@6f8efc29b200d32929f49075959781ed54ec270c # v3.5.0 with: path: ~/.cache/pip key: v1/${{ runner.os }}/pip/${{ hashFiles('{requirements,development}.txt') }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1a6984c..ada134f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2,15 +2,17 @@ name: Release on: release: types: [published] +permissions: + contents: read jobs: publish: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-python@v4 + - uses: actions/checkout@a37ce9120846195fa4ece8f58b268e6043cb2f26 # v3.7.0 + - uses: actions/setup-python@7f4fc3e22c37d6ff65e88745f38bd3157c663f7c # v4.9.1 with: python-version: 3.x - - uses: actions/cache@v3 + - uses: actions/cache@6f8efc29b200d32929f49075959781ed54ec270c # v3.5.0 with: path: ~/.cache/pip key: v1/${{ runner.os }}/pip/${{ hashFiles('{requirements,development}.txt') }} diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index 73d71d3..4a4f406 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -3,11 +3,17 @@ on: schedule: - cron: '0 19 * * 2' +permissions: + contents: read + jobs: stale: + permissions: + issues: write + pull-requests: write runs-on: ubuntu-latest steps: - - uses: actions/stale@v8 + - uses: actions/stale@1160a2240286f5da8ec72b1c0816ce2481aabf84 # v8.0.0 with: stale-issue-message: >- This issue is stale because it has been open for more than 14 days with no activity. diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 56877a9..a5a1188 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,6 +8,9 @@ on: type: string default: master +permissions: + contents: read + jobs: test: name: Test @@ -17,7 +20,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions-ecosystem/action-regex-match@v2 + - uses: actions-ecosystem/action-regex-match@9e6c4fb3d5e898f505be7a1fb6e7b0a278f6665b # v2.0.2 id: regex-match if: ${{ github.event_name == 'workflow_dispatch' }} with: @@ -26,22 +29,22 @@ jobs: - name: Break on invalid branch name run: exit 1 if: ${{ github.event_name == 'workflow_dispatch' && steps.regex-match.outputs && steps.regex-match.outputs.match == '' }} - - uses: actions/checkout@v3 - - uses: actions/setup-python@v4 + - uses: actions/checkout@a37ce9120846195fa4ece8f58b268e6043cb2f26 # v3.7.0 + - uses: actions/setup-python@7f4fc3e22c37d6ff65e88745f38bd3157c663f7c # v4.9.1 with: python-version: ${{ matrix.python }} - - uses: actions/cache@v3 + - uses: actions/cache@6f8efc29b200d32929f49075959781ed54ec270c # v3.5.0 with: path: ~/.cache/pip key: v1/${{ runner.os }}/pypi-${{matrix.python}}/${{ hashFiles('{requirements,development}.txt') }} restore-keys: v1/${{ runner.os }}/pypi-${matrix.python}/ - - uses: actions/setup-node@v3 + - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: node-version: 16 - name: Get yarn cache directory path id: yarn-cache-dir-path run: echo "::set-output name=dir::$(yarn cache dir)" - - uses: actions/cache@v3 + - uses: actions/cache@6f8efc29b200d32929f49075959781ed54ec270c # v3.5.0 with: path: ${{ steps.yarn-cache-dir-path.outputs.dir }} key: v1/${{ runner.os }}/node-${{ matrix.node }}/${{ hashFiles('**/yarn.lock') }} @@ -65,7 +68,7 @@ jobs: # no node project to link into); PATH is what makes percy resolvable. echo "$(yarn global bin)" >> "$GITHUB_PATH" export PATH="$(yarn global bin):$PATH" - cd ${{ github.workspace }} + cd "$GITHUB_WORKSPACE" yarn remove @percy/cli >/dev/null 2>&1 || true yarn link `echo $PERCY_PACKAGES` >/dev/null 2>&1 || true percy --version diff --git a/README.md b/README.md index f09950c..d63440e 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,13 @@ $ percy exec -- [python test command] - `name` (**required**) - The snapshot name; must be unique to each snapshot - `**kwargs` - [See per-snapshot configuration options](https://www.browserstack.com/docs/percy/take-percy-snapshots/overview#per-snapshot-configuration) +### Cookies + +By default the SDK sends the browser's cookies with each snapshot so Percy can load pages and +assets that need a logged-in session during asset discovery. To stop sending cookies, set +`PERCY_DO_NOT_USE_CAPTURED_COOKIES=true`. Assets behind authentication may then fail to load +and snapshots can render incorrectly; the SDK logs a warning once when this is enabled. + ### Migrating Config If you have a previous Percy configuration file, migrate it to the newest version with the diff --git a/percy/cache.py b/percy/cache.py index f82a2fa..1549064 100644 --- a/percy/cache.py +++ b/percy/cache.py @@ -1,9 +1,12 @@ +import threading import time class Cache: CACHE = {} CACHE_TIMEOUT = 5 * 60 # 300 seconds TIMEOUT_KEY = 'last_access_time' + # Guards CACHE so parallel runners sharing a process can't mutate it mid-iteration + LOCK = threading.RLock() # Caching Keys capabilities = 'capabilities' @@ -19,24 +22,27 @@ def check_types(cls, session_id, property): @classmethod def set_cache(cls, session_id, property, value): cls.check_types(session_id, property) - session = cls.CACHE.get(session_id, {}) - session[cls.TIMEOUT_KEY] = time.time() - session[property] = value - cls.CACHE[session_id] = session + with cls.LOCK: + session = cls.CACHE.get(session_id, {}) + session[cls.TIMEOUT_KEY] = time.time() + session[property] = value + cls.CACHE[session_id] = session @classmethod def get_cache(cls, session_id, property): cls.cleanup_cache() cls.check_types(session_id, property) - session = cls.CACHE.get(session_id, {}) - return session.get(property, None) + with cls.LOCK: + session = cls.CACHE.get(session_id, {}) + return session.get(property, None) @classmethod def cleanup_cache(cls): now = time.time() - session_ids = [] - for session_id, session in cls.CACHE.items(): - timestamp = session[cls.TIMEOUT_KEY] - if now - timestamp >= cls.CACHE_TIMEOUT: - session_ids.append(session_id) - list(map(cls.CACHE.pop, session_ids)) + with cls.LOCK: + session_ids = [ + session_id for session_id, session in cls.CACHE.items() + if now - session[cls.TIMEOUT_KEY] >= cls.CACHE_TIMEOUT + ] + for session_id in session_ids: + cls.CACHE.pop(session_id, None) diff --git a/percy/snapshot.py b/percy/snapshot.py index dfacfeb..d88417e 100644 --- a/percy/snapshot.py +++ b/percy/snapshot.py @@ -2,6 +2,7 @@ import os import platform import json +import tempfile from contextlib import contextmanager from functools import lru_cache from time import sleep @@ -35,6 +36,9 @@ def _get_bool_env(key): RESIZE_SETTLE_SECONDS = float(os.environ.get('PERCY_RESIZE_SETTLE_SECONDS') or 0.5) except (TypeError, ValueError): RESIZE_SETTLE_SECONDS = 0.5 +# Set to true to stop sending browser cookies to the CLI (read per snapshot) +DO_NOT_USE_CAPTURED_COOKIES_ENV = 'PERCY_DO_NOT_USE_CAPTURED_COOKIES' +_cookie_opt_out_warned = False # for logging LABEL = '[\u001b[35m' + ('percy:python' if PERCY_DEBUG else 'percy') + '\u001b[39m]' CDP_SUPPORT_SELENIUM = (str(SELENIUM_VERSION)[0].isdigit() and int( @@ -52,6 +56,25 @@ def log(message, lvl = 'info'): if lvl != 'debug' or PERCY_DEBUG: print(message) +def get_cookies_for_snapshot(driver): + """Collect browser cookies so the CLI can load authenticated assets during + discovery. Honours the PERCY_DO_NOT_USE_CAPTURED_COOKIES opt-out and never + raises: a cookie read failure only means no cookies are forwarded.""" + global _cookie_opt_out_warned # pylint: disable=global-statement + if _get_bool_env(DO_NOT_USE_CAPTURED_COOKIES_ENV): + if not _cookie_opt_out_warned: + _cookie_opt_out_warned = True + log('Not sending browser cookies to Percy because ' + f'{DO_NOT_USE_CAPTURED_COOKIES_ENV} is set. Pages or assets that need a ' + 'logged-in session may fail to load during asset discovery, so snapshots ' + 'can render incorrectly.', 'warn') + return [] + try: + return driver.get_cookies() + except Exception as e: # pylint: disable=broad-except + log(f'Could not read browser cookies, continuing without them: {e}', 'debug') + return [] + # Check if Percy is enabled, caching the result so it is only checked once @lru_cache(maxsize=None) def is_percy_enabled(): @@ -991,11 +1014,7 @@ def capture_responsive_dom(driver, cookies, config, percy_dom_script=None, **kwa dom_snapshots.append(dom_snapshot) # Optional debug dump — gated to avoid polluting the user's CWD on every CI run. if PERCY_DEBUG: - try: - with open("output_file.json", "w", encoding="utf-8") as file_handle: - json.dump(dom_snapshots, file_handle, indent=4) - except Exception as e: # pylint: disable=broad-except - log(f"Could not write debug snapshot dump: {type(e).__name__}: {e}", "debug") + _write_responsive_debug_dump(dom_snapshots) # Restore the original window size after capture. No snapshot follows, so # skip the resize-event poll (wait_for_resize=False) — it serves no purpose # here and is the call that hangs geckodriver in CI. @@ -1003,6 +1022,21 @@ def capture_responsive_dom(driver, cookies, config, percy_dom_script=None, **kwa driver, current_width, current_height, resize_count + 1, wait_for_resize=False) return dom_snapshots +def _write_responsive_debug_dump(dom_snapshots): + # Owner-only temp file with an unpredictable name; cookies are left out + # because they're not needed to debug the captured DOM. + try: + redacted = [ + {k: v for k, v in snap.items() if k != 'cookies'} if isinstance(snap, dict) else snap + for snap in dom_snapshots + ] + fd, path = tempfile.mkstemp(prefix='percy-responsive-dom-', suffix='.json') + with os.fdopen(fd, "w", encoding="utf-8") as file_handle: + json.dump(redacted, file_handle, indent=4) + log(f"Wrote responsive snapshot debug dump to {path}", "debug") + except Exception as e: # pylint: disable=broad-except + log(f"Could not write debug snapshot dump: {type(e).__name__}: {e}", "debug") + def is_responsive_snapshot_capture(config, **kwargs): # Don't run resposive snapshot capture when defer uploads is enabled if 'percy' in config and config['percy'].get('deferUploads', False): return False @@ -1028,7 +1062,7 @@ def percy_snapshot(driver, name, **kwargs): # Expose closed shadow roots via CDP before serialization so PercyDOM # can find them through the WeakMap (Chromium-only; non-Chromium no-ops). expose_closed_shadow_roots(driver) - cookies = driver.get_cookies() + cookies = get_cookies_for_snapshot(driver) # Serialize and capture the DOM if is_responsive_snapshot_capture(data['config'], **kwargs): diff --git a/tests/test_cache.py b/tests/test_cache.py index fcaa061..693d85e 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -1,4 +1,5 @@ # . # pylint: disable=[arguments-differ, protected-access] +import threading import time import unittest from unittest.mock import patch @@ -57,3 +58,31 @@ def test_cleanup_cache(self): self.assertIn(self.session_id, self.cache.CACHE) self.cache.cleanup_cache() self.assertNotIn(self.session_id, self.cache.CACHE) + + def test_cleanup_cache_blocks_concurrent_writers_while_iterating(self): + cache = self.cache + writer_blocked = [] + writers = [] + + class InterleavingDict(dict): + # Simulates another thread writing to the cache mid-iteration + def items(self): + writer = threading.Thread( + target=cache.set_cache, + args=('other_session', Cache.capabilities, {})) + writer.start() + writers.append(writer) + writer.join(timeout=0.2) + writer_blocked.append(writer.is_alive()) + for item in list(super().items()): + yield item + + original = cache.CACHE + cache.CACHE = InterleavingDict(original) + try: + cache.cleanup_cache() # must not raise "dictionary changed size during iteration" + writers[0].join(timeout=5) + self.assertEqual(writer_blocked, [True]) + self.assertIn('other_session', cache.CACHE) + finally: + cache.CACHE = original diff --git a/tests/test_snapshot_units.py b/tests/test_snapshot_units.py index dd57d07..2ac9823 100644 --- a/tests/test_snapshot_units.py +++ b/tests/test_snapshot_units.py @@ -5,6 +5,9 @@ import importlib import json import os +import shutil +import stat +import tempfile import unittest from unittest.mock import patch, MagicMock, Mock @@ -471,6 +474,13 @@ def current_url(self): class TestResponsiveDebugDump(unittest.TestCase): + def setUp(self): + self.tmpdir = tempfile.mkdtemp() + patcher = patch('percy.snapshot.tempfile.tempdir', self.tmpdir) + patcher.start() + self.addCleanup(patcher.stop) + self.addCleanup(shutil.rmtree, self.tmpdir, True) + def tearDown(self): if os.path.exists('output_file.json'): os.remove('output_file.json') @@ -489,7 +499,32 @@ def test_debug_dump_is_written_when_percy_debug(self): result = local.capture_responsive_dom( self._driver(), {}, {}, percy_dom_script='PERCY_DOM') self.assertEqual(result, []) - self.assertTrue(os.path.exists('output_file.json')) + self.assertEqual(len(os.listdir(self.tmpdir)), 1) + + @patch('percy.snapshot.PERCY_DEBUG', True) + @patch('percy.snapshot.change_window_dimension_and_wait', MagicMock()) + @patch('percy.snapshot._wait_for_ready', MagicMock(return_value=None)) + @patch('percy.snapshot.get_responsive_widths', + MagicMock(return_value=[{'width': 375}])) + @patch('percy.snapshot.get_serialized_dom') + def test_debug_dump_is_private_temp_file_without_cookies(self, mock_dom): + mock_dom.return_value = {'html': '

', 'cookies': [{'name': 'sid', 'value': 's3cr3t'}]} + result = local.capture_responsive_dom( + self._driver(), [{'name': 'sid', 'value': 's3cr3t'}], {}, + percy_dom_script='PERCY_DOM') + # The snapshot sent to the CLI still carries its cookies + self.assertEqual(result[0]['cookies'], [{'name': 'sid', 'value': 's3cr3t'}]) + # No predictable file is dropped into the working directory + self.assertFalse(os.path.exists('output_file.json')) + files = os.listdir(self.tmpdir) + self.assertEqual(len(files), 1) + path = os.path.join(self.tmpdir, files[0]) + if os.name == 'posix': + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + with open(path, encoding='utf-8') as file_handle: + content = file_handle.read() + self.assertNotIn('s3cr3t', content) + self.assertEqual(json.loads(content), [{'html': '

', 'width': 375}]) @patch('percy.snapshot.PERCY_DEBUG', True) @patch('percy.snapshot.change_window_dimension_and_wait', MagicMock()) @@ -502,5 +537,72 @@ def test_debug_dump_failure_is_logged_not_raised(self, _mock_dump): self.assertEqual(result, []) +class TestCookieForwarding(unittest.TestCase): + SESSION_COOKIES = [ + {'name': 'session_id', 'value': 'abc', 'domain': 'example.com', 'secure': True}, + {'name': 'auth_token', 'value': 'xyz', 'domain': 'example.com', 'secure': True}, + ] + + def setUp(self): + patcher = patch('percy.snapshot._cookie_opt_out_warned', False) + patcher.start() + self.addCleanup(patcher.stop) + + def _driver(self): + driver = MagicMock() + driver.get_cookies.return_value = list(self.SESSION_COOKIES) + driver.current_url = 'https://example.com/' + return driver + + @patch('percy.snapshot.is_percy_enabled', + MagicMock(return_value={'session_type': 'web', 'config': {}, 'widths': {}})) + @patch('percy.snapshot.fetch_percy_dom', MagicMock(return_value='PERCY_DOM')) + @patch('percy.snapshot.expose_closed_shadow_roots', MagicMock()) + @patch('percy.snapshot.get_serialized_dom') + @patch('percy.snapshot.requests.post') + def _take_snapshots(self, mock_post, mock_dom, count=1): + mock_dom.side_effect = lambda _d, cookies, **_k: {'html': '

', 'cookies': cookies} + mock_post.return_value.json.return_value = {'success': True} + for i in range(count): + local.percy_snapshot(self._driver(), f'snap {i}') + return [c for c in mock_post.call_args_list if c.args[0].endswith('/percy/snapshot')] + + @patch.dict(os.environ, {}, clear=False) + def test_default_forwards_session_cookies_unchanged(self): + os.environ.pop('PERCY_DO_NOT_USE_CAPTURED_COOKIES', None) + posts = self._take_snapshots() + self.assertEqual(posts[0].kwargs['json']['dom_snapshot']['cookies'], + self.SESSION_COOKIES) + + @patch.dict(os.environ, {'PERCY_DO_NOT_USE_CAPTURED_COOKIES': 'true'}) + @patch('percy.snapshot.log') + def test_opt_out_sends_no_cookies_and_warns_once(self, mock_log): + posts = self._take_snapshots(count=2) # pylint: disable=unexpected-keyword-arg + self.assertEqual(len(posts), 2) + for post in posts: + self.assertEqual(post.kwargs['json']['dom_snapshot']['cookies'], []) + warnings = [c for c in mock_log.call_args_list + if len(c.args) > 1 and c.args[1] == 'warn'] + self.assertEqual(len(warnings), 1) + self.assertEqual( + warnings[0].args[0], + 'Not sending browser cookies to Percy because PERCY_DO_NOT_USE_CAPTURED_COOKIES ' + 'is set. Pages or assets that need a logged-in session may fail to load during ' + 'asset discovery, so snapshots can render incorrectly.') + + @patch.dict(os.environ, {'PERCY_DO_NOT_USE_CAPTURED_COOKIES': 'false'}) + def test_opt_out_false_still_forwards(self): + driver = self._driver() + self.assertEqual(local.get_cookies_for_snapshot(driver), self.SESSION_COOKIES) + + @patch.dict(os.environ, {}, clear=False) + @patch('percy.snapshot.log') + def test_cookie_read_failure_is_not_raised(self, _mock_log): + os.environ.pop('PERCY_DO_NOT_USE_CAPTURED_COOKIES', None) + driver = self._driver() + driver.get_cookies.side_effect = Exception('no cookies for you') + self.assertEqual(local.get_cookies_for_snapshot(driver), []) + + if __name__ == '__main__': unittest.main()