From d068aaac138071a568ffc2321a8369fbf94267d9 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 15:32:34 -0400 Subject: [PATCH 1/4] Bug 2079908 - [GITHUB-ETL] Github-ETL should report errors to Sentry instead of exiting non-zero on partial failures --- main.py | 42 ++++++++++++++++++++++--- pyproject.toml | 1 + requirements.txt | 12 +++++-- tests/test_main.py | 78 ++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 124 insertions(+), 9 deletions(-) diff --git a/main.py b/main.py index 0da8372..4c3cf95 100755 --- a/main.py +++ b/main.py @@ -22,10 +22,12 @@ import jwt import requests +import sentry_sdk from google.api_core import exceptions as api_exceptions from google.api_core.client_options import ClientOptions from google.auth.credentials import AnonymousCredentials from google.cloud import bigquery +from sentry_sdk.integrations.logging import LoggingIntegration BUG_RE = re.compile(r"\b(?:bug|b=)\s*#?(\d+)\b", re.I) @@ -398,6 +400,25 @@ def setup_logging() -> None: ) +def setup_sentry() -> None: + """ + Report errors to Sentry when SENTRY_DSN is set; no-op otherwise. + + logger.error/logger.exception calls become Sentry events and lower levels + are attached as breadcrumbs. SENTRY_ENVIRONMENT and SENTRY_RELEASE are read + from the environment by the SDK. + """ + dsn = os.environ.get("SENTRY_DSN") + if not dsn: + return + sentry_sdk.init( + dsn=dsn, + integrations=[ + LoggingIntegration(level=logging.INFO, event_level=logging.ERROR) + ], + ) + + def _parse_github_timestamp(value: str | None) -> datetime | None: """ Parse a GitHub ISO-8601 timestamp (e.g. ``2026-06-17T14:53:58Z``) into an @@ -1722,11 +1743,19 @@ def main() -> int: 4. Repeat until no more data """ setup_logging() + setup_sentry() try: return _main() except RuntimeError as e: - logger.error(str(e)) + logger.exception(str(e)) return 1 + except SystemExit as e: + # Missing configuration: report it, then still fail the run. + sentry_sdk.capture_message(str(e), level="fatal") + raise + finally: + # The process exits right after this; send queued events first. + sentry_sdk.flush() def _main() -> int: @@ -1835,7 +1864,10 @@ def _main() -> int: # failed repo rather than propagating out of the executor and # discarding the results of other in-flight repos. logger.exception # records the worker thread's traceback for debugging in CI/prod. - logger.exception(f"Failed to process repo {repo}: {exc}") + # The Sentry event is tagged with the repo for filtering. + with sentry_sdk.new_scope() as scope: + scope.set_tag("repo", repo) + logger.exception(f"Failed to process repo {repo}: {exc}") failed_repos.append(repo) continue total_processed += processed @@ -1844,10 +1876,12 @@ def _main() -> int: ) if failed_repos: - logger.error( + # Per-repo failures were already reported to Sentry above. Exit 0 so a + # partial failure does not mark the cron job as failed. + logger.warning( f"ETL completed with failures. Failed repos: {', '.join(failed_repos)}" ) - return 1 + return 0 logger.info( f"GitHub ETL process completed successfully. Total PRs processed: {total_processed}" diff --git a/pyproject.toml b/pyproject.toml index e729a88..8d444bc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ dependencies = [ "requests>=2.25.0", "google-cloud-bigquery==3.25.0", "PyJWT[crypto]>=2.0.0", + "sentry-sdk>=2.0.0", ] [project.optional-dependencies] diff --git a/requirements.txt b/requirements.txt index 0196810..a65d4e0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,7 +7,9 @@ certifi==2026.1.4 \ --hash=sha256:9943707519e4add1115f44c2bc244f782c0249876bf51b6599fee1ffbedd685c \ --hash=sha256:ac726dd470482006e014ad384921ed6438c457018f4b3d204aea4281258b2120 - # via requests + # via + # requests + # sentry-sdk cffi==2.0.0 \ --hash=sha256:00bdf7acc5f795150faa6957054fbbca2439db2f775ce831222b66f192f03beb \ --hash=sha256:07b271772c100085dd28b74fa0cd81c8fb1a3ba18b21e03d7c27f3436a10606b \ @@ -458,6 +460,10 @@ rsa==4.9.1 \ --hash=sha256:68635866661c6836b8d39430f97a996acbd61bfa49406748ea243539fe239762 \ --hash=sha256:e7bdbfdb5497da4c07dfd35530e1a902659db6ff241e39d9953cad06ebd0ae75 # via google-auth +sentry-sdk==2.71.0 \ + --hash=sha256:7beb27a22f06396f3a05510c3da6ea10e1c79e02e859993c27ce4ff074e296d6 \ + --hash=sha256:9ed36b4c206048aa67b5ddba0a6e28d59fc55cf10d60f5f7aea2c3917184bf4a + # via github-etl (pyproject.toml) six==1.17.0 \ --hash=sha256:4721f391ed90541fddacab5acf947aa0d3dc7d27b2e1e8eda2be8970586c3274 \ --hash=sha256:ff70335d468e7eb6ec65b95b99d3a2836546063f63acc5171de367e834932a81 @@ -469,4 +475,6 @@ typing-extensions==4.15.0 \ urllib3==2.6.3 \ --hash=sha256:1b62b6884944a57dbe321509ab94fd4d3b307075e0c2eae991ac71ee15ad38ed \ --hash=sha256:bf272323e553dfb2e87d9bfd225ca7b0f467b919d7bbd355436d3fd37cb0acd4 - # via requests + # via + # requests + # sentry-sdk diff --git a/tests/test_main.py b/tests/test_main.py index 20fb2b3..eada617 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -540,7 +540,7 @@ def extract_side_effect(*args, **kwargs): ): result = main.main() - assert result == 1 # partial failure + assert result == 0 # partial failure is reported to Sentry, not the exit code assert mock_extract.call_count == 2 # both repos were attempted mock_load.assert_called_once() # only the successful repo loaded data @@ -562,7 +562,7 @@ def test_bare_exception_on_one_repo_is_isolated( """A bare Exception (e.g. from load_data) on one repo must not abort the others. The executor catches broadly, so the failing repo is recorded in failed_repos - (overall exit code 1) while the healthy repo still completes its load. + (reported to Sentry, overall exit code 0) while the healthy repo still completes its load. """ # Fresh iterator per repo (a shared return_value iterator would be exhausted # by whichever repo consumes it first). @@ -597,11 +597,83 @@ def transform_side_effect(chunk, repo): ): result = main.main() - assert result == 1 # partial failure recorded, run did not abort + assert result == 0 # partial failure recorded, run did not abort assert mock_extract.call_count == 2 # both repos were attempted assert mock_load.call_count == 2 # both repos reached the load step +@patch("main.sentry_sdk") +@patch("main.setup_logging") +@patch("main.bigquery.Client") +@patch("requests.Session") +@patch("main.extract_pull_requests") +def test_repo_failure_is_tagged_for_sentry( + mock_extract, + mock_session_class, + mock_bq_client, + mock_setup_logging, + mock_sentry, +): + """A failed repo is reported under a Sentry scope tagged with the repo name.""" + mock_extract.side_effect = main.TooManyRetriesError("GitHub API error 502") + scope = mock_sentry.new_scope.return_value.__enter__.return_value + + with patch.dict( + os.environ, + { + "GITHUB_REPOS": "mozilla/firefox", + "BIGQUERY_PROJECT": "test", + "BIGQUERY_DATASET": "test", + }, + clear=True, + ): + result = main.main() + + assert result == 0 + scope.set_tag.assert_called_once_with("repo", "mozilla/firefox") + mock_sentry.flush.assert_called_once() + + +@patch("main.sentry_sdk") +@patch("main.setup_logging") +def test_missing_config_is_reported_and_still_fails(mock_setup_logging, mock_sentry): + """Missing required env vars are sent to Sentry and still exit non-zero.""" + with patch.dict(os.environ, {}, clear=True): + with pytest.raises(SystemExit): + main.main() + + mock_sentry.capture_message.assert_called_once() + assert "BIGQUERY_PROJECT" in mock_sentry.capture_message.call_args.args[0] + mock_sentry.flush.assert_called_once() + + +@patch("main.sentry_sdk") +@patch("main.setup_logging") +@patch("main._main", side_effect=RuntimeError("token exchange failed")) +def test_runtime_error_still_fails(mock_main, mock_setup_logging, mock_sentry): + """A top-level RuntimeError still returns a non-zero exit code.""" + assert main.main() == 1 + mock_sentry.flush.assert_called_once() + + +class TestSetupSentry: + """Tests for setup_sentry initialization.""" + + @patch("main.sentry_sdk.init") + def test_noop_without_dsn(self, mock_init): + with patch.dict(os.environ, {}, clear=True): + main.setup_sentry() + mock_init.assert_not_called() + + @patch("main.sentry_sdk.init") + def test_initializes_with_dsn(self, mock_init): + dsn = "https://key@o0.ingest.sentry.io/0" + with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): + main.setup_sentry() + mock_init.assert_called_once() + assert mock_init.call_args.kwargs["dsn"] == dsn + + class TestResolveMaxWorkers: """Tests for _resolve_max_workers worker-count resolution.""" From f1290ee9ddff9cef3f2693e7bace645816c47468 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 16:22:53 -0400 Subject: [PATCH 2/4] Copilot review fixes --- main.py | 55 ++++++++++++++++++++++++++++++++-------------- tests/test_main.py | 24 ++++++++++++++------ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/main.py b/main.py index 4c3cf95..46005d3 100755 --- a/main.py +++ b/main.py @@ -400,23 +400,30 @@ def setup_logging() -> None: ) -def setup_sentry() -> None: +def setup_sentry() -> bool: """ Report errors to Sentry when SENTRY_DSN is set; no-op otherwise. logger.error/logger.exception calls become Sentry events and lower levels are attached as breadcrumbs. SENTRY_ENVIRONMENT and SENTRY_RELEASE are read from the environment by the SDK. + + Returns: + True if Sentry was initialized, False if SENTRY_DSN is unset. """ dsn = os.environ.get("SENTRY_DSN") if not dsn: - return + return False sentry_sdk.init( dsn=dsn, integrations=[ LoggingIntegration(level=logging.INFO, event_level=logging.ERROR) ], + # Tracebacks pass through frames holding the GitHub private key, app JWT + # and installation tokens; never ship frame locals to Sentry. + include_local_variables=False, ) + return True def _parse_github_timestamp(value: str | None) -> datetime | None: @@ -1732,6 +1739,25 @@ def _flush_pending() -> None: return processed +def _process_repo_in_sentry_scope(repo: str, *args, **kwargs) -> int: + """ + Run process_repo for *repo* inside its own Sentry isolation scope. + + Runs in the worker thread so the retry/backoff warnings logged while + processing the repo are attached as breadcrumbs to the failure event (Sentry + scopes are per-thread, and pool threads are reused across repos). The event + is tagged with the repo for filtering. Exceptions are logged then re-raised + so the caller can record the repo as failed. + """ + with sentry_sdk.isolation_scope() as scope: + scope.set_tag("repo", repo) + try: + return process_repo(repo, *args, **kwargs) + except Exception as exc: + logger.exception(f"Failed to process repo {repo}: {exc}") + raise + + def main() -> int: """ Main ETL process with chunked processing. @@ -1743,9 +1769,9 @@ def main() -> int: 4. Repeat until no more data """ setup_logging() - setup_sentry() + sentry_enabled = setup_sentry() try: - return _main() + return _main(sentry_enabled) except RuntimeError as e: logger.exception(str(e)) return 1 @@ -1758,7 +1784,7 @@ def main() -> int: sentry_sdk.flush() -def _main() -> int: +def _main(sentry_enabled: bool = False) -> int: logger.info("Starting GitHub ETL process with chunked processing") github_app_id = os.environ.get("GITHUB_APP_ID") or None @@ -1839,7 +1865,7 @@ def _main() -> int: with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: future_to_repo = { executor.submit( - process_repo, + _process_repo_in_sentry_scope, repo, github_app_id, github_private_key, @@ -1858,16 +1884,12 @@ def _main() -> int: repo = future_to_repo[future] try: processed = future.result() - except Exception as exc: + except Exception: # Catch broadly so one repo's failure (a TooManyRetriesError, a # RuntimeError, or a bare Exception from load_data) is recorded as a # failed repo rather than propagating out of the executor and - # discarding the results of other in-flight repos. logger.exception - # records the worker thread's traceback for debugging in CI/prod. - # The Sentry event is tagged with the repo for filtering. - with sentry_sdk.new_scope() as scope: - scope.set_tag("repo", repo) - logger.exception(f"Failed to process repo {repo}: {exc}") + # discarding the results of other in-flight repos. The worker has + # already logged/reported it (see _process_repo_in_sentry_scope). failed_repos.append(repo) continue total_processed += processed @@ -1876,12 +1898,13 @@ def _main() -> int: ) if failed_repos: - # Per-repo failures were already reported to Sentry above. Exit 0 so a - # partial failure does not mark the cron job as failed. logger.warning( f"ETL completed with failures. Failed repos: {', '.join(failed_repos)}" ) - return 0 + # With Sentry configured, per-repo failures were reported there, so exit 0 + # to avoid marking the cron job as failed. Without it, the exit code is the + # only signal, so fail the run. + return 0 if sentry_enabled else 1 logger.info( f"GitHub ETL process completed successfully. Total PRs processed: {total_processed}" diff --git a/tests/test_main.py b/tests/test_main.py index eada617..4e599f3 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -540,7 +540,7 @@ def extract_side_effect(*args, **kwargs): ): result = main.main() - assert result == 0 # partial failure is reported to Sentry, not the exit code + assert result == 1 # no SENTRY_DSN, so the exit code must signal the failure assert mock_extract.call_count == 2 # both repos were attempted mock_load.assert_called_once() # only the successful repo loaded data @@ -562,7 +562,7 @@ def test_bare_exception_on_one_repo_is_isolated( """A bare Exception (e.g. from load_data) on one repo must not abort the others. The executor catches broadly, so the failing repo is recorded in failed_repos - (reported to Sentry, overall exit code 0) while the healthy repo still completes its load. + (exit code 1 without SENTRY_DSN) while the healthy repo still completes its load. """ # Fresh iterator per repo (a shared return_value iterator would be exhausted # by whichever repo consumes it first). @@ -597,7 +597,7 @@ def transform_side_effect(chunk, repo): ): result = main.main() - assert result == 0 # partial failure recorded, run did not abort + assert result == 1 # partial failure recorded, run did not abort assert mock_extract.call_count == 2 # both repos were attempted assert mock_load.call_count == 2 # both repos reached the load step @@ -614,13 +614,15 @@ def test_repo_failure_is_tagged_for_sentry( mock_setup_logging, mock_sentry, ): - """A failed repo is reported under a Sentry scope tagged with the repo name.""" + """A failed repo is reported under a Sentry scope tagged with the repo name, + and the run exits 0 because Sentry is configured.""" mock_extract.side_effect = main.TooManyRetriesError("GitHub API error 502") - scope = mock_sentry.new_scope.return_value.__enter__.return_value + scope = mock_sentry.isolation_scope.return_value.__enter__.return_value with patch.dict( os.environ, { + "SENTRY_DSN": "https://key@o0.ingest.sentry.io/0", "GITHUB_REPOS": "mozilla/firefox", "BIGQUERY_PROJECT": "test", "BIGQUERY_DATASET": "test", @@ -662,17 +664,25 @@ class TestSetupSentry: @patch("main.sentry_sdk.init") def test_noop_without_dsn(self, mock_init): with patch.dict(os.environ, {}, clear=True): - main.setup_sentry() + assert main.setup_sentry() is False mock_init.assert_not_called() @patch("main.sentry_sdk.init") def test_initializes_with_dsn(self, mock_init): dsn = "https://key@o0.ingest.sentry.io/0" with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): - main.setup_sentry() + assert main.setup_sentry() is True mock_init.assert_called_once() assert mock_init.call_args.kwargs["dsn"] == dsn + @patch("main.sentry_sdk.init") + def test_does_not_send_local_variables(self, mock_init): + """Frame locals hold the private key, JWT and tokens; never send them.""" + dsn = "https://key@o0.ingest.sentry.io/0" + with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): + main.setup_sentry() + assert mock_init.call_args.kwargs["include_local_variables"] is False + class TestResolveMaxWorkers: """Tests for _resolve_max_workers worker-count resolution.""" From c50f95ca7b3b0997013dd34b5a30c827ce780dc3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 17:49:14 -0400 Subject: [PATCH 3/4] Copilot review fixes --- main.py | 26 ++++++++++++++------------ tests/test_main.py | 15 +++++++++++++++ tests/test_token_store.py | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 62 insertions(+), 13 deletions(-) diff --git a/main.py b/main.py index 46005d3..c94b6b8 100755 --- a/main.py +++ b/main.py @@ -364,12 +364,12 @@ def get_installation_access_token( expected_status=201, ) + # Never include resp.text in these errors: the body may carry the + # token, and error messages are reported to Sentry. try: data = resp.json() except requests.exceptions.JSONDecodeError as e: - raise RuntimeError( - f"Failed to parse access token response: {e}: {resp.text}" - ) + raise RuntimeError(f"Failed to parse access token response: {e}") try: access_token = AccessToken( token=data["token"], @@ -377,7 +377,8 @@ def get_installation_access_token( ) except KeyError as e: raise RuntimeError( - f"Unexpected access token response structure, missing key {e}: {resp.text}" + f"Unexpected access token response structure, missing key {e}; " + f"keys present: {sorted(data)}" ) except ValueError as e: raise RuntimeError( @@ -404,9 +405,10 @@ def setup_sentry() -> bool: """ Report errors to Sentry when SENTRY_DSN is set; no-op otherwise. - logger.error/logger.exception calls become Sentry events and lower levels - are attached as breadcrumbs. SENTRY_ENVIRONMENT and SENTRY_RELEASE are read - from the environment by the SDK. + Log records at INFO and above are attached as breadcrumbs only; events are + sent explicitly with sentry_sdk.capture_exception/capture_message so each + failure is reported exactly once (several paths log an error and then raise). + SENTRY_ENVIRONMENT and SENTRY_RELEASE are read from the environment by the SDK. Returns: True if Sentry was initialized, False if SENTRY_DSN is unset. @@ -416,9 +418,7 @@ def setup_sentry() -> bool: return False sentry_sdk.init( dsn=dsn, - integrations=[ - LoggingIntegration(level=logging.INFO, event_level=logging.ERROR) - ], + integrations=[LoggingIntegration(level=logging.INFO, event_level=None)], # Tracebacks pass through frames holding the GitHub private key, app JWT # and installation tokens; never ship frame locals to Sentry. include_local_variables=False, @@ -1746,8 +1746,8 @@ def _process_repo_in_sentry_scope(repo: str, *args, **kwargs) -> int: Runs in the worker thread so the retry/backoff warnings logged while processing the repo are attached as breadcrumbs to the failure event (Sentry scopes are per-thread, and pool threads are reused across repos). The event - is tagged with the repo for filtering. Exceptions are logged then re-raised - so the caller can record the repo as failed. + is tagged with the repo for filtering. Exceptions are logged, reported, then + re-raised so the caller can record the repo as failed. """ with sentry_sdk.isolation_scope() as scope: scope.set_tag("repo", repo) @@ -1755,6 +1755,7 @@ def _process_repo_in_sentry_scope(repo: str, *args, **kwargs) -> int: return process_repo(repo, *args, **kwargs) except Exception as exc: logger.exception(f"Failed to process repo {repo}: {exc}") + sentry_sdk.capture_exception(exc) raise @@ -1774,6 +1775,7 @@ def main() -> int: return _main(sentry_enabled) except RuntimeError as e: logger.exception(str(e)) + sentry_sdk.capture_exception(e) return 1 except SystemExit as e: # Missing configuration: report it, then still fail the run. diff --git a/tests/test_main.py b/tests/test_main.py index 4e599f3..a88a925 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,3 +1,4 @@ +import logging import os import threading from unittest.mock import MagicMock, Mock, patch @@ -633,6 +634,8 @@ def test_repo_failure_is_tagged_for_sentry( assert result == 0 scope.set_tag.assert_called_once_with("repo", "mozilla/firefox") + # Reported exactly once, even though the failure is also logged. + mock_sentry.capture_exception.assert_called_once() mock_sentry.flush.assert_called_once() @@ -655,6 +658,7 @@ def test_missing_config_is_reported_and_still_fails(mock_setup_logging, mock_sen def test_runtime_error_still_fails(mock_main, mock_setup_logging, mock_sentry): """A top-level RuntimeError still returns a non-zero exit code.""" assert main.main() == 1 + mock_sentry.capture_exception.assert_called_once() mock_sentry.flush.assert_called_once() @@ -683,6 +687,17 @@ def test_does_not_send_local_variables(self, mock_init): main.setup_sentry() assert mock_init.call_args.kwargs["include_local_variables"] is False + @patch("main.LoggingIntegration") + @patch("main.sentry_sdk.init") + def test_logs_are_breadcrumbs_only(self, mock_init, mock_logging_integration): + """Log records must not become events; failures are captured explicitly.""" + dsn = "https://key@o0.ingest.sentry.io/0" + with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): + main.setup_sentry() + mock_logging_integration.assert_called_once_with( + level=logging.INFO, event_level=None + ) + class TestResolveMaxWorkers: """Tests for _resolve_max_workers worker-count resolution.""" diff --git a/tests/test_token_store.py b/tests/test_token_store.py index 24c59fe..4c043ea 100644 --- a/tests/test_token_store.py +++ b/tests/test_token_store.py @@ -6,10 +6,15 @@ when X-RateLimit-Remaining is 0. - TokenStore: caches valid tokens, expires tokens within the 60s skew window, and hands out a stable per-installation lock. + - get_installation_access_token: error messages for malformed token responses + never include the response body (it may carry the token). """ from datetime import datetime, timedelta, timezone -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock, patch + +import pytest +import requests import main from main import AccessToken, TokenStore, _is_rate_limited @@ -82,3 +87,30 @@ def test_token_store_lock_is_stable_per_installation(): def test_module_level_token_store_exists(): assert isinstance(main.token_store, TokenStore) + + +@pytest.mark.parametrize( + "json_side_effect", + [ + # Token present but expires_at missing. + [{"token": "ghs_secret"}], + # Body is not valid JSON. + requests.exceptions.JSONDecodeError("bad", '{"token": "ghs_secret"', 0), + ], +) +def test_access_token_errors_do_not_leak_response_body(json_side_effect): + """Error messages reach Sentry, so they must never contain the token.""" + resp = Mock() + resp.json.side_effect = json_side_effect + resp.text = '{"token": "ghs_secret"' + + with ( + patch("main._build_session", return_value=MagicMock()), + patch("main.github_request", return_value=resp), + patch("main.token_store", TokenStore()), + patch.dict(main.repo_installation_cache, {"mozilla/firefox": 1}), + ): + with pytest.raises(RuntimeError) as excinfo: + main.get_installation_access_token("jwt", "mozilla/firefox", "https://api") + + assert "ghs_secret" not in str(excinfo.value) From 8cef21e80b9e93be4edc430f0e267dcf4ca4f2e5 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 9 Oct 2026 14:48:07 -0400 Subject: [PATCH 4/4] Review fixes --- tests/test_main.py | 49 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/tests/test_main.py b/tests/test_main.py index a88a925..feff4f3 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,9 +1,11 @@ import logging import os import threading +import uuid from unittest.mock import MagicMock, Mock, patch import pytest +import sentry_sdk import main @@ -679,13 +681,48 @@ def test_initializes_with_dsn(self, mock_init): mock_init.assert_called_once() assert mock_init.call_args.kwargs["dsn"] == dsn - @patch("main.sentry_sdk.init") - def test_does_not_send_local_variables(self, mock_init): - """Frame locals hold the private key, JWT and tokens; never send them.""" + def test_does_not_send_local_variables(self): + """Frame locals hold the private key, JWT and tokens. Never send them.""" + # Generated at runtime so it can't leak via source context lines, + # only via frame locals. + secret = uuid.uuid4().hex + events = [] + + class CapturingTransport(sentry_sdk.transport.Transport): + def capture_envelope(self, envelope): + events.append(envelope.get_event()) + + real_init = sentry_sdk.init + + def init_with_capturing_transport(*args, **kwargs): + return real_init(*args, transport=CapturingTransport(), **kwargs) + + def fail_with_secret_in_scope(): + private_key = secret # noqa: F841 + raise RuntimeError("token exchange failed") + dsn = "https://key@o0.ingest.sentry.io/0" - with patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True): - main.setup_sentry() - assert mock_init.call_args.kwargs["include_local_variables"] is False + try: + with ( + patch.dict(os.environ, {"SENTRY_DSN": dsn}, clear=True), + patch( + "main.sentry_sdk.init", side_effect=init_with_capturing_transport + ), + ): + assert main.setup_sentry() is True + try: + fail_with_secret_in_scope() + except RuntimeError: + sentry_sdk.capture_exception() + finally: + sentry_sdk.get_client().close() + sentry_sdk.get_global_scope().set_client(None) + + assert len(events) == 1 + frames = events[0]["exception"]["values"][-1]["stacktrace"]["frames"] + assert any(f["function"] == "fail_with_secret_in_scope" for f in frames) + assert all("vars" not in f for f in frames) + assert secret not in repr(events[0]) @patch("main.LoggingIntegration") @patch("main.sentry_sdk.init")