From 13e241140d2a4bdbf0d456d40101c7287d7554e3 Mon Sep 17 00:00:00 2001 From: arobson-ods Date: Mon, 7 Sep 2026 11:04:47 +0100 Subject: [PATCH 1/4] chore: ignore .pem files --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index db03d9c..7418cb1 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,6 @@ __pycache__/ /.env /test.db /test-audit-log-public-key.pem + +# all key files +*.pem From 241682108ea0202bae74de04baf2739772993ae5 Mon Sep 17 00:00:00 2001 From: arobson-ods Date: Mon, 7 Sep 2026 12:33:53 +0100 Subject: [PATCH 2/4] build: add sentry-sdk as an explicit dependency Adds sentry-sdk ~=2.35 to pyproject.toml ahead of wiring up Sentry error reporting. It was previously present only transitively, via fastapi[standard] -> fastapi-cli -> fastapi-cloud-cli (sentry-sdk>=2.20.0), which resolved to 2.34.1; declaring it directly moves it to the latest 2.x (2.68.1). The <3.0 bound is deliberate: a break in error reporting is silent, so a 3.x migration should be a conscious change rather than something a routine lockfile refresh can do. Regenerates requirements.txt and requirements_dev.txt in a linux/amd64 container matching the Dockerfile base. Regenerating on macOS drops greenlet, because SQLAlchemy requires it on x86_64 and aarch64 but not on Apple Silicon's arm64, which loses the pin for the deployment platform. Corrects the pip-compile invocation recorded in the README for requirements.txt, notes that the two lockfiles take different flags, and documents that regeneration must happen on Linux. --- README.md | 11 ++++++++++- pyproject.toml | 1 + requirements.txt | 6 ++++-- requirements_dev.txt | 6 ++++-- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 0487f7d..f2c6509 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ Care should be taken to make sure that the `.env` variables match the log (`APP_ New dependencies are added to `pyproject.toml`. Once these have been added `requirements.txt` and/or `requirements_dev.txt` need to be regenerated. With: ``` -pip-compile --output-file=requirements.txt --strip-extras +pip-compile --all-build-deps --strip-extras ``` and/or @@ -132,6 +132,15 @@ and/or pip-compile --extra=dev --output-file=requirements_dev.txt --strip-extras ``` +Note that the two commands take different options, so run each as given above rather than +applying one set of flags to both files. The command line recorded at the top of each +generated file is the authoritative record of how that file was built. + +**Regenerate on Linux, not on macOS.** `pip-compile` resolves for the platform it runs on and +has no cross-platform mode, so a macOS run silently drops dependencies that the deployment +target needs. SQLAlchemy, for example, requires `greenlet` on `x86_64` and `aarch64` but not +on Apple Silicon's `arm64`, so a run on an M-series Mac omits it and loses the pin. + ### Checking and linting Linting is setup with `isort` and `black` and checked with `flake8`. Static type checking is performed by `mypy`. Configurations are stored in `pyproject.toml`. To use these linters and checkers you will first need to install the development dependencies: diff --git a/pyproject.toml b/pyproject.toml index f7f7ba8..24bf132 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ dependencies = [ "python-dotenv>=1.1.1", "psycopg[binary]>3", "requests==2.32.4", + "sentry-sdk~=2.35", "sqlmodel==0.0.25", "SQLAlchemy==2.0.43", "types-requests==2.32.4.20250611" diff --git a/requirements.txt b/requirements.txt index e29e9f9..9577817 100644 --- a/requirements.txt +++ b/requirements.txt @@ -135,8 +135,10 @@ rich-toolkit==0.14.9 # fastapi-cloud-cli rignore==0.6.4 # via fastapi-cloud-cli -sentry-sdk==2.34.1 - # via fastapi-cloud-cli +sentry-sdk==2.68.1 + # via + # fastapi-cloud-cli + # register-your-data-api (pyproject.toml) shellingham==1.5.4 # via typer sniffio==1.3.1 diff --git a/requirements_dev.txt b/requirements_dev.txt index 248341a..3fef94a 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -199,8 +199,10 @@ rich-toolkit==0.14.9 # fastapi-cloud-cli rignore==0.6.4 # via fastapi-cloud-cli -sentry-sdk==2.34.1 - # via fastapi-cloud-cli +sentry-sdk==2.68.1 + # via + # fastapi-cloud-cli + # register-your-data-api (pyproject.toml) shellingham==1.5.4 # via typer smmap==5.0.2 From a0a5a8946336280a0f9bb5b062b504621a5b86a4 Mon Sep 17 00:00:00 2001 From: arobson-ods Date: Thu, 10 Sep 2026 12:12:31 +0100 Subject: [PATCH 3/4] feat: report errors and traces to Sentry Initialises the SDK in src/main.py before the application object is created. Optional by design: no DSN, a malformed DSN, or a test run leaves it uninitialised and the API unaffected. sentry.py is the only module importing sentry_sdk; application code reports errors by logging them, so the service can be replaced by editing one file. Withholds what the SDK would otherwise transmit: frame locals and request bodies (both carry bearer tokens, and bodies are sent on successful requests too), outgoing query strings (SuiteCRM record IDs), and the audit log entirely (its CRITICAL records contain the Authorization header). A failed startup is now logged rather than printed, so the failure that stops the service is reported too. --- .env.example | 8 + CHANGELOG.md | 12 + README.md | 75 ++++- src/main.py | 18 +- src/register_your_data_api/sentry.py | 255 +++++++++++++++ tests/conftest.py | 4 + tests/unit/test_main.py | 32 ++ tests/unit/test_sentry.py | 451 +++++++++++++++++++++++++++ 8 files changed, 852 insertions(+), 3 deletions(-) create mode 100644 src/register_your_data_api/sentry.py create mode 100644 tests/unit/test_main.py create mode 100644 tests/unit/test_sentry.py diff --git a/.env.example b/.env.example index 8c102b3..51eaa99 100644 --- a/.env.example +++ b/.env.example @@ -47,3 +47,11 @@ PROMETHEUS_PORT = 1111 JWKS_URI = "https://example.org/jwks" JWT_AUDIENCE = "some_audience" + +# Sentry error monitoring. Optional: leave SENTRY_DSN empty or unset to disable Sentry +# entirely - the application runs normally without it. +SENTRY_DSN="" + +SENTRY_ENVIRONMENT="local-development" + +SENTRY_TRACES_SAMPLE_RATE=1.0 diff --git a/CHANGELOG.md b/CHANGELOG.md index bad0438..afae81c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Sentry error monitoring and request tracing, initialised in `src/main.py` before the + FastAPI application is created. Configured by the optional `SENTRY_DSN`, + `SENTRY_ENVIRONMENT`, and `SENTRY_TRACES_SAMPLE_RATE` environment variables; when no + DSN is set the SDK is not initialised and the application runs unchanged, and neither is + it when the DSN is malformed or when running under pytest. Stack frame local variables, + request bodies, and outgoing request query strings are deliberately not sent: the local + variables and the bodies would otherwise transmit bearer tokens and contact details, and + the query strings carry the record IDs a request touched. The audit log is excluded + entirely, since its records are written at `CRITICAL` and carry the contents of the + `Authorization` header, and a failed startup is now logged rather than printed so that + it is reported as well. + ### Changed ### Deprecated diff --git a/README.md b/README.md index f2c6509..23a1767 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,80 @@ The application is configured using a set of environment variables in a `.env` f | `AUDIT_LOG_PUBLIC_KEY_PATH` | Path to the public key for encrypting audit logs. | | `PROMETHEUS_PORT` | Port to serve Prometheus metrics from. | | `JWT_AUDIENCE` | Audience that we expect to find in JWTs from the identity server. | - +| `SENTRY_DSN` | DSN of the Sentry project to report errors to. **Optional** — leave empty or unset and Sentry is not initialised, and the application runs normally. | +| `SENTRY_ENVIRONMENT` | Environment name that events are tagged with in Sentry, e.g. `"dev"` or `"prod"`. Defaults to `"local-development"` rather than the SDK's own default of `"production"`, so that an unconfigured environment can never be mistaken for the live one. | +| `SENTRY_TRACES_SAMPLE_RATE` | Proportion of requests traced for performance monitoring, `0.0`–`1.0`. Defaults to `1.0`; lower it (e.g. `0.1`) if trace volume becomes a problem. | + +### Error monitoring with Sentry + +Sentry is initialised in `src/main.py` by `register_your_data_api.sentry.setup_sentry()`, +before the FastAPI application object is created, so that its Starlette/FastAPI +integrations are in place. It reports unhandled exceptions and traces requests. + +Configuration is read directly from the environment rather than through `Context`, +because `Context` is not created until the application lifespan runs, which is after the +SDK has to be initialised. Sentry is optional by design: error reporting should never be +the reason the API fails to start, so a missing DSN disables it rather than raising, and +so does a DSN the SDK rejects as malformed — `sentry_sdk.init` raises `BadDsn` for those, +which would otherwise stop the process at import time before there is a logger to report +it to. + +The test suite must not report to a real project. It imports `src/main.py` through +`tests/helpers/mocking.py`, so `setup_sentry()` runs during collection and a developer's +`.env` would otherwise point the suite's deliberately-provoked errors — and a transaction +per request — at whichever project that DSN names. `tests/conftest.py` empties +`SENTRY_DSN` for the session, which is the SDK's own way of being switched off. + +`include_local_variables=False` because otherwise Sentry would attach the local +variables of every stack frame to an event, and this application's credentials reach +the stack in forms which cannot all be recognised by name: an ASGI frame's locals hold +the raw `scope`/`request` objects, whose headers are a list of `(bytes, bytes)` tuples +rather than a named field, and the bearer token is a local variable in `auth/authn.py` +in its own right as well as a field of `UserAndCredentials`. Local variables are +therefore not sent at all, which costs the variable values in a traceback but keeps +the file, line, function and source line of every frame. With frame locals enabled +a live bearer token is transmitted in the event payload . + +`send_default_pii=False` keeps cookies and the client IP address out of events, and the +`EventScrubber` denylist in `sentry.py` withholds this application's own secrets by name +wherever the SDK collects them by other means. + +Be careful about what `send_default_pii=False` does **not** do, because the option name +suggests more than it delivers: + +* It does not keep request **headers** out of events. The SDK substitutes the sensitive + ones and passes the rest through, so `host`, `user-agent` and `content-type` are sent. + The `Authorization` header is withheld by the scrubber's `authorization` denylist entry, + not by this option. +* It does not keep request **bodies** out of events — the SDK collects those regardless of + it, bounded by `max_request_body_size`. Because tracing is enabled the body rides out + on the transaction event for **successful** requests too, so a `POST` creating a + reporting org would have transmitted its `contact_email` on every call. + `max_request_body_size="never"` is what actually withholds them. + +Deciding that a variable holds a credential is still a manual step: +`tests/unit/test_sentry.py::test_sentry_scrubs_every_configuration_variable_which_looks_like_a_secret` +is a backstop that catches the common cases by name fragment, but a credential with an +unremarkable name would satisfy it. + +Sentry also records the query string of every outgoing HTTP request, with the values +intact. This application queries SuiteCRM by building record filters into the query +string, so those values carry the IDs of the people and organisations a request touched; +`before_breadcrumb` and `before_send_transaction` withhold them. The method, the URL +without its query string and the response status are kept, so a breadcrumb still says +which request was being made. + +**The audit log is never sent to Sentry.** Sentry turns any log record of `ERROR` or +above into an event, and the audit log is written at `CRITICAL` for authentication +failures — where `auth/authn.py` records the contents of the `Authorization` header, +including the credential itself, for a request whose scheme is not `bearer`. Sending +those records to a third party in plain text would defeat the point of encrypting the +audit log at rest, so `sentry.py` calls both `ignore_logger()` and +`ignore_logger_for_sentry_logs()` for it — the SDK keeps two separate ignore lists and +the first covers only events and breadcrumbs. Sentry Logs are off, so the second call +changes nothing today; it is there so that enabling them later cannot silently start +sending the audit log. The application's diagnostic log is still reported, which is +how application code reports errors without referencing the SDK. ### FineGrainedAuthorisation database migrations diff --git a/src/main.py b/src/main.py index 4034218..61344be 100644 --- a/src/main.py +++ b/src/main.py @@ -1,6 +1,7 @@ """Register Your Data API""" import contextlib +import logging import sys from typing import AsyncIterator @@ -10,6 +11,7 @@ import register_your_data_api.exception_handlers import register_your_data_api.util as util from register_your_data_api.routers import datasets, discoverable_reporting_orgs, misc, reporting_orgs, users +from register_your_data_api.sentry import setup_sentry @contextlib.asynccontextmanager @@ -19,8 +21,14 @@ async def prod_lifespan(app: FastAPI) -> AsyncIterator[None]: context.setup() prometheus_client.start_http_server(int(context.env["PROMETHEUS_PORT"])) - except Exception as err: - print(f"Could not initialise application - error setting up context {err}") + except Exception: + # Logged rather than printed so that it reaches error monitoring: a service which + # will not start is the failure most worth being told about, and SystemExit is a + # BaseException, so nothing downstream of this would report it. With no handlers + # configured — Context, and therefore the application logger, is what just failed — + # logging still writes the message and traceback to stderr via its last-resort + # handler. The SDK flushes on process exit. + logging.getLogger(__name__).exception("Could not initialise application - error setting up context") sys.exit("Could not startup") app.state.context = context @@ -37,6 +45,12 @@ def add_routers_and_general_exception_handling(app: FastAPI) -> None: register_your_data_api.exception_handlers.add_exception_handlers(app) +# Sentry must be initialised before the application object below is created, so that its +# Starlette/FastAPI integrations are in place for it. The integrations patch modules that +# are already imported, so only the object's creation has to come after this, not the +# imports. A no-op when no DSN is configured. +setup_sentry() + app = FastAPI(title="Register Your Data", lifespan=prod_lifespan, redirect_slashes=False) add_routers_and_general_exception_handling(app) diff --git a/src/register_your_data_api/sentry.py b/src/register_your_data_api/sentry.py new file mode 100644 index 0000000..b4728e5 --- /dev/null +++ b/src/register_your_data_api/sentry.py @@ -0,0 +1,255 @@ +"""Sentry error monitoring and request tracing. + +Sentry has to be initialised before the FastAPI application object is created, which is +earlier than the ``Context`` (created in the application lifespan) exists. Configuration +is therefore read straight from the environment here rather than going through +``Context``, using the same precedence that ``Context`` uses: values from the ``.env`` +file in the current directory, overridden by real environment variables. + +This follows the same approach as the IATI Bulk Data Service's ``src/config/sentry.py``, +so that the two services withhold credentials from Sentry in the same way. +""" + +import importlib.metadata +import os +from typing import Any, Final + +import dotenv +import sentry_sdk +from sentry_sdk.integrations.logging import ignore_logger, ignore_logger_for_sentry_logs +from sentry_sdk.scrubber import DEFAULT_DENYLIST, EventScrubber +from sentry_sdk.types import Breadcrumb, BreadcrumbHint, Event, Hint + +# Sentry's SDK sends no traces at all unless a sample rate is set, so a default is +# supplied here rather than leaving it to the SDK +DEFAULT_TRACES_SAMPLE_RATE: Final[float] = 1.0 + +# The audit logger, whose records must never be sent to Sentry. Sentry turns any log +# record of ERROR or above into an event, and the audit log is written at CRITICAL for +# authentication failures; those records carry the contents of the Authorization header, +# including the credential itself (see auth/authn.py). The audit log has its own +# encrypted destination and must not be duplicated to a third party in plain text. +# Must match the logger name used by util.Context._setup_loggers. +AUDIT_LOGGER_NAME: Final[str] = "ryd-api-audit" + +# Used when SENTRY_ENVIRONMENT is not set, in preference to the SDK's own default of +# 'production', so that an unconfigured environment can never be mistaken for the live one +UNCONFIGURED_ENVIRONMENT: Final[str] = "local-development" + +# What Sentry itself puts in place of a value it withholds +WITHHELD: Final[str] = "[Filtered]" + +# The parts of a request's URL which Sentry records with their values intact, and which +# can therefore carry a credential or an identifier. +# +# Both of the SDK's naming schemes are listed. Which one is used depends on whether span +# streaming is enabled: the SDK's http.client instrumentation records 'http.query' and +# 'http.fragment' without regard to send_default_pii, and 'url.query' and 'url.fragment' +# only when send_default_pii is set (sentry_sdk/integrations/stdlib.py). This application +# currently takes the former path, which is the one that needs withholding; the latter is +# listed so that a future SDK release which changes the default cannot quietly stop these +# from matching. +REQUEST_URL_PARTS_TO_WITHHOLD: Final[tuple[str, ...]] = ( + "http.query", + "http.fragment", + "url.query", + "url.fragment", +) + +# Names of this application's own secrets and sensitive fields, withheld by name in +# addition to Sentry's own default denylist. The audit log is deliberately encrypted at +# rest, so audit content must not be shipped to a third party in plain text either. +SENSITIVE_NAMES: Final[list[str]] = [ + "access_token", + "audit_msg", + "AUDIT_LOG_PRIVATE_KEY_PATH", + "AZURE_COMMUNICATION_SERVICE_CONNECTION_STRING", + "DATA_REGISTRY_SUITECRM_CLIENT_ID", + "DATA_REGISTRY_SUITECRM_CLIENT_SECRET", + "FGA_PROVIDER_CONNECTION_STRING", + "SENTRY_DSN", + "suitecrm_audit_headers", + "USER_CRM_UUID_CONFIG_STRING", +] + + +def get_environment_config() -> dict[str, str]: + """Reads configuration with the same precedence as ``Context``: ``.env`` then os.environ.""" + + env: dict[str, str] = {k: v for k, v in dotenv.dotenv_values(".env").items() if v is not None} + env.update(os.environ) + return env + + +def get_release() -> str | None: + """Builds the Sentry release identifier from the installed package version.""" + + try: + return f"register-your-data-api@{importlib.metadata.version('register-your-data-api')}" + except importlib.metadata.PackageNotFoundError: + return None + + +def setup_sentry(config: dict[str, str] | None = None) -> bool: + """Initialises Sentry error reporting and tracing, if a DSN has been configured. + + The Sentry SDK does nothing when it has no DSN, so local development and test runs + need no Sentry setup at all: leaving SENTRY_DSN unset means nothing is sent anywhere. + Error reporting must also never be the reason the API fails to start. + + Parameters + ---------- + config : dict[str, str] | None, optional + Configuration to use. Read from the environment when not supplied. + + Returns + ------- + bool + True if Sentry was initialised, False if it was skipped for want of a DSN. + """ + + settings = get_environment_config() if config is None else config + + if not settings.get("SENTRY_DSN", "").strip(): + print("Sentry: SENTRY_DSN is not set, so error reporting is disabled") + return False + + environment = settings.get("SENTRY_ENVIRONMENT", "").strip() or UNCONFIGURED_ENVIRONMENT + + try: + initialise_sdk(settings["SENTRY_DSN"].strip(), environment, get_traces_sample_rate(settings)) + except Exception as err: + # A malformed DSN makes sentry_sdk.init raise BadDsn, and this runs at import time + # in src/main.py, before there is a logger to report it to. Error reporting must + # never be the reason the API fails to start, so a DSN which cannot be used + # disables reporting rather than killing the process. + print(f"Sentry: could not initialise error reporting, so it is disabled ({err})") + return False + + # Keep the audit log out of Sentry entirely. See AUDIT_LOGGER_NAME above: these + # records are written at CRITICAL and carry credentials, and they have their own + # encrypted destination. The application's diagnostic log is still reported. + # + # The SDK keeps two separate ignore lists, and ignore_logger covers only events and + # breadcrumbs — its own docstring says it "does not affect Sentry Logs". Sentry Logs + # are off (`enable_logs` defaults to False) so the second call changes nothing today; + # it is here so that enabling them later cannot silently start sending the audit log. + ignore_logger(AUDIT_LOGGER_NAME) + ignore_logger_for_sentry_logs(AUDIT_LOGGER_NAME) + + print(f"Sentry: error reporting enabled for environment '{environment}'") + + return True + + +def initialise_sdk(dsn: str, environment: str, traces_sample_rate: float) -> None: + """Calls sentry_sdk.init with this application's options.""" + + sentry_sdk.init( + dsn=dsn, + environment=environment, + release=get_release(), + traces_sample_rate=traces_sample_rate, + # this API serves end users and handles their personal data, so there is nothing + # to be gained from letting the SDK attach identifying information to events + send_default_pii=False, + # Request bodies are NOT sent. send_default_pii does not cover them: the SDK + # collects bodies "regardless of PII today, bounded by max_request_body_size" + # (sentry_sdk data collection defaults), and because tracing is enabled the body + # rides out on the transaction event for SUCCESSFUL requests too. Verified: a + # POST to a reporting-org endpoint transmitted its contact_email on success. + max_request_body_size="never", + # Sentry attaches the local variables of every stack frame to an event, and this + # application's credentials reach the stack in forms which cannot all be matched + # by name: an ASGI frame's locals hold the raw scope/request objects, whose + # headers are a list of (bytes, bytes) tuples rather than a named field, and the + # bearer token is itself a local variable in auth.authn and a field of + # UserAndCredentials. Sending no local variables is the only way to withhold all + # of them. + include_local_variables=False, + # kept as well, because it scrubs the values the SDK collects by other means + event_scrubber=EventScrubber( + denylist=DEFAULT_DENYLIST + SENSITIVE_NAMES, + recursive=True, + ), + before_breadcrumb=before_breadcrumb, + before_send_transaction=before_send_transaction, + # the server is stopped by interrupting it, which is a normal way for the process + # to end rather than a fault worth reporting + ignore_errors=[KeyboardInterrupt], + # mark this application's own frames as in-app so they stand out in a traceback + in_app_include=["register_your_data_api", "main"], + ) + + +def before_breadcrumb(crumb: Breadcrumb, _hint: BreadcrumbHint) -> Breadcrumb | None: + """Withholds the query string of the outgoing HTTP requests Sentry records as breadcrumbs. + + Sentry records each request's query string with its values intact. This application + queries SuiteCRM by building filters into the query string, so those values carry the + IDs of the people and organisations a request touched.""" + + withhold_request_url_parts(crumb.get("data")) + + return crumb + + +def before_send_transaction(event: Event, _hint: Hint) -> Event | None: + """Withholds the same query strings from the spans of a transaction. + + Tracing is enabled, so requests to SuiteCRM and the FGA database are sent as spans of + the transaction for the request that made them, and `before_send` is not given + transactions and so cannot cover this.""" + + spans = event.get("spans") + + # Sentry replaces a value it has trimmed with a marker object, so the spans are not + # necessarily a list, and this must not be the thing which raises + if isinstance(spans, list): + for span in spans: + withhold_request_url_parts(span.get("data")) + + return event + + +def withhold_request_url_parts(data: Any) -> None: + """Replaces the parts of a recorded request URL which can carry a credential or an ID. + + The method, the URL without its query string, and the response status are left alone, + so a breadcrumb still says which request was being made.""" + + if not isinstance(data, dict): + return + + for part in REQUEST_URL_PARTS_TO_WITHHOLD: + if part in data: + data[part] = WITHHELD + + +def get_traces_sample_rate(config: dict[str, str]) -> float: + """Returns the configured trace sample rate, falling back to the default if it is + unset or unusable. A bad value here must not stop the app from starting, nor stop + errors from being reported.""" + + configured_rate = config.get("SENTRY_TRACES_SAMPLE_RATE", "").strip() + + if not configured_rate: + return DEFAULT_TRACES_SAMPLE_RATE + + try: + rate = float(configured_rate) + except ValueError: + print( + f"Sentry: SENTRY_TRACES_SAMPLE_RATE '{configured_rate}' is not a number: " + f"using {DEFAULT_TRACES_SAMPLE_RATE}" + ) + return DEFAULT_TRACES_SAMPLE_RATE + + if not 0.0 <= rate <= 1.0: + print( + f"Sentry: SENTRY_TRACES_SAMPLE_RATE '{configured_rate}' is not between 0 and 1: " + f"using {DEFAULT_TRACES_SAMPLE_RATE}" + ) + return DEFAULT_TRACES_SAMPLE_RATE + + return rate diff --git a/tests/conftest.py b/tests/conftest.py index cee7e9e..77baaec 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,7 @@ import logging +import os logging.getLogger("sqlalchemy.engine").setLevel(logging.WARNING) + +# Force an empty Sentry DSN so tests never report to a real project +os.environ["SENTRY_DSN"] = "" diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py new file mode 100644 index 0000000..14b5699 --- /dev/null +++ b/tests/unit/test_main.py @@ -0,0 +1,32 @@ +"""Tests for the application entry point.""" + +import asyncio +import logging +from unittest import mock + +import pytest + +import main + + +def test_a_failed_startup_is_logged_with_the_exception_attached() -> None: + """A service which will not start is worth being told about. `SystemExit` is + a `BaseException`, so nothing downstream of `sys.exit` would see it, and error + monitoring picks up records of ERROR and above. + + The record must carry `exc_info`, because without it the event reaching Sentry would + say only that startup failed and not why.""" + + async def enter_lifespan() -> None: + async with main.prod_lifespan(mock.MagicMock()): + pass + + with mock.patch("register_your_data_api.util.Context", side_effect=RuntimeError("no AUDIT_LOG_PUBLIC_KEY_PATH")): + with mock.patch.object(logging.getLogger("main"), "error") as logged: + with pytest.raises(SystemExit): + asyncio.run(enter_lifespan()) + + # exc_info is what attaches the exception; without it the reported event would say + # only that startup failed and not why. Logger.exception delegates to Logger.error + # with exc_info=True, which is why the assertion is on `error`. + logged.assert_called_once_with("Could not initialise application - error setting up context", exc_info=True) diff --git a/tests/unit/test_sentry.py b/tests/unit/test_sentry.py new file mode 100644 index 0000000..b1d0a34 --- /dev/null +++ b/tests/unit/test_sentry.py @@ -0,0 +1,451 @@ +"""Tests for the Sentry error monitoring setup. + +Mirrors the Bulk Data Service's tests of the same name, so that both services are held +to the same standard about what may leave the machine. +""" + +import logging +import os +from typing import Any +from unittest import mock + +import fastapi +import sentry_sdk +from fastapi.testclient import TestClient +from sentry_sdk.transport import Transport + +from register_your_data_api.sentry import ( + AUDIT_LOGGER_NAME, + DEFAULT_TRACES_SAMPLE_RATE, + UNCONFIGURED_ENVIRONMENT, + WITHHELD, + before_breadcrumb, + before_send_transaction, + setup_sentry, +) +from register_your_data_api.util import Context + +BASE_CONFIG = { + "SENTRY_DSN": "https://examplekey@o0.ingest.sentry.io/1", + "SENTRY_ENVIRONMENT": "dev", + "SENTRY_TRACES_SAMPLE_RATE": "0.25", +} + +# Configuration variable names which hold a credential, recognised by name. A backstop +# against a new secret variable being added to Context without being scrubbed. +SECRET_NAME_FRAGMENTS = ("SECRET", "PASSWORD", "CONNECTION_STRING", "PRIVATE_KEY", "TOKEN", "DSN") + + +def initialise_with(config_overrides: dict[str, str]) -> dict[str, Any]: + """Initialises Sentry with the base config plus the given overrides, and returns the + keyword arguments which would have been passed to the SDK.""" + + with mock.patch("register_your_data_api.sentry.sentry_sdk") as mocked_sdk: + setup_sentry(BASE_CONFIG | config_overrides) + + if not mocked_sdk.init.called: + return {} + + return dict(mocked_sdk.init.call_args.kwargs) + + +def test_sentry_not_initialised_when_no_dsn_configured() -> None: + + assert initialise_with({"SENTRY_DSN": ""}) == {} + + +def test_setup_sentry_reports_whether_it_initialised() -> None: + """main.py calls this at import time; it must not raise when Sentry is switched off.""" + + with mock.patch("register_your_data_api.sentry.sentry_sdk"): + assert setup_sentry(BASE_CONFIG) is True + assert setup_sentry(BASE_CONFIG | {"SENTRY_DSN": ""}) is False + + +def test_sentry_initialised_with_dsn_environment_and_release() -> None: + + init_args = initialise_with({}) + + assert init_args["dsn"] == BASE_CONFIG["SENTRY_DSN"] + assert init_args["environment"] == "dev" + assert init_args["release"] is not None + assert init_args["release"].startswith("register-your-data-api@") + + +def test_sentry_environment_not_reported_as_production_when_unconfigured() -> None: + + assert initialise_with({"SENTRY_ENVIRONMENT": ""})["environment"] == UNCONFIGURED_ENVIRONMENT + + +def test_sentry_does_not_send_stack_frame_variables() -> None: + """The bearer token reaches the stack in forms the scrubber cannot match by name: an + ASGI frame's locals hold the raw scope/request objects, whose headers are a list of + (bytes, bytes) tuples, and the token is a local variable in auth.authn in its own + right. Sending no local variables is the only way to withhold all of them.""" + + assert initialise_with({})["include_local_variables"] is False + + +def test_sentry_does_not_send_personally_identifying_information() -> None: + + assert initialise_with({})["send_default_pii"] is False + + +def test_sentry_ignores_interrupts() -> None: + + assert KeyboardInterrupt in initialise_with({})["ignore_errors"] + + +def test_sentry_marks_this_application_frames_as_in_app() -> None: + + assert "register_your_data_api" in initialise_with({})["in_app_include"] + + +def test_sentry_uses_configured_traces_sample_rate() -> None: + + assert initialise_with({})["traces_sample_rate"] == 0.25 + + +def test_sentry_falls_back_to_default_traces_sample_rate_when_unset() -> None: + + assert initialise_with({"SENTRY_TRACES_SAMPLE_RATE": ""})["traces_sample_rate"] == DEFAULT_TRACES_SAMPLE_RATE + + +def test_sentry_falls_back_to_default_traces_sample_rate_when_not_a_number() -> None: + + assert initialise_with({"SENTRY_TRACES_SAMPLE_RATE": "lots"})["traces_sample_rate"] == DEFAULT_TRACES_SAMPLE_RATE + + +def test_sentry_falls_back_to_default_traces_sample_rate_when_out_of_range() -> None: + + assert initialise_with({"SENTRY_TRACES_SAMPLE_RATE": "50"})["traces_sample_rate"] == DEFAULT_TRACES_SAMPLE_RATE + + +def test_sentry_scrubs_secrets_nested_inside_collected_values() -> None: + """The secret values sit inside dictionaries rather than being top-level fields, so + scrubbing has to be recursive.""" + + assert initialise_with({})["event_scrubber"].recursive is True + + +def test_sentry_scrubs_every_configuration_variable_which_looks_like_a_secret() -> None: + """A backstop: if a new credential is added to Context's required variables, it has to + be withheld by name too.""" + + scrubber = initialise_with({})["event_scrubber"] + + secret_vars = [ + name + for name in Context._REQUIRED_ENV_VARS + if any(fragment in name.upper() for fragment in SECRET_NAME_FRAGMENTS) + ] + + assert secret_vars, "the name-fragment backstop matched nothing, so this test proves nothing" + + for name in secret_vars: + assert name.lower() in scrubber.denylist, f"{name} looks like a secret, but Sentry would not scrub it" + + +def test_sentry_is_given_the_request_url_hooks() -> None: + + init_args = initialise_with({}) + + assert init_args["before_breadcrumb"] is before_breadcrumb + + assert init_args["before_send_transaction"] is before_send_transaction + + +def test_query_strings_are_withheld_from_request_breadcrumbs() -> None: + """Sentry records query strings with their values, and this application queries + SuiteCRM by building record IDs into the query string.""" + + crumb = before_breadcrumb( + { + "type": "http", + "data": { + "method": "GET", + "url": "https://dev.suitecrm.example.org/Api/V8/module/Contacts", + "http.query": "filter[id][eq]=698e0c1f-4e80-faa9-6533-68de801d1735", + "http.fragment": "somewhere", + "status_code": 403, + }, + }, + {}, + ) + + assert crumb is not None + + assert crumb["data"]["http.query"] == WITHHELD + assert crumb["data"]["http.fragment"] == WITHHELD + + # what is left has to be enough to say which request failed + assert crumb["data"]["method"] == "GET" + assert crumb["data"]["url"] == "https://dev.suitecrm.example.org/Api/V8/module/Contacts" + assert crumb["data"]["status_code"] == 403 + + +def test_breadcrumbs_without_request_data_are_left_alone() -> None: + + assert before_breadcrumb({"type": "log", "message": "a message"}, {}) == { + "type": "log", + "message": "a message", + } + + assert before_breadcrumb({"type": "http", "data": None}, {}) == {"type": "http", "data": None} + + +def test_query_strings_are_withheld_from_transaction_spans() -> None: + + # typed loosely because the SDK's Event marks every key as optional, so reading one + # back is a type error even where the test has just supplied it + transaction: Any = { + "type": "transaction", + "spans": [ + {"op": "http.client", "data": {"url": "https://example.com/x", "http.query": "filter[id][eq]=abc"}}, + {"op": "db", "data": {"db.system": "postgresql"}}, + ], + } + + event: Any = before_send_transaction(transaction, {}) + + assert event is not None + + assert event["spans"][0]["data"]["http.query"] == WITHHELD + assert event["spans"][0]["data"]["url"] == "https://example.com/x" + assert event["spans"][1]["data"] == {"db.system": "postgresql"} + + +def test_transactions_whose_spans_sentry_has_trimmed_are_left_alone() -> None: + """Sentry replaces a value it has trimmed with a marker object, so the spans are not + necessarily a list. The hook must not be the thing which raises.""" + + trimmed: Any = {"type": "transaction", "spans": object()} + + assert before_send_transaction(trimmed, {}) is not None + + +def test_the_audit_log_is_ignored_for_sentry_logs_as_well_as_for_events() -> None: + """The SDK keeps two separate ignore lists, and `ignore_logger` covers only events and + breadcrumbs — its own docstring says it "does not affect Sentry Logs". Sentry Logs are + off today, so this changes nothing now; it exists so that enabling them later cannot + silently start sending the audit log, whose records carry the Authorization header.""" + + with ( + mock.patch("register_your_data_api.sentry.sentry_sdk"), + mock.patch("register_your_data_api.sentry.ignore_logger") as ignore_for_events, + mock.patch("register_your_data_api.sentry.ignore_logger_for_sentry_logs") as ignore_for_logs, + ): + setup_sentry(BASE_CONFIG) + + ignore_for_events.assert_called_once_with(AUDIT_LOGGER_NAME) + + ignore_for_logs.assert_called_once_with(AUDIT_LOGGER_NAME) + + +def test_sentry_does_not_send_request_bodies() -> None: + """`send_default_pii` does not cover request bodies: the SDK collects them regardless + of it, bounded by `max_request_body_size`. Because tracing is enabled the body also + rides out on the transaction event for SUCCESSFUL requests, so without this a POST to + a reporting-org endpoint would transmit the contact email of every organisation + created.""" + + assert initialise_with({})["max_request_body_size"] == "never" + + +def test_a_malformed_dsn_disables_reporting_rather_than_raising() -> None: + """setup_sentry runs at import time in src/main.py, so a DSN which sentry_sdk rejects + would otherwise stop the API from starting, before there is a logger to report it + to.""" + + for malformed in ("https://sentry.example.org", "not-a-url", "https://key@host"): + assert setup_sentry(BASE_CONFIG | {"SENTRY_DSN": malformed}) is False + + +def test_the_test_session_cannot_report_to_a_real_sentry_project() -> None: + """The suite imports src/main.py through tests/helpers/mocking.py, which calls + setup_sentry() at import time, and a developer's .env holds a working DSN. + tests/conftest.py empties SENTRY_DSN so that reading the environment disables Sentry; + this fails if that line is removed.""" + + assert os.environ.get("SENTRY_DSN") == "", "tests/conftest.py should have emptied SENTRY_DSN" + + assert setup_sentry() is False + + +class CapturingTransport(Transport): + """Captures the envelopes the SDK would have transmitted, so that what would have left + the machine can be inspected. Used in place of Sentry's own HTTP transport, so + nothing is sent and no socket is opened.""" + + def __init__(self) -> None: + super().__init__() + self.envelopes: list[Any] = [] + + def capture_envelope(self, envelope: Any) -> None: + self.envelopes.append(envelope) + + @property + def transmitted(self) -> str: + """What the SDK serialised for transmission, which is what has to be free of + credentials.""" + + return "\n".join( + item.get_bytes().decode("utf-8", "replace") for envelope in self.envelopes for item in envelope.items + ) + + +def initialise_sentry_with_captured_transport( + transport: CapturingTransport, config_overrides: dict[str, str] | None = None +) -> None: + """Runs the application's own initialisation, substituting only the transport, so that + the options under test are the ones the application really uses.""" + + real_init = sentry_sdk.init + + with mock.patch( + "register_your_data_api.sentry.sentry_sdk.init", + lambda **kwargs: real_init(**{**kwargs, "transport": transport}), + ): + setup_sentry(BASE_CONFIG | {"SENTRY_DSN": "https://examplekey@example.invalid/1"} | (config_overrides or {})) + + +AUDIT_CANARY = "CANARY-audit-credential-8c1d4a" # nosec B105 + + +def test_audit_log_records_are_not_sent_to_sentry() -> None: + """The audit log must not be duplicated to Sentry. Sentry turns any record of ERROR + or above into an event, and the audit log is written at CRITICAL for authentication + failures, where the record carries the contents of the Authorization header including + the credential itself. The audit log has its own encrypted destination.""" + + transport = CapturingTransport() + initialise_sentry_with_captured_transport(transport) + + try: + # the shape of what auth.authn logs when the authorisation scheme is not bearer + logging.getLogger(AUDIT_LOGGER_NAME).critical( + "Received request with malformed authorisation HTTP header. " + f"SCHEME=Token PARAM={AUDIT_CANARY}. METHOD=GET" + ) + sentry_sdk.flush() + finally: + sentry_sdk.get_client().close() + + assert AUDIT_CANARY not in transport.transmitted + + assert not transport.envelopes, "the audit record produced an envelope, so it was not ignored" + + +BODY_CANARY = "CANARY-contact-email-4b7e2f@example.org" + + +def test_a_request_body_is_not_transmitted_even_when_the_request_succeeds() -> None: + """The behavioural counterpart to test_sentry_does_not_send_request_bodies. + + Tracing is enabled, so a successful request produces a transaction event, and the SDK + attaches the parsed request body to it. A POST to a reporting-org endpoint carries a + contact email, so this covers the successful path as well as the failing one.""" + + transport = CapturingTransport() + # every transaction must be sampled, or this test would pass whenever the sample rate + # in BASE_CONFIG happened to discard the one transaction it depends on + initialise_sentry_with_captured_transport(transport, {"SENTRY_TRACES_SAMPLE_RATE": "1.0"}) + + probe = fastapi.FastAPI() + + @probe.post("/organisations") + def _create() -> dict[str, str]: + return {"status": "created"} + + try: + with TestClient(probe) as client: + response = client.post("/organisations", json={"contact_email": BODY_CANARY}) + assert response.status_code == 200, "the request must succeed, or this tests the wrong path" + sentry_sdk.flush() + finally: + sentry_sdk.get_client().close() + + assert transport.envelopes, "nothing was sent, so this test would pass for the wrong reason" + + assert "transaction" in transport.transmitted, "no transaction was sent, so the body had no carrier" + + assert BODY_CANARY not in transport.transmitted + + +def test_application_log_errors_are_still_sent_to_sentry() -> None: + """Ignoring the audit logger must not silence the diagnostic log, which is how + application code reports errors without referencing the SDK.""" + + transport = CapturingTransport() + initialise_sentry_with_captured_transport(transport) + + try: + logging.getLogger("ryd-api").error("a diagnostic error worth reporting") + sentry_sdk.flush() + finally: + sentry_sdk.get_client().close() + + assert "a diagnostic error worth reporting" in transport.transmitted + + +def test_the_ignored_logger_is_the_one_the_context_actually_creates() -> None: + """AUDIT_LOGGER_NAME is duplicated from util.Context, so a rename there would silently + start sending audit records to Sentry.""" + + context = Context(logs_to_stdout=True) + # logs_to_stdout means no log files are opened; the key path is still read, and the + # formatter that would consume it is stood in for + context._env = {"APP_LOG_LEVEL": "DEBUG", "AUDIT_LOG_PUBLIC_KEY_PATH": "not-read"} + with ( + mock.patch("register_your_data_api.util.EncryptedFormatter"), + mock.patch("builtins.open", mock.mock_open(read_data=b"")), + ): + context._setup_loggers() + + assert context.audit_logger.name == AUDIT_LOGGER_NAME + + +BEARER_TOKEN_CANARY = "CANARY-bearer-token-3f9c1e7a5b2d" # nosec B105 + + +def test_a_failing_request_does_not_send_the_bearer_token() -> None: + """The regression test for the whole arrangement. Sentry would otherwise attach the + local variables of every stack frame, and an ASGI frame's locals hold the request + object, whose headers are a list of (bytes, bytes) tuples that the scrubber cannot + match by name. + + This drives the real SDK and a real Starlette request rather than stand-ins, so it + keeps testing the real behaviour if either changes how it holds the headers.""" + + transport = CapturingTransport() + + real_init = sentry_sdk.init + + with mock.patch( + "register_your_data_api.sentry.sentry_sdk.init", + lambda **kwargs: real_init(**kwargs, transport=transport), + ): + setup_sentry(BASE_CONFIG | {"SENTRY_DSN": "https://examplekey@example.invalid/1"}) + + try: + # what a request handler does: the token is a local variable, and the request + # object holding the raw header is a local of the frames above it + def handler_frame(authorization: str) -> None: + scope = {"type": "http", "headers": [(b"authorization", authorization.encode())]} + raise RuntimeError(f"failed while handling request with scope containing {len(scope)} keys") + + try: + handler_frame(f"Bearer {BEARER_TOKEN_CANARY}") + except Exception: + sentry_sdk.capture_exception() + + sentry_sdk.flush() + finally: + sentry_sdk.get_client().close() + + assert transport.envelopes, "the SDK sent nothing, so this test would pass for the wrong reason" + + # confirms the failure which was reported is the one this test is about + assert "RuntimeError" in transport.transmitted, "the exception was not reported, so nothing was proved" + + assert BEARER_TOKEN_CANARY not in transport.transmitted From 45dc1ed756884b7415e1e21b60d41cb8c4a2613b Mon Sep 17 00:00:00 2001 From: arobson-ods Date: Mon, 14 Sep 2026 15:39:59 +0100 Subject: [PATCH 4/4] fix: stop withholding outgoing query strings from sentry --- CHANGELOG.md | 7 ++- README.md | 14 ++--- src/register_your_data_api/sentry.py | 69 +----------------------- tests/unit/test_sentry.py | 81 ---------------------------- 4 files changed, 9 insertions(+), 162 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afae81c..432c0f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 FastAPI application is created. Configured by the optional `SENTRY_DSN`, `SENTRY_ENVIRONMENT`, and `SENTRY_TRACES_SAMPLE_RATE` environment variables; when no DSN is set the SDK is not initialised and the application runs unchanged, and neither is - it when the DSN is malformed or when running under pytest. Stack frame local variables, - request bodies, and outgoing request query strings are deliberately not sent: the local - variables and the bodies would otherwise transmit bearer tokens and contact details, and - the query strings carry the record IDs a request touched. The audit log is excluded + it when the DSN is malformed or when running under pytest. Stack frame local variables + and request bodies are deliberately not sent, as they would otherwise transmit bearer + tokens and contact details. The audit log is excluded entirely, since its records are written at `CRITICAL` and carry the contents of the `Authorization` header, and a failed startup is now logged rather than printed so that it is reported as well. diff --git a/README.md b/README.md index 23a1767..02b44a9 100644 --- a/README.md +++ b/README.md @@ -86,8 +86,11 @@ suggests more than it delivers: * It does not keep request **headers** out of events. The SDK substitutes the sensitive ones and passes the rest through, so `host`, `user-agent` and `content-type` are sent. - The `Authorization` header is withheld by the scrubber's `authorization` denylist entry, - not by this option. + The `Authorization` header *is* withheld by this option, though — `_filter_headers` in + the SDK returns every header untouched when PII is on, and substitutes its + `SENSITIVE_HEADERS` only when it is off. The `EventScrubber` is not what protects the + bearer token, so trimming its denylist would not expose the token — but turning + `send_default_pii` on would. * It does not keep request **bodies** out of events — the SDK collects those regardless of it, bounded by `max_request_body_size`. Because tracing is enabled the body rides out on the transaction event for **successful** requests too, so a `POST` creating a @@ -99,13 +102,6 @@ Deciding that a variable holds a credential is still a manual step: is a backstop that catches the common cases by name fragment, but a credential with an unremarkable name would satisfy it. -Sentry also records the query string of every outgoing HTTP request, with the values -intact. This application queries SuiteCRM by building record filters into the query -string, so those values carry the IDs of the people and organisations a request touched; -`before_breadcrumb` and `before_send_transaction` withhold them. The method, the URL -without its query string and the response status are kept, so a breadcrumb still says -which request was being made. - **The audit log is never sent to Sentry.** Sentry turns any log record of `ERROR` or above into an event, and the audit log is written at `CRITICAL` for authentication failures — where `auth/authn.py` records the contents of the `Authorization` header, diff --git a/src/register_your_data_api/sentry.py b/src/register_your_data_api/sentry.py index b4728e5..45186ff 100644 --- a/src/register_your_data_api/sentry.py +++ b/src/register_your_data_api/sentry.py @@ -12,13 +12,12 @@ import importlib.metadata import os -from typing import Any, Final +from typing import Final import dotenv import sentry_sdk from sentry_sdk.integrations.logging import ignore_logger, ignore_logger_for_sentry_logs from sentry_sdk.scrubber import DEFAULT_DENYLIST, EventScrubber -from sentry_sdk.types import Breadcrumb, BreadcrumbHint, Event, Hint # Sentry's SDK sends no traces at all unless a sample rate is set, so a default is # supplied here rather than leaving it to the SDK @@ -36,26 +35,6 @@ # 'production', so that an unconfigured environment can never be mistaken for the live one UNCONFIGURED_ENVIRONMENT: Final[str] = "local-development" -# What Sentry itself puts in place of a value it withholds -WITHHELD: Final[str] = "[Filtered]" - -# The parts of a request's URL which Sentry records with their values intact, and which -# can therefore carry a credential or an identifier. -# -# Both of the SDK's naming schemes are listed. Which one is used depends on whether span -# streaming is enabled: the SDK's http.client instrumentation records 'http.query' and -# 'http.fragment' without regard to send_default_pii, and 'url.query' and 'url.fragment' -# only when send_default_pii is set (sentry_sdk/integrations/stdlib.py). This application -# currently takes the former path, which is the one that needs withholding; the latter is -# listed so that a future SDK release which changes the default cannot quietly stop these -# from matching. -REQUEST_URL_PARTS_TO_WITHHOLD: Final[tuple[str, ...]] = ( - "http.query", - "http.fragment", - "url.query", - "url.fragment", -) - # Names of this application's own secrets and sensitive fields, withheld by name in # addition to Sentry's own default denylist. The audit log is deliberately encrypted at # rest, so audit content must not be shipped to a third party in plain text either. @@ -172,8 +151,6 @@ def initialise_sdk(dsn: str, environment: str, traces_sample_rate: float) -> Non denylist=DEFAULT_DENYLIST + SENSITIVE_NAMES, recursive=True, ), - before_breadcrumb=before_breadcrumb, - before_send_transaction=before_send_transaction, # the server is stopped by interrupting it, which is a normal way for the process # to end rather than a fault worth reporting ignore_errors=[KeyboardInterrupt], @@ -182,50 +159,6 @@ def initialise_sdk(dsn: str, environment: str, traces_sample_rate: float) -> Non ) -def before_breadcrumb(crumb: Breadcrumb, _hint: BreadcrumbHint) -> Breadcrumb | None: - """Withholds the query string of the outgoing HTTP requests Sentry records as breadcrumbs. - - Sentry records each request's query string with its values intact. This application - queries SuiteCRM by building filters into the query string, so those values carry the - IDs of the people and organisations a request touched.""" - - withhold_request_url_parts(crumb.get("data")) - - return crumb - - -def before_send_transaction(event: Event, _hint: Hint) -> Event | None: - """Withholds the same query strings from the spans of a transaction. - - Tracing is enabled, so requests to SuiteCRM and the FGA database are sent as spans of - the transaction for the request that made them, and `before_send` is not given - transactions and so cannot cover this.""" - - spans = event.get("spans") - - # Sentry replaces a value it has trimmed with a marker object, so the spans are not - # necessarily a list, and this must not be the thing which raises - if isinstance(spans, list): - for span in spans: - withhold_request_url_parts(span.get("data")) - - return event - - -def withhold_request_url_parts(data: Any) -> None: - """Replaces the parts of a recorded request URL which can carry a credential or an ID. - - The method, the URL without its query string, and the response status are left alone, - so a breadcrumb still says which request was being made.""" - - if not isinstance(data, dict): - return - - for part in REQUEST_URL_PARTS_TO_WITHHOLD: - if part in data: - data[part] = WITHHELD - - def get_traces_sample_rate(config: dict[str, str]) -> float: """Returns the configured trace sample rate, falling back to the default if it is unset or unusable. A bad value here must not stop the app from starting, nor stop diff --git a/tests/unit/test_sentry.py b/tests/unit/test_sentry.py index b1d0a34..f46ba66 100644 --- a/tests/unit/test_sentry.py +++ b/tests/unit/test_sentry.py @@ -18,9 +18,6 @@ AUDIT_LOGGER_NAME, DEFAULT_TRACES_SAMPLE_RATE, UNCONFIGURED_ENVIRONMENT, - WITHHELD, - before_breadcrumb, - before_send_transaction, setup_sentry, ) from register_your_data_api.util import Context @@ -146,84 +143,6 @@ def test_sentry_scrubs_every_configuration_variable_which_looks_like_a_secret() assert name.lower() in scrubber.denylist, f"{name} looks like a secret, but Sentry would not scrub it" -def test_sentry_is_given_the_request_url_hooks() -> None: - - init_args = initialise_with({}) - - assert init_args["before_breadcrumb"] is before_breadcrumb - - assert init_args["before_send_transaction"] is before_send_transaction - - -def test_query_strings_are_withheld_from_request_breadcrumbs() -> None: - """Sentry records query strings with their values, and this application queries - SuiteCRM by building record IDs into the query string.""" - - crumb = before_breadcrumb( - { - "type": "http", - "data": { - "method": "GET", - "url": "https://dev.suitecrm.example.org/Api/V8/module/Contacts", - "http.query": "filter[id][eq]=698e0c1f-4e80-faa9-6533-68de801d1735", - "http.fragment": "somewhere", - "status_code": 403, - }, - }, - {}, - ) - - assert crumb is not None - - assert crumb["data"]["http.query"] == WITHHELD - assert crumb["data"]["http.fragment"] == WITHHELD - - # what is left has to be enough to say which request failed - assert crumb["data"]["method"] == "GET" - assert crumb["data"]["url"] == "https://dev.suitecrm.example.org/Api/V8/module/Contacts" - assert crumb["data"]["status_code"] == 403 - - -def test_breadcrumbs_without_request_data_are_left_alone() -> None: - - assert before_breadcrumb({"type": "log", "message": "a message"}, {}) == { - "type": "log", - "message": "a message", - } - - assert before_breadcrumb({"type": "http", "data": None}, {}) == {"type": "http", "data": None} - - -def test_query_strings_are_withheld_from_transaction_spans() -> None: - - # typed loosely because the SDK's Event marks every key as optional, so reading one - # back is a type error even where the test has just supplied it - transaction: Any = { - "type": "transaction", - "spans": [ - {"op": "http.client", "data": {"url": "https://example.com/x", "http.query": "filter[id][eq]=abc"}}, - {"op": "db", "data": {"db.system": "postgresql"}}, - ], - } - - event: Any = before_send_transaction(transaction, {}) - - assert event is not None - - assert event["spans"][0]["data"]["http.query"] == WITHHELD - assert event["spans"][0]["data"]["url"] == "https://example.com/x" - assert event["spans"][1]["data"] == {"db.system": "postgresql"} - - -def test_transactions_whose_spans_sentry_has_trimmed_are_left_alone() -> None: - """Sentry replaces a value it has trimmed with a marker object, so the spans are not - necessarily a list. The hook must not be the thing which raises.""" - - trimmed: Any = {"type": "transaction", "spans": object()} - - assert before_send_transaction(trimmed, {}) is not None - - def test_the_audit_log_is_ignored_for_sentry_logs_as_well_as_for_events() -> None: """The SDK keeps two separate ignore lists, and `ignore_logger` covers only events and breadcrumbs — its own docstring says it "does not affect Sentry Logs". Sentry Logs are