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/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index bad0438..432c0f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ 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 + 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. + ### Changed ### Deprecated diff --git a/README.md b/README.md index 0487f7d..02b44a9 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,76 @@ 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 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 + 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. + +**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 @@ -123,7 +192,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 +201,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 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..45186ff --- /dev/null +++ b/src/register_your_data_api/sentry.py @@ -0,0 +1,188 @@ +"""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 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 + +# 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" + +# 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, + ), + # 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 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..f46ba66 --- /dev/null +++ b/tests/unit/test_sentry.py @@ -0,0 +1,370 @@ +"""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, + 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_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